| Message ID | 20220520120937.28925-14-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: mm: Remove open-coding mappings | expand |
Hi Julien, On 20.05.2022 14:09, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > In a follow-up patch, we will want to populate the boot allocator > separately for arm64. The code will end up to be very similar to the one > on arm32. So move out the code in a new helper populate_boot_allocator(). > > For now the code is still protected by CONFIG_ARM_32 to avoid any build > failure on arm64. > > Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with > xenheap_mfn_end as they are equivalent. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > > Changes in v4: > - Patch added > --- > xen/arch/arm/setup.c | 90 +++++++++++++++++++++++++------------------- > 1 file changed, 51 insertions(+), 39 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed48a..3d5a2283d4ef 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void) > } > > #ifdef CONFIG_ARM_32 > +/* > + * Populate the boot allocator. All the RAM but the following regions > + * will be added: > + * - Modules (e.g., Xen, Kernel) > + * - Reserved regions > + * - Xenheap > + */ > +static void __init populate_boot_allocator(void) > +{ > + unsigned int i; Shouldn't this be an int (as it was previously) because ... > + const struct meminfo *banks = &bootinfo.mem; > + > + for ( i = 0; i < banks->nr_banks; i++ ) ... nr_banks is int ? Apart from that: Reviewed-by: Michal Orzel <michal.orzel@arm.com> Cheers, Michal
On 23/05/2022 08:28, Michal Orzel wrote: > Hi Julien, Hi Michal, > > On 20.05.2022 14:09, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> In a follow-up patch, we will want to populate the boot allocator >> separately for arm64. The code will end up to be very similar to the one >> on arm32. So move out the code in a new helper populate_boot_allocator(). >> >> For now the code is still protected by CONFIG_ARM_32 to avoid any build >> failure on arm64. >> >> Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with >> xenheap_mfn_end as they are equivalent. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> --- >> >> Changes in v4: >> - Patch added >> --- >> xen/arch/arm/setup.c | 90 +++++++++++++++++++++++++------------------- >> 1 file changed, 51 insertions(+), 39 deletions(-) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index d5d0792ed48a..3d5a2283d4ef 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void) >> } >> >> #ifdef CONFIG_ARM_32 >> +/* >> + * Populate the boot allocator. All the RAM but the following regions >> + * will be added: >> + * - Modules (e.g., Xen, Kernel) >> + * - Reserved regions >> + * - Xenheap >> + */ >> +static void __init populate_boot_allocator(void) >> +{ >> + unsigned int i; > Shouldn't this be an int (as it was previously) because ... >> + const struct meminfo *banks = &bootinfo.mem; >> + >> + for ( i = 0; i < banks->nr_banks; i++ ) > ... nr_banks is int ? Hmmm... AFAIK banks->nr_banks never hold a negative value, so I am not sure why it was introduced as an "int". Looking through the code, we seem to have a mix of "unsigned int" and "int". There seem to be less on the latter, so I have sent a patch to switch nr_banks to "unsigned int" [1]. This is based on this series thought and I would like to keep the "unsigned int" here. > > Apart from that: > Reviewed-by: Michal Orzel <michal.orzel@arm.com> Thanks! Please let me know if this reviewed-by hold. Cheers, [1] https://lore.kernel.org/xen-devel/20220523194631.66262-1-julien@xen.org
On 23.05.2022 21:51, Julien Grall wrote: > > > On 23/05/2022 08:28, Michal Orzel wrote: >> Hi Julien, > > Hi Michal, > >> >> On 20.05.2022 14:09, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> In a follow-up patch, we will want to populate the boot allocator >>> separately for arm64. The code will end up to be very similar to the one >>> on arm32. So move out the code in a new helper populate_boot_allocator(). >>> >>> For now the code is still protected by CONFIG_ARM_32 to avoid any build >>> failure on arm64. >>> >>> Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with >>> xenheap_mfn_end as they are equivalent. >>> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> >>> --- >>> >>> Changes in v4: >>> - Patch added >>> --- >>> xen/arch/arm/setup.c | 90 +++++++++++++++++++++++++------------------- >>> 1 file changed, 51 insertions(+), 39 deletions(-) >>> >>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>> index d5d0792ed48a..3d5a2283d4ef 100644 >>> --- a/xen/arch/arm/setup.c >>> +++ b/xen/arch/arm/setup.c >>> @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void) >>> } >>> #ifdef CONFIG_ARM_32 >>> +/* >>> + * Populate the boot allocator. All the RAM but the following regions >>> + * will be added: >>> + * - Modules (e.g., Xen, Kernel) >>> + * - Reserved regions >>> + * - Xenheap >>> + */ >>> +static void __init populate_boot_allocator(void) >>> +{ >>> + unsigned int i; >> Shouldn't this be an int (as it was previously) because ... >>> + const struct meminfo *banks = &bootinfo.mem; >>> + >>> + for ( i = 0; i < banks->nr_banks; i++ ) >> ... nr_banks is int ? > > Hmmm... AFAIK banks->nr_banks never hold a negative value, so I am not sure why it was introduced as an "int". > > Looking through the code, we seem to have a mix of "unsigned int" and "int". There seem to be less on the latter, so I have sent a patch to switch nr_banks to "unsigned int" [1]. That's great, thanks. > > This is based on this series thought and I would like to keep the "unsigned int" here. > >> >> Apart from that: >> Reviewed-by: Michal Orzel <michal.orzel@arm.com> > > Thanks! Please let me know if this reviewed-by hold. Definitely yes. > > Cheers, > > [1] https://lore.kernel.org/xen-devel/20220523194631.66262-1-julien@xen.org > Cheers, Michal
Hi Julien, > On 20 May 2022, at 13:09, Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > In a follow-up patch, we will want to populate the boot allocator > separately for arm64. The code will end up to be very similar to the one > on arm32. So move out the code in a new helper populate_boot_allocator(). > > For now the code is still protected by CONFIG_ARM_32 to avoid any build > failure on arm64. > > Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with > xenheap_mfn_end as they are equivalent. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > > --- > > Changes in v4: > - Patch added > --- > xen/arch/arm/setup.c | 90 +++++++++++++++++++++++++------------------- > 1 file changed, 51 insertions(+), 39 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed48a..3d5a2283d4ef 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void) > } > > #ifdef CONFIG_ARM_32 > +/* > + * Populate the boot allocator. All the RAM but the following regions > + * will be added: > + * - Modules (e.g., Xen, Kernel) > + * - Reserved regions > + * - Xenheap > + */ > +static void __init populate_boot_allocator(void) > +{ > + unsigned int i; > + const struct meminfo *banks = &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; > + > + s = bank->start; > + while ( s < bank_end ) > + { > + paddr_t n = bank_end; > + > + 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; > + > + /* Avoid the xenheap */ > + if ( s < mfn_to_maddr(xenheap_mfn_end) && > + mfn_to_maddr(xenheap_mfn_start) < e ) > + { > + e = mfn_to_maddr(xenheap_mfn_start); > + n = mfn_to_maddr(xenheap_mfn_end); > + } > + > + fw_unreserved_regions(s, e, init_boot_pages, 0); > + s = n; > + } > + } > +} > + > static void __init setup_mm(void) > { > - paddr_t ram_start, ram_end, ram_size; > - paddr_t s, e; > + paddr_t ram_start, ram_end, ram_size, e; > unsigned long ram_pages; > unsigned long heap_pages, xenheap_pages, domheap_pages; > int i; > @@ -718,43 +766,7 @@ static void __init setup_mm(void) > setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages, xenheap_pages); > > /* Add non-xenheap memory */ > - for ( i = 0; i < bootinfo.mem.nr_banks; i++ ) > - { > - paddr_t bank_start = bootinfo.mem.bank[i].start; > - paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size; > - > - s = bank_start; > - while ( s < bank_end ) > - { > - paddr_t n = bank_end; > - > - e = next_module(s, &n); > - > - if ( e == ~(paddr_t)0 ) > - { > - e = n = ram_end; > - } > - > - /* > - * Module in a RAM bank other than the one which we are > - * not dealing with here. > - */ > - if ( e > bank_end ) > - e = bank_end; > - > - /* Avoid the xenheap */ > - if ( s < mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages)) > - && mfn_to_maddr(xenheap_mfn_start) < e ) > - { > - e = mfn_to_maddr(xenheap_mfn_start); > - n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages)); > - } > - > - fw_unreserved_regions(s, e, init_boot_pages, 0); > - > - s = n; > - } > - } > + populate_boot_allocator(); > > /* Frame table covers all of RAM region, including holes */ > setup_frametable_mappings(ram_start, ram_end); > -- > 2.32.0 >
On Fri, 20 May 2022, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > In a follow-up patch, we will want to populate the boot allocator > separately for arm64. The code will end up to be very similar to the one > on arm32. So move out the code in a new helper populate_boot_allocator(). > > For now the code is still protected by CONFIG_ARM_32 to avoid any build > failure on arm64. > > Take the opportunity to replace mfn_add(xen_mfn_start, xenheap_pages) with > xenheap_mfn_end as they are equivalent. > > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > Changes in v4: > - Patch added > --- > xen/arch/arm/setup.c | 90 +++++++++++++++++++++++++------------------- > 1 file changed, 51 insertions(+), 39 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index d5d0792ed48a..3d5a2283d4ef 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void) > } > > #ifdef CONFIG_ARM_32 > +/* > + * Populate the boot allocator. All the RAM but the following regions > + * will be added: > + * - Modules (e.g., Xen, Kernel) > + * - Reserved regions > + * - Xenheap > + */ > +static void __init populate_boot_allocator(void) > +{ > + unsigned int i; > + const struct meminfo *banks = &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; > + > + s = bank->start; > + while ( s < bank_end ) > + { > + paddr_t n = bank_end; > + > + 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; > + > + /* Avoid the xenheap */ > + if ( s < mfn_to_maddr(xenheap_mfn_end) && > + mfn_to_maddr(xenheap_mfn_start) < e ) > + { > + e = mfn_to_maddr(xenheap_mfn_start); > + n = mfn_to_maddr(xenheap_mfn_end); > + } > + > + fw_unreserved_regions(s, e, init_boot_pages, 0); > + s = n; > + } > + } > +} > + > static void __init setup_mm(void) > { > - paddr_t ram_start, ram_end, ram_size; > - paddr_t s, e; > + paddr_t ram_start, ram_end, ram_size, e; > unsigned long ram_pages; > unsigned long heap_pages, xenheap_pages, domheap_pages; > int i; > @@ -718,43 +766,7 @@ static void __init setup_mm(void) > setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages, xenheap_pages); > > /* Add non-xenheap memory */ > - for ( i = 0; i < bootinfo.mem.nr_banks; i++ ) > - { > - paddr_t bank_start = bootinfo.mem.bank[i].start; > - paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size; > - > - s = bank_start; > - while ( s < bank_end ) > - { > - paddr_t n = bank_end; > - > - e = next_module(s, &n); > - > - if ( e == ~(paddr_t)0 ) > - { > - e = n = ram_end; > - } > - > - /* > - * Module in a RAM bank other than the one which we are > - * not dealing with here. > - */ > - if ( e > bank_end ) > - e = bank_end; > - > - /* Avoid the xenheap */ > - if ( s < mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages)) > - && mfn_to_maddr(xenheap_mfn_start) < e ) > - { > - e = mfn_to_maddr(xenheap_mfn_start); > - n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages)); > - } > - > - fw_unreserved_regions(s, e, init_boot_pages, 0); > - > - s = n; > - } > - } > + populate_boot_allocator(); > > /* Frame table covers all of RAM region, including holes */ > setup_frametable_mappings(ram_start, ram_end); > -- > 2.32.0 >
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index d5d0792ed48a..3d5a2283d4ef 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -637,10 +637,58 @@ static void __init init_staticmem_pages(void) } #ifdef CONFIG_ARM_32 +/* + * Populate the boot allocator. All the RAM but the following regions + * will be added: + * - Modules (e.g., Xen, Kernel) + * - Reserved regions + * - Xenheap + */ +static void __init populate_boot_allocator(void) +{ + unsigned int i; + const struct meminfo *banks = &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; + + s = bank->start; + while ( s < bank_end ) + { + paddr_t n = bank_end; + + 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; + + /* Avoid the xenheap */ + if ( s < mfn_to_maddr(xenheap_mfn_end) && + mfn_to_maddr(xenheap_mfn_start) < e ) + { + e = mfn_to_maddr(xenheap_mfn_start); + n = mfn_to_maddr(xenheap_mfn_end); + } + + fw_unreserved_regions(s, e, init_boot_pages, 0); + s = n; + } + } +} + static void __init setup_mm(void) { - paddr_t ram_start, ram_end, ram_size; - paddr_t s, e; + paddr_t ram_start, ram_end, ram_size, e; unsigned long ram_pages; unsigned long heap_pages, xenheap_pages, domheap_pages; int i; @@ -718,43 +766,7 @@ static void __init setup_mm(void) setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages, xenheap_pages); /* Add non-xenheap memory */ - for ( i = 0; i < bootinfo.mem.nr_banks; i++ ) - { - paddr_t bank_start = bootinfo.mem.bank[i].start; - paddr_t bank_end = bank_start + bootinfo.mem.bank[i].size; - - s = bank_start; - while ( s < bank_end ) - { - paddr_t n = bank_end; - - e = next_module(s, &n); - - if ( e == ~(paddr_t)0 ) - { - e = n = ram_end; - } - - /* - * Module in a RAM bank other than the one which we are - * not dealing with here. - */ - if ( e > bank_end ) - e = bank_end; - - /* Avoid the xenheap */ - if ( s < mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages)) - && mfn_to_maddr(xenheap_mfn_start) < e ) - { - e = mfn_to_maddr(xenheap_mfn_start); - n = mfn_to_maddr(mfn_add(xenheap_mfn_start, xenheap_pages)); - } - - fw_unreserved_regions(s, e, init_boot_pages, 0); - - s = n; - } - } + populate_boot_allocator(); /* Frame table covers all of RAM region, including holes */ setup_frametable_mappings(ram_start, ram_end);