Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
Instead of using multiple function parameters to deliver various shm-related
info, like host physical address, SHMID, etc, and with the introduction
of new struct "shm_membank", we could switch to use "shm_membank" as
function parameter to replace them all, to make codes more clear and
tidy.
Signed-off-by: Penny Zheng <penny.zh...@arm.com>
---
xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c0fd13f6ed..d2b9e60b5c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain
*d,
}
#ifdef CONFIG_STATIC_SHM
-static int __init acquire_nr_borrower_domain(struct domain *d,
- paddr_t pbase, paddr_t psize,
- unsigned long *nr_borrowers)
+static struct shm_membank * __init acquire_shm_membank(const char *shm_id)
You are returning bootinfo. AFAICT, nobody you modify it after it was
populated. So can this be const?
Also, I think the function wants to be renamed because this is too
similar to the function acquire_shared_memory_bank().
I would rename it to find_shared_memory_bank().
{
unsigned int 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.shm_mem.bank[bank].membank->start;
- paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
-
- if ( (pbase == bank_start) && (psize == bank_size) )
+ if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )
break;
- }
if ( bank == bootinfo.shm_mem.nr_banks )
- return -ENOENT;
-
- *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
+ return NULL;
- return 0;
+ return &bootinfo.shm_mem.bank[bank];
}
/*
* This function checks whether the static shared memory region is
* already allocated to dom_io.
*/
-static bool __init is_shm_allocated_to_domio(paddr_t pbase)
+static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
AFAICT, the function will not modify shm_membank. So please use const.
{
struct page_info *page;
struct domain *d;
- page = maddr_to_page(pbase);
+ page = maddr_to_page(shm_membank->membank->start);
d = page_get_owner_and_reference(page);
if ( d == NULL )
return false;
@@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct
domain *d,
}
static int __init assign_shared_memory(struct domain *d,
- uint32_t addr_cells, uint32_t
size_cells,
- paddr_t pbase, paddr_t psize,
+ struct shm_membank *shm_membank,
Same here.
paddr_t gbase)
{
mfn_t smfn;
int ret = 0;
unsigned long nr_pages, nr_borrowers, i;
struct page_info *page;
+ paddr_t pbase, psize;
+
+ pbase = shm_membank->membank->start;
+ psize = shm_membank->membank->size;
printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
d, pbase, pbase + psize);
@@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
* Get the right amount of references per page, which is the number of
* borrower domains.
*/
- ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
- if ( ret )
- return ret;
+ nr_borrowers = shm_membank->nr_shm_borrowers;
/*
* Instead of letting borrower domain get a page ref, we add as many
@@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct
kernel_info *kinfo,
const char *role_str;
const char *shm_id;
bool owner_dom_io = true;
+ struct shm_membank *shm_membank;
same here.
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
@@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct
kernel_info *kinfo,
}
BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >=
MAX_SHM_ID_LENGTH));
+ shm_membank = acquire_shm_membank(shm_id);
+ if ( !shm_membank )
+ {
+ printk("%pd: failed to acquire %s shared memory bank\n",
+ d, shm_id);
+ return -ENOENT;
+ }
+
/*
* DOMID_IO is a fake domain and is not described in the Device-Tree.
* Therefore when the owner of the shared region is DOMID_IO, we will
* only find the borrowers.
*/
- if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
+ if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
(!owner_dom_io && strcmp(role_str, "owner") == 0) )
{
/*
@@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct
kernel_info *kinfo,
* specified, so they should be assigned to dom_io.
*/
ret = assign_shared_memory(owner_dom_io ? dom_io : d,
- addr_cells, size_cells,
- pbase, psize, gbase);
+ shm_membank, gbase);
if ( ret )
return ret;
}
Cheers,
--
Julien Grall