| Message ID | 20220905072635.16294-4-Henry.Wang@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce reserved heap | expand |
Hi Henry, On 05/09/2022 08:26, Henry Wang wrote: > This commit firstly adds a bool field `reserved_heap` to bootinfo. > This newly introduced field 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. > Note that the xenheap on Arm32 should be always contiguous, so also > add a helper fit_xenheap_in_reserved_heap() for Arm32 to find the > required xenheap in the reserved heap regions. > > 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()` and replace the open-coding address ~0 by INVALID_PADDR. > > Signed-off-by: Henry Wang <Henry.Wang@arm.com> > --- > Changes from v1 to v2: > - Move the global bool `reserved_heap` to bootinfo. > - Replace the open open-coding address ~0 by INVALID_PADDR. > - Do not use reverted logic in heap_pages calculation. > - Remove unused Arm32 reserved_heap_start variable. > - Decouple the arm32 reserved heap too small size check with region > end check. > - Reuse the arm32 original xenheap finding logic with the new helper > to make sure xenheap on arm32 is contiguous. > 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 | 1 + > xen/arch/arm/setup.c | 116 +++++++++++++++++++++++++++---- > 3 files changed, 104 insertions(+), 15 deletions(-) > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c > index 3796a4bd75..616bf5ce47 100644 > --- a/xen/arch/arm/bootfdt.c > +++ b/xen/arch/arm/bootfdt.c > @@ -322,6 +322,8 @@ static int __init process_chosen_node(const void *fdt, int node, > &bootinfo.reserved_mem, MEMBANK_RSVD_HEAP); > if ( rc ) > return rc; > + > + bootinfo.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 d0cc556833..22fb950bc8 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -82,6 +82,7 @@ struct bootinfo { > #ifdef CONFIG_ACPI > struct meminfo acpi; > #endif > + bool reserved_heap; > }; > > struct map_range_data > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 8d3f859982..0b4f7cb909 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -556,6 +556,43 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, > } > return e; > } > + > +/* > + * Find the contiguous xenheap region that fits in the reserved heap region with There might be multiple. So "Find a contiguous...". I would also drop "xenheap". > + * required size and alignment, and return the end address of xenheap. I would write "and return the end address of the region if found otherwise 0". > + */ > +static paddr_t __init fit_xenheap_in_reserved_heap(uint32_t size, paddr_t align) > +{ > + int i; Please use unsigned int. > + paddr_t end = 0, aligned_start, aligned_end; > + paddr_t bank_start, bank_size, bank_end; > + > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_HEAP ) NIT: You could avoid the extra indentation by reverting the condition. > + { > + bank_start = bootinfo.reserved_mem.bank[i].start; > + bank_size = bootinfo.reserved_mem.bank[i].size; > + bank_end = bank_start + bank_size; > + > + if ( bank_size < size ) > + continue; > + > + aligned_end = bank_end & ~(align - 1); > + aligned_start = (aligned_end - size) & ~(align - 1); I find the logic a bit confusing. AFAIU, aligned_start could be below the start of the RAM which is not what I would usually expect. The code works. So no change requested. > + > + if ( aligned_start > bank_start ) > + /* > + * Arm32 allocates xenheap from higher address to lower, so if This code is also called on arm32. So what are you referring to? Is it consider_modules()? > + * there are multiple memory banks that satisfy the requirement, > + * use the highest bank. > + */ > + end = max(end, aligned_end); > + } > + } > + > + return end; > +} > #endif > > /* > @@ -713,8 +750,9 @@ 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_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); > @@ -734,9 +772,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); > @@ -745,19 +783,42 @@ static void __init setup_mm(void) > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > + if ( bootinfo.reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_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_end = max(reserved_heap_end, bank_end); > + } > + } > + > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > + if ( reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) > + panic("Too small reserved heap region, should be at least 32M\n"); This is a bit misleading. 32MB is not sufficient, it also has to be contiguous. So I would drop this panic() completely. > + } > + > /* > * If the user has not requested otherwise via the command line > * then locate the xenheap using these constraints: > * > * - 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 most 1GB or 1/32 the total RAM in the system > + * (there is no reserved heap) or 1/32 the total reserved Did you forgot to add "if" before "there"? > + * heap region (there is reserved heap) if less The new wording suggests that the 1GB limit only applies when the admin doesn't specify the reserved heap. However, we don't support larger heap than 1GB. So the limit should also apply for the reserved heap. So how about: - must be at most 1GB or 1/32 the total RAM in the system (or reserved heap if enabled) > * - must be at least 32M > * > * We try to allocate the largest xenheap possible within these > * constraints. > */ > - heap_pages = ram_pages; > + heap_pages = bootinfo.reserved_heap ? reserved_heap_pages : ram_pages; You can avoid the ternary operation here by setting heap_pages in the 'if' above and add a else for the 'ram_pages' part. In fact, 'ram_pages' could be completely dropped in favor of 'total_pages'. > + > if ( opt_xenheap_megabytes ) > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > else > @@ -767,9 +828,15 @@ static void __init setup_mm(void) > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > } > > + /* > + * On Arm32, xenheap must be contiguous, look for one of the region > + * that matches the above-mentioned xenheap constraints. > + */ IMHO this is already implied by the large comment above. But if you want to be more obvious, then I think this should belong to the comment above. > do > { > - e = consider_modules(ram_start, ram_end, > + e = bootinfo.reserved_heap ? > + fit_xenheap_in_reserved_heap(pfn_to_paddr(xenheap_pages), 32<<20) : Please use MB(32) in new code. > + consider_modules(ram_start, ram_end, > pfn_to_paddr(xenheap_pages), > 32<<20, 0); > if ( e ) > @@ -779,7 +846,7 @@ static void __init setup_mm(void) > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); > > if ( ! e ) > - panic("Not not enough space for xenheap\n"); > + panic("Not enough space for xenheap\n"); > > domheap_pages = heap_pages - xenheap_pages; > > @@ -824,9 +891,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 = INVALID_PADDR, bank_start = INVALID_PADDR; > + paddr_t ram_end = 0, bank_end = 0; > + paddr_t ram_size = 0, bank_size = 0; > unsigned int i; > > init_pdx(); > @@ -835,17 +902,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 there are non-empty reserved heap regions, (only) add these regions I am not sure what you mean by "non-empty" here. How about something like: "If a reserved heap was provided by the admin, populate the boot allocator with the corresponding regions only". > + * in the boot allocator. > + */ > + if ( bootinfo.reserved_heap ) > + { > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > + { > + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_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(); For arm32, shouldn't we also only add the reserved heap (minus the xenheap) to the boot allocator? At which point, I would move the change in populate_boot_allocator(). Cheers,
Hi Julien, Thanks for your comments, I added my reply and some of the questions that I am not 100% sure inline below. > -----Original Message----- > From: Julien Grall <julien@xen.org> > Hi Henry, > > + > > +/* > > + * Find the contiguous xenheap region that fits in the reserved heap region > with > > There might be multiple. So "Find a contiguous...". I would also drop > "xenheap". I will follow the wording that you suggested here and ... > > > + * required size and alignment, and return the end address of xenheap. > > I would write "and return the end address of the region if found > otherwise 0". ...here. > > > + */ > > +static paddr_t __init fit_xenheap_in_reserved_heap(uint32_t size, paddr_t > align) > > +{ > > + int i; > > Please use unsigned int. Ah sure. > > > + paddr_t end = 0, aligned_start, aligned_end; > > + paddr_t bank_start, bank_size, bank_end; > > + > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_HEAP ) > NIT: You could avoid the extra indentation by reverting the condition. Sorry I am not 100% sure the extra indentation you were referring to. Are you suggesting that we need to do a ``` if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_RSVD_HEAP ) continue; bank_start = bootinfo.reserved_mem.bank[i].start; ... ``` ? If so I will change in v3. > > > + { > > + bank_start = bootinfo.reserved_mem.bank[i].start; > > + bank_size = bootinfo.reserved_mem.bank[i].size; > > + bank_end = bank_start + bank_size; > > + > > + if ( bank_size < size ) > > + continue; > > + > > + aligned_end = bank_end & ~(align - 1); > > + aligned_start = (aligned_end - size) & ~(align - 1); > > I find the logic a bit confusing. AFAIU, aligned_start could be below > the start of the RAM which is not what I would usually expect. Yeah I understand your concern. Here I want to make sure even if the given size is not aligned (although less likely happen in real life given the size calculation logic in setup_mm) the code still work. So the aligned_start = (aligned_end - size) & ~(align - 1) and below if (aligned_start > bank_start) check. > > The code works. So no change requested. Thanks! > > > > + > > + if ( aligned_start > bank_start ) > > + /* > > + * Arm32 allocates xenheap from higher address to lower, so if > > This code is also called on arm32. So what are you referring to? Is it > consider_modules()? Yes, I think the current arm32 behavior in consider_modules() is what I am referring to. In fact, I just want to add some comments that explain why we need the end = max(end, aligned_end) since technically if there are multiple reserved heap banks and all of them can fit the xenheap region, we can use either of them. But following the current behavior we can only use the highest bank to keep the consistency. > > > + * there are multiple memory banks that satisfy the requirement, > > + * use the highest bank. > > + */ > > + end = max(end, aligned_end); > > + } > > + } > > + > > + return end; > > +} > > #endif > > > > /* > > @@ -713,8 +750,9 @@ 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_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); > > @@ -734,9 +772,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); > > @@ -745,19 +783,42 @@ static void __init setup_mm(void) > > > > total_pages = ram_pages = ram_size >> PAGE_SHIFT; > > > > + if ( bootinfo.reserved_heap ) > > + { > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].type == > MEMBANK_RSVD_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_end = max(reserved_heap_end, bank_end); > > + } > > + } > > + > > + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; > > + if ( reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) > > + panic("Too small reserved heap region, should be at least 32M\n"); > > This is a bit misleading. 32MB is not sufficient, it also has to be > contiguous. So I would drop this panic() completely. Sure. > > > + } > > + > > /* > > * If the user has not requested otherwise via the command line > > * then locate the xenheap using these constraints: > > * > > * - 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 most 1GB or 1/32 the total RAM in the system > > + * (there is no reserved heap) or 1/32 the total reserved > > Did you forgot to add "if" before "there"? I will use the wording that you suggested in ... > > > + * heap region (there is reserved heap) if less > > The new wording suggests that the 1GB limit only applies when the admin > doesn't specify the reserved heap. However, we don't support larger heap > than 1GB. So the limit should also apply for the reserved heap. So how > about: > > - must be at most 1GB or 1/32 the total RAM in the system (or reserved > heap if enabled) ...here. > > > * - must be at least 32M > > * > > * We try to allocate the largest xenheap possible within these > > * constraints. > > */ > > - heap_pages = ram_pages; > > + heap_pages = bootinfo.reserved_heap ? reserved_heap_pages : > ram_pages; > > You can avoid the ternary operation here by setting heap_pages in the > 'if' above and add a else for the 'ram_pages' part. Sorry I might understand your comment in the wrong way, but do you suggest we need to: ``` if ( bootinfo.reserved_heap ) { ... heap_pages = reserved_heap_pages; } else heap_pages = total_pages; ``` ? If so I will do that in v3. > > In fact, 'ram_pages' could be completely dropped in favor of 'total_pages'. Good point, I will do that in v3. > > > + > > if ( opt_xenheap_megabytes ) > > xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); > > else > > @@ -767,9 +828,15 @@ static void __init setup_mm(void) > > xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); > > } > > > > + /* > > + * On Arm32, xenheap must be contiguous, look for one of the region > > + * that matches the above-mentioned xenheap constraints. > > + */ > > IMHO this is already implied by the large comment above. But if you want > to be more obvious, then I think this should belong to the comment above. I think I will add "the xenheap should be contiguous" as I missed this one Before you mentioned this in the code review. I think adding this will avoid people who change this part in the future making mistakes like I did :)) > > > do > > { > > - e = consider_modules(ram_start, ram_end, > > + e = bootinfo.reserved_heap ? > > + fit_xenheap_in_reserved_heap(pfn_to_paddr(xenheap_pages), > 32<<20) : > > Please use MB(32) in new code. Oh sure. Thanks for pointing this out. > > > + consider_modules(ram_start, ram_end, > > pfn_to_paddr(xenheap_pages), > > 32<<20, 0); > > if ( e ) > > @@ -779,7 +846,7 @@ static void __init setup_mm(void) > > } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20- > PAGE_SHIFT) ); > > > > if ( ! e ) > > - panic("Not not enough space for xenheap\n"); > > + panic("Not enough space for xenheap\n"); > > > > domheap_pages = heap_pages - xenheap_pages; > > > > @@ -824,9 +891,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 = INVALID_PADDR, bank_start = INVALID_PADDR; > > + paddr_t ram_end = 0, bank_end = 0; > > + paddr_t ram_size = 0, bank_size = 0; > > unsigned int i; > > > > init_pdx(); > > @@ -835,17 +902,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 there are non-empty reserved heap regions, (only) add these > regions > > I am not sure what you mean by "non-empty" here. How about something > like: > > "If a reserved heap was provided by the admin, populate the boot > allocator with the corresponding regions only". Sure. > > > + * in the boot allocator. > > + */ > > + if ( bootinfo.reserved_heap ) > > + { > > + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) > > + { > > + if ( bootinfo.reserved_mem.bank[i].type == > MEMBANK_RSVD_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(); > > For arm32, shouldn't we also only add the reserved heap (minus the > xenheap) to the boot allocator? At which point, I would move the change > in populate_boot_allocator(). Sorry I am not sure what this comment about...as here the code is for arm64. For the question, yes. For the latter one, do you request some changes? If so, could you please kindly elaborate a little bit more? Thanks. Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Henry On 06/09/2022 02:53, Henry Wang wrote: > Thanks for your comments, I added my reply and some of the questions > that I am not 100% sure inline below. > >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Hi Henry, >>> + >>> +/* >>> + * Find the contiguous xenheap region that fits in the reserved heap region >> with >> >> There might be multiple. So "Find a contiguous...". I would also drop >> "xenheap". > > I will follow the wording that you suggested here and ... > >> >>> + * required size and alignment, and return the end address of xenheap. >> >> I would write "and return the end address of the region if found >> otherwise 0". > > ...here. > >> >>> + */ >>> +static paddr_t __init fit_xenheap_in_reserved_heap(uint32_t size, paddr_t >> align) >>> +{ >>> + int i; >> >> Please use unsigned int. > > Ah sure. > >> >>> + paddr_t end = 0, aligned_start, aligned_end; >>> + paddr_t bank_start, bank_size, bank_end; >>> + >>> + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) >>> + { >>> + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_HEAP ) >> NIT: You could avoid the extra indentation by reverting the condition. > > Sorry I am not 100% sure the extra indentation you were referring to. > Are you suggesting that we need to do a > ``` > if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_RSVD_HEAP ) > continue; > > bank_start = bootinfo.reserved_mem.bank[i].start; > ... > ``` > > ? Yes. > > If so I will change in v3. > >> >>> + { >>> + bank_start = bootinfo.reserved_mem.bank[i].start; >>> + bank_size = bootinfo.reserved_mem.bank[i].size; >>> + bank_end = bank_start + bank_size; >>> + >>> + if ( bank_size < size ) >>> + continue; >>> + >>> + aligned_end = bank_end & ~(align - 1); >>> + aligned_start = (aligned_end - size) & ~(align - 1); >> >> I find the logic a bit confusing. AFAIU, aligned_start could be below >> the start of the RAM which is not what I would usually expect. > > Yeah I understand your concern. Here I want to make sure even if > the given size is not aligned (although less likely happen in real life > given the size calculation logic in setup_mm) the code still work. I don't think I agree on the less likely here. The regions are provided by in the Device-Tree. And there are more chance they are incorrect because the value will be specific to a software/device stack. Related to this discussion, I can't find any alignment requirement in the device-tree binding. I think we at least want to require 64KB aligned (so the same Device-Tree works if we were going to support 64KB page granularity). >> >>> + >>> + if ( aligned_start > bank_start ) >>> + /* >>> + * Arm32 allocates xenheap from higher address to lower, so if >> >> This code is also called on arm32. So what are you referring to? Is it >> consider_modules()? > > Yes, I think the current arm32 behavior in consider_modules() is what > I am referring to. In fact, I just want to add some comments that explain why > we need the end = max(end, aligned_end) since technically if there are > multiple reserved heap banks and all of them can fit the xenheap region, > we can use either of them. But following the current behavior we can only use > the highest bank to keep the consistency. Xenheap is currently allocated the highest possible so there is enough low memory available for domain memory. This is in order to allow 32-bit DMA device to function. I am less certain this makes sense when the heap is reserved. Because an admin could decide to define the heap solely above/below 4GB. That said, nothing in the document suggests that domain memory would not be allocated from the reserved heap. So I would suggest to write the following comment: "Allocate the xenheap as high as possible to keep low-memory available (assuming the admin supplied region below 4GB) for other use (e.g. domain memory allocation)." Also, I think the documentation wants to be updated to clarify whether the reserved heap could be used to allocate domain. If it could, then I think we need to explain that the region should contain enough memory below some 4GB to cater 32-bit DMA. >> The new wording suggests that the 1GB limit only applies when the admin >> doesn't specify the reserved heap. However, we don't support larger heap >> than 1GB. So the limit should also apply for the reserved heap. So how >> about: >> >> - must be at most 1GB or 1/32 the total RAM in the system (or reserved >> heap if enabled) > > ...here. > >> >>> * - must be at least 32M >>> * >>> * We try to allocate the largest xenheap possible within these >>> * constraints. >>> */ >>> - heap_pages = ram_pages; >>> + heap_pages = bootinfo.reserved_heap ? reserved_heap_pages : >> ram_pages; >> >> You can avoid the ternary operation here by setting heap_pages in the >> 'if' above and add a else for the 'ram_pages' part. > > Sorry I might understand your comment in the wrong way, but do you > suggest we need to: > ``` > if ( bootinfo.reserved_heap ) > { > ... > heap_pages = reserved_heap_pages; > } > else > heap_pages = total_pages; > ``` > ? Yes. > If so I will do that in v3. Thanks. >>> + * in the boot allocator. >>> + */ >>> + if ( bootinfo.reserved_heap ) >>> + { >>> + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) >>> + { >>> + if ( bootinfo.reserved_mem.bank[i].type == >> MEMBANK_RSVD_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(); >> >> For arm32, shouldn't we also only add the reserved heap (minus the >> xenheap) to the boot allocator? At which point, I would move the change >> in populate_boot_allocator(). > > Sorry I am not sure what this comment about...as here the code is for arm64. Right, I wasn't sure where to comment because you don't touch the call to populate_boot_allocator(). > For the question, yes. > For the latter one, do you request some changes? If so, could you please kindly > elaborate a little bit more? Thanks. Yes I am requesting some change because I think the code on arm32 is incorrect (the boot allocator will not be populated with the reserved heap). I think the code should be moved in populate_boot_allocator(): if ( bootinfo.reserved_heap ) { for ( ...; i < bootinfo.reserved_mem.nr_banks; i++ ) [....] init_boot_pages_pages() } Note that to handle arm32, you will also need to exclude the xenheap area. Cheers,
Hi Julien, Thanks for the clarification and your patience. For the populate_boot_allocator() change, I attached my change in the end, and personally I would like to hear your opinion before sending v3 since we now have limited time. > -----Original Message----- > From: Julien Grall <julien@xen.org> > >>> + { > >>> + bank_start = bootinfo.reserved_mem.bank[i].start; > >>> + bank_size = bootinfo.reserved_mem.bank[i].size; > >>> + bank_end = bank_start + bank_size; > >>> + > >>> + if ( bank_size < size ) > >>> + continue; > >>> + > >>> + aligned_end = bank_end & ~(align - 1); > >>> + aligned_start = (aligned_end - size) & ~(align - 1); > >> > >> I find the logic a bit confusing. AFAIU, aligned_start could be below > >> the start of the RAM which is not what I would usually expect. > > > > Yeah I understand your concern. Here I want to make sure even if > > the given size is not aligned (although less likely happen in real life > > given the size calculation logic in setup_mm) the code still work. > Sorry I probably explained in the wrong way in previous mail, but since no change requested here this is purely for discussion. In the code we are sure aligned_end calculation will make sure the end address will satisfy the alignment requirement within the range to a aligned (lower) address. The aligned_start = (aligned_end - size) & ~(align - 1) will make sure the start address is following the same alignment requirement, but the only issue would be in this case the start address will below the region start, hence the if ( aligned_start > bank_start ) check. > I don't think I agree on the less likely here. The regions are provided > by in the Device-Tree. And there are more chance they are incorrect > because the value will be specific to a software/device stack. > > Related to this discussion, I can't find any alignment requirement in > the device-tree binding. I think we at least want to require 64KB > aligned (so the same Device-Tree works if we were going to support 64KB > page granularity). I agree we need to require 64KB alignment, and currently we are following this because we are doing 32MB alignment. I will add a comment in the function comment to mention we at least want a 64KB alignment so that future callers will not make mistakes. > > >> > >>> + > >>> + if ( aligned_start > bank_start ) > >>> + /* > >>> + * Arm32 allocates xenheap from higher address to lower, so if > >> > >> This code is also called on arm32. So what are you referring to? Is it > >> consider_modules()? > > > > Yes, I think the current arm32 behavior in consider_modules() is what > > I am referring to. In fact, I just want to add some comments that explain > why > > we need the end = max(end, aligned_end) since technically if there are > > multiple reserved heap banks and all of them can fit the xenheap region, > > we can use either of them. But following the current behavior we can only > use > > the highest bank to keep the consistency. > > Xenheap is currently allocated the highest possible so there is enough > low memory available for domain memory. This is in order to allow 32-bit > DMA device to function. > > I am less certain this makes sense when the heap is reserved. Because an > admin could decide to define the heap solely above/below 4GB. > > That said, nothing in the document suggests that domain memory would not > be allocated from the reserved heap. So I would suggest to write the > following comment: > > "Allocate the xenheap as high as possible to keep low-memory available > (assuming the admin supplied region below 4GB) for other use (e.g. > domain memory allocation)." Sure. > > Also, I think the documentation wants to be updated to clarify whether > the reserved heap could be used to allocate domain. If it could, then I > think we need to explain that the region should contain enough memory > below some 4GB to cater 32-bit DMA. Ok I will add in v3. > > >>> + /* > >>> + * No reserved heap regions: > >>> * For simplicity, add all the free regions in the boot allocator. > >>> */ > >>> - populate_boot_allocator(); > >>> + else > >>> + populate_boot_allocator(); > >> > >> For arm32, shouldn't we also only add the reserved heap (minus the > >> xenheap) to the boot allocator? At which point, I would move the change > >> in populate_boot_allocator(). > > > > Sorry I am not sure what this comment about...as here the code is for > arm64. > > Right, I wasn't sure where to comment because you don't touch the call > to populate_boot_allocator(). > > > For the question, yes. > > For the latter one, do you request some changes? If so, could you please > kindly > > elaborate a little bit more? Thanks. > > Yes I am requesting some change because I think the code on arm32 is > incorrect (the boot allocator will not be populated with the reserved heap). > > I think the code should be moved in populate_boot_allocator(): > > if ( bootinfo.reserved_heap ) > { > for ( ...; i < bootinfo.reserved_mem.nr_banks; i++ ) > [....] > init_boot_pages_pages() > } > > Note that to handle arm32, you will also need to exclude the xenheap area. When I implement the code, I found that the arm32 Xenheap excluding logic somehow can be reused. So I think I tried to reuse as much as current code. Would below populate_boot_allocator() seem ok to you? static void __init populate_boot_allocator(void) { unsigned int i; const struct meminfo *banks = bootinfo.static_heap ? &bootinfo.reserved_mem : &bootinfo.mem; for ( i = 0; i < banks->nr_banks; i++ ) { const struct membank *bank = &banks->bank[i]; paddr_t bank_end = bank->start + bank->size; paddr_t s, e; if ( bootinfo.static_heap && bank->type != MEMBANK_STATIC_HEAP ) continue; s = bank->start; while ( s < bank_end ) { paddr_t n = bank_end; if ( bootinfo.static_heap ) e = bank_end; else { e = next_module(s, &n); if ( e == ~(paddr_t)0 ) e = n = bank_end; /* * Module in a RAM bank other than the one which we are * not dealing with here. */ if ( e > bank_end ) e = bank_end; } #ifdef CONFIG_ARM_32 /* Avoid the xenheap */ if ( s < mfn_to_maddr(directmap_mfn_end) && mfn_to_maddr(directmap_mfn_start) < e ) { e = mfn_to_maddr(directmap_mfn_start); n = mfn_to_maddr(directmap_mfn_end); } #endif if ( bootinfo.static_heap ) init_boot_pages(s, e); else fw_unreserved_regions(s, e, init_boot_pages, 0); s = n; } } } Kind regards, Henry > > Cheers, > > -- > Julien Grall
Hi Henry, On 06/09/2022 12:11, Henry Wang wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >>>>> + { >>>>> + bank_start = bootinfo.reserved_mem.bank[i].start; >>>>> + bank_size = bootinfo.reserved_mem.bank[i].size; >>>>> + bank_end = bank_start + bank_size; >>>>> + >>>>> + if ( bank_size < size ) >>>>> + continue; >>>>> + >>>>> + aligned_end = bank_end & ~(align - 1); >>>>> + aligned_start = (aligned_end - size) & ~(align - 1); >>>> >>>> I find the logic a bit confusing. AFAIU, aligned_start could be below >>>> the start of the RAM which is not what I would usually expect. >>> >>> Yeah I understand your concern. Here I want to make sure even if >>> the given size is not aligned (although less likely happen in real life >>> given the size calculation logic in setup_mm) the code still work. >> > > Sorry I probably explained in the wrong way in previous mail, but since no > change requested here this is purely for discussion. In the code we > are sure aligned_end calculation will make sure the end address will > satisfy the alignment requirement within the range to a aligned (lower) > address. The aligned_start = (aligned_end - size) & ~(align - 1) will make > sure the start address is following the same alignment requirement, but > the only issue would be in this case the start address will below the region > start, hence the if ( aligned_start > bank_start ) check. > >> I don't think I agree on the less likely here. The regions are provided >> by in the Device-Tree. And there are more chance they are incorrect >> because the value will be specific to a software/device stack. >> >> Related to this discussion, I can't find any alignment requirement in >> the device-tree binding. I think we at least want to require 64KB >> aligned (so the same Device-Tree works if we were going to support 64KB >> page granularity). > > I agree we need to require 64KB alignment, and currently we are following > this because we are doing 32MB alignment. Hmmm... I think we are talking about two different things here. What I am referring to is the alignment of the start/end of the region provided by the admin. [...] >> I think the code should be moved in populate_boot_allocator(): >> >> if ( bootinfo.reserved_heap ) >> { >> for ( ...; i < bootinfo.reserved_mem.nr_banks; i++ ) >> [....] >> init_boot_pages_pages() >> } >> >> Note that to handle arm32, you will also need to exclude the xenheap area. > > When I implement the code, I found that the arm32 Xenheap excluding logic > somehow can be reused. > > So I think I tried to reuse as much as current code. Would below > populate_boot_allocator() seem ok to you? I would prefer if they are separate because the logic can be simplified when using the static heap (the xenheap cannot across a region). Something like: for ( i = 0; i < banks->nr_banks; i++ ) { #ifdef CONFIG_ARM_32 if ( (bank_start >= mfn_to_maddr(direct_mfn_start) && bank_end < mfn_to_maddr(direct_mfn_start) ) { /* Add the memory *before* and *after* the region */ } else #endif init_boot_pages(s, e); } > > static void __init populate_boot_allocator(void) > { > unsigned int i; > const struct meminfo *banks = bootinfo.static_heap ? > &bootinfo.reserved_mem : &bootinfo.mem; > > for ( i = 0; i < banks->nr_banks; i++ ) > { > const struct membank *bank = &banks->bank[i]; > paddr_t bank_end = bank->start + bank->size; > paddr_t s, e; > > if ( bootinfo.static_heap && bank->type != MEMBANK_STATIC_HEAP ) > continue; > > s = bank->start; > while ( s < bank_end ) > { > paddr_t n = bank_end; > > if ( bootinfo.static_heap ) > e = bank_end; > else > { > e = next_module(s, &n); > > if ( e == ~(paddr_t)0 ) > e = n = bank_end; > > /* > * Module in a RAM bank other than the one which we are > * not dealing with here. > */ > if ( e > bank_end ) > e = bank_end; > } > > #ifdef CONFIG_ARM_32 > /* Avoid the xenheap */ > if ( s < mfn_to_maddr(directmap_mfn_end) && > mfn_to_maddr(directmap_mfn_start) < e ) > { > e = mfn_to_maddr(directmap_mfn_start); > n = mfn_to_maddr(directmap_mfn_end); > } > #endif > > if ( bootinfo.static_heap ) > init_boot_pages(s, e); > else > fw_unreserved_regions(s, e, init_boot_pages, 0); > > s = n; > } > } > } > > Kind regards, > Henry > >> >> Cheers, >> >> -- >> Julien Grall
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > > I agree we need to require 64KB alignment, and currently we are following > > this because we are doing 32MB alignment. > Hmmm... I think we are talking about two different things here. What I > am referring to is the alignment of the start/end of the region provided > by the admin. Ahh sorry my bad, yeah I should add the alignment requirement in dt-binding. Will do that in v3. > > [...] > > >> Note that to handle arm32, you will also need to exclude the xenheap > area. > > > > When I implement the code, I found that the arm32 Xenheap excluding > logic > > somehow can be reused. > > > > So I think I tried to reuse as much as current code. Would below > > populate_boot_allocator() seem ok to you? > > I would prefer if they are separate because the logic can be simplified > when using the static heap (the xenheap cannot across a region). I think whether separate this logic or not is personal taste, I did miss the "xenheap cannot across the bank" part so I agree your suggestion is better, but I think... > > Something like: > > for ( i = 0; i < banks->nr_banks; i++ ) > { > > #ifdef CONFIG_ARM_32 > if ( (bank_start >= mfn_to_maddr(direct_mfn_start) && > bank_end < mfn_to_maddr(direct_mfn_start) ) ... this is probably wrong or I misunderstood? IMHO the xenheap is always smaller (or equal) than the bank, based on the logic that finding a contiguous xenheap in a bank. So the code I propose would be: ``` @@ -712,12 +712,37 @@ static void __init populate_boot_allocator(void) { unsigned int i; const struct meminfo *banks = &bootinfo.mem; + paddr_t s, e; + + if ( bootinfo.static_heap ) + { + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) + { + if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP ) + continue; + + s = bootinfo.reserved_mem.bank[i].start; + e = s + bootinfo.reserved_mem.bank[i].size; +#ifdef CONFIG_ARM_32 + /* Avoid the xenheap, note that the xenheap cannot across a bank */ + if ( s <= mfn_to_maddr(directmap_mfn_start) && + e >= mfn_to_maddr(directmap_mfn_end) ) + { + init_boot_pages(s, mfn_to_maddr(directmap_mfn_start)); + init_boot_pages(mfn_to_maddr(directmap_mfn_end), e); + } + else +#endif + init_boot_pages(s, e); + } + + return; + } for ( i = 0; i < banks->nr_banks; i++ ) /* The original logic in populate_boot_allocator()*/ ``` Kind regards, Henry
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 3796a4bd75..616bf5ce47 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -322,6 +322,8 @@ static int __init process_chosen_node(const void *fdt, int node, &bootinfo.reserved_mem, MEMBANK_RSVD_HEAP); if ( rc ) return rc; + + bootinfo.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 d0cc556833..22fb950bc8 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -82,6 +82,7 @@ struct bootinfo { #ifdef CONFIG_ACPI struct meminfo acpi; #endif + bool reserved_heap; }; struct map_range_data diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 8d3f859982..0b4f7cb909 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -556,6 +556,43 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, } return e; } + +/* + * Find the contiguous xenheap region that fits in the reserved heap region with + * required size and alignment, and return the end address of xenheap. + */ +static paddr_t __init fit_xenheap_in_reserved_heap(uint32_t size, paddr_t align) +{ + int i; + paddr_t end = 0, aligned_start, aligned_end; + paddr_t bank_start, bank_size, bank_end; + + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) + { + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_HEAP ) + { + bank_start = bootinfo.reserved_mem.bank[i].start; + bank_size = bootinfo.reserved_mem.bank[i].size; + bank_end = bank_start + bank_size; + + if ( bank_size < size ) + continue; + + aligned_end = bank_end & ~(align - 1); + aligned_start = (aligned_end - size) & ~(align - 1); + + if ( aligned_start > bank_start ) + /* + * Arm32 allocates xenheap from higher address to lower, so if + * there are multiple memory banks that satisfy the requirement, + * use the highest bank. + */ + end = max(end, aligned_end); + } + } + + return end; +} #endif /* @@ -713,8 +750,9 @@ 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_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); @@ -734,9 +772,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); @@ -745,19 +783,42 @@ static void __init setup_mm(void) total_pages = ram_pages = ram_size >> PAGE_SHIFT; + if ( bootinfo.reserved_heap ) + { + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) + { + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_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_end = max(reserved_heap_end, bank_end); + } + } + + reserved_heap_pages = reserved_heap_size >> PAGE_SHIFT; + if ( reserved_heap_pages < 32<<(20-PAGE_SHIFT) ) + panic("Too small reserved heap region, should be at least 32M\n"); + } + /* * If the user has not requested otherwise via the command line * then locate the xenheap using these constraints: * * - 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 most 1GB or 1/32 the total RAM in the system + * (there is no reserved heap) or 1/32 the total reserved + * heap region (there is reserved heap) if less * - must be at least 32M * * We try to allocate the largest xenheap possible within these * constraints. */ - heap_pages = ram_pages; + heap_pages = bootinfo.reserved_heap ? reserved_heap_pages : ram_pages; + if ( opt_xenheap_megabytes ) xenheap_pages = opt_xenheap_megabytes << (20-PAGE_SHIFT); else @@ -767,9 +828,15 @@ static void __init setup_mm(void) xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT)); } + /* + * On Arm32, xenheap must be contiguous, look for one of the region + * that matches the above-mentioned xenheap constraints. + */ do { - e = consider_modules(ram_start, ram_end, + e = bootinfo.reserved_heap ? + fit_xenheap_in_reserved_heap(pfn_to_paddr(xenheap_pages), 32<<20) : + consider_modules(ram_start, ram_end, pfn_to_paddr(xenheap_pages), 32<<20, 0); if ( e ) @@ -779,7 +846,7 @@ static void __init setup_mm(void) } while ( !opt_xenheap_megabytes && xenheap_pages > 32<<(20-PAGE_SHIFT) ); if ( ! e ) - panic("Not not enough space for xenheap\n"); + panic("Not enough space for xenheap\n"); domheap_pages = heap_pages - xenheap_pages; @@ -824,9 +891,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 = INVALID_PADDR, bank_start = INVALID_PADDR; + paddr_t ram_end = 0, bank_end = 0; + paddr_t ram_size = 0, bank_size = 0; unsigned int i; init_pdx(); @@ -835,17 +902,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 there are non-empty reserved heap regions, (only) add these regions + * in the boot allocator. + */ + if ( bootinfo.reserved_heap ) + { + for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ ) + { + if ( bootinfo.reserved_mem.bank[i].type == MEMBANK_RSVD_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 bool field `reserved_heap` to bootinfo. This newly introduced field 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. Note that the xenheap on Arm32 should be always contiguous, so also add a helper fit_xenheap_in_reserved_heap() for Arm32 to find the required xenheap in the reserved heap regions. 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()` and replace the open-coding address ~0 by INVALID_PADDR. Signed-off-by: Henry Wang <Henry.Wang@arm.com> --- Changes from v1 to v2: - Move the global bool `reserved_heap` to bootinfo. - Replace the open open-coding address ~0 by INVALID_PADDR. - Do not use reverted logic in heap_pages calculation. - Remove unused Arm32 reserved_heap_start variable. - Decouple the arm32 reserved heap too small size check with region end check. - Reuse the arm32 original xenheap finding logic with the new helper to make sure xenheap on arm32 is contiguous. 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 | 1 + xen/arch/arm/setup.c | 116 +++++++++++++++++++++++++++---- 3 files changed, 104 insertions(+), 15 deletions(-)