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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-10-01 at 17:41 +0200, Jan Beulich wrote:
> On 30.09.2024 17:08, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/mm.h
> > +++ b/xen/arch/riscv/include/asm/mm.h
> > @@ -28,7 +28,20 @@ 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));
> > +    if ((va >= DIRECTMAP_VIRT_START) &&
> > +        (va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)))
> > +        return directmapoff_to_maddr(va - DIRECTMAP_VIRT_START);
> 
> While the cover letter states a dependency on another series, I'm
> unable
> to spot directmapoff_to_maddr() in the tree or in that other series.
The definition of directmap_off_to_maddr() is in xen/pdx.h:

#ifdef CONFIG_PDX_COMPRESSION
...
/**
 * 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 paddr_t directmapoff_to_maddr(unsigned long offset)
{
    return ((((paddr_t)offset << pfn_pdx_hole_shift) & ma_top_mask) |
            (offset & ma_va_bottom_mask));
}
...
#else /* !CONFIG_PDX_COMPRESSION */
...
/* directmap is indexed by by maddr */
#define maddr_to_directmapoff(x) (x)
#define directmapoff_to_maddr(x) (x)
...
#endif

> 
> > +    BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2));
> > +    ASSERT(((long)va >> (PAGETABLE_ORDER + PAGE_SHIFT)) ==
> > +           ((long)XEN_VIRT_START >> (PAGETABLE_ORDER +
> > PAGE_SHIFT)));
> 
> What's the point of the casts here? va is unsigned long and
> XEN_VIRT_START
> is a literal number (which probably ought to have a UL suffix).
I thought that it could be the same as for x86 that:
 /* Signed, so ((long)XEN_VIRT_START >> 30) fits in an imm32. */
But checking the generated code for RISC-V casts could be dropped
as the generated code is the same.

Regarding UL, I will update to _AC(XEN_VIRT_START, UL).

> 
> > +    return phys_offset + va;
> 
> Don't you need to subtract XEN_VIRT_START here?
It shouldn't as XEN_VIRT_START is taken into account during phys_offset
calculation ( as you mentioned in the reply ).

Regarding the name of phys_offset variable, could it be better to
rename it to load_offset?

As an option I can just add the comment above return:
 /* phys_offset = load_start - XEN_VIRT_START */

~ Oleksii