* [PATCH v1 01/13] xen/arm: introduce static shared memory
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 1:59 ` Stefano Stabellini
2022-03-11 6:11 ` [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED Penny Zheng
` (11 subsequent siblings)
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, 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.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
docs/misc/arm/device-tree/booting.txt | 118 ++++++++++++++++++++++++++
xen/arch/arm/bootfdt.c | 52 ++++++++++++
2 files changed, 170 insertions(+)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index a94125394e..f702ade817 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -355,3 +355,121 @@ 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 u8 value 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.
+
+- 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/bootfdt.c b/xen/arch/arm/bootfdt.c
index afaa0e249b..7de0f8cea9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -363,6 +363,56 @@ static int __init process_domain_node(const void *fdt, int node,
size_cells, &bootinfo.reserved_mem, true);
}
+static __initdata DECLARE_BITMAP(shm_mask, NR_MEM_BANKS);
+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;
+ u32 id;
+
+ 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;
+
+ id = device_tree_get_u32(fdt, node, "xen,shm-id", NR_MEM_BANKS);
+ if ( id >= NR_MEM_BANKS )
+ return -EINVAL;
+
+ /*
+ * A shared memory region could be shared between multiple domains. We
+ * use the shm_mask bitmask to prevent iterating over all reserved memory
+ * regions each time.
+ */
+ if ( !test_bit(id, shm_mask) )
+ {
+ /*
+ * xen,shared-mem = <paddr, size, gaddr>;
+ * Memory region starting from physical address #paddr of #size shall
+ * be mapped to guest physical address #gaddr as shared memory region.
+ */
+ cell = (const __be32 *)prop->data;
+ device_tree_get_reg(&cell, address_cells, size_cells, &paddr, &size);
+
+ /* shared memory shall be reserved from 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++;
+ set_bit(id, shm_mask);
+ }
+
+ return 0;
+}
+
static int __init early_scan_node(const void *fdt,
int node, const char *name, int depth,
u32 address_cells, u32 size_cells,
@@ -383,6 +433,8 @@ 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);
+ 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);
if ( rc < 0 )
printk("fdt: node `%s': parsing failed\n", name);
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 01/13] xen/arm: introduce static shared memory
2022-03-11 6:11 ` [PATCH v1 01/13] xen/arm: introduce static shared memory Penny Zheng
@ 2022-03-18 1:59 ` Stefano Stabellini
0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 1:59 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 11 Mar 2022, Penny Zheng wrote:
> 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.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> docs/misc/arm/device-tree/booting.txt | 118 ++++++++++++++++++++++++++
> xen/arch/arm/bootfdt.c | 52 ++++++++++++
> 2 files changed, 170 insertions(+)
>
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index a94125394e..f702ade817 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -355,3 +355,121 @@ device-tree:
>
> This will reserve a 512MB region starting at the host physical address
> 0x30000000 to be exclusively used by DomU1.
> +
> +Static Shared Memory
> +=============
We ran out of = :-)
> +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 u8 value 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.
This is good. We also need to say how to know how many cells each of the
three elements take: how many cells is the host physical address, how
many cells is the size, how many cells is the guest physical address.
This node is a subnode of a xen,domain compatible node (or chosen) which
comes with an #address-cells and a #size-cells. This is similarly to
multiboot,kernel nodes, so we could do the same. Here we could say that
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/bootfdt.c b/xen/arch/arm/bootfdt.c
> index afaa0e249b..7de0f8cea9 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -363,6 +363,56 @@ static int __init process_domain_node(const void *fdt, int node,
> size_cells, &bootinfo.reserved_mem, true);
> }
>
> +static __initdata DECLARE_BITMAP(shm_mask, NR_MEM_BANKS);
> +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;
> + u32 id;
> +
> + 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;
> +
> + id = device_tree_get_u32(fdt, node, "xen,shm-id", NR_MEM_BANKS);
> + if ( id >= NR_MEM_BANKS )
> + return -EINVAL;
> +
> + /*
> + * A shared memory region could be shared between multiple domains. We
> + * use the shm_mask bitmask to prevent iterating over all reserved memory
> + * regions each time.
> + */
> + if ( !test_bit(id, shm_mask) )
> + {
> + /*
> + * xen,shared-mem = <paddr, size, gaddr>;
> + * Memory region starting from physical address #paddr of #size shall
> + * be mapped to guest physical address #gaddr as shared memory region.
> + */
> + cell = (const __be32 *)prop->data;
> + device_tree_get_reg(&cell, address_cells, size_cells, &paddr, &size);
> +
> + /* shared memory shall be reserved from 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++;
> + set_bit(id, shm_mask);
> + }
> + return 0;
> +}
> +
> static int __init early_scan_node(const void *fdt,
> int node, const char *name, int depth,
> u32 address_cells, u32 size_cells,
> @@ -383,6 +433,8 @@ 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);
> + 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);
>
> if ( rc < 0 )
> printk("fdt: node `%s': parsing failed\n", name);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
2022-03-11 6:11 ` [PATCH v1 01/13] xen/arm: introduce static shared memory Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 1:59 ` Stefano Stabellini
2022-03-18 8:53 ` Jan Beulich
2022-03-11 6:11 ` [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared Penny Zheng
` (10 subsequent siblings)
12 siblings, 2 replies; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu, Penny Zheng
From: Penny Zheng <penzhe01@a011292.shanghai.arm.com>
In case to own statically shared pages when owner domain is not
explicitly defined, this commits propose a special domain DOMID_SHARED,
and we assign it 0x7FF5, as one of the system domains.
Statically shared memory reuses the same way of initialization with static
memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
We intends to do shared domain creation after setup_virt_paging so shared
domain could successfully do p2m initialization.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/Kconfig | 7 +++++++
xen/arch/arm/domain.c | 12 ++++++++++--
xen/arch/arm/include/asm/domain.h | 6 ++++++
xen/arch/arm/setup.c | 22 ++++++++++++++++++++++
xen/common/domain.c | 11 +++++++----
xen/common/page_alloc.c | 5 +++++
xen/common/vsprintf.c | 9 +++++----
xen/include/public/xen.h | 6 ++++++
xen/include/xen/sched.h | 2 ++
9 files changed, 70 insertions(+), 10 deletions(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ecfa6822e4..c54accefb1 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -106,6 +106,13 @@ config TEE
source "arch/arm/tee/Kconfig"
+config STATIC_SHM
+ bool "Statically shared memory on a dom0less system" if UNSUPPORTED
+ depends on STATIC_MEMORY
+ default n
+ 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/domain.c b/xen/arch/arm/domain.c
index 8110c1df86..1ff1df5d3f 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -44,6 +44,10 @@
DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
+#ifdef CONFIG_STATIC_SHM
+struct domain *__read_mostly dom_shared;
+#endif
+
static void do_idle(void)
{
unsigned int cpu = smp_processor_id();
@@ -703,7 +707,7 @@ int arch_domain_create(struct domain *d,
if ( is_idle_domain(d) )
return 0;
- ASSERT(config != NULL);
+ ASSERT(is_shared_domain(d) ? config == NULL : config != NULL);
#ifdef CONFIG_IOREQ_SERVER
ioreq_domain_init(d);
@@ -712,12 +716,16 @@ int arch_domain_create(struct domain *d,
d->arch.directmap = flags & CDF_directmap;
/* p2m_init relies on some value initialized by the IOMMU subsystem */
- if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
+ if ( (rc = iommu_domain_init(d, is_shared_domain(d) ? 0 : config->iommu_opts)) != 0 )
goto fail;
if ( (rc = p2m_init(d)) != 0 )
goto fail;
+ /* DOMID_shared is sufficiently constructed after p2m initialization. */
+ if ( is_shared_domain(d) )
+ return 0;
+
rc = -ENOMEM;
if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
goto fail;
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index c56f6e4398..ea7a7219a3 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -31,6 +31,12 @@ enum domain_type {
#define is_domain_direct_mapped(d) (d)->arch.directmap
+#ifdef CONFIG_STATIC_SHM
+extern struct domain *dom_shared;
+#else
+#define dom_shared NULL
+#endif
+
/*
* Is the domain using the host memory layout?
*
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d5d0792ed4..f6a3b04958 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
return ( !dom0found && domUfound );
}
+#ifdef CONFIG_STATIC_SHM
+static void __init setup_shared_domain(void)
+{
+ /*
+ * Initialise our DOMID_SHARED domain.
+ * This domain owns statically shared pages when owner domain is not
+ * explicitly defined.
+ */
+ dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
+ if ( IS_ERR(dom_shared) )
+ panic("Failed to create d[SHARED]: %ld\n", PTR_ERR(dom_shared));
+}
+#endif
+
size_t __read_mostly dcache_line_bytes;
/* C entry point for boot CPU */
@@ -1022,6 +1036,14 @@ void __init start_xen(unsigned long boot_phys_offset,
apply_alternatives_all();
enable_errata_workarounds();
+#ifdef CONFIG_STATIC_SHM
+ /*
+ * This needs to be called **after** setup_virt_paging so shared
+ * domains could successfully do p2m initialization.
+ */
+ setup_shared_domain();
+#endif
+
/* Create initial domain 0. */
if ( !is_dom0less_mode() )
create_dom0();
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3742322d22..5cdd0b9f5b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -643,11 +643,14 @@ struct domain *domain_create(domid_t domid,
rangeset_domain_initialise(d);
- /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
- if ( is_system_domain(d) && !is_idle_domain(d) )
+ /*
+ * DOMID_{XEN,IO,etc} (other than IDLE and DOMID_shared) are
+ * sufficiently constructed.
+ */
+ if ( is_system_domain(d) && !is_idle_domain(d) && !is_shared_domain(d) )
return d;
- if ( !is_idle_domain(d) )
+ if ( !is_idle_domain(d) && !is_shared_domain(d) )
{
if ( !is_hardware_domain(d) )
d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
@@ -663,7 +666,7 @@ struct domain *domain_create(domid_t domid,
goto fail;
init_status |= INIT_arch;
- if ( !is_idle_domain(d) )
+ if ( !is_idle_domain(d) && !is_shared_domain(d) )
{
watchdog_domain_init(d);
init_status |= INIT_watchdog;
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index f8749b0787..e5e357969d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2616,6 +2616,11 @@ struct domain *get_pg_owner(domid_t domid)
switch ( domid )
{
+#ifdef CONFIG_STATIC_SHM
+ case DOMID_SHARED:
+ pg_owner = rcu_lock_domain(dom_shared);
+ break;
+#endif
case DOMID_IO:
pg_owner = rcu_lock_domain(dom_io);
break;
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index b278961cc3..a22854001b 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -359,10 +359,11 @@ static char *print_domain(char *str, const char *end, const struct domain *d)
switch ( d->domain_id )
{
- case DOMID_IO: name = "[IO]"; break;
- case DOMID_XEN: name = "[XEN]"; break;
- case DOMID_COW: name = "[COW]"; break;
- case DOMID_IDLE: name = "[IDLE]"; break;
+ case DOMID_IO: name = "[IO]"; break;
+ case DOMID_XEN: name = "[XEN]"; break;
+ case DOMID_COW: name = "[COW]"; break;
+ case DOMID_IDLE: name = "[IDLE]"; break;
+ case DOMID_SHARED: name = "[SHARED]"; break;
/*
* In principle, we could ASSERT_UNREACHABLE() in the default case.
* However, this path is used to print out crash information, which
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index e373592c33..2e00741f09 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -612,6 +612,12 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
/* DOMID_INVALID is used to identify pages with unknown owner. */
#define DOMID_INVALID xen_mk_uint(0x7FF4)
+/*
+ * DOMID_SHARED is used as the owner of statically shared pages, when
+ * owner is not explicitly defined.
+ */
+#define DOMID_SHARED xen_mk_uint(0x7FF5)
+
/* Idle domain. */
#define DOMID_IDLE xen_mk_uint(0x7FFF)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 24a9a87f83..2fb236f4ea 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -618,6 +618,8 @@ static inline bool is_system_domain(const struct domain *d)
return d->domain_id >= DOMID_FIRST_RESERVED;
}
+#define is_shared_domain(d) ((d)->domain_id == DOMID_SHARED)
+
#define DOMAIN_DESTROYED (1u << 31) /* assumes atomic_t is >= 32 bits */
#define put_domain(_d) \
if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-11 6:11 ` [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED Penny Zheng
@ 2022-03-18 1:59 ` Stefano Stabellini
2022-03-18 6:43 ` Penny Zheng
2022-03-18 8:53 ` Jan Beulich
1 sibling, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 1:59 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penzhe01@a011292.shanghai.arm.com>
>
> In case to own statically shared pages when owner domain is not
> explicitly defined, this commits propose a special domain DOMID_SHARED,
> and we assign it 0x7FF5, as one of the system domains.
>
> Statically shared memory reuses the same way of initialization with static
> memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
Why does it depend on CONFIG_STATIC_MEMORY? This is a genuine question,
I am not trying to scope-creep the series. Is there an actual technical
dependency on CONFIG_STATIC_MEMORY? If not, it would be super useful to
be able to share memory statically even between normal dom0less guests
(of course it would be responsibility of the user to provide the right
addresses and avoid mapping clashes.) I know that some of our users have
requested this feature in the past.
> We intends to do shared domain creation after setup_virt_paging so shared
> domain could successfully do p2m initialization.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/Kconfig | 7 +++++++
> xen/arch/arm/domain.c | 12 ++++++++++--
> xen/arch/arm/include/asm/domain.h | 6 ++++++
> xen/arch/arm/setup.c | 22 ++++++++++++++++++++++
> xen/common/domain.c | 11 +++++++----
> xen/common/page_alloc.c | 5 +++++
> xen/common/vsprintf.c | 9 +++++----
> xen/include/public/xen.h | 6 ++++++
> xen/include/xen/sched.h | 2 ++
> 9 files changed, 70 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..c54accefb1 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -106,6 +106,13 @@ config TEE
>
> source "arch/arm/tee/Kconfig"
>
> +config STATIC_SHM
> + bool "Statically shared memory on a dom0less system" if UNSUPPORTED
> + depends on STATIC_MEMORY
> + default n
> + 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/domain.c b/xen/arch/arm/domain.c
> index 8110c1df86..1ff1df5d3f 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -44,6 +44,10 @@
>
> DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>
> +#ifdef CONFIG_STATIC_SHM
> +struct domain *__read_mostly dom_shared;
> +#endif
This one should probably go to xen/common/domain.c to stay close to the
other special domains.
> static void do_idle(void)
> {
> unsigned int cpu = smp_processor_id();
> @@ -703,7 +707,7 @@ int arch_domain_create(struct domain *d,
> if ( is_idle_domain(d) )
> return 0;
>
> - ASSERT(config != NULL);
> + ASSERT(is_shared_domain(d) ? config == NULL : config != NULL);
>
> #ifdef CONFIG_IOREQ_SERVER
> ioreq_domain_init(d);
> @@ -712,12 +716,16 @@ int arch_domain_create(struct domain *d,
> d->arch.directmap = flags & CDF_directmap;
>
> /* p2m_init relies on some value initialized by the IOMMU subsystem */
> - if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> + if ( (rc = iommu_domain_init(d, is_shared_domain(d) ? 0 : config->iommu_opts)) != 0 )
> goto fail;
>
> if ( (rc = p2m_init(d)) != 0 )
> goto fail;
>
> + /* DOMID_shared is sufficiently constructed after p2m initialization. */
> + if ( is_shared_domain(d) )
> + return 0;
> +
> rc = -ENOMEM;
> if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
> goto fail;
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index c56f6e4398..ea7a7219a3 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -31,6 +31,12 @@ enum domain_type {
>
> #define is_domain_direct_mapped(d) (d)->arch.directmap
>
> +#ifdef CONFIG_STATIC_SHM
> +extern struct domain *dom_shared;
> +#else
> +#define dom_shared NULL
> +#endif
I think this should probably go to xen/include/xen/mm.h to stay close to
the others (dom_xen, dom_io and dom_cow).
> /*
> * Is the domain using the host memory layout?
> *
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d5d0792ed4..f6a3b04958 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
> return ( !dom0found && domUfound );
> }
>
> +#ifdef CONFIG_STATIC_SHM
> +static void __init setup_shared_domain(void)
> +{
> + /*
> + * Initialise our DOMID_SHARED domain.
> + * This domain owns statically shared pages when owner domain is not
> + * explicitly defined.
> + */
> + dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
> + if ( IS_ERR(dom_shared) )
> + panic("Failed to create d[SHARED]: %ld\n", PTR_ERR(dom_shared));
> +}
> +#endif
> +
> size_t __read_mostly dcache_line_bytes;
>
> /* C entry point for boot CPU */
> @@ -1022,6 +1036,14 @@ void __init start_xen(unsigned long boot_phys_offset,
> apply_alternatives_all();
> enable_errata_workarounds();
>
> +#ifdef CONFIG_STATIC_SHM
> + /*
> + * This needs to be called **after** setup_virt_paging so shared
> + * domains could successfully do p2m initialization.
^ domain
I take you are talking about DOMID_SHARED rather than any domain sharing
memory statically. Maybe it clearer if you say "so DOMID_SHARED could
successfully do p2m initialization".
> + */
> + setup_shared_domain();
> +#endif
> +
> /* Create initial domain 0. */
> if ( !is_dom0less_mode() )
> create_dom0();
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3742322d22..5cdd0b9f5b 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -643,11 +643,14 @@ struct domain *domain_create(domid_t domid,
>
> rangeset_domain_initialise(d);
>
> - /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
> - if ( is_system_domain(d) && !is_idle_domain(d) )
> + /*
> + * DOMID_{XEN,IO,etc} (other than IDLE and DOMID_shared) are
> + * sufficiently constructed.
> + */
> + if ( is_system_domain(d) && !is_idle_domain(d) && !is_shared_domain(d) )
> return d;
>
> - if ( !is_idle_domain(d) )
> + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> {
> if ( !is_hardware_domain(d) )
> d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> @@ -663,7 +666,7 @@ struct domain *domain_create(domid_t domid,
> goto fail;
> init_status |= INIT_arch;
>
> - if ( !is_idle_domain(d) )
> + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> {
> watchdog_domain_init(d);
> init_status |= INIT_watchdog;
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index f8749b0787..e5e357969d 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2616,6 +2616,11 @@ struct domain *get_pg_owner(domid_t domid)
>
> switch ( domid )
> {
> +#ifdef CONFIG_STATIC_SHM
> + case DOMID_SHARED:
> + pg_owner = rcu_lock_domain(dom_shared);
> + break;
> +#endif
> case DOMID_IO:
> pg_owner = rcu_lock_domain(dom_io);
> break;
> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
> index b278961cc3..a22854001b 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -359,10 +359,11 @@ static char *print_domain(char *str, const char *end, const struct domain *d)
>
> switch ( d->domain_id )
> {
> - case DOMID_IO: name = "[IO]"; break;
> - case DOMID_XEN: name = "[XEN]"; break;
> - case DOMID_COW: name = "[COW]"; break;
> - case DOMID_IDLE: name = "[IDLE]"; break;
> + case DOMID_IO: name = "[IO]"; break;
> + case DOMID_XEN: name = "[XEN]"; break;
> + case DOMID_COW: name = "[COW]"; break;
> + case DOMID_IDLE: name = "[IDLE]"; break;
> + case DOMID_SHARED: name = "[SHARED]"; break;
> /*
> * In principle, we could ASSERT_UNREACHABLE() in the default case.
> * However, this path is used to print out crash information, which
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index e373592c33..2e00741f09 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -612,6 +612,12 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> /* DOMID_INVALID is used to identify pages with unknown owner. */
> #define DOMID_INVALID xen_mk_uint(0x7FF4)
>
> +/*
> + * DOMID_SHARED is used as the owner of statically shared pages, when
> + * owner is not explicitly defined.
> + */
> +#define DOMID_SHARED xen_mk_uint(0x7FF5)
> +
> /* Idle domain. */
> #define DOMID_IDLE xen_mk_uint(0x7FFF)
>
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 24a9a87f83..2fb236f4ea 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -618,6 +618,8 @@ static inline bool is_system_domain(const struct domain *d)
> return d->domain_id >= DOMID_FIRST_RESERVED;
> }
>
> +#define is_shared_domain(d) ((d)->domain_id == DOMID_SHARED)
> +
> #define DOMAIN_DESTROYED (1u << 31) /* assumes atomic_t is >= 32 bits */
> #define put_domain(_d) \
> if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-18 1:59 ` Stefano Stabellini
@ 2022-03-18 6:43 ` Penny Zheng
2022-03-18 22:02 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-18 6:43 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, Penny Zheng, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Hi Stefano
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Friday, March 18, 2022 9:59 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Penny Zheng
> <penzhe01@a011292.shanghai.arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.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 v1 02/13] xen/arm: introduce a special domain
> DOMID_SHARED
>
> On Fri, 11 Mar 2022, Penny Zheng wrote:
> > From: Penny Zheng <penzhe01@a011292.shanghai.arm.com>
> >
> > In case to own statically shared pages when owner domain is not
> > explicitly defined, this commits propose a special domain
> > DOMID_SHARED, and we assign it 0x7FF5, as one of the system domains.
> >
> > Statically shared memory reuses the same way of initialization with
> > static memory, hence this commits proposes a new Kconfig
> > CONFIG_STATIC_SHM to wrap related codes, and this option depends on
> static memory(CONFIG_STATIC_MEMORY).
>
> Why does it depend on CONFIG_STATIC_MEMORY? This is a genuine question,
> I am not trying to scope-creep the series. Is there an actual technical
> dependency on CONFIG_STATIC_MEMORY? If not, it would be super useful to
> be able to share memory statically even between normal dom0less guests (of
> course it would be responsibility of the user to provide the right addresses and
> avoid mapping clashes.) I know that some of our users have requested this
> feature in the past.
>
I may find a proper way to rephrase here. My poor English writing skill...
When I implemented domain on static allocation, statically configured guest RAM is
treated as static memory in Xen and I introduced a few helpers to initialize/allocate/free
static memory, like acquire_staticmem_pages, etc, and all these helpers are guarded with
CONFIG_STATIC_MEMORY.
I want to reuse these helpers on static shared memory, so CONFIG_STATIC_SHM depends
on CONFIG_STATIC_MEMORY.
So I'm not restricting sharing static memory between domain on static allocation, current
Implementation is also useful to normal dom0less guests.
>
> > We intends to do shared domain creation after setup_virt_paging so
> > shared domain could successfully do p2m initialization.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/Kconfig | 7 +++++++
> > xen/arch/arm/domain.c | 12 ++++++++++--
> > xen/arch/arm/include/asm/domain.h | 6 ++++++
> > xen/arch/arm/setup.c | 22 ++++++++++++++++++++++
> > xen/common/domain.c | 11 +++++++----
> > xen/common/page_alloc.c | 5 +++++
> > xen/common/vsprintf.c | 9 +++++----
> > xen/include/public/xen.h | 6 ++++++
> > xen/include/xen/sched.h | 2 ++
> > 9 files changed, 70 insertions(+), 10 deletions(-)
> >
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > ecfa6822e4..c54accefb1 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -106,6 +106,13 @@ config TEE
> >
> > source "arch/arm/tee/Kconfig"
> >
> > +config STATIC_SHM
> > + bool "Statically shared memory on a dom0less system" if UNSUPPORTED
> > + depends on STATIC_MEMORY
> > + default n
> > + 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/domain.c b/xen/arch/arm/domain.c index
> > 8110c1df86..1ff1df5d3f 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -44,6 +44,10 @@
> >
> > DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +struct domain *__read_mostly dom_shared; #endif
>
> This one should probably go to xen/common/domain.c to stay close to the
> other special domains.
>
Ack. Thx
>
> > static void do_idle(void)
> > {
> > unsigned int cpu = smp_processor_id(); @@ -703,7 +707,7 @@ int
> > arch_domain_create(struct domain *d,
> > if ( is_idle_domain(d) )
> > return 0;
> >
> > - ASSERT(config != NULL);
> > + ASSERT(is_shared_domain(d) ? config == NULL : config != NULL);
> >
> > #ifdef CONFIG_IOREQ_SERVER
> > ioreq_domain_init(d);
> > @@ -712,12 +716,16 @@ int arch_domain_create(struct domain *d,
> > d->arch.directmap = flags & CDF_directmap;
> >
> > /* p2m_init relies on some value initialized by the IOMMU subsystem */
> > - if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> > + if ( (rc = iommu_domain_init(d, is_shared_domain(d) ? 0 :
> > + config->iommu_opts)) != 0 )
> > goto fail;
> >
> > if ( (rc = p2m_init(d)) != 0 )
> > goto fail;
> >
> > + /* DOMID_shared is sufficiently constructed after p2m initialization. */
> > + if ( is_shared_domain(d) )
> > + return 0;
> > +
> > rc = -ENOMEM;
> > if ( (d->shared_info = alloc_xenheap_pages(0, 0)) == NULL )
> > goto fail;
> > diff --git a/xen/arch/arm/include/asm/domain.h
> > b/xen/arch/arm/include/asm/domain.h
> > index c56f6e4398..ea7a7219a3 100644
> > --- a/xen/arch/arm/include/asm/domain.h
> > +++ b/xen/arch/arm/include/asm/domain.h
> > @@ -31,6 +31,12 @@ enum domain_type {
> >
> > #define is_domain_direct_mapped(d) (d)->arch.directmap
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +extern struct domain *dom_shared;
> > +#else
> > +#define dom_shared NULL
> > +#endif
>
> I think this should probably go to xen/include/xen/mm.h to stay close to the
> others (dom_xen, dom_io and dom_cow).
>
Ack, thx
>
> > /*
> > * Is the domain using the host memory layout?
> > *
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index
> > d5d0792ed4..f6a3b04958 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
> > return ( !dom0found && domUfound ); }
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +static void __init setup_shared_domain(void) {
> > + /*
> > + * Initialise our DOMID_SHARED domain.
> > + * This domain owns statically shared pages when owner domain is not
> > + * explicitly defined.
> > + */
> > + dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
> > + if ( IS_ERR(dom_shared) )
> > + panic("Failed to create d[SHARED]: %ld\n",
> > +PTR_ERR(dom_shared)); } #endif
> > +
> > size_t __read_mostly dcache_line_bytes;
> >
> > /* C entry point for boot CPU */
> > @@ -1022,6 +1036,14 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> > apply_alternatives_all();
> > enable_errata_workarounds();
> >
> > +#ifdef CONFIG_STATIC_SHM
> > + /*
> > + * This needs to be called **after** setup_virt_paging so shared
> > + * domains could successfully do p2m initialization.
> ^ domain
>
> I take you are talking about DOMID_SHARED rather than any domain sharing
> memory statically. Maybe it clearer if you say "so DOMID_SHARED could
> successfully do p2m initialization".
>
Ack, thx.
>
> > + */
> > + setup_shared_domain();
> > +#endif
> > +
> > /* Create initial domain 0. */
> > if ( !is_dom0less_mode() )
> > create_dom0();
> > diff --git a/xen/common/domain.c b/xen/common/domain.c index
> > 3742322d22..5cdd0b9f5b 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -643,11 +643,14 @@ struct domain *domain_create(domid_t domid,
> >
> > rangeset_domain_initialise(d);
> >
> > - /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
> > - if ( is_system_domain(d) && !is_idle_domain(d) )
> > + /*
> > + * DOMID_{XEN,IO,etc} (other than IDLE and DOMID_shared) are
> > + * sufficiently constructed.
> > + */
> > + if ( is_system_domain(d) && !is_idle_domain(d) &&
> > + !is_shared_domain(d) )
> > return d;
> >
> > - if ( !is_idle_domain(d) )
> > + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> > {
> > if ( !is_hardware_domain(d) )
> > d->nr_pirqs = nr_static_irqs + extra_domU_irqs; @@ -663,7
> > +666,7 @@ struct domain *domain_create(domid_t domid,
> > goto fail;
> > init_status |= INIT_arch;
> >
> > - if ( !is_idle_domain(d) )
> > + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> > {
> > watchdog_domain_init(d);
> > init_status |= INIT_watchdog; diff --git
> > a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > f8749b0787..e5e357969d 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2616,6 +2616,11 @@ struct domain *get_pg_owner(domid_t domid)
> >
> > switch ( domid )
> > {
> > +#ifdef CONFIG_STATIC_SHM
> > + case DOMID_SHARED:
> > + pg_owner = rcu_lock_domain(dom_shared);
> > + break;
> > +#endif
> > case DOMID_IO:
> > pg_owner = rcu_lock_domain(dom_io);
> > break;
> > diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c index
> > b278961cc3..a22854001b 100644
> > --- a/xen/common/vsprintf.c
> > +++ b/xen/common/vsprintf.c
> > @@ -359,10 +359,11 @@ static char *print_domain(char *str, const char
> > *end, const struct domain *d)
> >
> > switch ( d->domain_id )
> > {
> > - case DOMID_IO: name = "[IO]"; break;
> > - case DOMID_XEN: name = "[XEN]"; break;
> > - case DOMID_COW: name = "[COW]"; break;
> > - case DOMID_IDLE: name = "[IDLE]"; break;
> > + case DOMID_IO: name = "[IO]"; break;
> > + case DOMID_XEN: name = "[XEN]"; break;
> > + case DOMID_COW: name = "[COW]"; break;
> > + case DOMID_IDLE: name = "[IDLE]"; break;
> > + case DOMID_SHARED: name = "[SHARED]"; break;
> > /*
> > * In principle, we could ASSERT_UNREACHABLE() in the default case.
> > * However, this path is used to print out crash information,
> > which diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index e373592c33..2e00741f09 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -612,6 +612,12 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> > /* DOMID_INVALID is used to identify pages with unknown owner. */
> > #define DOMID_INVALID xen_mk_uint(0x7FF4)
> >
> > +/*
> > + * DOMID_SHARED is used as the owner of statically shared pages, when
> > + * owner is not explicitly defined.
> > + */
> > +#define DOMID_SHARED xen_mk_uint(0x7FF5)
> > +
> > /* Idle domain. */
> > #define DOMID_IDLE xen_mk_uint(0x7FFF)
> >
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index
> > 24a9a87f83..2fb236f4ea 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -618,6 +618,8 @@ static inline bool is_system_domain(const struct
> domain *d)
> > return d->domain_id >= DOMID_FIRST_RESERVED; }
> >
> > +#define is_shared_domain(d) ((d)->domain_id == DOMID_SHARED)
> > +
> > #define DOMAIN_DESTROYED (1u << 31) /* assumes atomic_t is >= 32 bits
> > */ #define put_domain(_d) \
> > if ( atomic_dec_and_test(&(_d)->refcnt) ) domain_destroy(_d)
> > --
> > 2.25.1
> >
Cheers,
---
Penny Zheng
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-18 6:43 ` Penny Zheng
@ 2022-03-18 22:02 ` Stefano Stabellini
0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 22:02 UTC (permalink / raw)
To: Penny Zheng
Cc: Stefano Stabellini, xen-devel, nd, Penny Zheng, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
On Fri, 18 Mar 2022, Penny Zheng wrote:
> > On Fri, 11 Mar 2022, Penny Zheng wrote:
> > > From: Penny Zheng <penzhe01@a011292.shanghai.arm.com>
> > >
> > > In case to own statically shared pages when owner domain is not
> > > explicitly defined, this commits propose a special domain
> > > DOMID_SHARED, and we assign it 0x7FF5, as one of the system domains.
> > >
> > > Statically shared memory reuses the same way of initialization with
> > > static memory, hence this commits proposes a new Kconfig
> > > CONFIG_STATIC_SHM to wrap related codes, and this option depends on
> > static memory(CONFIG_STATIC_MEMORY).
> >
> > Why does it depend on CONFIG_STATIC_MEMORY? This is a genuine question,
> > I am not trying to scope-creep the series. Is there an actual technical
> > dependency on CONFIG_STATIC_MEMORY? If not, it would be super useful to
> > be able to share memory statically even between normal dom0less guests (of
> > course it would be responsibility of the user to provide the right addresses and
> > avoid mapping clashes.) I know that some of our users have requested this
> > feature in the past.
> >
>
> I may find a proper way to rephrase here. My poor English writing skill...
> When I implemented domain on static allocation, statically configured guest RAM is
> treated as static memory in Xen and I introduced a few helpers to initialize/allocate/free
> static memory, like acquire_staticmem_pages, etc, and all these helpers are guarded with
> CONFIG_STATIC_MEMORY.
> I want to reuse these helpers on static shared memory, so CONFIG_STATIC_SHM depends
> on CONFIG_STATIC_MEMORY.
>
> So I'm not restricting sharing static memory between domain on static allocation, current
> Implementation is also useful to normal dom0less guests.
Ah, excellent! That makes sense.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-11 6:11 ` [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED Penny Zheng
2022-03-18 1:59 ` Stefano Stabellini
@ 2022-03-18 8:53 ` Jan Beulich
2022-03-18 21:50 ` Stefano Stabellini
` (2 more replies)
1 sibling, 3 replies; 51+ messages in thread
From: Jan Beulich @ 2022-03-18 8:53 UTC (permalink / raw)
To: Penny Zheng
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel
On 11.03.2022 07:11, Penny Zheng wrote:
> In case to own statically shared pages when owner domain is not
> explicitly defined, this commits propose a special domain DOMID_SHARED,
> and we assign it 0x7FF5, as one of the system domains.
>
> Statically shared memory reuses the same way of initialization with static
> memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
>
> We intends to do shared domain creation after setup_virt_paging so shared
> domain could successfully do p2m initialization.
There's nothing said here, in the earlier patch, or in the cover letter
about the security aspects of this. There is a reason we haven't been
allowing arbitrary, un-supervised sharing of memory between domains. It
wants clarifying why e.g. grants aren't an option to achieve what you
need, and how you mean to establish which domains are / aren't permitted
to access any individual page owned by this domain.
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -106,6 +106,13 @@ config TEE
>
> source "arch/arm/tee/Kconfig"
>
> +config STATIC_SHM
> + bool "Statically shared memory on a dom0less system" if UNSUPPORTED
> + depends on STATIC_MEMORY
> + default n
Nit: "default n" is redundant and hence would imo better be omitted.
> @@ -712,12 +716,16 @@ int arch_domain_create(struct domain *d,
> d->arch.directmap = flags & CDF_directmap;
>
> /* p2m_init relies on some value initialized by the IOMMU subsystem */
> - if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> + if ( (rc = iommu_domain_init(d, is_shared_domain(d) ? 0 : config->iommu_opts)) != 0 )
Nit: Overlong line.
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
> return ( !dom0found && domUfound );
> }
>
> +#ifdef CONFIG_STATIC_SHM
> +static void __init setup_shared_domain(void)
> +{
> + /*
> + * Initialise our DOMID_SHARED domain.
> + * This domain owns statically shared pages when owner domain is not
> + * explicitly defined.
> + */
> + dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
> + if ( IS_ERR(dom_shared) )
> + panic("Failed to create d[SHARED]: %ld\n", PTR_ERR(dom_shared));
I don't think this should be a panic - the system ought to be able to
come up fine, just without actually using this domain. After all this
is an optional feature which may not actually be used.
Also, along the lines of what Stefano has said, this setting up of
the domain would also better live next to where the other special
domains are set up. And even if it was to remain here, ...
> @@ -1022,6 +1036,14 @@ void __init start_xen(unsigned long boot_phys_offset,
> apply_alternatives_all();
> enable_errata_workarounds();
>
> +#ifdef CONFIG_STATIC_SHM
> + /*
> + * This needs to be called **after** setup_virt_paging so shared
> + * domains could successfully do p2m initialization.
> + */
> + setup_shared_domain();
> +#endif
... the #ifdef-ary here should be avoided by moving the other
#ifdef inside the function body.
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -643,11 +643,14 @@ struct domain *domain_create(domid_t domid,
>
> rangeset_domain_initialise(d);
>
> - /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
> - if ( is_system_domain(d) && !is_idle_domain(d) )
> + /*
> + * DOMID_{XEN,IO,etc} (other than IDLE and DOMID_shared) are
> + * sufficiently constructed.
> + */
> + if ( is_system_domain(d) && !is_idle_domain(d) && !is_shared_domain(d) )
> return d;
>
> - if ( !is_idle_domain(d) )
> + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> {
> if ( !is_hardware_domain(d) )
> d->nr_pirqs = nr_static_irqs + extra_domU_irqs;
> @@ -663,7 +666,7 @@ struct domain *domain_create(domid_t domid,
> goto fail;
> init_status |= INIT_arch;
>
> - if ( !is_idle_domain(d) )
> + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> {
> watchdog_domain_init(d);
> init_status |= INIT_watchdog;
All of these extra is_shared_domain() are quite ugly to see added.
First and foremost going this route doesn't scale very well - consider
how the code will look like when two more special domains with special
needs would be added. I think you want to abstract this some by
introducing one (or a small set of) new is_...() or e.g. needs_...()
predicates.
Further (there's no particularly good place to mention this) I'm
afraid I don't view "shared" as a good name: It's not the domain
which is shared, but it's the domain to hold shared memory. For this
my first consideration would be to see whether an existing special
domain can be re-used; after all the set of reserved domain IDs is
a very limited one, and hence each value taken from there should come
with a very good reason. We did such re-use e.g. when introducing
quarantining for PCI devices, by associating them with DOM_IO rather
than inventing a new DOM_QUARANTINE. If there are good reasons
speaking against such re-use, then I'd like to ask to consider e.g.
DOMID_SHM / DOMID_SHMEM plus associated predicate.
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2616,6 +2616,11 @@ struct domain *get_pg_owner(domid_t domid)
>
> switch ( domid )
> {
> +#ifdef CONFIG_STATIC_SHM
> + case DOMID_SHARED:
> + pg_owner = rcu_lock_domain(dom_shared);
> + break;
> +#endif
Please can you avoid #ifdef in cases like this one, by instead using
case DOMID_SHMEM:
pg_owner = dom_shared ? rcu_lock_domain(dom_shared) : NULL;
break;
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -618,6 +618,8 @@ static inline bool is_system_domain(const struct domain *d)
> return d->domain_id >= DOMID_FIRST_RESERVED;
> }
>
> +#define is_shared_domain(d) ((d)->domain_id == DOMID_SHARED)
Would this better evaluate to "false" when !STATIC_SHM, such that
the compiler can eliminate respective conditionals and/or code?
Jan
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-18 8:53 ` Jan Beulich
@ 2022-03-18 21:50 ` Stefano Stabellini
2022-03-21 8:48 ` Jan Beulich
2022-03-18 22:20 ` Stefano Stabellini
2022-04-15 9:52 ` Penny Zheng
2 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 21:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Penny Zheng, nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel
On Fri, 18 Mar 2022, Jan Beulich wrote:
> On 11.03.2022 07:11, Penny Zheng wrote:
> > In case to own statically shared pages when owner domain is not
> > explicitly defined, this commits propose a special domain DOMID_SHARED,
> > and we assign it 0x7FF5, as one of the system domains.
> >
> > Statically shared memory reuses the same way of initialization with static
> > memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> > related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
> >
> > We intends to do shared domain creation after setup_virt_paging so shared
> > domain could successfully do p2m initialization.
>
> There's nothing said here, in the earlier patch, or in the cover letter
> about the security aspects of this. There is a reason we haven't been
> allowing arbitrary, un-supervised sharing of memory between domains. It
> wants clarifying why e.g. grants aren't an option to achieve what you
> need, and how you mean to establish which domains are / aren't permitted
> to access any individual page owned by this domain.
I'll let Penny write a full reply but I'll chime in to try to help with
the explanation.
This is not arbitrary un-supervised sharing of memory between domains,
which indeed is concerning.
This is statically-configured, supervised by the system configurator,
sharing of memory between domains.
And in fact safety (which is just a different aspect of security) is one
of the primary goals for this work.
In safety-critical environments, 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 VM and they need to communication 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. It is faster too.
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
themselves.
Hopefully I made things clearer and not murkier :-)
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-18 21:50 ` Stefano Stabellini
@ 2022-03-21 8:48 ` Jan Beulich
2022-03-21 20:03 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Jan Beulich @ 2022-03-21 8:48 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Penny Zheng, nd, Penny Zheng, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
On 18.03.2022 22:50, Stefano Stabellini wrote:
> On Fri, 18 Mar 2022, Jan Beulich wrote:
>> On 11.03.2022 07:11, Penny Zheng wrote:
>>> In case to own statically shared pages when owner domain is not
>>> explicitly defined, this commits propose a special domain DOMID_SHARED,
>>> and we assign it 0x7FF5, as one of the system domains.
>>>
>>> Statically shared memory reuses the same way of initialization with static
>>> memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
>>> related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
>>>
>>> We intends to do shared domain creation after setup_virt_paging so shared
>>> domain could successfully do p2m initialization.
>>
>> There's nothing said here, in the earlier patch, or in the cover letter
>> about the security aspects of this. There is a reason we haven't been
>> allowing arbitrary, un-supervised sharing of memory between domains. It
>> wants clarifying why e.g. grants aren't an option to achieve what you
>> need, and how you mean to establish which domains are / aren't permitted
>> to access any individual page owned by this domain.
>
>
> I'll let Penny write a full reply but I'll chime in to try to help with
> the explanation.
>
> This is not arbitrary un-supervised sharing of memory between domains,
> which indeed is concerning.
>
> This is statically-configured, supervised by the system configurator,
> sharing of memory between domains.
>
> And in fact safety (which is just a different aspect of security) is one
> of the primary goals for this work.
>
> In safety-critical environments, 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 VM and they need to communication 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. It is faster too.
>
> 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
> themselves.
>
> Hopefully I made things clearer and not murkier :-)
It adds some helpful background, yes, but at the same time it doesn't
address the security concern at all: How are access permissions
managed when the owning domain is a special one? I haven't spotted
any recording of the domains which are actually permitted to map /
access the pages in questions. (But of course I also only looked at
non-Arm-specific code. I'd expect such code not to live in arch-
specific files.)
Jan
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-21 8:48 ` Jan Beulich
@ 2022-03-21 20:03 ` Stefano Stabellini
2022-04-09 9:11 ` Julien Grall
0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-21 20:03 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Penny Zheng, nd, Penny Zheng, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel
On Mon, 21 Mar 2022, Jan Beulich wrote:
> On 18.03.2022 22:50, Stefano Stabellini wrote:
> > On Fri, 18 Mar 2022, Jan Beulich wrote:
> >> On 11.03.2022 07:11, Penny Zheng wrote:
> >>> In case to own statically shared pages when owner domain is not
> >>> explicitly defined, this commits propose a special domain DOMID_SHARED,
> >>> and we assign it 0x7FF5, as one of the system domains.
> >>>
> >>> Statically shared memory reuses the same way of initialization with static
> >>> memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> >>> related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
> >>>
> >>> We intends to do shared domain creation after setup_virt_paging so shared
> >>> domain could successfully do p2m initialization.
> >>
> >> There's nothing said here, in the earlier patch, or in the cover letter
> >> about the security aspects of this. There is a reason we haven't been
> >> allowing arbitrary, un-supervised sharing of memory between domains. It
> >> wants clarifying why e.g. grants aren't an option to achieve what you
> >> need, and how you mean to establish which domains are / aren't permitted
> >> to access any individual page owned by this domain.
> >
> >
> > I'll let Penny write a full reply but I'll chime in to try to help with
> > the explanation.
> >
> > This is not arbitrary un-supervised sharing of memory between domains,
> > which indeed is concerning.
> >
> > This is statically-configured, supervised by the system configurator,
> > sharing of memory between domains.
> >
> > And in fact safety (which is just a different aspect of security) is one
> > of the primary goals for this work.
> >
> > In safety-critical environments, 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 VM and they need to communication 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. It is faster too.
> >
> > 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
> > themselves.
> >
> > Hopefully I made things clearer and not murkier :-)
>
> It adds some helpful background, yes, but at the same time it doesn't
> address the security concern at all: How are access permissions
> managed when the owning domain is a special one? I haven't spotted
> any recording of the domains which are actually permitted to map /
> access the pages in questions. (But of course I also only looked at
> non-Arm-specific code. I'd expect such code not to live in arch-
> specific files.)
All this static memory sharing is statically done at __init time only.
It should not be possible to trigger any further memory sharing at
runtime (if there is, that would be a bug). In the Arm patches, all the
related functions are marked as __init and only called at boot time.
They map the memory owned by DOMID_SHARED at given guest
pseudo-physical addresses, also specified in device tree.
There are no new interfaces for the guest to map this memory because it
is already "pre-mapped".
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-21 20:03 ` Stefano Stabellini
@ 2022-04-09 9:11 ` Julien Grall
2022-04-15 8:08 ` Penny Zheng
0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2022-04-09 9:11 UTC (permalink / raw)
To: Stefano Stabellini, Jan Beulich
Cc: Penny Zheng, nd, Penny Zheng, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
xen-devel
Hi Stefano,
On 21/03/2022 20:03, Stefano Stabellini wrote:
> On Mon, 21 Mar 2022, Jan Beulich wrote:
>> On 18.03.2022 22:50, Stefano Stabellini wrote:
>>> On Fri, 18 Mar 2022, Jan Beulich wrote:
>>>> On 11.03.2022 07:11, Penny Zheng wrote:
>>>>> In case to own statically shared pages when owner domain is not
>>>>> explicitly defined, this commits propose a special domain DOMID_SHARED,
>>>>> and we assign it 0x7FF5, as one of the system domains.
>>>>>
>>>>> Statically shared memory reuses the same way of initialization with static
>>>>> memory, hence this commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
>>>>> related codes, and this option depends on static memory(CONFIG_STATIC_MEMORY).
>>>>>
>>>>> We intends to do shared domain creation after setup_virt_paging so shared
>>>>> domain could successfully do p2m initialization.
>>>>
>>>> There's nothing said here, in the earlier patch, or in the cover letter
>>>> about the security aspects of this. There is a reason we haven't been
>>>> allowing arbitrary, un-supervised sharing of memory between domains. It
>>>> wants clarifying why e.g. grants aren't an option to achieve what you
>>>> need, and how you mean to establish which domains are / aren't permitted
>>>> to access any individual page owned by this domain.
>>>
>>>
>>> I'll let Penny write a full reply but I'll chime in to try to help with
>>> the explanation.
>>>
>>> This is not arbitrary un-supervised sharing of memory between domains,
>>> which indeed is concerning.
>>>
>>> This is statically-configured, supervised by the system configurator,
>>> sharing of memory between domains.
>>>
>>> And in fact safety (which is just a different aspect of security) is one
>>> of the primary goals for this work.
>>>
>>> In safety-critical environments, 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 VM and they need to communication 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. It is faster too.
>>>
>>> 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
>>> themselves.
>>>
>>> Hopefully I made things clearer and not murkier :-)
>>
>> It adds some helpful background, yes, but at the same time it doesn't
>> address the security concern at all: How are access permissions
>> managed when the owning domain is a special one? I haven't spotted
>> any recording of the domains which are actually permitted to map /
>> access the pages in questions. (But of course I also only looked at
>> non-Arm-specific code. I'd expect such code not to live in arch-
>> specific files.)
>
> All this static memory sharing is statically done at __init time only.
> It should not be possible to trigger any further memory sharing at
> runtime (if there is, that would be a bug).
Looking at the code, get_pg_owner() will be able to handle DOMID_SHARED.
So anyone that is permitted to access DOMID_SHARED will be able to map
any memory region at runtime.
> There are no new interfaces for the guest to map this memory because it
> is already "pre-mapped".
It can via XENMAPSPACE_gmfn_foreign (assuming proper permission).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-04-09 9:11 ` Julien Grall
@ 2022-04-15 8:08 ` Penny Zheng
2022-04-15 22:18 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-04-15 8:08 UTC (permalink / raw)
To: Julien Grall, Stefano Stabellini, Jan Beulich
Cc: nd, Penny Zheng, Bertrand Marquis, Volodymyr Babchuk,
Andrew Cooper, George Dunlap, Wei Liu, xen-devel, Wei Chen
Hi Julien and Stefano
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, April 9, 2022 5:12 PM
> To: Stefano Stabellini <sstabellini@kernel.org>; Jan Beulich
> <jbeulich@suse.com>
> Cc: Penny Zheng <Penny.Zheng@arm.com>; nd <nd@arm.com>; Penny Zheng
> <penzhe01@a011292.shanghai.arm.com>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1 02/13] xen/arm: introduce a special domain
> DOMID_SHARED
>
> Hi Stefano,
>
> On 21/03/2022 20:03, Stefano Stabellini wrote:
> > On Mon, 21 Mar 2022, Jan Beulich wrote:
> >> On 18.03.2022 22:50, Stefano Stabellini wrote:
> >>> On Fri, 18 Mar 2022, Jan Beulich wrote:
> >>>> On 11.03.2022 07:11, Penny Zheng wrote:
> >>>>> In case to own statically shared pages when owner domain is not
> >>>>> explicitly defined, this commits propose a special domain
> >>>>> DOMID_SHARED, and we assign it 0x7FF5, as one of the system domains.
> >>>>>
> >>>>> Statically shared memory reuses the same way of initialization
> >>>>> with static memory, hence this commits proposes a new Kconfig
> >>>>> CONFIG_STATIC_SHM to wrap related codes, and this option depends
> on static memory(CONFIG_STATIC_MEMORY).
> >>>>>
> >>>>> We intends to do shared domain creation after setup_virt_paging so
> >>>>> shared domain could successfully do p2m initialization.
> >>>>
> >>>> There's nothing said here, in the earlier patch, or in the cover
> >>>> letter about the security aspects of this. There is a reason we
> >>>> haven't been allowing arbitrary, un-supervised sharing of memory
> >>>> between domains. It wants clarifying why e.g. grants aren't an
> >>>> option to achieve what you need, and how you mean to establish
> >>>> which domains are / aren't permitted to access any individual page
> owned by this domain.
> >>>
> >>>
> >>> I'll let Penny write a full reply but I'll chime in to try to help
> >>> with the explanation.
> >>>
> >>> This is not arbitrary un-supervised sharing of memory between
> >>> domains, which indeed is concerning.
> >>>
> >>> This is statically-configured, supervised by the system
> >>> configurator, sharing of memory between domains.
> >>>
> >>> And in fact safety (which is just a different aspect of security) is
> >>> one of the primary goals for this work.
> >>>
> >>> In safety-critical environments, 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 VM and they need to communication 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. It is faster too.
> >>>
> >>> 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 themselves.
> >>>
> >>> Hopefully I made things clearer and not murkier :-)
> >>
> >> It adds some helpful background, yes, but at the same time it doesn't
> >> address the security concern at all: How are access permissions
> >> managed when the owning domain is a special one? I haven't spotted
> >> any recording of the domains which are actually permitted to map /
> >> access the pages in questions. (But of course I also only looked at
> >> non-Arm-specific code. I'd expect such code not to live in arch-
> >> specific files.)
> >
> > All this static memory sharing is statically done at __init time only.
> > It should not be possible to trigger any further memory sharing at
> > runtime (if there is, that would be a bug).
>
> Looking at the code, get_pg_owner() will be able to handle DOMID_SHARED.
> So anyone that is permitted to access DOMID_SHARED will be able to map any
> memory region at runtime.
>
> > There are no new interfaces for the guest to map this memory because
> > it is already "pre-mapped".
>
> It can via XENMAPSPACE_gmfn_foreign (assuming proper permission).
>
Correct me if I'm wrong:
The existing XENMAPSPACE_gmfn_foreign only allows privileged Dom0 to map
memory pages from one foreign DomU to itself. So It can happen that Dom0 is
using XENMAPSPACE_gmfn_foreign to (maliciously?) access shared memory owned
by DOMID_SHARED, and for now only Dom0 could.
So, maybe we should enhance the check of xsm_map_gmfn_foreign() to forbid the
access to DOMID_SHARED, hmm, but static shared memory region could actually be owned
by any arbitrary domain.
So, how about we add restriction on the page itself?
Pages of static shared memory region are static memory(with PGC_reserved flag on),
so maybe we could restrict XENMAPSPACE_gmfn_foreign to access any PGC_reserved pages?
> Cheers,
>
> --
Cheers,
--
Penny Zheng
> Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-04-15 8:08 ` Penny Zheng
@ 2022-04-15 22:18 ` Stefano Stabellini
2022-04-15 23:45 ` Julien Grall
0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-04-15 22:18 UTC (permalink / raw)
To: Penny Zheng
Cc: Julien Grall, Stefano Stabellini, Jan Beulich, nd, Penny Zheng,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel, Wei Chen
On Fri, 15 Apr 2022, Penny Zheng wrote:
> > Hi Stefano,
> >
> > On 21/03/2022 20:03, Stefano Stabellini wrote:
> > > On Mon, 21 Mar 2022, Jan Beulich wrote:
> > >> On 18.03.2022 22:50, Stefano Stabellini wrote:
> > >>> On Fri, 18 Mar 2022, Jan Beulich wrote:
> > >>>> On 11.03.2022 07:11, Penny Zheng wrote:
> > >>>>> In case to own statically shared pages when owner domain is not
> > >>>>> explicitly defined, this commits propose a special domain
> > >>>>> DOMID_SHARED, and we assign it 0x7FF5, as one of the system domains.
> > >>>>>
> > >>>>> Statically shared memory reuses the same way of initialization
> > >>>>> with static memory, hence this commits proposes a new Kconfig
> > >>>>> CONFIG_STATIC_SHM to wrap related codes, and this option depends
> > on static memory(CONFIG_STATIC_MEMORY).
> > >>>>>
> > >>>>> We intends to do shared domain creation after setup_virt_paging so
> > >>>>> shared domain could successfully do p2m initialization.
> > >>>>
> > >>>> There's nothing said here, in the earlier patch, or in the cover
> > >>>> letter about the security aspects of this. There is a reason we
> > >>>> haven't been allowing arbitrary, un-supervised sharing of memory
> > >>>> between domains. It wants clarifying why e.g. grants aren't an
> > >>>> option to achieve what you need, and how you mean to establish
> > >>>> which domains are / aren't permitted to access any individual page
> > owned by this domain.
> > >>>
> > >>>
> > >>> I'll let Penny write a full reply but I'll chime in to try to help
> > >>> with the explanation.
> > >>>
> > >>> This is not arbitrary un-supervised sharing of memory between
> > >>> domains, which indeed is concerning.
> > >>>
> > >>> This is statically-configured, supervised by the system
> > >>> configurator, sharing of memory between domains.
> > >>>
> > >>> And in fact safety (which is just a different aspect of security) is
> > >>> one of the primary goals for this work.
> > >>>
> > >>> In safety-critical environments, 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 VM and they need to communication 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. It is faster too.
> > >>>
> > >>> 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 themselves.
> > >>>
> > >>> Hopefully I made things clearer and not murkier :-)
> > >>
> > >> It adds some helpful background, yes, but at the same time it doesn't
> > >> address the security concern at all: How are access permissions
> > >> managed when the owning domain is a special one? I haven't spotted
> > >> any recording of the domains which are actually permitted to map /
> > >> access the pages in questions. (But of course I also only looked at
> > >> non-Arm-specific code. I'd expect such code not to live in arch-
> > >> specific files.)
> > >
> > > All this static memory sharing is statically done at __init time only.
> > > It should not be possible to trigger any further memory sharing at
> > > runtime (if there is, that would be a bug).
> >
> > Looking at the code, get_pg_owner() will be able to handle DOMID_SHARED.
> > So anyone that is permitted to access DOMID_SHARED will be able to map any
> > memory region at runtime.
> >
> > > There are no new interfaces for the guest to map this memory because
> > > it is already "pre-mapped".
> >
> > It can via XENMAPSPACE_gmfn_foreign (assuming proper permission).
> >
>
> Correct me if I'm wrong:
> The existing XENMAPSPACE_gmfn_foreign only allows privileged Dom0 to map
> memory pages from one foreign DomU to itself. So It can happen that Dom0 is
> using XENMAPSPACE_gmfn_foreign to (maliciously?) access shared memory owned
> by DOMID_SHARED, and for now only Dom0 could.
No, currently there is no protection against dom0 doing malicious
operations. Dom0 can poweroff the entire system.
If we are certain that only dom0 (and not other domains) can use
XENMAPSPACE_gmfn_foreign to access shared memory owned by DOMID_SHARED
then we are good. Looking at the code, and also considering that we have
agreed to move to DOMID_IO, get_pg_owner can already handle DOMID_IO.
Next is the xsm_map_gmfn_foreign(XSM_TARGET, d, od) check, which would
fail unless the asking domain is privileged over the target domain.
xsm_map_gmfn_foreign would fail for all domains except dom0.
So I think we are OK. I don't think we need to do anything else.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-04-15 22:18 ` Stefano Stabellini
@ 2022-04-15 23:45 ` Julien Grall
0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2022-04-15 23:45 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Andrew Cooper, Bertrand Marquis, George Dunlap, Jan Beulich,
Julien Grall, Penny Zheng, Penny Zheng, Volodymyr Babchuk,
Wei Chen, Wei Liu, nd, xen-devel
[-- Attachment #1: Type: text/plain, Size: 5899 bytes --]
Hi,
On Fri, 15 Apr 2022 at 22:19, Stefano Stabellini <sstabellini@kernel.org>
wrote:
> On Fri, 15 Apr 2022, Penny Zheng wrote:
> > > Hi Stefano,
> > >
> > > On 21/03/2022 20:03, Stefano Stabellini wrote:
> > > > On Mon, 21 Mar 2022, Jan Beulich wrote:
> > > >> On 18.03.2022 22:50, Stefano Stabellini wrote:
> > > >>> On Fri, 18 Mar 2022, Jan Beulich wrote:
> > > >>>> On 11.03.2022 07:11, Penny Zheng wrote:
> > > >>>>> In case to own statically shared pages when owner domain is not
> > > >>>>> explicitly defined, this commits propose a special domain
> > > >>>>> DOMID_SHARED, and we assign it 0x7FF5, as one of the system
> domains.
> > > >>>>>
> > > >>>>> Statically shared memory reuses the same way of initialization
> > > >>>>> with static memory, hence this commits proposes a new Kconfig
> > > >>>>> CONFIG_STATIC_SHM to wrap related codes, and this option depends
> > > on static memory(CONFIG_STATIC_MEMORY).
> > > >>>>>
> > > >>>>> We intends to do shared domain creation after setup_virt_paging
> so
> > > >>>>> shared domain could successfully do p2m initialization.
> > > >>>>
> > > >>>> There's nothing said here, in the earlier patch, or in the cover
> > > >>>> letter about the security aspects of this. There is a reason we
> > > >>>> haven't been allowing arbitrary, un-supervised sharing of memory
> > > >>>> between domains. It wants clarifying why e.g. grants aren't an
> > > >>>> option to achieve what you need, and how you mean to establish
> > > >>>> which domains are / aren't permitted to access any individual page
> > > owned by this domain.
> > > >>>
> > > >>>
> > > >>> I'll let Penny write a full reply but I'll chime in to try to help
> > > >>> with the explanation.
> > > >>>
> > > >>> This is not arbitrary un-supervised sharing of memory between
> > > >>> domains, which indeed is concerning.
> > > >>>
> > > >>> This is statically-configured, supervised by the system
> > > >>> configurator, sharing of memory between domains.
> > > >>>
> > > >>> And in fact safety (which is just a different aspect of security)
> is
> > > >>> one of the primary goals for this work.
> > > >>>
> > > >>> In safety-critical environments, 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 VM and they need to communication 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. It is faster
> too.
> > > >>>
> > > >>> 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 themselves.
> > > >>>
> > > >>> Hopefully I made things clearer and not murkier :-)
> > > >>
> > > >> It adds some helpful background, yes, but at the same time it
> doesn't
> > > >> address the security concern at all: How are access permissions
> > > >> managed when the owning domain is a special one? I haven't spotted
> > > >> any recording of the domains which are actually permitted to map /
> > > >> access the pages in questions. (But of course I also only looked at
> > > >> non-Arm-specific code. I'd expect such code not to live in arch-
> > > >> specific files.)
> > > >
> > > > All this static memory sharing is statically done at __init time
> only.
> > > > It should not be possible to trigger any further memory sharing at
> > > > runtime (if there is, that would be a bug).
> > >
> > > Looking at the code, get_pg_owner() will be able to handle
> DOMID_SHARED.
> > > So anyone that is permitted to access DOMID_SHARED will be able to map
> any
> > > memory region at runtime.
> > >
> > > > There are no new interfaces for the guest to map this memory because
> > > > it is already "pre-mapped".
> > >
> > > It can via XENMAPSPACE_gmfn_foreign (assuming proper permission).
> > >
> >
> > Correct me if I'm wrong:
> > The existing XENMAPSPACE_gmfn_foreign only allows privileged Dom0 to map
> > memory pages from one foreign DomU to itself. So It can happen that Dom0
> is
> > using XENMAPSPACE_gmfn_foreign to (maliciously?) access shared memory
> owned
> > by DOMID_SHARED, and for now only Dom0 could.
>
> No, currently there is no protection against dom0 doing malicious
> operations. Dom0 can poweroff the entire system.
This is the classic argument… Yes, on a default setup, dom0 is fully
trusted today. However there are way to reduce the trust via XSM.
For new interfaces we should also try to avoid considering dom0 as fully
trusted whenever it is possible. This is one of the example where I think
this should be done. The more if we use DOMID_IO as we may add more than
shared pages there…
>
> If we are certain that only dom0 (and not other domains) can use
> XENMAPSPACE_gmfn_foreign to access shared memory owned by DOMID_SHARED
> then we are good. Looking at the code, and also considering that we have
> agreed to move to DOMID_IO, get_pg_owner can already handle DOMID_IO.
AFAIK, dom0 cannot map DOMID_IO page for now because get_page_from_gfn
would not work. This would change with this approach.
>
> Next is the xsm_map_gmfn_foreign(XSM_TARGET, d, od) check, which would
> fail unless the asking domain is privileged over the target domain.
> xsm_map_gmfn_foreign would fail for all domains except dom0.
This depends your XSM policy. In this case, think we need to prevent
runtime mapping via the hypercall interface. We can relax it afterwards if
we have use cases for it.
>
> So I think we are OK. I don't think we need to do anything else.
Cheers,
--
Julien Grall
[-- Attachment #2: Type: text/html, Size: 8700 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-18 8:53 ` Jan Beulich
2022-03-18 21:50 ` Stefano Stabellini
@ 2022-03-18 22:20 ` Stefano Stabellini
2022-04-15 9:52 ` Penny Zheng
2 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 22:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Penny Zheng, nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel
On Fri, 18 Mar 2022, Jan Beulich wrote:
> Further (there's no particularly good place to mention this) I'm
> afraid I don't view "shared" as a good name: It's not the domain
> which is shared, but it's the domain to hold shared memory.
To be honest I thought the same thing when reading this patch but
couldn't easily come up with a better name.
> For this
> my first consideration would be to see whether an existing special
> domain can be re-used; after all the set of reserved domain IDs is
> a very limited one, and hence each value taken from there should come
> with a very good reason. We did such re-use e.g. when introducing
> quarantining for PCI devices, by associating them with DOM_IO rather
> than inventing a new DOM_QUARANTINE. If there are good reasons
> speaking against such re-use, then I'd like to ask to consider e.g.
> DOMID_SHM / DOMID_SHMEM plus associated predicate.
From my point of view is totally fine to reuse one of the existing
special domains if we can.
DOMID_IO seems to be the closest match but its current definition
doesn't cover what Penny is trying to achieve. I am happy to change its
definition and make it wider to support this use-case too.
It is not trivial to come up with an appropriate description though. I
took a stab at it but I couldn't come up with anything better:
* DOMID_IO is used for mapping memory and MMIO regions when no explicit
* Domain need to be specified.
*
* For instance, DOMID_IO is the owner of memory pre-shared among
* multiple domains at boot time, when no explicit owner is specified.
*
* Also, DOMID_IO is used to restrict page-table updates to mapping I/O
* memory. Although no Foreign Domain need be specified to map I/O
* pages, DOMID_IO is useful to ensure that no mappings to the OS's own
* heap are accidentally installed. (e.g., in Linux this could cause
* havoc as reference counts aren't adjusted on the I/O-mapping code
* path). This only makes sense as HYPERVISOR_mmu_update()'s and
* HYPERVISOR_update_va_mapping_otherdomain()'s "foreigndom" argument.
* For HYPERVISOR_mmu_update() context it can be specified by any
* calling domain, otherwise it's only permitted if the caller is
* privileged.
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-03-18 8:53 ` Jan Beulich
2022-03-18 21:50 ` Stefano Stabellini
2022-03-18 22:20 ` Stefano Stabellini
@ 2022-04-15 9:52 ` Penny Zheng
2022-04-15 23:34 ` Julien Grall
2022-04-19 8:10 ` Jan Beulich
2 siblings, 2 replies; 51+ messages in thread
From: Penny Zheng @ 2022-04-15 9:52 UTC (permalink / raw)
To: Jan Beulich
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel, Wei Chen
Hi jan
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, March 18, 2022 4:53 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: nd <nd@arm.com>; Penny Zheng
> <penzhe01@a011292.shanghai.arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.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>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1 02/13] xen/arm: introduce a special domain
> DOMID_SHARED
>
> On 11.03.2022 07:11, Penny Zheng wrote:
> > In case to own statically shared pages when owner domain is not
> > explicitly defined, this commits propose a special domain
> > DOMID_SHARED, and we assign it 0x7FF5, as one of the system domains.
> >
> > Statically shared memory reuses the same way of initialization with
> > static memory, hence this commits proposes a new Kconfig
> > CONFIG_STATIC_SHM to wrap related codes, and this option depends on
> static memory(CONFIG_STATIC_MEMORY).
> >
> > We intends to do shared domain creation after setup_virt_paging so
> > shared domain could successfully do p2m initialization.
>
> There's nothing said here, in the earlier patch, or in the cover letter about the
> security aspects of this. There is a reason we haven't been allowing arbitrary,
> un-supervised sharing of memory between domains. It wants clarifying why e.g.
> grants aren't an option to achieve what you need, and how you mean to
> establish which domains are / aren't permitted to access any individual page
> owned by this domain.
>
I'll add the security aspects what Stefano explains in the cover letter next serie
for better understanding.
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -106,6 +106,13 @@ config TEE
> >
> > source "arch/arm/tee/Kconfig"
> >
> > +config STATIC_SHM
> > + bool "Statically shared memory on a dom0less system" if UNSUPPORTED
> > + depends on STATIC_MEMORY
> > + default n
>
> Nit: "default n" is redundant and hence would imo better be omitted.
>
> > @@ -712,12 +716,16 @@ int arch_domain_create(struct domain *d,
> > d->arch.directmap = flags & CDF_directmap;
> >
> > /* p2m_init relies on some value initialized by the IOMMU subsystem */
> > - if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> > + if ( (rc = iommu_domain_init(d, is_shared_domain(d) ? 0 :
> > + config->iommu_opts)) != 0 )
>
> Nit: Overlong line.
>
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
> > return ( !dom0found && domUfound ); }
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +static void __init setup_shared_domain(void) {
> > + /*
> > + * Initialise our DOMID_SHARED domain.
> > + * This domain owns statically shared pages when owner domain is not
> > + * explicitly defined.
> > + */
> > + dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
> > + if ( IS_ERR(dom_shared) )
> > + panic("Failed to create d[SHARED]: %ld\n",
> > +PTR_ERR(dom_shared));
>
> I don't think this should be a panic - the system ought to be able to come up
> fine, just without actually using this domain. After all this is an optional
> feature which may not actually be used.
>
> Also, along the lines of what Stefano has said, this setting up of the domain
> would also better live next to where the other special domains are set up. And
> even if it was to remain here, ...
>
The reason why I place the setting up here is that DOMID_SHARED needs to map
pre-configured static shared memory in its p2m table, so it must be set up
after system P2M initialization(setup_virt_paging()). setup_system_domains()
is called before system P2M initialization on xen/arch/arm/setup.c, which
can't meet the requirement.
> > @@ -1022,6 +1036,14 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> > apply_alternatives_all();
> > enable_errata_workarounds();
> >
> > +#ifdef CONFIG_STATIC_SHM
> > + /*
> > + * This needs to be called **after** setup_virt_paging so shared
> > + * domains could successfully do p2m initialization.
> > + */
> > + setup_shared_domain();
> > +#endif
>
> ... the #ifdef-ary here should be avoided by moving the other #ifdef inside the
> function body.
>
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -643,11 +643,14 @@ struct domain *domain_create(domid_t domid,
> >
> > rangeset_domain_initialise(d);
> >
> > - /* DOMID_{XEN,IO,etc} (other than IDLE) are sufficiently constructed. */
> > - if ( is_system_domain(d) && !is_idle_domain(d) )
> > + /*
> > + * DOMID_{XEN,IO,etc} (other than IDLE and DOMID_shared) are
> > + * sufficiently constructed.
> > + */
> > + if ( is_system_domain(d) && !is_idle_domain(d) &&
> > + !is_shared_domain(d) )
> > return d;
> >
> > - if ( !is_idle_domain(d) )
> > + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> > {
> > if ( !is_hardware_domain(d) )
> > d->nr_pirqs = nr_static_irqs + extra_domU_irqs; @@ -663,7
> > +666,7 @@ struct domain *domain_create(domid_t domid,
> > goto fail;
> > init_status |= INIT_arch;
> >
> > - if ( !is_idle_domain(d) )
> > + if ( !is_idle_domain(d) && !is_shared_domain(d) )
> > {
> > watchdog_domain_init(d);
> > init_status |= INIT_watchdog;
>
> All of these extra is_shared_domain() are quite ugly to see added.
> First and foremost going this route doesn't scale very well - consider how the
> code will look like when two more special domains with special needs would
> be added. I think you want to abstract this some by introducing one (or a small
> set of) new is_...() or e.g. needs_...() predicates.
>
Agree. Shared domain needs the p2m initialization(p2m_init), which is
in arch_domain_create. So I will introduce a new helper needs_arch_domain_creation()
to include these system domains which need to call arch_domain_create to
be sufficiently constructed.
> Further (there's no particularly good place to mention this) I'm afraid I don't
> view "shared" as a good name: It's not the domain which is shared, but it's the
> domain to hold shared memory. For this my first consideration would be to
> see whether an existing special domain can be re-used; after all the set of
> reserved domain IDs is a very limited one, and hence each value taken from
> there should come with a very good reason. We did such re-use e.g. when
> introducing quarantining for PCI devices, by associating them with DOM_IO
> rather than inventing a new DOM_QUARANTINE. If there are good reasons
> speaking against such re-use, then I'd like to ask to consider e.g.
> DOMID_SHM / DOMID_SHMEM plus associated predicate.
>
I'll take stefano's suggestion to reuse DOMID_IO.
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2616,6 +2616,11 @@ struct domain *get_pg_owner(domid_t domid)
> >
> > switch ( domid )
> > {
> > +#ifdef CONFIG_STATIC_SHM
> > + case DOMID_SHARED:
> > + pg_owner = rcu_lock_domain(dom_shared);
> > + break;
> > +#endif
>
> Please can you avoid #ifdef in cases like this one, by instead using
>
> case DOMID_SHMEM:
> pg_owner = dom_shared ? rcu_lock_domain(dom_shared) : NULL;
> break;
>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -618,6 +618,8 @@ static inline bool is_system_domain(const struct
> domain *d)
> > return d->domain_id >= DOMID_FIRST_RESERVED; }
> >
> > +#define is_shared_domain(d) ((d)->domain_id == DOMID_SHARED)
>
> Would this better evaluate to "false" when !STATIC_SHM, such that the
> compiler can eliminate respective conditionals and/or code?
>
> Jan
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-04-15 9:52 ` Penny Zheng
@ 2022-04-15 23:34 ` Julien Grall
2022-04-19 8:10 ` Jan Beulich
1 sibling, 0 replies; 51+ messages in thread
From: Julien Grall @ 2022-04-15 23:34 UTC (permalink / raw)
To: Penny Zheng
Cc: Andrew Cooper, Bertrand Marquis, George Dunlap, Jan Beulich,
Julien Grall, Penny Zheng, Stefano Stabellini, Volodymyr Babchuk,
Wei Chen, Wei Liu, nd, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]
Hi Penny,
Sorry for the formatting.
On Fri, 15 Apr 2022 at 09:53, Penny Zheng <Penny.Zheng@arm.com> wrote:
> Hi jan
>
> > -----
> > > +#ifdef CONFIG_STATIC_SHM
> > > +static void __init setup_shared_domain(void) {
> > > + /*
> > > + * Initialise our DOMID_SHARED domain.
> > > + * This domain owns statically shared pages when owner domain is
> not
> > > + * explicitly defined.
> > > + */
> > > + dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
> > > + if ( IS_ERR(dom_shared) )
> > > + panic("Failed to create d[SHARED]: %ld\n",
> > > +PTR_ERR(dom_shared));
> >
> > I don't think this should be a panic - the system ought to be able to
> come up
> > fine, just without actually using this domain. After all this is an
> optional
> > feature which may not actually be used.
> >
> > Also, along the lines of what Stefano has said, this setting up of the
> domain
> > would also better live next to where the other special domains are set
> up. And
> > even if it was to remain here, ...
> >
>
> The reason why I place the setting up here is that DOMID_SHARED needs to
> map
> pre-configured static shared memory in its p2m table, so it must be set up
> after system P2M initialization(setup_virt_paging()).
> setup_system_domains()
> is called before system P2M initialization on xen/arch/arm/setup.c, which
> can't meet the requirement.
AFAIU, DOM_SHARED (or whatever you want to call it) will never run and the
GFN will always be equal to the MFN. So it sounds to me that creating a P2M
is a bit over the top. Instead, we should have a special case in
get_page_from_gfn like we did for DOM_XEN.
Cheers,
--
Julien Grall
[-- Attachment #2: Type: text/html, Size: 2567 bytes --]
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED
2022-04-15 9:52 ` Penny Zheng
2022-04-15 23:34 ` Julien Grall
@ 2022-04-19 8:10 ` Jan Beulich
1 sibling, 0 replies; 51+ messages in thread
From: Jan Beulich @ 2022-04-19 8:10 UTC (permalink / raw)
To: Penny Zheng
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Wei Liu, xen-devel, Wei Chen
On 15.04.2022 11:52, Penny Zheng wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, March 18, 2022 4:53 PM
>>
>> On 11.03.2022 07:11, Penny Zheng wrote:
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -855,6 +855,20 @@ static bool __init is_dom0less_mode(void)
>>> return ( !dom0found && domUfound ); }
>>>
>>> +#ifdef CONFIG_STATIC_SHM
>>> +static void __init setup_shared_domain(void) {
>>> + /*
>>> + * Initialise our DOMID_SHARED domain.
>>> + * This domain owns statically shared pages when owner domain is not
>>> + * explicitly defined.
>>> + */
>>> + dom_shared = domain_create(DOMID_SHARED, NULL, CDF_directmap);
>>> + if ( IS_ERR(dom_shared) )
>>> + panic("Failed to create d[SHARED]: %ld\n",
>>> +PTR_ERR(dom_shared));
>>
>> I don't think this should be a panic - the system ought to be able to come up
>> fine, just without actually using this domain. After all this is an optional
>> feature which may not actually be used.
>>
>> Also, along the lines of what Stefano has said, this setting up of the domain
>> would also better live next to where the other special domains are set up. And
>> even if it was to remain here, ...
>>
>
> The reason why I place the setting up here is that DOMID_SHARED needs to map
> pre-configured static shared memory in its p2m table, so it must be set up
> after system P2M initialization(setup_virt_paging()). setup_system_domains()
> is called before system P2M initialization on xen/arch/arm/setup.c, which
> can't meet the requirement.
While possibly moot with the plan to use DomIO (and my hope that you don't
mean to move DomIO's creation), I'd like to point out that there can't be
"too early" setting up of something. If it happens earlier than where you
need it, all you need to do is arrange for the further setup you mean to
add to be invoked separately, whenever it's time to do so.
Jan
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
2022-03-11 6:11 ` [PATCH v1 01/13] xen/arm: introduce static shared memory Penny Zheng
2022-03-11 6:11 ` [PATCH v1 02/13] xen/arm: introduce a special domain DOMID_SHARED Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 1:59 ` Stefano Stabellini
2022-03-11 6:11 ` [PATCH v1 04/13] xen/arm: add P2M type parameter in guest_physmap_add_pages Penny Zheng
` (9 subsequent siblings)
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
This commit introduces process_shm to cope with static shared memory in
domain construction.
This commit only considers allocating static shared memory to dom_shared
when owner domain is not explicitly defined in device tree, the other
scenario will be covered in the following patches.
Static shared memory could reuse acquire_static_memory_bank() to acquire
and allocate static memory.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 116 +++++++++++++++++++++++++++++++++++-
1 file changed, 115 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de..6e6349caac 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -527,7 +527,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 )
{
@@ -751,6 +752,113 @@ 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
+static __initdata DECLARE_BITMAP(shm_mask, NR_MEM_BANKS);
+
+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);
+
+}
+
+static int __init allocate_shared_memory(struct domain *d,
+ u32 addr_cells, u32 size_cells,
+ paddr_t pbase, paddr_t psize,
+ paddr_t gbase)
+{
+ mfn_t smfn;
+ int ret = 0;
+
+ printk(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;
+
+ ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize));
+ if ( ret )
+ {
+ dprintk(XENLOG_ERR, "Failed to map shared memory to %pd.\n", d);
+ return ret;
+ }
+
+ return ret;
+}
+
+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_shared. */
+ /*
+ * Per shared memory region could be shared between multiple domains.
+ * In case re-allocating the same shared memory region, we use bitmask
+ * shm_mask to record whether this shared memory region has ever been
+ * allocated already.
+ */
+ if ( !test_bit(shm_id, shm_mask) )
+ {
+ /*
+ * Allocate statically shared pages to the default dom_shared.
+ * Set up P2M, and dom_shared is a direct-map domain,
+ * so GFN == PFN.
+ */
+ ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
+ pbase, psize, pbase);
+ if ( ret )
+ return ret;
+
+ set_bit(shm_id, shm_mask);
+ }
+ }
+
+ return 0;
+}
+#endif /* CONFIG_STATIC_SHM */
#else
static void __init allocate_static_memory(struct domain *d,
struct kernel_info *kinfo,
@@ -3150,6 +3258,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
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared
2022-03-11 6:11 ` [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared Penny Zheng
@ 2022-03-18 1:59 ` Stefano Stabellini
2022-03-18 8:35 ` Penny Zheng
0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 1:59 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> This commit introduces process_shm to cope with static shared memory in
> domain construction.
>
> This commit only considers allocating static shared memory to dom_shared
> when owner domain is not explicitly defined in device tree, the other
> scenario will be covered in the following patches.
>
> Static shared memory could reuse acquire_static_memory_bank() to acquire
> and allocate static memory.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 116 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 115 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8be01678de..6e6349caac 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -527,7 +527,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);
Why this change?
> ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
> if ( PFN_DOWN(*psize) > UINT_MAX )
> {
> @@ -751,6 +752,113 @@ 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
> +static __initdata DECLARE_BITMAP(shm_mask, NR_MEM_BANKS);
> +
> +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);
> +
> +}
> +
> +static int __init allocate_shared_memory(struct domain *d,
> + u32 addr_cells, u32 size_cells,
> + paddr_t pbase, paddr_t psize,
> + paddr_t gbase)
> +{
> + mfn_t smfn;
> + int ret = 0;
> +
> + printk(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;
> +
> + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize));
> + if ( ret )
> + {
> + dprintk(XENLOG_ERR, "Failed to map shared memory to %pd.\n", d);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +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_shared. */
> + /*
> + * Per shared memory region could be shared between multiple domains.
> + * In case re-allocating the same shared memory region, we use bitmask
> + * shm_mask to record whether this shared memory region has ever been
> + * allocated already.
> + */
> + if ( !test_bit(shm_id, shm_mask) )
> + {
> + /*
> + * Allocate statically shared pages to the default dom_shared.
> + * Set up P2M, and dom_shared is a direct-map domain,
> + * so GFN == PFN.
> + */
> + ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
> + pbase, psize, pbase);
^gbase
The last parameter should be gbase instead of pbase?
Reading this patch is not clear that only the "owner" code path is
implemented here. The "borrower" code path is implemented later and
missing in this patch. I think it would be good to clarify that in the
commit message.
Under this light, allocate_shared_memory is supposed to be only called
for the owner. I think we should probably mention that in the in-code
comment too.
I don't think we need to define a second copy of shm_mask. Can we reuse
the one in bootfdt.c?
Or we could get rid of shm_mask entirely here if we check whether the
pages were already allocated in the owner p2m.
> + if ( ret )
> + return ret;
> +
> + set_bit(shm_id, shm_mask);
> + }
> + }
> +
> + return 0;
> +}
> +#endif /* CONFIG_STATIC_SHM */
> #else
> static void __init allocate_static_memory(struct domain *d,
> struct kernel_info *kinfo,
> @@ -3150,6 +3258,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
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared
2022-03-18 1:59 ` Stefano Stabellini
@ 2022-03-18 8:35 ` Penny Zheng
2022-03-18 22:27 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-18 8:35 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Friday, March 18, 2022 10:00 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 03/13] xen/arm: allocate static shared memory to
> dom_shared
>
> On Fri, 11 Mar 2022, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This commit introduces process_shm to cope with static shared memory
> > in domain construction.
> >
> > This commit only considers allocating static shared memory to
> > dom_shared when owner domain is not explicitly defined in device tree,
> > the other scenario will be covered in the following patches.
> >
> > Static shared memory could reuse acquire_static_memory_bank() to
> > acquire and allocate static memory.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain_build.c | 116
> > +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 115 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 8be01678de..6e6349caac 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -527,7 +527,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);
>
> Why this change?
>
This helper is also used for acquiring static memory as guest RAM for statically configured
domain.
And since we are reusing it for static shared memory, but try to avoid parsing the property
here, the "xen,static-shm" property getting parsed in different ways in process_shm.
So this change is needed here.
And I think I need to add in-code comment to explain. ;)
>
> > ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> PAGE_SIZE));
> > if ( PFN_DOWN(*psize) > UINT_MAX )
> > {
> > @@ -751,6 +752,113 @@ 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
> > +static __initdata DECLARE_BITMAP(shm_mask, NR_MEM_BANKS);
> > +
> > +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);
> > +
> > +}
> > +
> > +static int __init allocate_shared_memory(struct domain *d,
> > + u32 addr_cells, u32 size_cells,
> > + paddr_t pbase, paddr_t psize,
> > + paddr_t gbase) {
> > + mfn_t smfn;
> > + int ret = 0;
> > +
> > + printk(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;
> > +
> > + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
> PFN_DOWN(psize));
> > + if ( ret )
> > + {
> > + dprintk(XENLOG_ERR, "Failed to map shared memory to %pd.\n", d);
> > + return ret;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +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_shared. */
> > + /*
> > + * Per shared memory region could be shared between multiple
> domains.
> > + * In case re-allocating the same shared memory region, we use bitmask
> > + * shm_mask to record whether this shared memory region has ever
> been
> > + * allocated already.
> > + */
> > + if ( !test_bit(shm_id, shm_mask) )
> > + {
> > + /*
> > + * Allocate statically shared pages to the default dom_shared.
> > + * Set up P2M, and dom_shared is a direct-map domain,
> > + * so GFN == PFN.
> > + */
> > + ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
> > + pbase, psize, pbase);
> ^gbase
>
> The last parameter should be gbase instead of pbase?
>
Yes, and since dom_shared is a direct-map domain, GFN == PFN, so pbase should be
ok here. I mentioned it on comments.
And Why I make dom_shared direct-map domain is that in this way we don't need to decode
owner GFN when doing foreign memory mapping for borrower domain.
>
> Reading this patch is not clear that only the "owner" code path is
> implemented here. The "borrower" code path is implemented later and
> missing in this patch. I think it would be good to clarify that in the commit
> message.
>
> Under this light, allocate_shared_memory is supposed to be only called for the
> owner. I think we should probably mention that in the in-code comment too.
>
Yes, only owner domain could allocate memory, I'll add it to in-code comment.
> I don't think we need to define a second copy of shm_mask. Can we reuse the
> one in bootfdt.c?
>
Yes, maybe I should reuse than reintroduce. And before using the bitmap here,
I need to clear it totally to clean away all the stale info from bootfdt.c.
> Or we could get rid of shm_mask entirely here if we check whether the pages
> were already allocated in the owner p2m.
>
>
Hmm, that means that we need to do the page walk each time? That's kinds of
time-consuming, or am I missing some convenient way to check?
> > + if ( ret )
> > + return ret;
> > +
> > + set_bit(shm_id, shm_mask);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +#endif /* CONFIG_STATIC_SHM */
> > #else
> > static void __init allocate_static_memory(struct domain *d,
> > struct kernel_info *kinfo,
> > @@ -3150,6 +3258,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
> > --
> > 2.25.1
Cheers,
---
Penny Zheng
> >
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared
2022-03-18 8:35 ` Penny Zheng
@ 2022-03-18 22:27 ` Stefano Stabellini
0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 22:27 UTC (permalink / raw)
To: Penny Zheng
Cc: Stefano Stabellini, xen-devel, nd, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 18 Mar 2022, Penny Zheng wrote:
> > On Fri, 11 Mar 2022, Penny Zheng wrote:
> > > From: Penny Zheng <penny.zheng@arm.com>
> > >
> > > This commit introduces process_shm to cope with static shared memory
> > > in domain construction.
> > >
> > > This commit only considers allocating static shared memory to
> > > dom_shared when owner domain is not explicitly defined in device tree,
> > > the other scenario will be covered in the following patches.
> > >
> > > Static shared memory could reuse acquire_static_memory_bank() to
> > > acquire and allocate static memory.
> > >
> > > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > > ---
> > > xen/arch/arm/domain_build.c | 116
> > > +++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 115 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 8be01678de..6e6349caac 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -527,7 +527,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);
> >
> > Why this change?
> >
>
> This helper is also used for acquiring static memory as guest RAM for statically configured
> domain.
>
> And since we are reusing it for static shared memory, but try to avoid parsing the property
> here, the "xen,static-shm" property getting parsed in different ways in process_shm.
> So this change is needed here.
>
> And I think I need to add in-code comment to explain. ;)
>
> >
> > > ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize,
> > PAGE_SIZE));
> > > if ( PFN_DOWN(*psize) > UINT_MAX )
> > > {
> > > @@ -751,6 +752,113 @@ 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
> > > +static __initdata DECLARE_BITMAP(shm_mask, NR_MEM_BANKS);
> > > +
> > > +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);
> > > +
> > > +}
> > > +
> > > +static int __init allocate_shared_memory(struct domain *d,
> > > + u32 addr_cells, u32 size_cells,
> > > + paddr_t pbase, paddr_t psize,
> > > + paddr_t gbase) {
> > > + mfn_t smfn;
> > > + int ret = 0;
> > > +
> > > + printk(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;
> > > +
> > > + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn,
> > PFN_DOWN(psize));
> > > + if ( ret )
> > > + {
> > > + dprintk(XENLOG_ERR, "Failed to map shared memory to %pd.\n", d);
> > > + return ret;
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +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_shared. */
> > > + /*
> > > + * Per shared memory region could be shared between multiple
> > domains.
> > > + * In case re-allocating the same shared memory region, we use bitmask
> > > + * shm_mask to record whether this shared memory region has ever
> > been
> > > + * allocated already.
> > > + */
> > > + if ( !test_bit(shm_id, shm_mask) )
> > > + {
> > > + /*
> > > + * Allocate statically shared pages to the default dom_shared.
> > > + * Set up P2M, and dom_shared is a direct-map domain,
> > > + * so GFN == PFN.
> > > + */
> > > + ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
> > > + pbase, psize, pbase);
> > ^gbase
> >
> > The last parameter should be gbase instead of pbase?
> >
>
> Yes, and since dom_shared is a direct-map domain, GFN == PFN, so pbase should be
> ok here. I mentioned it on comments.
>
> And Why I make dom_shared direct-map domain is that in this way we don't need to decode
> owner GFN when doing foreign memory mapping for borrower domain.
>
> >
> > Reading this patch is not clear that only the "owner" code path is
> > implemented here. The "borrower" code path is implemented later and
> > missing in this patch. I think it would be good to clarify that in the commit
> > message.
> >
> > Under this light, allocate_shared_memory is supposed to be only called for the
> > owner. I think we should probably mention that in the in-code comment too.
> >
>
> Yes, only owner domain could allocate memory, I'll add it to in-code comment.
>
> > I don't think we need to define a second copy of shm_mask. Can we reuse the
> > one in bootfdt.c?
> >
>
> Yes, maybe I should reuse than reintroduce. And before using the bitmap here,
> I need to clear it totally to clean away all the stale info from bootfdt.c.
>
> > Or we could get rid of shm_mask entirely here if we check whether the pages
> > were already allocated in the owner p2m.
> >
> >
>
> Hmm, that means that we need to do the page walk each time? That's kinds of
> time-consuming, or am I missing some convenient way to check?
No page walk. I think it should be possible with:
- mfn_to_page
- page_get_owner
both of them are direct access. Assuming that the page owner is set
correctly.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 04/13] xen/arm: add P2M type parameter in guest_physmap_add_pages
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (2 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 03/13] xen/arm: allocate static shared memory to dom_shared Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-11 6:11 ` [PATCH v1 05/13] xen/arm: introduce get_pages_from_gfn Penny Zheng
` (8 subsequent siblings)
12 siblings, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
In order to cover the scenario where users intend to set up guest
p2m foreign mapping with nr_pages, this commit adds a new P2M type
parameter in guest_physmap_add_pages.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 5 +++--
xen/arch/arm/include/asm/p2m.h | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6e6349caac..984e70e5fc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -507,7 +507,7 @@ static bool __init append_static_memory_to_bank(struct domain *d,
else
sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
- res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
+ res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages, p2m_ram_rw);
if ( res )
{
dprintk(XENLOG_ERR, "Failed to map pages to DOMU: %d", res);
@@ -787,7 +787,8 @@ static int __init allocate_shared_memory(struct domain *d,
if ( mfn_eq(smfn, INVALID_MFN) )
return -EINVAL;
- ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize));
+ ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize),
+ p2m_ram_rw);
if ( ret )
{
dprintk(XENLOG_ERR, "Failed to map shared memory to %pd.\n", d);
diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index 8cce459b67..58590145b0 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -317,9 +317,10 @@ guest_physmap_add_page(struct domain *d, gfn_t gfn, mfn_t mfn,
static inline int guest_physmap_add_pages(struct domain *d,
gfn_t gfn,
mfn_t mfn,
- unsigned int nr_pages)
+ unsigned int nr_pages,
+ p2m_type_t t)
{
- return p2m_insert_mapping(d, gfn, nr_pages, mfn, p2m_ram_rw);
+ return p2m_insert_mapping(d, gfn, nr_pages, mfn, t);
}
mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn);
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v1 05/13] xen/arm: introduce get_pages_from_gfn
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (3 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 04/13] xen/arm: add P2M type parameter in guest_physmap_add_pages Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-11 6:11 ` [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
` (7 subsequent siblings)
12 siblings, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
In order to getting statically shared pages based on gfn with nr_pages, this
commit introduces a new helper get_pages_from_gfn to acquire a set of pages
based on [gfn, gfn + nr_gfns), with the same P2M type.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/include/asm/p2m.h | 37 ++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index 58590145b0..b23024b9a1 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -370,6 +370,43 @@ static inline struct page_info *get_page_from_gfn(
return page;
}
+static inline int get_pages_from_gfn(struct domain *d, unsigned long gfn,
+ unsigned long nr_gfns,
+ struct page_info **pages, p2m_type_t *t,
+ p2m_query_t q)
+{
+ p2m_type_t _t;
+ unsigned long i = 0;
+ int ret = 0;
+
+ for ( ; i < nr_gfns; i++ )
+ {
+ pages[i] = get_page_from_gfn(d, gfn + i, t, q);
+
+ if ( !pages[i] )
+ {
+ ret = -ENOENT;
+ goto fail_get;
+ }
+
+ if ( i == 0 )
+ _t = *t;
+ /* Check if all pages share the same p2m type. */
+ else if ( *t != _t )
+ {
+ ret = -EINVAL;
+ goto fail_get;
+ }
+ }
+
+ return ret;
+
+ fail_get:
+ while( --i >= 0 )
+ put_page(pages[i]);
+ return ret;
+}
+
int get_page_type(struct page_info *page, unsigned long type);
bool is_iomem_page(mfn_t mfn);
static inline int get_page_and_type(struct page_info *page,
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (4 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 05/13] xen/arm: introduce get_pages_from_gfn Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 2:00 ` Stefano Stabellini
2022-04-08 22:59 ` Julien Grall
2022-03-11 6:11 ` [PATCH v1 07/13] xen/arm: create shared memory nodes in guest device tree Penny Zheng
` (6 subsequent siblings)
12 siblings, 2 replies; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
This commits introduces a new helper guest_physmap_add_shm to set up shared
memory foreign mapping for borrower domain.
Firstly it should get and take reference of statically shared pages from
owner dom_shared. Then it will setup P2M foreign memory map of these statically
shared pages for borrower domain.
This commits only considers owner domain is the default dom_shared, the
other scenario will be covered in the following patches.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 52 +++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 984e70e5fc..8cee5ffbd1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct domain *d,
return ret;
}
+static int __init guest_physmap_add_shm(struct domain *od, struct domain *bd,
+ unsigned long o_gfn,
+ unsigned long b_gfn,
+ unsigned long nr_gfns)
+{
+ struct page_info **pages = NULL;
+ p2m_type_t p2mt, t;
+ int ret = 0;
+
+ pages = xmalloc_array(struct page_info *, nr_gfns);
+ if ( !pages )
+ return -ENOMEM;
+
+ /*
+ * Take reference of statically shared pages from owner domain.
+ * Reference will be released when destroying shared memory region.
+ */
+ ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
+ if ( ret )
+ {
+ ret = -EINVAL;
+ goto fail_pages;
+ }
+
+ if ( p2m_is_ram(p2mt) )
+ t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
+ else
+ {
+ ret = -EINVAL;
+ goto fail_pages;
+ }
+
+ /* Set up guest foreign map. */
+ ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
+ nr_gfns, t);
+
+ fail_pages:
+ xfree(pages);
+
+ return ret;
+}
+
static int __init process_shm(struct domain *d,
const struct dt_device_node *node)
{
@@ -855,6 +897,16 @@ static int __init process_shm(struct domain *d,
set_bit(shm_id, shm_mask);
}
+
+ /*
+ * All domains are borrower domains when owner domain is the
+ * default dom_shared, so here we could just set up P2M foreign
+ * mapping for borrower domain immediately.
+ */
+ ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
+ PFN_DOWN(gbase), PFN_DOWN(psize));
+ if ( ret )
+ return ret;
}
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-03-11 6:11 ` [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
@ 2022-03-18 2:00 ` Stefano Stabellini
2022-03-29 3:44 ` Penny Zheng
2022-04-08 22:59 ` Julien Grall
1 sibling, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 2:00 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> This commits introduces a new helper guest_physmap_add_shm to set up shared
> memory foreign mapping for borrower domain.
>
> Firstly it should get and take reference of statically shared pages from
> owner dom_shared. Then it will setup P2M foreign memory map of these statically
> shared pages for borrower domain.
>
> This commits only considers owner domain is the default dom_shared, the
> other scenario will be covered in the following patches.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 52 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 984e70e5fc..8cee5ffbd1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct domain *d,
> return ret;
> }
>
> +static int __init guest_physmap_add_shm(struct domain *od, struct domain *bd,
> + unsigned long o_gfn,
> + unsigned long b_gfn,
> + unsigned long nr_gfns)
They should probably be gfn_t type
> +{
> + struct page_info **pages = NULL;
> + p2m_type_t p2mt, t;
> + int ret = 0;
> +
> + pages = xmalloc_array(struct page_info *, nr_gfns);
> + if ( !pages )
> + return -ENOMEM;
> +
> + /*
> + * Take reference of statically shared pages from owner domain.
> + * Reference will be released when destroying shared memory region.
> + */
> + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> + if ( ret )
> + {
> + ret = -EINVAL;
> + goto fail_pages;
> + }
> +
> + if ( p2m_is_ram(p2mt) )
> + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
> + else
> + {
> + ret = -EINVAL;
> + goto fail_pages;
> + }
One idea is to initialize p2mt = p2m_ram_rw and pass it to
get_pages_from_gfn. Then get_pages_from_gfn can return error immediately
if any of the pages are of different type.
This way there is no need for checking again here.
> + /* Set up guest foreign map. */
> + ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> + nr_gfns, t);
> +
> + fail_pages:
> + xfree(pages);
> +
> + return ret;
> +}
> +
> static int __init process_shm(struct domain *d,
> const struct dt_device_node *node)
> {
> @@ -855,6 +897,16 @@ static int __init process_shm(struct domain *d,
>
> set_bit(shm_id, shm_mask);
> }
> +
> + /*
> + * All domains are borrower domains when owner domain is the
> + * default dom_shared, so here we could just set up P2M foreign
> + * mapping for borrower domain immediately.
> + */
> + ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> + PFN_DOWN(gbase), PFN_DOWN(psize));
> + if ( ret )
> + return ret;
> }
>
> return 0;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-03-18 2:00 ` Stefano Stabellini
@ 2022-03-29 3:44 ` Penny Zheng
2022-04-08 22:18 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-29 3:44 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano
Sorry for the late response, got sidetracked an emergency issue. ;/
> -----Original Message-----
> From: Stefano Stabellini <sstabelliHini@kernel.org>
> Sent: Friday, March 18, 2022 10:00 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> mapping for borrower domain
>
> On Fri, 11 Mar 2022, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This commits introduces a new helper guest_physmap_add_shm to set up
> > shared memory foreign mapping for borrower domain.
> >
> > Firstly it should get and take reference of statically shared pages
> > from owner dom_shared. Then it will setup P2M foreign memory map of
> > these statically shared pages for borrower domain.
> >
> > This commits only considers owner domain is the default dom_shared,
> > the other scenario will be covered in the following patches.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain_build.c | 52
> > +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 984e70e5fc..8cee5ffbd1 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct
> domain *d,
> > return ret;
> > }
> >
> > +static int __init guest_physmap_add_shm(struct domain *od, struct domain
> *bd,
> > + unsigned long o_gfn,
> > + unsigned long b_gfn,
> > + unsigned long nr_gfns)
>
> They should probably be gfn_t type
>
>
Sure, will do.
> > +{
> > + struct page_info **pages = NULL;
> > + p2m_type_t p2mt, t;
> > + int ret = 0;
> > +
> > + pages = xmalloc_array(struct page_info *, nr_gfns);
> > + if ( !pages )
> > + return -ENOMEM;
> > +
> > + /*
> > + * Take reference of statically shared pages from owner domain.
> > + * Reference will be released when destroying shared memory region.
> > + */
> > + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> > + if ( ret )
> > + {
> > + ret = -EINVAL;
> > + goto fail_pages;
> > + }
> > +
> > + if ( p2m_is_ram(p2mt) )
> > + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
> p2m_map_foreign_ro;
> > + else
> > + {
> > + ret = -EINVAL;
> > + goto fail_pages;
> > + }
>
> One idea is to initialize p2mt = p2m_ram_rw and pass it to
> get_pages_from_gfn. Then get_pages_from_gfn can return error immediately
> if any of the pages are of different type.
>
> This way there is no need for checking again here.
>
Right now, the memory attribute of static shared memory is RW as default,
What if we add memory attribute setting in device tree configuration, sometimes,
Users want to specify that borrower domain only has RO right, hmm, then the
Initialization for p2mt could be either p2m_ram_rw or p2m_ram_ro?
In such case, we could add another parameter in guest_physmap_add_shm to
show the p2m type...
Hope I understand what you suggested here.
>
> > + /* Set up guest foreign map. */
> > + ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> > + nr_gfns, t);
> > +
> > + fail_pages:
> > + xfree(pages);
> > +
> > + return ret;
> > +}
> > +
> > static int __init process_shm(struct domain *d,
> > const struct dt_device_node *node) {
> > @@ -855,6 +897,16 @@ static int __init process_shm(struct domain *d,
> >
> > set_bit(shm_id, shm_mask);
> > }
> > +
> > + /*
> > + * All domains are borrower domains when owner domain is the
> > + * default dom_shared, so here we could just set up P2M foreign
> > + * mapping for borrower domain immediately.
> > + */
> > + ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> > + PFN_DOWN(gbase), PFN_DOWN(psize));
> > + if ( ret )
> > + return ret;
> > }
> >
> > return 0;
> > --
> > 2.25.1
> >
---
Cheers,
Penny Zheng
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-03-29 3:44 ` Penny Zheng
@ 2022-04-08 22:18 ` Stefano Stabellini
2022-04-08 22:50 ` Julien Grall
0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-04-08 22:18 UTC (permalink / raw)
To: Penny Zheng
Cc: Stefano Stabellini, xen-devel, nd, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Tue, 29 Mar 2022, Penny Zheng wrote:
> Hi Stefano
>
> Sorry for the late response, got sidetracked an emergency issue. ;/
>
> > -----Original Message-----
> > From: Stefano Stabellini <sstabelliHini@kernel.org>
> > Sent: Friday, March 18, 2022 10:00 AM
> > To: Penny Zheng <Penny.Zheng@arm.com>
> > Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Stefano Stabellini
> > <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> > <Volodymyr_Babchuk@epam.com>
> > Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> > mapping for borrower domain
> >
> > On Fri, 11 Mar 2022, Penny Zheng wrote:
> > > From: Penny Zheng <penny.zheng@arm.com>
> > >
> > > This commits introduces a new helper guest_physmap_add_shm to set up
> > > shared memory foreign mapping for borrower domain.
> > >
> > > Firstly it should get and take reference of statically shared pages
> > > from owner dom_shared. Then it will setup P2M foreign memory map of
> > > these statically shared pages for borrower domain.
> > >
> > > This commits only considers owner domain is the default dom_shared,
> > > the other scenario will be covered in the following patches.
> > >
> > > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > > ---
> > > xen/arch/arm/domain_build.c | 52
> > > +++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 52 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 984e70e5fc..8cee5ffbd1 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct
> > domain *d,
> > > return ret;
> > > }
> > >
> > > +static int __init guest_physmap_add_shm(struct domain *od, struct domain
> > *bd,
> > > + unsigned long o_gfn,
> > > + unsigned long b_gfn,
> > > + unsigned long nr_gfns)
> >
> > They should probably be gfn_t type
> >
> >
>
> Sure, will do.
>
> > > +{
> > > + struct page_info **pages = NULL;
> > > + p2m_type_t p2mt, t;
> > > + int ret = 0;
> > > +
> > > + pages = xmalloc_array(struct page_info *, nr_gfns);
> > > + if ( !pages )
> > > + return -ENOMEM;
> > > +
> > > + /*
> > > + * Take reference of statically shared pages from owner domain.
> > > + * Reference will be released when destroying shared memory region.
> > > + */
> > > + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> > > + if ( ret )
> > > + {
> > > + ret = -EINVAL;
> > > + goto fail_pages;
> > > + }
> > > +
> > > + if ( p2m_is_ram(p2mt) )
> > > + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
> > p2m_map_foreign_ro;
> > > + else
> > > + {
> > > + ret = -EINVAL;
> > > + goto fail_pages;
> > > + }
> >
> > One idea is to initialize p2mt = p2m_ram_rw and pass it to
> > get_pages_from_gfn. Then get_pages_from_gfn can return error immediately
> > if any of the pages are of different type.
> >
> > This way there is no need for checking again here.
> >
>
> Right now, the memory attribute of static shared memory is RW as default,
> What if we add memory attribute setting in device tree configuration, sometimes,
> Users want to specify that borrower domain only has RO right, hmm, then the
> Initialization for p2mt could be either p2m_ram_rw or p2m_ram_ro?
> In such case, we could add another parameter in guest_physmap_add_shm to
> show the p2m type...
> Hope I understand what you suggested here.
Yes, I think I understand. I think your suggestion is OK too. However,
your suggestion is much more work than mine: I was only suggesting a
small improvement limited to guest_physmap_add_shm and
get_pages_from_gfn. Your suggestion involves a device tree change that
would have a larger impact on the patch series. So if you are up for it,
I am happy to review it. I am also fine just to have a small improvement
on guest_physmap_add_shm/get_pages_from_gfn.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-04-08 22:18 ` Stefano Stabellini
@ 2022-04-08 22:50 ` Julien Grall
2022-04-08 23:18 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2022-04-08 22:50 UTC (permalink / raw)
To: Stefano Stabellini, Penny Zheng
Cc: xen-devel, nd, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano,
On 08/04/2022 23:18, Stefano Stabellini wrote:
> On Tue, 29 Mar 2022, Penny Zheng wrote:
>> Right now, the memory attribute of static shared memory is RW as default,
>> What if we add memory attribute setting in device tree configuration, sometimes,
>> Users want to specify that borrower domain only has RO right, hmm, then the
>> Initialization for p2mt could be either p2m_ram_rw or p2m_ram_ro?
>> In such case, we could add another parameter in guest_physmap_add_shm to
>> show the p2m type...
>> Hope I understand what you suggested here.
>
> Yes, I think I understand. I think your suggestion is OK too. However,
> your suggestion is much more work than mine: I was only suggesting a
> small improvement limited to guest_physmap_add_shm and
> get_pages_from_gfn. Your suggestion involves a device tree change that
> would have a larger impact on the patch series. So if you are up for it,
> I am happy to review it. I am also fine just to have a small improvement
> on guest_physmap_add_shm/get_pages_from_gfn.
AFAIU, your proposal would mean that the behavior for
get_pages_from_gfn() and get_page_from_gfn() will differ. This is not great.
I don't think we can easily change the behavior of get_page_from_gfn()
because some callers can accept multiple types.
Furthermore, while today we only check p2m_ram_rw. It might be possible
that we would want to check different type. For instance, if we want to
use read-only mapping, it would be fine to accept p2m_ram_ro and p2m_ram_rw.
So overall, I am not in favor of initializing p2mt and let
get_pages_from_gfn() to check the type.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-04-08 22:50 ` Julien Grall
@ 2022-04-08 23:18 ` Stefano Stabellini
0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-04-08 23:18 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Penny Zheng, xen-devel, nd, Bertrand Marquis,
Volodymyr Babchuk
On Fri, 8 Apr 2022, Julien Grall wrote:
> On 08/04/2022 23:18, Stefano Stabellini wrote:
> > On Tue, 29 Mar 2022, Penny Zheng wrote:
> > > Right now, the memory attribute of static shared memory is RW as default,
> > > What if we add memory attribute setting in device tree configuration,
> > > sometimes,
> > > Users want to specify that borrower domain only has RO right, hmm, then
> > > the
> > > Initialization for p2mt could be either p2m_ram_rw or p2m_ram_ro?
> > > In such case, we could add another parameter in guest_physmap_add_shm to
> > > show the p2m type...
> > > Hope I understand what you suggested here.
> >
> > Yes, I think I understand. I think your suggestion is OK too. However,
> > your suggestion is much more work than mine: I was only suggesting a
> > small improvement limited to guest_physmap_add_shm and
> > get_pages_from_gfn. Your suggestion involves a device tree change that
> > would have a larger impact on the patch series. So if you are up for it,
> > I am happy to review it. I am also fine just to have a small improvement
> > on guest_physmap_add_shm/get_pages_from_gfn.
>
> AFAIU, your proposal would mean that the behavior for get_pages_from_gfn() and
> get_page_from_gfn() will differ. This is not great.
>
> I don't think we can easily change the behavior of get_page_from_gfn() because
> some callers can accept multiple types.
>
> Furthermore, while today we only check p2m_ram_rw. It might be possible that
> we would want to check different type. For instance, if we want to use
> read-only mapping, it would be fine to accept p2m_ram_ro and p2m_ram_rw.
>
> So overall, I am not in favor of initializing p2mt and let
> get_pages_from_gfn() to check the type.
OK. In that case, I suggest to leave the code pretty much as is in
regards of how get_pages_from_gfn is called from guest_physmap_add_shm.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-03-11 6:11 ` [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
2022-03-18 2:00 ` Stefano Stabellini
@ 2022-04-08 22:59 ` Julien Grall
2022-04-09 9:30 ` Julien Grall
2022-04-20 8:51 ` Penny Zheng
1 sibling, 2 replies; 51+ messages in thread
From: Julien Grall @ 2022-04-08 22:59 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> This commits introduces a new helper guest_physmap_add_shm to set up shared
> memory foreign mapping for borrower domain.
>
> Firstly it should get and take reference of statically shared pages from
> owner dom_shared. Then it will setup P2M foreign memory map of these statically
> shared pages for borrower domain.
>
> This commits only considers owner domain is the default dom_shared, the
> other scenario will be covered in the following patches.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 52 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 984e70e5fc..8cee5ffbd1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct domain *d,
> return ret;
> }
>
> +static int __init guest_physmap_add_shm(struct domain *od, struct domain *bd,
> + unsigned long o_gfn,
> + unsigned long b_gfn,
> + unsigned long nr_gfns)
> +{
> + struct page_info **pages = NULL;
> + p2m_type_t p2mt, t;
> + int ret = 0;
You don't need to initialize ret.
> +
> + pages = xmalloc_array(struct page_info *, nr_gfns);
> + if ( !pages )
> + return -ENOMEM;
> +
> + /*
> + * Take reference of statically shared pages from owner domain.
> + * Reference will be released when destroying shared memory region.
> + */
> + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> + if ( ret )
> + {
> + ret = -EINVAL;
> + goto fail_pages;
> + }
> +
> + if ( p2m_is_ram(p2mt) )
> + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw : p2m_map_foreign_ro;
> + else
> + {
> + ret = -EINVAL;
> + goto fail_pages;
Where would we release the references?
> + }
> +
> + /* Set up guest foreign map. */
> + ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> + nr_gfns, t);
A few things:
- The beginning of the code assumes that the MFN may be discontiguous
in the physical memory. But here, you are assuming they are contiguous.
If you want the latter, then you should check the MFNs are contiguous.
That said, I am not sure if this restriction is really necessary.
- IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you
need to revert it in case of failure.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-04-08 22:59 ` Julien Grall
@ 2022-04-09 9:30 ` Julien Grall
2022-04-20 8:53 ` Penny Zheng
2022-04-20 8:51 ` Penny Zheng
1 sibling, 1 reply; 51+ messages in thread
From: Julien Grall @ 2022-04-09 9:30 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 08/04/2022 23:59, Julien Grall wrote:
> On 11/03/2022 06:11, Penny Zheng wrote:
>> From: Penny Zheng <penny.zheng@arm.com>
>>
>> This commits introduces a new helper guest_physmap_add_shm to set up
>> shared
>> memory foreign mapping for borrower domain.
>>
>> Firstly it should get and take reference of statically shared pages from
>> owner dom_shared. Then it will setup P2M foreign memory map of these
>> statically
>> shared pages for borrower domain.
>>
>> This commits only considers owner domain is the default dom_shared, the
>> other scenario will be covered in the following patches.
>>
>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>> ---
>> xen/arch/arm/domain_build.c | 52 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 984e70e5fc..8cee5ffbd1 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct
>> domain *d,
>> return ret;
>> }
>> +static int __init guest_physmap_add_shm(struct domain *od, struct
>> domain *bd,
>> + unsigned long o_gfn,
>> + unsigned long b_gfn,
>> + unsigned long nr_gfns)
>> +{
>> + struct page_info **pages = NULL;
>> + p2m_type_t p2mt, t;
>> + int ret = 0;
>
> You don't need to initialize ret.
>
>> +
>> + pages = xmalloc_array(struct page_info *, nr_gfns);
>> + if ( !pages )
>> + return -ENOMEM;
>> +
>> + /*
>> + * Take reference of statically shared pages from owner domain.
>> + * Reference will be released when destroying shared memory region.
>> + */
>> + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt,
>> P2M_ALLOC);
>> + if ( ret )
>> + {
>> + ret = -EINVAL;
>> + goto fail_pages;
>> + }
>> +
>> + if ( p2m_is_ram(p2mt) )
>> + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
>> p2m_map_foreign_ro;
>> + else
>> + {
>> + ret = -EINVAL;
>> + goto fail_pages;
>
> Where would we release the references?
>
>> + }
>> +
>> + /* Set up guest foreign map. */
>> + ret = guest_physmap_add_pages(bd, _gfn(b_gfn),
>> page_to_mfn(pages[0]),
>> + nr_gfns, t);
>
> A few things:
> - The beginning of the code assumes that the MFN may be discontiguous
> in the physical memory. But here, you are assuming they are contiguous.
> If you want the latter, then you should check the MFNs are contiguous.
> That said, I am not sure if this restriction is really necessary.
>
> - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you
> need to revert it in case of failure.
There is another issue here. guest_physmap_add_pages() may use superpage
mapping. The P2M code is currently assuming the foreing mapping will be
using L3 mapping (4KB).
Do you need to use superpage mapping here?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-04-09 9:30 ` Julien Grall
@ 2022-04-20 8:53 ` Penny Zheng
0 siblings, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-04-20 8:53 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, April 9, 2022 5:31 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Bertrand
> Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> mapping for borrower domain
>
> Hi,
>
> On 08/04/2022 23:59, Julien Grall wrote:
> > On 11/03/2022 06:11, Penny Zheng wrote:
> >> From: Penny Zheng <penny.zheng@arm.com>
> >>
> >> This commits introduces a new helper guest_physmap_add_shm to set up
> >> shared memory foreign mapping for borrower domain.
> >>
> >> Firstly it should get and take reference of statically shared pages
> >> from owner dom_shared. Then it will setup P2M foreign memory map of
> >> these statically shared pages for borrower domain.
> >>
> >> This commits only considers owner domain is the default dom_shared,
> >> the other scenario will be covered in the following patches.
> >>
> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> >> ---
> >> xen/arch/arm/domain_build.c | 52
> >> +++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 52 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/domain_build.c
> >> b/xen/arch/arm/domain_build.c index 984e70e5fc..8cee5ffbd1 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct
> >> domain *d,
> >> return ret;
> >> }
> >> +static int __init guest_physmap_add_shm(struct domain *od, struct
> >> domain *bd,
> >> + unsigned long o_gfn,
> >> + unsigned long b_gfn,
> >> + unsigned long nr_gfns) {
> >> + struct page_info **pages = NULL;
> >> + p2m_type_t p2mt, t;
> >> + int ret = 0;
> >
> > You don't need to initialize ret.
> >
> >> +
> >> + pages = xmalloc_array(struct page_info *, nr_gfns);
> >> + if ( !pages )
> >> + return -ENOMEM;
> >> +
> >> + /*
> >> + * Take reference of statically shared pages from owner domain.
> >> + * Reference will be released when destroying shared memory region.
> >> + */
> >> + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt,
> >> P2M_ALLOC);
> >> + if ( ret )
> >> + {
> >> + ret = -EINVAL;
> >> + goto fail_pages;
> >> + }
> >> +
> >> + if ( p2m_is_ram(p2mt) )
> >> + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
> >> p2m_map_foreign_ro;
> >> + else
> >> + {
> >> + ret = -EINVAL;
> >> + goto fail_pages;
> >
> > Where would we release the references?
> >
> >> + }
> >> +
> >> + /* Set up guest foreign map. */
> >> + ret = guest_physmap_add_pages(bd, _gfn(b_gfn),
> >> page_to_mfn(pages[0]),
> >> + nr_gfns, t);
> >
> > A few things:
> > - The beginning of the code assumes that the MFN may be
> > discontiguous in the physical memory. But here, you are assuming they are
> contiguous.
> > If you want the latter, then you should check the MFNs are contiguous.
> > That said, I am not sure if this restriction is really necessary.
> >
> > - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So
> > you need to revert it in case of failure.
>
>
> There is another issue here. guest_physmap_add_pages() may use superpage
> mapping. The P2M code is currently assuming the foreing mapping will be
> using L3 mapping (4KB).
>
> Do you need to use superpage mapping here?
>
Right now, there is no user case on my side needing superpage mapping.
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain
2022-04-08 22:59 ` Julien Grall
2022-04-09 9:30 ` Julien Grall
@ 2022-04-20 8:51 ` Penny Zheng
1 sibling, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-04-20 8:51 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, April 9, 2022 6:59 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Bertrand
> Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> mapping for borrower domain
>
> Hi Penny,
>
> On 11/03/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This commits introduces a new helper guest_physmap_add_shm to set up
> > shared memory foreign mapping for borrower domain.
> >
> > Firstly it should get and take reference of statically shared pages
> > from owner dom_shared. Then it will setup P2M foreign memory map of
> > these statically shared pages for borrower domain.
> >
> > This commits only considers owner domain is the default dom_shared,
> > the other scenario will be covered in the following patches.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain_build.c | 52
> +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 984e70e5fc..8cee5ffbd1 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct
> domain *d,
> > return ret;
> > }
> >
> > +static int __init guest_physmap_add_shm(struct domain *od, struct domain
> *bd,
> > + unsigned long o_gfn,
> > + unsigned long b_gfn,
> > + unsigned long nr_gfns) {
> > + struct page_info **pages = NULL;
> > + p2m_type_t p2mt, t;
> > + int ret = 0;
>
> You don't need to initialize ret.
>
> > +
> > + pages = xmalloc_array(struct page_info *, nr_gfns);
> > + if ( !pages )
> > + return -ENOMEM;
> > +
> > + /*
> > + * Take reference of statically shared pages from owner domain.
> > + * Reference will be released when destroying shared memory region.
> > + */
> > + ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> > + if ( ret )
> > + {
> > + ret = -EINVAL;
> > + goto fail_pages;
> > + }
> > +
> > + if ( p2m_is_ram(p2mt) )
> > + t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
> p2m_map_foreign_ro;
> > + else
> > + {
> > + ret = -EINVAL;
> > + goto fail_pages;
>
> Where would we release the references?
>
> > + }
> > +
> > + /* Set up guest foreign map. */
> > + ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> > + nr_gfns, t);
>
> A few things:
> - The beginning of the code assumes that the MFN may be discontiguous in
> the physical memory. But here, you are assuming they are contiguous.
> If you want the latter, then you should check the MFNs are contiguous.
> That said, I am not sure if this restriction is really necessary.
>
Oh,, caught me, it never occurred to me that the pages in array `pages` could
be discontinuous.
I definitely want the latter. This function is always dealing with a memory region
(contiguous MFNS) each time. So maybe a loop to check
page_to_mfn(pages[i]) == mfn_add(smfn, i) is needed.
> - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you
> need to revert it in case of failure.
>
Yes, both failure points you are referring to are requiring reverting the mappings.
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 07/13] xen/arm: create shared memory nodes in guest device tree
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (5 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 2:00 ` Stefano Stabellini
2022-03-11 6:11 ` [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain Penny Zheng
` (5 subsequent siblings)
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
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>
---
xen/arch/arm/domain_build.c | 144 ++++++++++++++++++++++++++++++
xen/arch/arm/include/asm/domain.h | 1 +
xen/arch/arm/include/asm/setup.h | 3 +
3 files changed, 148 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8cee5ffbd1..997df46ddd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -840,6 +840,28 @@ static int __init guest_physmap_add_shm(struct domain *od, struct domain *bd,
return ret;
}
+static int __init append_shm_bank_to_domain(struct domain *d,
+ paddr_t start, paddr_t size,
+ u32 shm_id)
+{
+ /* Allocate memory at first insertion. */
+ if ( d->arch.shm_mem == NULL )
+ {
+ d->arch.shm_mem = xmalloc_bytes(sizeof(struct meminfo));
+ if ( d->arch.shm_mem == NULL )
+ return -ENOMEM;
+
+ memset(d->arch.shm_mem, 0, sizeof(struct meminfo));
+ }
+
+ d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].start = start;
+ d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].size = size;
+ d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].shm_id = shm_id;
+ d->arch.shm_mem->nr_banks++;
+
+ return 0;
+}
+
static int __init process_shm(struct domain *d,
const struct dt_device_node *node)
{
@@ -907,6 +929,14 @@ static int __init process_shm(struct domain *d,
PFN_DOWN(gbase), PFN_DOWN(psize));
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(d, gbase, psize, shm_id);
+ if ( ret )
+ return ret;
}
return 0;
@@ -1237,6 +1267,115 @@ 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;
+ int reg_size = addrcells + sizecells;
+
+ 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++ )
+ {
+ u64 start = mem->bank[i].start;
+ u64 size = mem->bank[i].size;
+ u32 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, *cells;
+ unsigned int len;
+
+ 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;
+
+ len = reg_size * sizeof(__be32);
+ reg = xmalloc_bytes(len);
+ if ( reg == NULL )
+ return -ENOMEM;
+ cells = reg;
+
+ dt_child_set_range(&cells, addrcells, sizecells, start, size);
+
+ res = fdt_property(fdt, "reg", reg, len);
+ xfree(reg);
+ 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-mem";
+
+ if ( mem == NULL )
+ /* 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 = 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;
@@ -2974,6 +3113,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,
+ d->arch.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
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index ea7a7219a3..6df37d2c46 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -110,6 +110,7 @@ struct arch_domain
#endif
bool directmap;
+ struct meminfo *shm_mem;
} __cacheline_aligned;
struct arch_vcpu
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 7a1e1d6798..b6ff04889c 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
+ u32 shm_id ; /* ID identifier of a static shared memory bank. */
+#endif
};
struct meminfo {
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 07/13] xen/arm: create shared memory nodes in guest device tree
2022-03-11 6:11 ` [PATCH v1 07/13] xen/arm: create shared memory nodes in guest device tree Penny Zheng
@ 2022-03-18 2:00 ` Stefano Stabellini
0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 2:00 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> 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>
> ---
> xen/arch/arm/domain_build.c | 144 ++++++++++++++++++++++++++++++
> xen/arch/arm/include/asm/domain.h | 1 +
> xen/arch/arm/include/asm/setup.h | 3 +
> 3 files changed, 148 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8cee5ffbd1..997df46ddd 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -840,6 +840,28 @@ static int __init guest_physmap_add_shm(struct domain *od, struct domain *bd,
> return ret;
> }
>
> +static int __init append_shm_bank_to_domain(struct domain *d,
> + paddr_t start, paddr_t size,
> + u32 shm_id)
> +{
> + /* Allocate memory at first insertion. */
> + if ( d->arch.shm_mem == NULL )
> + {
> + d->arch.shm_mem = xmalloc_bytes(sizeof(struct meminfo));
> + if ( d->arch.shm_mem == NULL )
> + return -ENOMEM;
> +
> + memset(d->arch.shm_mem, 0, sizeof(struct meminfo));
_xzalloc ?
> + }
> +
> + d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].start = start;
> + d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].size = size;
> + d->arch.shm_mem->bank[d->arch.shm_mem->nr_banks].shm_id = shm_id;
> + d->arch.shm_mem->nr_banks++;
> +
> + return 0;
> +}
> +
> static int __init process_shm(struct domain *d,
> const struct dt_device_node *node)
> {
> @@ -907,6 +929,14 @@ static int __init process_shm(struct domain *d,
> PFN_DOWN(gbase), PFN_DOWN(psize));
> 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(d, gbase, psize, shm_id);
> + if ( ret )
> + return ret;
> }
>
> return 0;
> @@ -1237,6 +1267,115 @@ 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;
> + int reg_size = addrcells + sizecells;
> +
> + 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++ )
> + {
> + u64 start = mem->bank[i].start;
> + u64 size = mem->bank[i].size;
> + u32 shm_id = mem->bank[i].shm_id;
Wasn't shm_id supposed to be u8?
> + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> + char buf[27];
> + const char compat[] = "xen,shared-memory-v1";
> + __be32 *reg, *cells;
> + unsigned int len;
alignment
> + 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;
> +
> + len = reg_size * sizeof(__be32);
alignment
> + reg = xmalloc_bytes(len);
This is the same size for each bank. I think it is better as a local
variable on the stack.
> + if ( reg == NULL )
> + return -ENOMEM;
> + cells = reg;
> +
> + dt_child_set_range(&cells, addrcells, sizecells, start, size);
> +
> + res = fdt_property(fdt, "reg", reg, len);
> + xfree(reg);
> + 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-mem";
The node is called "reserved-memory", why did you initialize it to
"reserved-mem" instead?
> + if ( mem == NULL )
> + /* 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;
We should also add #address-cells and #size-cells properties
> +
> + 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;
> @@ -2974,6 +3113,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,
> + d->arch.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
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index ea7a7219a3..6df37d2c46 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -110,6 +110,7 @@ struct arch_domain
> #endif
>
> bool directmap;
> + struct meminfo *shm_mem;
> } __cacheline_aligned;
>
> struct arch_vcpu
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 7a1e1d6798..b6ff04889c 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
> + u32 shm_id ; /* ID identifier of a static shared memory bank. */
> +#endif
No need to be u32 considering that the field is u8. In any case, this is
an internal Xen header so we would use uint8_t.
> };
>
> struct meminfo {
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (6 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 07/13] xen/arm: create shared memory nodes in guest device tree Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-04-09 9:25 ` Julien Grall
2022-03-11 6:11 ` [PATCH v1 09/13] xen/arm: enable statically shared memory on Dom0 Penny Zheng
` (4 subsequent siblings)
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
This commit introduces a new helper destroy_domain_shm to destroy static
shared memory at domain de-construction.
This patch only considers the scenario where the owner domain is the
default dom_shared, for user-defined owner domain, it will be covered in
the following patches.
Since all domains are borrower domains, we could simply remove guest P2M
foreign mapping of statically shared memory region and drop the reference
added at guest_physmap_add_shm.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 1ff1df5d3f..f0bfd67fe5 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -34,6 +34,7 @@
#include <asm/platform.h>
#include <asm/procinfo.h>
#include <asm/regs.h>
+#include <asm/setup.h>
#include <asm/tee/tee.h>
#include <asm/vfp.h>
#include <asm/vgic.h>
@@ -993,6 +994,48 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
return ret;
}
+#ifdef CONFIG_STATIC_SHM
+static int domain_destroy_shm(struct domain *d)
+{
+ int ret = 0;
+ unsigned long i = 0UL, j;
+
+ if ( d->arch.shm_mem == NULL )
+ return ret;
+ else
+ {
+ for ( ; i < d->arch.shm_mem->nr_banks; i++ )
+ {
+ unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem->bank[i].size);
+ gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start);
+
+ for ( j = 0; j < nr_gfns; j++ )
+ {
+ mfn_t mfn;
+
+ mfn = gfn_to_mfn(d, gfn_add(gfn, j));
+ if ( !mfn_valid(mfn) )
+ {
+ dprintk(XENLOG_ERR,
+ "Domain %pd page number %lx invalid.\n",
+ d, gfn_x(gfn) + i);
+ return -EINVAL;
+ }
+
+ ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, 0);
+ if ( ret )
+ return ret;
+
+ /* Drop the reference. */
+ put_page(mfn_to_page(mfn));
+ }
+ }
+ }
+
+ return ret;
+}
+#endif
+
/*
* Record the current progress. Subsequent hypercall continuations will
* logically restart work from this point.
@@ -1039,6 +1082,11 @@ int domain_relinquish_resources(struct domain *d)
*/
domain_vpl011_deinit(d);
+#ifdef CONFIG_STATIC_SHM
+ ret = domain_destroy_shm(d);
+ if ( ret )
+ return ret;
+#endif
#ifdef CONFIG_IOREQ_SERVER
ioreq_server_destroy_all(d);
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain
2022-03-11 6:11 ` [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain Penny Zheng
@ 2022-04-09 9:25 ` Julien Grall
2022-04-21 7:00 ` Penny Zheng
0 siblings, 1 reply; 51+ messages in thread
From: Julien Grall @ 2022-04-09 9:25 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> This commit introduces a new helper destroy_domain_shm to destroy static
> shared memory at domain de-construction.
>
> This patch only considers the scenario where the owner domain is the
> default dom_shared, for user-defined owner domain, it will be covered in
> the following patches.
>
> Since all domains are borrower domains, we could simply remove guest P2M
> foreign mapping of statically shared memory region and drop the reference
> added at guest_physmap_add_shm.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 1ff1df5d3f..f0bfd67fe5 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -34,6 +34,7 @@
> #include <asm/platform.h>
> #include <asm/procinfo.h>
> #include <asm/regs.h>
> +#include <asm/setup.h>
> #include <asm/tee/tee.h>
> #include <asm/vfp.h>
> #include <asm/vgic.h>
> @@ -993,6 +994,48 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
> return ret;
> }
>
> +#ifdef CONFIG_STATIC_SHM
> +static int domain_destroy_shm(struct domain *d)
> +{
> + int ret = 0;
> + unsigned long i = 0UL, j;
> +
> + if ( d->arch.shm_mem == NULL )
> + return ret;
You already return the value here. So...
> + else
... the else is pointless.
> + {
> + for ( ; i < d->arch.shm_mem->nr_banks; i++ )
> + {
> + unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem->bank[i].size);
> + gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start);
> +
> + for ( j = 0; j < nr_gfns; j++ )
> + {
> + mfn_t mfn;
> +
> + mfn = gfn_to_mfn(d, gfn_add(gfn, j));
A domain is allowed to modify its P2M. So there are no guarantee that
the GFN will still point to the shared memory. This will allow the guest...
> + if ( !mfn_valid(mfn) )
> + {
> + dprintk(XENLOG_ERR,
> + "Domain %pd page number %lx invalid.\n",
> + d, gfn_x(gfn) + i);
> + return -EINVAL;
... to actively prevent destruction.
> + }
> +
> + ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, 0);
> + if ( ret )
> + return ret;
> +
> + /* Drop the reference. */
> + put_page(mfn_to_page(mfn));
guest_physmap_remove_page() will already drop the reference taken for
the foreign mapping. I couldn't find any other reference taken, what is
the put_page() for?
Also, as per above we don't know whether this is a page from the shared
page. So we can't blindly call put_page().
However, I don't think we need any specific code here. We can rely on
relinquish_p2m_mappings() to drop any reference. If there is an extra
one for shared mappings, then we should update p2m_put_l3_page().
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain
2022-04-09 9:25 ` Julien Grall
@ 2022-04-21 7:00 ` Penny Zheng
0 siblings, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-04-21 7:00 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi, julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, April 9, 2022 5:26 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Bertrand
> Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 08/13] xen/arm: destroy static shared memory when
> de-construct domain
>
> Hi Penny,
>
> On 11/03/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > This commit introduces a new helper destroy_domain_shm to destroy
> > static shared memory at domain de-construction.
> >
> > This patch only considers the scenario where the owner domain is the
> > default dom_shared, for user-defined owner domain, it will be covered
> > in the following patches.
> >
> > Since all domains are borrower domains, we could simply remove guest
> > P2M foreign mapping of statically shared memory region and drop the
> > reference added at guest_physmap_add_shm.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain.c | 48
> +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index
> > 1ff1df5d3f..f0bfd67fe5 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -34,6 +34,7 @@
> > #include <asm/platform.h>
> > #include <asm/procinfo.h>
> > #include <asm/regs.h>
> > +#include <asm/setup.h>
> > #include <asm/tee/tee.h>
> > #include <asm/vfp.h>
> > #include <asm/vgic.h>
> > @@ -993,6 +994,48 @@ static int relinquish_memory(struct domain *d,
> struct page_list_head *list)
> > return ret;
> > }
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +static int domain_destroy_shm(struct domain *d) {
> > + int ret = 0;
> > + unsigned long i = 0UL, j;
> > +
> > + if ( d->arch.shm_mem == NULL )
> > + return ret;
>
> You already return the value here. So...
>
> > + else
>
> ... the else is pointless.
>
> > + {
> > + for ( ; i < d->arch.shm_mem->nr_banks; i++ )
> > + {
> > + unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem-
> >bank[i].size);
> > + gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start);
> > +
> > + for ( j = 0; j < nr_gfns; j++ )
> > + {
> > + mfn_t mfn;
> > +
> > + mfn = gfn_to_mfn(d, gfn_add(gfn, j));
>
> A domain is allowed to modify its P2M. So there are no guarantee that the
> GFN will still point to the shared memory. This will allow the guest...
>
> > + if ( !mfn_valid(mfn) )
> > + {
> > + dprintk(XENLOG_ERR,
> > + "Domain %pd page number %lx invalid.\n",
> > + d, gfn_x(gfn) + i);
> > + return -EINVAL;
>
> ... to actively prevent destruction.
>
> > + }
>
>
> > +
> > + ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, 0);
> > + if ( ret )
> > + return ret;
> > +
> > + /* Drop the reference. */
> > + put_page(mfn_to_page(mfn));
>
> guest_physmap_remove_page() will already drop the reference taken for the
> foreign mapping. I couldn't find any other reference taken, what is the
> put_page() for?
>
> Also, as per above we don't know whether this is a page from the shared page.
> So we can't blindly call put_page().
>
> However, I don't think we need any specific code here. We can rely on
> relinquish_p2m_mappings() to drop any reference. If there is an extra one for
> shared mappings, then we should update p2m_put_l3_page().
>
True, true. Thanks for pointing this out!
In p2m_put_l3_page, it has already called put_page() for foreign mapping,
definitely no extra one here!
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 09/13] xen/arm: enable statically shared memory on Dom0
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (7 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 08/13] xen/arm: destroy static shared memory when de-construct domain Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-11 6:11 ` [PATCH v1 10/13] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
` (3 subsequent siblings)
12 siblings, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, 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>
---
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 997df46ddd..d35f98ff9c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2561,6 +2561,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,
+ d->arch.shm_mem);
+ if ( res )
+ return res;
}
res = fdt_end_node(kinfo->fdt);
@@ -3572,6 +3577,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);
@@ -3606,6 +3614,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, 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] 51+ messages in thread* [PATCH v1 10/13] xen/arm: allocate static shared memory to a specific owner domain
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (8 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 09/13] xen/arm: enable statically shared memory on Dom0 Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 2:00 ` Stefano Stabellini
2022-03-11 6:11 ` [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map Penny Zheng
` (2 subsequent siblings)
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
If owner property is defined, then owner domain of a static shared memory
region is not the default dom_shared anymore, but a specific domain.
This commit implements allocating static shared memory to a specific domain
when owner property is defined.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 63 ++++++++++++++++++++++++++++---------
1 file changed, 48 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d35f98ff9c..7ee4d33e0b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -872,6 +872,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_shared = true;
dt_for_each_child_node(node, shm_node)
{
@@ -899,6 +901,13 @@ static int __init process_shm(struct domain *d,
gbase = dt_read_number(cells, addr_cells);
/* TODO: Consider owner domain is not the default dom_shared. */
+ /*
+ * "role" property is optional and if it is defined explicitly,
+ * so the owner domain is not the default "dom_shared" domain.
+ */
+ if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
+ owner_dom_shared = false;
+
/*
* Per shared memory region could be shared between multiple domains.
* In case re-allocating the same shared memory region, we use bitmask
@@ -907,17 +916,38 @@ static int __init process_shm(struct domain *d,
*/
if ( !test_bit(shm_id, shm_mask) )
{
- /*
- * Allocate statically shared pages to the default dom_shared.
- * Set up P2M, and dom_shared is a direct-map domain,
- * so GFN == PFN.
- */
- ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
- pbase, psize, pbase);
- if ( ret )
- return ret;
-
- set_bit(shm_id, shm_mask);
+ if ( !owner_dom_shared )
+ {
+ if ( strcmp(role_str, "owner") == 0 )
+ {
+ /*
+ * Allocate statically shared pages to a specific owner
+ * domain.
+ */
+ ret = allocate_shared_memory(d, shm_id, addr_cells,
+ size_cells, pbase, psize,
+ gbase);
+ if ( ret )
+ return ret;
+
+ set_bit(shm_id, shm_mask);
+ }
+ }
+ else
+ {
+ /*
+ * Allocate statically shared pages to the default dom_shared.
+ * Set up P2M, and dom_shared is a direct-map domain,
+ * so GFN == PFN.
+ */
+ ret = allocate_shared_memory(dom_shared, shm_id,
+ addr_cells, size_cells, pbase,
+ psize, pbase);
+ if ( ret )
+ return ret;
+
+ set_bit(shm_id, shm_mask);
+ }
}
/*
@@ -925,10 +955,13 @@ static int __init process_shm(struct domain *d,
* default dom_shared, so here we could just set up P2M foreign
* mapping for borrower domain immediately.
*/
- ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
- PFN_DOWN(gbase), PFN_DOWN(psize));
- if ( ret )
- return ret;
+ if ( owner_dom_shared )
+ {
+ ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
+ PFN_DOWN(gbase), PFN_DOWN(psize));
+ if ( ret )
+ return ret;
+ }
/*
* Record static shared memory region info for later setting
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 10/13] xen/arm: allocate static shared memory to a specific owner domain
2022-03-11 6:11 ` [PATCH v1 10/13] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
@ 2022-03-18 2:00 ` Stefano Stabellini
0 siblings, 0 replies; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 2:00 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> If owner property is defined, then owner domain of a static shared memory
> region is not the default dom_shared anymore, but a specific domain.
>
> This commit implements allocating static shared memory to a specific domain
> when owner property is defined.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 63 ++++++++++++++++++++++++++++---------
> 1 file changed, 48 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d35f98ff9c..7ee4d33e0b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -872,6 +872,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_shared = true;
>
> dt_for_each_child_node(node, shm_node)
> {
> @@ -899,6 +901,13 @@ static int __init process_shm(struct domain *d,
> gbase = dt_read_number(cells, addr_cells);
>
> /* TODO: Consider owner domain is not the default dom_shared. */
We should remove this comment?
> + /*
> + * "role" property is optional and if it is defined explicitly,
> + * so the owner domain is not the default "dom_shared" domain.
> + */
> + if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
> + owner_dom_shared = false;
> +
> /*
> * Per shared memory region could be shared between multiple domains.
> * In case re-allocating the same shared memory region, we use bitmask
> @@ -907,17 +916,38 @@ static int __init process_shm(struct domain *d,
> */
> if ( !test_bit(shm_id, shm_mask) )
> {
> - /*
> - * Allocate statically shared pages to the default dom_shared.
> - * Set up P2M, and dom_shared is a direct-map domain,
> - * so GFN == PFN.
> - */
> - ret = allocate_shared_memory(dom_shared, addr_cells, size_cells,
> - pbase, psize, pbase);
> - if ( ret )
> - return ret;
> -
> - set_bit(shm_id, shm_mask);
> + if ( !owner_dom_shared )
> + {
> + if ( strcmp(role_str, "owner") == 0 )
> + {
> + /*
> + * Allocate statically shared pages to a specific owner
> + * domain.
> + */
> + ret = allocate_shared_memory(d, shm_id, addr_cells,
> + size_cells, pbase, psize,
> + gbase);
> + if ( ret )
> + return ret;
> +
> + set_bit(shm_id, shm_mask);
> + }
> + }
> + else
> + {
> + /*
> + * Allocate statically shared pages to the default dom_shared.
> + * Set up P2M, and dom_shared is a direct-map domain,
> + * so GFN == PFN.
> + */
> + ret = allocate_shared_memory(dom_shared, shm_id,
> + addr_cells, size_cells, pbase,
> + psize, pbase);
> + if ( ret )
> + return ret;
> +
> + set_bit(shm_id, shm_mask);
> + }
These two chunks are identical if not for dom_shared / d. Can we just
do:
if ( owner_dom_shared )
d = dom_shared;
on top? Then we can implement this only once.
> }
>
> /*
> @@ -925,10 +955,13 @@ static int __init process_shm(struct domain *d,
> * default dom_shared, so here we could just set up P2M foreign
> * mapping for borrower domain immediately.
> */
> - ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> - PFN_DOWN(gbase), PFN_DOWN(psize));
> - if ( ret )
> - return ret;
> + if ( owner_dom_shared )
> + {
> + ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> + PFN_DOWN(gbase), PFN_DOWN(psize));
> + if ( ret )
> + return ret;
> + }
What happens if the borrower is specified before the owner in device
tree? I see the case is handle by the next patch. Maybe we can have at
least a comment here or in the commit message.
>
> /*
> * Record static shared memory region info for later setting
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (9 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 10/13] xen/arm: allocate static shared memory to a specific owner domain Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-18 2:01 ` Stefano Stabellini
2022-03-11 6:11 ` [PATCH v1 12/13] xen/arm: defer foreign memory map in shm_init_late Penny Zheng
2022-03-11 6:11 ` [PATCH v1 13/13] xen/arm: unmap foreign memory mapping when destroyed domain is owner domain Penny Zheng
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
In a few scenarios where owner domain, is defined after borrower domain in
device tree configuration, then statically shared pages haven't been properly
allocated if borrower domain tries to do foreign memory map during
domain construction.
In order to cover such scenario, we defer all borrower domains' foreign
memory map after all domain construction finished, then only need to store
shm-info during domain construction.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain.c | 3 +++
xen/arch/arm/domain_build.c | 34 ++++++++++++++++++++++++++++++-
xen/arch/arm/include/asm/domain.h | 25 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index f0bfd67fe5..73ffbfb918 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -47,6 +47,9 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
#ifdef CONFIG_STATIC_SHM
struct domain *__read_mostly dom_shared;
+
+shm_info_t shm_list[NR_MEM_BANKS];
+DECLARE_BITMAP(shm_list_mask, NR_MEM_BANKS);
#endif
static void do_idle(void)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7ee4d33e0b..4b19160674 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -771,7 +771,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
}
-static int __init allocate_shared_memory(struct domain *d,
+static int __init allocate_shared_memory(struct domain *d, u32 shm_id,
u32 addr_cells, u32 size_cells,
paddr_t pbase, paddr_t psize,
paddr_t gbase)
@@ -795,6 +795,18 @@ static int __init allocate_shared_memory(struct domain *d,
return ret;
}
+ /*
+ * If owner domain is not default dom_shared, shm-info of owner domain
+ * shall also be recorded for later deferred foreign memory map.
+ */
+ if ( d != dom_shared )
+ {
+ shm_list[shm_id].owner_dom = d->domain_id;
+ shm_list[shm_id].owner_gbase = gbase;
+ shm_list[shm_id].size = psize;
+ set_bit(shm_id, shm_list_mask);
+ }
+
return ret;
}
@@ -962,6 +974,26 @@ static int __init process_shm(struct domain *d,
if ( ret )
return ret;
}
+ else
+ {
+ if ( strcmp(role_str, "borrower") == 0 )
+ {
+ /*
+ * In a few scenarios where owner domain, is defined after
+ * borrower domain in device tree configuration, statically
+ * shared pages haven't been properly allocated if borrower
+ * domain here tries to do foreign memory map.
+ * In order to cover such scenario, we defer all borrower
+ * domains'foreign memory map after all domain construction
+ * finished, and only store shm-info here for later use.
+ */
+ shm_list[shm_id].borrower_dom[shm_list[shm_id].nr_borrower] =
+ d->domain_id;
+ shm_list[shm_id].borrower_gbase[shm_list[shm_id].nr_borrower] =
+ gbase;
+ shm_list[shm_id].nr_borrower++;
+ }
+ }
/*
* Record static shared memory region info for later setting
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 6df37d2c46..1c0f2e22ca 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -10,6 +10,7 @@
#include <asm/gic.h>
#include <asm/vgic.h>
#include <asm/vpl011.h>
+#include <asm/setup.h>
#include <public/hvm/params.h>
struct hvm_domain
@@ -33,6 +34,30 @@ enum domain_type {
#ifdef CONFIG_STATIC_SHM
extern struct domain *dom_shared;
+
+/* Maximum number of borrower domains. */
+#define NR_SHM_DOMAIN 32
+/*
+ * shm_list is indexed by unique identifier "xen,shm-id", but it only stores
+ * a subset of static shared memory regions, of which owner domain is not the
+ * default dom_shared.
+ * shm_list_mask bitmask is to record the position of these static shared
+ * memory regions.
+ * Per bit represents a entry in shm_list, and setting it 1 means the
+ * static shared memory region here is owned by a specific domain, then bit 0
+ * means the static shared memory region here is either owned by the default
+ * dom_shared or no static shared memory region here at all.
+ */
+typedef struct {
+ domid_t owner_dom;
+ paddr_t owner_gbase;
+ paddr_t size;
+ domid_t borrower_dom[NR_SHM_DOMAIN];
+ paddr_t borrower_gbase[NR_SHM_DOMAIN];
+ unsigned long nr_borrower;
+} shm_info_t;
+extern shm_info_t shm_list[NR_MEM_BANKS];
+extern unsigned long shm_list_mask[BITS_TO_LONGS(NR_MEM_BANKS)];
#else
#define dom_shared NULL
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map
2022-03-11 6:11 ` [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map Penny Zheng
@ 2022-03-18 2:01 ` Stefano Stabellini
2022-03-29 8:37 ` Penny Zheng
0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-03-18 2:01 UTC (permalink / raw)
To: Penny Zheng
Cc: xen-devel, nd, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Fri, 11 Mar 2022, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> In a few scenarios where owner domain, is defined after borrower domain in
> device tree configuration, then statically shared pages haven't been properly
> allocated if borrower domain tries to do foreign memory map during
> domain construction.
>
> In order to cover such scenario, we defer all borrower domains' foreign
> memory map after all domain construction finished, then only need to store
> shm-info during domain construction.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain.c | 3 +++
> xen/arch/arm/domain_build.c | 34 ++++++++++++++++++++++++++++++-
> xen/arch/arm/include/asm/domain.h | 25 +++++++++++++++++++++++
> 3 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index f0bfd67fe5..73ffbfb918 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -47,6 +47,9 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
>
> #ifdef CONFIG_STATIC_SHM
> struct domain *__read_mostly dom_shared;
> +
> +shm_info_t shm_list[NR_MEM_BANKS];
Instead of adding shm_list, maybe we can we re-use mem->bank
(bootinfo.reserved_mem)?
It is already storing the physical address and size (added in patch #1
with process_shm_node). We should be able to find the other info from
the mfn: mfn_to_page, page_get_owner, mfn_to_gfn. At most, we need to
mark the memory bank as shared and we could do that with another field
in struct membank.
> +DECLARE_BITMAP(shm_list_mask, NR_MEM_BANKS);
This is the third bitmask we introduce :-)
Can we narrow it down to a single bitmask? Maybe we don't need it at all
if we switch to using bootinfo.mem.bank.
> #endif
>
> static void do_idle(void)
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7ee4d33e0b..4b19160674 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -771,7 +771,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
>
> }
>
> -static int __init allocate_shared_memory(struct domain *d,
> +static int __init allocate_shared_memory(struct domain *d, u32 shm_id,
No need for it to be u32?
> u32 addr_cells, u32 size_cells,
> paddr_t pbase, paddr_t psize,
> paddr_t gbase)
> @@ -795,6 +795,18 @@ static int __init allocate_shared_memory(struct domain *d,
> return ret;
> }
>
> + /*
> + * If owner domain is not default dom_shared, shm-info of owner domain
> + * shall also be recorded for later deferred foreign memory map.
> + */
> + if ( d != dom_shared )
> + {
> + shm_list[shm_id].owner_dom = d->domain_id;
> + shm_list[shm_id].owner_gbase = gbase;
> + shm_list[shm_id].size = psize;
> + set_bit(shm_id, shm_list_mask);
> + }
> return ret;
> }
>
> @@ -962,6 +974,26 @@ static int __init process_shm(struct domain *d,
> if ( ret )
> return ret;
> }
> + else
> + {
> + if ( strcmp(role_str, "borrower") == 0 )
> + {
> + /*
> + * In a few scenarios where owner domain, is defined after
> + * borrower domain in device tree configuration, statically
> + * shared pages haven't been properly allocated if borrower
> + * domain here tries to do foreign memory map.
> + * In order to cover such scenario, we defer all borrower
> + * domains'foreign memory map after all domain construction
> + * finished, and only store shm-info here for later use.
> + */
> + shm_list[shm_id].borrower_dom[shm_list[shm_id].nr_borrower] =
> + d->domain_id;
> + shm_list[shm_id].borrower_gbase[shm_list[shm_id].nr_borrower] =
> + gbase;
> + shm_list[shm_id].nr_borrower++;
> + }
> + }
Maybe we don't need to defer this at all. guest_physmap_add_shm does
only two things:
1) get a page ref using the owner domain
2) add page to borrower p2m
We can do 2) straight away even if the owner is not yet allocated.
For 1), we need to get the right amount of references when the owner is
allocated (which could be after the borrowers).
Keeping in mind that we have already parsed all the info in
xen/arch/arm/bootfdt.c:process_shm_node, I wonder if we can add a field
to mem->bank[mem->nr_banks] to keep a count of the number of borrowers.
Then when we allocate the page to the owner here, we just get as many
additional reference as the number of borrowers.
This would:
- add a field to bootinfo.reserved_mem
- remove the need for shm_list
- remove the need for shm_list_mask
- remove the need for the deferral
Just trying to make things simpler :-)
> /*
> * Record static shared memory region info for later setting
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index 6df37d2c46..1c0f2e22ca 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -10,6 +10,7 @@
> #include <asm/gic.h>
> #include <asm/vgic.h>
> #include <asm/vpl011.h>
> +#include <asm/setup.h>
> #include <public/hvm/params.h>
>
> struct hvm_domain
> @@ -33,6 +34,30 @@ enum domain_type {
>
> #ifdef CONFIG_STATIC_SHM
> extern struct domain *dom_shared;
> +
> +/* Maximum number of borrower domains. */
> +#define NR_SHM_DOMAIN 32
> +/*
> + * shm_list is indexed by unique identifier "xen,shm-id", but it only stores
> + * a subset of static shared memory regions, of which owner domain is not the
> + * default dom_shared.
> + * shm_list_mask bitmask is to record the position of these static shared
> + * memory regions.
> + * Per bit represents a entry in shm_list, and setting it 1 means the
> + * static shared memory region here is owned by a specific domain, then bit 0
> + * means the static shared memory region here is either owned by the default
> + * dom_shared or no static shared memory region here at all.
> + */
> +typedef struct {
> + domid_t owner_dom;
> + paddr_t owner_gbase;
> + paddr_t size;
> + domid_t borrower_dom[NR_SHM_DOMAIN];
> + paddr_t borrower_gbase[NR_SHM_DOMAIN];
> + unsigned long nr_borrower;
> +} shm_info_t;
> +extern shm_info_t shm_list[NR_MEM_BANKS];
> +extern unsigned long shm_list_mask[BITS_TO_LONGS(NR_MEM_BANKS)];
> #else
> #define dom_shared NULL
> #endif
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map
2022-03-18 2:01 ` Stefano Stabellini
@ 2022-03-29 8:37 ` Penny Zheng
2022-04-08 22:46 ` Stefano Stabellini
0 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-29 8:37 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, nd, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano
> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Friday, March 18, 2022 10:01 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign
> memory map
>
> On Fri, 11 Mar 2022, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > In a few scenarios where owner domain, is defined after borrower
> > domain in device tree configuration, then statically shared pages
> > haven't been properly allocated if borrower domain tries to do foreign
> > memory map during domain construction.
> >
> > In order to cover such scenario, we defer all borrower domains'
> > foreign memory map after all domain construction finished, then only
> > need to store shm-info during domain construction.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain.c | 3 +++
> > xen/arch/arm/domain_build.c | 34 ++++++++++++++++++++++++++++++-
> > xen/arch/arm/include/asm/domain.h | 25 +++++++++++++++++++++++
> > 3 files changed, 61 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index
> > f0bfd67fe5..73ffbfb918 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -47,6 +47,9 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> >
> > #ifdef CONFIG_STATIC_SHM
> > struct domain *__read_mostly dom_shared;
> > +
> > +shm_info_t shm_list[NR_MEM_BANKS];
>
> Instead of adding shm_list, maybe we can we re-use mem->bank
> (bootinfo.reserved_mem)?
>
> It is already storing the physical address and size (added in patch #1 with
> process_shm_node). We should be able to find the other info from the mfn:
> mfn_to_page, page_get_owner, mfn_to_gfn. At most, we need to mark the
> memory bank as shared and we could do that with another field in struct
> membank.
>
>
> > +DECLARE_BITMAP(shm_list_mask, NR_MEM_BANKS);
>
> This is the third bitmask we introduce :-)
>
> Can we narrow it down to a single bitmask? Maybe we don't need it at all if we
> switch to using bootinfo.mem.bank.
>
>
> > #endif
> >
> > static void do_idle(void)
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 7ee4d33e0b..4b19160674 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -771,7 +771,7 @@ static mfn_t __init
> > acquire_shared_memory_bank(struct domain *d,
> >
> > }
> >
> > -static int __init allocate_shared_memory(struct domain *d,
> > +static int __init allocate_shared_memory(struct domain *d, u32
> > +shm_id,
>
> No need for it to be u32?
>
>
> > u32 addr_cells, u32 size_cells,
> > paddr_t pbase, paddr_t psize,
> > paddr_t gbase) @@ -795,6
> > +795,18 @@ static int __init allocate_shared_memory(struct domain *d,
> > return ret;
> > }
> >
> > + /*
> > + * If owner domain is not default dom_shared, shm-info of owner domain
> > + * shall also be recorded for later deferred foreign memory map.
> > + */
> > + if ( d != dom_shared )
> > + {
> > + shm_list[shm_id].owner_dom = d->domain_id;
> > + shm_list[shm_id].owner_gbase = gbase;
> > + shm_list[shm_id].size = psize;
> > + set_bit(shm_id, shm_list_mask);
> > + }
> > return ret;
> > }
> >
> > @@ -962,6 +974,26 @@ static int __init process_shm(struct domain *d,
> > if ( ret )
> > return ret;
> > }
> > + else
> > + {
> > + if ( strcmp(role_str, "borrower") == 0 )
> > + {
> > + /*
> > + * In a few scenarios where owner domain, is defined after
> > + * borrower domain in device tree configuration, statically
> > + * shared pages haven't been properly allocated if borrower
> > + * domain here tries to do foreign memory map.
> > + * In order to cover such scenario, we defer all borrower
> > + * domains'foreign memory map after all domain construction
> > + * finished, and only store shm-info here for later use.
> > + */
> > + shm_list[shm_id].borrower_dom[shm_list[shm_id].nr_borrower] =
> > + d->domain_id;
> > + shm_list[shm_id].borrower_gbase[shm_list[shm_id].nr_borrower] =
> > + gbase;
> > + shm_list[shm_id].nr_borrower++;
> > + }
> > + }
>
> Maybe we don't need to defer this at all. guest_physmap_add_shm does only
> two things:
>
> 1) get a page ref using the owner domain
> 2) add page to borrower p2m
>
>
> We can do 2) straight away even if the owner is not yet allocated.
>
> For 1), we need to get the right amount of references when the owner is
> allocated (which could be after the borrowers).
>
> Keeping in mind that we have already parsed all the info in
> xen/arch/arm/bootfdt.c:process_shm_node, I wonder if we can add a field to
> mem->bank[mem->nr_banks] to keep a count of the number of borrowers.
>
> Then when we allocate the page to the owner here, we just get as many
> additional reference as the number of borrowers.
>
> This would:
> - add a field to bootinfo.reserved_mem
> - remove the need for shm_list
> - remove the need for shm_list_mask
> - remove the need for the deferral
>
> Just trying to make things simpler :-)
>
Thanks for the detailed comments.
Here is what I thought and understood, PLZ correct me if I'm wrong. ;)
'''
> For 1), we need to get the right amount of references when the owner is
> allocated (which could be after the borrowers).
'''
We could add another field `nr_shm_borrowers` in struct membank to
keep a count of the number of borrowers, which is also the number of
the reference count.
And like you said, it shall be done in xen/arch/arm/bootfdt.c:process_shm_node.
The only shortcoming here is that we need to iterate the parsed shm
mem banks each time to do the increment, since we used shm_mask
at first to treat space with time, but it really could decrease the
complexity here and remove all the defer codes.
But here is another concern about the shm_list. Another reason why I
created shm_list is that when destroyed domain is an owner domain,
we need to unmap shared memory for all borrower domains too. IMO,
so that it fits the definition of owner and borrower. It is also
what commit "xen/arm: unmap foreign memory mapping when
destroyed domain is owner domain" is trying to solve.
we need a way to find all borrower domains info, when you
only know the owner domain.
Based on your suggestion, I suggest to add new field
` domid_t *shm_borrower_dom` and `paddr_t *shm_borrower_gbase`
in struct membank. And both two fields are going to be allocated and set
during domain construction phase xen/arch/arm/domain_build.c:process_shm.
Another thing is that maybe we could not store all above shared mem
banks in bootinfo.reserved_mem, since it is only valid during boot time,
and we need that info also for domain deconstruction at runtime.
Another struct meminfo shm_list may still also be needed.
BTW, I suggest that the index of shm_list.bank is shm_id
In case user abuse it in device tree configuration, we let xen to allocate.
In xen/arch/arm/bootfdt.c:process_shm_node, each time parsing a new
shm node, iterate the whole shm_list and only if the physical
address and size of all entries are not matched, we allocate a new bank for the
shm node.
>
> > /*
> > * Record static shared memory region info for later setting
> > diff --git a/xen/arch/arm/include/asm/domain.h
> > b/xen/arch/arm/include/asm/domain.h
> > index 6df37d2c46..1c0f2e22ca 100644
> > --- a/xen/arch/arm/include/asm/domain.h
> > +++ b/xen/arch/arm/include/asm/domain.h
> > @@ -10,6 +10,7 @@
> > #include <asm/gic.h>
> > #include <asm/vgic.h>
> > #include <asm/vpl011.h>
> > +#include <asm/setup.h>
> > #include <public/hvm/params.h>
> >
> > struct hvm_domain
> > @@ -33,6 +34,30 @@ enum domain_type {
> >
> > #ifdef CONFIG_STATIC_SHM
> > extern struct domain *dom_shared;
> > +
> > +/* Maximum number of borrower domains. */ #define NR_SHM_DOMAIN
> 32
> > +/*
> > + * shm_list is indexed by unique identifier "xen,shm-id", but it only
> > +stores
> > + * a subset of static shared memory regions, of which owner domain is
> > +not the
> > + * default dom_shared.
> > + * shm_list_mask bitmask is to record the position of these static
> > +shared
> > + * memory regions.
> > + * Per bit represents a entry in shm_list, and setting it 1 means the
> > + * static shared memory region here is owned by a specific domain,
> > +then bit 0
> > + * means the static shared memory region here is either owned by the
> > +default
> > + * dom_shared or no static shared memory region here at all.
> > + */
> > +typedef struct {
> > + domid_t owner_dom;
> > + paddr_t owner_gbase;
> > + paddr_t size;
> > + domid_t borrower_dom[NR_SHM_DOMAIN];
> > + paddr_t borrower_gbase[NR_SHM_DOMAIN];
> > + unsigned long nr_borrower;
> > +} shm_info_t;
> > +extern shm_info_t shm_list[NR_MEM_BANKS]; extern unsigned long
> > +shm_list_mask[BITS_TO_LONGS(NR_MEM_BANKS)];
> > #else
> > #define dom_shared NULL
> > #endif
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 51+ messages in thread* RE: [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map
2022-03-29 8:37 ` Penny Zheng
@ 2022-04-08 22:46 ` Stefano Stabellini
2022-04-09 9:14 ` Julien Grall
0 siblings, 1 reply; 51+ messages in thread
From: Stefano Stabellini @ 2022-04-08 22:46 UTC (permalink / raw)
To: Penny Zheng
Cc: Stefano Stabellini, xen-devel, nd, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
On Tue, 29 Mar 2022, Penny Zheng wrote:
> > On Fri, 11 Mar 2022, Penny Zheng wrote:
> > > From: Penny Zheng <penny.zheng@arm.com>
> > >
> > > In a few scenarios where owner domain, is defined after borrower
> > > domain in device tree configuration, then statically shared pages
> > > haven't been properly allocated if borrower domain tries to do foreign
> > > memory map during domain construction.
> > >
> > > In order to cover such scenario, we defer all borrower domains'
> > > foreign memory map after all domain construction finished, then only
> > > need to store shm-info during domain construction.
> > >
> > > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > > ---
> > > xen/arch/arm/domain.c | 3 +++
> > > xen/arch/arm/domain_build.c | 34 ++++++++++++++++++++++++++++++-
> > > xen/arch/arm/include/asm/domain.h | 25 +++++++++++++++++++++++
> > > 3 files changed, 61 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index
> > > f0bfd67fe5..73ffbfb918 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -47,6 +47,9 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> > >
> > > #ifdef CONFIG_STATIC_SHM
> > > struct domain *__read_mostly dom_shared;
> > > +
> > > +shm_info_t shm_list[NR_MEM_BANKS];
> >
> > Instead of adding shm_list, maybe we can we re-use mem->bank
> > (bootinfo.reserved_mem)?
> >
> > It is already storing the physical address and size (added in patch #1 with
> > process_shm_node). We should be able to find the other info from the mfn:
> > mfn_to_page, page_get_owner, mfn_to_gfn. At most, we need to mark the
> > memory bank as shared and we could do that with another field in struct
> > membank.
> >
> >
> > > +DECLARE_BITMAP(shm_list_mask, NR_MEM_BANKS);
> >
> > This is the third bitmask we introduce :-)
> >
> > Can we narrow it down to a single bitmask? Maybe we don't need it at all if we
> > switch to using bootinfo.mem.bank.
> >
> >
> > > #endif
> > >
> > > static void do_idle(void)
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 7ee4d33e0b..4b19160674 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -771,7 +771,7 @@ static mfn_t __init
> > > acquire_shared_memory_bank(struct domain *d,
> > >
> > > }
> > >
> > > -static int __init allocate_shared_memory(struct domain *d,
> > > +static int __init allocate_shared_memory(struct domain *d, u32
> > > +shm_id,
> >
> > No need for it to be u32?
> >
> >
> > > u32 addr_cells, u32 size_cells,
> > > paddr_t pbase, paddr_t psize,
> > > paddr_t gbase) @@ -795,6
> > > +795,18 @@ static int __init allocate_shared_memory(struct domain *d,
> > > return ret;
> > > }
> > >
> > > + /*
> > > + * If owner domain is not default dom_shared, shm-info of owner domain
> > > + * shall also be recorded for later deferred foreign memory map.
> > > + */
> > > + if ( d != dom_shared )
> > > + {
> > > + shm_list[shm_id].owner_dom = d->domain_id;
> > > + shm_list[shm_id].owner_gbase = gbase;
> > > + shm_list[shm_id].size = psize;
> > > + set_bit(shm_id, shm_list_mask);
> > > + }
> > > return ret;
> > > }
> > >
> > > @@ -962,6 +974,26 @@ static int __init process_shm(struct domain *d,
> > > if ( ret )
> > > return ret;
> > > }
> > > + else
> > > + {
> > > + if ( strcmp(role_str, "borrower") == 0 )
> > > + {
> > > + /*
> > > + * In a few scenarios where owner domain, is defined after
> > > + * borrower domain in device tree configuration, statically
> > > + * shared pages haven't been properly allocated if borrower
> > > + * domain here tries to do foreign memory map.
> > > + * In order to cover such scenario, we defer all borrower
> > > + * domains'foreign memory map after all domain construction
> > > + * finished, and only store shm-info here for later use.
> > > + */
> > > + shm_list[shm_id].borrower_dom[shm_list[shm_id].nr_borrower] =
> > > + d->domain_id;
> > > + shm_list[shm_id].borrower_gbase[shm_list[shm_id].nr_borrower] =
> > > + gbase;
> > > + shm_list[shm_id].nr_borrower++;
> > > + }
> > > + }
> >
> > Maybe we don't need to defer this at all. guest_physmap_add_shm does only
> > two things:
> >
> > 1) get a page ref using the owner domain
> > 2) add page to borrower p2m
> >
> >
> > We can do 2) straight away even if the owner is not yet allocated.
> >
> > For 1), we need to get the right amount of references when the owner is
> > allocated (which could be after the borrowers).
> >
> > Keeping in mind that we have already parsed all the info in
> > xen/arch/arm/bootfdt.c:process_shm_node, I wonder if we can add a field to
> > mem->bank[mem->nr_banks] to keep a count of the number of borrowers.
> >
> > Then when we allocate the page to the owner here, we just get as many
> > additional reference as the number of borrowers.
> >
> > This would:
> > - add a field to bootinfo.reserved_mem
> > - remove the need for shm_list
> > - remove the need for shm_list_mask
> > - remove the need for the deferral
> >
> > Just trying to make things simpler :-)
> >
>
> Thanks for the detailed comments.
> Here is what I thought and understood, PLZ correct me if I'm wrong. ;)
> '''
> > For 1), we need to get the right amount of references when the owner is
> > allocated (which could be after the borrowers).
> '''
> We could add another field `nr_shm_borrowers` in struct membank to
> keep a count of the number of borrowers, which is also the number of
> the reference count.
Yes
> And like you said, it shall be done in xen/arch/arm/bootfdt.c:process_shm_node.
> The only shortcoming here is that we need to iterate the parsed shm
> mem banks each time to do the increment, since we used shm_mask
> at first to treat space with time, but it really could decrease the
> complexity here and remove all the defer codes.
Yes, that's true, but the number of elements in the array should be very
small. We can always optimized data structures later if needed. Also, we
would save one loop in shm_init_late.
> But here is another concern about the shm_list. Another reason why I
> created shm_list is that when destroyed domain is an owner domain,
> we need to unmap shared memory for all borrower domains too. IMO,
> so that it fits the definition of owner and borrower. It is also
> what commit "xen/arm: unmap foreign memory mapping when
> destroyed domain is owner domain" is trying to solve.
Actually, I am not sure about this. I don't think that we need to unmap
the memory at the borrowers. We just decrease the ref count on the
pages. When the borrower domains die, the ref count goes to zero and the
pages are finally freed.
So I don't think that when the owner dies, we need to actively go and
unmap the pages at the borrowers. Also because it would likely cause
them to crash: from their point of view the memory was there, and
suddenly it is not there anymore.
But we don't need to do anything special to decrease the ref count on
the shared pages because it would happen automatically when the owner
domain is destroyed.
> we need a way to find all borrower domains info, when you
> only know the owner domain.
> Based on your suggestion, I suggest to add new field
> ` domid_t *shm_borrower_dom` and `paddr_t *shm_borrower_gbase`
> in struct membank. And both two fields are going to be allocated and set
> during domain construction phase xen/arch/arm/domain_build.c:process_shm.
>
> Another thing is that maybe we could not store all above shared mem
> banks in bootinfo.reserved_mem, since it is only valid during boot time,
> and we need that info also for domain deconstruction at runtime.
> Another struct meminfo shm_list may still also be needed.
>
> BTW, I suggest that the index of shm_list.bank is shm_id
> In case user abuse it in device tree configuration, we let xen to allocate.
> In xen/arch/arm/bootfdt.c:process_shm_node, each time parsing a new
> shm node, iterate the whole shm_list and only if the physical
> address and size of all entries are not matched, we allocate a new bank for the
> shm node.
>
> >
> > > /*
> > > * Record static shared memory region info for later setting
> > > diff --git a/xen/arch/arm/include/asm/domain.h
> > > b/xen/arch/arm/include/asm/domain.h
> > > index 6df37d2c46..1c0f2e22ca 100644
> > > --- a/xen/arch/arm/include/asm/domain.h
> > > +++ b/xen/arch/arm/include/asm/domain.h
> > > @@ -10,6 +10,7 @@
> > > #include <asm/gic.h>
> > > #include <asm/vgic.h>
> > > #include <asm/vpl011.h>
> > > +#include <asm/setup.h>
> > > #include <public/hvm/params.h>
> > >
> > > struct hvm_domain
> > > @@ -33,6 +34,30 @@ enum domain_type {
> > >
> > > #ifdef CONFIG_STATIC_SHM
> > > extern struct domain *dom_shared;
> > > +
> > > +/* Maximum number of borrower domains. */ #define NR_SHM_DOMAIN
> > 32
> > > +/*
> > > + * shm_list is indexed by unique identifier "xen,shm-id", but it only
> > > +stores
> > > + * a subset of static shared memory regions, of which owner domain is
> > > +not the
> > > + * default dom_shared.
> > > + * shm_list_mask bitmask is to record the position of these static
> > > +shared
> > > + * memory regions.
> > > + * Per bit represents a entry in shm_list, and setting it 1 means the
> > > + * static shared memory region here is owned by a specific domain,
> > > +then bit 0
> > > + * means the static shared memory region here is either owned by the
> > > +default
> > > + * dom_shared or no static shared memory region here at all.
> > > + */
> > > +typedef struct {
> > > + domid_t owner_dom;
> > > + paddr_t owner_gbase;
> > > + paddr_t size;
> > > + domid_t borrower_dom[NR_SHM_DOMAIN];
> > > + paddr_t borrower_gbase[NR_SHM_DOMAIN];
> > > + unsigned long nr_borrower;
> > > +} shm_info_t;
> > > +extern shm_info_t shm_list[NR_MEM_BANKS]; extern unsigned long
> > > +shm_list_mask[BITS_TO_LONGS(NR_MEM_BANKS)];
> > > #else
> > > #define dom_shared NULL
> > > #endif
> > > --
> > > 2.25.1
> > >
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map
2022-04-08 22:46 ` Stefano Stabellini
@ 2022-04-09 9:14 ` Julien Grall
0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2022-04-09 9:14 UTC (permalink / raw)
To: Stefano Stabellini, Penny Zheng
Cc: xen-devel, nd, Bertrand Marquis, Volodymyr Babchuk
Hi Stefano,
On 08/04/2022 23:46, Stefano Stabellini wrote:
> So I don't think that when the owner dies, we need to actively go and
> unmap the pages at the borrowers. Also because it would likely cause
> them to crash: from their point of view the memory was there, and
> suddenly it is not there anymore.
I agree with that. Also, the code is likely going to be quite complex
because there are a lot of things that can go wrong (e.g. the domain ID
has been re-used). I will comment there directly.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread
* [PATCH v1 12/13] xen/arm: defer foreign memory map in shm_init_late
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (10 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 11/13] xen/arm: store shm-info for deferred foreign memory map Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-03-11 6:11 ` [PATCH v1 13/13] xen/arm: unmap foreign memory mapping when destroyed domain is owner domain Penny Zheng
12 siblings, 0 replies; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
This commit introduces a new helper shm_init_late to implement
deferred foreign memory mapping of static shared memory.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 51 +++++++++++++++++++++++++++++++
xen/arch/arm/include/asm/domain.h | 1 +
xen/arch/arm/setup.c | 6 ++++
3 files changed, 58 insertions(+)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4b19160674..f6ef5a702f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1006,6 +1006,57 @@ static int __init process_shm(struct domain *d,
return 0;
}
+
+int __init shm_init_late(void)
+{
+ unsigned long i = 0UL, shm_id;
+ int ret = 0;
+ struct domain *od, **bd = NULL;
+ unsigned long o_gfn, b_gfn, nr_gfns;
+
+ for ( shm_id = find_first_bit(shm_list_mask, NR_MEM_BANKS);
+ shm_id < NR_MEM_BANKS;
+ shm_id = find_next_bit(shm_list_mask, NR_MEM_BANKS, shm_id + 1) )
+
+ {
+ /* Acquire the only owner domain. */
+ od = get_domain_by_id(shm_list[shm_id].owner_dom);
+ if ( od == NULL )
+ return -ESRCH;
+ o_gfn = PFN_DOWN(shm_list[shm_id].owner_gbase);
+ nr_gfns = PFN_DOWN(shm_list[shm_id].size);
+
+ bd = xmalloc_array(struct domain *, shm_list[shm_id].nr_borrower);
+ if ( !bd )
+ return -ENOMEM;
+ /* Set up foreign memory map for all borrower domains. */
+ for ( i = 0; i < shm_list[shm_id].nr_borrower; i++ )
+ {
+ bd[i] = get_domain_by_id(shm_list[shm_id].borrower_dom[i]);
+ if ( bd[i] == NULL )
+ {
+ return -ESRCH;
+ goto fail;
+ }
+
+ b_gfn = PFN_DOWN(shm_list[shm_id].borrower_gbase[i]);
+ ret = guest_physmap_add_shm(od, bd[i], o_gfn, b_gfn, nr_gfns);
+ if ( ret )
+ {
+ ret = -EINVAL;
+ goto fail;
+ }
+ }
+
+ xfree(bd);
+ }
+ return ret;
+
+ fail:
+ xfree(bd);
+
+ return ret;
+}
#endif /* CONFIG_STATIC_SHM */
#else
static void __init allocate_static_memory(struct domain *d,
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 1c0f2e22ca..c3f2155f5c 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -58,6 +58,7 @@ typedef struct {
} shm_info_t;
extern shm_info_t shm_list[NR_MEM_BANKS];
extern unsigned long shm_list_mask[BITS_TO_LONGS(NR_MEM_BANKS)];
+extern int shm_init_late(void);
#else
#define dom_shared NULL
#endif
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index f6a3b04958..4987b71111 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1051,7 +1051,13 @@ void __init start_xen(unsigned long boot_phys_offset,
printk(XENLOG_INFO "Xen dom0less mode detected\n");
if ( acpi_disabled )
+ {
create_domUs();
+#ifdef CONFIG_STATIC_SHM
+ if ( shm_init_late() )
+ panic("Failed to set up deferred foreign memory mapping of static shared memory.\n");
+#endif
+ }
/*
* This needs to be called **before** heap_init_late() so modules
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH v1 13/13] xen/arm: unmap foreign memory mapping when destroyed domain is owner domain
2022-03-11 6:11 [PATCH v1 00/13] Static shared memory on dom0less system Penny Zheng
` (11 preceding siblings ...)
2022-03-11 6:11 ` [PATCH v1 12/13] xen/arm: defer foreign memory map in shm_init_late Penny Zheng
@ 2022-03-11 6:11 ` Penny Zheng
2022-04-09 9:44 ` Julien Grall
12 siblings, 1 reply; 51+ messages in thread
From: Penny Zheng @ 2022-03-11 6:11 UTC (permalink / raw)
To: xen-devel
Cc: nd, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
From: Penny Zheng <penny.zheng@arm.com>
When destroyed domain is an owner domain of a static shared memory
region, then we need to ensure that all according borrower domains
shall not have the access to this static shared memory region too.
This commit covers above scenario through unmapping all borrowers'
according foreign memory mapping when destroyed domain is a owner
domain of a static shared memory region.
NOTE: It will best for users to destroy all borrowers before the owner
domain in case encountering data abort when accidentally accessing
the static shared memory region.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain.c | 88 ++++++++++++++++++++++++++++++++++---------
1 file changed, 71 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 73ffbfb918..8f4a8dcbfc 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -998,10 +998,39 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
}
#ifdef CONFIG_STATIC_SHM
+static int destroy_shm(struct domain *d, gfn_t gfn, unsigned long nr_gfns)
+{
+ unsigned long i = 0;
+ int ret = 0;
+
+ for ( ; i < nr_gfns; i++ )
+ {
+ mfn_t mfn;
+
+ mfn = gfn_to_mfn(d, gfn_add(gfn, i));
+ if ( !mfn_valid(mfn) )
+ {
+ dprintk(XENLOG_ERR,
+ "Domain %pd page number %lx invalid.\n",
+ d, gfn_x(gfn) + i);
+ return -EINVAL;
+ }
+
+ ret = guest_physmap_remove_page(d, gfn_add(gfn, i), mfn, 0);
+ if ( ret )
+ return ret;
+
+ /* Drop the reference. */
+ put_page(mfn_to_page(mfn));
+ }
+
+ return ret;
+}
+
static int domain_destroy_shm(struct domain *d)
{
int ret = 0;
- unsigned long i = 0UL, j;
+ unsigned long i = 0UL;
if ( d->arch.shm_mem == NULL )
return ret;
@@ -1009,29 +1038,54 @@ static int domain_destroy_shm(struct domain *d)
{
for ( ; i < d->arch.shm_mem->nr_banks; i++ )
{
+ u32 shm_id = d->arch.shm_mem->bank[i].shm_id;
unsigned long nr_gfns = PFN_DOWN(d->arch.shm_mem->bank[i].size);
gfn_t gfn = gaddr_to_gfn(d->arch.shm_mem->bank[i].start);
- for ( j = 0; j < nr_gfns; j++ )
+ if ( test_bit(shm_id, shm_list_mask) )
{
- mfn_t mfn;
-
- mfn = gfn_to_mfn(d, gfn_add(gfn, j));
- if ( !mfn_valid(mfn) )
+ domid_t od = shm_list[shm_id].owner_dom;
+ unsigned long j;
+ /*
+ * If it is a owner domain, then after it gets destroyed,
+ * static shared memory region shall be unaccessible to all
+ * borrower domains too.
+ */
+ if ( d->domain_id == od )
{
- dprintk(XENLOG_ERR,
- "Domain %pd page number %lx invalid.\n",
- d, gfn_x(gfn) + i);
- return -EINVAL;
+ struct domain *bd;
+
+ for ( j = 0; j < shm_list[shm_id].nr_borrower; j++ )
+ {
+ bd = get_domain_by_id(shm_list[shm_id].borrower_dom[j]);
+ /*
+ * borrower domain could be dead already, in such case
+ * no need to do the unmapping.
+ */
+ if ( bd != NULL )
+ {
+ gfn_t b_gfn = gaddr_to_gfn(
+ shm_list[shm_id].borrower_gbase[j]);
+ ret = destroy_shm(bd, b_gfn, nr_gfns);
+ if ( ret )
+ dprintk(XENLOG_ERR,
+ "Domain %pd: failed to destroy static shared memory.\n",
+ bd);
+ }
+ }
+
+ continue;
}
-
- ret = guest_physmap_remove_page(d, gfn_add(gfn, j), mfn, 0);
- if ( ret )
- return ret;
-
- /* Drop the reference. */
- put_page(mfn_to_page(mfn));
}
+ /*
+ * As borrower domain, remove foreign memory mapping and drop the
+ * reference count.
+ */
+ ret = destroy_shm(d, gfn, nr_gfns);
+ if ( ret )
+ dprintk(XENLOG_ERR,
+ "Domain %pd: failed to destroy static shared memory.\n",
+ d);
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v1 13/13] xen/arm: unmap foreign memory mapping when destroyed domain is owner domain
2022-03-11 6:11 ` [PATCH v1 13/13] xen/arm: unmap foreign memory mapping when destroyed domain is owner domain Penny Zheng
@ 2022-04-09 9:44 ` Julien Grall
0 siblings, 0 replies; 51+ messages in thread
From: Julien Grall @ 2022-04-09 9:44 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: nd, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 11/03/2022 06:11, Penny Zheng wrote:
> From: Penny Zheng <penny.zheng@arm.com>
>
> When destroyed domain is an owner domain of a static shared memory
> region, then we need to ensure that all according borrower domains
> shall not have the access to this static shared memory region too.
As Stefano wrote, I don't think this is necessary. The page reference
accounting will keep the page alive until everyone have released the page.
So can you explain why you want to do that?
>
> This commit covers above scenario through unmapping all borrowers'
> according foreign memory mapping when destroyed domain is a owner
> domain of a static shared memory region.
>
> NOTE: It will best for users to destroy all borrowers before the owner
> domain in case encountering data abort when accidentally accessing
> the static shared memory region.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain.c | 88 ++++++++++++++++++++++++++++++++++---------
> 1 file changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 73ffbfb918..8f4a8dcbfc 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -998,10 +998,39 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
> }
>
> #ifdef CONFIG_STATIC_SHM
> +static int destroy_shm(struct domain *d, gfn_t gfn, unsigned long nr_gfns)
If you still plan to go ahead with this approach, then I would prefer if
this function is created in patch #8. This will help to reduce the churn
in this patch.
[...]
> - for ( j = 0; j < nr_gfns; j++ )
> + if ( test_bit(shm_id, shm_list_mask) )
> {
> - mfn_t mfn;
> -
> - mfn = gfn_to_mfn(d, gfn_add(gfn, j));
> - if ( !mfn_valid(mfn) )
> + domid_t od = shm_list[shm_id].owner_dom;
> + unsigned long j;
> + /*
> + * If it is a owner domain, then after it gets destroyed,
> + * static shared memory region shall be unaccessible to all
> + * borrower domains too.
> + */
> + if ( d->domain_id == od )
> {
> - dprintk(XENLOG_ERR,
> - "Domain %pd page number %lx invalid.\n",
> - d, gfn_x(gfn) + i);
> - return -EINVAL;
> + struct domain *bd;
> +
> + for ( j = 0; j < shm_list[shm_id].nr_borrower; j++ )
> + {
> + bd = get_domain_by_id(shm_list[shm_id].borrower_dom[j]);
> + /*
> + * borrower domain could be dead already, in such case
> + * no need to do the unmapping.
The domain ID could have been re-used. So it is not enough to lookup for
the ID.
> + */
> + if ( bd != NULL )
> + {
> + gfn_t b_gfn = gaddr_to_gfn(
> + shm_list[shm_id].borrower_gbase[j]);
> + ret = destroy_shm(bd, b_gfn, nr_gfns);
> + if ( ret )
> + dprintk(XENLOG_ERR,
> + "Domain %pd: failed to destroy static shared memory.\n",
> + bd);
In the commit message, you wrote you want to remove the pages from the
borrower. But here, you will ignore a failure and continue to destroy
the owner like nothing happened.
If you are concerned that the borrower can still use the pages. Then we
should make sure that they are removed in every cases.
However, I think the code is going to quite complex. So I think we
should consider to do nothing here and let the borrower use the pages
until they die.
Potentially, we could notify the borrowers that the owner died so they
can decide to remove/shutdown themself. Of course, it would mean we are
relying on the borrowers to be nice. An alternative would be to destroy
them.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 51+ messages in thread