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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Thursday, September 1, 2022 11:40 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,
> 
> 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]>
> 

Ok, about the binding order change, do you prefer it in v7 or 4.17-post,
since it may also need a few code tweak.

> >
> >>> 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.
> 

Right, the heap may not be fully functional, understood.

> 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