| Message ID | 20220824073127.16762-3-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce reserved heap | expand |
Hi Henry, On 24/08/2022 09:31, Henry Wang wrote: > > This commit firstly adds a global variable `reserved_heap`. > This newly introduced global variable is set at the device tree > parsing time if the reserved heap ranges are defined in the device > tree chosen node. > Did you consider putting reserved_heap into bootinfo structure? It would help to avoid introducing new global variables that are only used in places making use of the bootinfo anyway. > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use > the reserved heap region for both domheap and xenheap allocation. > > For Arm64, In `setup_mm`, if the reserved heap is enabled and used, > we make sure that only these reserved heap pages are added to the > boot allocator. These reserved heap pages in the boot allocator are > added to the heap allocator at `end_boot_allocator()`. > > If the reserved heap is disabled, we stick to current page allocation > strategy at boot time. > > Also, take the chance to correct a "double not" print in Arm32 > `setup_mm()`. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > With reserved heap enabled, for Arm64, naming of global variables such > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous, > wondering if we should rename these variables. > --- > Changes from RFC to v1: > - Rebase on top of latest `setup_mm()` changes. > - Added Arm32 logic in `setup_mm()`. > --- > xen/arch/arm/bootfdt.c | 2 + > xen/arch/arm/include/asm/setup.h | 2 + > xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++------- > 3 files changed, 67 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 33704ca487..ab73b6e212 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node, > true); > if ( rc ) > return rc; > + > + reserved_heap = true; > } > > printk("Checking for initrd in /chosen\n"); > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index e80f3d6201..00536a6d55 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo; > > extern domid_t max_init_domid; > > +extern bool reserved_heap; > + > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); > > size_t estimate_efi_size(unsigned int mem_nr_banks); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 500307edc0..fe76cf6325 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > domid_t __read_mostly max_init_domid; > > +bool __read_mostly reserved_heap; > + > static __used void init_done(void) > { > /* Must be done past setting system_state. */ > @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) > #ifdef CONFIG_ARM_32 > static void __init setup_mm(void) > { > - paddr_t ram_start, ram_end, ram_size, e; > - unsigned long ram_pages; > + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size; > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > + reserved_heap_size = 0; > + unsigned long ram_pages, reserved_heap_pages = 0; > unsigned long heap_pages, xenheap_pages, domheap_pages; > unsigned int i; > const uint32_t ctr = READ_CP32(CTR); > @@ -720,9 +724,9 @@ static void __init setup_mm(void) > > for ( i = 1; i < bootinfo.mem.nr_banks; i++ ) > { > - paddr_t bank_start = bootinfo.mem.bank[i].start; > - paddr_t bank_size = bootinfo.mem.bank[i].size; > - paddr_t bank_end = bank_start + bank_size; > + bank_start = bootinfo.mem.bank[i].start; > + bank_size = bootinfo.mem.bank[i].size; > + bank_end = bank_start + bank_size; > > ram_size = ram_size + bank_size; > ram_start = min(ram_start,bank_start); > @@ -731,6 +735,25 @@ static void __init setup_mm(void) > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > + if ( reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + reserved_heap_size += bank_size; > + reserved_heap_start = min(reserved_heap_start, bank_start); You do not need reserved_heap_start as you do not use it at any place later on. In your current implementation you just need reserved_heap_size and reserved_heap_end. > + reserved_heap_end = max(reserved_heap_end, bank_end); > + } > + } > + > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > + } > + > /* > * If the user has not requested otherwise via the command line > * then locate the xenheap using these constraints: > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > * We try to allocate the largest xenheap possible within these > * constraints. > */ > - heap_pages = ram_pages; > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; I must say that the reverted logic is harder to read. This is a matter of taste but please consider the following: heap_pages = reserved_heap ? reserved_heap_pages : ram_pages; The same applies to ... > + > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > do > { > - e = consider_modules(ram_start, ram_end, > + e = !reserved_heap ? ... here. > + consider_modules(ram_start, ram_end, > pfn_to_paddr(xenheap_pages), > - 32<<20, 0); > + 32<<20, 0) : > + reserved_heap_end; > + > if ( e ) > break; > > xenheap_pages >>= 1; > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); > > - if ( ! e ) > - panic("Not not enough space for xenheap\n"); > + if ( ! e || > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) I'm not sure about this. You are checking if the size of the reserved heap is less than 32MB and this has nothing to do with the following panic message. > + panic("Not enough space for xenheap\n"); > > domheap_pages = heap_pages - xenheap_pages; > > @@ -810,9 +838,9 @@ static void __init setup_mm(void) > static void __init setup_mm(void) > { > const struct meminfo *banks = &bootinfo.mem; > - paddr_t ram_start = ~0; > - paddr_t ram_end = 0; > - paddr_t ram_size = 0; > + paddr_t ram_start = ~0, bank_start = ~0; > + paddr_t ram_end = 0, bank_end = 0; > + paddr_t ram_size = 0, bank_size = 0; > unsigned int i; > > init_pdx(); > @@ -821,17 +849,36 @@ static void __init setup_mm(void) > * We need some memory to allocate the page-tables used for the xenheap > * mappings. But some regions may contain memory already allocated > * for other uses (e.g. modules, reserved-memory...). > - * > + * If reserved heap regions are properly defined, (only) add these regions How can you say at this stage whether the reserved heap regions are defined properly? > + * in the boot allocator. > + */ > + if ( reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + init_boot_pages(bank_start, bank_end); > + } > + } > + } > + /* > + * No reserved heap regions: > * For simplicity, add all the free regions in the boot allocator. > */ > - populate_boot_allocator(); > + else > + populate_boot_allocator(); > > total_pages = 0; > > for ( i = 0; i < banks->nr_banks; i++ ) > { > const struct membank *bank = &banks->bank[i]; > - paddr_t bank_end = bank->start + bank->size; > + bank_end = bank->start + bank->size; > > ram_size = ram_size + bank->size; > ram_start = min(ram_start, bank->start); > -- > 2.17.1 > > ~Michal
On Wed, 24 Aug 2022, Henry Wang wrote: > This commit firstly adds a global variable `reserved_heap`. > This newly introduced global variable is set at the device tree > parsing time if the reserved heap ranges are defined in the device > tree chosen node. > > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use > the reserved heap region for both domheap and xenheap allocation. > > For Arm64, In `setup_mm`, if the reserved heap is enabled and used, > we make sure that only these reserved heap pages are added to the > boot allocator. These reserved heap pages in the boot allocator are > added to the heap allocator at `end_boot_allocator()`. > > If the reserved heap is disabled, we stick to current page allocation > strategy at boot time. > > Also, take the chance to correct a "double not" print in Arm32 > `setup_mm()`. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > With reserved heap enabled, for Arm64, naming of global variables such > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous, > wondering if we should rename these variables. > --- > Changes from RFC to v1: > - Rebase on top of latest `setup_mm()` changes. > - Added Arm32 logic in `setup_mm()`. > --- > xen/arch/arm/bootfdt.c | 2 + > xen/arch/arm/include/asm/setup.h | 2 + > xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++------- > 3 files changed, 67 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 33704ca487..ab73b6e212 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node, > true); > if ( rc ) > return rc; > + > + reserved_heap = true; > } > > printk("Checking for initrd in /chosen\n"); > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index e80f3d6201..00536a6d55 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo; > > extern domid_t max_init_domid; > > +extern bool reserved_heap; > + > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); > > size_t estimate_efi_size(unsigned int mem_nr_banks); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 500307edc0..fe76cf6325 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > domid_t __read_mostly max_init_domid; > > +bool __read_mostly reserved_heap; > + > static __used void init_done(void) > { > /* Must be done past setting system_state. */ > @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) > #ifdef CONFIG_ARM_32 > static void __init setup_mm(void) > { > - paddr_t ram_start, ram_end, ram_size, e; > - unsigned long ram_pages; > + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size; > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, INVALID_PADDR or ~0ULL > + reserved_heap_size = 0; > + unsigned long ram_pages, reserved_heap_pages = 0; > unsigned long heap_pages, xenheap_pages, domheap_pages; > unsigned int i; > const uint32_t ctr = READ_CP32(CTR); > @@ -720,9 +724,9 @@ static void __init setup_mm(void) > > for ( i = 1; i < bootinfo.mem.nr_banks; i++ ) > { > - paddr_t bank_start = bootinfo.mem.bank[i].start; > - paddr_t bank_size = bootinfo.mem.bank[i].size; > - paddr_t bank_end = bank_start + bank_size; > + bank_start = bootinfo.mem.bank[i].start; > + bank_size = bootinfo.mem.bank[i].size; > + bank_end = bank_start + bank_size; > > ram_size = ram_size + bank_size; > ram_start = min(ram_start,bank_start); > @@ -731,6 +735,25 @@ static void __init setup_mm(void) > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > + if ( reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + reserved_heap_size += bank_size; > + reserved_heap_start = min(reserved_heap_start, bank_start); > + reserved_heap_end = max(reserved_heap_end, bank_end); > + } > + } > + > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > + } > + > /* > * If the user has not requested otherwise via the command line > * then locate the xenheap using these constraints: > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > * We try to allocate the largest xenheap possible within these > * constraints. > */ > - heap_pages = ram_pages; > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > + > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > do > { > - e = consider_modules(ram_start, ram_end, > + e = !reserved_heap ? > + consider_modules(ram_start, ram_end, > pfn_to_paddr(xenheap_pages), > - 32<<20, 0); > + 32<<20, 0) : > + reserved_heap_end; > + > if ( e ) > break; > > xenheap_pages >>= 1; > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); > > - if ( ! e ) > - panic("Not not enough space for xenheap\n"); > + if ( ! e || > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > + panic("Not enough space for xenheap\n"); I would skip the do/while loop completely if reserved_heap. We don't need it anyway and we can automatically calculate xenheap_pages in a single line. > domheap_pages = heap_pages - xenheap_pages; > > @@ -810,9 +838,9 @@ static void __init setup_mm(void) > static void __init setup_mm(void) > { > const struct meminfo *banks = &bootinfo.mem; > - paddr_t ram_start = ~0; > - paddr_t ram_end = 0; > - paddr_t ram_size = 0; > + paddr_t ram_start = ~0, bank_start = ~0; > + paddr_t ram_end = 0, bank_end = 0; > + paddr_t ram_size = 0, bank_size = 0; > unsigned int i; Please use INVALID_PADDR or ~0ULL > > init_pdx(); > @@ -821,17 +849,36 @@ static void __init setup_mm(void) > * We need some memory to allocate the page-tables used for the xenheap > * mappings. But some regions may contain memory already allocated > * for other uses (e.g. modules, reserved-memory...). > - * > + * If reserved heap regions are properly defined, (only) add these regions > + * in the boot allocator. > + */ > + if ( reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + init_boot_pages(bank_start, bank_end); > + } > + } > + } > + /* > + * No reserved heap regions: > * For simplicity, add all the free regions in the boot allocator. > */ > - populate_boot_allocator(); > + else > + populate_boot_allocator(); > > total_pages = 0; > > for ( i = 0; i < banks->nr_banks; i++ ) > { > const struct membank *bank = &banks->bank[i]; > - paddr_t bank_end = bank->start + bank->size; > + bank_end = bank->start + bank->size; > > ram_size = ram_size + bank->size; > ram_start = min(ram_start, bank->start); > -- > 2.17.1 >
Hi Michal, Sorry about the late reply - I had a couple of days off. Thank you very much for the review! I will add my reply and answer some of your questions below. > -----Original Message----- > From: Michal Orzel <michal.orzel@amd.com> > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > heap allocator > > > This commit firstly adds a global variable `reserved_heap`. > > This newly introduced global variable is set at the device tree > > parsing time if the reserved heap ranges are defined in the device > > tree chosen node. > > > Did you consider putting reserved_heap into bootinfo structure? Actually I did, but I saw current bootinfo only contains some structs so I was not sure if this is the preferred way, but since you are raising this question, I will follow this method in v2. > It would help to avoid introducing new global variables that are only used > in places making use of the bootinfo anyway. Ack. > > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > > + { > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > + > > + reserved_heap_size += bank_size; > > + reserved_heap_start = min(reserved_heap_start, bank_start); > You do not need reserved_heap_start as you do not use it at any place later > on. > In your current implementation you just need reserved_heap_size and > reserved_heap_end. Good point, thank you and I will remove in v2. > > > /* > > * If the user has not requested otherwise via the command line > > * then locate the xenheap using these constraints: > > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > > * We try to allocate the largest xenheap possible within these > > * constraints. > > */ > > - heap_pages = ram_pages; > > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > I must say that the reverted logic is harder to read. This is a matter of taste > but > please consider the following: > heap_pages = reserved_heap ? reserved_heap_pages : ram_pages; > The same applies to ... Sure, I will use the way you suggested. > > > + > > if ( opt_xenheap_megabytes ) > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > else > > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > > > do > > { > > - e = consider_modules(ram_start, ram_end, > > + e = !reserved_heap ? > ... here. And here :)) > > > + consider_modules(ram_start, ram_end, > > pfn_to_paddr(xenheap_pages), > > - 32<<20, 0); > > + 32<<20, 0) : > > + reserved_heap_end; > > + > > if ( e ) > > break; > > > > xenheap_pages >>= 1; > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > PAGE_SHIFT) ); > > > > - if ( ! e ) > > - panic("Not not enough space for xenheap\n"); > > + if ( ! e || > > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > I'm not sure about this. You are checking if the size of the reserved heap is > less than 32MB > and this has nothing to do with the following panic message. Hmmm, I am not sure if I understand your question correctly, so here there are actually 2 issues: (1) The double not in the panic message. (2) The size of xenheap. If you check the comment of the xenheap constraints above, one rule of the xenheap size is it "must be at least 32M". If I am not mistaken, we need to follow the same rule with the reserved heap setup, so here we need to check the size and if <32M then panic. > > > + panic("Not enough space for xenheap\n"); > > > > domheap_pages = heap_pages - xenheap_pages; > > > > @@ -810,9 +838,9 @@ static void __init setup_mm(void) > > static void __init setup_mm(void) > > { > > const struct meminfo *banks = &bootinfo.mem; > > - paddr_t ram_start = ~0; > > - paddr_t ram_end = 0; > > - paddr_t ram_size = 0; > > + paddr_t ram_start = ~0, bank_start = ~0; > > + paddr_t ram_end = 0, bank_end = 0; > > + paddr_t ram_size = 0, bank_size = 0; > > unsigned int i; > > > > init_pdx(); > > @@ -821,17 +849,36 @@ static void __init setup_mm(void) > > * We need some memory to allocate the page-tables used for the > xenheap > > * mappings. But some regions may contain memory already allocated > > * for other uses (e.g. modules, reserved-memory...). > > - * > > + * If reserved heap regions are properly defined, (only) add these > regions > How can you say at this stage whether the reserved heap regions are defined > properly? Because if the reserved heap regions are not properly defined, in the device tree parsing phase the global variable "reserved_heap" can never be true. Did I understand your question correctly? Or maybe we need to change the wording here in the comment? Kind regards, Henry > > > + * in the boot allocator. > > + */ > > + if ( reserved_heap ) > > + { > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > > + { > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > + > > + init_boot_pages(bank_start, bank_end); > > + } > > + } > > + } > > + /* > > + * No reserved heap regions: > > * For simplicity, add all the free regions in the boot allocator. > > */ > > - populate_boot_allocator(); > > + else > > + populate_boot_allocator(); > > > > total_pages = 0; > > > > for ( i = 0; i < banks->nr_banks; i++ ) > > { > > const struct membank *bank = &banks->bank[i]; > > - paddr_t bank_end = bank->start + bank->size; > > + bank_end = bank->start + bank->size; > > > > ram_size = ram_size + bank->size; > > ram_start = min(ram_start, bank->start); > > -- > > 2.17.1 > > > > > > ~Michal
Hi Henry, On 30/08/2022 08:11, Henry Wang wrote: > > Hi Michal, > > Sorry about the late reply - I had a couple of days off. Thank you very > much for the review! I will add my reply and answer some of your > questions below. > >> -----Original Message----- >> From: Michal Orzel <michal.orzel@amd.com> >> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and >> heap allocator >> >>> This commit firstly adds a global variable `reserved_heap`. >>> This newly introduced global variable is set at the device tree >>> parsing time if the reserved heap ranges are defined in the device >>> tree chosen node. >>> >> Did you consider putting reserved_heap into bootinfo structure? > > Actually I did, but I saw current bootinfo only contains some structs so > I was not sure if this is the preferred way, but since you are raising this > question, I will follow this method in v2. This is what I think would be better but maintainers will have a decisive vote. > >> It would help to avoid introducing new global variables that are only used >> in places making use of the bootinfo anyway. > > Ack. > >> >>> + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) >>> + { >>> + if ( bootinfo.reserved_mem.bank[i].xen_heap ) >>> + { >>> + bank_start = bootinfo.reserved_mem.bank[i].start; >>> + bank_size = bootinfo.reserved_mem.bank[i].size; >>> + bank_end = bank_start + bank_size; >>> + >>> + reserved_heap_size += bank_size; >>> + reserved_heap_start = min(reserved_heap_start, bank_start); >> You do not need reserved_heap_start as you do not use it at any place later >> on. >> In your current implementation you just need reserved_heap_size and >> reserved_heap_end. > > Good point, thank you and I will remove in v2. > >> >>> /* >>> * If the user has not requested otherwise via the command line >>> * then locate the xenheap using these constraints: >>> @@ -743,7 +766,8 @@ static void __init setup_mm(void) >>> * We try to allocate the largest xenheap possible within these >>> * constraints. >>> */ >>> - heap_pages = ram_pages; >>> + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; >> I must say that the reverted logic is harder to read. This is a matter of taste >> but >> please consider the following: >> heap_pages = reserved_heap ? reserved_heap_pages : ram_pages; >> The same applies to ... > > Sure, I will use the way you suggested. > >> >>> + >>> if ( opt_xenheap_megabytes ) >>> xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); >>> else >>> @@ -755,17 +779,21 @@ static void __init setup_mm(void) >>> >>> do >>> { >>> - e = consider_modules(ram_start, ram_end, >>> + e = !reserved_heap ? >> ... here. > > And here :)) > >> >>> + consider_modules(ram_start, ram_end, >>> pfn_to_paddr(xenheap_pages), >>> - 32<<20, 0); >>> + 32<<20, 0) : >>> + reserved_heap_end; >>> + >>> if ( e ) >>> break; >>> >>> xenheap_pages >>= 1; >>> } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- >> PAGE_SHIFT) ); >>> >>> - if ( ! e ) >>> - panic("Not not enough space for xenheap\n"); >>> + if ( ! e || >>> + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) >> I'm not sure about this. You are checking if the size of the reserved heap is >> less than 32MB >> and this has nothing to do with the following panic message. > > Hmmm, I am not sure if I understand your question correctly, so here there > are actually 2 issues: > (1) The double not in the panic message. > (2) The size of xenheap. > > If you check the comment of the xenheap constraints above, one rule of the > xenheap size is it "must be at least 32M". If I am not mistaken, we need to > follow the same rule with the reserved heap setup, so here we need to check > the size and if <32M then panic. This is totally fine. What I mean is that the check you introduced does not correspond to the panic message below. In case of reserved heap, its size is selected by the user. "Not enough space for xenheap" means that there is not enough space to be reserved for heap, meaning its size is too large. But your check is about size being too small. > >> >>> + panic("Not enough space for xenheap\n"); >>> >>> domheap_pages = heap_pages - xenheap_pages; >>> >>> @@ -810,9 +838,9 @@ static void __init setup_mm(void) >>> static void __init setup_mm(void) >>> { >>> const struct meminfo *banks = &bootinfo.mem; >>> - paddr_t ram_start = ~0; >>> - paddr_t ram_end = 0; >>> - paddr_t ram_size = 0; >>> + paddr_t ram_start = ~0, bank_start = ~0; >>> + paddr_t ram_end = 0, bank_end = 0; >>> + paddr_t ram_size = 0, bank_size = 0; >>> unsigned int i; >>> >>> init_pdx(); >>> @@ -821,17 +849,36 @@ static void __init setup_mm(void) >>> * We need some memory to allocate the page-tables used for the >> xenheap >>> * mappings. But some regions may contain memory already allocated >>> * for other uses (e.g. modules, reserved-memory...). >>> - * >>> + * If reserved heap regions are properly defined, (only) add these >> regions >> How can you say at this stage whether the reserved heap regions are defined >> properly? > > Because if the reserved heap regions are not properly defined, in the device > tree parsing phase the global variable "reserved_heap" can never be true. > > Did I understand your question correctly? Or maybe we need to change the > wording here in the comment? FWICS, reserved_heap will be set to true even if a user describes an empty region for reserved heap. This cannot be consider a properly defined region for a heap. ~Michal
Hi Michal, > -----Original Message----- > From: Michal Orzel <michal.orzel@amd.com> > >>> > >> Did you consider putting reserved_heap into bootinfo structure? > > > > Actually I did, but I saw current bootinfo only contains some structs so > > I was not sure if this is the preferred way, but since you are raising this > > question, I will follow this method in v2. > This is what I think would be better but maintainers will have a decisive vote. Then let's wait for more input from maintainers. > > >>> > >>> - if ( ! e ) > >>> - panic("Not not enough space for xenheap\n"); > >>> + if ( ! e || > >>> + ( reserved_heap && reserved_heap_pages < 32<<(20- > PAGE_SHIFT) ) ) > >> I'm not sure about this. You are checking if the size of the reserved heap is > >> less than 32MB > >> and this has nothing to do with the following panic message. > > > > Hmmm, I am not sure if I understand your question correctly, so here there > > are actually 2 issues: > > (1) The double not in the panic message. > > (2) The size of xenheap. > > > > If you check the comment of the xenheap constraints above, one rule of > the > > xenheap size is it "must be at least 32M". If I am not mistaken, we need to > > follow the same rule with the reserved heap setup, so here we need to > check > > the size and if <32M then panic. > This is totally fine. What I mean is that the check you introduced does not > correspond > to the panic message below. In case of reserved heap, its size is selected by > the user. > "Not enough space for xenheap" means that there is not enough space to be > reserved for heap, > meaning its size is too large. But your check is about size being too small. Actually my understanding of "Not enough space for xenheap" is xenheap is too large so we need to reserve more space, which is slightly different than your opinion. But I am not the native speaker so it is highly likely that I am making mistakes... How about changing the panic message to "Not enough memory for xenheap"? This would remove the ambiguity here IMHO. > > >>> + * If reserved heap regions are properly defined, (only) add these > >> regions > >> How can you say at this stage whether the reserved heap regions are > defined > >> properly? > > > > Because if the reserved heap regions are not properly defined, in the > device > > tree parsing phase the global variable "reserved_heap" can never be true. > > > > Did I understand your question correctly? Or maybe we need to change the > > wording here in the comment? > > FWICS, reserved_heap will be set to true even if a user describes an empty > region > for reserved heap. This cannot be consider a properly defined region for a > heap. Oh good point, thank you for pointing this out. I will change the comments here to "If there are non-empty reserved heap regions". I am not sure if adding an empty region check before setting the "reserved_heap" would be a good idea, because adding such check would add another for loop to find a non-empty reserved heap bank. What do you think? Kind regards, Henry > > ~Michal
Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > > INVALID_PADDR or ~0ULL Ack. > > > /* > > * If the user has not requested otherwise via the command line > > * then locate the xenheap using these constraints: > > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > > * We try to allocate the largest xenheap possible within these > > * constraints. > > */ > > - heap_pages = ram_pages; > > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > > + > > if ( opt_xenheap_megabytes ) > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > else > > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > > > do > > { > > - e = consider_modules(ram_start, ram_end, > > + e = !reserved_heap ? > > + consider_modules(ram_start, ram_end, > > pfn_to_paddr(xenheap_pages), > > - 32<<20, 0); > > + 32<<20, 0) : > > + reserved_heap_end; > > + > > if ( e ) > > break; > > > > xenheap_pages >>= 1; > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > PAGE_SHIFT) ); > > > > - if ( ! e ) > > - panic("Not not enough space for xenheap\n"); > > + if ( ! e || > > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > > + panic("Not enough space for xenheap\n"); > > > I would skip the do/while loop completely if reserved_heap. We don't > need it anyway I agree with this. > and we can automatically calculate xenheap_pages in a single line. Here I am a little bit confused. Sorry to ask but could you please explain a little bit more about why we can calculate the xenheap_pages in a single line? Below is the code snippet in my mind, is this correct? if (reserved_heap) e = reserved_heap_end; else { do { e = consider_modules(ram_start, ram_end, pfn_to_paddr(xenheap_pages), 32<<20, 0); if ( e ) break; xenheap_pages >>= 1; } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); } > > > domheap_pages = heap_pages - xenheap_pages; > > > > @@ -810,9 +838,9 @@ static void __init setup_mm(void) > > static void __init setup_mm(void) > > { > > const struct meminfo *banks = &bootinfo.mem; > > - paddr_t ram_start = ~0; > > - paddr_t ram_end = 0; > > - paddr_t ram_size = 0; > > + paddr_t ram_start = ~0, bank_start = ~0; > > + paddr_t ram_end = 0, bank_end = 0; > > + paddr_t ram_size = 0, bank_size = 0; > > unsigned int i; > > Please use INVALID_PADDR or ~0ULL Ack. Kind regards, Henry > > > > > > init_pdx(); > > @@ -821,17 +849,36 @@ static void __init setup_mm(void) > > * We need some memory to allocate the page-tables used for the > xenheap > > * mappings. But some regions may contain memory already allocated > > * for other uses (e.g. modules, reserved-memory...). > > - * > > + * If reserved heap regions are properly defined, (only) add these > regions > > + * in the boot allocator. > > + */ > > + if ( reserved_heap ) > > + { > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > > + { > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > + > > + init_boot_pages(bank_start, bank_end); > > + } > > + } > > + } > > + /* > > + * No reserved heap regions: > > * For simplicity, add all the free regions in the boot allocator. > > */ > > - populate_boot_allocator(); > > + else > > + populate_boot_allocator(); > > > > total_pages = 0; > > > > for ( i = 0; i < banks->nr_banks; i++ ) > > { > > const struct membank *bank = &banks->bank[i]; > > - paddr_t bank_end = bank->start + bank->size; > > + bank_end = bank->start + bank->size; > > > > ram_size = ram_size + bank->size; > > ram_start = min(ram_start, bank->start); > > -- > > 2.17.1 > >
Hi Henry, On 30/08/2022 10:00, Henry Wang wrote: > > Hi Michal, > >> -----Original Message----- >> From: Michal Orzel <michal.orzel@amd.com> >>>>> >>>> Did you consider putting reserved_heap into bootinfo structure? >>> >>> Actually I did, but I saw current bootinfo only contains some structs so >>> I was not sure if this is the preferred way, but since you are raising this >>> question, I will follow this method in v2. >> This is what I think would be better but maintainers will have a decisive vote. > > Then let's wait for more input from maintainers. > >> >>>>> >>>>> - if ( ! e ) >>>>> - panic("Not not enough space for xenheap\n"); >>>>> + if ( ! e || >>>>> + ( reserved_heap && reserved_heap_pages < 32<<(20- >> PAGE_SHIFT) ) ) >>>> I'm not sure about this. You are checking if the size of the reserved heap is >>>> less than 32MB >>>> and this has nothing to do with the following panic message. >>> >>> Hmmm, I am not sure if I understand your question correctly, so here there >>> are actually 2 issues: >>> (1) The double not in the panic message. >>> (2) The size of xenheap. >>> >>> If you check the comment of the xenheap constraints above, one rule of >> the >>> xenheap size is it "must be at least 32M". If I am not mistaken, we need to >>> follow the same rule with the reserved heap setup, so here we need to >> check >>> the size and if <32M then panic. >> This is totally fine. What I mean is that the check you introduced does not >> correspond >> to the panic message below. In case of reserved heap, its size is selected by >> the user. >> "Not enough space for xenheap" means that there is not enough space to be >> reserved for heap, >> meaning its size is too large. But your check is about size being too small. > > Actually my understanding of "Not enough space for xenheap" is xenheap > is too large so we need to reserve more space, which is slightly different than > your opinion. But I am not the native speaker so it is highly likely that I am > making mistakes...My understanding is exactly the same as yours :), meaning heap is too large. But your check is against heap being to small (less than 32M). So basically if the following check fails: "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )" it means that the heap region defined by a user is too small (not too large), because according to requirements it should be at least 32M. > > How about changing the panic message to "Not enough memory for xenheap"? > This would remove the ambiguity here IMHO. > >> >>>>> + * If reserved heap regions are properly defined, (only) add these >>>> regions >>>> How can you say at this stage whether the reserved heap regions are >> defined >>>> properly? >>> >>> Because if the reserved heap regions are not properly defined, in the >> device >>> tree parsing phase the global variable "reserved_heap" can never be true. >>> >>> Did I understand your question correctly? Or maybe we need to change the >>> wording here in the comment? >> >> FWICS, reserved_heap will be set to true even if a user describes an empty >> region >> for reserved heap. This cannot be consider a properly defined region for a >> heap. > > Oh good point, thank you for pointing this out. I will change the comments > here to "If there are non-empty reserved heap regions". I am not sure if adding > an empty region check before setting the "reserved_heap" would be a good > idea, because adding such check would add another for loop to find a non-empty > reserved heap bank. What do you think? > > Kind regards, > Henry > >> >> ~Michal
Hi Michal, > -----Original Message----- > From: Michal Orzel <michal.orzel@amd.com> > >> This is totally fine. What I mean is that the check you introduced does not > >> correspond > >> to the panic message below. In case of reserved heap, its size is selected > by > >> the user. > >> "Not enough space for xenheap" means that there is not enough space to > be > >> reserved for heap, > >> meaning its size is too large. But your check is about size being too small. > > > > Actually my understanding of "Not enough space for xenheap" is xenheap > > is too large so we need to reserve more space, which is slightly different > than > > your opinion. But I am not the native speaker so it is highly likely that I am > > making mistakes... > My understanding is exactly the same as yours :), > meaning heap is too large. Oh I think get your point. Let me try to explain myself and thanks for your patience :)) The reserved heap region defined in the device tree should be used for both Xenheap and domain heap, so if we reserved a too small region (<32M), an error should pop because the reserved region is not enough for xenheap, and user should reserve more. [...] > But your check is against heap being to small (less than 32M). > So basically if the following check fails: > "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )" > it means that the heap region defined by a user is too small (not too large), > because according to requirements it should be at least 32M. [...] So in that case, printing "Not enough space for xenheap" means the reserved region cannot satisfy the minimal requirement of the space of xenheap (at least 32M), and this is in consistent with the check. Kind regards, Henry
On 30/08/2022 11:17, Henry Wang wrote: > > Hi Michal, > >> -----Original Message----- >> From: Michal Orzel <michal.orzel@amd.com> >>>> This is totally fine. What I mean is that the check you introduced does not >>>> correspond >>>> to the panic message below. In case of reserved heap, its size is selected >> by >>>> the user. >>>> "Not enough space for xenheap" means that there is not enough space to >> be >>>> reserved for heap, >>>> meaning its size is too large. But your check is about size being too small. >>> >>> Actually my understanding of "Not enough space for xenheap" is xenheap >>> is too large so we need to reserve more space, which is slightly different >> than >>> your opinion. But I am not the native speaker so it is highly likely that I am >>> making mistakes... >> My understanding is exactly the same as yours :), >> meaning heap is too large. > > Oh I think get your point. Let me try to explain myself and thanks for your > patience :)) > > The reserved heap region defined in the device tree should be used for both > Xenheap and domain heap, so if we reserved a too small region (<32M), > an error should pop because the reserved region is not enough for xenheap, > and user should reserve more. > [...] > >> But your check is against heap being to small (less than 32M). >> So basically if the following check fails: >> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )" >> it means that the heap region defined by a user is too small (not too large), >> because according to requirements it should be at least 32M. > > [...] > So in that case, printing "Not enough space for xenheap" means the reserved > region cannot satisfy the minimal requirement of the space of xenheap (at least > 32M), and this is in consistent with the check. Ok, it clearly depends on the way someone understands this sentence. Currently this panic can be triggered if the heap size is too large and should be read as "heap is too large to fit in because there is not enough space within RAM considering modules (e - s < size)". Usually (and also in this case) space refers to a region to contain another one. You are reusing the same message for different meaning, that is "user defined too small heap and this space (read as size) is not enough". Let's leave it to someone else to decide. ~Michal
Hi Michal, > -----Original Message----- > From: Michal Orzel <michal.orzel@amd.com> > > > > Oh I think get your point. Let me try to explain myself and thanks for your > > patience :)) > > > > The reserved heap region defined in the device tree should be used for > both > > Xenheap and domain heap, so if we reserved a too small region (<32M), > > an error should pop because the reserved region is not enough for > xenheap, > > and user should reserve more. > > [...] > > > >> But your check is against heap being to small (less than 32M). > >> So basically if the following check fails: > >> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )" > >> it means that the heap region defined by a user is too small (not too large), > >> because according to requirements it should be at least 32M. > > > > [...] > > So in that case, printing "Not enough space for xenheap" means the > reserved > > region cannot satisfy the minimal requirement of the space of xenheap (at > least > > 32M), and this is in consistent with the check. > > Ok, it clearly depends on the way someone understands this sentence. > Currently this panic can be triggered if the heap size is too large and > should be read as "heap is too large to fit in because there is not enough > space > within RAM considering modules (e - s < size)". Usually (and also in this case) > space refers to a region to contain another one. > > You are reusing the same message for different meaning, that is "user > defined too > small heap and this space (read as size) is not enough". Yes, thanks for the explanation. I think maybe rewording the message to "Not enough memory for allocating xenheap" would remove the ambiguity to some extent? Because the user-defined heap region should cover both xenheap and domain heap at the same time, the small user-defined heap means "xenheap is too large to fit in the user-defined heap region", which is in consistent with your interpretation of the current "xenheap is too large to fit in because there is not enough space within RAM considering modules" > > Let's leave it to someone else to decide. I agree. Kind regards, Henry > > ~Michal
On Tue, 30 Aug 2022, Henry Wang wrote: > > > /* > > > * If the user has not requested otherwise via the command line > > > * then locate the xenheap using these constraints: > > > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > > > * We try to allocate the largest xenheap possible within these > > > * constraints. > > > */ > > > - heap_pages = ram_pages; > > > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > > > + > > > if ( opt_xenheap_megabytes ) > > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > > else > > > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > > > > > do > > > { > > > - e = consider_modules(ram_start, ram_end, > > > + e = !reserved_heap ? > > > + consider_modules(ram_start, ram_end, > > > pfn_to_paddr(xenheap_pages), > > > - 32<<20, 0); > > > + 32<<20, 0) : > > > + reserved_heap_end; > > > + > > > if ( e ) > > > break; > > > > > > xenheap_pages >>= 1; > > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > > PAGE_SHIFT) ); > > > > > > - if ( ! e ) > > > - panic("Not not enough space for xenheap\n"); > > > + if ( ! e || > > > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > > > + panic("Not enough space for xenheap\n"); > > > > > > I would skip the do/while loop completely if reserved_heap. We don't > > need it anyway > > I agree with this. > > > and we can automatically calculate xenheap_pages in a single line. > > Here I am a little bit confused. Sorry to ask but could you please explain > a little bit more about why we can calculate the xenheap_pages in a single > line? Below is the code snippet in my mind, is this correct? > > if (reserved_heap) coding style > e = reserved_heap_end; > else > { > do > { > e = consider_modules(ram_start, ram_end, > pfn_to_paddr(xenheap_pages), > 32<<20, 0); > if ( e ) > break; > > xenheap_pages >>= 1; > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); > } Yes, this is what I meant. But also, here the loop is also for adjusting xenheap_pages, and xenheap_pages is initialized as follows: if ( opt_xenheap_megabytes ) xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); else { xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); } In the reserved_heap case, it doesn't make sense to initialize xenheap_pages like that, right? It should be something like: if ( opt_xenheap_megabytes ) xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); else if ( reserved_heap ) xenheap_pages = heap_pages; else { xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); } But also it looks like that on arm32 we have specific requirements for Xen heap: * - must be 32 MiB aligned * - must not include Xen itself or the boot modules * - must be at most 1GB or 1/32 the total RAM in the system if less * - must be at least 32M I think we should check at least the 32MB alignment and 32MB minimum size before using the xen_heap bank. In short I think this patch should: - add a check for 32MB alignment and size of the xen_heap memory bank - if reserved_heap, set xenheap_pages = heap_pages - if reserved_heap, skip the consider_modules do/while Does it make sense?
On Tue, 30 Aug 2022, Henry Wang wrote: > > -----Original Message----- > > From: Michal Orzel <michal.orzel@amd.com> > > > > > > Oh I think get your point. Let me try to explain myself and thanks for your > > > patience :)) > > > > > > The reserved heap region defined in the device tree should be used for > > both > > > Xenheap and domain heap, so if we reserved a too small region (<32M), > > > an error should pop because the reserved region is not enough for > > xenheap, > > > and user should reserve more. > > > [...] > > > > > >> But your check is against heap being to small (less than 32M). > > >> So basically if the following check fails: > > >> "( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) )" > > >> it means that the heap region defined by a user is too small (not too large), > > >> because according to requirements it should be at least 32M. > > > > > > [...] > > > So in that case, printing "Not enough space for xenheap" means the > > reserved > > > region cannot satisfy the minimal requirement of the space of xenheap (at > > least > > > 32M), and this is in consistent with the check. > > > > Ok, it clearly depends on the way someone understands this sentence. > > Currently this panic can be triggered if the heap size is too large and > > should be read as "heap is too large to fit in because there is not enough > > space > > within RAM considering modules (e - s < size)". Usually (and also in this case) > > space refers to a region to contain another one. > > > > You are reusing the same message for different meaning, that is "user > > defined too > > small heap and this space (read as size) is not enough". > > Yes, thanks for the explanation. I think maybe rewording the message > to "Not enough memory for allocating xenheap" would remove the ambiguity > to some extent? Because the user-defined heap region should cover both > xenheap and domain heap at the same time, the small user-defined heap > means "xenheap is too large to fit in the user-defined heap region", which is > in consistent with your interpretation of the current "xenheap is too large to fit > in because there is not enough space within RAM considering modules" I think we should have a separate check specific for the device tree input parameters to make sure the region is correct, that way we can have a specific error message, such as: "xen,static-heap address needs to be 32MB aligned and the size a multiple of 32MB."
On Tue, 30 Aug 2022, Henry Wang wrote: > Hi Michal, > > > -----Original Message----- > > From: Michal Orzel <michal.orzel@amd.com> > > >>> > > >> Did you consider putting reserved_heap into bootinfo structure? > > > > > > Actually I did, but I saw current bootinfo only contains some structs so > > > I was not sure if this is the preferred way, but since you are raising this > > > question, I will follow this method in v2. > > This is what I think would be better but maintainers will have a decisive vote. > > Then let's wait for more input from maintainers. I don't have a strong preference and the way the current code is written, it would actually take less memory as is (the extra bool xen_heap comes for free.) I would keep the patch as is for now and for 4.17. If Julien prefers a refactoring of bootinfo/meminfo I think it could be done after the release if you are up to it.
Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > > > and we can automatically calculate xenheap_pages in a single line. > > > > Here I am a little bit confused. Sorry to ask but could you please explain > > a little bit more about why we can calculate the xenheap_pages in a single > > line? Below is the code snippet in my mind, is this correct? > > > > if (reserved_heap) > > coding style > > > e = reserved_heap_end; > > else > > { > > do > > { > > e = consider_modules(ram_start, ram_end, > > pfn_to_paddr(xenheap_pages), > > 32<<20, 0); > > if ( e ) > > break; > > > > xenheap_pages >>= 1; > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > PAGE_SHIFT) ); > > } > > Yes, this is what I meant. Thank you very much for your detailed explanation below! [...] > > But also, here the loop is also for adjusting xenheap_pages, and > xenheap_pages is initialized as follows: > > > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else > { > xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; > xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > } > > > In the reserved_heap case, it doesn't make sense to initialize > xenheap_pages like that, right? It should be something like: I am not sure about that, since we already have heap_pages = reserved_heap ? reserved_heap_pages : ram_pages; the heap_pages is supposed to contain domheap_pages + xenheap_pages based on the reserved heap definition discussed in the RFC. from the code in... > > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else if ( reserved_heap ) > xenheap_pages = heap_pages; ...here, setting xenheap_pages to heap_pages makes me a little bit confused. > else > { > xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; > xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > } If we keep this logic as this patch does, we can have the requirements... > > But also it looks like that on arm32 we have specific requirements for > Xen heap: > > * - must be 32 MiB aligned > * - must not include Xen itself or the boot modules > * - must be at most 1GB or 1/32 the total RAM in the system if less > * - must be at least 32M ...here, with the "1/32 the total RAM" now being "1/32 of the total reserved heap region", since heap_pages is now reserved_heap_pages. > > I think we should check at least the 32MB alignment and 32MB minimum > size before using the xen_heap bank. > > > In short I think this patch should: > > - add a check for 32MB alignment and size of the xen_heap memory bank > - if reserved_heap, set xenheap_pages = heap_pages > - if reserved_heap, skip the consider_modules do/while > > Does it make sense? I left some of my thoughts above to explain my understanding, but I might be wrong, thank you for your patience! Kind regards, Henry
Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > heap allocator > > On Tue, 30 Aug 2022, Henry Wang wrote: > > > -----Original Message----- > > > From: Michal Orzel <michal.orzel@amd.com> > > > > > > > > Oh I think get your point. Let me try to explain myself and thanks for > your > > > > patience :)) > > > > > > > > The reserved heap region defined in the device tree should be used for > > > both > > > > Xenheap and domain heap, so if we reserved a too small region (<32M), > > > > an error should pop because the reserved region is not enough for > > > xenheap, > > > > and user should reserve more. > > > > [...] > > > > > > > >> But your check is against heap being to small (less than 32M). > > > >> So basically if the following check fails: > > > >> "( reserved_heap && reserved_heap_pages < 32<<(20- > PAGE_SHIFT) ) )" > > > >> it means that the heap region defined by a user is too small (not too > large), > > > >> because according to requirements it should be at least 32M. > > > > > > > > [...] > > > > So in that case, printing "Not enough space for xenheap" means the > > > reserved > > > > region cannot satisfy the minimal requirement of the space of xenheap > (at > > > least > > > > 32M), and this is in consistent with the check. > > > > > > Ok, it clearly depends on the way someone understands this sentence. > > > Currently this panic can be triggered if the heap size is too large and > > > should be read as "heap is too large to fit in because there is not enough > > > space > > > within RAM considering modules (e - s < size)". Usually (and also in this > case) > > > space refers to a region to contain another one. > > > > > > You are reusing the same message for different meaning, that is "user > > > defined too > > > small heap and this space (read as size) is not enough". > > > > Yes, thanks for the explanation. I think maybe rewording the message > > to "Not enough memory for allocating xenheap" would remove the > ambiguity > > to some extent? Because the user-defined heap region should cover both > > xenheap and domain heap at the same time, the small user-defined heap > > means "xenheap is too large to fit in the user-defined heap region", which > is > > in consistent with your interpretation of the current "xenheap is too large > to fit > > in because there is not enough space within RAM considering modules" > > I think we should have a separate check specific for the device tree > input parameters to make sure the region is correct, that way we can > have a specific error message, such as: > > "xen,static-heap address needs to be 32MB aligned and the size a > multiple of 32MB." Sure, will follow this. Kind regards, Henry
Hi Stefano, > -----Original Message----- > From: Henry Wang > I am not sure about that, since we already have > heap_pages = reserved_heap ? reserved_heap_pages : ram_pages; > > the heap_pages is supposed to contain domheap_pages + xenheap_pages > based on the reserved heap definition discussed in the RFC. To add a little bit more about the background, here is the RFC discussion [1]. I should have attached this in my previous reply, sorry. [1] https://lore.kernel.org/xen-devel/316007B7-51BA-4820-8F6F-018BC6D3A077@arm.com/ Kind regards, Henry
On Wed, 31 Aug 2022, Henry Wang wrote: > > -----Original Message----- > > From: Stefano Stabellini <sstabellini@kernel.org> > > > > and we can automatically calculate xenheap_pages in a single line. > > > > > > Here I am a little bit confused. Sorry to ask but could you please explain > > > a little bit more about why we can calculate the xenheap_pages in a single > > > line? Below is the code snippet in my mind, is this correct? > > > > > > if (reserved_heap) > > > > coding style > > > > > e = reserved_heap_end; > > > else > > > { > > > do > > > { > > > e = consider_modules(ram_start, ram_end, > > > pfn_to_paddr(xenheap_pages), > > > 32<<20, 0); > > > if ( e ) > > > break; > > > > > > xenheap_pages >>= 1; > > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > > PAGE_SHIFT) ); > > > } > > > > Yes, this is what I meant. > > Thank you very much for your detailed explanation below! > [...] > > > > > But also, here the loop is also for adjusting xenheap_pages, and > > xenheap_pages is initialized as follows: > > > > > > if ( opt_xenheap_megabytes ) > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > else > > { > > xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; > > xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); > > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > > } > > > > > > In the reserved_heap case, it doesn't make sense to initialize > > xenheap_pages like that, right? It should be something like: > > I am not sure about that, since we already have > heap_pages = reserved_heap ? reserved_heap_pages : ram_pages; > > the heap_pages is supposed to contain domheap_pages + xenheap_pages > based on the reserved heap definition discussed in the RFC. > > from the code in... > > > > > if ( opt_xenheap_megabytes ) > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > else if ( reserved_heap ) > > xenheap_pages = heap_pages; > > ...here, setting xenheap_pages to heap_pages makes me a little bit > confused. > > > else > > { > > xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; > > xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); > > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > > } > > If we keep this logic as this patch does, we can have the requirements... > > > > > But also it looks like that on arm32 we have specific requirements for > > Xen heap: > > > > * - must be 32 MiB aligned > > * - must not include Xen itself or the boot modules > > * - must be at most 1GB or 1/32 the total RAM in the system if less > > * - must be at least 32M > > ...here, with the "1/32 the total RAM" now being "1/32 of the total reserved > heap region", since heap_pages is now reserved_heap_pages. I see. I didn't realize the full implications of the memory being used for both xenheap and domheap on arm32. In that case, I would simply do the following: heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; if ( opt_xenheap_megabytes ) xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); else { xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); } if ( reserved_heap ) e = reserved_heap_end; else { do { e = consider_modules(ram_start, ram_end, pfn_to_paddr(xenheap_pages), 32<<20, 0); if ( e ) break; xenheap_pages >>= 1; } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); } if ( ! e || ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) panic("Not enough space for xenheap\n"); domheap_pages = heap_pages - xenheap_pages;
Hi Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > > > But also it looks like that on arm32 we have specific requirements for > > > Xen heap: > > > > > > * - must be 32 MiB aligned > > > * - must not include Xen itself or the boot modules > > > * - must be at most 1GB or 1/32 the total RAM in the system if less > > > * - must be at least 32M > > > > ...here, with the "1/32 the total RAM" now being "1/32 of the total reserved > > heap region", since heap_pages is now reserved_heap_pages. > > I see. I didn't realize the full implications of the memory being used > for both xenheap and domheap on arm32. In that case, I would simply do > the following: > > > heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else > { > xenheap_pages = (heap_pages/32 + 0x1fffUL) & ~0x1fffUL; > xenheap_pages = max(xenheap_pages, 32UL<<(20-PAGE_SHIFT)); > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > } > > if ( reserved_heap ) > e = reserved_heap_end; > else > { > do > { > e = consider_modules(ram_start, ram_end, > pfn_to_paddr(xenheap_pages), > 32<<20, 0); > > if ( e ) > break; > > xenheap_pages >>= 1; > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > PAGE_SHIFT) ); > } > > if ( ! e || > ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > panic("Not enough space for xenheap\n"); > > domheap_pages = heap_pages - xenheap_pages; Thanks very much for your time and patience. I will follow this way - with the comment also updated of course (I didn't realize the comment needs to be changed until yesterday when I sent the reply to your last comment.) Kind regards, Henry
Hi Arm maintainers, > -----Original Message----- > Hi Henry, > > On 30/08/2022 08:11, Henry Wang wrote: > > > > Hi Michal, > > > > Sorry about the late reply - I had a couple of days off. Thank you very > > much for the review! I will add my reply and answer some of your > > questions below. > > > >> -----Original Message----- > >> From: Michal Orzel <michal.orzel@amd.com> > >> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot > and > >> heap allocator > >> > >>> This commit firstly adds a global variable `reserved_heap`. > >>> This newly introduced global variable is set at the device tree > >>> parsing time if the reserved heap ranges are defined in the device > >>> tree chosen node. > >>> > >> Did you consider putting reserved_heap into bootinfo structure? > > > > Actually I did, but I saw current bootinfo only contains some structs so > > I was not sure if this is the preferred way, but since you are raising this > > question, I will follow this method in v2. > > This is what I think would be better but maintainers will have a decisive vote. I think this is the only uncertain comment that I received during the latest review of this series. I agree that Michal is making a good point (Thanks!) but we are curious about what maintainers think. Could you please kindly share your opinion on the more preferred approach? I will do that in v2. Thanks very much! Kind regards, Henry
Hi, > On 1 Sep 2022, at 02:03, Henry Wang <Henry.Wang@arm.com> wrote: > > Hi Arm maintainers, > >> -----Original Message----- >> Hi Henry, >> >> On 30/08/2022 08:11, Henry Wang wrote: >>> >>> Hi Michal, >>> >>> Sorry about the late reply - I had a couple of days off. Thank you very >>> much for the review! I will add my reply and answer some of your >>> questions below. >>> >>>> -----Original Message----- >>>> From: Michal Orzel <michal.orzel@amd.com> >>>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot >> and >>>> heap allocator >>>> >>>>> This commit firstly adds a global variable `reserved_heap`. >>>>> This newly introduced global variable is set at the device tree >>>>> parsing time if the reserved heap ranges are defined in the device >>>>> tree chosen node. >>>>> >>>> Did you consider putting reserved_heap into bootinfo structure? >>> >>> Actually I did, but I saw current bootinfo only contains some structs so >>> I was not sure if this is the preferred way, but since you are raising this >>> question, I will follow this method in v2. >> >> This is what I think would be better but maintainers will have a decisive vote. > > I think this is the only uncertain comment that I received during the latest > review of this series. I agree that Michal is making a good point (Thanks!) but we > are curious about what maintainers think. Could you please kindly share your > opinion on the more preferred approach? I will do that in v2. Thanks very much! I think it does make sense to put this in bootinfo. Cheers Bertrand
Hi Bertrand, > -----Original Message----- > From: Bertrand Marquis <Bertrand.Marquis@arm.com> > > On 1 Sep 2022, at 02:03, Henry Wang <Henry.Wang@arm.com> wrote: > > > > Hi Arm maintainers, > > > >> -----Original Message----- > >> Hi Henry, > >> > >> On 30/08/2022 08:11, Henry Wang wrote: > >>> > >>> Hi Michal, > >>> > >>> Sorry about the late reply - I had a couple of days off. Thank you very > >>> much for the review! I will add my reply and answer some of your > >>> questions below. > >>> > >>>> -----Original Message----- > >>>> From: Michal Orzel <michal.orzel@amd.com> > >>>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot > >> and > >>>> heap allocator > >>>> > >>>>> This commit firstly adds a global variable `reserved_heap`. > >>>>> This newly introduced global variable is set at the device tree > >>>>> parsing time if the reserved heap ranges are defined in the device > >>>>> tree chosen node. > >>>>> > >>>> Did you consider putting reserved_heap into bootinfo structure? > >>> > >>> Actually I did, but I saw current bootinfo only contains some structs so > >>> I was not sure if this is the preferred way, but since you are raising this > >>> question, I will follow this method in v2. > >> > >> This is what I think would be better but maintainers will have a decisive > vote. > > > > I think this is the only uncertain comment that I received during the latest > > review of this series. I agree that Michal is making a good point (Thanks!) > but we > > are curious about what maintainers think. Could you please kindly share > your > > opinion on the more preferred approach? I will do that in v2. Thanks very > much! > > I think it does make sense to put this in bootinfo. I am good with that, then I think I will move this to bootinfo in v2 unless other objections. Thank you for the input. Kind regards, Henry > > Cheers > Bertrand >
On 30/08/2022 02:04, Stefano Stabellini wrote: >> size_t estimate_efi_size(unsigned int mem_nr_banks); >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 500307edc0..fe76cf6325 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); >> >> domid_t __read_mostly max_init_domid; >> >> +bool __read_mostly reserved_heap; >> + >> static __used void init_done(void) >> { >> /* Must be done past setting system_state. */ >> @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) >> #ifdef CONFIG_ARM_32 >> static void __init setup_mm(void) >> { >> - paddr_t ram_start, ram_end, ram_size, e; >> - unsigned long ram_pages; >> + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size; >> + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > > INVALID_PADDR or ~0ULL I would strongly prefer the former. It is more difficult to understand what's the value means in the latter. Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > >> + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > > > > INVALID_PADDR or ~0ULL > > I would strongly prefer the former. It is more difficult to understand > what's the value means in the latter. Thanks for the input. You mean the INVALID_PADDR correct? Yeah this is the one that I used in my local v2, will send it tomorrow after doing the bootinfo change. Kind regards, Henry > > Cheers, > > -- > Julien Grall
On 01/09/2022 15:01, Henry Wang wrote: > Hi Julien, > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >>>> + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, >>> >>> INVALID_PADDR or ~0ULL >> >> I would strongly prefer the former. It is more difficult to understand >> what's the value means in the latter. > > Thanks for the input. You mean the INVALID_PADDR correct? That's correct. Cheers,
Hi Henry, On 24/08/2022 08:31, Henry Wang wrote: > This commit firstly adds a global variable `reserved_heap`. > This newly introduced global variable is set at the device tree > parsing time if the reserved heap ranges are defined in the device > tree chosen node. > > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use > the reserved heap region for both domheap and xenheap allocation. > > For Arm64, In `setup_mm`, if the reserved heap is enabled and used, > we make sure that only these reserved heap pages are added to the > boot allocator. These reserved heap pages in the boot allocator are > added to the heap allocator at `end_boot_allocator()`. > > If the reserved heap is disabled, we stick to current page allocation > strategy at boot time. > > Also, take the chance to correct a "double not" print in Arm32 > `setup_mm()`. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > With reserved heap enabled, for Arm64, naming of global variables such > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous, > wondering if we should rename these variables. > --- > Changes from RFC to v1: > - Rebase on top of latest `setup_mm()` changes. > - Added Arm32 logic in `setup_mm()`. > --- > xen/arch/arm/bootfdt.c | 2 + > xen/arch/arm/include/asm/setup.h | 2 + > xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++------- > 3 files changed, 67 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 33704ca487..ab73b6e212 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node, > true); > if ( rc ) > return rc; > + > + reserved_heap = true; > } > > printk("Checking for initrd in /chosen\n"); > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index e80f3d6201..00536a6d55 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo; > > extern domid_t max_init_domid; > > +extern bool reserved_heap; > + > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); > > size_t estimate_efi_size(unsigned int mem_nr_banks); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 500307edc0..fe76cf6325 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); > > domid_t __read_mostly max_init_domid; > > +bool __read_mostly reserved_heap; > + > static __used void init_done(void) > { > /* Must be done past setting system_state. */ > @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) > #ifdef CONFIG_ARM_32 > static void __init setup_mm(void) > { > - paddr_t ram_start, ram_end, ram_size, e; > - unsigned long ram_pages; > + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size; > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > + reserved_heap_size = 0; > + unsigned long ram_pages, reserved_heap_pages = 0; > unsigned long heap_pages, xenheap_pages, domheap_pages; > unsigned int i; > const uint32_t ctr = READ_CP32(CTR); > @@ -720,9 +724,9 @@ static void __init setup_mm(void) > > for ( i = 1; i < bootinfo.mem.nr_banks; i++ ) > { > - paddr_t bank_start = bootinfo.mem.bank[i].start; > - paddr_t bank_size = bootinfo.mem.bank[i].size; > - paddr_t bank_end = bank_start + bank_size; > + bank_start = bootinfo.mem.bank[i].start; > + bank_size = bootinfo.mem.bank[i].size; > + bank_end = bank_start + bank_size; > > ram_size = ram_size + bank_size; > ram_start = min(ram_start,bank_start); > @@ -731,6 +735,25 @@ static void __init setup_mm(void) > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > + if ( reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + reserved_heap_size += bank_size; > + reserved_heap_start = min(reserved_heap_start, bank_start); > + reserved_heap_end = max(reserved_heap_end, bank_end); > + } > + } > + > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > + } > + > /* > * If the user has not requested otherwise via the command line > * then locate the xenheap using these constraints: > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > * We try to allocate the largest xenheap possible within these > * constraints. > */ > - heap_pages = ram_pages; > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > + > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > do > { > - e = consider_modules(ram_start, ram_end, > + e = !reserved_heap ? > + consider_modules(ram_start, ram_end, > pfn_to_paddr(xenheap_pages), > - 32<<20, 0); > + 32<<20, 0) : > + reserved_heap_end; Not entirely related to this series. Now the assumption is the admin will make sure that none of the reserved regions will overlap. Do we have any tool to help the admin to verify it? If yes, can we have a pointer in the documentation? If not, should this be done in Xen? Also, what happen with UEFI? Is it easy to guarantee the region will not be used? > + > if ( e ) > break; > > xenheap_pages >>= 1; > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); > > - if ( ! e ) > - panic("Not not enough space for xenheap\n"); > + if ( ! e || > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > + panic("Not enough space for xenheap\n"); So on arm32, the xenheap *must* be contiguous. AFAICT, reserved_heap_pages is the total number of pages in the heap. They may not be contiguous. So I think this wants to be reworked so we look for one of the region that match the definition written above the loop. > > domheap_pages = heap_pages - xenheap_pages; > > @@ -810,9 +838,9 @@ static void __init setup_mm(void) > static void __init setup_mm(void) > { > const struct meminfo *banks = &bootinfo.mem; > - paddr_t ram_start = ~0; > - paddr_t ram_end = 0; > - paddr_t ram_size = 0; > + paddr_t ram_start = ~0, bank_start = ~0; > + paddr_t ram_end = 0, bank_end = 0; > + paddr_t ram_size = 0, bank_size = 0; > unsigned int i; > > init_pdx(); > @@ -821,17 +849,36 @@ static void __init setup_mm(void) > * We need some memory to allocate the page-tables used for the xenheap > * mappings. But some regions may contain memory already allocated > * for other uses (e.g. modules, reserved-memory...). > - * > + * If reserved heap regions are properly defined, (only) add these regions > + * in the boot allocator. > + */ > + if ( reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + init_boot_pages(bank_start, bank_end); > + } > + } > + } > + /* > + * No reserved heap regions: > * For simplicity, add all the free regions in the boot allocator. > */ > - populate_boot_allocator(); > + else > + populate_boot_allocator(); > > total_pages = 0; > > for ( i = 0; i < banks->nr_banks; i++ ) > { This code is now becoming quite confusing to understanding. This loop is meant to map the xenheap. If I follow your documentation, it would mean that only the reserved region should be mapped. More confusingly, xenheap_* variables will cover the full RAM. Effectively, this is now more obvious that this is use for direct-mapping. So I think it would be better to rename the variable to directmap_* or similar. Ideally this should be in a separate patch. Cheers,
Hi Julien, Thanks for your review. > -----Original Message----- > From: Julien Grall <julien@xen.org> > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > heap allocator > > Hi Henry, > > On 24/08/2022 08:31, Henry Wang wrote: > > This commit firstly adds a global variable `reserved_heap`. > > This newly introduced global variable is set at the device tree > > parsing time if the reserved heap ranges are defined in the device > > tree chosen node. > > > > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use > > the reserved heap region for both domheap and xenheap allocation. > > > > For Arm64, In `setup_mm`, if the reserved heap is enabled and used, > > we make sure that only these reserved heap pages are added to the > > boot allocator. These reserved heap pages in the boot allocator are > > added to the heap allocator at `end_boot_allocator()`. > > > > If the reserved heap is disabled, we stick to current page allocation > > strategy at boot time. > > > > Also, take the chance to correct a "double not" print in Arm32 > > `setup_mm()`. > > > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > --- > > With reserved heap enabled, for Arm64, naming of global variables such > > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous, > > wondering if we should rename these variables. > > --- > > Changes from RFC to v1: > > - Rebase on top of latest `setup_mm()` changes. > > - Added Arm32 logic in `setup_mm()`. > > --- > > xen/arch/arm/bootfdt.c | 2 + > > xen/arch/arm/include/asm/setup.h | 2 + > > xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++------- > > 3 files changed, 67 insertions(+), 16 deletions(-) > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > index 33704ca487..ab73b6e212 100644 > > --- a/xen/arch/arm/bootfdt.c > > +++ b/xen/arch/arm/bootfdt.c > > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void > *fdt, int node, > > true); > > if ( rc ) > > return rc; > > + > > + reserved_heap = true; > > } > > > > printk("Checking for initrd in /chosen\n"); > > diff --git a/xen/arch/arm/include/asm/setup.h > b/xen/arch/arm/include/asm/setup.h > > index e80f3d6201..00536a6d55 100644 > > --- a/xen/arch/arm/include/asm/setup.h > > +++ b/xen/arch/arm/include/asm/setup.h > > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo; > > > > extern domid_t max_init_domid; > > > > +extern bool reserved_heap; > > + > > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); > > > > size_t estimate_efi_size(unsigned int mem_nr_banks); > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > index 500307edc0..fe76cf6325 100644 > > --- a/xen/arch/arm/setup.c > > +++ b/xen/arch/arm/setup.c > > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", > opt_xenheap_megabytes); > > > > domid_t __read_mostly max_init_domid; > > > > +bool __read_mostly reserved_heap; > > + > > static __used void init_done(void) > > { > > /* Must be done past setting system_state. */ > > @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) > > #ifdef CONFIG_ARM_32 > > static void __init setup_mm(void) > > { > > - paddr_t ram_start, ram_end, ram_size, e; > > - unsigned long ram_pages; > > + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, > bank_size; > > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > > + reserved_heap_size = 0; > > + unsigned long ram_pages, reserved_heap_pages = 0; > > unsigned long heap_pages, xenheap_pages, domheap_pages; > > unsigned int i; > > const uint32_t ctr = READ_CP32(CTR); > > @@ -720,9 +724,9 @@ static void __init setup_mm(void) > > > > for ( i = 1; i < bootinfo.mem.nr_banks; i++ ) > > { > > - paddr_t bank_start = bootinfo.mem.bank[i].start; > > - paddr_t bank_size = bootinfo.mem.bank[i].size; > > - paddr_t bank_end = bank_start + bank_size; > > + bank_start = bootinfo.mem.bank[i].start; > > + bank_size = bootinfo.mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > > > ram_size = ram_size + bank_size; > > ram_start = min(ram_start,bank_start); > > @@ -731,6 +735,25 @@ static void __init setup_mm(void) > > > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > > > + if ( reserved_heap ) > > + { > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > > + { > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > + > > + reserved_heap_size += bank_size; > > + reserved_heap_start = min(reserved_heap_start, bank_start); > > + reserved_heap_end = max(reserved_heap_end, bank_end); > > + } > > + } > > + > > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > > + } > > + > > /* > > * If the user has not requested otherwise via the command line > > * then locate the xenheap using these constraints: > > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > > * We try to allocate the largest xenheap possible within these > > * constraints. > > */ > > - heap_pages = ram_pages; > > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > > + > > if ( opt_xenheap_megabytes ) > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > else > > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > > > do > > { > > - e = consider_modules(ram_start, ram_end, > > + e = !reserved_heap ? > > + consider_modules(ram_start, ram_end, > > pfn_to_paddr(xenheap_pages), > > - 32<<20, 0); > > + 32<<20, 0) : > > + reserved_heap_end; > > Not entirely related to this series. Now the assumption is the admin > will make sure that none of the reserved regions will overlap. > > Do we have any tool to help the admin to verify it? If yes, can we have > a pointer in the documentation? If not, should this be done in Xen? In the RFC we had the same discussion of this issue [1] and I think a follow-up series might needed to do the overlap check if we want to do that in Xen. For the existing tool, I am thinking of ImageBuilder, but I am curious about Stefano's opinion. > > Also, what happen with UEFI? Is it easy to guarantee the region will not > be used? For now I think it is not easy to guarantee that, do you have some ideas in mind? I think I can follow this in above follow-up series to improve things. > > > + > > if ( e ) > > break; > > > > xenheap_pages >>= 1; > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > PAGE_SHIFT) ); > > > > - if ( ! e ) > > - panic("Not not enough space for xenheap\n"); > > + if ( ! e || > > + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) > > + panic("Not enough space for xenheap\n"); > > So on arm32, the xenheap *must* be contiguous. AFAICT, > reserved_heap_pages is the total number of pages in the heap. They may > not be contiguous. So I think this wants to be reworked so we look for > one of the region that match the definition written above the loop. Thanks for raising this concern, I will do this in V2. > > > > > domheap_pages = heap_pages - xenheap_pages; > > > > @@ -810,9 +838,9 @@ static void __init setup_mm(void) > > static void __init setup_mm(void) > > { > > const struct meminfo *banks = &bootinfo.mem; > > - paddr_t ram_start = ~0; > > - paddr_t ram_end = 0; > > - paddr_t ram_size = 0; > > + paddr_t ram_start = ~0, bank_start = ~0; > > + paddr_t ram_end = 0, bank_end = 0; > > + paddr_t ram_size = 0, bank_size = 0; > > unsigned int i; > > > > init_pdx(); > > @@ -821,17 +849,36 @@ static void __init setup_mm(void) > > * We need some memory to allocate the page-tables used for the > xenheap > > * mappings. But some regions may contain memory already allocated > > * for other uses (e.g. modules, reserved-memory...). > > - * > > + * If reserved heap regions are properly defined, (only) add these > regions > > + * in the boot allocator. > + */ > > + if ( reserved_heap ) > > + { > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > > + { > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > + > > + init_boot_pages(bank_start, bank_end); > > + } > > + } > > + } > > + /* > > + * No reserved heap regions: > > * For simplicity, add all the free regions in the boot allocator. > > */ > > - populate_boot_allocator(); > > + else > > + populate_boot_allocator(); > > > > total_pages = 0; > > > > for ( i = 0; i < banks->nr_banks; i++ ) > > { > > This code is now becoming quite confusing to understanding. This loop is > meant to map the xenheap. If I follow your documentation, it would mean > that only the reserved region should be mapped. Yes I think this is the same question that I raised in the scissors line of the commit message of this patch. What I intend to do is still mapping the whole RAM because of the xenheap_* variables that you mentioned in... > > More confusingly, xenheap_* variables will cover the full RAM. ...here. But only adding the reserved region to the boot allocator so the reserved region can become the heap later on. I am wondering if we have a more clear way to do that, any suggestions? > Effectively, this is now more obvious that this is use for > direct-mapping. So I think it would be better to rename the variable to > directmap_* or similar. > > Ideally this should be in a separate patch. [1] https://lore.kernel.org/xen-devel/48a0712c-eff8-dfc1-2136-59317f22321f@xen.org/ Kind regards, Henry > > Cheers, > > -- > Julien Grall
On Thu, 1 Sep 2022, Henry Wang wrote: > > -----Original Message----- > > From: Julien Grall <julien@xen.org> > > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > > heap allocator > > > > Hi Henry, > > > > On 24/08/2022 08:31, Henry Wang wrote: > > > This commit firstly adds a global variable `reserved_heap`. > > > This newly introduced global variable is set at the device tree > > > parsing time if the reserved heap ranges are defined in the device > > > tree chosen node. > > > > > > For Arm32, In `setup_mm`, if the reserved heap is enabled, we use > > > the reserved heap region for both domheap and xenheap allocation. > > > > > > For Arm64, In `setup_mm`, if the reserved heap is enabled and used, > > > we make sure that only these reserved heap pages are added to the > > > boot allocator. These reserved heap pages in the boot allocator are > > > added to the heap allocator at `end_boot_allocator()`. > > > > > > If the reserved heap is disabled, we stick to current page allocation > > > strategy at boot time. > > > > > > Also, take the chance to correct a "double not" print in Arm32 > > > `setup_mm()`. > > > > > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > > > --- > > > With reserved heap enabled, for Arm64, naming of global variables such > > > as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous, > > > wondering if we should rename these variables. > > > --- > > > Changes from RFC to v1: > > > - Rebase on top of latest `setup_mm()` changes. > > > - Added Arm32 logic in `setup_mm()`. > > > --- > > > xen/arch/arm/bootfdt.c | 2 + > > > xen/arch/arm/include/asm/setup.h | 2 + > > > xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++------- > > > 3 files changed, 67 insertions(+), 16 deletions(-) > > > > > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > > > index 33704ca487..ab73b6e212 100644 > > > --- a/xen/arch/arm/bootfdt.c > > > +++ b/xen/arch/arm/bootfdt.c > > > @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void > > *fdt, int node, > > > true); > > > if ( rc ) > > > return rc; > > > + > > > + reserved_heap = true; > > > } > > > > > > printk("Checking for initrd in /chosen\n"); > > > diff --git a/xen/arch/arm/include/asm/setup.h > > b/xen/arch/arm/include/asm/setup.h > > > index e80f3d6201..00536a6d55 100644 > > > --- a/xen/arch/arm/include/asm/setup.h > > > +++ b/xen/arch/arm/include/asm/setup.h > > > @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo; > > > > > > extern domid_t max_init_domid; > > > > > > +extern bool reserved_heap; > > > + > > > void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); > > > > > > size_t estimate_efi_size(unsigned int mem_nr_banks); > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > > index 500307edc0..fe76cf6325 100644 > > > --- a/xen/arch/arm/setup.c > > > +++ b/xen/arch/arm/setup.c > > > @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", > > opt_xenheap_megabytes); > > > > > > domid_t __read_mostly max_init_domid; > > > > > > +bool __read_mostly reserved_heap; > > > + > > > static __used void init_done(void) > > > { > > > /* Must be done past setting system_state. */ > > > @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) > > > #ifdef CONFIG_ARM_32 > > > static void __init setup_mm(void) > > > { > > > - paddr_t ram_start, ram_end, ram_size, e; > > > - unsigned long ram_pages; > > > + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, > > bank_size; > > > + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, > > > + reserved_heap_size = 0; > > > + unsigned long ram_pages, reserved_heap_pages = 0; > > > unsigned long heap_pages, xenheap_pages, domheap_pages; > > > unsigned int i; > > > const uint32_t ctr = READ_CP32(CTR); > > > @@ -720,9 +724,9 @@ static void __init setup_mm(void) > > > > > > for ( i = 1; i < bootinfo.mem.nr_banks; i++ ) > > > { > > > - paddr_t bank_start = bootinfo.mem.bank[i].start; > > > - paddr_t bank_size = bootinfo.mem.bank[i].size; > > > - paddr_t bank_end = bank_start + bank_size; > > > + bank_start = bootinfo.mem.bank[i].start; > > > + bank_size = bootinfo.mem.bank[i].size; > > > + bank_end = bank_start + bank_size; > > > > > > ram_size = ram_size + bank_size; > > > ram_start = min(ram_start,bank_start); > > > @@ -731,6 +735,25 @@ static void __init setup_mm(void) > > > > > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > > > > > + if ( reserved_heap ) > > > + { > > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > > + { > > > + if ( bootinfo.reserved_mem.bank[i].xen_heap ) > > > + { > > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > > + bank_end = bank_start + bank_size; > > > + > > > + reserved_heap_size += bank_size; > > > + reserved_heap_start = min(reserved_heap_start, bank_start); > > > + reserved_heap_end = max(reserved_heap_end, bank_end); > > > + } > > > + } > > > + > > > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > > > + } > > > + > > > /* > > > * If the user has not requested otherwise via the command line > > > * then locate the xenheap using these constraints: > > > @@ -743,7 +766,8 @@ static void __init setup_mm(void) > > > * We try to allocate the largest xenheap possible within these > > > * constraints. > > > */ > > > - heap_pages = ram_pages; > > > + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; > > > + > > > if ( opt_xenheap_megabytes ) > > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > > else > > > @@ -755,17 +779,21 @@ static void __init setup_mm(void) > > > > > > do > > > { > > > - e = consider_modules(ram_start, ram_end, > > > + e = !reserved_heap ? > > > + consider_modules(ram_start, ram_end, > > > pfn_to_paddr(xenheap_pages), > > > - 32<<20, 0); > > > + 32<<20, 0) : > > > + reserved_heap_end; > > > > Not entirely related to this series. Now the assumption is the admin > > will make sure that none of the reserved regions will overlap. > > > > Do we have any tool to help the admin to verify it? If yes, can we have > > a pointer in the documentation? If not, should this be done in Xen? > > In the RFC we had the same discussion of this issue [1] and I think a > follow-up series might needed to do the overlap check if we want to > do that in Xen. For the existing tool, I am thinking of ImageBuilder, but > I am curious about Stefano's opinion. Yes, ImageBuilder is a good option and we moved ImageBuilder under Xen Project to make it easier for people to contribute to it: https://gitlab.com/xen-project/imagebuilder > > Also, what happen with UEFI? Is it easy to guarantee the region will not > > be used? > > For now I think it is not easy to guarantee that, do you have some ideas > in mind? I think I can follow this in above follow-up series to improve things. For clarity, are we worried that the region is used by the bootloader for other things? For instance U-Boot or Tianocore placing some firmware tables inside the range specified for xenheap?
Hi Henry, On 01/09/2022 17:05, Henry Wang wrote: >>> @@ -755,17 +779,21 @@ static void __init setup_mm(void) >>> >>> do >>> { >>> - e = consider_modules(ram_start, ram_end, >>> + e = !reserved_heap ? >>> + consider_modules(ram_start, ram_end, >>> pfn_to_paddr(xenheap_pages), >>> - 32<<20, 0); >>> + 32<<20, 0) : >>> + reserved_heap_end; >> >> Not entirely related to this series. Now the assumption is the admin >> will make sure that none of the reserved regions will overlap. >> >> Do we have any tool to help the admin to verify it? If yes, can we have >> a pointer in the documentation? If not, should this be done in Xen? > > In the RFC we had the same discussion of this issue [1] and I think a > follow-up series might needed to do the overlap check if we want to > do that in Xen. For the existing tool, I am thinking of ImageBuilder, but > I am curious about Stefano's opinion. > >> >> Also, what happen with UEFI? Is it easy to guarantee the region will not >> be used? > > For now I think it is not easy to guarantee that, do you have some ideas > in mind? I think I can follow this in above follow-up series to improve things. I don't have any ideas how we can guarantee it (even when using image builder). I think we may have to end up to check the overlaps in Xen. > >> >>> + >>> if ( e ) >>> break; >>> >>> xenheap_pages >>= 1; >>> } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- >> PAGE_SHIFT) ); >>> >>> - if ( ! e ) >>> - panic("Not not enough space for xenheap\n"); >>> + if ( ! e || >>> + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) >>> + panic("Not enough space for xenheap\n"); >> >> So on arm32, the xenheap *must* be contiguous. AFAICT, >> reserved_heap_pages is the total number of pages in the heap. They may >> not be contiguous. So I think this wants to be reworked so we look for >> one of the region that match the definition written above the loop. > > Thanks for raising this concern, I will do this in V2. > >> >>> >>> domheap_pages = heap_pages - xenheap_pages; >>> >>> @@ -810,9 +838,9 @@ static void __init setup_mm(void) >>> static void __init setup_mm(void) >>> { >>> const struct meminfo *banks = &bootinfo.mem; >>> - paddr_t ram_start = ~0; >>> - paddr_t ram_end = 0; >>> - paddr_t ram_size = 0; >>> + paddr_t ram_start = ~0, bank_start = ~0; >>> + paddr_t ram_end = 0, bank_end = 0; >>> + paddr_t ram_size = 0, bank_size = 0; >>> unsigned int i; >>> >>> init_pdx(); >>> @@ -821,17 +849,36 @@ static void __init setup_mm(void) >>> * We need some memory to allocate the page-tables used for the >> xenheap >>> * mappings. But some regions may contain memory already allocated >>> * for other uses (e.g. modules, reserved-memory...). >>> - * >>> + * If reserved heap regions are properly defined, (only) add these >> regions >>> + * in the boot allocator. > + */ >>> + if ( reserved_heap ) >>> + { >>> + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) >>> + { >>> + if ( bootinfo.reserved_mem.bank[i].xen_heap ) >>> + { >>> + bank_start = bootinfo.reserved_mem.bank[i].start; >>> + bank_size = bootinfo.reserved_mem.bank[i].size; >>> + bank_end = bank_start + bank_size; >>> + >>> + init_boot_pages(bank_start, bank_end); >>> + } >>> + } >>> + } >>> + /* >>> + * No reserved heap regions: >>> * For simplicity, add all the free regions in the boot allocator. >>> */ >>> - populate_boot_allocator(); >>> + else >>> + populate_boot_allocator(); >>> >>> total_pages = 0; >>> >>> for ( i = 0; i < banks->nr_banks; i++ ) >>> { >> >> This code is now becoming quite confusing to understanding. This loop is >> meant to map the xenheap. If I follow your documentation, it would mean >> that only the reserved region should be mapped. > > Yes I think this is the same question that I raised in the scissors line of the > commit message of this patch. Sorry I didn't notice the comment after the scissors line. This is the same question :) > What I intend to do is still mapping the whole > RAM because of the xenheap_* variables that you mentioned in... > >> >> More confusingly, xenheap_* variables will cover the full RAM. > > ...here. But only adding the reserved region to the boot allocator so the > reserved region can become the heap later on. I am wondering if we > have a more clear way to do that, any suggestions? I think your code is correct. It only needs some renaming of the existing variable (maybe to directmap_*?) to make clear the area is used to access the RAM easily. Cheers,
Hi Stefano, On 01/09/2022 18:08, Stefano Stabellini wrote: >>> Also, what happen with UEFI? Is it easy to guarantee the region will not >>> be used? >> >> For now I think it is not easy to guarantee that, do you have some ideas >> in mind? I think I can follow this in above follow-up series to improve things. > > For clarity, are we worried that the region is used by the bootloader > for other things? For instance U-Boot or Tianocore placing some > firmware tables inside the range specified for xenheap? Yes. I think it would be difficult for an admin to figure out which regions are used. Although they are likely (?) going to be static for a given UEFI/U-boot build. My major concern is such bug can be very difficult to root cause because we have no safety in Xen. The most likely symptom would be random corruption. Cheers,
On Thu, 1 Sep 2022, Julien Grall wrote: > Hi Stefano, > > On 01/09/2022 18:08, Stefano Stabellini wrote: > > > > Also, what happen with UEFI? Is it easy to guarantee the region will not > > > > be used? > > > > > > For now I think it is not easy to guarantee that, do you have some ideas > > > in mind? I think I can follow this in above follow-up series to improve > > > things. > > > > For clarity, are we worried that the region is used by the bootloader > > for other things? For instance U-Boot or Tianocore placing some > > firmware tables inside the range specified for xenheap? > > Yes. I think it would be difficult for an admin to figure out which regions > are used. Although they are likely (?) going to be static for a given > UEFI/U-boot build. > > My major concern is such bug can be very difficult to root cause because we > have no safety in Xen. The most likely symptom would be random corruption. Thanks for the clarification. Yeah, I think we'll have to do some "creative thinking" to figure out a solution to this issue. I wonder if U-boot or Tianocore have some sort of API (or build-time data) to know the unavailable ranges. In any case, I think we can postpone to after the release.
Hi Julien and Stefano, > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: 2022年9月2日 9:51 > To: Julien Grall <julien@xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang > <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis > <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > heap allocator > > On Thu, 1 Sep 2022, Julien Grall wrote: > > Hi Stefano, > > > > On 01/09/2022 18:08, Stefano Stabellini wrote: > > > > > Also, what happen with UEFI? Is it easy to guarantee the region > will not > > > > > be used? > > > > > > > > For now I think it is not easy to guarantee that, do you have some > ideas > > > > in mind? I think I can follow this in above follow-up series to > improve > > > > things. > > > > > > For clarity, are we worried that the region is used by the bootloader > > > for other things? For instance U-Boot or Tianocore placing some > > > firmware tables inside the range specified for xenheap? > > > > Yes. I think it would be difficult for an admin to figure out which > regions > > are used. Although they are likely (?) going to be static for a given > > UEFI/U-boot build. > > > > My major concern is such bug can be very difficult to root cause because > we > > have no safety in Xen. The most likely symptom would be random > corruption. > > Thanks for the clarification. Yeah, I think we'll have to do some > "creative thinking" to figure out a solution to this issue. I wonder if > U-boot or Tianocore have some sort of API (or build-time data) to know > the unavailable ranges. > When Xen is booted through EFI, all the memory statically defined in the Device tree has a certain probability of conflicting with the memory occupied by EFI. This is difficult to avoid without the EFI bootloader intervening, but it is possible to detect such a conflict. For EFI reserved memory regions (like runtime service), they will not be reported as usable memory to Xen. The usable memory regions will be added to bootinfo.memblk as device tree boot. So I think all static defined memory regions would be check with bootinfo.memblk to find the conflict. For EFI relocate Xen and DTB, I think Xen itself can know these addresses easily and easy to check. But if we don't add code to uboot or EDK2, what can we do are just check, print error and panic. But starting from the actual usage scenario, because the scenarios using static heap are normally highly customized, their EFI firmware will bypass the static memory we defined in device tree when customizing. So maybe check conflict is enough? Cheers, Wei Chen > In any case, I think we can postpone to after the release.
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei > Chen > Sent: 2022年9月2日 11:03 > To: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall > <julien@xen.org> > Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; > Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > heap allocator > > Hi Julien and Stefano, > > > -----Original Message----- > > From: Stefano Stabellini <sstabellini@kernel.org> > > Sent: 2022年9月2日 9:51 > > To: Julien Grall <julien@xen.org> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang > > <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis > > <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr > Babchuk > > <Volodymyr_Babchuk@epam.com> > > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > > heap allocator > > > > On Thu, 1 Sep 2022, Julien Grall wrote: > > > Hi Stefano, > > > > > In any case, I think we can postpone to after the release. Maybe we can add some notes to say that this feature is still experimental in EFI + DTS boot? Cheers, Wei Chen
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > >> This code is now becoming quite confusing to understanding. This loop is > >> meant to map the xenheap. If I follow your documentation, it would > mean > >> that only the reserved region should be mapped. > > > > Yes I think this is the same question that I raised in the scissors line of the > > commit message of this patch. > > Sorry I didn't notice the comment after the scissors line. This is the > same question :) > > > What I intend to do is still mapping the whole > > RAM because of the xenheap_* variables that you mentioned in... > > > >> > >> More confusingly, xenheap_* variables will cover the full RAM. > > > > ...here. But only adding the reserved region to the boot allocator so the > > reserved region can become the heap later on. I am wondering if we > > have a more clear way to do that, any suggestions? > > I think your code is correct. It only needs some renaming of the > existing variable (maybe to directmap_*?) to make clear the area is used > to access the RAM easily. Thanks for the clarification. I checked the code and found the xenheap_* variables are: xenheap_virt_start, xenheap_virt_end, xenheap_mfn_start, xenheap_mfn_end, xenheap_base_pdx. For clarification, do we need to change all of them to directmap_*? A pure renaming should be easy (and I guess also safe), but maybe I am overthinking because arm32 also uses xenheap_virt_end, xenheap_mfn_start and xenheap_mfn_end. These variables refer to the real xenheap, I am not sure renaming these would reduce the readability for arm32. Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Wei, On 02/09/2022 04:07, Wei Chen wrote: > > >> -----Original Message----- >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei >> Chen >> Sent: 2022年9月2日 11:03 >> To: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall >> <julien@xen.org> >> Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; >> Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk >> <Volodymyr_Babchuk@epam.com> >> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and >> heap allocator >> >> Hi Julien and Stefano, >> >>> -----Original Message----- >>> From: Stefano Stabellini <sstabellini@kernel.org> >>> Sent: 2022年9月2日 9:51 >>> To: Julien Grall <julien@xen.org> >>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang >>> <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis >>> <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr >> Babchuk >>> <Volodymyr_Babchuk@epam.com> >>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and >>> heap allocator >>> >>> On Thu, 1 Sep 2022, Julien Grall wrote: >>>> Hi Stefano, >>> >> >>> In any case, I think we can postpone to after the release. > > Maybe we can add some notes to say that this feature is still > experimental in EFI + DTS boot? Why EFI + DTS only? Regardless the discussion about how to properly checking the region, I think this wants to be a tech preview. Cheers,
On 02/09/2022 04:30, Henry Wang wrote: > Hi Julien, Hi Henry, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >>>> This code is now becoming quite confusing to understanding. This loop is >>>> meant to map the xenheap. If I follow your documentation, it would >> mean >>>> that only the reserved region should be mapped. >>> >>> Yes I think this is the same question that I raised in the scissors line of the >>> commit message of this patch. >> >> Sorry I didn't notice the comment after the scissors line. This is the >> same question :) >> >>> What I intend to do is still mapping the whole >>> RAM because of the xenheap_* variables that you mentioned in... >>> >>>> >>>> More confusingly, xenheap_* variables will cover the full RAM. >>> >>> ...here. But only adding the reserved region to the boot allocator so the >>> reserved region can become the heap later on. I am wondering if we >>> have a more clear way to do that, any suggestions? >> >> I think your code is correct. It only needs some renaming of the >> existing variable (maybe to directmap_*?) to make clear the area is used >> to access the RAM easily. > > Thanks for the clarification. I checked the code and found the xenheap_* > variables are: > xenheap_virt_start, xenheap_virt_end, xenheap_mfn_start, > xenheap_mfn_end, xenheap_base_pdx. > > For clarification, do we need to change all of them to directmap_*? Good question. > > A pure renaming should be easy (and I guess also safe), but maybe I am > overthinking because arm32 also uses xenheap_virt_end, xenheap_mfn_start > and xenheap_mfn_end. These variables refer to the real xenheap, I am not > sure renaming these would reduce the readability for arm32. So on arm32, only the xenheap is direct mapped today. So I think it would be fine to switch the name to "directmap_*". For extra clarify we could add an alias for arm32 between "xenheap_*" and "directmap_*". Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > > Thanks for the clarification. I checked the code and found the xenheap_* > > variables are: > > xenheap_virt_start, xenheap_virt_end, xenheap_mfn_start, > > xenheap_mfn_end, xenheap_base_pdx. > > > > For clarification, do we need to change all of them to directmap_*? > > Good question. :))) Thanks for your patience! > > > > > A pure renaming should be easy (and I guess also safe), but maybe I am > > overthinking because arm32 also uses xenheap_virt_end, > xenheap_mfn_start > > and xenheap_mfn_end. These variables refer to the real xenheap, I am not > > sure renaming these would reduce the readability for arm32. > > So on arm32, only the xenheap is direct mapped today. So I think it > would be fine to switch the name to "directmap_*". For extra clarify we > could add an alias for arm32 between "xenheap_*" and "directmap_*". Sounds good to me, I think I will try to add a separate patch for purely renaming all above-mentioned variables, then another patch for creating the alias for arm32. Hope you would fine with this plan. Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2022年9月2日 16:05 > To: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org> > Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; > Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and > heap allocator > > Hi Wei, > > On 02/09/2022 04:07, Wei Chen wrote: > > > > > >> -----Original Message----- > >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of > Wei > >> Chen > >> Sent: 2022年9月2日 11:03 > >> To: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall > >> <julien@xen.org> > >> Cc: Henry Wang <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; > >> Bertrand Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > >> <Volodymyr_Babchuk@epam.com> > >> Subject: RE: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot > and > >> heap allocator > >> > >> Hi Julien and Stefano, > >> > >>> -----Original Message----- > >>> From: Stefano Stabellini <sstabellini@kernel.org> > >>> Sent: 2022年9月2日 9:51 > >>> To: Julien Grall <julien@xen.org> > >>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Henry Wang > >>> <Henry.Wang@arm.com>; xen-devel@lists.xenproject.org; Bertrand Marquis > >>> <Bertrand.Marquis@arm.com>; Wei Chen <Wei.Chen@arm.com>; Volodymyr > >> Babchuk > >>> <Volodymyr_Babchuk@epam.com> > >>> Subject: Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot > and > >>> heap allocator > >>> > >>> On Thu, 1 Sep 2022, Julien Grall wrote: > >>>> Hi Stefano, > >>> > >> > >>> In any case, I think we can postpone to after the release. > > > > Maybe we can add some notes to say that this feature is still > > experimental in EFI + DTS boot? > > Why EFI + DTS only? Regardless the discussion about how to properly > checking the region, I think this wants to be a tech preview. > I had thought something like uboot + dts will not have reserved memory regions like EFI runtime services. But I forgot uboot also will have some address to load Xen and DTB. In this case, Xen still need to check relocation addresses with static heap. So you're right, I agree with you. Cheers, Wei Chen. > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 33704ca487..ab73b6e212 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt, int node, true); if ( rc ) return rc; + + reserved_heap = true; } printk("Checking for initrd in /chosen\n"); diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index e80f3d6201..00536a6d55 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo; extern domid_t max_init_domid; +extern bool reserved_heap; + void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); size_t estimate_efi_size(unsigned int mem_nr_banks); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 500307edc0..fe76cf6325 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes); domid_t __read_mostly max_init_domid; +bool __read_mostly reserved_heap; + static __used void init_done(void) { /* Must be done past setting system_state. */ @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void) #ifdef CONFIG_ARM_32 static void __init setup_mm(void) { - paddr_t ram_start, ram_end, ram_size, e; - unsigned long ram_pages; + paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size; + paddr_t reserved_heap_start = ~0, reserved_heap_end = 0, + reserved_heap_size = 0; + unsigned long ram_pages, reserved_heap_pages = 0; unsigned long heap_pages, xenheap_pages, domheap_pages; unsigned int i; const uint32_t ctr = READ_CP32(CTR); @@ -720,9 +724,9 @@ static void __init setup_mm(void) for ( i = 1; i < bootinfo.mem.nr_banks; i++ ) { - paddr_t bank_start = bootinfo.mem.bank[i].start; - paddr_t bank_size = bootinfo.mem.bank[i].size; - paddr_t bank_end = bank_start + bank_size; + bank_start = bootinfo.mem.bank[i].start; + bank_size = bootinfo.mem.bank[i].size; + bank_end = bank_start + bank_size; ram_size = ram_size + bank_size; ram_start = min(ram_start,bank_start); @@ -731,6 +735,25 @@ static void __init setup_mm(void) total_pages = ram_pages = ram_size >> PAGE_SHIFT; + if ( reserved_heap ) + { + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) + { + if ( bootinfo.reserved_mem.bank[i].xen_heap ) + { + bank_start = bootinfo.reserved_mem.bank[i].start; + bank_size = bootinfo.reserved_mem.bank[i].size; + bank_end = bank_start + bank_size; + + reserved_heap_size += bank_size; + reserved_heap_start = min(reserved_heap_start, bank_start); + reserved_heap_end = max(reserved_heap_end, bank_end); + } + } + + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; + } + /* * If the user has not requested otherwise via the command line * then locate the xenheap using these constraints: @@ -743,7 +766,8 @@ static void __init setup_mm(void) * We try to allocate the largest xenheap possible within these * constraints. */ - heap_pages = ram_pages; + heap_pages = !reserved_heap ? ram_pages : reserved_heap_pages; + if ( opt_xenheap_megabytes ) xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); else @@ -755,17 +779,21 @@ static void __init setup_mm(void) do { - e = consider_modules(ram_start, ram_end, + e = !reserved_heap ? + consider_modules(ram_start, ram_end, pfn_to_paddr(xenheap_pages), - 32<<20, 0); + 32<<20, 0) : + reserved_heap_end; + if ( e ) break; xenheap_pages >>= 1; } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); - if ( ! e ) - panic("Not not enough space for xenheap\n"); + if ( ! e || + ( reserved_heap && reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) ) + panic("Not enough space for xenheap\n"); domheap_pages = heap_pages - xenheap_pages; @@ -810,9 +838,9 @@ static void __init setup_mm(void) static void __init setup_mm(void) { const struct meminfo *banks = &bootinfo.mem; - paddr_t ram_start = ~0; - paddr_t ram_end = 0; - paddr_t ram_size = 0; + paddr_t ram_start = ~0, bank_start = ~0; + paddr_t ram_end = 0, bank_end = 0; + paddr_t ram_size = 0, bank_size = 0; unsigned int i; init_pdx(); @@ -821,17 +849,36 @@ static void __init setup_mm(void) * We need some memory to allocate the page-tables used for the xenheap * mappings. But some regions may contain memory already allocated * for other uses (e.g. modules, reserved-memory...). - * + * If reserved heap regions are properly defined, (only) add these regions + * in the boot allocator. + */ + if ( reserved_heap ) + { + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) + { + if ( bootinfo.reserved_mem.bank[i].xen_heap ) + { + bank_start = bootinfo.reserved_mem.bank[i].start; + bank_size = bootinfo.reserved_mem.bank[i].size; + bank_end = bank_start + bank_size; + + init_boot_pages(bank_start, bank_end); + } + } + } + /* + * No reserved heap regions: * For simplicity, add all the free regions in the boot allocator. */ - populate_boot_allocator(); + else + populate_boot_allocator(); total_pages = 0; for ( i = 0; i < banks->nr_banks; i++ ) { const struct membank *bank = &banks->bank[i]; - paddr_t bank_end = bank->start + bank->size; + bank_end = bank->start + bank->size; ram_size = ram_size + bank->size; ram_start = min(ram_start, bank->start);
This commit firstly adds a global variable `reserved_heap`. This newly introduced global variable is set at the device tree parsing time if the reserved heap ranges are defined in the device tree chosen node. For Arm32, In `setup_mm`, if the reserved heap is enabled, we use the reserved heap region for both domheap and xenheap allocation. For Arm64, In `setup_mm`, if the reserved heap is enabled and used, we make sure that only these reserved heap pages are added to the boot allocator. These reserved heap pages in the boot allocator are added to the heap allocator at `end_boot_allocator()`. If the reserved heap is disabled, we stick to current page allocation strategy at boot time. Also, take the chance to correct a "double not" print in Arm32 `setup_mm()`. Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- With reserved heap enabled, for Arm64, naming of global variables such as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous, wondering if we should rename these variables. --- Changes from RFC to v1: - Rebase on top of latest `setup_mm()` changes. - Added Arm32 logic in `setup_mm()`. --- xen/arch/arm/bootfdt.c | 2 + xen/arch/arm/include/asm/setup.h | 2 + xen/arch/arm/setup.c | 79 +++++++++++++++++++++++++------- 3 files changed, 67 insertions(+), 16 deletions(-)