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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-10-08 at 12:34 +0200, Jan Beulich wrote:
> On 08.10.2024 12:26, oleksii.kurochko@xxxxxxxxx wrote:
> > On Fri, 2024-10-04 at 18:04 +0200, Oleksii Kurochko wrote:
> > > @@ -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?
> 
> Counter question: Why did you put the ASSERT() there when - once
> corrected - it's actually pointless? What you want to make sure is
> that virt_to_maddr() can't be invoked with bad argument (without
> noticing). If that's achieved with just the other assertion (as it
> looks to be), then leaving out this assertion ought to be fine.
Originally, I didn’t include the part after 'if (...)'. The purpose of
ASSERT(va < (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE)) was to ensure that
the virtual address fell within the expected (directmap) range. Later,
I added the part after 'if (...)', which extended the acceptable
virtual address range to also cover addresses from Xen’s linkage
address space. At that point, I should have removed the original
ASSERT() but overlooked it.

I will drop the first ASSERT() and update the commit message / comment
above virt_to_maddr() why it is enough to have only 1 ASSERT() after if
(...).

~ Oleksii