On 24.11.2023 11:30, Oleksii Kurochko wrote: > Also the patchs adds some helpful macros.
In how far they're (going to be) helpful is hard to tell without uses and without some suitable comments. > --- a/xen/arch/riscv/include/asm/config.h > +++ b/xen/arch/riscv/include/asm/config.h > @@ -77,12 +77,31 @@ > name: > #endif > > +#define VPN_BITS (9) > +#define OFFSET_BITS (12) Whose offset? In how far is this different from PAGE_SHIFT? > #ifdef CONFIG_RISCV_64 > + > +#define SLOTN_ENTRY_BITS (HYP_PT_ROOT_LEVEL * VPN_BITS + OFFSET_BITS) > +#define SLOTN(slot) (_AT(vaddr_t,slot) << SLOTN_ENTRY_BITS) Nit: Missing blank after comma. > +#define SLOTN_ENTRY_SIZE SLOTN(1) > + > #define XEN_VIRT_START 0xFFFFFFFFC0000000 /* (_AC(-1, UL) + 1 - GB(1)) */ > + > +#define FRAMETABLE_VIRT_START SLOTN(196) > +#define FRAMETABLE_SIZE GB(3) > +#define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) > +#define FRAMETABLE_VIRT_END (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) > + > +#define VMAP_VIRT_START SLOTN(194) > +#define VMAP_VIRT_SIZE GB(1) May I suggest that you keep these blocks sorted by slot number? Or wait, the layout comment further up is also in decreasing order, so that's fine here, but then can all of this please be moved next to the comment actually providing the necessary context (thus eliminating the need for new comments)? You'll then also notice that the generalization here (keeping basically the same layout for e.g. SATP_MODE_SV48, just shifted by 9 bits) isn't in line with the comment there. > @@ -95,6 +114,8 @@ > #define RV_STAGE1_MODE SATP_MODE_SV32 > #endif > > +#define HYP_PT_ROOT_LEVEL (CONFIG_PAGING_LEVELS - 1) I understand that CONFIG_PAGING_LEVELS is defined only just up from here, but what that identifier stands for is quite clear. It would seem to me that moving this up ahead if its first use would help clarity. Jan
