Re: [PATCH v6 1/3] xen/riscv: introduce setup_mm()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2024 13:47, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -90,6 +90,7 @@
>  #define DIRECTMAP_SLOT_START    200
>  #define DIRECTMAP_VIRT_START    SLOTN(DIRECTMAP_SLOT_START)
>  #define DIRECTMAP_SIZE          (SLOTN(DIRECTMAP_SLOT_END) - 
> SLOTN(DIRECTMAP_SLOT_START))
> +#define DIRECTMAP_VIRT_END      (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)

While it is of course okay to have this value be inclusive (matching
FRAMETABLE_VIRT_END), I'd like to point out that
- on x86 *_END are exclusive (i.e. there's some risk of confusion),
- RISC-V's DIRECTMAP_SIZE appears to assume DIRECTMAP_SLOT_END is
  exclusive, when from all I can tell (in particular the table in the
  earlier comment) it's inclusive.

> @@ -25,8 +27,12 @@
>  
>  static inline void *maddr_to_virt(paddr_t ma)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    unsigned long va_offset = maddr_to_directmapoff(ma);
> +
> +    ASSERT(va_offset >= DIRECTMAP_VIRT_START - directmap_virt_start);
> +    ASSERT(va_offset <= DIRECTMAP_VIRT_END - directmap_virt_start);
> +
> +    return (void *)(directmap_virt_start + va_offset);
>  }

If you added in directmap_virt_start right when setting the variable,
you'd simplify the assertions. The unsigned long arithmetic is going to
be okay either way. (The variable may want renaming if doing so, perhaps
simply to "va".)

Preferably with the latter adjustment and pending clarification on the
intentions wrt the comment further up
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan