Hi 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.zh...@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