| Message ID | 20190218113600.9540-2-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm. | expand |
On Mon, 18 Feb 2019, Julien Grall wrote: > mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in > the Arm code to use the former. This is good but maybe we can go even further. You should also be able to replace one call site of pfn_to_pdx in mfn_valid and the one in maddr_to_virt. Something like this: diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index eafa26f..b3455ea 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len) /* XXX -- account for base */ #define mfn_valid(mfn) ({ \ unsigned long __m_f_n = mfn_x(mfn); \ - likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \ + likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \ }) /* Convert between machine frame numbers and page-info structures. */ @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) #else static inline void *maddr_to_virt(paddr_t ma) { - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); return (void *)(XENHEAP_VIRT_START - mfn_to_maddr(xenheap_mfn_start) + ((ma & ma_va_bottom_mask) | > No functional changes. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/mm.c | 2 +- > xen/include/asm-arm/mm.h | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 01ae2cccc0..be5338bb4c 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -886,7 +886,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) > int i; > #endif > > - frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT); > + frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps)); > /* Round up to 2M or 32M boundary, as appropriate. */ > frametable_size = ROUNDUP(frametable_size, mapping_size); > base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12)); > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index eafa26f56e..7b6aaf5e3f 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -225,7 +225,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len) > /* Convert between frame number and address formats. */ > #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT) > #define paddr_to_pfn(pa) ((unsigned long)((pa) >> PAGE_SHIFT)) > -#define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa)) > +#define paddr_to_pdx(pa) mfn_to_pdx(maddr_to_mfn(pa)) > #define gfn_to_gaddr(gfn) pfn_to_paddr(gfn_x(gfn)) > #define gaddr_to_gfn(ga) _gfn(paddr_to_pfn(ga)) > #define mfn_to_maddr(mfn) pfn_to_paddr(mfn_x(mfn)) > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) > #else > static inline void *maddr_to_virt(paddr_t ma) > { > - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); > + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); > return (void *)(XENHEAP_VIRT_START - > mfn_to_maddr(xenheap_mfn_start) + > ((ma & ma_va_bottom_mask) | > @@ -301,7 +301,7 @@ static inline struct page_info *virt_to_page(const void *v) > ASSERT(va < xenheap_virt_end); > > pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT; > - pdx += pfn_to_pdx(mfn_x(xenheap_mfn_start)); > + pdx += mfn_to_pdx(xenheap_mfn_start); > return frame_table + pdx - frametable_base_pdx; > } > > -- > 2.11.0 >
Hi, On 4/15/19 10:55 PM, Stefano Stabellini wrote: > On Mon, 18 Feb 2019, Julien Grall wrote: >> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in >> the Arm code to use the former. > > This is good but maybe we can go even further. > > You should also be able to replace one call site of pfn_to_pdx in > mfn_valid and the one in maddr_to_virt. Something like this: > > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index eafa26f..b3455ea 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len) > /* XXX -- account for base */ > #define mfn_valid(mfn) ({ \ > unsigned long __m_f_n = mfn_x(mfn); \ > - likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \ > + likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); \ This is quite undesirable, you will end up to evaluate mfn twice here. The other solution is to turn _m_f_n to an mfn_t but then it does make much difference as you would need to use a mfn_x(mfn) in the code. > }) > > /* Convert between machine frame numbers and page-info structures. */ > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) > #else > static inline void *maddr_to_virt(paddr_t ma) > { > - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); > + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); > return (void *)(XENHEAP_VIRT_START - > mfn_to_maddr(xenheap_mfn_start) + > ((ma & ma_va_bottom_mask) | > I fail to see what this chunk adds compare to the existing one... >> @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) >> #else >> static inline void *maddr_to_virt(paddr_t ma) >> { >> - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); >> + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); >> return (void *)(XENHEAP_VIRT_START - >> mfn_to_maddr(xenheap_mfn_start) + >> ((ma & ma_va_bottom_mask) | ... here. Cheers,
On Mon, 15 Apr 2019, Julien Grall wrote: > Hi, > > On 4/15/19 10:55 PM, Stefano Stabellini wrote: > > On Mon, 18 Feb 2019, Julien Grall wrote: > > > mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in > > > the Arm code to use the former. > > > > This is good but maybe we can go even further. > > > > You should also be able to replace one call site of pfn_to_pdx in > > mfn_valid and the one in maddr_to_virt. Something like this: > > > > > > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > > index eafa26f..b3455ea 100644 > > --- a/xen/include/asm-arm/mm.h > > +++ b/xen/include/asm-arm/mm.h > > @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, > > size_t len) > > /* XXX -- account for base */ > > #define mfn_valid(mfn) ({ > > \ > > unsigned long __m_f_n = mfn_x(mfn); > > \ > > - likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && > > __mfn_valid(__m_f_n)); \ > > + likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); > > \ > > This is quite undesirable, you will end up to evaluate mfn twice here. Yes you are right > The other solution is to turn _m_f_n to an mfn_t but then it does make much > difference as you would need to use a mfn_x(mfn) in the code. I think that would be better > > }) > > /* Convert between machine frame numbers and page-info structures. */ > > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) > > #else > > static inline void *maddr_to_virt(paddr_t ma) > > { > > - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); > > + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); > > return (void *)(XENHEAP_VIRT_START - > > mfn_to_maddr(xenheap_mfn_start) + > > ((ma & ma_va_bottom_mask) | > > > > I fail to see what this chunk adds compare to the existing one... > > > > @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) > > > #else > > > static inline void *maddr_to_virt(paddr_t ma) > > > { > > > - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> > > > PAGE_SHIFT)); > > > + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> > > > PAGE_SHIFT)); > > > return (void *)(XENHEAP_VIRT_START - > > > mfn_to_maddr(xenheap_mfn_start) + > > > ((ma & ma_va_bottom_mask) | > ... here. That's weird. I think `patch' didn't apply completely the patch to my tree. Great you already have it.
Hi, On 4/15/19 11:25 PM, Stefano Stabellini wrote: > On Mon, 15 Apr 2019, Julien Grall wrote: >> Hi, >> >> On 4/15/19 10:55 PM, Stefano Stabellini wrote: >>> On Mon, 18 Feb 2019, Julien Grall wrote: >>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in >>>> the Arm code to use the former. >>> >>> This is good but maybe we can go even further. >>> >>> You should also be able to replace one call site of pfn_to_pdx in >>> mfn_valid and the one in maddr_to_virt. Something like this: >>> >>> >>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h >>> index eafa26f..b3455ea 100644 >>> --- a/xen/include/asm-arm/mm.h >>> +++ b/xen/include/asm-arm/mm.h >>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, >>> size_t len) >>> /* XXX -- account for base */ >>> #define mfn_valid(mfn) ({ >>> \ >>> unsigned long __m_f_n = mfn_x(mfn); >>> \ >>> - likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && >>> __mfn_valid(__m_f_n)); \ >>> + likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); >>> \ >> >> This is quite undesirable, you will end up to evaluate mfn twice here. > > Yes you are right > > >> The other solution is to turn _m_f_n to an mfn_t but then it does make much >> difference as you would need to use a mfn_x(mfn) in the code. > > I think that would be better I should have probably said that I haven't done that in the patch because adding the mfn_x(...) is breached the 80-characters limit. So we would need to split the line. I don't really think this will make easier to read the code in this context. I am curious to know how this would make it better. Cheers,
(+ Andrew and Jan) On 15/04/2019 23:42, Julien Grall wrote: > Hi, > > On 4/15/19 11:25 PM, Stefano Stabellini wrote: >> On Mon, 15 Apr 2019, Julien Grall wrote: >>> Hi, >>> >>> On 4/15/19 10:55 PM, Stefano Stabellini wrote: >>>> On Mon, 18 Feb 2019, Julien Grall wrote: >>>>> mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in >>>>> the Arm code to use the former. >>>> >>>> This is good but maybe we can go even further. >>>> >>>> You should also be able to replace one call site of pfn_to_pdx in >>>> mfn_valid and the one in maddr_to_virt. Something like this: >>>> >>>> >>>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h >>>> index eafa26f..b3455ea 100644 >>>> --- a/xen/include/asm-arm/mm.h >>>> +++ b/xen/include/asm-arm/mm.h >>>> @@ -209,7 +209,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, >>>> size_t len) >>>> /* XXX -- account for base */ >>>> #define mfn_valid(mfn) ({ >>>> \ >>>> unsigned long __m_f_n = mfn_x(mfn); >>>> \ >>>> - likely(pfn_to_pdx(__m_f_n) >= frametable_base_pdx && >>>> __mfn_valid(__m_f_n)); \ >>>> + likely(mfn_to_pdx(mfn) >= frametable_base_pdx && __mfn_valid(__m_f_n)); >>>> \ >>> >>> This is quite undesirable, you will end up to evaluate mfn twice here. I have been looking at making __mfn_valid(...) typesafe. While it compiles on Arm, I have a build error on x86: In file included from /home/julieng/works/xen/xen/include/asm/x86_64/page.h:47:0, from /home/julieng/works/xen/xen/include/asm/page.h:23, from /home/julieng/works/xen/xen/include/asm/current.h:12, from /home/julieng/works/xen/xen/include/asm/smp.h:10, from /home/julieng/works/xen/xen/include/xen/smp.h:4, from /home/julieng/works/xen/xen/include/xen/perfc.h:7, from x86_64/asm-offsets.c:8: /home/julieng/works/xen/xen/include/xen/pdx.h:24:18: error: unknown type name ‘mfn_t’ bool __mfn_valid(mfn_t mfn); ^~~~~ In file included from /home/julieng/works/xen/xen/include/xen/config.h:13:0, from <command-line>:0: /home/julieng/works/xen/xen/include/asm/mm.h: In function ‘get_page_from_mfn’: /home/julieng/works/xen/xen/include/asm/page.h:262:29: error: implicit declaration of function ‘__mfn_valid’ [-Werror=implicit-function-declaration] #define mfn_valid(mfn) __mfn_valid(mfn) ^ /home/julieng/works/xen/xen/include/xen/compiler.h:11:43: note: in definition of macro ‘unlikely’ #define unlikely(x) __builtin_expect(!!(x),0) We get away on Arm because mfn_valid is implemented in mm.h. On x86 it is implemented in page.h. I am quite impressed we never had build failure in common code until now... Anyway, it is not the first time I see build error when trying to make the code using typesafe gfn/mfn. The headers dependency are quite messy in general. Andrew, Jan do you have a suggestion how to process on the x86 side? Cheers,
>>> On 17.04.19 at 19:07, <julien.grall@arm.com> wrote: > Anyway, it is not the first time I see build error when trying to make the code using > typesafe gfn/mfn. The headers dependency are quite messy in general. Because of this, ... > Andrew, Jan do you have a suggestion how to process on the x86 side? ... I don't, I'm sorry, without being able to invest some time to play with this myself. Jan
Hi Jan, On 25/04/2019 12:20, Jan Beulich wrote: >>>> On 17.04.19 at 19:07, <julien.grall@arm.com> wrote: >> Anyway, it is not the first time I see build error when trying to make the code using >> typesafe gfn/mfn. The headers dependency are quite messy in general. > > Because of this, ... > >> Andrew, Jan do you have a suggestion how to process on the x86 side? > > ... I don't, I'm sorry, without being able to invest some time to > play with this myself. I thought I would CCed you just in case you have an idea to quickly fix it. Anyway, this is more a cleanup and not a priority for me. Cheers,
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 01ae2cccc0..be5338bb4c 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -886,7 +886,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe) int i; #endif - frametable_base_pdx = pfn_to_pdx(ps >> PAGE_SHIFT); + frametable_base_pdx = mfn_to_pdx(maddr_to_mfn(ps)); /* Round up to 2M or 32M boundary, as appropriate. */ frametable_size = ROUNDUP(frametable_size, mapping_size); base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 32<<(20-12)); diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index eafa26f56e..7b6aaf5e3f 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -225,7 +225,7 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len) /* Convert between frame number and address formats. */ #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT) #define paddr_to_pfn(pa) ((unsigned long)((pa) >> PAGE_SHIFT)) -#define paddr_to_pdx(pa) pfn_to_pdx(paddr_to_pfn(pa)) +#define paddr_to_pdx(pa) mfn_to_pdx(maddr_to_mfn(pa)) #define gfn_to_gaddr(gfn) pfn_to_paddr(gfn_x(gfn)) #define gaddr_to_gfn(ga) _gfn(paddr_to_pfn(ga)) #define mfn_to_maddr(mfn) pfn_to_paddr(mfn_x(mfn)) @@ -253,7 +253,7 @@ static inline void *maddr_to_virt(paddr_t ma) #else static inline void *maddr_to_virt(paddr_t ma) { - ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); + ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT)); return (void *)(XENHEAP_VIRT_START - mfn_to_maddr(xenheap_mfn_start) + ((ma & ma_va_bottom_mask) | @@ -301,7 +301,7 @@ static inline struct page_info *virt_to_page(const void *v) ASSERT(va < xenheap_virt_end); pdx = (va - XENHEAP_VIRT_START) >> PAGE_SHIFT; - pdx += pfn_to_pdx(mfn_x(xenheap_mfn_start)); + pdx += mfn_to_pdx(xenheap_mfn_start); return frame_table + pdx - frametable_base_pdx; }
mfn_to_pdx adds more safety than pfn_to_pdx. Replace all but on place in the Arm code to use the former. No functional changes. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/mm.c | 2 +- xen/include/asm-arm/mm.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)