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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-11-25 at 16:49 +0100, Jan Beulich wrote:
> 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.
Agree, overlooked that DIRECTMAP_SIZE is exclusive,  the value should
correspond to the table thereby DIRECTMAP_SIZE should be inclusive and
defined as:
#define DIRECTMAP_SIZE (SLOTN(DIRECTMAP_SLOT_END + 1) -
SLOTN(DIRECTMAP_SLOT_START))

and then DIRECTMAP_VIRT_END could be left as it is defined now:
   #define DIRECTMAP_VIRT_END      (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE
   - 1)

Regarding the first one point. Do you think it would be better to
follow x86 approach and have *_END to be exclusive? Then
FRAMETABLE_VIRT_END should be updated too?

> 
> > @@ -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".)

Just to be sure that I understand your point correct. Do you mean the
following:
    static inline void *maddr_to_virt(paddr_t ma)
    {
   -    unsigned long va_offset = maddr_to_directmapoff(ma);
   +    unsigned long va = maddr_to_directmapoff(ma) +
   directmap_virt_start;
    
   -    ASSERT(va_offset >= DIRECTMAP_VIRT_START -
   directmap_virt_start);
   -    ASSERT(va_offset <= DIRECTMAP_VIRT_END - directmap_virt_start);
   +    ASSERT(va >= DIRECTMAP_VIRT_START);
   +    ASSERT(va <= DIRECTMAP_VIRT_END);
    
   -    return (void *)(directmap_virt_start + va_offset);
   +    return (void *)va;
    }

Thanks.

~ Oleksii

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