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 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? (And
that's aside from me (a) not following the naming (vs their purpose) and
(b) not seeing what _PAGE_*_BIT are needed for, not even thinking about
the leading underscores in these identifiers again.)

> ...
> /*
>  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
>  * meant to be used outside of this header.
>  */
> // #define _PAGE_DEVICE    _PAGE_XN
> #define _PAGE_NORMAL    _PAGE_PRESENT
> 
> #define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL | _PAGE_RO | _PAGE_XN |
> _PAGE_W)
> 
> #define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> 
> And _PAGE_PRESENT in pgtbl-bits.h:
> 
> #define _PAGE_PRESENT   (1 << 0)
> 
> I prefer to introduce all this things when it will be really used.

I understand that, yet for easy things it may help doing them right
away, rather than leaving them to be touched (in a straightforward way)
again very soon.

Jan