RE: [PATCH v5 1/8] xen/arm: introduce static shared memory

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

Before submitting the v6 patch series, I would like to double confirm that ...

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Wednesday, June 29, 2022 6:18 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
> 
> 
> 
> On 29/06/2022 06:38, Penny Zheng wrote:
> > Hi Julien
> 
> Hi Penny,
> 
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xxxxxxx>
> >> Sent: Saturday, June 25, 2022 1:55 AM
> >> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> >> <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> >> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@xxxxxxxx>
> >> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
> >>
> >> Hi Penny,
> >>
> >> On 20/06/2022 06:11, Penny Zheng wrote:
> >>> From: Penny Zheng <penny.zheng@xxxxxxx>
> >>>
> >>> This patch serie introduces a new feature: setting up static
> >>
> >> Typo: s/serie/series/
> >>
> >>> 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>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >>> ---
> >>> 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 | 120
> >> ++++++++++++++++++++++++++
> >>>    xen/arch/arm/Kconfig                  |   6 ++
> >>>    xen/arch/arm/bootfdt.c                |  68 +++++++++++++++
> >>>    xen/arch/arm/include/asm/setup.h      |   3 +
> >>>    4 files changed, 197 insertions(+)
> >>>
> >>> diff --git a/docs/misc/arm/device-tree/booting.txt
> >>> b/docs/misc/arm/device-tree/booting.txt
> >>> index 98253414b8..6467bc5a28 100644
> >>> --- a/docs/misc/arm/device-tree/booting.txt
> >>> +++ b/docs/misc/arm/device-tree/booting.txt
> >>> @@ -378,3 +378,123 @@ 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 8-bit integer that represents the unique identifier of the
> >>> + shared
> >> memory
> >>> +    region. The maximum identifier shall be "xen,shm-id = <0xff>".
> >>> +
> >>> +- 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. 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.
> >>
> >> Sorry for jump in the discussion late. But as this is going to be a
> >> stable ABI, I would to make sure the interface is going to be easily
> extendable.
> >>
> >> AFAIU, with your proposal the host physical address is mandatory. I
> >> would expect that some user may want to share memory but don't care
> >> about the exact location in memory. So I think it would be good to
> >> make it optional in the binding.
> >>
> >> I think this wants to be done now because it would be difficult to
> >> change the binding afterwards (the host physical address is the first set 
> >> of
> cells).
> >>
> >> The Xen doesn't need to handle the optional case.
> >>

... what you suggested here is that during "xen,shared-mem" device tree property
parsing process, if we find that user doesn't provide physical address, we will 
output
an error, saying that it is not supported at the moment, right?

> >
> > Sure, I'll make "the host physical address" optional here, and right
> > now, with no actual code implementation. I'll make up it later in free
> > time~
> >
> > The user case you mentioned here is that we let xen to allocate an
> > arbitrary static shared memory region, so size and guest physical address
> are still mandatory, right?
> 
> That's correct.
> 
> Cheers,
> 
> --
> Julien Grall