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.fance...@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.or...@amd.com>
~Michal
