This commit re-arranges the static shared memory regions into a separate array
shm_meminfo. And static shared memory region now would have its own structure
'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer
to the normal memory bank 'membank'. This will avoid continuing to grow
'membank'.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
xen/arch/arm/include/asm/kernel.h | 2 +-
xen/arch/arm/include/asm/setup.h | 16 +++++++++----
4 files changed, 59 insertions(+), 34 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..ccf281cd37 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int node,
const __be32 *cell;
paddr_t paddr, gaddr, size;
struct meminfo *mem = &bootinfo.reserved_mem;
+ struct shm_meminfo *shm_mem = &bootinfo.shm_mem;
unsigned int i;
int len;
bool owner = false;
@@ -455,17 +456,21 @@ static int __init process_shm_node(const void *fdt, int node,
return -EINVAL;
}
- for ( i = 0; i < mem->nr_banks; i++ )
+ for ( i = 0; i < shm_mem->nr_banks; i++ )
{
+ paddr_t bank_start = shm_mem->bank[i].membank->start;
+ paddr_t bank_size = shm_mem->bank[i].membank->size;
+
/*
* Meet the following check:
* 1) The shm ID matches and the region exactly match
* 2) The shm ID doesn't match and the region doesn't overlap
* with an existing one
*/
- if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
+ if ( paddr == bank_start && size == bank_size )
{
- if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
+ if ( strncmp(shm_id,
+ shm_mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
break;
else
{
@@ -477,17 +482,17 @@ static int __init process_shm_node(const void *fdt, int node,
else
{
paddr_t end = paddr + size;
- paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
+ paddr_t bank_end = bank_start + bank_size;
- if ( (end <= paddr) || (bank_end <= mem->bank[i].start) )
+ if ( (end <= paddr) || (bank_end <= bank_start) )
{
printk("fdt: static shared memory region %s overflow\n", shm_id);
return -EINVAL;
}
- if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
+ if ( (end <= bank_start) || (paddr >= bank_end) )
{
- if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
+ if ( strcmp(shm_id, shm_mem->bank[i].shm_id) != 0 )
continue;
else
{
@@ -499,22 +504,27 @@ static int __init process_shm_node(const void *fdt, int node,
else
{
printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
- mem->bank[i].start, bank_end);
+ bank_start, bank_end);
return -EINVAL;
}
}
}
- if ( i == mem->nr_banks )
+ if ( i == shm_mem->nr_banks )
{
- if ( i < NR_MEM_BANKS )
+ if ( (i < NR_MEM_BANKS) && (mem->nr_banks < NR_MEM_BANKS) )
{
/* Static shared memory shall be reserved from any other use. */
- safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
- mem->bank[mem->nr_banks].start = paddr;
- mem->bank[mem->nr_banks].size = size;
- mem->bank[mem->nr_banks].type = MEMBANK_STATIC_DOMAIN;
+ struct membank *membank = &mem->bank[mem->nr_banks];
+
+ membank->start = paddr;
+ membank->size = size;
+ membank->type = MEMBANK_STATIC_DOMAIN;
mem->nr_banks++;
+
+ safe_strcpy(shm_mem->bank[i].shm_id, shm_id);
+ shm_mem->bank[i].membank = membank;
+ shm_mem->nr_banks++;
}
else
{
@@ -527,7 +537,7 @@ static int __init process_shm_node(const void *fdt, int node,
* to calculate the reference count.
*/
if ( !owner )
- mem->bank[i].nr_shm_borrowers++;
+ shm_mem->bank[i].nr_shm_borrowers++;
return 0;
}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bd30d3798c..c0fd13f6ed 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct domain *d,
{
unsigned int bank;
- /* Iterate reserved memory to find requested shm bank. */
- for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
+ /* Iterate static shared memory to find requested shm bank. */
+ for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
{
- paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
- paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
+ paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
+ paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
if ( (pbase == bank_start) && (psize == bank_size) )
break;
}
- if ( bank == bootinfo.reserved_mem.nr_banks )
+ if ( bank == bootinfo.shm_mem.nr_banks )
return -ENOENT;
- *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
+ *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
return 0;
}
@@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
paddr_t start, paddr_t size,
const char *shm_id)
{
+ struct membank *membank;
+
if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
return -ENOMEM;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
+ membank = xmalloc_bytes(sizeof(struct membank));
+ if ( membank == NULL )
+ return -ENOMEM;
+
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size;
safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
kinfo->shm_mem.nr_banks++;
@@ -1355,7 +1362,7 @@ static int __init make_memory_node(const struct domain *d,
static int __init make_shm_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct meminfo *mem)
+ const struct shm_meminfo *mem)
{
unsigned int i = 0;
int res = 0;
@@ -1372,8 +1379,8 @@ static int __init make_shm_memory_node(const struct domain *d,
for ( ; i < mem->nr_banks; i++ )
{
- uint64_t start = mem->bank[i].start;
- uint64_t size = mem->bank[i].size;
+ uint64_t start = mem->bank[i].membank->start;
+ uint64_t size = mem->bank[i].membank->size;
/* Placeholder for xen-shmem@ + a 64-bit number + \0 */
char buf[27];
const char compat[] = "xen,shared-memory-v1";
@@ -1382,7 +1389,7 @@ static int __init make_shm_memory_node(const struct domain *d,
__be32 *cells;
unsigned int len = (addrcells + sizecells) * sizeof(__be32);
- snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
+ snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, start);
res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1426,7 +1433,7 @@ static int __init make_shm_memory_node(const struct domain *d,
static int __init make_shm_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct meminfo *mem)
+ const struct shm_meminfo *mem)
{
ASSERT_UNREACHABLE();
return -EOPNOTSUPP;
@@ -1436,7 +1443,7 @@ static int __init make_shm_memory_node(const struct domain *d,
static int __init make_resv_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct meminfo *mem)
+ const struct shm_meminfo *mem)
{
int res = 0;
/* Placeholder for reserved-memory\0 */
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 5bb30c3f2f..f47ba9d619 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -38,7 +38,7 @@ struct kernel_info {
void *fdt; /* flat device tree */
paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
struct meminfo mem;
- struct meminfo shm_mem;
+ struct shm_meminfo shm_mem;
/* kernel entry point */
paddr_t entry;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index fdbf68aadc..2d4ae0f00a 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -50,10 +50,6 @@ struct membank {
paddr_t start;
paddr_t size;
enum membank_type type;
-#ifdef CONFIG_STATIC_SHM
- char shm_id[MAX_SHM_ID_LENGTH];
- unsigned int nr_shm_borrowers;
-#endif
};
struct meminfo {
@@ -61,6 +57,17 @@ struct meminfo {
struct membank bank[NR_MEM_BANKS];
};
+struct shm_membank {
+ char shm_id[MAX_SHM_ID_LENGTH];
+ unsigned int nr_shm_borrowers;
+ struct membank *membank;
+};
+
+struct shm_meminfo {
+ unsigned int nr_banks;
+ struct shm_membank bank[NR_MEM_BANKS];
+};
+
/*
* The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
* The purpose of the domU flag is to avoid getting confused in
@@ -105,6 +112,7 @@ struct bootinfo {
struct meminfo acpi;
#endif
bool static_heap;
+ struct shm_meminfo shm_mem;
};
struct map_range_data
--
2.25.1Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
> This commit re-arranges the static shared memory regions into a separate array
> shm_meminfo. And static shared memory region now would have its own structure
> 'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer
> to the normal memory bank 'membank'. This will avoid continuing to grow
> 'membank'.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
> xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
> xen/arch/arm/include/asm/kernel.h | 2 +-
> xen/arch/arm/include/asm/setup.h | 16 +++++++++----
> 4 files changed, 59 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 6014c0f852..ccf281cd37 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int node,
> const __be32 *cell;
> paddr_t paddr, gaddr, size;
> struct meminfo *mem = &bootinfo.reserved_mem;
After this patch, 'mem' is barely going to be used. So I would recommend
to remove it or restrict the scope.
This will make easier to confirm that most of the use of 'mem' have been
replaced with 'shm_mem' and reduce the risk of confusion between the two
(the name are quite similar).
[...]
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bd30d3798c..c0fd13f6ed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct domain *d,
> {
> unsigned int bank;
>
> - /* Iterate reserved memory to find requested shm bank. */
> - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> + /* Iterate static shared memory to find requested shm bank. */
> + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
> {
> - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
> + paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be
removed. But it looks like there was none. I guess that was a mistake in
the existing code?
>
> if ( (pbase == bank_start) && (psize == bank_size) )
> break;
> }
>
> - if ( bank == bootinfo.reserved_mem.nr_banks )
> + if ( bank == bootinfo.shm_mem.nr_banks )
> return -ENOENT;
>
> - *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
> + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
>
> return 0;
> }
> @@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
> paddr_t start, paddr_t size,
> const char *shm_id)
> {
> + struct membank *membank;
> +
> if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
> return -ENOMEM;
>
> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
> + membank = xmalloc_bytes(sizeof(struct membank));
You allocate memory but never free it. However, I think it would be
better to avoid the dynamic allocation. So I would consider to not use
the structure shm_membank and instead create a specific one for the
domain construction.
> + if ( membank == NULL )
> + return -ENOMEM;
> +
> + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank;
> + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start;
> + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size;
The last two could be replaced with:
membank->start = start;
membank->size = size;
This would make the code more readable. Also, while you are modifying
the code, I would consider to introduce a local variable that points to
kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].
[...]
> struct meminfo {
> @@ -61,6 +57,17 @@ struct meminfo {
> struct membank bank[NR_MEM_BANKS];
> };
>
> +struct shm_membank {
> + char shm_id[MAX_SHM_ID_LENGTH];
> + unsigned int nr_shm_borrowers;
> + struct membank *membank;
After the change I suggest above, I would expect that the field of
membank will not be updated. So I would add const here.
Cheers,
--
Julien Grall
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Sunday, January 8, 2023 7:44 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory
> region
>
> Hi Penny,
>
Hi Julien
> On 15/11/2022 02:52, Penny Zheng wrote:
> > This commit re-arranges the static shared memory regions into a
> > separate array shm_meminfo. And static shared memory region now
> would
> > have its own structure 'shm_membank' to hold all shm-related members,
> > like shm_id, etc, and a pointer to the normal memory bank 'membank'.
> > This will avoid continuing to grow 'membank'.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
> > xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
> > xen/arch/arm/include/asm/kernel.h | 2 +-
> > xen/arch/arm/include/asm/setup.h | 16 +++++++++----
> > 4 files changed, 59 insertions(+), 34 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> > 6014c0f852..ccf281cd37 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt,
> int node,
> > const __be32 *cell;
> > paddr_t paddr, gaddr, size;
> > struct meminfo *mem = &bootinfo.reserved_mem;
>
> After this patch, 'mem' is barely going to be used. So I would recommend to
> remove it or restrict the scope.
>
Hope I understand correctly, you are saying that all static shared memory bank will be
described as "struct shm_membank". That's right.
However when host address is provided, we still need an instance of "struct membank"
to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in
as static pages.
That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the
same object.
If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like
Init_staticmem_pages, dt_unreserved_regions, etc
> This will make easier to confirm that most of the use of 'mem' have been
> replaced with 'shm_mem' and reduce the risk of confusion between the two
> (the name are quite similar).
>
> [...]
>
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index bd30d3798c..c0fd13f6ed 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -757,20 +757,20 @@ static int __init
> acquire_nr_borrower_domain(struct domain *d,
> > {
> > unsigned int bank;
> >
> > - /* Iterate reserved memory to find requested shm bank. */
> > - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> > + /* Iterate static shared memory to find requested shm bank. */
> > + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
> > {
> > - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> > - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> > + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank-
> >start;
> > + paddr_t bank_size =
> > + bootinfo.shm_mem.bank[bank].membank->size;
>
> I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be
> removed. But it looks like there was none. I guess that was a mistake in the
> existing code?
>
Oh, you're right, the type shall also be checked.
> >
> > if ( (pbase == bank_start) && (psize == bank_size) )
> > break;
> > }
> >
> > - if ( bank == bootinfo.reserved_mem.nr_banks )
> > + if ( bank == bootinfo.shm_mem.nr_banks )
> > return -ENOENT;
> >
> > - *nr_borrowers =
> bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
> > + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
> >
> > return 0;
> > }
> > @@ -907,11 +907,18 @@ static int __init
> append_shm_bank_to_domain(struct kernel_info *kinfo,
> > paddr_t start, paddr_t size,
> > const char *shm_id)
> > {
> > + struct membank *membank;
> > +
> > if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
> > return -ENOMEM;
> >
> > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
> > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
> > + membank = xmalloc_bytes(sizeof(struct membank));
>
> You allocate memory but never free it. However, I think it would be better to
> avoid the dynamic allocation. So I would consider to not use the structure
> shm_membank and instead create a specific one for the domain construction.
>
True, a local variable "struct meminfo shm_banks" could be introduced only
for domain construction in function construct_domU
> > + if ( membank == NULL )
> > + return -ENOMEM;
> > +
> > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank =
> membank;
> > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start =
> start;
> > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size =
> > + size;
>
> The last two could be replaced with:
>
> membank->start = start;
> membank->size = size;
>
> This would make the code more readable. Also, while you are modifying the
> code, I would consider to introduce a local variable that points to
> kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].
>
> [...]
>
> > struct meminfo {
> > @@ -61,6 +57,17 @@ struct meminfo {
> > struct membank bank[NR_MEM_BANKS];
> > };
> >
> > +struct shm_membank {
> > + char shm_id[MAX_SHM_ID_LENGTH];
> > + unsigned int nr_shm_borrowers;
> > + struct membank *membank;
>
> After the change I suggest above, I would expect that the field of
> membank will not be updated. So I would add const here.
>
> Cheers,
>
> --
> Julien Grall
Hi Penny,
On 09/01/2023 07:48, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Sunday, January 8, 2023 7:44 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory
>> region
>> On 15/11/2022 02:52, Penny Zheng wrote:
>>> This commit re-arranges the static shared memory regions into a
>>> separate array shm_meminfo. And static shared memory region now
>> would
>>> have its own structure 'shm_membank' to hold all shm-related members,
>>> like shm_id, etc, and a pointer to the normal memory bank 'membank'.
>>> This will avoid continuing to grow 'membank'.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
>>> xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
>>> xen/arch/arm/include/asm/kernel.h | 2 +-
>>> xen/arch/arm/include/asm/setup.h | 16 +++++++++----
>>> 4 files changed, 59 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
>>> 6014c0f852..ccf281cd37 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt,
>> int node,
>>> const __be32 *cell;
>>> paddr_t paddr, gaddr, size;
>>> struct meminfo *mem = &bootinfo.reserved_mem;
>>
>> After this patch, 'mem' is barely going to be used. So I would recommend to
>> remove it or restrict the scope.
>>
>
> Hope I understand correctly, you are saying that all static shared memory bank will be
> described as "struct shm_membank". That's right.
> However when host address is provided, we still need an instance of "struct membank"
> to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in
> as static pages.
> That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the
> same object.
I wasn't talking about the field in "struct shm_membank". Instead, I was
referring to the local variable:
struct meminfo *mem = &bootinfo.reserved_mem;
AFAICT, the only use after this patch is when you add a new bank in
shm_mem. So you could restrict the scope of the local variable.
> If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like
> Init_staticmem_pages, dt_unreserved_regions, etc
>
>> This will make easier to confirm that most of the use of 'mem' have been
>> replaced with 'shm_mem' and reduce the risk of confusion between the two
>> (the name are quite similar).
>>
>> [...]
>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index bd30d3798c..c0fd13f6ed 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -757,20 +757,20 @@ static int __init
>> acquire_nr_borrower_domain(struct domain *d,
>>> {
>>> unsigned int bank;
>>>
>>> - /* Iterate reserved memory to find requested shm bank. */
>>> - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
>>> + /* Iterate static shared memory to find requested shm bank. */
>>> + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
>>> {
>>> - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
>>> - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
>>> + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank-
>>> start;
>>> + paddr_t bank_size =
>>> + bootinfo.shm_mem.bank[bank].membank->size;
>>
>> I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be
>> removed. But it looks like there was none. I guess that was a mistake in the
>> existing code?
>>
>
> Oh, you're right, the type shall also be checked.
Just to clarify, with this patch you don't need to check the type. I was
pointing out a latent error in the existing code.
>
>>>
>>> if ( (pbase == bank_start) && (psize == bank_size) )
>>> break;
>>> }
>>>
>>> - if ( bank == bootinfo.reserved_mem.nr_banks )
>>> + if ( bank == bootinfo.shm_mem.nr_banks )
>>> return -ENOENT;
>>>
>>> - *nr_borrowers =
>> bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
>>> + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
>>>
>>> return 0;
>>> }
>>> @@ -907,11 +907,18 @@ static int __init
>> append_shm_bank_to_domain(struct kernel_info *kinfo,
>>> paddr_t start, paddr_t size,
>>> const char *shm_id)
>>> {
>>> + struct membank *membank;
>>> +
>>> if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
>>> return -ENOMEM;
>>>
>>> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
>>> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
>>> + membank = xmalloc_bytes(sizeof(struct membank));
>>
>> You allocate memory but never free it. However, I think it would be better to
>> avoid the dynamic allocation. So I would consider to not use the structure
>> shm_membank and instead create a specific one for the domain construction.
>>
>
> True, a local variable "struct meminfo shm_banks" could be introduced only
> for domain construction in function construct_domU
Hmmm... I didn't suggest to introduce a local variable. I would still
much prefer if we keep using 'kinfo' because we keep all the domain
building information in one place.
So ``struct meninfo`` should want to be defined in ``kinfo``.
Cheers,
--
Julien Grall
© 2016 - 2023 Red Hat, Inc.