Re: [Xen-devel] [PATCH for-next 1/9] xen/arm: Use mfn_to_pdx instead of pfn_to_pdx when possible

[Julien Grall <julien.grall@xxxxxxx>]

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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel