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

From: Julien Grall <julien@xen.org>
To: Stefano Stabellini <sstabellini@kernel.org>,
	Penny Zheng <Penny.Zheng@arm.com>
Cc: xen-devel@lists.xenproject.org, wei.chen@arm.com,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v2 8/9] xen/arm: create shared memory nodes in guest device tree
Date: Sat, 7 May 2022 12:26:05 +0100	[thread overview]
Message-ID: <ec3379a9-e182-4358-ccf8-e07712ea9dcd@xen.org> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2205061750400.43560@ubuntu-linux-20-04-desktop>

Hi,

On 07/05/2022 02:09, Stefano Stabellini wrote:
> On Fri, 6 May 2022, 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.
>>
>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>> ---
>> v2 change:
>> - using xzalloc
>> - shm_id should be uint8_t
>> - make reg a local variable
>> - add #address-cells and #size-cells properties
>> - fix alignment
>> ---
>>   xen/arch/arm/domain_build.c       | 144 ++++++++++++++++++++++++++++++
>>   xen/arch/arm/include/asm/domain.h |   1 +
>>   xen/arch/arm/include/asm/setup.h  |   1 +
>>   3 files changed, 146 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 8d299a3616..f08606d2c0 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -894,6 +894,26 @@ static int __init allocate_shared_memory(struct domain *d,
>>       return ret;
>>   }
>>   
>> +static int __init append_shm_bank_to_domain(struct domain *d,
>> +                                            paddr_t start, paddr_t size,
>> +                                            u32 shm_id)
>> +{
>> +    /* Allocate memory at first insertion. */
>> +    if ( d->arch.shm_mem == NULL )
>> +    {
>> +        d->arch.shm_mem = xzalloc(struct meminfo);
>> +        if ( d->arch.shm_mem == NULL )
>> +            return -ENOMEM;
>> +    }
>> +
>> +    d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].start = start;
>> +    d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].size = size;
>> +    d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].shm_id = shm_id;
>> +    d->arch.shm_mem->nr_banks++;

bank[] is a fixed size array. So we should check we don't overflow.

>> +
>> +    return 0;
>> +}
> 
> Can we call xfree(d->arch.shm_mem) at domain destruction?
> 
> Even better, we might be able to call it earlier, soon after the
> make_resv_memory_node() call because we don't need it any longer after
> that?

If this is only used during domain build, then the field should move to 
kernel_info. With that, there will also be no concern on whether we need 
to allocate the structure or not.

Cheers,

-- 
Julien Grall