On 01.10.2024 17:41, 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.
>
>> + 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).
>
>> + return phys_offset + va;
>
> Don't you need to subtract XEN_VIRT_START here?
Oh, that subtraction is part of the calculation of phys_offset. The variable's
name then doesn't really reflect its purpose, imo.
Jan
