Re: [Xen-devel] [PATCH] xen/arm64: Correctly compute the virtual address in maddr_to_virt()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 18 Jul 2019, Julien Grall wrote:
> The helper maddr_to_virt() is used to translate a machine address to a
> virtual address. To save some valuable address space, some part of the
> machine address may be compressed.
> 
> In theory the PDX code is free to compress any bits so there are no
> guarantee the machine index computed will be always greater than
> xenheap_mfn_start. This would result to return a virtual address that is
> not part of the direct map and trigger a crash at least on debug-build later
> on because of the check in virt_to_page().
> 
> A recently reverted patch (see 1191156361 "xen/arm: fix mask calculation
> in pdx_init_mask") allows the PDX to compress more bits and triggered a
> crash on AMD Seattle Platform.
> 
> Avoid the crash by keeping track of the base PDX for the xenheap and use
> it for computing the virtual address.
> 
> Note that virt_to_maddr() does not need to have similar modification as
> it is using the hardware to translate the virtual address to a machine
> address.
> 
> Take the opportunity to fix the ASSERT() as the direct map base address
> correspond to the start of the RAM (this is not always 0).
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Thank you very much for debugging and fixing this problem! It is
surprising that it has been working at all :-)


> ---
> 
> With that, the patch 1191156361 "xen/arm: fix mask calculation in
> pdx_init_mask" could be re-instated.
> ---
>  xen/arch/arm/mm.c        | 2 ++
>  xen/include/asm-arm/mm.h | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 44258ad89c..e1cdeaaf2f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -165,6 +165,7 @@ mfn_t xenheap_mfn_end __read_mostly;
>  vaddr_t xenheap_virt_end __read_mostly;
>  #ifdef CONFIG_ARM_64
>  vaddr_t xenheap_virt_start __read_mostly;
> +unsigned long xenheap_base_pdx __read_mostly;
>  #endif
>  
>  unsigned long frametable_base_pdx __read_mostly;
> @@ -796,6 +797,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>      if ( mfn_eq(xenheap_mfn_start, INVALID_MFN) )
>      {
>          xenheap_mfn_start = _mfn(base_mfn);
> +        xenheap_base_pdx = mfn_to_pdx(_mfn(base_mfn));
>          xenheap_virt_start = DIRECTMAP_VIRT_START +
>              (base_mfn - mfn) * PAGE_SIZE;
>      }

I can see that this would work, but wouldn't it be a better fit to set
xenheap_base_pdx in xen/arch/arm/setup.c:setup_mm, where we already set:


    xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
    xenheap_mfn_start = maddr_to_mfn(ram_start);
    xenheap_mfn_end = maddr_to_mfn(ram_end);

Or it too late by then?



> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 3dbc8a6469..d6b5544015 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -135,6 +135,7 @@ extern mfn_t xenheap_mfn_start, xenheap_mfn_end;
>  extern vaddr_t xenheap_virt_end;
>  #ifdef CONFIG_ARM_64
>  extern vaddr_t xenheap_virt_start;
> +extern unsigned long xenheap_base_pdx;
>  #endif
>  
>  #ifdef CONFIG_ARM_32
> @@ -253,9 +254,10 @@ static inline void *maddr_to_virt(paddr_t ma)
>  #else
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    ASSERT(mfn_to_pdx(maddr_to_mfn(ma)) < (DIRECTMAP_SIZE >> PAGE_SHIFT));
> +    ASSERT((mfn_to_pdx(maddr_to_mfn(ma)) - xenheap_base_pdx) <
> +           (DIRECTMAP_SIZE >> PAGE_SHIFT));
>      return (void *)(XENHEAP_VIRT_START -
> -                    mfn_to_maddr(xenheap_mfn_start) +
> +                    (xenheap_base_pdx << PAGE_SHIFT) +
>                      ((ma & ma_va_bottom_mask) |
>                       ((ma & ma_top_mask) >> pfn_pdx_hole_shift)));
>  }

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