Re: [PATCH v7 1/9] xen/arm: introduce static shared memory

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 06/09/2022 09:59, Penny Zheng wrote:
> +    for ( i = 0; i < mem->nr_banks; i++ )
> +    {
> +        /*
> +         * Meet the following check:
> +         * 1) The shm ID matches and the region exactly match
> +         * 2) The shm ID doesn't match and the region doesn't overlap
> +         * with an existing one
> +         */
> +        if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> +        {
> +            if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
> +                break;
> +            else
> +            {
> +                printk("fdt: xen,shm-id %s does not match for all the nodes using 
> the same region.\n",
> +                       shm_id);
> +                return -EINVAL;
> +            }
> +        }
> +        else
> +        {
> +            paddr_t end = paddr + size;
> +            paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
> +
> +            if ( (end <= paddr) || (bank_end <= mem->bank[i].start) )
> +                printk("fdt: static shared memory region %s overflow\n", 
> shm_id);
Why are you continuing here?

> +
> +            if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
> +            {
> +                if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
> +                    continue;
> +                else
> +                {
> +                    printk("fdt: different shared memory region could not share the 
> same shm ID %s\n",
> +                           shm_id);
> +                    return -EINVAL;
> +                }
> +            }
> +            else
> +            {
> +                printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" 
> - %#"PRIpaddr"\n",
> +                        mem->bank[i].start, bank_end);
> +                return -EINVAL;
> +            }
> +        }
> +    }
> +
> +    if ( i == mem->nr_banks )
> +    {
> +        if ( i < NR_MEM_BANKS )
> +        {
> +            /* Static shared memory shall be reserved from any other use. */
> +            safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
> +            mem->bank[mem->nr_banks].start = paddr;
> +            mem->bank[mem->nr_banks].size = size;
> +            mem->bank[mem->nr_banks].xen_domain = true;
> +            mem->nr_banks++;
> +        }
> +        else
> +        {
> +            printk("Warning: Max number of supported memory regions 
> reached.\n");
> +            return -ENOSPC;
> +        }
> +    }
> +    /*
> +     * keep a count of the number of borrowers, which later may be used
> +     * to calculate the reference count.
> +     */
> +    if ( !owner )
> +        mem->bank[i].nr_shm_borrowers++;
> +
> +    return 0;
> +}
> +#else
> +static int __init process_shm_node(const void *fdt, int node,
> +                                   uint32_t address_cells, uint32_t size_cells)
> +{
> +    printk("CONFIG_STATIC_SHM must be enabled for parsing static shared memory 
> nodes\n");
> +    WARN();
NIT: The WARN() here seems pointless because the call trace is not deep 
*and* it would not be printed unless you are using earlyprintk.
Anyway, the only reason I am not acking this patch is related to the 
question I asked above.
Cheers,

--
Julien Grall