* [PATCH v6 0/9] static shared memory on dom0less system
@ 2022-07-21 13:21 Penny Zheng
2022-07-21 13:21 ` [PATCH v6 1/9] xen/arm: introduce static shared memory Penny Zheng
` (8 more replies)
0 siblings, 9 replies; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
In safety-critical environment, it is not considered safe to
dynamically change important configurations at runtime. Everything
should be statically defined and statically verified.
In this case, if the system configuration knows a priori that there are
only 2 VMs and they need to communicate over shared memory, it is safer
to pre-configure the shared memory at build time rather than let the VMs
attempt to share memory at runtime. And it is faster too.
Furthermore, on dom0less system, the legacy way to build up communication
channels between domains, like grant table, are normally absent there.
So this patch serie introduces a set of static shared memory device tree nodes
to allow users to statically set up shared memory on dom0less system, enabling
domains to do shm-based communication.
The only way to trigger this static shared memory configuration should
be via device tree, which is at the same level as the XSM rules.
It was inspired by the patch serie of ["xl/libxl-based shared mem](
https://marc.info/?l=xen-devel&m=154404821731186ory").
Looking into related [design link](
https://lore.kernel.org/all/a50d9fde-1d06-7cda-2779-9eea9e1c0134@xen.org/T/)
for more details.
Penny Zheng (9):
xen/arm: introduce static shared memory
xen/arm: allocate static shared memory to the default owner dom_io
xen/arm: allocate static shared memory to a specific owner domain
xen/arm: introduce put_page_nr and get_page_nr
xen/arm: Add additional reference to owner domain when the owner is
allocated
xen/arm: set up shared memory foreign mapping for borrower domain
xen/arm: create shared memory nodes in guest device tree
xen/arm: enable statically shared memory on Dom0
xen: Add static memory sharing in SUPPORT.md
SUPPORT.md | 6 +
docs/misc/arm/device-tree/booting.txt | 124 ++++++++
xen/arch/arm/Kconfig | 6 +
xen/arch/arm/bootfdt.c | 157 ++++++++++
xen/arch/arm/domain_build.c | 402 ++++++++++++++++++++++++++
xen/arch/arm/include/asm/kernel.h | 1 +
xen/arch/arm/include/asm/mm.h | 4 +
xen/arch/arm/include/asm/setup.h | 9 +
xen/arch/arm/mm.c | 42 ++-
xen/common/domain.c | 3 +
10 files changed, 745 insertions(+), 9 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-08-26 13:17 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 2/9] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
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] >
+
+ 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.
+
+
+- role (Optional)
+
+ A string property specifying the ownership of a shared memory region,
+ the value must be one of the following: "owner", or "borrower"
+ A shared memory region could be explicitly backed by one domain, which is
+ called "owner domain", and all the other domains who are also sharing
+ this region are called "borrower domain".
+ If not specified, the default value is "borrower" and owner is
+ DOMID_IO, a system domain.
+
+As an example:
+
+chosen {
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ xen,xen-bootargs = "console=dtuart dtuart=serial0 bootscrub=0";
+
+ ......
+
+ /* this is for Dom0 */
+ dom0-shared-mem@10000000 {
+ compatible = "xen,domain-shared-memory-v1";
+ role = "owner";
+ xen,shm-id = "my-shared-mem-0";
+ xen,shared-mem = <0x10000000 0x10000000 0x10000000>;
+ }
+
+ domU1 {
+ compatible = "xen,domain";
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ memory = <0 131072>;
+ cpus = <2>;
+ vpl011;
+
+ /*
+ * shared memory region identified as 0x0(xen,shm-id = <0x0>)
+ * is shared between Dom0 and DomU1.
+ */
+ domU1-shared-mem@10000000 {
+ compatible = "xen,domain-shared-memory-v1";
+ role = "borrower";
+ xen,shm-id = "my-shared-mem-0";
+ xen,shared-mem = <0x10000000 0x10000000 0x50000000>;
+ }
+
+ /*
+ * shared memory region identified as 0x1(xen,shm-id = <0x1>)
+ * is shared between DomU1 and DomU2.
+ */
+ domU1-shared-mem@50000000 {
+ compatible = "xen,domain-shared-memory-v1";
+ xen,shm-id = "my-shared-mem-1";
+ xen,shared-mem = <0x50000000 0x20000000 0x60000000>;
+ }
+
+ ......
+
+ };
+
+ domU2 {
+ compatible = "xen,domain";
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ memory = <0 65536>;
+ cpus = <1>;
+
+ /*
+ * shared memory region identified as 0x1(xen,shm-id = <0x1>)
+ * is shared between domU1 and domU2.
+ */
+ domU2-shared-mem@50000000 {
+ compatible = "xen,domain-shared-memory-v1";
+ xen,shm-id = "my-shared-mem-1";
+ xen,shared-mem = <0x50000000 0x20000000 0x70000000>;
+ }
+
+ ......
+ };
+};
+
+This is an example with two static shared memory regions.
+
+For the static shared memory region identified as "my-shared-mem-0", host
+physical address starting at 0x10000000 of 256MB will be reserved to be
+shared between Dom0 and DomU1. It will get mapped at 0x10000000 in Dom0 guest
+physical address space, and at 0x50000000 in DomU1 guest physical address space.
+Dom0 is explicitly defined as the owner domain, and DomU1 is the borrower domain.
+
+For the static shared memory region identified as "my-shared-mem-1", host
+physical address starting at 0x50000000 of 512MB will be reserved to be
+shared between DomU1 and DomU2. It will get mapped at 0x60000000 in DomU1 guest
+physical address space, and at 0x70000000 in DomU2 guest physical address space.
+DomU1 and DomU2 are both the borrower domain, the owner domain is the default
+owner domain DOMID_IO.
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index be9eff0141..7321f47c0f 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -139,6 +139,12 @@ config TEE
source "arch/arm/tee/Kconfig"
+config STATIC_SHM
+ bool "Statically shared memory on a dom0less system" if UNSUPPORTED
+ depends on STATIC_MEMORY
+ help
+ This option enables statically shared memory on a dom0less system.
+
endmenu
menu "ARM errata workaround via the alternative framework"
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ec81a45de9..c62d8867ec 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -13,6 +13,7 @@
#include <xen/init.h>
#include <xen/efi.h>
#include <xen/device_tree.h>
+#include <xen/lib.h>
#include <xen/libfdt/libfdt.h>
#include <xen/sort.h>
#include <xsm/xsm.h>
@@ -361,6 +362,158 @@ static int __init process_domain_node(const void *fdt, int node,
size_cells, &bootinfo.reserved_mem, true);
}
+#ifdef CONFIG_STATIC_SHM
+static int __init process_shm_node(const void *fdt, int node,
+ u32 address_cells, u32 size_cells)
+{
+ const struct fdt_property *prop, *prop_id, *prop_role;
+ const __be32 *cell;
+ paddr_t paddr, size;
+ struct meminfo *mem = &bootinfo.reserved_mem;
+ unsigned int i;
+ int len;
+ bool owner = false;
+ const char *shm_id;
+
+ if ( address_cells < 1 || size_cells < 1 )
+ {
+ printk("fdt: invalid #address-cells or #size-cells for static shared memory node.\n");
+ return -EINVAL;
+ }
+
+ /*
+ * "xen,shm-id" property holds an arbitrary string with a strict limit
+ * on the number of characters, MAX_SHM_ID_LENGTH
+ */
+ prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
+ if ( !prop_id )
+ return -ENOENT;
+ shm_id = (const char *)prop_id->data;
+ if ( strnlen(shm_id, MAX_SHM_ID_LENGTH) == MAX_SHM_ID_LENGTH )
+ {
+ printk("fdt: invalid xen,shm-id %s, it must be limited to %d characters\n",
+ shm_id, MAX_SHM_ID_LENGTH);
+ return -EINVAL;
+ }
+
+ /*
+ * "role" property is optional and if it is defined explicitly,
+ * it must be either `owner` or `borrower`.
+ */
+ prop_role = fdt_get_property(fdt, node, "role", NULL);
+ if ( prop_role )
+ {
+ if ( !strcmp(prop_role->data, "owner") )
+ owner = true;
+ else if ( strcmp(prop_role->data, "borrower") )
+ {
+ printk("fdt: invalid `role` property for static shared memory node.\n");
+ return -EINVAL;
+ }
+ }
+
+ /*
+ * xen,shared-mem = <paddr, size, gaddr>;
+ * Memory region starting from physical address #paddr of #size shall
+ * be mapped to guest physical address #gaddr as static shared memory
+ * region.
+ */
+ prop = fdt_get_property(fdt, node, "xen,shared-mem", &len);
+ if ( !prop )
+ return -ENOENT;
+
+ if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
+ {
+ /* TODO: physical address is optional. */
+ if ( len == dt_cells_to_size(size_cells + address_cells) )
+ printk("fdt: host physical address must be chosen by users at the moment.\n");
+
+ printk("fdt: invalid `xen,shared-mem` property.\n");
+ return -EINVAL;
+ }
+
+ cell = (const __be32 *)prop->data;
+ device_tree_get_reg(&cell, address_cells, size_cells, &paddr, &size);
+
+ if ( !size )
+ {
+ printk("fdt: the size for static shared memory region can not be zero\n");
+ return -EINVAL;
+ }
+
+ for ( i = 0; i < mem->nr_banks; i++ )
+ {
+ /*
+ * Meet 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
+ * with an existing one
+ */
+ if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
+ {
+ if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
+ break;
+ else
+ {
+ printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
+ shm_id);
+ return -EINVAL;
+ }
+ }
+ else
+ {
+ paddr_t end = paddr + size;
+ paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
+
+ if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
+ {
+ if ( strncmp(shm_id, mem->bank[i].shm_id,
+ MAX_SHM_ID_LENGTH) != 0 )
+ continue;
+ else
+ {
+ printk("fdt: different shared memory region could not share the same shm ID %s\n",
+ shm_id);
+ return -EINVAL;
+ }
+ }
+ else
+ {
+ printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
+ mem->bank[i].start, bank_end);
+ return -EINVAL;
+ }
+ }
+ }
+
+ if ( i == mem->nr_banks )
+ {
+ if ( i < NR_MEM_BANKS )
+ {
+ /* Static shared memory shall be reserved from any other use. */
+ safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
+ mem->bank[mem->nr_banks].start = paddr;
+ mem->bank[mem->nr_banks].size = size;
+ mem->bank[mem->nr_banks].xen_domain = true;
+ mem->nr_banks++;
+ }
+ else
+ {
+ printk("Warning: Max number of supported memory regions reached.\n");
+ return -ENOSPC;
+ }
+ }
+ /*
+ * keep a count of the number of borrowers, which later may be used
+ * to calculate the reference count.
+ */
+ if ( !owner )
+ mem->bank[i].nr_shm_borrowers++;
+
+ return 0;
+}
+#endif
+
static int __init early_scan_node(const void *fdt,
int node, const char *name, int depth,
u32 address_cells, u32 size_cells,
@@ -386,6 +539,10 @@ static int __init early_scan_node(const void *fdt,
process_chosen_node(fdt, node, name, address_cells, size_cells);
else if ( depth == 2 && device_tree_node_compatible(fdt, node, "xen,domain") )
rc = process_domain_node(fdt, node, name, address_cells, size_cells);
+#ifdef CONFIG_STATIC_SHM
+ else if ( depth <= 3 && device_tree_node_compatible(fdt, node, "xen,domain-shared-memory-v1") )
+ rc = process_shm_node(fdt, node, address_cells, size_cells);
+#endif
if ( rc < 0 )
printk("fdt: node `%s': parsing failed\n", name);
diff --git 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
+
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
};
struct meminfo {
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 2/9] xen/arm: allocate static shared memory to the default owner dom_io
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
2022-07-21 13:21 ` [PATCH v6 1/9] xen/arm: introduce static shared memory Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-08-26 13:59 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 3/9] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
From: Penny Zheng <penny.zheng@arm.com>
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 <penny.zheng@arm.com>
---
v6 change:
- use if-array to check psize, pbase and gbase are suitably aligned and
valid
- use mfn_valid to check (pbase, pbase + psize)
- check d->max_pages will not overflow
- refine acquire_shared_memory_bank to not reuse acquire_static_memory_bank,
then input pbase and psize do not need to be used as a pointer.
- use if-array to check if page owner is dom_io
- in-code comment refinement
---
v5 change:
- refine in-code comment
---
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 | 155 ++++++++++++++++++++++++++++++++++++
xen/common/domain.c | 3 +
2 files changed, 158 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3fd1186b53..ff2aebaf28 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -759,6 +759,155 @@ 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);
+ if ( page_get_owner(page) == NULL )
+ return false;
+
+ if ( page_get_owner(page) != dom_io )
+ {
+ printk(XENLOG_ERR
+ "shm memory node has already been allocated to a specific owner %pd, Please check your configuration\n",
+ page_get_owner(page));
+ return false;
+ }
+
+ return true;
+}
+
+static mfn_t __init acquire_shared_memory_bank(struct domain *d,
+ paddr_t pbase, paddr_t psize)
+{
+ mfn_t smfn;
+ unsigned long nr_pfns;
+ int res;
+
+ /*
+ * Pages of statically shared memory shall be included
+ * into domain_tot_pages().
+ */
+ nr_pfns = PFN_DOWN(psize);
+ if ( (UINT_MAX - d->max_pages) < nr_pfns )
+ {
+ printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
+ d, nr_pfns);
+ return INVALID_MFN;
+ }
+ d->max_pages += nr_pfns;
+
+ smfn = maddr_to_mfn(pbase);
+ res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
+ if ( res )
+ {
+ printk(XENLOG_ERR
+ "%pd: failed to acquire static memory: %d.\n", d, res);
+ return INVALID_MFN;
+ }
+
+ return smfn;
+}
+
+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,
+ "%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
+ d, pbase, pbase + psize);
+
+ smfn = acquire_shared_memory_bank(d, 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;
+
+ dt_for_each_child_node(node, shm_node)
+ {
+ const struct dt_property *prop;
+ const __be32 *cells;
+ uint32_t addr_cells, size_cells;
+ paddr_t gbase, pbase, psize;
+ int ret = 0;
+ unsigned int i;
+
+ if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
+ continue;
+
+ /*
+ * xen,shared-mem = <pbase, psize, gbase>;
+ * TODO: pbase is optional.
+ */
+ 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);
+ ASSERT(prop);
+ cells = (const __be32 *)prop->value;
+ device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
+ gbase = dt_read_number(cells, addr_cells);
+ if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
+ {
+ printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
+ d, pbase, gbase);
+ return -EINVAL;
+ }
+ if ( !IS_ALIGNED(psize, PAGE_SIZE) )
+ {
+ printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n",
+ d, psize);
+ return -EINVAL;
+ }
+
+ for ( i = 0; i < PFN_DOWN(psize); i++ )
+ if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
+ {
+ printk("%pd: invalid physical address 0x%"PRI_mfn"\n",
+ d, mfn_x(mfn_add(maddr_to_mfn(pbase), i)));
+ return -EINVAL;
+ }
+
+ /* 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,
@@ -3236,6 +3385,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 618410e3b2..c8564113e9 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -780,6 +780,9 @@ 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 is also the default owner of memory pre-shared among multiple
+ * domains at boot time.
*/
dom_io = domain_create(DOMID_IO, NULL, 0);
if ( IS_ERR(dom_io) )
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 3/9] xen/arm: allocate static shared memory to a specific owner domain
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
2022-07-21 13:21 ` [PATCH v6 1/9] xen/arm: introduce static shared memory Penny Zheng
2022-07-21 13:21 ` [PATCH v6 2/9] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-07-21 13:21 ` [PATCH v6 4/9] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
` (5 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Penny Zheng
If owner property is defined, then owner domain of a static shared memory
region is not the default dom_io anymore, but a specific domain.
This commit implements allocating static shared memory to a specific domain
when owner property is defined.
Coding flow for dealing borrower domain will be introduced later in the
following commits.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v6 change:
- fix coding-style
- role_str and owner_dom_io shall be defined within the loop
---
v5 change:
- no change
---
v4 change:
- no changes
---
v3 change:
- simplify the code since o_gbase is not used if the domain is dom_io
---
v2 change:
- P2M mapping is restricted to normal domain
- in-code comment fix
---
xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index ff2aebaf28..a7e95c34a7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -818,9 +818,11 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
static int __init allocate_shared_memory(struct domain *d,
u32 addr_cells, u32 size_cells,
- paddr_t pbase, paddr_t psize)
+ paddr_t pbase, paddr_t psize,
+ paddr_t gbase)
{
mfn_t smfn;
+ int ret = 0;
dprintk(XENLOG_INFO,
"%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
@@ -834,8 +836,18 @@ static int __init allocate_shared_memory(struct domain *d,
* 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;
+ if ( d != dom_io )
+ {
+ ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
+ PFN_DOWN(psize));
+ if ( ret )
+ {
+ printk(XENLOG_ERR "Failed to map shared memory to %pd.\n", d);
+ return ret;
+ }
+ }
+
+ return ret;
}
static int __init process_shm(struct domain *d,
@@ -851,6 +863,8 @@ static int __init process_shm(struct domain *d,
paddr_t gbase, pbase, psize;
int ret = 0;
unsigned int i;
+ const char *role_str;
+ bool owner_dom_io = true;
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
@@ -887,19 +901,27 @@ static int __init process_shm(struct domain *d,
return -EINVAL;
}
- /* TODO: Consider owner domain is not the default dom_io. */
+ /*
+ * "role" property is optional and if it is defined explicitly,
+ * then the owner domain is not the default "dom_io" domain.
+ */
+ if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
+ owner_dom_io = false;
+
/*
* 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.
+ * So when owner domain is the default dom_io, 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) )
+ if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
+ (!owner_dom_io && strcmp(role_str, "owner") == 0) )
{
- /* Allocate statically shared pages to the default owner dom_io. */
- ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
- pbase, psize);
+ /* Allocate statically shared pages to the owner domain. */
+ ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
+ addr_cells, size_cells,
+ pbase, psize, gbase);
if ( ret )
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 4/9] xen/arm: introduce put_page_nr and get_page_nr
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
` (2 preceding siblings ...)
2022-07-21 13:21 ` [PATCH v6 3/9] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-07-21 13:21 ` [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
` (4 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Penny Zheng, Julien Grall
Later, we need to add the right amount of references, which should be
the number of borrower domains, to the owner domain. Since we only have
get_page() to increment the page reference by 1, a loop is needed per
page, which is inefficient and time-consuming.
To save the loop time, this commit introduces a set of new helpers
put_page_nr() and get_page_nr() to increment/drop the page reference by nr.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v6 change:
- no change
---
v5 change:
- no change
---
v4 changes:
- fix the assert about checking overflow to make sure that the right equation
return is at least equal to nr
- simplify the assert about checking the underflow
---
v3 changes:
- check overflow with "n"
- remove spurious change
- bring back the check that we enter the loop only when count_info is
greater than 0
---
v2 change:
- new commit
---
xen/arch/arm/include/asm/mm.h | 4 ++++
xen/arch/arm/mm.c | 42 +++++++++++++++++++++++++++--------
2 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index c4bc3cd1e5..f1ab0fd030 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -343,6 +343,10 @@ void free_init_memory(void);
int guest_physmap_mark_populate_on_demand(struct domain *d, unsigned long gfn,
unsigned int order);
+extern bool get_page_nr(struct page_info *page, const struct domain *domain,
+ unsigned long nr);
+extern void put_page_nr(struct page_info *page, unsigned long nr);
+
extern void put_page_type(struct page_info *page);
static inline void put_page_and_type(struct page_info *page)
{
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 009b8cd9ef..d654980706 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1587,21 +1587,29 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
return 0;
}
-struct domain *page_get_owner_and_reference(struct page_info *page)
+static struct domain *page_get_owner_and_nr_reference(struct page_info *page,
+ unsigned long nr)
{
unsigned long x, y = page->count_info;
struct domain *owner;
+ /* Restrict nr to avoid "double" overflow */
+ if ( nr >= PGC_count_mask )
+ {
+ ASSERT_UNREACHABLE();
+ return NULL;
+ }
+
do {
x = y;
/*
* Count == 0: Page is not allocated, so we cannot take a reference.
* Count == -1: Reference count would wrap, which is invalid.
*/
- if ( unlikely(((x + 1) & PGC_count_mask) <= 1) )
+ if ( unlikely(((x + nr) & PGC_count_mask) <= nr) )
return NULL;
}
- while ( (y = cmpxchg(&page->count_info, x, x + 1)) != x );
+ while ( (y = cmpxchg(&page->count_info, x, x + nr)) != x );
owner = page_get_owner(page);
ASSERT(owner);
@@ -1609,14 +1617,19 @@ struct domain *page_get_owner_and_reference(struct page_info *page)
return owner;
}
-void put_page(struct page_info *page)
+struct domain *page_get_owner_and_reference(struct page_info *page)
+{
+ return page_get_owner_and_nr_reference(page, 1);
+}
+
+void put_page_nr(struct page_info *page, unsigned long nr)
{
unsigned long nx, x, y = page->count_info;
do {
- ASSERT((y & PGC_count_mask) != 0);
+ ASSERT((y & PGC_count_mask) >= nr);
x = y;
- nx = x - 1;
+ nx = x - nr;
}
while ( unlikely((y = cmpxchg(&page->count_info, x, nx)) != x) );
@@ -1626,19 +1639,30 @@ void put_page(struct page_info *page)
}
}
-bool get_page(struct page_info *page, const struct domain *domain)
+void put_page(struct page_info *page)
+{
+ put_page_nr(page, 1);
+}
+
+bool get_page_nr(struct page_info *page, const struct domain *domain,
+ unsigned long nr)
{
- const struct domain *owner = page_get_owner_and_reference(page);
+ const struct domain *owner = page_get_owner_and_nr_reference(page, nr);
if ( likely(owner == domain) )
return true;
if ( owner != NULL )
- put_page(page);
+ put_page_nr(page, nr);
return false;
}
+bool get_page(struct page_info *page, const struct domain *domain)
+{
+ return get_page_nr(page, domain, 1);
+}
+
/* Common code requires get_page_type and put_page_type.
* We don't care about typecounts so we just do the minimum to make it
* happy. */
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
` (3 preceding siblings ...)
2022-07-21 13:21 ` [PATCH v6 4/9] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-09-01 17:12 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 6/9] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
` (3 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Penny Zheng
Borrower domain will fail to get a page ref using the owner domain
during allocation, when the owner is created after borrower.
So here, we decide to get and add the right amount of reference, which
is the number of borrowers, when the owner is allocated.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v6 change:
- adapt to the change of "nr_shm_borrowers"
- add in-code comment to explain if the borrower is created first, we intend to
add pages in the P2M without reference.
---
v5 change:
- no change
---
v4 changes:
- no change
---
v3 change:
- printk rather than dprintk since it is a serious error
---
v2 change:
- new commit
---
xen/arch/arm/domain_build.c | 60 +++++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a7e95c34a7..e891e800a7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -761,6 +761,30 @@ static void __init assign_static_memory_11(struct domain *d,
}
#ifdef CONFIG_STATIC_SHM
+static int __init acquire_nr_borrower_domain(struct domain *d,
+ paddr_t pbase, paddr_t psize,
+ unsigned long *nr_borrowers)
+{
+ unsigned long bank;
+
+ /* Iterate reserved memory to find requested shm bank. */
+ for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
+ {
+ paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
+ paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
+
+ if ( (pbase == bank_start) && (psize == bank_size) )
+ break;
+ }
+
+ if ( bank == bootinfo.reserved_mem.nr_banks )
+ return -ENOENT;
+
+ *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
+
+ return 0;
+}
+
/*
* This function checks whether the static shared memory region is
* already allocated to dom_io.
@@ -823,6 +847,8 @@ static int __init allocate_shared_memory(struct domain *d,
{
mfn_t smfn;
int ret = 0;
+ unsigned long nr_pages, nr_borrowers, i;
+ struct page_info *page;
dprintk(XENLOG_INFO,
"%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
@@ -836,6 +862,7 @@ static int __init allocate_shared_memory(struct domain *d,
* 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
*/
+ nr_pages = PFN_DOWN(psize);
if ( d != dom_io )
{
ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
@@ -847,6 +874,39 @@ static int __init allocate_shared_memory(struct domain *d,
}
}
+ /*
+ * Get the right amount of references per page, which is the number of
+ * borrower domains.
+ */
+ ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
+ if ( ret )
+ return ret;
+
+ /*
+ * Instead of letting borrower domain get a page ref, we add as many
+ * additional reference as the number of borrowers when the owner
+ * is allocated, since there is a chance that owner is created
+ * after borrower.
+ * So if the borrower is created first, it will cause adding pages
+ * in the P2M without reference.
+ */
+ page = mfn_to_page(smfn);
+ for ( i = 0; i < nr_pages; i++ )
+ {
+ if ( !get_page_nr(page + i, d, nr_borrowers) )
+ {
+ printk(XENLOG_ERR
+ "Failed to add %lu references to page %"PRI_mfn".\n",
+ nr_borrowers, mfn_x(smfn) + i);
+ goto fail;
+ }
+ }
+
+ return 0;
+
+ fail:
+ while ( --i >= 0 )
+ put_page_nr(page + i, nr_borrowers);
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 6/9] xen/arm: set up shared memory foreign mapping for borrower domain
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
` (4 preceding siblings ...)
2022-07-21 13:21 ` [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-07-21 13:21 ` [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree Penny Zheng
` (2 subsequent siblings)
8 siblings, 0 replies; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Penny Zheng
This commit sets up shared memory foreign mapping for borrower domain.
If owner domain is the default dom_io, all shared domain are treated as
borrower domain.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v6 change:
- no change
---
v5 change:
- no change
---
v4 changes:
- no change
---
v3 change:
- use map_regions_p2mt instead
---
v2 change:
- remove guest_physmap_add_shm, since for borrower domain, we only
do P2M foreign memory mapping now.
---
xen/arch/arm/domain_build.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e891e800a7..f76fbbc644 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -985,6 +985,15 @@ static int __init process_shm(struct domain *d,
if ( ret )
return ret;
}
+
+ if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
+ {
+ /* Set up P2M foreign mapping for borrower domain. */
+ ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
+ _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
+ if ( ret )
+ return ret;
+ }
}
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
` (5 preceding siblings ...)
2022-07-21 13:21 ` [PATCH v6 6/9] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-08-26 13:13 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 8/9] xen/arm: enable statically shared memory on Dom0 Penny Zheng
2022-07-21 13:21 ` [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md Penny Zheng
8 siblings, 1 reply; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Penny Zheng
We expose the shared memory to the domU using the "xen,shared-memory-v1"
reserved-memory binding. See
Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt
in Linux for the corresponding device tree binding.
To save the cost of re-parsing shared memory device tree configuration when
creating shared memory nodes in guest device tree, this commit adds new field
"shm_mem" to store shm-info per domain.
For each shared memory region, a range is exposed under
the /reserved-memory node as a child node. Each range sub-node is
named xen-shmem@<address> and has the following properties:
- compatible:
compatible = "xen,shared-memory-v1"
- reg:
the base guest physical address and size of the shared memory region
- xen,id:
a string that identifies the shared memory region.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v6 change:
- change "struct meminfo *mem" to "const struct meminfo *mem"
- change "unsigned long i" to "unsigned int i" to match the type of nr_banks.
- accroding to the Linux binding, "xen,id" is meant to be a string, not
an integer
---
v5 change:
- no change
---
v4 change:
- no change
---
v3 change:
- move field "shm_mem" to kernel_info
---
v2 change:
- using xzalloc
- shm_id should be uint8_t
- make reg a local variable
- add #address-cells and #size-cells properties
- fix alignment
---
xen/arch/arm/domain_build.c | 146 +++++++++++++++++++++++++++++-
xen/arch/arm/include/asm/kernel.h | 1 +
2 files changed, 145 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f76fbbc644..8d2c465c99 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -910,7 +910,22 @@ static int __init allocate_shared_memory(struct domain *d,
return ret;
}
-static int __init process_shm(struct domain *d,
+static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
+ paddr_t start, paddr_t size,
+ const char *shm_id)
+{
+ if ( (kinfo->shm_mem.nr_banks + 1) > NR_MEM_BANKS )
+ return -ENOMEM;
+
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
+ safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
+ kinfo->shm_mem.nr_banks++;
+
+ return 0;
+}
+
+static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
struct dt_device_node *shm_node;
@@ -924,6 +939,7 @@ static int __init process_shm(struct domain *d,
int ret = 0;
unsigned int i;
const char *role_str;
+ const char *shm_id;
bool owner_dom_io = true;
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
@@ -968,6 +984,9 @@ static int __init process_shm(struct domain *d,
if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
owner_dom_io = false;
+ dt_property_read_string(shm_node, "xen,shm-id", &shm_id);
+ ASSERT((strlen(shm_id) > 0) && (strlen(shm_id) < MAX_SHM_ID_LENGTH));
+
/*
* Per static shared memory region could be shared between multiple
* domains.
@@ -994,6 +1013,14 @@ static int __init process_shm(struct domain *d,
if ( ret )
return ret;
}
+
+ /*
+ * Record static shared memory region info for later setting
+ * up shm-node in guest device tree.
+ */
+ ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id);
+ if ( ret )
+ return ret;
}
return 0;
@@ -1324,6 +1351,116 @@ static int __init make_memory_node(const struct domain *d,
return res;
}
+#ifdef CONFIG_STATIC_SHM
+static int __init make_shm_memory_node(const struct domain *d,
+ void *fdt,
+ int addrcells, int sizecells,
+ const struct meminfo *mem)
+{
+ unsigned int i = 0;
+ int res = 0;
+
+ if ( mem->nr_banks == 0 )
+ return -ENOENT;
+
+ /*
+ * For each shared memory region, a range is exposed under
+ * the /reserved-memory node as a child node. Each range sub-node is
+ * named xen-shmem@<address>.
+ */
+ dt_dprintk("Create xen-shmem node\n");
+
+ for ( ; i < mem->nr_banks; i++ )
+ {
+ uint64_t start = mem->bank[i].start;
+ uint64_t size = mem->bank[i].size;
+ /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
+ char buf[27];
+ const char compat[] = "xen,shared-memory-v1";
+ __be32 reg[addrcells + sizecells];
+ __be32 *cells;
+ unsigned int len = (addrcells + sizecells) * sizeof(__be32);
+
+ snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
+ res = fdt_begin_node(fdt, buf);
+ if ( res )
+ return res;
+
+ res = fdt_property(fdt, "compatible", compat, sizeof(compat));
+ if ( res )
+ return res;
+
+ cells = reg;
+ dt_child_set_range(&cells, addrcells, sizecells, start, size);
+
+ res = fdt_property(fdt, "reg", reg, len);
+ if ( res )
+ return res;
+
+ dt_dprintk("Shared memory bank %u: %#"PRIx64"->%#"PRIx64"\n",
+ i, start, start + size);
+
+ res = fdt_property_string(fdt, "xen,id", mem->bank[i].shm_id);
+ if ( res )
+ return res;
+
+ res = fdt_end_node(fdt);
+ if ( res )
+ return res;
+ }
+
+ return res;
+}
+#else
+static int __init make_shm_memory_node(const struct domain *d,
+ void *fdt,
+ int addrcells, int sizecells,
+ struct meminfo *mem)
+{
+ ASSERT_UNREACHABLE();
+}
+#endif
+
+static int __init make_resv_memory_node(const struct domain *d,
+ void *fdt,
+ int addrcells, int sizecells,
+ struct meminfo *mem)
+{
+ int res = 0;
+ /* Placeholder for reserved-memory\0 */
+ char resvbuf[16] = "reserved-memory";
+
+ if ( mem->nr_banks == 0 )
+ /* No shared memory provided. */
+ return 0;
+
+ dt_dprintk("Create reserved-memory node\n");
+
+ res = fdt_begin_node(fdt, resvbuf);
+ if ( res )
+ return res;
+
+ res = fdt_property(fdt, "ranges", NULL, 0);
+ if ( res )
+ return res;
+
+ res = fdt_property_cell(fdt, "#address-cells", addrcells);
+ if ( res )
+ return res;
+
+ res = fdt_property_cell(fdt, "#size-cells", sizecells);
+ if ( res )
+ return res;
+
+ res = make_shm_memory_node(d, fdt, addrcells, sizecells, mem);
+ if ( res )
+ return res;
+
+ res = fdt_end_node(fdt);
+
+ return res;
+}
+
static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
{
struct meminfo *ext_regions = data;
@@ -3101,6 +3238,11 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
if ( ret )
goto err;
+ ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ &kinfo->shm_mem);
+ if ( ret )
+ goto err;
+
/*
* domain_handle_dtb_bootmodule has to be called before the rest of
* the device tree is generated because it depends on the value of
@@ -3477,7 +3619,7 @@ static int __init construct_domU(struct domain *d,
assign_static_memory_11(d, &kinfo, node);
#ifdef CONFIG_STATIC_SHM
- rc = process_shm(d, node);
+ rc = process_shm(d, &kinfo, node);
if ( rc < 0 )
return rc;
#endif
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index c4dc039b54..2cc506b100 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -19,6 +19,7 @@ struct kernel_info {
void *fdt; /* flat device tree */
paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
struct meminfo mem;
+ struct meminfo shm_mem;
/* kernel entry point */
paddr_t entry;
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 8/9] xen/arm: enable statically shared memory on Dom0
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
` (6 preceding siblings ...)
2022-07-21 13:21 ` [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-07-21 13:21 ` [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md Penny Zheng
8 siblings, 0 replies; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
To add statically shared memory nodes in Dom0, user shall put according
static shared memory configuration under /chosen node.
This commit adds shm-processing function process_shm in construct_dom0
to enable statically shared memory on Dom0.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v6 change:
- no change
---
v5 change:
- no change
---
v4 change:
- no change
---
v3 change:
- no change
---
v2 change:
- no change
---
xen/arch/arm/domain_build.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8d2c465c99..7c592d63ab 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2684,6 +2684,11 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
if ( res )
return res;
}
+
+ res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ &kinfo->shm_mem);
+ if ( res )
+ return res;
}
res = fdt_end_node(kinfo->fdt);
@@ -3756,6 +3761,9 @@ static int __init construct_dom0(struct domain *d)
{
struct kernel_info kinfo = {};
int rc;
+#ifdef CONFIG_STATIC_SHM
+ const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
+#endif
/* Sanity! */
BUG_ON(d->domain_id != 0);
@@ -3790,6 +3798,12 @@ static int __init construct_dom0(struct domain *d)
allocate_memory_11(d, &kinfo);
find_gnttab_region(d, &kinfo);
+#ifdef CONFIG_STATIC_SHM
+ rc = process_shm(d, &kinfo, chosen);
+ if ( rc < 0 )
+ return rc;
+#endif
+
/* Map extra GIC MMIO, irqs and other hw stuffs to dom0. */
rc = gic_map_hwdom_extra_mappings(d);
if ( rc < 0 )
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
` (7 preceding siblings ...)
2022-07-21 13:21 ` [PATCH v6 8/9] xen/arm: enable statically shared memory on Dom0 Penny Zheng
@ 2022-07-21 13:21 ` Penny Zheng
2022-08-26 7:21 ` Michal Orzel
8 siblings, 1 reply; 23+ messages in thread
From: Penny Zheng @ 2022-07-21 13:21 UTC (permalink / raw)
To: xen-devel
Cc: Penny Zheng, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Penny Zheng
on ARM, static memory sharing is tech preview, which shall be documented
in SUPPORT.md
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v6 change:
- new commit
---
SUPPORT.md | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/SUPPORT.md b/SUPPORT.md
index 8e040d1c1e..3dfe6d2fbe 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -299,6 +299,12 @@ Allow sharing of identical pages between guests
Status, x86 HVM: Experimental
+### Static Memory Sharing
+
+Allow memory pre-shared among multiple domains at boot time through device tree configuration
+
+ Status, ARM: Tech Preview
+
### Memory Paging
Allow pages belonging to guests to be paged to disk
--
2.25.1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md
2022-07-21 13:21 ` [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md Penny Zheng
@ 2022-08-26 7:21 ` Michal Orzel
2022-08-29 6:20 ` Penny Zheng
0 siblings, 1 reply; 23+ messages in thread
From: Michal Orzel @ 2022-08-26 7:21 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
Hi Penny,
On 21/07/2022 15:21, Penny Zheng wrote:
> on ARM, static memory sharing is tech preview, which shall be documented
NIT: missing 'a' before 'tech preview'.
> in SUPPORT.md
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v6 change:
> - new commit
> ---
> SUPPORT.md | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/SUPPORT.md b/SUPPORT.md
> index 8e040d1c1e..3dfe6d2fbe 100644
> --- a/SUPPORT.md
> +++ b/SUPPORT.md
> @@ -299,6 +299,12 @@ Allow sharing of identical pages between guests
>
> Status, x86 HVM: Experimental
>
> +### Static Memory Sharing
> +
> +Allow memory pre-shared among multiple domains at boot time through device tree configuration
This does not explicitly state that this feature is for dom0less only.
How about taking what you wrote in booting.txt:
"Allow to statically set up shared memory on dom0less system, enabling domains to do shm-based communication".
> +
> + Status, ARM: Tech Preview
> +
> ### Memory Paging
>
> Allow pages belonging to guests to be paged to disk
~Michal
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree
2022-07-21 13:21 ` [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree Penny Zheng
@ 2022-08-26 13:13 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2022-08-26 13:13 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 21/07/2022 14:21, Penny Zheng wrote:
> +#ifdef CONFIG_STATIC_SHM
> +static int __init make_shm_memory_node(const struct domain *d,
> + void *fdt,
> + int addrcells, int sizecells,
> + const struct meminfo *mem)
> +{
> + unsigned int i = 0;
> + int res = 0;
> +
> + if ( mem->nr_banks == 0 )
> + return -ENOENT;
> +
> + /*
> + * For each shared memory region, a range is exposed under
> + * the /reserved-memory node as a child node. Each range sub-node is
> + * named xen-shmem@<address>.
> + */
> + dt_dprintk("Create xen-shmem node\n");
> +
> + for ( ; i < mem->nr_banks; i++ )
> + {
> + uint64_t start = mem->bank[i].start;
> + uint64_t size = mem->bank[i].size;
> + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> + char buf[27];
> + const char compat[] = "xen,shared-memory-v1";
> + __be32 reg[addrcells + sizecells];
This doesn't build for me:
arch/arm/domain_build.c: In function ‘make_shm_memory_node’:
arch/arm/domain_build.c:1380:9: error: ISO C90 forbids variable length
array ‘reg’ [-Werror=vla]
__be32 reg[addrcells + sizecells];
^~~~~~
I haven't yet review the patch. But I think we would want to dynamically
allocate 'reg' like we do in other places unless it is possible to know
the maximum size of the array.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-07-21 13:21 ` [PATCH v6 1/9] xen/arm: introduce static shared memory Penny Zheng
@ 2022-08-26 13:17 ` Julien Grall
2022-08-29 6:57 ` Penny Zheng
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2022-08-26 13:17 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
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.
> +
> +
> +- role (Optional)
> +
> + A string property specifying the ownership of a shared memory region,
> + the value must be one of the following: "owner", or "borrower"
> + A shared memory region could be explicitly backed by one domain, which is
> + called "owner domain", and all the other domains who are also sharing
> + this region are called "borrower domain".
> + If not specified, the default value is "borrower" and owner is
> + DOMID_IO, a system domain.
> +
> +As an example:
> +
> +chosen {
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + xen,xen-bootargs = "console=dtuart dtuart=serial0 bootscrub=0";
> +
> + ......
> +
> + /* this is for Dom0 */
> + dom0-shared-mem@10000000 {
> + compatible = "xen,domain-shared-memory-v1";
> + role = "owner";
> + xen,shm-id = "my-shared-mem-0";
> + xen,shared-mem = <0x10000000 0x10000000 0x10000000>;
> + }
> +
> + domU1 {
> + compatible = "xen,domain";
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + memory = <0 131072>;
> + cpus = <2>;
> + vpl011;
> +
> + /*
> + * shared memory region identified as 0x0(xen,shm-id = <0x0>)
> + * is shared between Dom0 and DomU1.
> + */
> + domU1-shared-mem@10000000 {
> + compatible = "xen,domain-shared-memory-v1";
> + role = "borrower";
> + xen,shm-id = "my-shared-mem-0";
> + xen,shared-mem = <0x10000000 0x10000000 0x50000000>;
> + }
> +
> + /*
> + * shared memory region identified as 0x1(xen,shm-id = <0x1>)
> + * is shared between DomU1 and DomU2.
> + */
> + domU1-shared-mem@50000000 {
> + compatible = "xen,domain-shared-memory-v1";
> + xen,shm-id = "my-shared-mem-1";
> + xen,shared-mem = <0x50000000 0x20000000 0x60000000>;
> + }
> +
> + ......
> +
> + };
> +
> + domU2 {
> + compatible = "xen,domain";
> + #address-cells = <0x1>;
> + #size-cells = <0x1>;
> + memory = <0 65536>;
> + cpus = <1>;
> +
> + /*
> + * shared memory region identified as 0x1(xen,shm-id = <0x1>)
> + * is shared between domU1 and domU2.
> + */
> + domU2-shared-mem@50000000 {
> + compatible = "xen,domain-shared-memory-v1";
> + xen,shm-id = "my-shared-mem-1";
> + xen,shared-mem = <0x50000000 0x20000000 0x70000000>;
> + }
> +
> + ......
> + };
> +};
> +
> +This is an example with two static shared memory regions.
> +
> +For the static shared memory region identified as "my-shared-mem-0", host
> +physical address starting at 0x10000000 of 256MB will be reserved to be
> +shared between Dom0 and DomU1. It will get mapped at 0x10000000 in Dom0 guest
> +physical address space, and at 0x50000000 in DomU1 guest physical address space.
> +Dom0 is explicitly defined as the owner domain, and DomU1 is the borrower domain.
> +
> +For the static shared memory region identified as "my-shared-mem-1", host
> +physical address starting at 0x50000000 of 512MB will be reserved to be
> +shared between DomU1 and DomU2. It will get mapped at 0x60000000 in DomU1 guest
> +physical address space, and at 0x70000000 in DomU2 guest physical address space.
> +DomU1 and DomU2 are both the borrower domain, the owner domain is the default
> +owner domain DOMID_IO.
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index be9eff0141..7321f47c0f 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -139,6 +139,12 @@ config TEE
>
> source "arch/arm/tee/Kconfig"
>
> +config STATIC_SHM
> + bool "Statically shared memory on a dom0less system" if UNSUPPORTED
> + depends on STATIC_MEMORY
> + help
> + This option enables statically shared memory on a dom0less system.
> +
> endmenu
>
> menu "ARM errata workaround via the alternative framework"
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ec81a45de9..c62d8867ec 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -13,6 +13,7 @@
> #include <xen/init.h>
> #include <xen/efi.h>
> #include <xen/device_tree.h>
> +#include <xen/lib.h>
> #include <xen/libfdt/libfdt.h>
> #include <xen/sort.h>
> #include <xsm/xsm.h>
> @@ -361,6 +362,158 @@ static int __init process_domain_node(const void *fdt, int node,
> size_cells, &bootinfo.reserved_mem, true);
> }
>
> +#ifdef CONFIG_STATIC_SHM
> +static int __init process_shm_node(const void *fdt, int node,
> + u32 address_cells, u32 size_cells)
> +{
> + const struct fdt_property *prop, *prop_id, *prop_role;
> + const __be32 *cell;
> + paddr_t paddr, size;
> + struct meminfo *mem = &bootinfo.reserved_mem;
> + unsigned int i;
> + int len;
> + bool owner = false;
> + const char *shm_id;
> +
> + if ( address_cells < 1 || size_cells < 1 )
> + {
> + printk("fdt: invalid #address-cells or #size-cells for static shared memory node.\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * "xen,shm-id" property holds an arbitrary string with a strict limit
> + * on the number of characters, MAX_SHM_ID_LENGTH
> + */
> + prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> + if ( !prop_id )
> + return -ENOENT;
> + shm_id = (const char *)prop_id->data;
> + if ( strnlen(shm_id, MAX_SHM_ID_LENGTH) == MAX_SHM_ID_LENGTH )
> + {
> + printk("fdt: invalid xen,shm-id %s, it must be limited to %d characters\n",
NIT: s/%d/%u/ as MAX_SHM_ID_LENGTH can not be negative.
> + shm_id, MAX_SHM_ID_LENGTH);
> + return -EINVAL;
> + }
> +
> + /*
> + * "role" property is optional and if it is defined explicitly,
> + * it must be either `owner` or `borrower`.
> + */
> + prop_role = fdt_get_property(fdt, node, "role", NULL);
> + if ( prop_role )
> + {
> + if ( !strcmp(prop_role->data, "owner") )
> + owner = true;
> + else if ( strcmp(prop_role->data, "borrower") )
> + {
> + printk("fdt: invalid `role` property for static shared memory node.\n");
> + return -EINVAL;
> + }
> + }
> +
> + /*
> + * xen,shared-mem = <paddr, size, gaddr>;
> + * Memory region starting from physical address #paddr of #size shall
> + * be mapped to guest physical address #gaddr as static shared memory
> + * region.
> + */
> + prop = fdt_get_property(fdt, node, "xen,shared-mem", &len);
> + if ( !prop )
> + return -ENOENT;
> +
> + if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
> + {
> + /* TODO: physical address is optional. */
NIT: I would drop this comment because the printk() clearly indicate
what's unsupported.
> + if ( len == dt_cells_to_size(size_cells + address_cells) )
> + printk("fdt: host physical address must be chosen by users at the moment.\n");
> +
> + printk("fdt: invalid `xen,shared-mem` property.\n");
> + return -EINVAL;
> + }
> +
> + cell = (const __be32 *)prop->data;
> + device_tree_get_reg(&cell, address_cells, size_cells, &paddr, &size);
> +
> + if ( !size )
> + {
> + printk("fdt: the size for static shared memory region can not be zero\n");
> + return -EINVAL;
> + }
> +
> + for ( i = 0; i < mem->nr_banks; i++ )
> + {
> + /*
> + * Meet 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
> + * with an existing one
> + */
> + if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> + {
> + if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
> + break;
> + else
> + {
> + printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
> + shm_id);
> + return -EINVAL;
> + }
> + }
> + else
> + {
> + paddr_t end = paddr + size;
> + paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
In both cases, end/bank_end may end up to be lower than
paddr/mem->bank[i].start. So I think we also want to check that they
don't overflow.
> +
> + if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
> + {
> + if ( strncmp(shm_id, mem->bank[i].shm_id,
> + MAX_SHM_ID_LENGTH) != 0 )
You have already validate the string. So you could use strcmp() here.
Otherwise, it seems...
> + continue;
> + else
> + {
> + printk("fdt: different shared memory region could not share the same shm ID %s\n",
> + shm_id);
... odd to print the value if you don't trust it :).
> + return -EINVAL;
> + }
> + }
> + else
> + {
> + printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
> + mem->bank[i].start, bank_end);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + if ( i == mem->nr_banks )
> + {
> + if ( i < NR_MEM_BANKS )
> + {
> + /* Static shared memory shall be reserved from any other use. */
> + safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
> + mem->bank[mem->nr_banks].start = paddr;
> + mem->bank[mem->nr_banks].size = size;
> + mem->bank[mem->nr_banks].xen_domain = true;
> + mem->nr_banks++;
> + }
> + else
> + {
> + printk("Warning: Max number of supported memory regions reached.\n");
> + return -ENOSPC;
> + }
> + }
> + /*
> + * keep a count of the number of borrowers, which later may be used
> + * to calculate the reference count.
> + */
> + if ( !owner )
> + mem->bank[i].nr_shm_borrowers++;
> +
> + return 0;
> +}
> +#endif
> +
> static int __init early_scan_node(const void *fdt,
> int node, const char *name, int depth,
> u32 address_cells, u32 size_cells,
> @@ -386,6 +539,10 @@ static int __init early_scan_node(const void *fdt,
> process_chosen_node(fdt, node, name, address_cells, size_cells);
> else if ( depth == 2 && device_tree_node_compatible(fdt, node, "xen,domain") )
> rc = process_domain_node(fdt, node, name, address_cells, size_cells);
> +#ifdef CONFIG_STATIC_SHM
> + else if ( depth <= 3 && device_tree_node_compatible(fdt, node, "xen,domain-shared-memory-v1") )
> + rc = process_shm_node(fdt, node, address_cells, size_cells);
> +#endif
I would prefer if we provide a dummy helper for process_shm_node() when
!CONFIG_STATIC_SHM. This would also have the advantage to let the user
know that they are trying to use shared memory with a Xen that doesn't
support it.
>
> if ( rc < 0 )
> printk("fdt: node `%s': parsing failed\n", name);
> diff --git 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.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/9] xen/arm: allocate static shared memory to the default owner dom_io
2022-07-21 13:21 ` [PATCH v6 2/9] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
@ 2022-08-26 13:59 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2022-08-26 13:59 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Jan Beulich, Wei Liu
Hi Penny,
On 21/07/2022 14:21, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> 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 <penny.zheng@arm.com>
> ---
> v6 change:
> - use if-array to check psize, pbase and gbase are suitably aligned and
> valid
> - use mfn_valid to check (pbase, pbase + psize)
> - check d->max_pages will not overflow
> - refine acquire_shared_memory_bank to not reuse acquire_static_memory_bank,
> then input pbase and psize do not need to be used as a pointer.
> - use if-array to check if page owner is dom_io
> - in-code comment refinement
> ---
> v5 change:
> - refine in-code comment
> ---
> 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 | 155 ++++++++++++++++++++++++++++++++++++
> xen/common/domain.c | 3 +
> 2 files changed, 158 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3fd1186b53..ff2aebaf28 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -759,6 +759,155 @@ 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);
> + if ( page_get_owner(page) == NULL )
Sorry, I should have stopped the issue with the call before.
page_get_owner() can only be called for pages that are marked inuse.
This is because the field "domain" is part of an union.
So I think you want to use page_get_owner_and_reference() here. The
reference will have to be dropped using put_page(). I think it should be
fine to do within the same function because we are still at init and
therefore the page owner shouldn't change. I would explain that in a
comment.
> + return false;
> +
> + if ( page_get_owner(page) != dom_io )
Let's not duplicate the call to page_get_owner(). You can use the result
from the caller above and drop the reference afterwards (see above).
> + {
> + printk(XENLOG_ERR
> + "shm memory node has already been allocated to a specific owner %pd, Please check your configuration\n",
> + page_get_owner(page));
> + return false;
> + }
> +
> + return true;
> +}
> +
> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> + paddr_t pbase, paddr_t psize)
> +{
> + mfn_t smfn;
> + unsigned long nr_pfns;
> + int res;
> +
> + /*
> + * Pages of statically shared memory shall be included
> + * into domain_tot_pages().
> + */
> + nr_pfns = PFN_DOWN(psize);
> + if ( (UINT_MAX - d->max_pages) < nr_pfns )
> + {
> + printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> + d, nr_pfns);
> + return INVALID_MFN;
> + }
> + d->max_pages += nr_pfns;
> +
> + smfn = maddr_to_mfn(pbase);
> + res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> + if ( res )
> + {
> + printk(XENLOG_ERR
> + "%pd: failed to acquire static memory: %d.\n", d, res);
Should not we adjust "d->max_pages" if acquire_domstatic_pages() fails?
> + return INVALID_MFN;
> + }
> +
> + return smfn;
> +}
> +
> +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,
> + "%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> + d, pbase, pbase + psize);
> +
> + smfn = acquire_shared_memory_bank(d, pbase, psize);
> + if ( mfn_eq(smfn, INVALID_MFN) )
> + return -EINVAL;
> +
> + /*
> + * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
It is not quite clear why DOMID_XEN is mentioned here. Yes, it is
auto-translated like DOMID_IO, but code doesn't care about that here.
> + * It sees RAM 1:1 and we do not need to create P2M mapping for it
> + */
The overall comment reads a bit odd without the implement for non-dom0.
I don't have a good suggestion other than have a different comment for
now and rewording it. But that's just churn.
Regarding the current comment I would suggest the following:
DOMID_IO is auto-translated (i.e. it seems RAM 1:1). So we do not need
to create mapping in the P2M.
> + 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;
> +
> + dt_for_each_child_node(node, shm_node)
> + {
> + const struct dt_property *prop;
> + const __be32 *cells;
> + uint32_t addr_cells, size_cells;
> + paddr_t gbase, pbase, psize;
> + int ret = 0;
> + unsigned int i;
> +
> + if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> + continue;
> +
> + /*
> + * xen,shared-mem = <pbase, psize, gbase>;
> + * TODO: pbase is optional.
> + */
> + 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); > + ASSERT(prop);
I would suggest to switch to BUG_ON() because this is init code. So
better to be obvious.
> + cells = (const __be32 *)prop->value;
> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> + gbase = dt_read_number(cells, addr_cells);
> + if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
> + {
> + printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
> + d, pbase, gbase);
> + return -EINVAL;
> + }
> + if ( !IS_ALIGNED(psize, PAGE_SIZE) )
> + {
> + printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n",
> + d, psize);
> + return -EINVAL;
> + }
> +
> + for ( i = 0; i < PFN_DOWN(psize); i++ )
> + if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
> + {
> + printk("%pd: invalid physical address 0x%"PRI_mfn"\n",
> + d, mfn_x(mfn_add(maddr_to_mfn(pbase), i)));
> + return -EINVAL;
> + }
> +
> + /* TODO: Consider owner domain is not the default dom_io. */
> + /*
> + * Per static shared memory region could be shared between multiple
> + * domains.
Well, it would not be shared memory otherwise :). I think here you are
referring to the fact that the owner may be DOMID_IO *and* the area
still shared with multiple borrower.
> + * 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.
> + */
I think it would be worth explaining in the commit message that DOMID_IO
is not a "real" domain. This is why you need to check whether the shared
area has been reserved. So how about replacing the two paragraphs with:
"DOMID_IO is a fake domain and is not described in the Device-Tree.
Therefore When the owner of the shared region is DOMID_IO, we will only
find the borrowers."
> + if ( !is_shm_allocated_to_domio(pbase) )
> + {
> + /* Allocate statically shared pages to the default owner dom_io. */
This is mostly repeating the line below. How about:
"We found the first borrower of the region, the owner was not specified
so they should be assign to dom0".
> + ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> + pbase, psize);
While proposing some comments above, I realized that this function is
not really allocating memory. It is assigning a set of pages to domain.
So may I suggest to rename it to assign_shared_memory()? Or maybe
acquire_shared_memory()?
> + if ( ret )
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_STATIC_SHM */
> #else
> static void __init allocate_static_memory(struct domain *d,
> struct kernel_info *kinfo,
> @@ -3236,6 +3385,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 618410e3b2..c8564113e9 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -780,6 +780,9 @@ 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 is also the default owner of memory pre-shared among multiple
> + * domains at boot time.
> */
> dom_io = domain_create(DOMID_IO, NULL, 0);
> if ( IS_ERR(dom_io) )
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md
2022-08-26 7:21 ` Michal Orzel
@ 2022-08-29 6:20 ` Penny Zheng
0 siblings, 0 replies; 23+ messages in thread
From: Penny Zheng @ 2022-08-29 6:20 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Andrew Cooper, George Dunlap, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Sent: Friday, August 26, 2022 3:21 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap
> <george.dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wl@xen.org>
> Subject: Re: [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md
>
> Hi Penny,
>
Hi Michal
WoW,, super excited to see you back here;))))))))
> On 21/07/2022 15:21, Penny Zheng wrote:
> > on ARM, static memory sharing is tech preview, which shall be documented
> NIT: missing 'a' before 'tech preview'.
>
> > in SUPPORT.md
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v6 change:
> > - new commit
> > ---
> > SUPPORT.md | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/SUPPORT.md b/SUPPORT.md
> > index 8e040d1c1e..3dfe6d2fbe 100644
> > --- a/SUPPORT.md
> > +++ b/SUPPORT.md
> > @@ -299,6 +299,12 @@ Allow sharing of identical pages between guests
> >
> > Status, x86 HVM: Experimental
> >
> > +### Static Memory Sharing
> > +
> > +Allow memory pre-shared among multiple domains at boot time through
> device tree configuration
> This does not explicitly state that this feature is for dom0less only.
> How about taking what you wrote in booting.txt:
> "Allow to statically set up shared memory on dom0less system, enabling
> domains to do shm-based communication".
>
Ok, will do
> > +
> > + Status, ARM: Tech Preview
> > +
> > ### Memory Paging
> >
> > Allow pages belonging to guests to be paged to disk
>
> ~Michal
A thousand thanks
Penny
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-08-26 13:17 ` Julien Grall
@ 2022-08-29 6:57 ` Penny Zheng
2022-08-29 21:16 ` Stefano Stabellini
2022-09-01 15:40 ` Julien Grall
0 siblings, 2 replies; 23+ messages in thread
From: Penny Zheng @ 2022-08-29 6:57 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
> -----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.
"
Do you think I shall further point out that right now, this part feature is not implemented
in codes?
> > 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.
I'm open to the discussion and will let it decide what it finally will be. ;)
> Cheers,
>
> --
> Julien Grall
Cheers,
--
Penny Zheng
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-08-29 6:57 ` Penny Zheng
@ 2022-08-29 21:16 ` Stefano Stabellini
2022-09-01 15:40 ` Julien Grall
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2022-08-29 21:16 UTC (permalink / raw)
To: Penny Zheng
Cc: Julien Grall, xen-devel, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
On Mon, 29 Aug 2022, 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.
> "
> Do you think I shall further point out that right now, this part feature is not implemented
> in codes?
Yeah. I would use:
Host physical address is optional, when missing Xen decides the
location (currently unimplemented).
> > > 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.
Yeah, exactly
> 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.
>
> I'm open to the discussion and will let it decide what it finally will be. ;)
Yeah something like that would work. I'll let Julien comments on the
specifics, but that's not for this week.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-08-29 6:57 ` Penny Zheng
2022-08-29 21:16 ` Stefano Stabellini
@ 2022-09-01 15:40 ` Julien Grall
2022-09-01 15:44 ` Bertrand Marquis
2022-09-02 3:26 ` Penny Zheng
1 sibling, 2 replies; 23+ messages in thread
From: Julien Grall @ 2022-09-01 15:40 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-09-01 15:40 ` Julien Grall
@ 2022-09-01 15:44 ` Bertrand Marquis
2022-09-02 3:26 ` Penny Zheng
1 sibling, 0 replies; 23+ messages in thread
From: Bertrand Marquis @ 2022-09-01 15:44 UTC (permalink / raw)
To: Julien Grall
Cc: Penny Zheng, xen-devel, Stefano Stabellini, Volodymyr Babchuk
Hi,
> On 1 Sep 2022, at 16:40, Julien Grall <julien@xen.org> wrote:
>
> 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.
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated
2022-07-21 13:21 ` [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
@ 2022-09-01 17:12 ` Julien Grall
2022-09-02 1:59 ` Stefano Stabellini
0 siblings, 1 reply; 23+ messages in thread
From: Julien Grall @ 2022-09-01 17:12 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 21/07/2022 14:21, Penny Zheng wrote:
> Borrower domain will fail to get a page ref using the owner domain
> during allocation, when the owner is created after borrower.
>
> So here, we decide to get and add the right amount of reference, which
> is the number of borrowers, when the owner is allocated.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
IMHO, this tag should not have been kept given...
> ---
> v6 change:
> - adapt to the change of "nr_shm_borrowers"
... this change. 'reviewed-by' means that the person reviewed the code
and therefore agree with the logic. So I would only keep the tag if the
change is trivial (including typo, coding style) and would drop it (or
confirm with the person) otherwise.
Stefano, can you confirm you are happy that your reviewed-by tag is kept?
> - add in-code comment to explain if the borrower is created first, we intend to
> add pages in the P2M without reference.
> ---
> v5 change:
> - no change
> ---
> v4 changes:
> - no change
> ---
> v3 change:
> - printk rather than dprintk since it is a serious error
> ---
> v2 change:
> - new commit
> ---
> xen/arch/arm/domain_build.c | 60 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index a7e95c34a7..e891e800a7 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -761,6 +761,30 @@ static void __init assign_static_memory_11(struct domain *d,
> }
>
> #ifdef CONFIG_STATIC_SHM
> +static int __init acquire_nr_borrower_domain(struct domain *d,
> + paddr_t pbase, paddr_t psize,
> + unsigned long *nr_borrowers)
> +{
> + unsigned long bank;
NIT: AFAICT nr_banks is an "unsigned int".
Other than that:
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated
2022-09-01 17:12 ` Julien Grall
@ 2022-09-02 1:59 ` Stefano Stabellini
0 siblings, 0 replies; 23+ messages in thread
From: Stefano Stabellini @ 2022-09-02 1:59 UTC (permalink / raw)
To: Julien Grall
Cc: Penny Zheng, xen-devel, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk
On Thu, 1 Sep 2022, Julien Grall wrote:
> Hi Penny,
>
> On 21/07/2022 14:21, Penny Zheng wrote:
> > Borrower domain will fail to get a page ref using the owner domain
> > during allocation, when the owner is created after borrower.
> >
> > So here, we decide to get and add the right amount of reference, which
> > is the number of borrowers, when the owner is allocated.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
> IMHO, this tag should not have been kept given...
>
> > ---
> > v6 change:
> > - adapt to the change of "nr_shm_borrowers"
>
> ... this change. 'reviewed-by' means that the person reviewed the code and
> therefore agree with the logic. So I would only keep the tag if the change is
> trivial (including typo, coding style) and would drop it (or confirm with the
> person) otherwise.
>
> Stefano, can you confirm you are happy that your reviewed-by tag is kept?
Yes I confirm
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > - add in-code comment to explain if the borrower is created first, we intend
> > to
> > add pages in the P2M without reference.
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 changes:
> > - no change
> > ---
> > v3 change:
> > - printk rather than dprintk since it is a serious error
> > ---
> > v2 change:
> > - new commit
> > ---
> > xen/arch/arm/domain_build.c | 60 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 60 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index a7e95c34a7..e891e800a7 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -761,6 +761,30 @@ static void __init assign_static_memory_11(struct
> > domain *d,
> > }
> > #ifdef CONFIG_STATIC_SHM
> > +static int __init acquire_nr_borrower_domain(struct domain *d,
> > + paddr_t pbase, paddr_t psize,
> > + unsigned long *nr_borrowers)
> > +{
> > + unsigned long bank;
>
> NIT: AFAICT nr_banks is an "unsigned int".
>
> Other than that:
>
> Acked-by: Julien Grall <jgrall@amazon.com>
>
> Cheers,
>
> --
> Julien Grall
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-09-01 15:40 ` Julien Grall
2022-09-01 15:44 ` Bertrand Marquis
@ 2022-09-02 3:26 ` Penny Zheng
2022-09-02 8:13 ` Julien Grall
1 sibling, 1 reply; 23+ messages in thread
From: Penny Zheng @ 2022-09-02 3:26 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Thursday, September 1, 2022 11:40 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,
>
> 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]>
>
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/9] xen/arm: introduce static shared memory
2022-09-02 3:26 ` Penny Zheng
@ 2022-09-02 8:13 ` Julien Grall
0 siblings, 0 replies; 23+ messages in thread
From: Julien Grall @ 2022-09-02 8:13 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 02/09/2022 04:26, Penny Zheng wrote:
>>> 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.
The binding will become stable as soon as we release 4.17. So this would
need to be fixed before releasing.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-09-02 8:13 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-21 13:21 [PATCH v6 0/9] static shared memory on dom0less system Penny Zheng
2022-07-21 13:21 ` [PATCH v6 1/9] xen/arm: introduce static shared memory Penny Zheng
2022-08-26 13:17 ` Julien Grall
2022-08-29 6:57 ` Penny Zheng
2022-08-29 21:16 ` Stefano Stabellini
2022-09-01 15:40 ` Julien Grall
2022-09-01 15:44 ` Bertrand Marquis
2022-09-02 3:26 ` Penny Zheng
2022-09-02 8:13 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 2/9] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
2022-08-26 13:59 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 3/9] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
2022-07-21 13:21 ` [PATCH v6 4/9] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
2022-07-21 13:21 ` [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
2022-09-01 17:12 ` Julien Grall
2022-09-02 1:59 ` Stefano Stabellini
2022-07-21 13:21 ` [PATCH v6 6/9] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
2022-07-21 13:21 ` [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree Penny Zheng
2022-08-26 13:13 ` Julien Grall
2022-07-21 13:21 ` [PATCH v6 8/9] xen/arm: enable statically shared memory on Dom0 Penny Zheng
2022-07-21 13:21 ` [PATCH v6 9/9] xen: Add static memory sharing in SUPPORT.md Penny Zheng
2022-08-26 7:21 ` Michal Orzel
2022-08-29 6:20 ` Penny Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.