| Message ID | 20240409114543.3332150-7-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt1 | expand |
Hi Luca, On 09/04/2024 13:45, Luca Fancellu wrote: > > > The function find_unallocated_memory is using the same code to > loop through 3 structure of the same type, in order to avoid > code duplication, rework the code to have only one loop that > goes through all the structures. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > v2: > - Add comment in the loop inside find_unallocated_memory to > improve readability > v1: > - new patch > --- > --- > xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------ > 1 file changed, 25 insertions(+), 45 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 57cf92668ae6..269aaff4d067 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, > static int __init find_unallocated_memory(const struct kernel_info *kinfo, > struct membanks *ext_regions) > { > - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo); > - const struct membanks *mem = bootinfo_get_mem(); > - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); > + const struct membanks *mem_banks[] = { > + bootinfo_get_mem(), > + kernel_info_get_mem_const(kinfo), > + bootinfo_get_reserved_mem(), > + }; > struct rangeset *unalloc_mem; > paddr_t start, end; > - unsigned int i; > + unsigned int i, j; > int res; > > dt_dprintk("Find unallocated memory for extended regions\n"); > @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > if ( !unalloc_mem ) > return -ENOMEM; > > - /* Start with all available RAM */ > - for ( i = 0; i < mem->nr_banks; i++ ) > - { > - start = mem->bank[i].start; > - end = mem->bank[i].start + mem->bank[i].size; > - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), > - PFN_DOWN(end - 1)); > - if ( res ) > + /* > + * Exclude the following regions, in order: > + * 1) Start with all available RAM > + * 2) Remove RAM assigned to Dom0 > + * 3) Remove reserved memory Given this commit and the previous code, I expect one call to rangeset_add_range() and 3 calls to rangeset_remove_range(). However ... > + * The order comes from the initialization of the variable "mem_banks" > + * above > + */ > + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) > + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) > { > - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", > - start, end); > - goto out; > - } > - } > - > - /* Remove RAM assigned to Dom0 */ > - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) > - { > - start = kinfo_mem->bank[i].start; > - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; > - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), > + start = mem_banks[i]->bank[j].start; > + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size; > + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), ... here you always call rangeset_add_range() which is wrong. For direct mapped domain you would e.g. register its RAM region as extended region. ~Michal
> On 9 Apr 2024, at 14:38, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > > On 09/04/2024 13:45, Luca Fancellu wrote: >> >> >> The function find_unallocated_memory is using the same code to >> loop through 3 structure of the same type, in order to avoid >> code duplication, rework the code to have only one loop that >> goes through all the structures. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> v2: >> - Add comment in the loop inside find_unallocated_memory to >> improve readability >> v1: >> - new patch >> --- >> --- >> xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------ >> 1 file changed, 25 insertions(+), 45 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 57cf92668ae6..269aaff4d067 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, >> static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> struct membanks *ext_regions) >> { >> - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo); >> - const struct membanks *mem = bootinfo_get_mem(); >> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); >> + const struct membanks *mem_banks[] = { >> + bootinfo_get_mem(), >> + kernel_info_get_mem_const(kinfo), >> + bootinfo_get_reserved_mem(), >> + }; >> struct rangeset *unalloc_mem; >> paddr_t start, end; >> - unsigned int i; >> + unsigned int i, j; >> int res; >> >> dt_dprintk("Find unallocated memory for extended regions\n"); >> @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> if ( !unalloc_mem ) >> return -ENOMEM; >> >> - /* Start with all available RAM */ >> - for ( i = 0; i < mem->nr_banks; i++ ) >> - { >> - start = mem->bank[i].start; >> - end = mem->bank[i].start + mem->bank[i].size; >> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), >> - PFN_DOWN(end - 1)); >> - if ( res ) >> + /* >> + * Exclude the following regions, in order: >> + * 1) Start with all available RAM >> + * 2) Remove RAM assigned to Dom0 >> + * 3) Remove reserved memory > Given this commit and the previous code, I expect one call to rangeset_add_range() and > 3 calls to rangeset_remove_range(). However ... >> + * The order comes from the initialization of the variable "mem_banks" >> + * above >> + */ >> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) >> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) >> { >> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", >> - start, end); >> - goto out; >> - } >> - } >> - >> - /* Remove RAM assigned to Dom0 */ >> - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) >> - { >> - start = kinfo_mem->bank[i].start; >> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; >> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), >> + start = mem_banks[i]->bank[j].start; >> + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size; >> + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), > ... here you always call rangeset_add_range() which is wrong. For direct mapped domain > you would e.g. register its RAM region as extended region. Right, I read it wrong initially, my mistake, here we are adding all available ram and later removing the dom0 regions and reserved regions. Will fix > > ~Michal
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 57cf92668ae6..269aaff4d067 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, static int __init find_unallocated_memory(const struct kernel_info *kinfo, struct membanks *ext_regions) { - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo); - const struct membanks *mem = bootinfo_get_mem(); - const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); + const struct membanks *mem_banks[] = { + bootinfo_get_mem(), + kernel_info_get_mem_const(kinfo), + bootinfo_get_reserved_mem(), + }; struct rangeset *unalloc_mem; paddr_t start, end; - unsigned int i; + unsigned int i, j; int res; dt_dprintk("Find unallocated memory for extended regions\n"); @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, if ( !unalloc_mem ) return -ENOMEM; - /* Start with all available RAM */ - for ( i = 0; i < mem->nr_banks; i++ ) - { - start = mem->bank[i].start; - end = mem->bank[i].start + mem->bank[i].size; - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), - PFN_DOWN(end - 1)); - if ( res ) + /* + * Exclude the following regions, in order: + * 1) Start with all available RAM + * 2) Remove RAM assigned to Dom0 + * 3) Remove reserved memory + * The order comes from the initialization of the variable "mem_banks" + * above + */ + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) + for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) { - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); - goto out; - } - } - - /* Remove RAM assigned to Dom0 */ - for ( i = 0; i < kinfo_mem->nr_banks; i++ ) - { - start = kinfo_mem->bank[i].start; - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), + start = mem_banks[i]->bank[j].start; + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size; + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end - 1)); - if ( res ) - { - printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); - goto out; - } - } - - /* Remove reserved-memory regions */ - for ( i = 0; i < reserved_mem->nr_banks; i++ ) - { - start = reserved_mem->bank[i].start; - end = reserved_mem->bank[i].start + reserved_mem->bank[i].size; - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), - PFN_DOWN(end - 1)); - if ( res ) - { - printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); - goto out; + if ( res ) + { + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", + start, end); + goto out; + } } - } /* Remove grant table region */ if ( kinfo->gnttab_size )
The function find_unallocated_memory is using the same code to loop through 3 structure of the same type, in order to avoid code duplication, rework the code to have only one loop that goes through all the structures. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v2: - Add comment in the loop inside find_unallocated_memory to improve readability v1: - new patch --- --- xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------ 1 file changed, 25 insertions(+), 45 deletions(-)