Re: [PATCH v2 32/39] xen/riscv: add minimal stuff to asm/page.h to build full Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.12.2023 12:57, Oleksii wrote:
> On Mon, 2023-12-18 at 12:36 +0100, Jan Beulich wrote:
>> On 18.12.2023 11:45, Oleksii wrote:
>>> On Thu, 2023-12-14 at 16:57 +0100, Jan Beulich wrote:
>>>> On 24.11.2023 11:30, Oleksii Kurochko wrote:
>>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>> I wonder though ...
>>>>
>>>>> --- a/xen/arch/riscv/include/asm/page.h
>>>>> +++ b/xen/arch/riscv/include/asm/page.h
>>>>> @@ -6,6 +6,7 @@
>>>>>  #ifndef __ASSEMBLY__
>>>>>  
>>>>>  #include <xen/const.h>
>>>>> +#include <xen/bug.h>
>>>>>  #include <xen/types.h>
>>>>>  
>>>>>  #include <asm/mm.h>
>>>>> @@ -32,6 +33,9 @@
>>>>>  #define PTE_LEAF_DEFAULT            (PTE_VALID | PTE_READABLE
>>>>> |
>>>>> PTE_WRITABLE)
>>>>>  #define PTE_TABLE                   (PTE_VALID)
>>>>>  
>>>>> +/* TODO */
>>>>> +#define PAGE_HYPERVISOR 0
>>>>
>>>> ... whether this couldn't be defined properly right away.
>>> It can be introduced now but it requires some additional defines to
>>> be
>>> introduced in the same time:
>>>
>>> #define _PAGE_W_BIT     2
>>> #define _PAGE_XN_BIT    3
>>> #define _PAGE_RO_BIT    1
>>> #define _PAGE_XN        (1U << _PAGE_XN_BIT)
>>> #define _PAGE_RO        (1U << _PAGE_RO_BIT)
>>> #define _PAGE_W         (1U << _PAGE_W_BIT)
>>
>> Why would you need these, when you already have
>> PTE_{READABLE,WRITABLE,EXECUTABLE} just out of context from above?
> I thought that the hypervisor page table is fully software-related, and
> that's why a separate PAGE*BIT was introduced. These bits can be
> different from PTE* bits, which are hardware-related
> https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/include/asm/page.h?ref_type=heads#L66
> 
> It looks like I misunderstood that, and PTE* can be used everywhere
> instead of _PAGE*. Alternatively, we could consider renaming everything
> to PAGE* to maintain consistency across architectures.
> 
> Does it make sense?

Sure. Whether renaming makes sense is harder to tell though. It would
be only the name prefix that's uniform, as how exactly e.g. permissions
are controlled is arch-specific anyway. The one place I'm aware where
PAGE_* (or, sadly, still _PAGE_*) would matter for common code is
_PAGE_NONE (not sure though whether that's something that can / wants
to be expressed for RISC-V).

Jan