Re: [PATCH 5/8] mm: Factor out the pdx compression logic in ma/va converters

[Julien Grall <julien@xxxxxxx>]

Hi Alejandro,

On 17/07/2023 17:03, Alejandro Vallejo wrote:
> This patch factors out the pdx compression logic hardcoded in both ports
> for the maddr<->vaddr conversion functions.
>
> Touches both x86 and arm ports.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>   xen/arch/arm/include/asm/mm.h          |  3 +--
>   xen/arch/x86/include/asm/x86_64/page.h | 28 +++++++++++---------------
>   xen/include/xen/pdx.h                  | 25 +++++++++++++++++++++++
>   3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 1a83f41879..78cb23858a 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -320,8 +320,7 @@ static inline void *maddr_to_virt(paddr_t ma)
>              (DIRECTMAP_SIZE >> PAGE_SHIFT));
>       return (void *)(XENHEAP_VIRT_START -
>                       (directmap_base_pdx << PAGE_SHIFT) +
> -                    ((ma & ma_va_bottom_mask) |
> -                     ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
> +                    maddr_to_directmapoff(ma));
>   }
>   #endif
>   
> diff --git a/xen/arch/x86/include/asm/x86_64/page.h b/xen/arch/x86/include/asm/x86_64/page.h
> index 53faa7875b..b589c93e77 100644
> --- a/xen/arch/x86/include/asm/x86_64/page.h
> +++ b/xen/arch/x86/include/asm/x86_64/page.h
> @@ -36,26 +36,22 @@ static inline unsigned long __virt_to_maddr(unsigned long 
> va)
>   {
>       ASSERT(va < DIRECTMAP_VIRT_END);
>       if ( va >= DIRECTMAP_VIRT_START )
> -        va -= DIRECTMAP_VIRT_START;
> -    else
> -    {
> -        BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1));
> -        /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */
> -        ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) ==
> -               ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT)));
> -
> -        va += xen_phys_start - XEN_VIRT_START;
> -    }
> -    return (va & ma_va_bottom_mask) |
> -           ((va << pfn_pdx_hole_shift) & ma_top_mask);
> +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> +
> +    BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1));
> +    /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */
> +    ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) ==
> +           ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT)));
> +
> +    return xen_phys_start + va - XEN_VIRT_START;
>   }
>   
>   static inline void *__maddr_to_virt(unsigned long ma)
>   {
> -    ASSERT(pfn_to_pdx(ma >> PAGE_SHIFT) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> -    return (void *)(DIRECTMAP_VIRT_START +
> -                    ((ma & ma_va_bottom_mask) |
> -                     ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
> +    /* Offset in the direct map, accounting for pdx compression */
> +    size_t va_offset = maddr_to_directmapoff(ma);
> +    ASSERT(va_offset < DIRECTMAP_SIZE);
> +    return (void *)(DIRECTMAP_VIRT_START + va_offset);
>   }
>   
>   /* read access (should only be used for debug printk's) */
> diff --git a/xen/include/xen/pdx.h b/xen/include/xen/pdx.h
> index 67ae20e89c..f8ca0f5821 100644
> --- a/xen/include/xen/pdx.h
> +++ b/xen/include/xen/pdx.h
> @@ -158,6 +158,31 @@ static inline unsigned long pdx_to_pfn(unsigned long pdx)
>   #define mfn_to_pdx(mfn) pfn_to_pdx(mfn_x(mfn))
>   #define pdx_to_mfn(pdx) _mfn(pdx_to_pfn(pdx))
>   
> +/**
> + * Computes the offset into the direct map of an maddr
> + *
> + * @param ma Machine address
> + * @return Offset on the direct map where that
> + *         machine address can be accessed
> + */
> +static inline unsigned long maddr_to_directmapoff(uint64_t ma)
> +{
> +    return ((ma & ma_top_mask) >> pfn_pdx_hole_shift) |
> +           (ma & ma_va_bottom_mask);
NIT: I got a bit confused because your re-order the two operations. I 
guess this was done because it is nicer to read.
Anyway, I have confirmed the logic is still the same (just different 
ordering).
> +}
> +
> +/**
> + * Computes a machine address given a direct map offset
> + *
> + * @param offset Offset into the direct map
> + * @return Corresponding machine address of that virtual location
> + */
> +static inline uint64_t directmapoff_to_maddr(unsigned long offset)
> +{
> +    return ((offset << pfn_pdx_hole_shift) & ma_top_mask) |
'unsigned long' may be 32-bit. So I think you want to cast offset to 
uint64_t.
Cheers,

--
Julien Grall