| Message ID | 20240515142626.3480640-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt2 | expand |
Hi Luca, On 15/05/2024 16:26, Luca Fancellu wrote: > > > Wrap the code and logic that is calling assign_shared_memory > and map_regions_p2mt into a new function 'handle_shared_mem_bank', > it will become useful later when the code will allow the user to > don't pass the host physical address. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > v2 changes: > - add blank line, move owner_dom_io computation inside > handle_shared_mem_bank in order to reduce args count, remove > not needed BUGON(). (Michal) > --- > xen/arch/arm/static-shmem.c | 87 ++++++++++++++++++++++--------------- > 1 file changed, 53 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index 0afc86c43f85..8a14d120690c 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, > return 0; > } > > +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, > + const char *role_str, > + const struct membank *shm_bank) > +{ > + bool owner_dom_io = true; > + paddr_t pbase, psize; > + int ret; > + > + pbase = shm_bank->start; > + psize = shm_bank->size; > + > + /* > + * "role" property is optional and if it is defined explicitly, > + * then the owner domain is not the default "dom_io" domain. > + */ > + if ( role_str != NULL ) > + owner_dom_io = false; > + > + /* > + * 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)) || > + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) > + { > + /* > + * We found the first borrower of the region, the owner was not > + * specified, so they should be assigned to dom_io. > + */ > + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank); > + if ( ret ) > + return ret; > + } > + > + if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) > + { > + /* Set up P2M foreign mapping for borrower domain. */ > + ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), > + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); > + if ( ret ) > + return ret; > + } > + > + return 0; > +} > + > int __init process_shm(struct domain *d, struct kernel_info *kinfo, > const struct dt_device_node *node) > { > @@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > paddr_t gbase, pbase, psize; > int ret = 0; > unsigned int i; > - const char *role_str; > + const char *role_str = NULL; > const char *shm_id; > - bool owner_dom_io = true; > > if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) > continue; > @@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > return -EINVAL; > } > > - /* > - * "role" property is optional and if it is defined explicitly, > - * then the owner domain is not the default "dom_io" domain. > - */ > - if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) > - owner_dom_io = false; > + /* "role" property is optional */ > + dt_property_read_string(shm_node, "role", &role_str); This now violates a MISRA rule saying that if a function returns a value, this value needs to be checked. I think you should check if return value is non zero and if such, assign role_str NULL (thus removing such assignment from a definition). Other than that: Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi Michal, >> >> - /* >> - * "role" property is optional and if it is defined explicitly, >> - * then the owner domain is not the default "dom_io" domain. >> - */ >> - if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) >> - owner_dom_io = false; >> + /* "role" property is optional */ >> + dt_property_read_string(shm_node, "role", &role_str); > This now violates a MISRA rule saying that if a function returns a value, this value needs to be checked. > I think you should check if return value is non zero and if such, assign role_str NULL (thus removing such > assignment from a definition). Sure, I’ll do it. > > Other than that: > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > > ~Michal Cheers, Luca
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 0afc86c43f85..8a14d120690c 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, return 0; } +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, + const char *role_str, + const struct membank *shm_bank) +{ + bool owner_dom_io = true; + paddr_t pbase, psize; + int ret; + + pbase = shm_bank->start; + psize = shm_bank->size; + + /* + * "role" property is optional and if it is defined explicitly, + * then the owner domain is not the default "dom_io" domain. + */ + if ( role_str != NULL ) + owner_dom_io = false; + + /* + * 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)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) + { + /* + * We found the first borrower of the region, the owner was not + * specified, so they should be assigned to dom_io. + */ + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank); + if ( ret ) + return ret; + } + + if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) + { + /* Set up P2M foreign mapping for borrower domain. */ + ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); + if ( ret ) + return ret; + } + + return 0; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, paddr_t gbase, pbase, psize; int ret = 0; unsigned int i; - const char *role_str; + const char *role_str = NULL; const char *shm_id; - bool owner_dom_io = true; if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) continue; @@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, return -EINVAL; } - /* - * "role" property is optional and if it is defined explicitly, - * then the owner domain is not the default "dom_io" domain. - */ - if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) - owner_dom_io = false; + /* "role" property is optional */ + dt_property_read_string(shm_node, "role", &role_str); - /* - * 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)) || - (!owner_dom_io && strcmp(role_str, "owner") == 0) ) - { - /* - * We found the first borrower of the region, the owner was not - * specified, so they should be assigned to dom_io. - */ - ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, - boot_shm_bank); - if ( ret ) - return ret; - } - - if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) - { - /* Set up P2M foreign mapping for borrower domain. */ - ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), - _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); - if ( ret ) - return ret; - } + ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank); + if ( ret ) + return ret; /* * Record static shared memory region info for later setting
Wrap the code and logic that is calling assign_shared_memory and map_regions_p2mt into a new function 'handle_shared_mem_bank', it will become useful later when the code will allow the user to don't pass the host physical address. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- v2 changes: - add blank line, move owner_dom_io computation inside handle_shared_mem_bank in order to reduce args count, remove not needed BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 87 ++++++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 34 deletions(-)