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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 1 Sep 2022, at 16:40, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Penny,
> 
> On 29/08/2022 07:57, Penny Zheng wrote:
>>> -----Original Message-----
>>> From: Julien Grall <julien@xxxxxxx>
>>> Sent: Friday, August 26, 2022 9:17 PM
>>> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
>>> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
>>> <Volodymyr_Babchuk@xxxxxxxx>
>>> 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@xxxxxxx>
>>>> 
>>>> 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@xxxxxxx>
>>>> ---
>>>> 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.

Following ongoing gitlab discussion, it might be a good example of a case where 
creating a new gitlab ticket would make sense :-)

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall