Re: [PATCH v7 7/9] xen/arm: create shared memory nodes in guest device tree

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 06/09/2022 09:59, Penny Zheng wrote:
> We expose the shared memory to the domU using the "xen,shared-memory-v1"
> reserved-memory binding. See
> Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt
> in Linux for the corresponding device tree binding.
>
> To save the cost of re-parsing shared memory device tree configuration when
> creating shared memory nodes in guest device tree, this commit adds new field
> "shm_mem" to store shm-info per domain.
>
> For each shared memory region, a range is exposed under
> the /reserved-memory node as a child node. Each range sub-node is
> named xen-shmem@<address> and has the following properties:
> - compatible:
>          compatible = "xen,shared-memory-v1"
> - reg:
>          the base guest physical address and size of the shared memory region
> - xen,id:
>          a string that identifies the shared memory region.
So technically, there is a property "xen,offset" that should be 
specified for the borrowers.
TBH, I don't quite understand what this property is used for. So it is 
not quite clear why this is skipped.
The Stefano is the author of the binding. So maybe he can explain the 
purpose of the property and help to document it in the commit message 
why this is ignored.
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d0ff487cc6..3b7436030e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -914,7 +914,22 @@ static int __init assign_shared_memory(struct domain *d,
>       return ret;
>   }
>   
> -static int __init process_shm(struct domain *d,
> +static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
> +                                            paddr_t start, paddr_t size,
> +                                            const char *shm_id)
> +{
> +    if ( (kinfo->shm_mem.nr_banks + 1) > NR_MEM_BANKS )
NIT: The +1 could be avoided if you use >=. This would also avoid to 
think about the overflow case for nr_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;
> +    safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
> +    kinfo->shm_mem.nr_banks++;
> +
> +    return 0;
> +}
> +
> +static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>                                 const struct dt_device_node *node)
>   {
>       struct dt_device_node *shm_node;
> @@ -928,6 +943,7 @@ static int __init process_shm(struct domain *d,
>           int ret = 0;
>           unsigned int i;
>           const char *role_str;
> +        const char *shm_id;
>           bool owner_dom_io = true;
>   
>           if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> @@ -972,6 +988,9 @@ static int __init process_shm(struct domain *d,
>           if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>               owner_dom_io = false;
>   
> +        dt_property_read_string(shm_node, "xen,shm-id", &shm_id);
shm_id will only be set if dt_property_read_string() returns 0. 
Otherwise it will be unknown. Given...
> +        BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
... this BUG_ON(), I assuming you want to double check that the property 
is correct. So you want to also check the return value.
Otherwise, I suspect a static analyzer will complain that you may use 
unitialized value.
[...]

> +static int __init make_resv_memory_node(const struct domain *d,
> +                                        void *fdt,
> +                                        int addrcells, int sizecells,
> +                                        struct meminfo *mem)
AFAICT 'mem' could be const.

> +{
> +    int res = 0;
> +    /* Placeholder for reserved-memory\0 */
> +    char resvbuf[16] = "reserved-memory";
Same here.

Cheers,

--
Julien Grall