| Message ID | 20221115025235.1378931-3-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory | expand |
Hello, Reading this patch, I found one place that can be improved. On 11/15/22 03: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.zheng@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) > { > 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 ) The target shared memory is found and the bank can be returned directly here (return &bootinfo.shm_mem.bank[bank];). Therefore, the out-of-bounds condition check can be removed below. > break; > - } > > if ( bank == bootinfo.shm_mem.nr_banks ) This can be removed, but only return NULL because the target memory is not found. > - 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) > { > 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, > 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; > > 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; > }
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.zheng@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,
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) { 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) { 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, 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; 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; }
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.zheng@arm.com> --- xen/arch/arm/domain_build.c | 46 ++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 23 deletions(-)