On 11.07.2024 11:31, Oleksii wrote: > On Wed, 2024-07-10 at 12:38 +0200, Jan Beulich wrote: >> On 03.07.2024 12:42, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/include/asm/config.h >>> +++ b/xen/arch/riscv/include/asm/config.h >>> @@ -74,6 +74,9 @@ >>> #error "unsupported RV_STAGE1_MODE" >>> #endif >>> >>> +#define XEN_SIZE MB(2) >>> +#define XEN_VIRT_END (XEN_VIRT_START + XEN_SIZE) >> >> Probably wants accompanying by an assertion in the linker script. Or >> else >> how would one notice when Xen grows bigger than this? > I use XEN_SIZE in the linker script here: > ASSERT(_end - _start <= MB(2), "Xen too large for early-boot > assumptions")
And that's the problem: You want to switch to using XEN_SIZE there. There should never be two separate constant that need updating at the same time. Keep such to a single place. >>> @@ -99,6 +102,9 @@ >>> #define VMAP_VIRT_START SLOTN(VMAP_SLOT_START) >>> #define VMAP_VIRT_SIZE GB(1) >>> >>> +#define BOOT_FDT_VIRT_START XEN_VIRT_END >>> +#define BOOT_FDT_VIRT_SIZE MB(4) >> >> Is the 4 selected arbitrarily, or derived from something? > Yes, it was chosen arbitrarily. I just checked that I don't have any > DTBs larger than 2 MB, but decided to add a little extra space and > doubled it to an additional 2 MB. Code comment then, please, or at the very least mention of this in the description. >>> @@ -39,9 +42,11 @@ static unsigned long __ro_after_init >>> phys_offset; >>> * isn't 2 MB aligned. >>> * >>> * CONFIG_PAGING_LEVELS page tables are needed for the identity >>> mapping, >>> - * except that the root page table is shared with the initial >>> mapping >>> + * except that the root page table is shared with the initial >>> mapping. >>> + * >>> + * CONFIG_PAGING_LEVELS page tables are needed for device tree >>> mapping. >>> */ >>> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1) >>> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 3 + 1 + >>> 1) >> >> Considering what would happen if two or three more of such >> requirements >> were added, maybe better >> >> #define PGTBL_INITIAL_COUNT (CONFIG_PAGING_LEVELS * 3 - 1) >> >> ? However, why is it CONFIG_PAGING_LEVELS that's needed, and not >> CONFIG_PAGING_LEVELS - 1? The top level table is the same as the >> identity map's, isn't it? > The top level table is the same, but I just wanted to be sure that if > DTB size is bigger then 2Mb then we need 2xL0 page tables. Makes sense, but then needs expressing that way (by using BOOT_FDT_VIRT_SIZE). Otherwise (see also above) think of what will happen if BOOT_FDT_VIRT_SIZE is updated without touching this expression. Jan
