On 26.11.2024 12:57, [email protected] 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
