On 21.03.2023 18:08, Oleksii wrote:
> On Mon, 2023-03-20 at 17:41 +0100, Jan Beulich wrote:
>> On 16.03.2023 17:43, Oleksii Kurochko wrote:
>>> +#define LEVEL_ORDER(lvl) (lvl * PAGETABLE_ORDER)
>>> +#define LEVEL_SHIFT(lvl) (LEVEL_ORDER(lvl) +
>>> PAGE_ORDER)
>>> +#define LEVEL_SIZE(lvl) (_AT(paddr_t, 1) <<
>>> LEVEL_SHIFT(lvl))
>>> +
>>> +#define XEN_PT_LEVEL_SHIFT(lvl) LEVEL_SHIFT(lvl)
>>> +#define XEN_PT_LEVEL_ORDER(lvl) LEVEL_ORDER(lvl)
>>> +#define XEN_PT_LEVEL_SIZE(lvl) LEVEL_SIZE(lvl)
>>
>> Mind me asking what these are good for? Doesn't one set of macros
>> suffice?
> Do you mean XEN_PT_LEVEL_{SHIFT...}? it can be used only one pair of
> macros. I'll check how they are used in ARM ( I copied that macros from
> there ).
There's no similar duplication in Arm code: They have LEVEL_..._GS(),
but that takes a second parameter.
>>> +#define XEN_PT_LEVEL_MAP_MASK(lvl) (~(XEN_PT_LEVEL_SIZE(lvl) -
>>> 1))
>>> +#define XEN_PT_LEVEL_MASK(lvl) (VPN_MASK <<
>>> XEN_PT_LEVEL_SHIFT(lvl))
>>> +
>>> +#define PTE_SHIFT 10
>>
>> What does this describe? According to its single use here it may
>> simply require a better name.
> If look at Sv39 page table entry in riscv-priviliged.pdf. It has the
> following description:
> 63 62 61 60 54 53 28 27 19 18 10 9 8 7 6 5 4 3 2 1 0
> N PBMT Rererved PPN[2] PPN[1] PPN[0] RSW D A G U X W R V
> So 10 it means the 0-9 bits.
Right. As said, I think the name needs improving, so it becomes clear
what it refers to. Possibly PTE_PPN_SHIFT?
Another question: Do you really mean to only support Sv39?
>>> +/* Page Table entry */
>>> +typedef struct {
>>> + uint64_t pte;
>>> +} pte_t;
>>
>> Not having read the respective spec (yet) I'm wondering if this
>> really
>> is this way also for RV32 (despite the different PAGETABLE_ORDER).
> RISC-V architecture support several MMU mode to translate VA to PA.
> The following mode can be: Sv32, Sv39, Sv48, Sv57 and PAGESIZE is equal
> to 8 in all cases except Sv32 ( it is equal to 4 ).
I guess you mean PTESIZE.
> but I looked at
> different big projects who have RISC-V support and no one supports
> Sv32.
>
> So it will be correct for RV32 as it supports the same Sv32 and Sv39
> modes too.
Would you mind extending the comment then to mention that there's no
intention to support Sv32, even on RV32? (Alternatively, as per a
remark you had further down, some #ifdef-ary may be needed.)
Jan
