* [PATCH v5 0/8] static shared memory on dom0less system
@ 2022-06-20 5:11 Penny Zheng
2022-06-20 5:11 ` [PATCH v5 1/8] xen/arm: introduce static shared memory Penny Zheng
` (7 more replies)
0 siblings, 8 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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 (8):
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
docs/misc/arm/device-tree/booting.txt | 120 ++++++++
xen/arch/arm/Kconfig | 6 +
xen/arch/arm/bootfdt.c | 68 +++++
xen/arch/arm/domain_build.c | 378 +++++++++++++++++++++++++-
xen/arch/arm/include/asm/kernel.h | 1 +
xen/arch/arm/include/asm/mm.h | 4 +
xen/arch/arm/include/asm/setup.h | 4 +
xen/arch/arm/mm.c | 42 ++-
xen/common/domain.c | 3 +
9 files changed, 616 insertions(+), 10 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-24 17:55 ` Julien Grall
2022-06-24 19:25 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
` (6 subsequent siblings)
7 siblings, 2 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
This patch serie 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v5 change:
- no change
---
v4 change:
- nit fix on doc
---
v3 change:
- make nr_shm_domain unsigned int
---
v2 change:
- document refinement
- remove bitmap and use the iteration to check
- add a new field nr_shm_domain to keep the number of shared domain
---
docs/misc/arm/device-tree/booting.txt | 120 ++++++++++++++++++++++++++
xen/arch/arm/Kconfig | 6 ++
xen/arch/arm/bootfdt.c | 68 +++++++++++++++
xen/arch/arm/include/asm/setup.h | 3 +
4 files changed, 197 insertions(+)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 98253414b8..6467bc5a28 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -378,3 +378,123 @@ device-tree:
This will reserve a 512MB region starting at the host physical address
0x30000000 to be exclusively used by DomU1.
+
+Static Shared Memory
+====================
+
+The static shared memory device tree nodes allow users to statically set up
+shared memory on dom0less system, enabling domains to do shm-based
+communication.
+
+- compatible
+
+ "xen,domain-shared-memory-v1"
+
+- xen,shm-id
+
+ An 8-bit integer that represents the unique identifier of the shared memory
+ region. The maximum identifier shall be "xen,shm-id = <0xff>".
+
+- xen,shared-mem
+
+ An array takes a physical address, which is the base address of the
+ shared memory region in host physical address space, a size, and a guest
+ physical address, as the target address of the mapping. The number of cells
+ for the host address (and size) is the same as the guest pseudo-physical
+ address and they are inherited from the parent node.
+
+- 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
+ "dom_shared", 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 = <0x0>;
+ 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 = <0x0>;
+ 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 = <0x1>;
+ 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 = <0x1>;
+ xen,shared-mem = <0x50000000 0x20000000 0x70000000>;
+ }
+
+ ......
+ };
+};
+
+This is an example with two static shared memory regions.
+
+For the static shared memory region identified as 0x0, 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 0x1, 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 dom_shared.
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..38dcb05d5d 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -361,6 +361,70 @@ 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;
+ const __be32 *cell;
+ paddr_t paddr, size;
+ struct meminfo *mem = &bootinfo.reserved_mem;
+ unsigned long i;
+
+ if ( address_cells < 1 || size_cells < 1 )
+ {
+ printk("fdt: invalid #address-cells or #size-cells for static shared memory node.\n");
+ return -EINVAL;
+ }
+
+ prop = fdt_get_property(fdt, node, "xen,shared-mem", NULL);
+ if ( !prop )
+ return -ENOENT;
+
+ /*
+ * 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.
+ */
+ cell = (const __be32 *)prop->data;
+ device_tree_get_reg(&cell, address_cells, size_cells, &paddr, &size);
+ for ( i = 0; i < mem->nr_banks; i++ )
+ {
+ /*
+ * A static shared memory region could be shared between multiple
+ * domains.
+ */
+ if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
+ break;
+ }
+
+ if ( i == mem->nr_banks )
+ {
+ if ( i < NR_MEM_BANKS )
+ {
+ /* Static shared memory shall be reserved from any other use. */
+ 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 domains, which later may be used to
+ * calculate the number of the reference count.
+ */
+ mem->bank[i].nr_shm_domain++;
+
+ 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 +450,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..5063e5d077 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -27,6 +27,9 @@ 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
+ unsigned int nr_shm_domain;
+#endif
};
struct meminfo {
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
2022-06-20 5:11 ` [PATCH v5 1/8] xen/arm: introduce static shared memory Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-24 18:22 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
` (5 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
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 | 132 +++++++++++++++++++++++++++++++++++-
xen/common/domain.c | 3 +
2 files changed, 134 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7ddd16c26d..91a5ace851 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -527,6 +527,10 @@ static bool __init append_static_memory_to_bank(struct domain *d,
return true;
}
+/*
+ * If cell is NULL, pbase and psize should hold valid values.
+ * Otherwise, cell will be populated together with pbase and psize.
+ */
static mfn_t __init acquire_static_memory_bank(struct domain *d,
const __be32 **cell,
u32 addr_cells, u32 size_cells,
@@ -535,7 +539,8 @@ static mfn_t __init acquire_static_memory_bank(struct domain *d,
mfn_t smfn;
int res;
- device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
+ if ( cell )
+ device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
if ( PFN_DOWN(*psize) > UINT_MAX )
{
@@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct domain *d,
panic("Failed to assign requested static memory for direct-map domain %pd.",
d);
}
+
+#ifdef CONFIG_STATIC_SHM
+/*
+ * This function checks whether the static shared memory region is
+ * already allocated to dom_io.
+ */
+static bool __init is_shm_allocated_to_domio(paddr_t pbase)
+{
+ struct page_info *page;
+
+ page = maddr_to_page(pbase);
+ ASSERT(page);
+
+ if ( page_get_owner(page) == NULL )
+ return false;
+
+ ASSERT(page_get_owner(page) == dom_io);
+ return true;
+}
+
+static mfn_t __init acquire_shared_memory_bank(struct domain *d,
+ u32 addr_cells, u32 size_cells,
+ paddr_t *pbase, paddr_t *psize)
+{
+ /*
+ * Pages of statically shared memory shall be included
+ * in domain_tot_pages().
+ */
+ d->max_pages += PFN_DOWN(*psize);
+
+ return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
+ pbase, psize);
+
+}
+
+/*
+ * Func allocate_shared_memory is supposed to be only called
+ * from the owner.
+ */
+static int __init allocate_shared_memory(struct domain *d,
+ u32 addr_cells, u32 size_cells,
+ paddr_t pbase, paddr_t psize)
+{
+ mfn_t smfn;
+
+ dprintk(XENLOG_INFO,
+ "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
+ pbase, pbase + psize);
+
+ smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
+ &psize);
+ if ( mfn_eq(smfn, INVALID_MFN) )
+ return -EINVAL;
+
+ /*
+ * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
+ * It sees RAM 1:1 and we do not need to create P2M mapping for it
+ */
+ ASSERT(d == dom_io);
+ return 0;
+}
+
+static int __init process_shm(struct domain *d,
+ const struct dt_device_node *node)
+{
+ struct dt_device_node *shm_node;
+ int ret = 0;
+ const struct dt_property *prop;
+ const __be32 *cells;
+ u32 shm_id;
+ u32 addr_cells, size_cells;
+ paddr_t gbase, pbase, psize;
+
+ dt_for_each_child_node(node, shm_node)
+ {
+ if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
+ continue;
+
+ if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
+ {
+ printk("Shared memory node does not provide \"xen,shm-id\" property.\n");
+ return -ENOENT;
+ }
+
+ addr_cells = dt_n_addr_cells(shm_node);
+ size_cells = dt_n_size_cells(shm_node);
+ prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
+ if ( !prop )
+ {
+ printk("Shared memory node does not provide \"xen,shared-mem\" property.\n");
+ return -ENOENT;
+ }
+ cells = (const __be32 *)prop->value;
+ /* xen,shared-mem = <pbase, psize, gbase>; */
+ device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
+ ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
+ gbase = dt_read_number(cells, addr_cells);
+
+ /* TODO: Consider owner domain is not the default dom_io. */
+ /*
+ * Per static shared memory region could be shared between multiple
+ * domains.
+ * In case re-allocating the same shared memory region, we check
+ * if it is already allocated to the default owner dom_io before
+ * the actual allocation.
+ */
+ if ( !is_shm_allocated_to_domio(pbase) )
+ {
+ /* Allocate statically shared pages to the default owner dom_io. */
+ ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
+ pbase, psize);
+ if ( ret )
+ return ret;
+ }
+ }
+
+ return 0;
+}
+#endif /* CONFIG_STATIC_SHM */
#else
static void __init allocate_static_memory(struct domain *d,
struct kernel_info *kinfo,
@@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain *d,
else
assign_static_memory_11(d, &kinfo, node);
+#ifdef CONFIG_STATIC_SHM
+ rc = process_shm(d, node);
+ if ( rc < 0 )
+ return rc;
+#endif
+
/*
* Base address and irq number are needed when creating vpl011 device
* tree node in prepare_dtb_domU, so initialization on related variables
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7570eae91a..7070f5a9b9 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] 38+ messages in thread
* [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
2022-06-20 5:11 ` [PATCH v5 1/8] xen/arm: introduce static shared memory Penny Zheng
2022-06-20 5:11 ` [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-24 19:07 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 4/8] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
` (4 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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>
---
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 91a5ace851..d4fd64e2bd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -805,9 +805,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,
"Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
@@ -822,8 +824,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,
@@ -836,6 +848,8 @@ static int __init process_shm(struct domain *d,
u32 shm_id;
u32 addr_cells, size_cells;
paddr_t gbase, pbase, psize;
+ const char *role_str;
+ bool owner_dom_io = true;
dt_for_each_child_node(node, shm_node)
{
@@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
gbase = dt_read_number(cells, addr_cells);
- /* TODO: Consider owner domain is not the default dom_io. */
+ /*
+ * "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] 38+ messages in thread
* [PATCH v5 4/8] xen/arm: introduce put_page_nr and get_page_nr
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
` (2 preceding siblings ...)
2022-06-20 5:11 ` [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-24 19:10 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
` (3 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
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>
---
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 045a8ba4bb..8384eac2c8 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 be37176a47..79b7d8de56 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] 38+ messages in thread
* [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
` (3 preceding siblings ...)
2022-06-20 5:11 ` [PATCH v5 4/8] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-24 19:18 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 6/8] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
` (2 subsequent siblings)
7 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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>
---
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 | 62 +++++++++++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d4fd64e2bd..650d18f5ef 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -799,6 +799,34 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
}
+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;
+
+ if ( d == dom_io )
+ *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain;
+ else
+ /* Exclude the owner domain itself. */
+ *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1;
+
+ return 0;
+}
+
/*
* Func allocate_shared_memory is supposed to be only called
* from the owner.
@@ -810,6 +838,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,
"Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
@@ -824,6 +854,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, PFN_DOWN(psize));
@@ -835,6 +866,37 @@ static int __init allocate_shared_memory(struct domain *d,
}
}
+ /*
+ * Get the right amount of references per page, which is the number of
+ * borrow domains.
+ */
+ ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
+ if ( ret )
+ return ret;
+
+ /*
+ * Instead of let 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.
+ */
+ 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] 38+ messages in thread
* [PATCH v5 6/8] xen/arm: set up shared memory foreign mapping for borrower domain
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
` (4 preceding siblings ...)
2022-06-20 5:11 ` [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-20 5:11 ` [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree Penny Zheng
2022-06-20 5:11 ` [PATCH v5 8/8] xen/arm: enable statically shared memory on Dom0 Penny Zheng
7 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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>
---
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 650d18f5ef..1584e6c2ce 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -962,6 +962,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] 38+ messages in thread
* [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
` (5 preceding siblings ...)
2022-06-20 5:11 ` [PATCH v5 6/8] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
2022-06-24 19:30 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 8/8] xen/arm: enable statically shared memory on Dom0 Penny Zheng
7 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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>
---
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 | 143 +++++++++++++++++++++++++++++-
xen/arch/arm/include/asm/kernel.h | 1 +
xen/arch/arm/include/asm/setup.h | 1 +
3 files changed, 143 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1584e6c2ce..4d62440a0e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -900,7 +900,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,
+ u32 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;
+ 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;
@@ -971,6 +986,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;
@@ -1301,6 +1324,117 @@ 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,
+ struct meminfo *mem)
+{
+ unsigned long 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;
+ uint8_t shm_id = mem->bank[i].shm_id;
+ /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
+ char buf[27];
+ const char compat[] = "xen,shared-memory-v1";
+ __be32 reg[4];
+ __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 %lu: %#"PRIx64"->%#"PRIx64"\n",
+ i, start, start + size);
+
+ res = fdt_property_cell(fdt, "xen,id", 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;
@@ -3078,6 +3212,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
@@ -3454,7 +3593,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;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 5063e5d077..7497cc40aa 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -29,6 +29,7 @@ struct membank {
bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
#ifdef CONFIG_STATIC_SHM
unsigned int nr_shm_domain;
+ uint8_t shm_id ; /* ID identifier of a static shared memory bank. */
#endif
};
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v5 8/8] xen/arm: enable statically shared memory on Dom0
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
` (6 preceding siblings ...)
2022-06-20 5:11 ` [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree Penny Zheng
@ 2022-06-20 5:11 ` Penny Zheng
7 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-20 5:11 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, 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>
---
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 4d62440a0e..b57c60f411 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2658,6 +2658,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);
@@ -3730,6 +3735,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);
@@ -3764,6 +3772,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] 38+ messages in thread
* Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-20 5:11 ` [PATCH v5 1/8] xen/arm: introduce static shared memory Penny Zheng
@ 2022-06-24 17:55 ` Julien Grall
2022-06-29 5:38 ` Penny Zheng
2022-06-29 8:39 ` Penny Zheng
2022-06-24 19:25 ` Julien Grall
1 sibling, 2 replies; 38+ messages in thread
From: Julien Grall @ 2022-06-24 17:55 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 20/06/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> This patch serie introduces a new feature: setting up static
Typo: s/serie/series/
> shared memory on a dom0less system, through device tree configuration.
>
> This commit parses shared memory node at boot-time, and reserve it in
> bootinfo.reserved_mem to avoid other use.
>
> This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> static-shm-related codes, and this option depends on static memory(
> CONFIG_STATIC_MEMORY). That's because that later we want to reuse a few
> helpers, guarded with CONFIG_STATIC_MEMORY, like acquire_staticmem_pages, etc,
> on static shared memory.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> v5 change:
> - no change
> ---
> v4 change:
> - nit fix on doc
> ---
> v3 change:
> - make nr_shm_domain unsigned int
> ---
> v2 change:
> - document refinement
> - remove bitmap and use the iteration to check
> - add a new field nr_shm_domain to keep the number of shared domain
> ---
> docs/misc/arm/device-tree/booting.txt | 120 ++++++++++++++++++++++++++
> xen/arch/arm/Kconfig | 6 ++
> xen/arch/arm/bootfdt.c | 68 +++++++++++++++
> xen/arch/arm/include/asm/setup.h | 3 +
> 4 files changed, 197 insertions(+)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..6467bc5a28 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,123 @@ device-tree:
>
> This will reserve a 512MB region starting at the host physical address
> 0x30000000 to be exclusively used by DomU1.
> +
> +Static Shared Memory
> +====================
> +
> +The static shared memory device tree nodes allow users to statically set up
> +shared memory on dom0less system, enabling domains to do shm-based
> +communication.
> +
> +- compatible
> +
> + "xen,domain-shared-memory-v1"
> +
> +- xen,shm-id
> +
> + An 8-bit integer that represents the unique identifier of the shared memory
> + region. The maximum identifier shall be "xen,shm-id = <0xff>".
> +
> +- xen,shared-mem
> +
> + An array takes a physical address, which is the base address of the
> + shared memory region in host physical address space, a size, and a guest
> + physical address, as the target address of the mapping. The number of cells
> + for the host address (and size) is the same as the guest pseudo-physical
> + address and they are inherited from the parent node.
Sorry for jump in the discussion late. But as this is going to be a
stable ABI, I would to make sure the interface is going to be easily
extendable.
AFAIU, with your proposal the host physical address is mandatory. I
would expect that some user may want to share memory but don't care
about the exact location in memory. So I think it would be good to make
it optional in the binding.
I think this wants to be done now because it would be difficult to
change the binding afterwards (the host physical address is the first
set of cells).
The Xen doesn't need to handle the optional case.
[...]
> 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
You also want to update SUPPORT.md.
> + 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..38dcb05d5d 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -361,6 +361,70 @@ 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;
> + const __be32 *cell;
> + paddr_t paddr, size;
> + struct meminfo *mem = &bootinfo.reserved_mem;
> + unsigned long i;
nr_banks is "unsigned int" so I think this should be "unsigned int" as well.
> +
> + if ( address_cells < 1 || size_cells < 1 )
> + {
> + printk("fdt: invalid #address-cells or #size-cells for static shared memory node.\n");
> + return -EINVAL;
> + }
> +
> + prop = fdt_get_property(fdt, node, "xen,shared-mem", NULL);
> + if ( !prop )
> + return -ENOENT;
> +
> + /*
> + * 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.
> + */
> + cell = (const __be32 *)prop->data;
> + device_tree_get_reg(&cell, address_cells, size_cells, &paddr, &size);
Please check the len of the property to confirm is it big enough to
contain "paddr", "size", and "gaddr".
> + for ( i = 0; i < mem->nr_banks; i++ )
> + {
> + /*
> + * A static shared memory region could be shared between multiple
> + * domains.
> + */
> + if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> + break;
> + }
> +
> + if ( i == mem->nr_banks )
> + {
> + if ( i < NR_MEM_BANKS )
> + {
> + /* Static shared memory shall be reserved from any other use. */
> + 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 domains, which later may be used to
> + * calculate the number of the reference count.
> + */
> + mem->bank[i].nr_shm_domain++;
> +
> + 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 +450,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..5063e5d077 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -27,6 +27,9 @@ 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
> + unsigned int nr_shm_domain;
> +#endif
> };
>
> struct meminfo {
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
2022-06-20 5:11 ` [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
@ 2022-06-24 18:22 ` Julien Grall
2022-06-29 7:13 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-24 18:22 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Hi Penny,
On 20/06/2022 06:11, 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.
The document in patch #1 suggest the page will be shared with
dom_shared. But here you say "DOMID_IO".
Which one is correct?
>
> 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>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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 | 132 +++++++++++++++++++++++++++++++++++-
> xen/common/domain.c | 3 +
> 2 files changed, 134 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7ddd16c26d..91a5ace851 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -527,6 +527,10 @@ static bool __init append_static_memory_to_bank(struct domain *d,
> return true;
> }
>
> +/*
> + * If cell is NULL, pbase and psize should hold valid values.
> + * Otherwise, cell will be populated together with pbase and psize.
> + */
> static mfn_t __init acquire_static_memory_bank(struct domain *d,
> const __be32 **cell,
> u32 addr_cells, u32 size_cells,
> @@ -535,7 +539,8 @@ static mfn_t __init acquire_static_memory_bank(struct domain *d,
> mfn_t smfn;
> int res;
>
> - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> + if ( cell )
> + device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
I think this is a bit of a hack. To me it sounds like this should be
moved out to a separate helper. This will also make the interface of
acquire_shared_memory_bank() less questionable (see below).
As this is v5, I would be OK with a follow-up for this split. But this
interface of acuiqre_shared_memory_bank() needs to change.
> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
In the context of your series, who is checking that both psize and pbase
are suitably aligned?
> if ( PFN_DOWN(*psize) > UINT_MAX )
> {
> @@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct domain *d,
> panic("Failed to assign requested static memory for direct-map domain %pd.",
> d);
> }
> +
> +#ifdef CONFIG_STATIC_SHM
> +/*
> + * This function checks whether the static shared memory region is
> + * already allocated to dom_io.
> + */
> +static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> +{
> + struct page_info *page;
> +
> + page = maddr_to_page(pbase);
> + ASSERT(page);
maddr_to_page() can never return NULL. If you want to check a page will
be valid, then you should use mfn_valid().
However, the ASSERT() implies that the address was suitably checked
before. But I can't find such check.
> +
> + if ( page_get_owner(page) == NULL )
> + return false;
> +
> + ASSERT(page_get_owner(page) == dom_io);
Could this be hit because of a wrong device-tree? If yes, then this
should not be an ASSERT() (they are not suitable to check user input).
> + return true;
> +}
> +
> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> + u32 addr_cells, u32 size_cells,
> + paddr_t *pbase, paddr_t *psize)
There is something that doesn't add-up in this interface. The use of
pointer implies that pbase and psize may be modified by the function. So...
> +{
> + /*
> + * Pages of statically shared memory shall be included
> + * in domain_tot_pages().
> + */
> + d->max_pages += PFN_DOWN(*psize);
... it sounds a bit strange to use psize here. If psize, can't be
modified than it should probably not be a pointer.
Also, where do you check that d->max_pages will not overflow?
> +
> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> + pbase, psize);
> +
> +}
> +
> +/*
> + * Func allocate_shared_memory is supposed to be only called
I am a bit concerned with the word "supposed". Are you implying that it
may be called by someone that is not the owner? If not, then it should
be "should".
Also NIT: Spell out completely "func". I.e "The function".
> + * from the owner.
I read from as "current should be the owner". But I guess this is not
what you mean here. Instead it looks like you mean "d" is the owner. So
I would write "d should be the owner of the shared area".
It would be good to have a check/ASSERT confirm this (assuming this is
easy to write).
> + */
> +static int __init allocate_shared_memory(struct domain *d,
> + u32 addr_cells, u32 size_cells,
> + paddr_t pbase, paddr_t psize)
> +{
> + mfn_t smfn;
> +
> + dprintk(XENLOG_INFO,
> + "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> + pbase, pbase + psize);
NIT: I would suggest to also print the domain. This could help to easily
figure out that 'd' wasn't the owner.
> +
> + smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
> + &psize);
> + if ( mfn_eq(smfn, INVALID_MFN) )
> + return -EINVAL;
> +
> + /*
> + * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated.
> + * It sees RAM 1:1 and we do not need to create P2M mapping for it
> + */
> + ASSERT(d == dom_io);
> + return 0;
> +}
> +
> +static int __init process_shm(struct domain *d,
> + const struct dt_device_node *node)
> +{
> + struct dt_device_node *shm_node;
> + int ret = 0;
> + const struct dt_property *prop;
> + const __be32 *cells;
> + u32 shm_id;
> + u32 addr_cells, size_cells;
> + paddr_t gbase, pbase, psize;
> +
> + dt_for_each_child_node(node, shm_node)
> + {
> + if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> + continue;
> +
> + if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
> + {
> + printk("Shared memory node does not provide \"xen,shm-id\" property.\n");
> + return -ENOENT;
> + }
> +
> + addr_cells = dt_n_addr_cells(shm_node);
> + size_cells = dt_n_size_cells(shm_node);
> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> + if ( !prop )
> + {
> + printk("Shared memory node does not provide \"xen,shared-mem\" property.\n");
> + return -ENOENT;
> + }
> + cells = (const __be32 *)prop->value;
> + /* xen,shared-mem = <pbase, psize, gbase>; */
> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
See above about what ASSERT()s are for.
> + gbase = dt_read_number(cells, addr_cells);
> +
> + /* TODO: Consider owner domain is not the default dom_io. */
> + /*
> + * Per static shared memory region could be shared between multiple
> + * domains.
> + * In case re-allocating the same shared memory region, we check
> + * if it is already allocated to the default owner dom_io before
> + * the actual allocation.
> + */
> + if ( !is_shm_allocated_to_domio(pbase) )
> + {
> + /* Allocate statically shared pages to the default owner dom_io. */
> + ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> + pbase, psize);
> + if ( ret )
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_STATIC_SHM */
> #else
> static void __init allocate_static_memory(struct domain *d,
> struct kernel_info *kinfo,
> @@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain *d,
> else
> assign_static_memory_11(d, &kinfo, node);
>
> +#ifdef CONFIG_STATIC_SHM
> + rc = process_shm(d, node);
> + if ( rc < 0 )
> + return rc;
> +#endif
> +
> /*
> * Base address and irq number are needed when creating vpl011 device
> * tree node in prepare_dtb_domU, so initialization on related variables
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 7570eae91a..7070f5a9b9 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] 38+ messages in thread
* Re: [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain
2022-06-20 5:11 ` [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
@ 2022-06-24 19:07 ` Julien Grall
2022-06-29 7:49 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-24 19:07 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 20/06/2022 06:11, Penny Zheng wrote:
> 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>
> ---
> 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 91a5ace851..d4fd64e2bd 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -805,9 +805,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,
> "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> @@ -822,8 +824,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));
Coding style: this line is over 80 characters. And...
> + if ( ret )
> + {
> + printk(XENLOG_ERR
> + "Failed to map shared memory to %pd.\n", d);
... this line could be merged with the previous one.
> + return ret;
> + }
> + }
> +
> + return ret;
> }
>
> static int __init process_shm(struct domain *d,
> @@ -836,6 +848,8 @@ static int __init process_shm(struct domain *d,
> u32 shm_id;
> u32 addr_cells, size_cells;
> paddr_t gbase, pbase, psize;
> + const char *role_str;
> + bool owner_dom_io = true;
I think it would be best if role_str and owner_dom_io are defined within
the loop. Same goes for all the other declarations.
>
> dt_for_each_child_node(node, shm_node)
> {
> @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
> ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
> gbase = dt_read_number(cells, addr_cells);
>
> - /* TODO: Consider owner domain is not the default dom_io. */
> + /*
> + * "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
IIUC, the role is per-region. However, owner_dom_io is first initialized
to false outside the loop. Therefore, the variable may not be correct on
the next region.
So I think you want to write:
owner_dom_io = !dt_property_read_string(...);
This can also be avoided if you reduce the scope of the variable (it is
meant to only be used in the loop).
> +
> /*
> * 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;
> }
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 4/8] xen/arm: introduce put_page_nr and get_page_nr
2022-06-20 5:11 ` [PATCH v5 4/8] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
@ 2022-06-24 19:10 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-06-24 19:10 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 20/06/2022 06:11, Penny Zheng wrote:
> 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>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated
2022-06-20 5:11 ` [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
@ 2022-06-24 19:18 ` Julien Grall
2022-06-29 8:00 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-24 19:18 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 20/06/2022 06:11, 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>
> ---
> 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 | 62 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d4fd64e2bd..650d18f5ef 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -799,6 +799,34 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>
> }
>
> +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;
> +
> + if ( d == dom_io )
> + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain;
> + else
> + /* Exclude the owner domain itself. */
NIT: I think this comment wants to be just above the 'if' and expanded
to explain why the "dom_io" is not included. AFAIU, this is because
"dom_io" is not described in the Device-Tree, so it would not be taken
into account for nr_shm_domain.
> + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1;
TBH, given the use here. I would have consider to not increment
nr_shm_domain if the role was owner in parsing code. This is v5 now, so
I would be OK with the comment above.
But I would suggest to consider it as a follow-up.
> +
> + return 0;
> +}
> +
> /*
> * Func allocate_shared_memory is supposed to be only called
> * from the owner.
> @@ -810,6 +838,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,
> "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> @@ -824,6 +854,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, PFN_DOWN(psize));
> @@ -835,6 +866,37 @@ static int __init allocate_shared_memory(struct domain *d,
> }
> }
>
> + /*
> + * Get the right amount of references per page, which is the number of
> + * borrow domains.
> + */
> + ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> + if ( ret )
> + return ret;
> +
> + /*
> + * Instead of let borrower domain get a page ref, we add as many
Typo: s/let/letting/
> + * additional reference as the number of borrowers when the owner
> + * is allocated, since there is a chance that owner is created
> + * after borrower.
What if the borrower is created first? Wouldn't this result to add pages
in the P2M without reference?
If yes, then I think this is worth an explaination.
> + */
> + page = mfn_to_page(smfn);
Where do you validate the range [smfn, nr_pages]?
> + 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;
> }
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-20 5:11 ` [PATCH v5 1/8] xen/arm: introduce static shared memory Penny Zheng
2022-06-24 17:55 ` Julien Grall
@ 2022-06-24 19:25 ` Julien Grall
2022-06-29 8:40 ` Penny Zheng
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-24 19:25 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
I have looked at the code and I have further questions about the binding.
On 20/06/2022 06:11, Penny Zheng wrote:
> ---
> docs/misc/arm/device-tree/booting.txt | 120 ++++++++++++++++++++++++++
> xen/arch/arm/Kconfig | 6 ++
> xen/arch/arm/bootfdt.c | 68 +++++++++++++++
> xen/arch/arm/include/asm/setup.h | 3 +
> 4 files changed, 197 insertions(+)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 98253414b8..6467bc5a28 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -378,3 +378,123 @@ device-tree:
>
> This will reserve a 512MB region starting at the host physical address
> 0x30000000 to be exclusively used by DomU1.
> +
> +Static Shared Memory
> +====================
> +
> +The static shared memory device tree nodes allow users to statically set up
> +shared memory on dom0less system, enabling domains to do shm-based
> +communication.
> +
> +- compatible
> +
> + "xen,domain-shared-memory-v1"
> +
> +- xen,shm-id
> +
> + An 8-bit integer that represents the unique identifier of the shared memory
> + region. The maximum identifier shall be "xen,shm-id = <0xff>".
There is nothing in Xen that will ensure that xen,shm-id will match for
all the nodes using the same region.
I see you write it to the guest device-tree. However there is a mismatch
of the type: here you use an integer whereas the guest binding is using
a string.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-06-20 5:11 ` [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree Penny Zheng
@ 2022-06-24 19:30 ` Julien Grall
2022-06-24 21:56 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-24 19:30 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 20/06/2022 06:11, Penny Zheng wrote:
> 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>
> ---
> 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 | 143 +++++++++++++++++++++++++++++-
> xen/arch/arm/include/asm/kernel.h | 1 +
> xen/arch/arm/include/asm/setup.h | 1 +
> 3 files changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1584e6c2ce..4d62440a0e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -900,7 +900,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,
> + u32 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;
> + 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;
> @@ -971,6 +986,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;
> @@ -1301,6 +1324,117 @@ 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,
> + struct meminfo *mem)
NIT: AFAICT mem is not changed, so it should be const.
> +{
> + unsigned long i = 0;
NIT: This should be "unsigned int" to match the type of nr_banks.
> + 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;
> + uint8_t shm_id = mem->bank[i].shm_id;
> + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> + char buf[27];
> + const char compat[] = "xen,shared-memory-v1";
> + __be32 reg[4];
> + __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 %lu: %#"PRIx64"->%#"PRIx64"\n",
> + i, start, start + size);
> +
> + res = fdt_property_cell(fdt, "xen,id", shm_id);
Looking at the Linux binding, "xen,id" is meant to be a string. But here
you are writing it as an integer.
Given that the Linux binding is already merged, I think the Xen binding
should be changed.
> + 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;
> @@ -3078,6 +3212,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
> @@ -3454,7 +3593,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;
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 5063e5d077..7497cc40aa 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -29,6 +29,7 @@ struct membank {
> bool xen_domain; /* whether the memory bank is bound to a Xen domain. */
> #ifdef CONFIG_STATIC_SHM
> unsigned int nr_shm_domain;
> + uint8_t shm_id ; /* ID identifier of a static shared memory bank. */
I am not entirely happy that we are defining shm_id for everyone. We are
at v5, so I am OK for now.
But I would at least like "shm_id" to be defined before nr_shm_domain so
we re-use the existing hole and avoid increasing the size of the structure.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-06-24 19:30 ` Julien Grall
@ 2022-06-24 21:56 ` Stefano Stabellini
2022-07-04 7:45 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2022-06-24 21:56 UTC (permalink / raw)
To: Julien Grall
Cc: Penny Zheng, xen-devel, wei.chen, Stefano Stabellini,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 24 Jun 2022, Julien Grall wrote:
> On 20/06/2022 06:11, Penny Zheng wrote:
> > 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>
> > ---
> > 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 | 143 +++++++++++++++++++++++++++++-
> > xen/arch/arm/include/asm/kernel.h | 1 +
> > xen/arch/arm/include/asm/setup.h | 1 +
> > 3 files changed, 143 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 1584e6c2ce..4d62440a0e 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -900,7 +900,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,
> > + u32 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;
> > + 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;
> > @@ -971,6 +986,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;
> > @@ -1301,6 +1324,117 @@ 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,
> > + struct meminfo *mem)
>
> NIT: AFAICT mem is not changed, so it should be const.
>
> > +{
> > + unsigned long i = 0;
>
> NIT: This should be "unsigned int" to match the type of nr_banks.
>
> > + 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;
> > + uint8_t shm_id = mem->bank[i].shm_id;
> > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > + char buf[27];
> > + const char compat[] = "xen,shared-memory-v1";
> > + __be32 reg[4];
> > + __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 %lu: %#"PRIx64"->%#"PRIx64"\n",
> > + i, start, start + size);
> > +
> > + res = fdt_property_cell(fdt, "xen,id", shm_id);
>
> Looking at the Linux binding, "xen,id" is meant to be a string. But here you
> are writing it as an integer.
Good catch!
> Given that the Linux binding is already merged, I think the Xen binding should
> be changed.
We would be compliant with both bindings (xen and linux) just by writing
shm_id as string here, but if it is not too difficult we might as well
harmonize the two bindings and also define xen,shm-id as a string.
On the Xen side, I would suggest to put a clear size limit so that the
string is easier to handle.
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-24 17:55 ` Julien Grall
@ 2022-06-29 5:38 ` Penny Zheng
2022-06-29 10:17 ` Julien Grall
2022-06-29 8:39 ` Penny Zheng
1 sibling, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-29 5:38 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 1:55 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
>
> Hi Penny,
>
> On 20/06/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This patch serie introduces a new feature: setting up static
>
> Typo: s/serie/series/
>
> > shared memory on a dom0less system, through device tree configuration.
> >
> > This commit parses shared memory node at boot-time, and reserve it in
> > bootinfo.reserved_mem to avoid other use.
> >
> > This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> > static-shm-related codes, and this option depends on static memory(
> > CONFIG_STATIC_MEMORY). That's because that later we want to reuse a
> > few helpers, guarded with CONFIG_STATIC_MEMORY, like
> > acquire_staticmem_pages, etc, on static shared memory.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 change:
> > - nit fix on doc
> > ---
> > v3 change:
> > - make nr_shm_domain unsigned int
> > ---
> > v2 change:
> > - document refinement
> > - remove bitmap and use the iteration to check
> > - add a new field nr_shm_domain to keep the number of shared domain
> > ---
> > docs/misc/arm/device-tree/booting.txt | 120
> ++++++++++++++++++++++++++
> > xen/arch/arm/Kconfig | 6 ++
> > xen/arch/arm/bootfdt.c | 68 +++++++++++++++
> > xen/arch/arm/include/asm/setup.h | 3 +
> > 4 files changed, 197 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 98253414b8..6467bc5a28 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -378,3 +378,123 @@ device-tree:
> >
> > This will reserve a 512MB region starting at the host physical address
> > 0x30000000 to be exclusively used by DomU1.
> > +
> > +Static Shared Memory
> > +====================
> > +
> > +The static shared memory device tree nodes allow users to statically
> > +set up shared memory on dom0less system, enabling domains to do
> > +shm-based communication.
> > +
> > +- compatible
> > +
> > + "xen,domain-shared-memory-v1"
> > +
> > +- xen,shm-id
> > +
> > + An 8-bit integer that represents the unique identifier of the shared
> memory
> > + region. The maximum identifier shall be "xen,shm-id = <0xff>".
> > +
> > +- xen,shared-mem
> > +
> > + An array takes a physical address, which is the base address of the
> > + shared memory region in host physical address space, a size, and a
> guest
> > + physical address, as the target address of the mapping. The number of
> cells
> > + for the host address (and size) is the same as the guest pseudo-physical
> > + address and they are inherited from the parent node.
>
> Sorry for jump in the discussion late. But as this is going to be a stable ABI, I
> would to make sure the interface is going to be easily extendable.
>
> AFAIU, with your proposal the host physical address is mandatory. I would
> expect that some user may want to share memory but don't care about the
> exact location in memory. So I think it would be good to make it optional in
> the binding.
>
> I think this wants to be done now because it would be difficult to change the
> binding afterwards (the host physical address is the first set of cells).
>
> The Xen doesn't need to handle the optional case.
>
Sure, I'll make "the host physical address" optional here, and right now, with no actual
code implementation. I'll make up it later in free time~
The user case you mentioned here is that we let xen to allocate an arbitrary static shared
memory region, so size and guest physical address are still mandatory, right?
> [...]
>
> > 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
>
> You also want to update SUPPORT.md.
>
Right.
> > + 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..38dcb05d5d 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -361,6 +361,70 @@ 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;
> > + const __be32 *cell;
> > + paddr_t paddr, size;
> > + struct meminfo *mem = &bootinfo.reserved_mem;
> > + unsigned long i;
>
> nr_banks is "unsigned int" so I think this should be "unsigned int" as well.
>
Right
> > +
> > + if ( address_cells < 1 || size_cells < 1 )
> > + {
> > + printk("fdt: invalid #address-cells or #size-cells for static shared
> memory node.\n");
> > + return -EINVAL;
> > + }
> > +
> > + prop = fdt_get_property(fdt, node, "xen,shared-mem", NULL);
> > + if ( !prop )
> > + return -ENOENT;
> > +
> > + /*
> > + * 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.
> > + */
> > + cell = (const __be32 *)prop->data;
> > + device_tree_get_reg(&cell, address_cells, size_cells, &paddr,
> > + &size);
>
> Please check the len of the property to confirm is it big enough to contain
> "paddr", "size", and "gaddr".
>
Sure, will do
> > + for ( i = 0; i < mem->nr_banks; i++ )
> > + {
> > + /*
> > + * A static shared memory region could be shared between multiple
> > + * domains.
> > + */
> > + if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> > + break;
> > + }
> > +
> > + if ( i == mem->nr_banks )
> > + {
> > + if ( i < NR_MEM_BANKS )
> > + {
> > + /* Static shared memory shall be reserved from any other use. */
> > + 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 domains, which later may be used to
> > + * calculate the number of the reference count.
> > + */
> > + mem->bank[i].nr_shm_domain++;
> > +
> > + 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 +450,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..5063e5d077 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -27,6 +27,9 @@ 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
> > + unsigned int nr_shm_domain;
> > +#endif
> > };
> >
> > struct meminfo {
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
2022-06-24 18:22 ` Julien Grall
@ 2022-06-29 7:13 ` Penny Zheng
2022-06-29 10:34 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-29 7:13 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 2:22 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
>
> Hi Penny,
>
> On 20/06/2022 06:11, 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.
>
> The document in patch #1 suggest the page will be shared with dom_shared.
> But here you say "DOMID_IO".
>
> Which one is correct?
>
I’ll fix the documentation, DOM_IO is the last call.
> >
> > 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>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > 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 | 132
> +++++++++++++++++++++++++++++++++++-
> > xen/common/domain.c | 3 +
> > 2 files changed, 134 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7ddd16c26d..91a5ace851 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -527,6 +527,10 @@ static bool __init
> append_static_memory_to_bank(struct domain *d,
> > return true;
> > }
> >
> > +/*
> > + * If cell is NULL, pbase and psize should hold valid values.
> > + * Otherwise, cell will be populated together with pbase and psize.
> > + */
> > static mfn_t __init acquire_static_memory_bank(struct domain *d,
> > const __be32 **cell,
> > u32 addr_cells, u32
> > size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
> acquire_static_memory_bank(struct domain *d,
> > mfn_t smfn;
> > int res;
> >
> > - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> > + if ( cell )
> > + device_tree_get_reg(cell, addr_cells, size_cells, pbase,
> > + psize);
>
> I think this is a bit of a hack. To me it sounds like this should be moved out to
> a separate helper. This will also make the interface of
> acquire_shared_memory_bank() less questionable (see below).
>
Ok, I'll try to not reuse acquire_static_memory_bank in
acquire_shared_memory_bank.
> As this is v5, I would be OK with a follow-up for this split. But this interface of
> acuiqre_shared_memory_bank() needs to change.
>
I'll try to fix it in next version.
> > ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> > PAGE_SIZE));
>
> In the context of your series, who is checking that both psize and pbase are
> suitably aligned?
>
Actually, the whole parsing process is redundant for the static shared memory.
I've already parsed it and checked it before in process_shm.
> > if ( PFN_DOWN(*psize) > UINT_MAX )
> > {
> > @@ -759,6 +764,125 @@ static void __init assign_static_memory_11(struct
> domain *d,
> > panic("Failed to assign requested static memory for direct-map
> domain %pd.",
> > d);
> > }
> > +
> > +#ifdef CONFIG_STATIC_SHM
> > +/*
> > + * This function checks whether the static shared memory region is
> > + * already allocated to dom_io.
> > + */
> > +static bool __init is_shm_allocated_to_domio(paddr_t pbase) {
> > + struct page_info *page;
> > +
> > + page = maddr_to_page(pbase);
> > + ASSERT(page);
>
> maddr_to_page() can never return NULL. If you want to check a page will be
> valid, then you should use mfn_valid().
>
> However, the ASSERT() implies that the address was suitably checked before.
> But I can't find such check.
>
> > +
> > + if ( page_get_owner(page) == NULL )
> > + return false;
> > +
> > + ASSERT(page_get_owner(page) == dom_io);
> Could this be hit because of a wrong device-tree? If yes, then this should not
> be an ASSERT() (they are not suitable to check user input).
>
Yes, it can happen, I'll change it to if-array to output the error.
> > + return true;
> > +}
> > +
> > +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> > + u32 addr_cells, u32 size_cells,
> > + paddr_t *pbase,
> > +paddr_t *psize)
>
> There is something that doesn't add-up in this interface. The use of pointer
> implies that pbase and psize may be modified by the function. So...
>
Just like you points out before, it's a bit hacky to use acquire_static_memory_bank,
and the pointer used here is also due to it. Internal parsing process of acquire_static_memory_bank
needs pointer to deliver the result.
I’ll rewrite acquire_shared_memory, and it will be like:
"
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
* in domain_tot_pages().
*/
nr_pfns = PFN_DOWN(psize);
if ( d->max_page + nr_pfns > UINT_MAX )
{
printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
d, psize);
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
}
"
> > +{
> > + /*
> > + * Pages of statically shared memory shall be included
> > + * in domain_tot_pages().
> > + */
> > + d->max_pages += PFN_DOWN(*psize);
>
> ... it sounds a bit strange to use psize here. If psize, can't be modified than it
> should probably not be a pointer.
>
> Also, where do you check that d->max_pages will not overflow?
>
I'll check the overflow as follows:
"
nr_pfns = PFN_DOWN(psize);
if ( d->max_page + nr_pfns > UINT_MAX )
{
printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
d, psize);
return INVALID_MFN;
}
d->max_pages += nr_pfns;
"
> > +
> > + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> > + pbase, psize);
> > +
> > +}
> > +
> > +/*
> > + * Func allocate_shared_memory is supposed to be only called
>
> I am a bit concerned with the word "supposed". Are you implying that it may
> be called by someone that is not the owner? If not, then it should be
> "should".
>
> Also NIT: Spell out completely "func". I.e "The function".
>
> > + * from the owner.
>
> I read from as "current should be the owner". But I guess this is not what you
> mean here. Instead it looks like you mean "d" is the owner. So I would write
> "d should be the owner of the shared area".
>
> It would be good to have a check/ASSERT confirm this (assuming this is easy
> to write).
>
The check is already in the calling path, I guess...
Only under certain circumstances, we could call allocate_shared_memory
> > + */
> > +static int __init allocate_shared_memory(struct domain *d,
> > + u32 addr_cells, u32 size_cells,
> > + paddr_t pbase, paddr_t
> > +psize) {
> > + mfn_t smfn;
> > +
> > + dprintk(XENLOG_INFO,
> > + "Allocate static shared memory BANK %#"PRIpaddr"-
> %#"PRIpaddr".\n",
> > + pbase, pbase + psize);
>
> NIT: I would suggest to also print the domain. This could help to easily figure
> out that 'd' wasn't the owner.
>
Sure
> > +
> > + smfn = acquire_shared_memory_bank(d, addr_cells, size_cells, &pbase,
> > + &psize);
> > + if ( mfn_eq(smfn, INVALID_MFN) )
> > + return -EINVAL;
> > +
> > + /*
> > + * DOMID_IO is the domain, like DOMID_XEN, that is not auto-
> translated.
> > + * It sees RAM 1:1 and we do not need to create P2M mapping for it
> > + */
> > + ASSERT(d == dom_io);
> > + return 0;
> > +}
> > +
> > +static int __init process_shm(struct domain *d,
> > + const struct dt_device_node *node) {
> > + struct dt_device_node *shm_node;
> > + int ret = 0;
> > + const struct dt_property *prop;
> > + const __be32 *cells;
> > + u32 shm_id;
> > + u32 addr_cells, size_cells;
> > + paddr_t gbase, pbase, psize;
> > +
> > + dt_for_each_child_node(node, shm_node)
> > + {
> > + if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-
> memory-v1") )
> > + continue;
> > +
> > + if ( !dt_property_read_u32(shm_node, "xen,shm-id", &shm_id) )
> > + {
> > + printk("Shared memory node does not provide \"xen,shm-id\"
> property.\n");
> > + return -ENOENT;
> > + }
> > +
> > + addr_cells = dt_n_addr_cells(shm_node);
> > + size_cells = dt_n_size_cells(shm_node);
> > + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> > + if ( !prop )
> > + {
> > + printk("Shared memory node does not provide \"xen,shared-
> mem\" property.\n");
> > + return -ENOENT;
> > + }
> > + cells = (const __be32 *)prop->value;
> > + /* xen,shared-mem = <pbase, psize, gbase>; */
> > + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> > + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> > + PAGE_SIZE));
>
> See above about what ASSERT()s are for.
>
Do you think address was suitably checked here, is it enough?
If it is enough, I'll modify above ASSERT() to mfn_valid()
> > + gbase = dt_read_number(cells, addr_cells);
> > +
> > + /* TODO: Consider owner domain is not the default dom_io. */
> > + /*
> > + * Per static shared memory region could be shared between multiple
> > + * domains.
> > + * In case re-allocating the same shared memory region, we check
> > + * if it is already allocated to the default owner dom_io before
> > + * the actual allocation.
> > + */
> > + if ( !is_shm_allocated_to_domio(pbase) )
> > + {
> > + /* Allocate statically shared pages to the default owner dom_io. */
> > + ret = allocate_shared_memory(dom_io, addr_cells, size_cells,
> > + pbase, psize);
> > + if ( ret )
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +#endif /* CONFIG_STATIC_SHM */
> > #else
> > static void __init allocate_static_memory(struct domain *d,
> > struct kernel_info *kinfo,
> > @@ -3236,6 +3360,12 @@ static int __init construct_domU(struct domain
> *d,
> > else
> > assign_static_memory_11(d, &kinfo, node);
> >
> > +#ifdef CONFIG_STATIC_SHM
> > + rc = process_shm(d, node);
> > + if ( rc < 0 )
> > + return rc;
> > +#endif
> > +
> > /*
> > * Base address and irq number are needed when creating vpl011
> device
> > * tree node in prepare_dtb_domU, so initialization on related
> > variables diff --git a/xen/common/domain.c b/xen/common/domain.c
> index
> > 7570eae91a..7070f5a9b9 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] 38+ messages in thread
* RE: [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain
2022-06-24 19:07 ` Julien Grall
@ 2022-06-29 7:49 ` Penny Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-29 7:49 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 3:07 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 3/8] xen/arm: allocate static shared memory to a
> specific owner domain
>
> Hi Penny,
>
> On 20/06/2022 06:11, Penny Zheng wrote:
> > 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>
> > ---
> > 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 91a5ace851..d4fd64e2bd 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -805,9 +805,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,
> > "Allocate static shared memory BANK
> > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -822,8 +824,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));
>
> Coding style: this line is over 80 characters. And...
>
> > + if ( ret )
> > + {
> > + printk(XENLOG_ERR
> > + "Failed to map shared memory to %pd.\n", d);
>
> ... this line could be merged with the previous one.
>
> > + return ret;
> > + }
> > + }
> > +
> > + return ret;
> > }
> >
> > static int __init process_shm(struct domain *d, @@ -836,6 +848,8 @@
> > static int __init process_shm(struct domain *d,
> > u32 shm_id;
> > u32 addr_cells, size_cells;
> > paddr_t gbase, pbase, psize;
> > + const char *role_str;
> > + bool owner_dom_io = true;
>
> I think it would be best if role_str and owner_dom_io are defined within the
> loop. Same goes for all the other declarations.
>
> >
> > dt_for_each_child_node(node, shm_node)
> > {
> > @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d,
> > ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> PAGE_SIZE));
> > gbase = dt_read_number(cells, addr_cells);
> >
> > - /* TODO: Consider owner domain is not the default dom_io. */
> > + /*
> > + * "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
> IIUC, the role is per-region. However, owner_dom_io is first initialized to
> false outside the loop. Therefore, the variable may not be correct on the next
> region.
>
> So I think you want to write:
>
> owner_dom_io = !dt_property_read_string(...);
>
> This can also be avoided if you reduce the scope of the variable (it is meant
> to only be used in the loop).
>
Yes, it is a bug, thx!!! I'll reduce the scope.
> > +
> > /*
> > * 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;
> > }
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated
2022-06-24 19:18 ` Julien Grall
@ 2022-06-29 8:00 ` Penny Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-29 8:00 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 3:18 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 5/8] xen/arm: Add additional reference to owner
> domain when the owner is allocated
>
> Hi Penny,
>
> On 20/06/2022 06:11, 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>
> > ---
> > 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 | 62
> +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 62 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d4fd64e2bd..650d18f5ef 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -799,6 +799,34 @@ static mfn_t __init
> > acquire_shared_memory_bank(struct domain *d,
> >
> > }
> >
> > +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;
> > +
> > + if ( d == dom_io )
> > + *nr_borrowers =
> bootinfo.reserved_mem.bank[bank].nr_shm_domain;
> > + else
> > + /* Exclude the owner domain itself. */
> NIT: I think this comment wants to be just above the 'if' and expanded to
> explain why the "dom_io" is not included. AFAIU, this is because "dom_io" is
> not described in the Device-Tree, so it would not be taken into account for
> nr_shm_domain.
>
> > + *nr_borrowers =
> > + bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1;
>
> TBH, given the use here. I would have consider to not increment
> nr_shm_domain if the role was owner in parsing code. This is v5 now, so I
> would be OK with the comment above.
>
> But I would suggest to consider it as a follow-up.
>
LTM, it is not a big change, I'll try to include it in the next serie~
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Func allocate_shared_memory is supposed to be only called
> > * from the owner.
> > @@ -810,6 +838,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,
> > "Allocate static shared memory BANK
> > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -824,6 +854,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,
> > PFN_DOWN(psize)); @@ -835,6 +866,37 @@ static int __init
> allocate_shared_memory(struct domain *d,
> > }
> > }
> >
> > + /*
> > + * Get the right amount of references per page, which is the number of
> > + * borrow domains.
> > + */
> > + ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> > + if ( ret )
> > + return ret;
> > +
> > + /*
> > + * Instead of let borrower domain get a page ref, we add as many
>
> Typo: s/let/letting/
>
> > + * additional reference as the number of borrowers when the owner
> > + * is allocated, since there is a chance that owner is created
> > + * after borrower.
>
> What if the borrower is created first? Wouldn't this result to add pages in the
> P2M without reference?
>
> If yes, then I think this is worth an explaination.
>
Yes, it is intended to be the way you said, and I'll add a comment to explain.
> > + */
> > + page = mfn_to_page(smfn);
>
> Where do you validate the range [smfn, nr_pages]?
>
> > + 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;
> > }
> >
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-24 17:55 ` Julien Grall
2022-06-29 5:38 ` Penny Zheng
@ 2022-06-29 8:39 ` Penny Zheng
2022-07-15 18:10 ` Julien Grall
1 sibling, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-06-29 8:39 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 1:55 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
>
> Hi Penny,
>
> On 20/06/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This patch serie introduces a new feature: setting up static
>
> Typo: s/serie/series/
>
> > shared memory on a dom0less system, through device tree configuration.
> >
> > This commit parses shared memory node at boot-time, and reserve it in
> > bootinfo.reserved_mem to avoid other use.
> >
> > This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> > static-shm-related codes, and this option depends on static memory(
> > CONFIG_STATIC_MEMORY). That's because that later we want to reuse a
> > few helpers, guarded with CONFIG_STATIC_MEMORY, like
> > acquire_staticmem_pages, etc, on static shared memory.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 change:
> > - nit fix on doc
> > ---
> > v3 change:
> > - make nr_shm_domain unsigned int
> > ---
> > v2 change:
> > - document refinement
> > - remove bitmap and use the iteration to check
> > - add a new field nr_shm_domain to keep the number of shared domain
> > ---
> > docs/misc/arm/device-tree/booting.txt | 120
> ++++++++++++++++++++++++++
> > xen/arch/arm/Kconfig | 6 ++
> > xen/arch/arm/bootfdt.c | 68 +++++++++++++++
> > xen/arch/arm/include/asm/setup.h | 3 +
> > 4 files changed, 197 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 98253414b8..6467bc5a28 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -378,3 +378,123 @@ device-tree:
> >
> > This will reserve a 512MB region starting at the host physical address
> > 0x30000000 to be exclusively used by DomU1.
> > +
> > +Static Shared Memory
> > +====================
> > +
> > +The static shared memory device tree nodes allow users to statically
> > +set up shared memory on dom0less system, enabling domains to do
> > +shm-based communication.
> > +
> > +- compatible
> > +
> > + "xen,domain-shared-memory-v1"
> > +
> > +- xen,shm-id
> > +
> > + An 8-bit integer that represents the unique identifier of the shared
> memory
> > + region. The maximum identifier shall be "xen,shm-id = <0xff>".
> > +
> > +- xen,shared-mem
> > +
> > + An array takes a physical address, which is the base address of the
> > + shared memory region in host physical address space, a size, and a
> guest
> > + physical address, as the target address of the mapping. The number of
> cells
> > + for the host address (and size) is the same as the guest pseudo-physical
> > + address and they are inherited from the parent node.
>
> Sorry for jump in the discussion late. But as this is going to be a stable ABI, I
> would to make sure the interface is going to be easily extendable.
>
> AFAIU, with your proposal the host physical address is mandatory. I would
> expect that some user may want to share memory but don't care about the
> exact location in memory. So I think it would be good to make it optional in
> the binding.
>
> I think this wants to be done now because it would be difficult to change the
> binding afterwards (the host physical address is the first set of cells).
>
> The Xen doesn't need to handle the optional case.
>
> [...]
>
> > 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
>
> You also want to update SUPPORT.md.
>
> > + 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..38dcb05d5d 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -361,6 +361,70 @@ 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;
> > + const __be32 *cell;
> > + paddr_t paddr, size;
> > + struct meminfo *mem = &bootinfo.reserved_mem;
> > + unsigned long i;
>
> nr_banks is "unsigned int" so I think this should be "unsigned int" as well.
>
> > +
> > + if ( address_cells < 1 || size_cells < 1 )
> > + {
> > + printk("fdt: invalid #address-cells or #size-cells for static shared
> memory node.\n");
> > + return -EINVAL;
> > + }
> > +
> > + prop = fdt_get_property(fdt, node, "xen,shared-mem", NULL);
> > + if ( !prop )
> > + return -ENOENT;
> > +
> > + /*
> > + * 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.
> > + */
> > + cell = (const __be32 *)prop->data;
> > + device_tree_get_reg(&cell, address_cells, size_cells, &paddr,
> > + &size);
>
> Please check the len of the property to confirm is it big enough to contain
> "paddr", "size", and "gaddr".
>
> > + for ( i = 0; i < mem->nr_banks; i++ )
> > + {
> > + /*
> > + * A static shared memory region could be shared between multiple
> > + * domains.
> > + */
> > + if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> > + break;
Maybe I need to add a check on shm-id:
"
/*
* A static shared memory region could be shared between multiple
* domains.
*/
if ( strcmp(shm_id, mem->bank[i].shm_id) == 0 )
{
if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
break;
else
{
printk("Warning: xen,shm-id %s does not match for all the nodes using the same region.\n",
shm_id);
return -EINVAL;
}
}
"
Wdyt？
> > + }
> > +
> > + if ( i == mem->nr_banks )
> > + {
> > + if ( i < NRshm_MEM_BANKS )
> > + {
> > + /* Static shared memory shall be reserved from any other use. */
> > + 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 domains, which later may be used to
> > + * calculate the number of the reference count.
> > + */
> > + mem->bank[i].nr_shm_domain++;
> > +
> > + 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 +450,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..5063e5d077 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -27,6 +27,9 @@ 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
> > + unsigned int nr_shm_domain;
> > +#endif
> > };
> >
> > struct meminfo {
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-24 19:25 ` Julien Grall
@ 2022-06-29 8:40 ` Penny Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-06-29 8:40 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 3:26 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
>
> Hi Penny,
>
> I have looked at the code and I have further questions about the binding.
>
> On 20/06/2022 06:11, Penny Zheng wrote:
> > ---
> > docs/misc/arm/device-tree/booting.txt | 120
> ++++++++++++++++++++++++++
> > xen/arch/arm/Kconfig | 6 ++
> > xen/arch/arm/bootfdt.c | 68 +++++++++++++++
> > xen/arch/arm/include/asm/setup.h | 3 +
> > 4 files changed, 197 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 98253414b8..6467bc5a28 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -378,3 +378,123 @@ device-tree:
> >
> > This will reserve a 512MB region starting at the host physical address
> > 0x30000000 to be exclusively used by DomU1.
> > +
> > +Static Shared Memory
> > +====================
> > +
> > +The static shared memory device tree nodes allow users to statically
> > +set up shared memory on dom0less system, enabling domains to do
> > +shm-based communication.
> > +
> > +- compatible
> > +
> > + "xen,domain-shared-memory-v1"
> > +
> > +- xen,shm-id
> > +
> > + An 8-bit integer that represents the unique identifier of the shared
> memory
> > + region. The maximum identifier shall be "xen,shm-id = <0xff>".
>
> There is nothing in Xen that will ensure that xen,shm-id will match for all the
> nodes using the same region.
>
True, we actually do not use this field, adding it here to just be aligned with Linux.
I could add a check in the very beginning when we parse the device tree.
I'll give more details to explain in which code locates.
> I see you write it to the guest device-tree. However there is a mismatch of the
> type: here you use an integer whereas the guest binding is using a string.
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-29 5:38 ` Penny Zheng
@ 2022-06-29 10:17 ` Julien Grall
2022-07-13 2:42 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-29 10:17 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 29/06/2022 06:38, Penny Zheng wrote:
> Hi Julien
Hi Penny,
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Saturday, June 25, 2022 1:55 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
>>
>> Hi Penny,
>>
>> On 20/06/2022 06:11, Penny Zheng wrote:
>>> From: Penny Zheng <penny.zheng@arm.com>
>>>
>>> This patch serie introduces a new feature: setting up static
>>
>> Typo: s/serie/series/
>>
>>> shared memory on a dom0less system, through device tree configuration.
>>>
>>> This commit parses shared memory node at boot-time, and reserve it in
>>> bootinfo.reserved_mem to avoid other use.
>>>
>>> This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
>>> static-shm-related codes, and this option depends on static memory(
>>> CONFIG_STATIC_MEMORY). That's because that later we want to reuse a
>>> few helpers, guarded with CONFIG_STATIC_MEMORY, like
>>> acquire_staticmem_pages, etc, on static shared memory.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>> v5 change:
>>> - no change
>>> ---
>>> v4 change:
>>> - nit fix on doc
>>> ---
>>> v3 change:
>>> - make nr_shm_domain unsigned int
>>> ---
>>> v2 change:
>>> - document refinement
>>> - remove bitmap and use the iteration to check
>>> - add a new field nr_shm_domain to keep the number of shared domain
>>> ---
>>> docs/misc/arm/device-tree/booting.txt | 120
>> ++++++++++++++++++++++++++
>>> xen/arch/arm/Kconfig | 6 ++
>>> xen/arch/arm/bootfdt.c | 68 +++++++++++++++
>>> xen/arch/arm/include/asm/setup.h | 3 +
>>> 4 files changed, 197 insertions(+)
>>>
>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>> b/docs/misc/arm/device-tree/booting.txt
>>> index 98253414b8..6467bc5a28 100644
>>> --- a/docs/misc/arm/device-tree/booting.txt
>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>> @@ -378,3 +378,123 @@ device-tree:
>>>
>>> This will reserve a 512MB region starting at the host physical address
>>> 0x30000000 to be exclusively used by DomU1.
>>> +
>>> +Static Shared Memory
>>> +====================
>>> +
>>> +The static shared memory device tree nodes allow users to statically
>>> +set up shared memory on dom0less system, enabling domains to do
>>> +shm-based communication.
>>> +
>>> +- compatible
>>> +
>>> + "xen,domain-shared-memory-v1"
>>> +
>>> +- xen,shm-id
>>> +
>>> + An 8-bit integer that represents the unique identifier of the shared
>> memory
>>> + region. The maximum identifier shall be "xen,shm-id = <0xff>".
>>> +
>>> +- xen,shared-mem
>>> +
>>> + An array takes a physical address, which is the base address of the
>>> + shared memory region in host physical address space, a size, and a
>> guest
>>> + physical address, as the target address of the mapping. The number of
>> cells
>>> + for the host address (and size) is the same as the guest pseudo-physical
>>> + address and they are inherited from the parent node.
>>
>> Sorry for jump in the discussion late. But as this is going to be a stable ABI, I
>> would to make sure the interface is going to be easily extendable.
>>
>> AFAIU, with your proposal the host physical address is mandatory. I would
>> expect that some user may want to share memory but don't care about the
>> exact location in memory. So I think it would be good to make it optional in
>> the binding.
>>
>> I think this wants to be done now because it would be difficult to change the
>> binding afterwards (the host physical address is the first set of cells).
>>
>> The Xen doesn't need to handle the optional case.
>>
>
> Sure, I'll make "the host physical address" optional here, and right now, with no actual
> code implementation. I'll make up it later in free time~
>
> The user case you mentioned here is that we let xen to allocate an arbitrary static shared
> memory region, so size and guest physical address are still mandatory, right?
That's correct.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
2022-06-29 7:13 ` Penny Zheng
@ 2022-06-29 10:34 ` Julien Grall
2022-07-04 7:20 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-06-29 10:34 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
On 29/06/2022 08:13, Penny Zheng wrote:
> Hi Julien
Hi Penny,
>
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Saturday, June 25, 2022 2:22 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
>> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
>> default owner dom_io
>>
>> Hi Penny,
>>
>> On 20/06/2022 06:11, 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.
>>
>> The document in patch #1 suggest the page will be shared with dom_shared.
>> But here you say "DOMID_IO".
>>
>> Which one is correct?
>>
>
> I’ll fix the documentation, DOM_IO is the last call.
>
>>>
>>> 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>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>> ---
>>> 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 | 132
>> +++++++++++++++++++++++++++++++++++-
>>> xen/common/domain.c | 3 +
>>> 2 files changed, 134 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 7ddd16c26d..91a5ace851 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -527,6 +527,10 @@ static bool __init
>> append_static_memory_to_bank(struct domain *d,
>>> return true;
>>> }
>>>
>>> +/*
>>> + * If cell is NULL, pbase and psize should hold valid values.
>>> + * Otherwise, cell will be populated together with pbase and psize.
>>> + */
>>> static mfn_t __init acquire_static_memory_bank(struct domain *d,
>>> const __be32 **cell,
>>> u32 addr_cells, u32
>>> size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
>> acquire_static_memory_bank(struct domain *d,
>>> mfn_t smfn;
>>> int res;
>>>
>>> - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
>>> + if ( cell )
>>> + device_tree_get_reg(cell, addr_cells, size_cells, pbase,
>>> + psize);
>>
>> I think this is a bit of a hack. To me it sounds like this should be moved out to
>> a separate helper. This will also make the interface of
>> acquire_shared_memory_bank() less questionable (see below).
>>
>
> Ok, I'll try to not reuse acquire_static_memory_bank in
> acquire_shared_memory_bank.
I am OK with that so long it doesn't involve too much duplication.
>>> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
>>> PAGE_SIZE));
>>
>> In the context of your series, who is checking that both psize and pbase are
>> suitably aligned?
>>
>
> Actually, the whole parsing process is redundant for the static shared memory.
> I've already parsed it and checked it before in process_shm.
I looked at process_shm() and couldn't find any code that would check
pbase and psize are suitable aligned (ASSERT() doesn't count).
>
>>> + return true;
>>> +}
>>> +
>>> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>>> + u32 addr_cells, u32 size_cells,
>>> + paddr_t *pbase,
>>> +paddr_t *psize)
>>
>> There is something that doesn't add-up in this interface. The use of pointer
>> implies that pbase and psize may be modified by the function. So...
>>
>
> Just like you points out before, it's a bit hacky to use acquire_static_memory_bank,
> and the pointer used here is also due to it. Internal parsing process of acquire_static_memory_bank
> needs pointer to deliver the result.
>
> I’ll rewrite acquire_shared_memory, and it will be like:
> "
> 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
> * in domain_tot_pages().
> */
> nr_pfns = PFN_DOWN(psize);
> if ( d->max_page + nr_pfns > UINT_MAX )
On Arm32, this check would always be true a 32-bit unsigned value is
always below UINT_MAX. On arm64, you might get away because nr_pfns is
unsigned long (so I think the type promotion works). But this is fragile.
I would suggest to use the following check:
(UINT_MAX - d->max_page) < nr_pfns
> {
> printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> d, psize);
> 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
> }
> "
>
>>> +{
>>> + /*
>>> + * Pages of statically shared memory shall be included
>>> + * in domain_tot_pages().
>>> + */
>>> + d->max_pages += PFN_DOWN(*psize);
>>
>> ... it sounds a bit strange to use psize here. If psize, can't be modified than it
>> should probably not be a pointer.
>>
>> Also, where do you check that d->max_pages will not overflow?
>>
>
> I'll check the overflow as follows:
See above about the check.
> "
> nr_pfns = PFN_DOWN(psize);
> if ( d->max_page + nr_pfns > UINT_MAX )
> {
> printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> d, psize);
> return INVALID_MFN;
> }
> d->max_pages += nr_pfns;
> "
>
>>> +
>>> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
>>> + pbase, psize);
>>> +
>>> +}
>>> +
>>> +/*
>>> + * Func allocate_shared_memory is supposed to be only called
>>
>> I am a bit concerned with the word "supposed". Are you implying that it may
>> be called by someone that is not the owner? If not, then it should be
>> "should".
>>
>> Also NIT: Spell out completely "func". I.e "The function".
>>
>>> + * from the owner.
>>
>> I read from as "current should be the owner". But I guess this is not what you
>> mean here. Instead it looks like you mean "d" is the owner. So I would write
>> "d should be the owner of the shared area".
>>
>> It would be good to have a check/ASSERT confirm this (assuming this is easy
>> to write).
>>
>
> The check is already in the calling path, I guess...
Can you please confirm it?
[...]
>>> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
>>> + if ( !prop )
>>> + {
>>> + printk("Shared memory node does not provide \"xen,shared-
>> mem\" property.\n");
>>> + return -ENOENT;
>>> + }
>>> + cells = (const __be32 *)prop->value;
>>> + /* xen,shared-mem = <pbase, psize, gbase>; */
>>> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
>>> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
>>> + PAGE_SIZE));
>>
>> See above about what ASSERT()s are for.
>>
>
> Do you think address was suitably checked here, is it enough?
As I wrote before, ASSERT() should not be used to check user inputs.
They need to happen in both debug and production build.
> If it is enough, I'll modify above ASSERT() to mfn_valid()
It is not clear what ASSERT() you are referring to.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
2022-06-29 10:34 ` Julien Grall
@ 2022-07-04 7:20 ` Penny Zheng
2022-07-15 18:43 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-07-04 7:20 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, June 29, 2022 6:35 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the
> default owner dom_io
>
>
>
> On 29/06/2022 08:13, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
Hi Julien
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Saturday, June 25, 2022 2:22 AM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; George Dunlap
> >> <george.dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Wei Liu
> >> <wl@xen.org>
> >> Subject: Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to
> >> the default owner dom_io
> >>
> >> Hi Penny,
> >>
> >> On 20/06/2022 06:11, 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.
> >>
> >> The document in patch #1 suggest the page will be shared with
> dom_shared.
> >> But here you say "DOMID_IO".
> >>
> >> Which one is correct?
> >>
> >
> > I’ll fix the documentation, DOM_IO is the last call.
> >
> >>>
> >>> 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>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>> ---
> >>> 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 | 132
> >> +++++++++++++++++++++++++++++++++++-
> >>> xen/common/domain.c | 3 +
> >>> 2 files changed, 134 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c
> >>> b/xen/arch/arm/domain_build.c index 7ddd16c26d..91a5ace851 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -527,6 +527,10 @@ static bool __init
> >> append_static_memory_to_bank(struct domain *d,
> >>> return true;
> >>> }
> >>>
> >>> +/*
> >>> + * If cell is NULL, pbase and psize should hold valid values.
> >>> + * Otherwise, cell will be populated together with pbase and psize.
> >>> + */
> >>> static mfn_t __init acquire_static_memory_bank(struct domain *d,
> >>> const __be32 **cell,
> >>> u32 addr_cells,
> >>> u32 size_cells, @@ -535,7 +539,8 @@ static mfn_t __init
> >> acquire_static_memory_bank(struct domain *d,
> >>> mfn_t smfn;
> >>> int res;
> >>>
> >>> - device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> >>> + if ( cell )
> >>> + device_tree_get_reg(cell, addr_cells, size_cells, pbase,
> >>> + psize);
> >>
> >> I think this is a bit of a hack. To me it sounds like this should be
> >> moved out to a separate helper. This will also make the interface of
> >> acquire_shared_memory_bank() less questionable (see below).
> >>
> >
> > Ok, I'll try to not reuse acquire_static_memory_bank in
> > acquire_shared_memory_bank.
>
> I am OK with that so long it doesn't involve too much duplication.
>
> >>> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> >>> PAGE_SIZE));
> >>
> >> In the context of your series, who is checking that both psize and
> >> pbase are suitably aligned?
> >>
> >
> > Actually, the whole parsing process is redundant for the static shared
> memory.
> > I've already parsed it and checked it before in process_shm.
>
> I looked at process_shm() and couldn't find any code that would check pbase
> and psize are suitable aligned (ASSERT() doesn't count).
>
> >
> >>> + return true;
> >>> +}
> >>> +
> >>> +static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> >>> + u32 addr_cells, u32 size_cells,
> >>> + paddr_t *pbase,
> >>> +paddr_t *psize)
> >>
> >> There is something that doesn't add-up in this interface. The use of
> >> pointer implies that pbase and psize may be modified by the function. So...
> >>
> >
> > Just like you points out before, it's a bit hacky to use
> > acquire_static_memory_bank, and the pointer used here is also due to
> > it. Internal parsing process of acquire_static_memory_bank needs pointer
> to deliver the result.
> >
> > I’ll rewrite acquire_shared_memory, and it will be like:
> > "
> > 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
> > * in domain_tot_pages().
> > */
> > nr_pfns = PFN_DOWN(psize);
> > if ( d->max_page + nr_pfns > UINT_MAX )
>
> On Arm32, this check would always be true a 32-bit unsigned value is always
> below UINT_MAX. On arm64, you might get away because nr_pfns is
> unsigned long (so I think the type promotion works). But this is fragile.
>
> I would suggest to use the following check:
>
> (UINT_MAX - d->max_page) < nr_pfns
>
> > {
> > printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> > d, psize);
> > 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
> > }
> > "
> >
> >>> +{
> >>> + /*
> >>> + * Pages of statically shared memory shall be included
> >>> + * in domain_tot_pages().
> >>> + */
> >>> + d->max_pages += PFN_DOWN(*psize);
> >>
> >> ... it sounds a bit strange to use psize here. If psize, can't be
> >> modified than it should probably not be a pointer.
> >>
> >> Also, where do you check that d->max_pages will not overflow?
> >>
> >
> > I'll check the overflow as follows:
>
> See above about the check.
>
> > "
> > nr_pfns = PFN_DOWN(psize);
> > if ( d->max_page + nr_pfns > UINT_MAX )
> > {
> > printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages: %lu.\n",
> > d, psize);
> > return INVALID_MFN;
> > }
> > d->max_pages += nr_pfns;
> > "
> >
> >>> +
> >>> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> >>> + pbase, psize);
> >>> +
> >>> +}
> >>> +
> >>> +/*
> >>> + * Func allocate_shared_memory is supposed to be only called
> >>
> >> I am a bit concerned with the word "supposed". Are you implying that
> >> it may be called by someone that is not the owner? If not, then it
> >> should be "should".
> >>
> >> Also NIT: Spell out completely "func". I.e "The function".
> >>
> >>> + * from the owner.
> >>
> >> I read from as "current should be the owner". But I guess this is not
> >> what you mean here. Instead it looks like you mean "d" is the owner.
> >> So I would write "d should be the owner of the shared area".
> >>
> >> It would be good to have a check/ASSERT confirm this (assuming this
> >> is easy to write).
> >>
> >
> > The check is already in the calling path, I guess...
>
> Can you please confirm it?
>
I mean that allocate_shared_memory is only called under the following condition, and
it confirms it is the right owner.
"
if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
(!owner_dom_io && strcmp(role_str, "owner") == 0) )
{
/* 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);
"
TBH, apart from that, I don't know how to check if the input d is the right owner, or am I
misunderstanding some your suggestion here?
> [...]
>
> >>> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> >>> + if ( !prop )
> >>> + {
> >>> + printk("Shared memory node does not provide
> >>> + \"xen,shared-
> >> mem\" property.\n");
> >>> + return -ENOENT;
> >>> + }
> >>> + cells = (const __be32 *)prop->value;
> >>> + /* xen,shared-mem = <pbase, psize, gbase>; */
> >>> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> >>> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> >>> + PAGE_SIZE));
> >>
> >> See above about what ASSERT()s are for.
> >>
> >
> > Do you think address was suitably checked here, is it enough?
>
> As I wrote before, ASSERT() should not be used to check user inputs.
> They need to happen in both debug and production build.
>
> > If it is enough, I'll modify above ASSERT() to mfn_valid()
>
> It is not clear what ASSERT() you are referring to.
>
For whether page is aligned, I will add the below check:
"
if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(psize, PAGE_SIZE) ||
!IS_ALIGNED(gbase, PAGE_SIZE) )
{
printk("%pd: physical address %lu, size %lu or guest address %lu is not suitably aligned.\n",
d, pbase, psize, gbase);
return -EINVAL;
}
"
For whether the whole address range is valid, I will add the below check:
"
for ( i = 0; i < PFN_DOWN(psize); i++ )
if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
{
printk("%pd: invalid physical address %"PRI_paddr" or size %"PRI_paddr"\n",
d, pbase, psize);
return -EINVAL;
}
"
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-06-24 21:56 ` Stefano Stabellini
@ 2022-07-04 7:45 ` Penny Zheng
2022-07-05 8:09 ` Julien Grall
2022-07-06 23:52 ` Stefano Stabellini
0 siblings, 2 replies; 38+ messages in thread
From: Penny Zheng @ 2022-07-04 7:45 UTC (permalink / raw)
To: Stefano Stabellini, Julien Grall
Cc: xen-devel, Wei Chen, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano and Julien
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Saturday, June 25, 2022 5:57 AM
> To: Julien Grall <julien@xen.org>
> Cc: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest
> device tree
>
> On Fri, 24 Jun 2022, Julien Grall wrote:
> > On 20/06/2022 06:11, Penny Zheng wrote:
> > > 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>
> > > ---
> > > 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 | 143
> +++++++++++++++++++++++++++++-
> > > xen/arch/arm/include/asm/kernel.h | 1 +
> > > xen/arch/arm/include/asm/setup.h | 1 +
> > > 3 files changed, 143 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c
> > > b/xen/arch/arm/domain_build.c index 1584e6c2ce..4d62440a0e 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -900,7 +900,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,
> > > + u32 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;
> > > + 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; @@ -971,6 +986,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;
> > > @@ -1301,6 +1324,117 @@ 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,
> > > + struct meminfo *mem)
> >
> > NIT: AFAICT mem is not changed, so it should be const.
> >
> > > +{
> > > + unsigned long i = 0;
> >
> > NIT: This should be "unsigned int" to match the type of nr_banks.
> >
> > > + 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;
> > > + uint8_t shm_id = mem->bank[i].shm_id;
> > > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > > + char buf[27];
> > > + const char compat[] = "xen,shared-memory-v1";
> > > + __be32 reg[4];
> > > + __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 %lu: %#"PRIx64"->%#"PRIx64"\n",
> > > + i, start, start + size);
> > > +
> > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> >
> > Looking at the Linux binding, "xen,id" is meant to be a string. But
> > here you are writing it as an integer.
>
> Good catch!
>
>
> > Given that the Linux binding is already merged, I think the Xen
> > binding should be changed.
>
> We would be compliant with both bindings (xen and linux) just by writing
> shm_id as string here, but if it is not too difficult we might as well harmonize
> the two bindings and also define xen,shm-id as a string.
>
> On the Xen side, I would suggest to put a clear size limit so that the string is
> easier to handle.
I've already made the xen,shm-id parsed as a string too, seeing the below code:
"
prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
if ( !prop_id )
return -ENOENT;
shm_id = simple_strtoul(prop_id->data, NULL, 10);
if ( shm_id >= NR_MEM_BANKS )
{
printk("fdt: invalid `xen,shm-id` %lu for static shared memory node.\n",
shm_id);
return -EINVAL;
}
"
The size limit is smaller than 256, just as stated in doc:
"
- xen,shm-id
A string that represents the unique identifier of the shared memory
region. The maximum identifier shall be "xen,shm-id = 255".
"
Hope this fits what both of you suggested~~~
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-07-04 7:45 ` Penny Zheng
@ 2022-07-05 8:09 ` Julien Grall
2022-07-05 23:21 ` Stefano Stabellini
2022-07-06 23:52 ` Stefano Stabellini
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-07-05 8:09 UTC (permalink / raw)
To: Penny Zheng, Stefano Stabellini
Cc: xen-devel, Wei Chen, Bertrand Marquis, Volodymyr Babchuk
On 04/07/2022 08:45, Penny Zheng wrote:
> Hi Stefano and Julien
Hi Penny,
>> -----Original Message-----
>> From: Stefano Stabellini <sstabellini@kernel.org>
>>>> + res = fdt_property_cell(fdt, "xen,id", shm_id);
>>>
>>> Looking at the Linux binding, "xen,id" is meant to be a string. But
>>> here you are writing it as an integer.
>>
>> Good catch!
>>
>>
>>> Given that the Linux binding is already merged, I think the Xen
>>> binding should be changed.
>>
>> We would be compliant with both bindings (xen and linux) just by writing
>> shm_id as string here, but if it is not too difficult we might as well harmonize
>> the two bindings and also define xen,shm-id as a string.
>>
>> On the Xen side, I would suggest to put a clear size limit so that the string is
>> easier to handle.
>
> I've already made the xen,shm-id parsed as a string too, seeing the below code:
> "
> prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> if ( !prop_id )
> return -ENOENT;
> shm_id = simple_strtoul(prop_id->data, NULL, 10);
Why do you want to convert the string to a number?
> if ( shm_id >= NR_MEM_BANKS )
IIRC, you are not using "shm_id" to index any bank. So why do you want
to check against NR_MEM_BANKS?
> {
> printk("fdt: invalid `xen,shm-id` %lu for static shared memory node.\n",
> shm_id);
> return -EINVAL;
> }
> "
> The size limit is smaller than 256, just as stated in doc:
> "
> - xen,shm-id
>
> A string that represents the unique identifier of the shared memory
> region. The maximum identifier shall be "xen,shm-id = 255".
The first sentence reads as the xen,shm-id can a free form string. But
then the second sentence suggests a number (not a string).
In any case, it is still unclear why you want to convert the string to
an ID. From my understanding, Stefano was suggested a limit on the
characters rather than a limit on the number.
If the latter is desirable, then the documentation should be a bit
clearer and you need to validate the input provided by the user.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-07-05 8:09 ` Julien Grall
@ 2022-07-05 23:21 ` Stefano Stabellini
0 siblings, 0 replies; 38+ messages in thread
From: Stefano Stabellini @ 2022-07-05 23:21 UTC (permalink / raw)
To: Julien Grall
Cc: Penny Zheng, Stefano Stabellini, xen-devel, Wei Chen,
Bertrand Marquis, Volodymyr Babchuk
On Tue, 5 Jul 2022, Julien Grall wrote:
> On 04/07/2022 08:45, Penny Zheng wrote:
> > Hi Stefano and Julien
>
> Hi Penny,
>
> > > -----Original Message-----
> > > From: Stefano Stabellini <sstabellini@kernel.org>
> > > > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> > > >
> > > > Looking at the Linux binding, "xen,id" is meant to be a string. But
> > > > here you are writing it as an integer.
> > >
> > > Good catch!
> > >
> > >
> > > > Given that the Linux binding is already merged, I think the Xen
> > > > binding should be changed.
> > >
> > > We would be compliant with both bindings (xen and linux) just by writing
> > > shm_id as string here, but if it is not too difficult we might as well
> > > harmonize
> > > the two bindings and also define xen,shm-id as a string.
> > >
> > > On the Xen side, I would suggest to put a clear size limit so that the
> > > string is
> > > easier to handle.
> >
> > I've already made the xen,shm-id parsed as a string too, seeing the below
> > code:
> > "
> > prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> > if ( !prop_id )
> > return -ENOENT;
> > shm_id = simple_strtoul(prop_id->data, NULL, 10);
>
> Why do you want to convert the string to a number?
>
>
> > if ( shm_id >= NR_MEM_BANKS )
>
> IIRC, you are not using "shm_id" to index any bank. So why do you want to
> check against NR_MEM_BANKS?
>
> > {
> > printk("fdt: invalid `xen,shm-id` %lu for static shared memory
> > node.\n",
> > shm_id);
> > return -EINVAL;
> > }
> > "
> > The size limit is smaller than 256, just as stated in doc:
> > "
> > - xen,shm-id
> >
> > A string that represents the unique identifier of the shared memory
> > region. The maximum identifier shall be "xen,shm-id = 255".
>
> The first sentence reads as the xen,shm-id can a free form string. But then
> the second sentence suggests a number (not a string).
>
> In any case, it is still unclear why you want to convert the string to an ID.
> From my understanding, Stefano was suggested a limit on the characters rather
> than a limit on the number.
Just confirming that yes, I was suggesting a strict limit on the number
of characters, assuming we accept a freeform string.
I think a freeform string is more convenient and flexible for the user.
But it is not required: our only requirement is that the Linux device
tree Xen generates has "xen,id" in the form of a string, but that could
be a string representing a number, e.g. "255".
> If the latter is desirable, then the documentation should be a bit clearer and
> you need to validate the input provided by the user.
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-07-04 7:45 ` Penny Zheng
2022-07-05 8:09 ` Julien Grall
@ 2022-07-06 23:52 ` Stefano Stabellini
2022-07-07 4:01 ` Penny Zheng
1 sibling, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2022-07-06 23:52 UTC (permalink / raw)
To: Penny Zheng
Cc: Stefano Stabellini, Julien Grall, xen-devel, Wei Chen,
Bertrand Marquis, Volodymyr Babchuk
On Mon, 4 Jul 2022, Penny Zheng wrote:
> Hi Stefano and Julien
>
> > -----Original Message-----
> > From: Stefano Stabellini <sstabellini@kernel.org>
> > Sent: Saturday, June 25, 2022 5:57 AM
> > To: Julien Grall <julien@xen.org>
> > Cc: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> > Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > Subject: Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest
> > device tree
> >
> > On Fri, 24 Jun 2022, Julien Grall wrote:
> > > On 20/06/2022 06:11, Penny Zheng wrote:
> > > > 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>
> > > > ---
> > > > 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 | 143
> > +++++++++++++++++++++++++++++-
> > > > xen/arch/arm/include/asm/kernel.h | 1 +
> > > > xen/arch/arm/include/asm/setup.h | 1 +
> > > > 3 files changed, 143 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/domain_build.c
> > > > b/xen/arch/arm/domain_build.c index 1584e6c2ce..4d62440a0e 100644
> > > > --- a/xen/arch/arm/domain_build.c
> > > > +++ b/xen/arch/arm/domain_build.c
> > > > @@ -900,7 +900,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,
> > > > + u32 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;
> > > > + 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; @@ -971,6 +986,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;
> > > > @@ -1301,6 +1324,117 @@ 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,
> > > > + struct meminfo *mem)
> > >
> > > NIT: AFAICT mem is not changed, so it should be const.
> > >
> > > > +{
> > > > + unsigned long i = 0;
> > >
> > > NIT: This should be "unsigned int" to match the type of nr_banks.
> > >
> > > > + 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;
> > > > + uint8_t shm_id = mem->bank[i].shm_id;
> > > > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > > > + char buf[27];
> > > > + const char compat[] = "xen,shared-memory-v1";
> > > > + __be32 reg[4];
> > > > + __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 %lu: %#"PRIx64"->%#"PRIx64"\n",
> > > > + i, start, start + size);
> > > > +
> > > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> > >
> > > Looking at the Linux binding, "xen,id" is meant to be a string. But
> > > here you are writing it as an integer.
> >
> > Good catch!
> >
> >
> > > Given that the Linux binding is already merged, I think the Xen
> > > binding should be changed.
> >
> > We would be compliant with both bindings (xen and linux) just by writing
> > shm_id as string here, but if it is not too difficult we might as well harmonize
> > the two bindings and also define xen,shm-id as a string.
> >
> > On the Xen side, I would suggest to put a clear size limit so that the string is
> > easier to handle.
>
> I've already made the xen,shm-id parsed as a string too, seeing the below code:
> "
> prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> if ( !prop_id )
> return -ENOENT;
> shm_id = simple_strtoul(prop_id->data, NULL, 10);
> if ( shm_id >= NR_MEM_BANKS )
> {
> printk("fdt: invalid `xen,shm-id` %lu for static shared memory node.\n",
> shm_id);
> return -EINVAL;
> }
> "
> The size limit is smaller than 256, just as stated in doc:
> "
> - xen,shm-id
>
> A string that represents the unique identifier of the shared memory
> region. The maximum identifier shall be "xen,shm-id = 255".
> "
> Hope this fits what both of you suggested~~~
Yes. I think supporting arbitrary strings like "my-shared-mem-1" would
be nice-to-have but I wouldn't make it a hard requirement.
"255" as a string would match Linux's requirements for xen,id.
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-07-06 23:52 ` Stefano Stabellini
@ 2022-07-07 4:01 ` Penny Zheng
2022-07-08 16:40 ` Stefano Stabellini
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-07-07 4:01 UTC (permalink / raw)
To: Stefano Stabellini, julien
Cc: Julien Grall, xen-devel, Wei Chen, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano and julien
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Thursday, July 7, 2022 7:53 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>;
> xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; Bertrand
> Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest
> device tree
>
> On Mon, 4 Jul 2022, Penny Zheng wrote:
> > Hi Stefano and Julien
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini <sstabellini@kernel.org>
> > > Sent: Saturday, June 25, 2022 5:57 AM
> > > To: Julien Grall <julien@xen.org>
> > > Cc: Penny Zheng <Penny.Zheng@arm.com>;
> > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>;
> Stefano
> > > Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> > > <Volodymyr_Babchuk@epam.com>
> > > Subject: Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in
> > > guest device tree
> > >
> > > On Fri, 24 Jun 2022, Julien Grall wrote:
> > > > On 20/06/2022 06:11, Penny Zheng wrote:
> > > > > 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>
> > > > > ---
> > > > > 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 | 143
> > > +++++++++++++++++++++++++++++-
> > > > > xen/arch/arm/include/asm/kernel.h | 1 +
> > > > > xen/arch/arm/include/asm/setup.h | 1 +
> > > > > 3 files changed, 143 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/domain_build.c
> > > > > b/xen/arch/arm/domain_build.c index 1584e6c2ce..4d62440a0e
> > > > > 100644
> > > > > --- a/xen/arch/arm/domain_build.c
> > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > @@ -900,7 +900,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,
> > > > > + u32 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;
> > > > > + 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; @@ -971,6 +986,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;
> > > > > @@ -1301,6 +1324,117 @@ 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,
> > > > > + struct meminfo *mem)
> > > >
> > > > NIT: AFAICT mem is not changed, so it should be const.
> > > >
> > > > > +{
> > > > > + unsigned long i = 0;
> > > >
> > > > NIT: This should be "unsigned int" to match the type of nr_banks.
> > > >
> > > > > + 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;
> > > > > + uint8_t shm_id = mem->bank[i].shm_id;
> > > > > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > > > > + char buf[27];
> > > > > + const char compat[] = "xen,shared-memory-v1";
> > > > > + __be32 reg[4];
> > > > > + __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 %lu: %#"PRIx64"-
> >%#"PRIx64"\n",
> > > > > + i, start, start + size);
> > > > > +
> > > > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> > > >
> > > > Looking at the Linux binding, "xen,id" is meant to be a string.
> > > > But here you are writing it as an integer.
> > >
> > > Good catch!
> > >
> > >
> > > > Given that the Linux binding is already merged, I think the Xen
> > > > binding should be changed.
> > >
> > > We would be compliant with both bindings (xen and linux) just by
> > > writing shm_id as string here, but if it is not too difficult we
> > > might as well harmonize the two bindings and also define xen,shm-id as a
> string.
> > >
> > > On the Xen side, I would suggest to put a clear size limit so that
> > > the string is easier to handle.
> >
> > I've already made the xen,shm-id parsed as a string too, seeing the below
> code:
> > "
> > prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> > if ( !prop_id )
> > return -ENOENT;
> > shm_id = simple_strtoul(prop_id->data, NULL, 10);
> > if ( shm_id >= NR_MEM_BANKS )
> > {
> > printk("fdt: invalid `xen,shm-id` %lu for static shared memory node.\n",
> > shm_id);
> > return -EINVAL;
> > }
> > "
> > The size limit is smaller than 256, just as stated in doc:
> > "
> > - xen,shm-id
> >
> > A string that represents the unique identifier of the shared memory
> > region. The maximum identifier shall be "xen,shm-id = 255".
> > "
> > Hope this fits what both of you suggested~~~
>
> Yes. I think supporting arbitrary strings like "my-shared-mem-1" would be
> nice-to-have but I wouldn't make it a hard requirement.
>
Oh, the example "my-shared-mem-1" really expands my mind, I think I understand
what you and Julien referred as free form string, which shall not be limited to only
numeric number... thanks!!!
You were suggesting a strict limit on the number of characters, TBH, I have no clue
What the standard is here. Any suggestions?
If considering padding, maybe 19?
"
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[19];
unsigned int nr_shm_borrowers;
#endif
};
"
> "255" as a string would match Linux's requirements for xen,id.
I will use your example "my-shm-mem-1", I think its better for users
to understand, at least for me...
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-07-07 4:01 ` Penny Zheng
@ 2022-07-08 16:40 ` Stefano Stabellini
2022-07-11 7:59 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Stefano Stabellini @ 2022-07-08 16:40 UTC (permalink / raw)
To: Penny Zheng
Cc: Stefano Stabellini, julien, xen-devel, Wei Chen,
Bertrand Marquis, Volodymyr Babchuk
On Thu, 7 Jul 2022, Penny Zheng wrote:
> Hi Stefano and julien
>
> > -----Original Message-----
> > From: Stefano Stabellini <sstabellini@kernel.org>
> > Sent: Thursday, July 7, 2022 7:53 AM
> > To: Penny Zheng <Penny.Zheng@arm.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>;
> > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; Bertrand
> > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> > <Volodymyr_Babchuk@epam.com>
> > Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest
> > device tree
> >
> > On Mon, 4 Jul 2022, Penny Zheng wrote:
> > > Hi Stefano and Julien
> > >
> > > > -----Original Message-----
> > > > From: Stefano Stabellini <sstabellini@kernel.org>
> > > > Sent: Saturday, June 25, 2022 5:57 AM
> > > > To: Julien Grall <julien@xen.org>
> > > > Cc: Penny Zheng <Penny.Zheng@arm.com>;
> > > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>;
> > Stefano
> > > > Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> > > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> > > > <Volodymyr_Babchuk@epam.com>
> > > > Subject: Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in
> > > > guest device tree
> > > >
> > > > On Fri, 24 Jun 2022, Julien Grall wrote:
> > > > > On 20/06/2022 06:11, Penny Zheng wrote:
> > > > > > 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>
> > > > > > ---
> > > > > > 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 | 143
> > > > +++++++++++++++++++++++++++++-
> > > > > > xen/arch/arm/include/asm/kernel.h | 1 +
> > > > > > xen/arch/arm/include/asm/setup.h | 1 +
> > > > > > 3 files changed, 143 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/xen/arch/arm/domain_build.c
> > > > > > b/xen/arch/arm/domain_build.c index 1584e6c2ce..4d62440a0e
> > > > > > 100644
> > > > > > --- a/xen/arch/arm/domain_build.c
> > > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > > @@ -900,7 +900,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,
> > > > > > + u32 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;
> > > > > > + 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; @@ -971,6 +986,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;
> > > > > > @@ -1301,6 +1324,117 @@ 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,
> > > > > > + struct meminfo *mem)
> > > > >
> > > > > NIT: AFAICT mem is not changed, so it should be const.
> > > > >
> > > > > > +{
> > > > > > + unsigned long i = 0;
> > > > >
> > > > > NIT: This should be "unsigned int" to match the type of nr_banks.
> > > > >
> > > > > > + 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;
> > > > > > + uint8_t shm_id = mem->bank[i].shm_id;
> > > > > > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > > > > > + char buf[27];
> > > > > > + const char compat[] = "xen,shared-memory-v1";
> > > > > > + __be32 reg[4];
> > > > > > + __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 %lu: %#"PRIx64"-
> > >%#"PRIx64"\n",
> > > > > > + i, start, start + size);
> > > > > > +
> > > > > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> > > > >
> > > > > Looking at the Linux binding, "xen,id" is meant to be a string.
> > > > > But here you are writing it as an integer.
> > > >
> > > > Good catch!
> > > >
> > > >
> > > > > Given that the Linux binding is already merged, I think the Xen
> > > > > binding should be changed.
> > > >
> > > > We would be compliant with both bindings (xen and linux) just by
> > > > writing shm_id as string here, but if it is not too difficult we
> > > > might as well harmonize the two bindings and also define xen,shm-id as a
> > string.
> > > >
> > > > On the Xen side, I would suggest to put a clear size limit so that
> > > > the string is easier to handle.
> > >
> > > I've already made the xen,shm-id parsed as a string too, seeing the below
> > code:
> > > "
> > > prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> > > if ( !prop_id )
> > > return -ENOENT;
> > > shm_id = simple_strtoul(prop_id->data, NULL, 10);
> > > if ( shm_id >= NR_MEM_BANKS )
> > > {
> > > printk("fdt: invalid `xen,shm-id` %lu for static shared memory node.\n",
> > > shm_id);
> > > return -EINVAL;
> > > }
> > > "
> > > The size limit is smaller than 256, just as stated in doc:
> > > "
> > > - xen,shm-id
> > >
> > > A string that represents the unique identifier of the shared memory
> > > region. The maximum identifier shall be "xen,shm-id = 255".
> > > "
> > > Hope this fits what both of you suggested~~~
> >
> > Yes. I think supporting arbitrary strings like "my-shared-mem-1" would be
> > nice-to-have but I wouldn't make it a hard requirement.
> >
>
> Oh, the example "my-shared-mem-1" really expands my mind, I think I understand
> what you and Julien referred as free form string, which shall not be limited to only
> numeric number... thanks!!!
>
> You were suggesting a strict limit on the number of characters, TBH, I have no clue
> What the standard is here. Any suggestions?
>
> If considering padding, maybe 19?
> "
> 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[19];
> unsigned int nr_shm_borrowers;
> #endif
> };
> "
Yeah I suggested a strict limit on the number of chars so that we could
embed the string in struct membank. I would pick 16 characters which is
equivalent to two uint64_t in terms of memory usage.
> > "255" as a string would match Linux's requirements for xen,id.
>
> I will use your example "my-shm-mem-1", I think its better for users
> to understand, at least for me...
+1
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree
2022-07-08 16:40 ` Stefano Stabellini
@ 2022-07-11 7:59 ` Penny Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-07-11 7:59 UTC (permalink / raw)
To: Stefano Stabellini
Cc: julien, xen-devel, Wei Chen, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Saturday, July 9, 2022 12:41 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; julien@xen.org; xen-
> devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; Bertrand
> Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest
> device tree
>
> On Thu, 7 Jul 2022, Penny Zheng wrote:
> > Hi Stefano and julien
> >
> > > -----Original Message-----
> > > From: Stefano Stabellini <sstabellini@kernel.org>
> > > Sent: Thursday, July 7, 2022 7:53 AM
> > > To: Penny Zheng <Penny.Zheng@arm.com>
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall
> > > <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Chen
> > > <Wei.Chen@arm.com>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>;
> > > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> > > Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in
> > > guest device tree
> > >
> > > On Mon, 4 Jul 2022, Penny Zheng wrote:
> > > > Hi Stefano and Julien
> > > >
> > > > > -----Original Message-----
> > > > > From: Stefano Stabellini <sstabellini@kernel.org>
> > > > > Sent: Saturday, June 25, 2022 5:57 AM
> > > > > To: Julien Grall <julien@xen.org>
> > > > > Cc: Penny Zheng <Penny.Zheng@arm.com>;
> > > > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>;
> > > Stefano
> > > > > Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> > > > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> > > > > <Volodymyr_Babchuk@epam.com>
> > > > > Subject: Re: [PATCH v5 7/8] xen/arm: create shared memory nodes
> > > > > in guest device tree
> > > > >
> > > > > On Fri, 24 Jun 2022, Julien Grall wrote:
> > > > > > On 20/06/2022 06:11, Penny Zheng wrote:
> > > > > > > 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>
> > > > > > > ---
> > > > > > > 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 | 143
> > > > > +++++++++++++++++++++++++++++-
> > > > > > > xen/arch/arm/include/asm/kernel.h | 1 +
> > > > > > > xen/arch/arm/include/asm/setup.h | 1 +
> > > > > > > 3 files changed, 143 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/xen/arch/arm/domain_build.c
> > > > > > > b/xen/arch/arm/domain_build.c index 1584e6c2ce..4d62440a0e
> > > > > > > 100644
> > > > > > > --- a/xen/arch/arm/domain_build.c
> > > > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > > > @@ -900,7 +900,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,
> > > > > > > + u32 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;
> > > > > > > + 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; @@ -971,6 +986,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;
> > > > > > > @@ -1301,6 +1324,117 @@ 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,
> > > > > > > + struct meminfo *mem)
> > > > > >
> > > > > > NIT: AFAICT mem is not changed, so it should be const.
> > > > > >
> > > > > > > +{
> > > > > > > + unsigned long i = 0;
> > > > > >
> > > > > > NIT: This should be "unsigned int" to match the type of nr_banks.
> > > > > >
> > > > > > > + 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;
> > > > > > > + uint8_t shm_id = mem->bank[i].shm_id;
> > > > > > > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > > > > > > + char buf[27];
> > > > > > > + const char compat[] = "xen,shared-memory-v1";
> > > > > > > + __be32 reg[4];
> > > > > > > + __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 %lu: %#"PRIx64"-
> > > >%#"PRIx64"\n",
> > > > > > > + i, start, start + size);
> > > > > > > +
> > > > > > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> > > > > >
> > > > > > Looking at the Linux binding, "xen,id" is meant to be a string.
> > > > > > But here you are writing it as an integer.
> > > > >
> > > > > Good catch!
> > > > >
> > > > >
> > > > > > Given that the Linux binding is already merged, I think the
> > > > > > Xen binding should be changed.
> > > > >
> > > > > We would be compliant with both bindings (xen and linux) just by
> > > > > writing shm_id as string here, but if it is not too difficult we
> > > > > might as well harmonize the two bindings and also define
> > > > > xen,shm-id as a
> > > string.
> > > > >
> > > > > On the Xen side, I would suggest to put a clear size limit so
> > > > > that the string is easier to handle.
> > > >
> > > > I've already made the xen,shm-id parsed as a string too, seeing
> > > > the below
> > > code:
> > > > "
> > > > prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
> > > > if ( !prop_id )
> > > > return -ENOENT;
> > > > shm_id = simple_strtoul(prop_id->data, NULL, 10);
> > > > if ( shm_id >= NR_MEM_BANKS )
> > > > {
> > > > printk("fdt: invalid `xen,shm-id` %lu for static shared memory
> node.\n",
> > > > shm_id);
> > > > return -EINVAL;
> > > > }
> > > > "
> > > > The size limit is smaller than 256, just as stated in doc:
> > > > "
> > > > - xen,shm-id
> > > >
> > > > A string that represents the unique identifier of the shared memory
> > > > region. The maximum identifier shall be "xen,shm-id = 255".
> > > > "
> > > > Hope this fits what both of you suggested~~~
> > >
> > > Yes. I think supporting arbitrary strings like "my-shared-mem-1"
> > > would be nice-to-have but I wouldn't make it a hard requirement.
> > >
> >
> > Oh, the example "my-shared-mem-1" really expands my mind, I think I
> > understand what you and Julien referred as free form string, which
> > shall not be limited to only numeric number... thanks!!!
> >
> > You were suggesting a strict limit on the number of characters, TBH, I
> > have no clue What the standard is here. Any suggestions?
> >
> > If considering padding, maybe 19?
> > "
> > 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[19];
> > unsigned int nr_shm_borrowers;
> > #endif
> > };
> > "
>
> Yeah I suggested a strict limit on the number of chars so that we could
> embed the string in struct membank. I would pick 16 characters which is
> equivalent to two uint64_t in terms of memory usage.
>
>
Hmm, Am I calculating wrongly? When it reaches to nr_shm_borrowers, it
requires 4 bytes-aligned, and if it is 16 characters, it will ask extra 3 bytes
to do the padding(8 + 8 + 1 + 16 + "3"). This is the reason why I chose 19, to
make use of every byte.
Or maybe 16 characters is applied to be the multiple orders of 2, which has more
flexibility for newly added field?
Just out of curiosity why you choose 16 over 19, hope it doesn't bother too much~
> > > "255" as a string would match Linux's requirements for xen,id.
> >
> > I will use your example "my-shm-mem-1", I think its better for users
> > to understand, at least for me...
>
> +1
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-29 10:17 ` Julien Grall
@ 2022-07-13 2:42 ` Penny Zheng
2022-07-13 9:09 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-07-13 2:42 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
Before submitting the v6 patch series, I would like to double confirm that ...
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, June 29, 2022 6:18 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
>
>
>
> On 29/06/2022 06:38, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Saturday, June 25, 2022 1:55 AM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>
> >> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
> >>
> >> Hi Penny,
> >>
> >> On 20/06/2022 06:11, Penny Zheng wrote:
> >>> From: Penny Zheng <penny.zheng@arm.com>
> >>>
> >>> This patch serie introduces a new feature: setting up static
> >>
> >> Typo: s/serie/series/
> >>
> >>> shared memory on a dom0less system, through device tree configuration.
> >>>
> >>> This commit parses shared memory node at boot-time, and reserve it
> >>> in bootinfo.reserved_mem to avoid other use.
> >>>
> >>> This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> >>> static-shm-related codes, and this option depends on static memory(
> >>> CONFIG_STATIC_MEMORY). That's because that later we want to reuse a
> >>> few helpers, guarded with CONFIG_STATIC_MEMORY, like
> >>> acquire_staticmem_pages, etc, on static shared memory.
> >>>
> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>> ---
> >>> v5 change:
> >>> - no change
> >>> ---
> >>> v4 change:
> >>> - nit fix on doc
> >>> ---
> >>> v3 change:
> >>> - make nr_shm_domain unsigned int
> >>> ---
> >>> v2 change:
> >>> - document refinement
> >>> - remove bitmap and use the iteration to check
> >>> - add a new field nr_shm_domain to keep the number of shared domain
> >>> ---
> >>> docs/misc/arm/device-tree/booting.txt | 120
> >> ++++++++++++++++++++++++++
> >>> xen/arch/arm/Kconfig | 6 ++
> >>> xen/arch/arm/bootfdt.c | 68 +++++++++++++++
> >>> xen/arch/arm/include/asm/setup.h | 3 +
> >>> 4 files changed, 197 insertions(+)
> >>>
> >>> diff --git a/docs/misc/arm/device-tree/booting.txt
> >>> b/docs/misc/arm/device-tree/booting.txt
> >>> index 98253414b8..6467bc5a28 100644
> >>> --- a/docs/misc/arm/device-tree/booting.txt
> >>> +++ b/docs/misc/arm/device-tree/booting.txt
> >>> @@ -378,3 +378,123 @@ device-tree:
> >>>
> >>> This will reserve a 512MB region starting at the host physical address
> >>> 0x30000000 to be exclusively used by DomU1.
> >>> +
> >>> +Static Shared Memory
> >>> +====================
> >>> +
> >>> +The static shared memory device tree nodes allow users to
> >>> +statically set up shared memory on dom0less system, enabling
> >>> +domains to do shm-based communication.
> >>> +
> >>> +- compatible
> >>> +
> >>> + "xen,domain-shared-memory-v1"
> >>> +
> >>> +- xen,shm-id
> >>> +
> >>> + An 8-bit integer that represents the unique identifier of the
> >>> + shared
> >> memory
> >>> + region. The maximum identifier shall be "xen,shm-id = <0xff>".
> >>> +
> >>> +- xen,shared-mem
> >>> +
> >>> + An array takes a physical address, which is the base address of the
> >>> + shared memory region in host physical address space, a size,
> >>> + and a
> >> guest
> >>> + physical address, as the target address of the mapping. The
> >>> + number of
> >> cells
> >>> + for the host address (and size) is the same as the guest pseudo-
> physical
> >>> + address and they are inherited from the parent node.
> >>
> >> Sorry for jump in the discussion late. But as this is going to be a
> >> stable ABI, I would to make sure the interface is going to be easily
> extendable.
> >>
> >> AFAIU, with your proposal the host physical address is mandatory. I
> >> would expect that some user may want to share memory but don't care
> >> about the exact location in memory. So I think it would be good to
> >> make it optional in the binding.
> >>
> >> I think this wants to be done now because it would be difficult to
> >> change the binding afterwards (the host physical address is the first set of
> cells).
> >>
> >> The Xen doesn't need to handle the optional case.
> >>
... what you suggested here is that during "xen,shared-mem" device tree property
parsing process, if we find that user doesn't provide physical address, we will output
an error, saying that it is not supported at the moment, right?
> >
> > Sure, I'll make "the host physical address" optional here, and right
> > now, with no actual code implementation. I'll make up it later in free
> > time~
> >
> > The user case you mentioned here is that we let xen to allocate an
> > arbitrary static shared memory region, so size and guest physical address
> are still mandatory, right?
>
> That's correct.
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-07-13 2:42 ` Penny Zheng
@ 2022-07-13 9:09 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-07-13 9:09 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 13/07/2022 03:42, Penny Zheng wrote:
>>>>
>>>> The Xen doesn't need to handle the optional case.
>>>>
>
> ... what you suggested here is that during "xen,shared-mem" device tree property
> parsing process, if we find that user doesn't provide physical address, we will output
> an error, saying that it is not supported at the moment, right?
You are correct.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-06-29 8:39 ` Penny Zheng
@ 2022-07-15 18:10 ` Julien Grall
2022-07-18 2:35 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2022-07-15 18:10 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 29/06/2022 09:39, Penny Zheng wrote:
>>> + for ( i = 0; i < mem->nr_banks; i++ )
>>> + {
>>> + /*
>>> + * A static shared memory region could be shared between multiple
>>> + * domains.
>>> + */
>>> + if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
>>> + break;
>
> Maybe I need to add a check on shm-id:
> "
> /*
> * A static shared memory region could be shared between multiple
> * domains.
> */
> if ( strcmp(shm_id, mem->bank[i].shm_id) == 0 )
> {
> if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> break;
> else
> {
> printk("Warning: xen,shm-id %s does not match for all the nodes using the same region.\n",
> shm_id);
> return -EINVAL;
> }
> }
> "
> Wdyt？
AFAICT, this would allow to region to overlap if they have different shm
ID. I am not entirely sure the rest of your code would work properly in
this case (what if the owner is different).
So I think we need the following checks:
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
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io
2022-07-04 7:20 ` Penny Zheng
@ 2022-07-15 18:43 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2022-07-15 18:43 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Hi Penny,
On 04/07/2022 08:20, Penny Zheng wrote:
>>>>> +
>>>>> + return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
>>>>> + pbase, psize);
>>>>> +
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Func allocate_shared_memory is supposed to be only called
>>>>
>>>> I am a bit concerned with the word "supposed". Are you implying that
>>>> it may be called by someone that is not the owner? If not, then it
>>>> should be "should".
>>>>
>>>> Also NIT: Spell out completely "func". I.e "The function".
>>>>
>>>>> + * from the owner.
>>>>
>>>> I read from as "current should be the owner". But I guess this is not
>>>> what you mean here. Instead it looks like you mean "d" is the owner.
>>>> So I would write "d should be the owner of the shared area".
>>>>
>>>> It would be good to have a check/ASSERT confirm this (assuming this
>>>> is easy to write).
>>>>
>>>
>>> The check is already in the calling path, I guess...
>>
>> Can you please confirm it?
>>
>
> I mean that allocate_shared_memory is only called under the following condition, and
> it confirms it is the right owner.
> "
> if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> {
> /* 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);
> "
I agree that this looks to be the case today. But error can slip easily,
so if we can add some ASSERT() in function then you could catch issues
during development. Hence why I suggested to add an ASSERT() if possible.
>
> TBH, apart from that, I don't know how to check if the input d is the right owner, or am I
> misunderstanding some your suggestion here?
Is page_get_owner() already properly set? If yes, you could ASSERT() the
first page of the range belongs to 'd'.
This is more an hardening exercise, so it is not critical if it is
difficult (or not possible) to have an ASSERT().
>
>> [...]
>>
>>>>> + prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
>>>>> + if ( !prop )
>>>>> + {
>>>>> + printk("Shared memory node does not provide
>>>>> + \"xen,shared-
>>>> mem\" property.\n");
>>>>> + return -ENOENT;
>>>>> + }
>>>>> + cells = (const __be32 *)prop->value;
>>>>> + /* xen,shared-mem = <pbase, psize, gbase>; */
>>>>> + device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
>>>>> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
>>>>> + PAGE_SIZE));
>>>>
>>>> See above about what ASSERT()s are for.
>>>>
>>>
>>> Do you think address was suitably checked here, is it enough?
>>
>> As I wrote before, ASSERT() should not be used to check user inputs.
>> They need to happen in both debug and production build.
>>
>>> If it is enough, I'll modify above ASSERT() to mfn_valid()
>>
>> It is not clear what ASSERT() you are referring to.
>>
>
> For whether page is aligned, I will add the below check:
> "
> if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(psize, PAGE_SIZE) ||
> !IS_ALIGNED(gbase, PAGE_SIZE) )
> {
> printk("%pd: physical address %lu, size %lu or guest address %lu is not suitably aligned.\n",
AFAICT, the type for the 3 variables is paddr_t. So you want to use
0x%"PRIpaddr" instead of %lu.
BTW, in general, for address it is preferable to use hexadecimal over
decimal.
> d, pbase, psize, gbase);
> return -EINVAL;
> }
> "
> For whether the whole address range is valid, I will add the below check:
> "
> for ( i = 0; i < PFN_DOWN(psize); i++ )
> if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
> {
> printk("%pd: invalid physical address %"PRI_paddr" or size %"PRI_paddr"\n",
s/PRI_paddr/PRIpaddr/
I am also not sure why you want to print the size here?
> d, pbase, psize);
> return -EINVAL;
> }
> "
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v5 1/8] xen/arm: introduce static shared memory
2022-07-15 18:10 ` Julien Grall
@ 2022-07-18 2:35 ` Penny Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-07-18 2:35 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, July 16, 2022 2:10 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
>
> Hi Penny,
>
> On 29/06/2022 09:39, Penny Zheng wrote:
> >>> + for ( i = 0; i < mem->nr_banks; i++ )
> >>> + {
> >>> + /*
> >>> + * A static shared memory region could be shared between multiple
> >>> + * domains.
> >>> + */
> >>> + if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> >>> + break;
> >
> > Maybe I need to add a check on shm-id:
> > "
> > /*
> > * A static shared memory region could be shared between multiple
> > * domains.
> > */
> > if ( strcmp(shm_id, mem->bank[i].shm_id) == 0 )
> > {
> > if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> > break;
> > else
> > {
> > printk("Warning: xen,shm-id %s does not match for all the nodes
> using the same region.\n",
> > shm_id);
> > return -EINVAL;
> > }
> > }
> > "
> > Wdyt？
>
> AFAICT, this would allow to region to overlap if they have different shm ID. I
> am not entirely sure the rest of your code would work properly in this case
> (what if the owner is different).
>
> So I think we need the following checks:
> 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
>
Understood, true, the overlap shall also be checked.
"
@@ -451,6 +453,31 @@ static int __init process_shm_node(const void *fdt, int node,
return -EINVAL;
}
}
+ else
+ {
+ paddr_t end = paddr + size;
+ paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
+
+ if ( (paddr < mem->bank[i].start && end <= mem->bank[i].start) ||
+ (paddr >= bank_end && end > bank_end) )
+ {
+ if ( strncmp(shm_id, mem->bank[i].shm_id,
+ MAX_SHM_ID_LENGTH) != 0 )
+ break;
+ 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 )
"
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2022-07-18 2:36 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-20 5:11 [PATCH v5 0/8] static shared memory on dom0less system Penny Zheng
2022-06-20 5:11 ` [PATCH v5 1/8] xen/arm: introduce static shared memory Penny Zheng
2022-06-24 17:55 ` Julien Grall
2022-06-29 5:38 ` Penny Zheng
2022-06-29 10:17 ` Julien Grall
2022-07-13 2:42 ` Penny Zheng
2022-07-13 9:09 ` Julien Grall
2022-06-29 8:39 ` Penny Zheng
2022-07-15 18:10 ` Julien Grall
2022-07-18 2:35 ` Penny Zheng
2022-06-24 19:25 ` Julien Grall
2022-06-29 8:40 ` Penny Zheng
2022-06-20 5:11 ` [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io Penny Zheng
2022-06-24 18:22 ` Julien Grall
2022-06-29 7:13 ` Penny Zheng
2022-06-29 10:34 ` Julien Grall
2022-07-04 7:20 ` Penny Zheng
2022-07-15 18:43 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 3/8] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
2022-06-24 19:07 ` Julien Grall
2022-06-29 7:49 ` Penny Zheng
2022-06-20 5:11 ` [PATCH v5 4/8] xen/arm: introduce put_page_nr and get_page_nr Penny Zheng
2022-06-24 19:10 ` Julien Grall
2022-06-20 5:11 ` [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated Penny Zheng
2022-06-24 19:18 ` Julien Grall
2022-06-29 8:00 ` Penny Zheng
2022-06-20 5:11 ` [PATCH v5 6/8] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
2022-06-20 5:11 ` [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree Penny Zheng
2022-06-24 19:30 ` Julien Grall
2022-06-24 21:56 ` Stefano Stabellini
2022-07-04 7:45 ` Penny Zheng
2022-07-05 8:09 ` Julien Grall
2022-07-05 23:21 ` Stefano Stabellini
2022-07-06 23:52 ` Stefano Stabellini
2022-07-07 4:01 ` Penny Zheng
2022-07-08 16:40 ` Stefano Stabellini
2022-07-11 7:59 ` Penny Zheng
2022-06-20 5:11 ` [PATCH v5 8/8] xen/arm: enable statically shared memory on Dom0 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.