Re: [PATCH v2 1/3] xen/riscv: implement virt_to_maddr()

[oleksii.kurochko@xxxxxxxxx]

On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote:
> Implement the virt_to_maddr() function to convert virtual addresses
> to machine addresses, including checks for address ranges such as
> the direct mapping area (DIRECTMAP_VIRT_START) and the Xen virtual
> address space. To implement this, the phys_offset variable is made
> accessible outside of riscv/mm.c.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V2:
>   - Drop casts in virt_to_maddr() for ASSERT which checks that VA is
>     in the range of where Xen is located.
>   - Add UL suffix for or XEN_VIRT_START by using _AC(..., UL) and add
> inclusion
>     of <xen/const.h>
>   - Add the comment above return which explains why there is no need
>     to do " - XEN_VIRT_START.
> ---
>  xen/arch/riscv/include/asm/config.h |  4 ++++
>  xen/arch/riscv/include/asm/mm.h     | 17 ++++++++++++++++-
>  xen/arch/riscv/mm.c                 |  2 +-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/riscv/include/asm/config.h
> b/xen/arch/riscv/include/asm/config.h
> index 7dbb235685..8884aeab16 100644
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -155,6 +155,10 @@
>  
>  #define IDENT_AREA_SIZE 64
>  
> +#ifndef __ASSEMBLY__
> +extern unsigned long phys_offset;
> +#endif
> +
>  #endif /* __RISCV_CONFIG_H__ */
>  /*
>   * Local variables:
> diff --git a/xen/arch/riscv/include/asm/mm.h
> b/xen/arch/riscv/include/asm/mm.h
> index 4b7b00b850..0f7879d685 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -5,6 +5,7 @@
>  
>  #include <public/xen.h>
>  #include <xen/bug.h>
> +#include <xen/const.h>
>  #include <xen/mm-frame.h>
>  #include <xen/pdx.h>
>  #include <xen/types.h>
> @@ -28,7 +29,21 @@ static inline void *maddr_to_virt(paddr_t ma)
>      return NULL;
>  }
>  
> -#define virt_to_maddr(va) ({ BUG_ON("unimplemented"); 0; })
> +static inline unsigned long virt_to_maddr(unsigned long va)
> +{
> +    ASSERT(va >= (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE));
It should be ASSERT(va < (...)).

Then I can't use virt_to_maddr() instead of LINK_TO_LOAD() as 
address from Xen's VA space ( XEN_VIRT_START ) are higher then
(DIRECTMAP_VIRT_START + DIRECTMAP_SIZE).

Or as an option we could consider to drop this ASSERT() as if
VA is from directmap range the if below will catch that; otherwise
we have another one ASSERT() which checks that VA is from Xen VA range
where it is sage to use (phys_offset + va).

Could we consider just dropping "ASSERT(va < (DIRECTMAP_VIRT_START +
DIRECTMAP_SIZE))" or I am missing something?

~ Oleksii


> +    if ((va >= DIRECTMAP_VIRT_START) &&
> +        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
> +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> +
> +    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
> +    ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
> +           (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER +
> PAGE_SHIFT)));
> +
> +    /* phys_offset = load_start - XEN_VIRT_START */
> +    return phys_offset + va;
> +}
> +#define virt_to_maddr(va) virt_to_maddr((unsigned long)(va))
>  
>  /* Convert between Xen-heap virtual addresses and machine frame
> numbers. */
>  #define __virt_to_mfn(va)  mfn_x(maddr_to_mfn(virt_to_maddr(va)))
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 4a628aef83..7a1919e07e 100644
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -26,7 +26,7 @@ struct mmu_desc {
>      pte_t *pgtbl_base;
>  };
>  
> -static unsigned long __ro_after_init phys_offset;
> +unsigned long __ro_after_init phys_offset;
>  
>  #define LOAD_TO_LINK(addr) ((unsigned long)(addr) - phys_offset)
>  #define LINK_TO_LOAD(addr) ((unsigned long)(addr) + phys_offset)