[v4,2/8] xen/arm: allocate static shared memory to the default owner dom_io

Commit Message

Penny Zheng May 17, 2022, 9:05 a.m. UTC

From: Penny Zheng <[e-mail]>

This commit introduces process_shm to cope with static shared memory in
domain construction.

DOMID_IO will be the default owner of memory pre-shared among multiple domains
at boot time, when no explicit owner is specified.

This commit only considers allocating static shared memory to dom_io
when owner domain is not explicitly defined in device tree, all the left,
including the "borrower" code path, the "explicit owner" code path, shall
be introduced later in the following patches.

Signed-off-by: Penny Zheng <[e-mail]>
Reviewed-by: Stefano Stabellini <[e-mail]>
---
v4 change:
- no changes
---
v3 change:
- refine in-code comment
---
v2 change:
- instead of introducing a new system domain, reuse the existing dom_io
- make dom_io a non-auto-translated domain, then no need to create P2M
for it
- change dom_io definition and make it wider to support static shm here too
- introduce is_shm_allocated_to_domio to check whether static shm is
allocated yet, instead of using shm_mask bitmap
- add in-code comment
---
 xen/arch/arm/domain_build.c | 132 +++++++++++++++++++++++++++++++++++-
 xen/common/domain.c         |   5 ++
 2 files changed, 136 insertions(+), 1 deletion(-)

Comments

Jan Beulich May 17, 2022, 4:01 p.m. UTC | #1

On 17.05.2022 11:05, Penny Zheng wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -780,6 +780,11 @@ void __init setup_system_domains(void)
>       * This domain owns I/O pages that are within the range of the page_info
>       * array. Mappings occur at the priv of the caller.
>       * Quarantined PCI devices will be associated with this domain.
> +     *
> +     * DOMID_IO could also be used for mapping memory when no explicit
> +     * domain is specified.
> +     * For instance, DOMID_IO is the owner of memory pre-shared among
> +     * multiple domains at boot time, when no explicit owner is specified.
>       */
>      dom_io = domain_create(DOMID_IO, NULL, 0);
>      if ( IS_ERR(dom_io) )

I'm sorry: The comment change is definitely better now than it was, but it
is still written in a way requiring further knowledge to understand what
it talks about. Without further context, "when no explicit domain is
specified" only raises questions. I would have tried to make a suggestion,
but I can't really figure what it is that you want to get across here.

Jan

Penny Zheng May 18, 2022, 3:14 a.m. UTC | #2

Hi Jan 

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Wednesday, May 18, 2022 12:01 AM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Julien Grall <[e-mail]>; Bertrand Marquis
> <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Andrew Cooper
> <[e-mail]>; George Dunlap <[e-mail]>;
> Wei Liu <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v4 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
> 
> On 17.05.2022 11:05, Penny Zheng wrote:
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -780,6 +780,11 @@ void __init setup_system_domains(void)
> >       * This domain owns I/O pages that are within the range of the page_info
> >       * array. Mappings occur at the priv of the caller.
> >       * Quarantined PCI devices will be associated with this domain.
> > +     *
> > +     * DOMID_IO could also be used for mapping memory when no explicit
> > +     * domain is specified.
> > +     * For instance, DOMID_IO is the owner of memory pre-shared among
> > +     * multiple domains at boot time, when no explicit owner is specified.
> >       */
> >      dom_io = domain_create(DOMID_IO, NULL, 0);
> >      if ( IS_ERR(dom_io) )
> 
> I'm sorry: The comment change is definitely better now than it was, but it is
> still written in a way requiring further knowledge to understand what it talks
> about. Without further context, "when no explicit domain is specified" only
> raises questions. I would have tried to make a suggestion, but I can't really
> figure what it is that you want to get across here.

How about I only retain the "For instance, xxx" and make it more in details.
"
DOMID_IO is also the default owner of memory pre-shared among multiple domains at
boot time, when no explicit owner is specified with "owner" property in static shared
memory device node. See section docs/misc/arm/device-tree/booting.txt: Static Shared Memory
for more details. 
"

> 
> Jan

Jan Beulich May 18, 2022, 6:35 a.m. UTC | #3

On 18.05.2022 05:14, Penny Zheng wrote:
> Hi Jan 
> 
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: Wednesday, May 18, 2022 12:01 AM
>> To: Penny Zheng <[e-mail]>
>> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
>> <[e-mail]>; Julien Grall <[e-mail]>; Bertrand Marquis
>> <[e-mail]>; Volodymyr Babchuk
>> <[e-mail]>; Andrew Cooper
>> <[e-mail]>; George Dunlap <[e-mail]>;
>> Wei Liu <[e-mail]>; [e-mail]
>> Subject: Re: [PATCH v4 2/8] xen/arm: allocate static shared memory to the
>> default owner dom_io
>>
>> On 17.05.2022 11:05, Penny Zheng wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -780,6 +780,11 @@ void __init setup_system_domains(void)
>>>       * This domain owns I/O pages that are within the range of the page_info
>>>       * array. Mappings occur at the priv of the caller.
>>>       * Quarantined PCI devices will be associated with this domain.
>>> +     *
>>> +     * DOMID_IO could also be used for mapping memory when no explicit
>>> +     * domain is specified.
>>> +     * For instance, DOMID_IO is the owner of memory pre-shared among
>>> +     * multiple domains at boot time, when no explicit owner is specified.
>>>       */
>>>      dom_io = domain_create(DOMID_IO, NULL, 0);
>>>      if ( IS_ERR(dom_io) )
>>
>> I'm sorry: The comment change is definitely better now than it was, but it is
>> still written in a way requiring further knowledge to understand what it talks
>> about. Without further context, "when no explicit domain is specified" only
>> raises questions. I would have tried to make a suggestion, but I can't really
>> figure what it is that you want to get across here.
> 
> How about I only retain the "For instance, xxx" and make it more in details.
> "
> DOMID_IO is also the default owner of memory pre-shared among multiple domains at
> boot time, when no explicit owner is specified with "owner" property in static shared
> memory device node. See section docs/misc/arm/device-tree/booting.txt: Static Shared Memory
> for more details. 
> "

This reads quite a bit better. Yet I continue to be puzzled about the
apparent conflict of "pre-shared" and "no explicit owner": How can
memory be (pre-)shared when the owner isn't known? Shouldn't all
memory have an owner? Or alternatively if this sharing model doesn't
require ownership, shouldn't all shared memory be owned by DomIO? In
any event, to leave such details out of here, perhaps the comment
could consist of just the first part of what you wrote, ending at
where the first comma is?

Jan

Penny Zheng June 17, 2022, 9:39 a.m. UTC | #4

Hi Jan

Sorry about the late reply, got sidetracked a few weeks.

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: Wednesday, May 18, 2022 2:36 PM
> To: Penny Zheng <[e-mail]>
> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Julien Grall <[e-mail]>; Bertrand Marquis
> <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Andrew Cooper
> <[e-mail]>; George Dunlap <[e-mail]>;
> Wei Liu <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v4 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
> 
> On 18.05.2022 05:14, Penny Zheng wrote:
> > Hi Jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >> Sent: Wednesday, May 18, 2022 12:01 AM
> >> To: Penny Zheng <[e-mail]>
> >> Cc: Wei Chen <[e-mail]>; Stefano Stabellini
> >> <[e-mail]>; Julien Grall <[e-mail]>; Bertrand
> >> Marquis <[e-mail]>; Volodymyr Babchuk
> >> <[e-mail]>; Andrew Cooper
> >> <[e-mail]>; George Dunlap
> >> <[e-mail]>; Wei Liu <[e-mail]>;
> >> [e-mail]
> >> Subject: Re: [PATCH v4 2/8] xen/arm: allocate static shared memory to
> >> the default owner dom_io
> >>
> >> On 17.05.2022 11:05, Penny Zheng wrote:
> >>> --- a/xen/common/domain.c
> >>> +++ b/xen/common/domain.c
> >>> @@ -780,6 +780,11 @@ void __init setup_system_domains(void)
> >>>       * This domain owns I/O pages that are within the range of the
> page_info
> >>>       * array. Mappings occur at the priv of the caller.
> >>>       * Quarantined PCI devices will be associated with this domain.
> >>> +     *
> >>> +     * DOMID_IO could also be used for mapping memory when no explicit
> >>> +     * domain is specified.
> >>> +     * For instance, DOMID_IO is the owner of memory pre-shared among
> >>> +     * multiple domains at boot time, when no explicit owner is specified.
> >>>       */
> >>>      dom_io = domain_create(DOMID_IO, NULL, 0);
> >>>      if ( IS_ERR(dom_io) )
> >>
> >> I'm sorry: The comment change is definitely better now than it was,
> >> but it is still written in a way requiring further knowledge to
> >> understand what it talks about. Without further context, "when no
> >> explicit domain is specified" only raises questions. I would have
> >> tried to make a suggestion, but I can't really figure what it is that you want
> to get across here.
> >
> > How about I only retain the "For instance, xxx" and make it more in details.
> > "
> > DOMID_IO is also the default owner of memory pre-shared among multiple
> > domains at boot time, when no explicit owner is specified with "owner"
> > property in static shared memory device node. See section
> > docs/misc/arm/device-tree/booting.txt: Static Shared Memory for more
> details.
> > "
> 
> This reads quite a bit better. Yet I continue to be puzzled about the apparent
> conflict of "pre-shared" and "no explicit owner": How can memory be (pre-
> )shared when the owner isn't known? Shouldn't all memory have an owner?
> Or alternatively if this sharing model doesn't require ownership, shouldn't all
> shared memory be owned by DomIO? In any event, to leave such details out of
> here, perhaps the comment could consist of just the first part of what you
> wrote, ending at where the first comma is?
> 

We have a short discussion about the memory ownership on my design link(
https://lore.kernel.org/all/[e-mail]/T/)
, we have user cases for both scenario.

Ok, I will modify the comment and only keep
"
DOMID_IO is also the default owner of memory pre-shared among multiple
domains at boot time.
"
 
> Jan

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index aa777741bd..1746c15b7c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -522,6 +522,10 @@  static bool __init append_static_memory_to_bank(struct domain *d,
     return true;
 }
 
+/*
+ * If cell is NULL, pbase and psize should hold valid values.
+ * Otherwise, cell will be populated together with pbase and psize.
+ */
 static mfn_t __init acquire_static_memory_bank(struct domain *d,
                                                const __be32 **cell,
                                                u32 addr_cells, u32 size_cells,
@@ -530,7 +534,8 @@  static mfn_t __init acquire_static_memory_bank(struct domain *d,
     mfn_t smfn;
     int res;
 
-    device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
+    if ( cell )
+        device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
     ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
     if ( PFN_DOWN(*psize) > UINT_MAX )
     {
@@ -754,6 +759,125 @@  static void __init assign_static_memory_11(struct domain *d,
     panic("Failed to assign requested static memory for direct-map domain %pd.",
           d);
 }
+
+#ifdef CONFIG_STATIC_SHM
+/*
+ * This function checks whether the static shared memory region is
+ * already allocated to dom_io.
+ */
+static bool __init is_shm_allocated_to_domio(paddr_t pbase)
+{
+    struct page_info *page;
+
+    page = maddr_to_page(pbase);
+    ASSERT(page);
+
+    if ( page_get_owner(page) == NULL )
+        return false;
+
+    ASSERT(page_get_owner(page) == dom_io);
+    return true;
+}
+
+static mfn_t __init acquire_shared_memory_bank(struct domain *d,
+                                               u32 addr_cells, u32 size_cells,
+                                               paddr_t *pbase, paddr_t *psize)
+{
+    /*
+     * Pages of statically shared memory shall be included
+     * in domain_tot_pages().
+     */
+    d->max_pages += PFN_DOWN(*psize);
+
+    return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
+                                      pbase, psize);
+
+}
+
+/*
+ * Func allocate_shared_memory is supposed to be only called
+ * from the owner.
+ */
+static int __init allocate_shared_memory(struct domain *d,
+                                         u32 addr_cells, u32 size_cells,
+                                         paddr_t pbase, paddr_t psize)
+{
+    mfn_t smfn;
+
+    dprintk(XENLOG_INFO,
+            "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
+            pbase, pbase + psize);
+
+    smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
+                                      &psize);
+    if ( mfn_eq(smfn, INVALID_MFN) )
+        return -EINVAL;
+
+    /*
+     * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
+     * It sees RAM 1:1 and we do not need to create P2M mapping for it
+     */
+    ASSERT(d == dom_io);
+    return 0;
+}
+
+static int __init process_shm(struct domain *d,
+                              const struct dt_device_node *node)
+{
+    struct dt_device_node *shm_node;
+    int ret = 0;
+    const struct dt_property *prop;
+    const __be32 *cells;
+    u32 shm_id;
+    u32 addr_cells, size_cells;
+    paddr_t gbase, pbase, psize;
+
+    dt_for_each_child_node(node, shm_node)
+    {
+        if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
+            continue;
+
+        if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
+        {
+            printk("Shared memory node does not provide \"xen,shm-id\" property.\n");
+            return -ENOENT;
+        }
+
+        addr_cells = dt_n_addr_cells(shm_node);
+        size_cells = dt_n_size_cells(shm_node);
+        prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
+        if ( !prop )
+        {
+            printk("Shared memory node does not provide \"xen,shared-mem\" property.\n");
+            return -ENOENT;
+        }
+        cells = (const __be32 *)prop->value;
+        /* xen,shared-mem = <pbase, psize, gbase>; */
+        device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
+        ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
+        gbase = dt_read_number(cells, addr_cells);
+
+        /* TODO: Consider owner domain is not the default dom_io. */
+        /*
+         * Per static shared memory region could be shared between multiple
+         * domains.
+         * In case re-allocating the same shared memory region, we check
+         * if it is already allocated to the default owner dom_io before
+         * the actual allocation.
+         */
+        if ( !is_shm_allocated_to_domio(pbase) )
+        {
+            /* Allocate statically shared pages to the default owner dom_io. */
+            ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
+                                         pbase, psize);
+            if ( ret )
+                return ret;
+        }
+    }
+
+    return 0;
+}
+#endif /* CONFIG_STATIC_SHM */
 #else
 static void __init allocate_static_memory(struct domain *d,
                                           struct kernel_info *kinfo,
@@ -3152,6 +3276,12 @@  static int __init construct_domU(struct domain *d,
     else
         assign_static_memory_11(d, &kinfo, node);
 
+#ifdef CONFIG_STATIC_SHM
+    rc = process_shm(d, node);
+    if ( rc < 0 )
+        return rc;
+#endif
+
     /*
      * Base address and irq number are needed when creating vpl011 device
      * tree node in prepare_dtb_domU, so initialization on related variables
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7570eae91a..833ace7641 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -780,6 +780,11 @@  void __init setup_system_domains(void)
      * This domain owns I/O pages that are within the range of the page_info
      * array. Mappings occur at the priv of the caller.
      * Quarantined PCI devices will be associated with this domain.
+     *
+     * DOMID_IO could also be used for mapping memory when no explicit
+     * domain is specified.
+     * For instance, DOMID_IO is the owner of memory pre-shared among
+     * multiple domains at boot time, when no explicit owner is specified.
      */
     dom_io = domain_create(DOMID_IO, NULL, 0);
     if ( IS_ERR(dom_io) )