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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan