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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2024 12:57, oleksii.kurochko@xxxxxxxxx wrote:
> 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?

I think it would be better if all ports agreed in this regard. That agreement
may also mean that x86 needs changing. Arm's mmu/layout.h also specifies
DIRECTMAP_VIRT_END as inclusive, for example (and, because of aliasing, also
HYPERVISOR_VIRT_END).

>>> @@ -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;
>     }

Yes (and at this point you could even fold the two assertions).

Jan