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

[Julien Grall <julien@xen.org>]

Hi Penny,

On 29/08/2022 07:57, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Friday, August 26, 2022 9:17 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>> <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v6 1/9] xen/arm: introduce static shared memory
>>
>> Hi Penny,
>>
> 
> Hi Julien
>   
>> On 21/07/2022 14:21, Penny Zheng wrote:
>>> From: Penny Zheng <penny.zheng@arm.com>
>>>
>>> This patch series introduces a new feature: setting up static shared
>>> memory on a dom0less system, through device tree configuration.
>>>
>>> This commit parses shared memory node at boot-time, and reserve it in
>>> bootinfo.reserved_mem to avoid other use.
>>>
>>> This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
>>> static-shm-related codes, and this option depends on static memory(
>>> CONFIG_STATIC_MEMORY). That's because that later we want to reuse a
>>> few helpers, guarded with CONFIG_STATIC_MEMORY, like
>>> acquire_staticmem_pages, etc, on static shared memory.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> v6 change:
>>> - when host physical address is ommited, output the error message
>>> since xen doesn't support it at the moment
>>> - add 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
>>> - change it to "unsigned int" to be aligned with nr_banks
>>> - check the len of the property to confirm is it big enough to contain
>>> "paddr", "size", and "gaddr"
>>> - shm_id defined before nr_shm_domain, so we could re-use the existing
>>> hole and avoid increasing the size of the structure.
>>> - change "nr_shm_domain" to "nr_shm_borrowers", to not increment if
>>> the role is owner in parsing code
>>> - make "xen,shm_id" property as arbitrary string, with a strict limit
>>> on the number of characters, MAX_SHM_ID_LENGTH
>>> ---
>>> v5 change:
>>> - no change
>>> ---
>>> v4 change:
>>> - nit fix on doc
>>> ---
>>> v3 change:
>>> - make nr_shm_domain unsigned int
>>> ---
>>> v2 change:
>>> - document refinement
>>> - remove bitmap and use the iteration to check
>>> - add a new field nr_shm_domain to keep the number of shared domain
>>> ---
>>>    docs/misc/arm/device-tree/booting.txt | 124 ++++++++++++++++++++
>>>    xen/arch/arm/Kconfig                  |   6 +
>>>    xen/arch/arm/bootfdt.c                | 157 ++++++++++++++++++++++++++
>>>    xen/arch/arm/include/asm/setup.h      |   9 ++
>>>    4 files changed, 296 insertions(+)
>>>
>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>> b/docs/misc/arm/device-tree/booting.txt
>>> index 98253414b8..8013fb98fe 100644
>>> --- a/docs/misc/arm/device-tree/booting.txt
>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>> @@ -378,3 +378,127 @@ device-tree:
>>>
>>>    This will reserve a 512MB region starting at the host physical address
>>>    0x30000000 to be exclusively used by DomU1.
>>> +
>>> +Static Shared Memory
>>> +====================
>>> +
>>> +The static shared memory device tree nodes allow users to statically
>>> +set up shared memory on dom0less system, enabling domains to do
>>> +shm-based communication.
>>> +
>>> +- compatible
>>> +
>>> +    "xen,domain-shared-memory-v1"
>>> +
>>> +- xen,shm-id
>>> +
>>> +    An arbitrary string that represents the unique identifier of the shared
>>> +    memory region, with a strict limit on the number of characters(\0
>> included),
>>> +    `MAX_SHM_ID_LENGTH(16)`. e.g. "xen,shm-id = "my-shared-mem-1"".
>>> +
>>> +- xen,shared-mem
>>> +
>>> +    An array takes a physical address, which is the base address of the
>>> +    shared memory region in host physical address space, a size, and a
>> guest
>>> +    physical address, as the target address of the mapping.
>>> +    e.g. xen,shared-mem = < [host physical address] [size] [guest
>>> + address] >
>>
>> Your implementation below is checking for overlap and also have some
>> restriction. Can they be documented in the binding?
>>
>>> +
>>> +    The number of cells for the host address (and size) is the same as the
>>> +    guest pseudo-physical address and they are inherited from the parent
>> node.
>>
>> In v5, we discussed to have the host address optional. However, the binding
>> has not been updated to reflect that. Note that I am not asking to implement,
>> but instead request that the binding can be used for such setup.
>>
> 
> How about:
> "
> Host physical address could be omitted by users, and let Xen decide where it locates.
> "

I am fine with that.

> Do you think I shall further point out that right now, this part feature is not implemented
> in codes?

I have made a couple of suggestion for the code. But I think the binding 
would look a bit odd without the host physical address. We would now have:

< [size] [guest address]>

I think it would be more natural if we had

<[guest address] [size]>

And

<[guest address] [size] [host physical address]>

> 
>>> a/xen/arch/arm/include/asm/setup.h
>> b/xen/arch/arm/include/asm/setup.h
>>> index 2bb01ecfa8..39d4e93b8b 100644
>>> --- a/xen/arch/arm/include/asm/setup.h
>>> +++ b/xen/arch/arm/include/asm/setup.h
>>> @@ -23,10 +23,19 @@ typedef enum {
>>>    }  bootmodule_kind;
>>>
>>>
>>> +#ifdef CONFIG_STATIC_SHM
>>> +/* Indicates the maximum number of characters(\0 included) for shm_id
>>> +*/ #define MAX_SHM_ID_LENGTH 16 #endif
>>
>> Is the #ifdef really needed?
>>
>>> +
>>>    struct membank {
>>>        paddr_t start;
>>>        paddr_t size;
>>>        bool xen_domain; /* whether the memory bank is bound to a Xen
>>> domain. */
>>> +#ifdef CONFIG_STATIC_SHM
>>> +    char shm_id[MAX_SHM_ID_LENGTH];
>>> +    unsigned int nr_shm_borrowers;
>>> +#endif
>>>    };
>>
>> If I calculated right, the structure will grow from 24 to 40 bytes. At the
>> moment, this is protected with CONFIG_STATIC_SHM which is unsupported.
>> However, I think we will need to do something as we can't continue to grow
>> 'membank' like that.
>>
>> I don't have a quick suggestion for 4.17 (the feature freeze is in a week). Long
>> term, I think we will want to consider to move the shm ID in a separate array
>> that could be referenced here.
>>
>> The other solution would be to have the shared memory regions in a
>> separate array. They would have their own structure which would either
>> embedded "membank" or contain a pointer/index to the bank.
>>
> 
> Ok, so other than this fixing, others will be addressed in the next serie. And this
> part fixing will be introduced in a new follow-up patch serie after 4.17 release.
> 
> I'm in favor of introducing a new structure to contain shm-related data and let
> 'membank' contains a pointer to it, like
> ```
>   +struct shm_membank {
> +    char shm_id[MAX_SHM_ID_LENGTH];
> +    unsigned int nr_shm_borrowers;
> +}
> +
>   struct membank {
>       paddr_t start;
>       paddr_t size;
>       bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
> +    struct shm_membank *shm;
>   };
> ```
> Then every time we introduce a new feature here, following this strategy, 'membank' will
> at most grow 8 bytes for the reference.

Be aware that we are very early in Xen and therefore dynamically 
allocating memory is not possible. Therefore 'shm_membank' would most 
likely need to be static.

At which point, this could be an index.

> 
> I'm open to the discussion and will let it decide what it finally will be. ;)

My original idea was to have 'shm_membank' pointing to the 'membank' 
rather than the other way around. But I don't have a strong argument 
either way.

So I would need to see the resulting code. Anyway, that's for post-4.17.

Cheers,

-- 
Julien Grall