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

[Julien Grall <julien.grall@arm.com>]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel