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 <[email protected]>
>>
>> Acked-by: Jan Beulich <[email protected]>
>>
>> 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
