* [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory
@ 2022-11-15 2:52 Penny Zheng
2022-11-15 2:52 ` [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region Penny Zheng
` (12 more replies)
0 siblings, 13 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
There are some unsolving issues on current 4.17 static shared memory
feature[1], including:
- In order to avoid keeping growing 'membank', having the shared memory
regions in a separate array is preferred. And shared memory region should
have its own structure which would contain a pointer/index to the 'membank'.
- Missing implementation on having the host address optional in
"xen,shared-mem" property
- Missing "xen,offset" feature, which is introduced in Linux DOC[2]
After fixing all above issues, we'd like to make 'UNSUPPORTED' static
shared memory feature 'SUPPORTED'.
[1] https://lore.kernel.org/all/20220908135513.1800511-1-Penny.Zheng@arm.com/
[2] https://www.kernel.org/doc/Documentation/devicetree/bindings/reserved-memory/xen%2Cshared-memory.txt
Penny Zheng (13):
xen/arm: re-arrange the static shared memory region
xen/arm: switch to use shm_membank as function parameter
xen/arm: introduce allocate_domheap_memory and guest_physmap_memory
xen/arm: expand shm_membank for unprovided host address
xen/arm: allocate shared memory from heap when host address not
provided
xen/arm: assign shared memory to owner when host address not provided
xen/arm: map shared memory to borrower when host address not provided
xen/arm: use paddr_assigned to indicate whether host address is
provided
xen/arm: refine docs about static shared memory
xen/arm: introduce "xen,offset" feature
xen/arm: implement "xen,offset" feature when host address provided
xen/arm: implement "xen,offset" feature when host address not provided
xen: make static shared memory supported in SUPPORT.md
SUPPORT.md | 2 +-
docs/misc/arm/device-tree/booting.txt | 67 ++-
xen/arch/arm/Kconfig | 2 +-
xen/arch/arm/bootfdt.c | 137 ++++--
xen/arch/arm/domain_build.c | 614 +++++++++++++++++++++-----
xen/arch/arm/include/asm/kernel.h | 2 +-
xen/arch/arm/include/asm/setup.h | 31 +-
7 files changed, 685 insertions(+), 170 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2023-01-08 11:44 ` Julien Grall
2023-02-07 20:55 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter Penny Zheng
` (11 subsequent siblings)
12 siblings, 2 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
This commit re-arranges the static shared memory regions into a separate array
shm_meminfo. And static shared memory region now would have its own structure
'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer
to the normal memory bank 'membank'. This will avoid continuing to grow
'membank'.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
xen/arch/arm/include/asm/kernel.h | 2 +-
xen/arch/arm/include/asm/setup.h | 16 +++++++++----
4 files changed, 59 insertions(+), 34 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 6014c0f852..ccf281cd37 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int node,
const __be32 *cell;
paddr_t paddr, gaddr, size;
struct meminfo *mem = &bootinfo.reserved_mem;
+ struct shm_meminfo *shm_mem = &bootinfo.shm_mem;
unsigned int i;
int len;
bool owner = false;
@@ -455,17 +456,21 @@ static int __init process_shm_node(const void *fdt, int node,
return -EINVAL;
}
- for ( i = 0; i < mem->nr_banks; i++ )
+ for ( i = 0; i < shm_mem->nr_banks; i++ )
{
+ paddr_t bank_start = shm_mem->bank[i].membank->start;
+ paddr_t bank_size = shm_mem->bank[i].membank->size;
+
/*
* Meet the following check:
* 1) The shm ID matches and the region exactly match
* 2) The shm ID doesn't match and the region doesn't overlap
* with an existing one
*/
- if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
+ if ( paddr == bank_start && size == bank_size )
{
- if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
+ if ( strncmp(shm_id,
+ shm_mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
break;
else
{
@@ -477,17 +482,17 @@ static int __init process_shm_node(const void *fdt, int node,
else
{
paddr_t end = paddr + size;
- paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
+ paddr_t bank_end = bank_start + bank_size;
- if ( (end <= paddr) || (bank_end <= mem->bank[i].start) )
+ if ( (end <= paddr) || (bank_end <= bank_start) )
{
printk("fdt: static shared memory region %s overflow\n", shm_id);
return -EINVAL;
}
- if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
+ if ( (end <= bank_start) || (paddr >= bank_end) )
{
- if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
+ if ( strcmp(shm_id, shm_mem->bank[i].shm_id) != 0 )
continue;
else
{
@@ -499,22 +504,27 @@ static int __init process_shm_node(const void *fdt, int node,
else
{
printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
- mem->bank[i].start, bank_end);
+ bank_start, bank_end);
return -EINVAL;
}
}
}
- if ( i == mem->nr_banks )
+ if ( i == shm_mem->nr_banks )
{
- if ( i < NR_MEM_BANKS )
+ if ( (i < NR_MEM_BANKS) && (mem->nr_banks < NR_MEM_BANKS) )
{
/* Static shared memory shall be reserved from any other use. */
- safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
- mem->bank[mem->nr_banks].start = paddr;
- mem->bank[mem->nr_banks].size = size;
- mem->bank[mem->nr_banks].type = MEMBANK_STATIC_DOMAIN;
+ struct membank *membank = &mem->bank[mem->nr_banks];
+
+ membank->start = paddr;
+ membank->size = size;
+ membank->type = MEMBANK_STATIC_DOMAIN;
mem->nr_banks++;
+
+ safe_strcpy(shm_mem->bank[i].shm_id, shm_id);
+ shm_mem->bank[i].membank = membank;
+ shm_mem->nr_banks++;
}
else
{
@@ -527,7 +537,7 @@ static int __init process_shm_node(const void *fdt, int node,
* to calculate the reference count.
*/
if ( !owner )
- mem->bank[i].nr_shm_borrowers++;
+ shm_mem->bank[i].nr_shm_borrowers++;
return 0;
}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index bd30d3798c..c0fd13f6ed 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct domain *d,
{
unsigned int bank;
- /* Iterate reserved memory to find requested shm bank. */
- for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
+ /* Iterate static shared memory to find requested shm bank. */
+ for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
{
- paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
- paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
+ paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
+ paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
if ( (pbase == bank_start) && (psize == bank_size) )
break;
}
- if ( bank == bootinfo.reserved_mem.nr_banks )
+ if ( bank == bootinfo.shm_mem.nr_banks )
return -ENOENT;
- *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
+ *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
return 0;
}
@@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
paddr_t start, paddr_t size,
const char *shm_id)
{
+ struct membank *membank;
+
if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
return -ENOMEM;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
+ membank = xmalloc_bytes(sizeof(struct membank));
+ if ( membank == NULL )
+ return -ENOMEM;
+
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size;
safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
kinfo->shm_mem.nr_banks++;
@@ -1355,7 +1362,7 @@ static int __init make_memory_node(const struct domain *d,
static int __init make_shm_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct meminfo *mem)
+ const struct shm_meminfo *mem)
{
unsigned int i = 0;
int res = 0;
@@ -1372,8 +1379,8 @@ static int __init make_shm_memory_node(const struct domain *d,
for ( ; i < mem->nr_banks; i++ )
{
- uint64_t start = mem->bank[i].start;
- uint64_t size = mem->bank[i].size;
+ uint64_t start = mem->bank[i].membank->start;
+ uint64_t size = mem->bank[i].membank->size;
/* Placeholder for xen-shmem@ + a 64-bit number + \0 */
char buf[27];
const char compat[] = "xen,shared-memory-v1";
@@ -1382,7 +1389,7 @@ static int __init make_shm_memory_node(const struct domain *d,
__be32 *cells;
unsigned int len = (addrcells + sizecells) * sizeof(__be32);
- snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
+ snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, start);
res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1426,7 +1433,7 @@ static int __init make_shm_memory_node(const struct domain *d,
static int __init make_shm_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct meminfo *mem)
+ const struct shm_meminfo *mem)
{
ASSERT_UNREACHABLE();
return -EOPNOTSUPP;
@@ -1436,7 +1443,7 @@ static int __init make_shm_memory_node(const struct domain *d,
static int __init make_resv_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- const struct meminfo *mem)
+ const struct shm_meminfo *mem)
{
int res = 0;
/* Placeholder for reserved-memory\0 */
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 5bb30c3f2f..f47ba9d619 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -38,7 +38,7 @@ struct kernel_info {
void *fdt; /* flat device tree */
paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
struct meminfo mem;
- struct meminfo shm_mem;
+ struct shm_meminfo shm_mem;
/* kernel entry point */
paddr_t entry;
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index fdbf68aadc..2d4ae0f00a 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -50,10 +50,6 @@ struct membank {
paddr_t start;
paddr_t size;
enum membank_type type;
-#ifdef CONFIG_STATIC_SHM
- char shm_id[MAX_SHM_ID_LENGTH];
- unsigned int nr_shm_borrowers;
-#endif
};
struct meminfo {
@@ -61,6 +57,17 @@ struct meminfo {
struct membank bank[NR_MEM_BANKS];
};
+struct shm_membank {
+ char shm_id[MAX_SHM_ID_LENGTH];
+ unsigned int nr_shm_borrowers;
+ struct membank *membank;
+};
+
+struct shm_meminfo {
+ unsigned int nr_banks;
+ struct shm_membank bank[NR_MEM_BANKS];
+};
+
/*
* The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
* The purpose of the domU flag is to avoid getting confused in
@@ -105,6 +112,7 @@ struct bootinfo {
struct meminfo acpi;
#endif
bool static_heap;
+ struct shm_meminfo shm_mem;
};
struct map_range_data
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
2022-11-15 2:52 ` [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 6:58 ` Jeungwoo Yoo
2023-01-08 11:56 ` Julien Grall
2022-11-15 2:52 ` [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory Penny Zheng
` (10 subsequent siblings)
12 siblings, 2 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
Instead of using multiple function parameters to deliver various shm-related
info, like host physical address, SHMID, etc, and with the introduction
of new struct "shm_membank", we could switch to use "shm_membank" as
function parameter to replace them all, to make codes more clear and
tidy.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c0fd13f6ed..d2b9e60b5c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain *d,
}
#ifdef CONFIG_STATIC_SHM
-static int __init acquire_nr_borrower_domain(struct domain *d,
- paddr_t pbase, paddr_t psize,
- unsigned long *nr_borrowers)
+static struct shm_membank * __init acquire_shm_membank(const char *shm_id)
{
unsigned int bank;
/* Iterate static shared memory to find requested shm bank. */
for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
- {
- paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
- paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
-
- if ( (pbase == bank_start) && (psize == bank_size) )
+ if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )
break;
- }
if ( bank == bootinfo.shm_mem.nr_banks )
- return -ENOENT;
-
- *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
+ return NULL;
- return 0;
+ return &bootinfo.shm_mem.bank[bank];
}
/*
* This function checks whether the static shared memory region is
* already allocated to dom_io.
*/
-static bool __init is_shm_allocated_to_domio(paddr_t pbase)
+static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
{
struct page_info *page;
struct domain *d;
- page = maddr_to_page(pbase);
+ page = maddr_to_page(shm_membank->membank->start);
d = page_get_owner_and_reference(page);
if ( d == NULL )
return false;
@@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
}
static int __init assign_shared_memory(struct domain *d,
- uint32_t addr_cells, uint32_t size_cells,
- paddr_t pbase, paddr_t psize,
+ struct shm_membank *shm_membank,
paddr_t gbase)
{
mfn_t smfn;
int ret = 0;
unsigned long nr_pages, nr_borrowers, i;
struct page_info *page;
+ paddr_t pbase, psize;
+
+ pbase = shm_membank->membank->start;
+ psize = shm_membank->membank->size;
printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
d, pbase, pbase + psize);
@@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
* Get the right amount of references per page, which is the number of
* borrower domains.
*/
- ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
- if ( ret )
- return ret;
+ nr_borrowers = shm_membank->nr_shm_borrowers;
/*
* Instead of letting borrower domain get a page ref, we add as many
@@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
const char *role_str;
const char *shm_id;
bool owner_dom_io = true;
+ struct shm_membank *shm_membank;
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
@@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
}
BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
+ shm_membank = acquire_shm_membank(shm_id);
+ if ( !shm_membank )
+ {
+ printk("%pd: failed to acquire %s shared memory bank\n",
+ d, shm_id);
+ return -ENOENT;
+ }
+
/*
* DOMID_IO is a fake domain and is not described in the Device-Tree.
* Therefore when the owner of the shared region is DOMID_IO, we will
* only find the borrowers.
*/
- if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
+ if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
(!owner_dom_io && strcmp(role_str, "owner") == 0) )
{
/*
@@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
* specified, so they should be assigned to dom_io.
*/
ret = assign_shared_memory(owner_dom_io ? dom_io : d,
- addr_cells, size_cells,
- pbase, psize, gbase);
+ shm_membank, gbase);
if ( ret )
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
2022-11-15 2:52 ` [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region Penny Zheng
2022-11-15 2:52 ` [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2023-02-07 20:56 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address Penny Zheng
` (9 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
We split the codes of allocate_bank_memory into two parts,
allocate_domheap_memory and guest_physmap_memory.
One is about allocating guest RAM from heap, which could be re-used later for
allocating static shared memory from heap when host address is not provided.
The other is building up guest P2M mapping.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 93 +++++++++++++++++++++++++++----------
1 file changed, 68 insertions(+), 25 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d2b9e60b5c..92763e96fc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -390,34 +390,18 @@ static void __init allocate_memory_11(struct domain *d,
}
}
-static bool __init allocate_bank_memory(struct domain *d,
- struct kernel_info *kinfo,
- gfn_t sgfn,
- paddr_t tot_size)
+static bool __init allocate_domheap_memory(struct domain *d,
+ paddr_t tot_size,
+ struct meminfo *mem)
{
- int res;
struct page_info *pg;
- struct membank *bank;
unsigned int max_order = ~0;
- /*
- * allocate_bank_memory can be called with a tot_size of zero for
- * the second memory bank. It is not an error and we can safely
- * avoid creating a zero-size memory bank.
- */
- if ( tot_size == 0 )
- return true;
-
- bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
- bank->start = gfn_to_gaddr(sgfn);
- bank->size = tot_size;
-
while ( tot_size > 0 )
{
unsigned int order = get_allocation_size(tot_size);
order = min(max_order, order);
-
pg = alloc_domheap_pages(d, order, 0);
if ( !pg )
{
@@ -437,15 +421,74 @@ static bool __init allocate_bank_memory(struct domain *d,
continue;
}
- res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
- if ( res )
- {
- dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+ if ( mem->nr_banks == NR_MEM_BANKS )
return false;
- }
+
+ mem->bank[mem->nr_banks].start = mfn_to_maddr(page_to_mfn(pg));
+ mem->bank[mem->nr_banks].size = 1UL << (PAGE_SHIFT + order);
+ mem->nr_banks++;
+ tot_size -= (1UL << (PAGE_SHIFT + order));
+ }
+
+ return true;
+}
+
+static int __init guest_physmap_memory(struct domain *d,
+ const struct meminfo *mem, gfn_t sgfn)
+{
+ unsigned int i;
+ int res;
+
+ for ( i = 0; i < mem->nr_banks; i++ )
+ {
+ paddr_t size = mem->bank[i].size;
+ unsigned int order = get_order_from_bytes(size);
+
+ /* Size must be power of two */
+ BUG_ON(!size || (size & (size - 1)));
+ res = guest_physmap_add_page(d, sgfn, maddr_to_mfn(mem->bank[i].start),
+ order);
+ if ( res )
+ return res;
sgfn = gfn_add(sgfn, 1UL << order);
- tot_size -= (1ULL << (PAGE_SHIFT + order));
+ }
+
+ return 0;
+}
+
+static bool __init allocate_bank_memory(struct domain *d,
+ struct kernel_info *kinfo,
+ gfn_t sgfn,
+ paddr_t total_size)
+{
+ struct membank *bank;
+ struct meminfo host = {0};
+
+ /*
+ * allocate_bank_memory can be called with a total_size of zero for
+ * the second memory bank. It is not an error and we can safely
+ * avoid creating a zero-size memory bank.
+ */
+ if ( total_size == 0 )
+ return true;
+
+ bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
+ bank->start = gfn_to_gaddr(sgfn);
+ bank->size = total_size;
+
+ if ( !allocate_domheap_memory(d, total_size, &host) )
+ {
+ printk(XENLOG_ERR "Failed to allocate (%"PRIpaddr"MB) pages to %pd\n",
+ total_size >> 20, d);
+ return false;
+ }
+
+ if ( guest_physmap_memory(d, &host, sgfn) )
+ {
+ printk(XENLOG_ERR "Failed to map (%"PRIpaddr"MB) pages to %pd\n",
+ total_size >> 20, d);
+ return false;
}
kinfo->mem.nr_banks++;
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (2 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2023-01-08 12:13 ` Julien Grall
2022-11-15 2:52 ` [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided Penny Zheng
` (8 subsequent siblings)
12 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
When host address is not provided in "xen,shared-mem", we let Xen
automatically allocate requested static shared memory from heap, and it
stands good chance of having multiple host memory banks allocated for the
requested static shared memory as a result. Therefore current membank is not
going to cover it.
This commit introduces a new field "mem" to cover both scenarios.
"struct membank" is used when host address is provided, whereas
"struct meminfo" shall be used when host address not provided.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/bootfdt.c | 6 +++---
xen/arch/arm/domain_build.c | 16 ++++++++--------
xen/arch/arm/include/asm/setup.h | 17 ++++++++++++++++-
3 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index ccf281cd37..2f34a8ea83 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -458,8 +458,8 @@ static int __init process_shm_node(const void *fdt, int node,
for ( i = 0; i < shm_mem->nr_banks; i++ )
{
- paddr_t bank_start = shm_mem->bank[i].membank->start;
- paddr_t bank_size = shm_mem->bank[i].membank->size;
+ paddr_t bank_start = shm_mem->bank[i].mem.bank->start;
+ paddr_t bank_size = shm_mem->bank[i].mem.bank->size;
/*
* Meet the following check:
@@ -523,7 +523,7 @@ static int __init process_shm_node(const void *fdt, int node,
mem->nr_banks++;
safe_strcpy(shm_mem->bank[i].shm_id, shm_id);
- shm_mem->bank[i].membank = membank;
+ shm_mem->bank[i].mem.bank = membank;
shm_mem->nr_banks++;
}
else
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 92763e96fc..fbb196d8a4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -818,7 +818,7 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
struct page_info *page;
struct domain *d;
- page = maddr_to_page(shm_membank->membank->start);
+ page = maddr_to_page(shm_membank->mem.bank->start);
d = page_get_owner_and_reference(page);
if ( d == NULL )
return false;
@@ -878,8 +878,8 @@ static int __init assign_shared_memory(struct domain *d,
struct page_info *page;
paddr_t pbase, psize;
- pbase = shm_membank->membank->start;
- psize = shm_membank->membank->size;
+ pbase = shm_membank->mem.bank->start;
+ psize = shm_membank->mem.bank->size;
printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
d, pbase, pbase + psize);
@@ -951,9 +951,9 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
if ( membank == NULL )
return -ENOMEM;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank = membank;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank->start = start;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank->size = size;
safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
kinfo->shm_mem.nr_banks++;
@@ -1422,8 +1422,8 @@ static int __init make_shm_memory_node(const struct domain *d,
for ( ; i < mem->nr_banks; i++ )
{
- uint64_t start = mem->bank[i].membank->start;
- uint64_t size = mem->bank[i].membank->size;
+ uint64_t start = mem->bank[i].mem.bank->start;
+ uint64_t size = mem->bank[i].mem.bank->size;
/* Placeholder for xen-shmem@ + a 64-bit number + \0 */
char buf[27];
const char compat[] = "xen,shared-memory-v1";
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 2d4ae0f00a..c54ffc8a5b 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -60,7 +60,22 @@ struct meminfo {
struct shm_membank {
char shm_id[MAX_SHM_ID_LENGTH];
unsigned int nr_shm_borrowers;
- struct membank *membank;
+ struct {
+ /*
+ * When host address is provided in "xen,shared-mem", then only one
+ * consistent host memory bank is behind each shared memory node.
+ */
+ struct membank *bank;
+ struct {
+ /*
+ * When host address is not provided in "xen,shared-mem", then
+ * we let Xen allocate requested memory from heap, and a shared
+ * memory bank could be consisted of multiple host memory banks.
+ */
+ struct meminfo *meminfo;
+ unsigned long total_size;
+ } banks;
+ } mem;
};
struct shm_meminfo {
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (3 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2023-01-08 12:22 ` Julien Grall
2023-02-07 20:57 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 06/13] xen/arm: assign shared memory to owner " Penny Zheng
` (7 subsequent siblings)
12 siblings, 2 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
when host address is not provided in "xen,shared-mem", we let Xen
allocate requested shared memory from heap, and once the shared memory is
allocated, it will be marked as static(PGC_static), which means that it will be
reserved as static memory, and will not go back to heap even on freeing.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 83 ++++++++++++++++++++++++++++++++++++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index fbb196d8a4..3de96882a5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -835,6 +835,72 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
return true;
}
+static int __init mark_shared_memory_static(struct shm_membank *shm_membank)
+{
+ unsigned int bank;
+ unsigned long i, nr_mfns;
+ struct page_info *pg;
+ struct meminfo *meminfo;
+
+ BUG_ON(!shm_membank->mem.banks.meminfo);
+ meminfo = shm_membank->mem.banks.meminfo;
+ for ( bank = 0; bank < meminfo->nr_banks; bank++ )
+ {
+ pg = mfn_to_page(maddr_to_mfn(meminfo->bank[bank].start));
+ nr_mfns = PFN_DOWN(meminfo->bank[bank].size);
+
+ for ( i = 0; i < nr_mfns; i++ )
+ {
+ /* The page should be already allocated from heap. */
+ if ( !pg[i].count_info & PGC_state_inuse )
+ {
+ printk(XENLOG_ERR
+ "pg[%lu] MFN %"PRI_mfn" c=%#lx\n",
+ i, mfn_x(page_to_mfn(pg)) + i, pg[i].count_info);
+ goto fail;
+ }
+
+ pg[i].count_info |= PGC_static;
+ }
+ }
+
+ return 0;
+
+ fail:
+ while ( bank >= 0 )
+ {
+ while ( --i >= 0 )
+ pg[i].count_info &= ~PGC_static;
+ i = PFN_DOWN(meminfo->bank[--bank].size);
+ }
+
+ return -EINVAL;
+}
+
+static int __init allocate_shared_memory(struct shm_membank *shm_membank,
+ paddr_t psize)
+{
+ struct meminfo *banks;
+ int ret;
+
+ BUG_ON(shm_membank->mem.banks.meminfo != NULL);
+
+ banks = xmalloc_bytes(sizeof(struct meminfo));
+ if ( banks == NULL )
+ return -ENOMEM;
+ shm_membank->mem.banks.meminfo = banks;
+ memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct meminfo));
+
+ if ( !allocate_domheap_memory(NULL, psize, shm_membank->mem.banks.meminfo) )
+ return -EINVAL;
+
+ ret = mark_shared_memory_static(shm_membank);
+ if ( ret )
+ return ret;
+
+ return 0;
+}
+
static mfn_t __init acquire_shared_memory_bank(struct domain *d,
paddr_t pbase, paddr_t psize)
{
@@ -975,7 +1041,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
unsigned int i;
const char *role_str;
const char *shm_id;
- bool owner_dom_io = true;
+ bool owner_dom_io = true, paddr_assigned = true;
struct shm_membank *shm_membank;
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
@@ -1035,6 +1101,21 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
return -ENOENT;
}
+ /*
+ * When host address is not provided in "xen,shared-mem",
+ * we let Xen allocate requested memory from heap at first domain.
+ */
+ if ( !paddr_assigned && !shm_membank->mem.banks.meminfo )
+ {
+ ret = allocate_shared_memory(shm_membank, psize);
+ if ( ret )
+ {
+ printk("%pd: failed to allocate shared memory bank(%"PRIpaddr"MB) from heap: %d\n",
+ d, psize >> 20, ret);
+ return ret;
+ }
+ }
+
/*
* DOMID_IO is a fake domain and is not described in the Device-Tree.
* Therefore when the owner of the shared region is DOMID_IO, we will
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (4 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2023-01-08 12:52 ` Julien Grall
2023-02-07 20:58 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 07/13] xen/arm: map shared memory to borrower " Penny Zheng
` (6 subsequent siblings)
12 siblings, 2 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
With the introduction of new scenario where host address is not provided
in "xen,shared-mem", the function "assign_shared_memory" shall be adapted
to it too.
Shared memory will already be allocated from heap, when calling
"assign_shared_memory" with unprovided host address.
So in "assign_shared_memory", we just need to assign these static shared pages
to its owner domain using function "assign_pages", and add as many
additional reference as the number of borrowers.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 160 ++++++++++++++++++++++++++++++------
1 file changed, 133 insertions(+), 27 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3de96882a5..faf0784bb0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -817,8 +817,12 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
{
struct page_info *page;
struct domain *d;
+ paddr_t pbase;
- page = maddr_to_page(shm_membank->mem.bank->start);
+ pbase = shm_membank->mem.banks.meminfo ?
+ shm_membank->mem.banks.meminfo->bank[0].start :
+ shm_membank->mem.bank->start;
+ page = maddr_to_page(pbase);
d = page_get_owner_and_reference(page);
if ( d == NULL )
return false;
@@ -907,6 +911,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
mfn_t smfn;
unsigned long nr_pfns;
int res;
+ struct page_info *page;
/*
* Pages of statically shared memory shall be included
@@ -922,33 +927,82 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
d->max_pages += nr_pfns;
smfn = maddr_to_mfn(pbase);
- res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
- if ( res )
+ page = mfn_to_page(smfn);
+ /*
+ * If page is allocated from heap as static shared memory, then we just
+ * assign it to the owner domain
+ */
+ if ( page->count_info == (PGC_state_inuse | PGC_static) )
{
- printk(XENLOG_ERR
- "%pd: failed to acquire static memory: %d.\n", d, res);
- d->max_pages -= nr_pfns;
- return INVALID_MFN;
+ res = assign_pages(page, nr_pfns, d, 0);
+ if ( res )
+ {
+ printk(XENLOG_ERR
+ "%pd: failed to assign static memory: %d.\n", d, res);
+ return INVALID_MFN;
+ }
+ }
+ else
+ {
+ res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
+ if ( res )
+ {
+ printk(XENLOG_ERR
+ "%pd: failed to acquire static memory: %d.\n", d, res);
+ d->max_pages -= nr_pfns;
+ return INVALID_MFN;
+ }
}
return smfn;
}
-static int __init assign_shared_memory(struct domain *d,
- struct shm_membank *shm_membank,
- paddr_t gbase)
+static void __init remove_shared_memory_ref(struct page_info *page,
+ unsigned long nr_pages,
+ unsigned long nr_borrowers)
{
- mfn_t smfn;
- int ret = 0;
- unsigned long nr_pages, nr_borrowers, i;
- struct page_info *page;
- paddr_t pbase, psize;
+ while ( --nr_pages >= 0 )
+ put_page_nr(page + nr_pages, nr_borrowers);
+}
- pbase = shm_membank->mem.bank->start;
- psize = shm_membank->mem.bank->size;
+static int __init add_shared_memory_ref(struct domain *d, struct page_info *page,
+ unsigned long nr_pages,
+ unsigned long nr_borrowers)
+{
+ unsigned int i;
- printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
- d, pbase, pbase + psize);
+ /*
+ * Instead of letting borrower domain get a page ref, we add as many
+ * additional reference as the number of borrowers when the owner
+ * is allocated, since there is a chance that owner is created
+ * after borrower.
+ * So if the borrower is created first, it will cause adding pages
+ * in the P2M without reference.
+ */
+ for ( i = 0; i < nr_pages; i++ )
+ {
+ if ( !get_page_nr(page + i, d, nr_borrowers) )
+ {
+ printk(XENLOG_ERR
+ "Failed to add %lu references to page %"PRI_mfn".\n",
+ nr_borrowers, mfn_x(page_to_mfn(page)) + i);
+ goto fail;
+ }
+ }
+ return 0;
+
+ fail:
+ remove_shared_memory_ref(page, i, nr_borrowers);
+ return -EINVAL;
+}
+
+static int __init acquire_shared_memory(struct domain *d,
+ paddr_t pbase, paddr_t psize,
+ paddr_t gbase)
+{
+ mfn_t smfn;
+ int ret = 0;
+ unsigned long nr_pages;
smfn = acquire_shared_memory_bank(d, pbase, psize);
if ( mfn_eq(smfn, INVALID_MFN) )
@@ -970,6 +1024,44 @@ static int __init assign_shared_memory(struct domain *d,
}
}
+ return 0;
+}
+
+static int __init assign_shared_memory(struct domain *d,
+ struct shm_membank *shm_membank,
+ paddr_t gbase)
+{
+ int ret = 0;
+ unsigned long nr_pages, nr_borrowers;
+ struct page_info *page;
+ unsigned int i;
+ struct meminfo *meminfo;
+
+ /* Host address is not provided in "xen,shared-mem" */
+ if ( shm_membank->mem.banks.meminfo )
+ {
+ meminfo = shm_membank->mem.banks.meminfo;
+ for ( i = 0; i < meminfo->nr_banks; i++ )
+ {
+ ret = acquire_shared_memory(d,
+ meminfo->bank[i].start,
+ meminfo->bank[i].size,
+ gbase);
+ if ( ret )
+ return ret;
+
+ gbase += meminfo->bank[i].size;
+ }
+ }
+ else
+ {
+ ret = acquire_shared_memory(d,
+ shm_membank->mem.bank->start,
+ shm_membank->mem.bank->size, gbase);
+ if ( ret )
+ return ret;
+ }
+
/*
* Get the right amount of references per page, which is the number of
* borrower domains.
@@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct domain *d,
* So if the borrower is created first, it will cause adding pages
* in the P2M without reference.
*/
- page = mfn_to_page(smfn);
- for ( i = 0; i < nr_pages; i++ )
+ if ( shm_membank->mem.banks.meminfo )
{
- if ( !get_page_nr(page + i, d, nr_borrowers) )
+ meminfo = shm_membank->mem.banks.meminfo;
+ for ( i = 0; i < meminfo->nr_banks; i++ )
{
- printk(XENLOG_ERR
- "Failed to add %lu references to page %"PRI_mfn".\n",
- nr_borrowers, mfn_x(smfn) + i);
- goto fail;
+ page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
+ nr_pages = PFN_DOWN(meminfo->bank[i].size);
+ ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
+ if ( ret )
+ goto fail;
}
}
+ else
+ {
+ page = mfn_to_page(
+ maddr_to_mfn(shm_membank->mem.bank->start));
+ nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
+ ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
+ if ( ret )
+ return ret;
+ }
return 0;
fail:
while ( --i >= 0 )
- put_page_nr(page + i, nr_borrowers);
+ {
+ page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
+ nr_pages = PFN_DOWN(meminfo->bank[i].size);
+ remove_shared_memory_ref(page, nr_pages, nr_borrowers);
+ }
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 07/13] xen/arm: map shared memory to borrower when host address not provided
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (5 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 06/13] xen/arm: assign shared memory to owner " Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 08/13] xen/arm: use paddr_assigned to indicate whether host address is provided Penny Zheng
` (5 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
With the introduction of new scenario where host address is not provided
in "xen,shared-mem", the function "map_regions_p2mt" for setting up
P2M foreign mapping for borrower domain shall be adapted to it too.
Here we implement a new helper "borrower_physmap_add_memory" to
cover both scenarios.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 54 ++++++++++++++++++++++++++++++++++---
1 file changed, 51 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index faf0784bb0..d0f7fc8fd7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1110,6 +1110,51 @@ static int __init assign_shared_memory(struct domain *d,
return ret;
}
+static int __init borrower_physmap_add_memory(struct domain *d,
+ struct shm_membank *shm_membank,
+ paddr_t gbase)
+{
+ int ret;
+ unsigned int i;
+ paddr_t start, size;
+ gfn_t sgfn;
+ struct meminfo *meminfo;
+
+ /* Host address is not provided in "xen,shared-mem" */
+ if ( shm_membank->mem.banks.meminfo )
+ {
+ meminfo = shm_membank->mem.banks.meminfo;
+ for ( i = 0; i < meminfo->nr_banks; i++ )
+ {
+ start = meminfo->bank[i].start;
+ size = meminfo->bank[i].size;
+ sgfn = _gfn(PFN_UP(gbase));
+
+ /* Set up P2M foreign mapping for borrower domain. */
+ ret = map_regions_p2mt(d, sgfn, PFN_DOWN(size), _mfn(PFN_UP(start)),
+ p2m_map_foreign_rw);
+ if ( ret )
+ return ret;
+
+ sgfn = gfn_add(sgfn, PFN_DOWN(size));
+ }
+ }
+ else
+ {
+ start = shm_membank->mem.bank->start;
+ size = shm_membank->mem.bank->size;
+ sgfn = _gfn(PFN_UP(gbase));
+
+ /* Set up P2M foreign mapping for borrower domain. */
+ ret = map_regions_p2mt(d, sgfn, PFN_DOWN(size), _mfn(PFN_UP(start)),
+ p2m_map_foreign_rw);
+ if ( ret )
+ return ret;
+ }
+
+ return 0;
+}
+
static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
paddr_t start, paddr_t size,
const char *shm_id)
@@ -1242,11 +1287,14 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
{
- /* Set up P2M foreign mapping for borrower domain. */
- ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
- _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
+ ret = borrower_physmap_add_memory(d, shm_membank, gbase);
if ( ret )
+ {
+ printk(XENLOG_ERR
+ "%pd: Failed to map foreign memory to borrower domain: %d",
+ d, ret);
return ret;
+ }
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 08/13] xen/arm: use paddr_assigned to indicate whether host address is provided
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (6 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 07/13] xen/arm: map shared memory to borrower " Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 09/13] xen/arm: refine docs about static shared memory Penny Zheng
` (4 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
We use paddr_assigned to indicate whether host address is provided, by
checking the length of "xen,shared-mem" property.
And the shm matching criteria shall also be adapt to the new scenario, by
adding when host address is not provided, if SHMID matches, the region size
must exactly match too.
Also, to make codes tidy and clear, we extract codes about parsing
"xen,shared-mem" property from function "process_shm" and move them into
a new helper "parse_shm_property".
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/bootfdt.c | 87 +++++++++++++++++++--------
xen/arch/arm/domain_build.c | 114 +++++++++++++++++++++++++-----------
2 files changed, 141 insertions(+), 60 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 2f34a8ea83..efaf49fd56 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -387,7 +387,7 @@ static int __init process_shm_node(const void *fdt, int node,
struct shm_meminfo *shm_mem = &bootinfo.shm_mem;
unsigned int i;
int len;
- bool owner = false;
+ bool owner = false, paddr_assigned = true;
const char *shm_id;
if ( address_cells < 1 || size_cells < 1 )
@@ -428,7 +428,7 @@ static int __init process_shm_node(const void *fdt, int node,
}
/*
- * xen,shared-mem = <paddr, gaddr, size>;
+ * xen,shared-mem = <paddr, gaddr, size>, and paddr could be optional
* Memory region starting from physical address #paddr of #size shall
* be mapped to guest physical address #gaddr as static shared memory
* region.
@@ -439,16 +439,24 @@ static int __init process_shm_node(const void *fdt, int node,
if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
{
+ /* paddr is not provided in "xen,shared-mem" */
if ( len == dt_cells_to_size(size_cells + address_cells) )
- printk("fdt: host physical address must be chosen by users at the moment.\n");
-
- printk("fdt: invalid `xen,shared-mem` property.\n");
- return -EINVAL;
+ paddr_assigned = false;
+ else
+ {
+ printk("fdt: invalid `xen,shared-mem` property.\n");
+ return -EINVAL;
+ }
}
cell = (const __be32 *)prop->data;
- device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
- size = dt_next_cell(size_cells, &cell);
+ if ( !paddr_assigned )
+ device_tree_get_reg(&cell, address_cells, size_cells, &gaddr, &size);
+ else
+ {
+ device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
+ size = dt_next_cell(size_cells, &cell);
+ }
if ( !size )
{
@@ -458,29 +466,45 @@ static int __init process_shm_node(const void *fdt, int node,
for ( i = 0; i < shm_mem->nr_banks; i++ )
{
- paddr_t bank_start = shm_mem->bank[i].mem.bank->start;
- paddr_t bank_size = shm_mem->bank[i].mem.bank->size;
-
/*
* Meet the following check:
+ * when host address is provided:
* 1) The shm ID matches and the region exactly match
* 2) The shm ID doesn't match and the region doesn't overlap
* with an existing one
+ * when host address is not provided:
+ * 1) The shm ID matches and the region size exactly match
*/
- if ( paddr == bank_start && size == bank_size )
+ if ( !paddr_assigned || (paddr == shm_mem->bank[i].mem.bank->start &&
+ size == shm_mem->bank[i].mem.bank->size) )
{
if ( strncmp(shm_id,
shm_mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
+ {
+ if ( !paddr_assigned &&
+ (size != shm_mem->bank[i].mem.banks.total_size) )
+ {
+ printk("fdt: when host address is not provided, if xen,shm-id %s matches, size must stay the same too.\n",
+ shm_id);
+ return -EINVAL;
+ }
+
break;
- else
+ }
+ else if ( paddr_assigned )
{
printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
shm_id);
return -EINVAL;
}
+
+ /* host address is not provided, and with different SHMID. */
+ continue;
}
else
{
+ paddr_t bank_start = shm_mem->bank[i].mem.bank->start;
+ paddr_t bank_size = shm_mem->bank[i].mem.bank->size;
paddr_t end = paddr + size;
paddr_t bank_end = bank_start + bank_size;
@@ -512,26 +536,35 @@ static int __init process_shm_node(const void *fdt, int node,
if ( i == shm_mem->nr_banks )
{
- if ( (i < NR_MEM_BANKS) && (mem->nr_banks < NR_MEM_BANKS) )
+ if ( i < NR_MEM_BANKS )
{
- /* Static shared memory shall be reserved from any other use. */
- struct membank *membank = &mem->bank[mem->nr_banks];
+ if ( !paddr_assigned )
+ {
+ shm_mem->bank[i].mem.banks.total_size = size;
+ goto out;
+ }
+ else if ( mem->nr_banks < NR_MEM_BANKS )
+ {
+ /* Static shared memory shall be reserved from any other use. */
+ struct membank *membank = &mem->bank[mem->nr_banks];
- membank->start = paddr;
- membank->size = size;
- membank->type = MEMBANK_STATIC_DOMAIN;
- mem->nr_banks++;
+ membank->start = paddr;
+ membank->size = size;
+ membank->type = MEMBANK_STATIC_DOMAIN;
+ mem->nr_banks++;
+ shm_mem->bank[i].mem.bank = membank;
+ }
+ else
+ goto fail;
+ out:
safe_strcpy(shm_mem->bank[i].shm_id, shm_id);
- shm_mem->bank[i].mem.bank = membank;
shm_mem->nr_banks++;
}
else
- {
- printk("Warning: Max number of supported memory regions reached.\n");
- return -ENOSPC;
- }
+ goto fail;
}
+
/*
* keep a count of the number of borrowers, which later may be used
* to calculate the reference count.
@@ -540,6 +573,10 @@ static int __init process_shm_node(const void *fdt, int node,
shm_mem->bank[i].nr_shm_borrowers++;
return 0;
+
+ fail:
+ printk("Warning: Max number of supported memory regions reached.\n");
+ return -ENOSPC;
}
#else
static int __init process_shm_node(const void *fdt, int node,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d0f7fc8fd7..80d37245a8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1177,6 +1177,81 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
return 0;
}
+static int __init parse_shm_property(struct domain *d,
+ const struct dt_device_node *shm_node,
+ bool *paddr_assigned, paddr_t *pbase,
+ paddr_t *gbase, paddr_t *psize)
+{
+ const struct dt_property *prop;
+ const __be32 *cells;
+ uint32_t len;
+ unsigned int i;
+ uint32_t addr_cells, size_cells;
+
+ addr_cells = dt_n_addr_cells(shm_node);
+ size_cells = dt_n_size_cells(shm_node);
+
+ /* xen,shared-mem = <pbase, gbase, size>; And pbase could be optional. */
+ prop = dt_find_property(shm_node, "xen,shared-mem", &len);
+ BUG_ON(!prop);
+ cells = (const __be32 *)prop->value;
+
+ if ( len != dt_cells_to_size(addr_cells + size_cells + addr_cells) )
+ {
+ /* pbase is not provided in "xen,shared-mem" */
+ if ( len == dt_cells_to_size(size_cells + addr_cells) )
+ *paddr_assigned = false;
+ else
+ {
+ printk("fdt: invalid `xen,shared-mem` property.\n");
+ return -EINVAL;
+ }
+ }
+
+ if ( !*paddr_assigned )
+ {
+ device_tree_get_reg(&cells, addr_cells, size_cells, gbase, psize);
+ goto out;
+ }
+ else
+ {
+ device_tree_get_reg(&cells, addr_cells, addr_cells, pbase, gbase);
+ *psize = dt_read_number(cells, size_cells);
+ }
+
+ if ( !IS_ALIGNED(*pbase, PAGE_SIZE) )
+ {
+ printk("%pd: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
+ d, *pbase);
+ return -EINVAL;
+ }
+
+ for ( i = 0; i < PFN_DOWN(*psize); i++ )
+ if ( !mfn_valid(mfn_add(maddr_to_mfn(*pbase), i)) )
+ {
+ printk("%pd: invalid physical address 0x%"PRI_mfn"\n",
+ d, mfn_x(mfn_add(maddr_to_mfn(*pbase), i)));
+ return -EINVAL;
+ }
+
+ out:
+ if ( !IS_ALIGNED(*psize, PAGE_SIZE) )
+ {
+ printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n",
+ d, *psize);
+ return -EINVAL;
+ }
+
+ if ( !IS_ALIGNED(*gbase, PAGE_SIZE) )
+ {
+ printk("%pd: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
+ d, *gbase);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
@@ -1184,12 +1259,8 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
dt_for_each_child_node(node, shm_node)
{
- const struct dt_property *prop;
- const __be32 *cells;
- uint32_t addr_cells, size_cells;
paddr_t gbase, pbase, psize;
int ret = 0;
- unsigned int i;
const char *role_str;
const char *shm_id;
bool owner_dom_io = true, paddr_assigned = true;
@@ -1198,37 +1269,10 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
- /*
- * xen,shared-mem = <pbase, gbase, size>;
- * TODO: pbase is optional.
- */
- addr_cells = dt_n_addr_cells(shm_node);
- size_cells = dt_n_size_cells(shm_node);
- prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
- BUG_ON(!prop);
- cells = (const __be32 *)prop->value;
- device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
- psize = dt_read_number(cells, size_cells);
- if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
- {
- printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n",
- d, pbase, gbase);
- return -EINVAL;
- }
- if ( !IS_ALIGNED(psize, PAGE_SIZE) )
- {
- printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n",
- d, psize);
- return -EINVAL;
- }
-
- for ( i = 0; i < PFN_DOWN(psize); i++ )
- if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
- {
- printk("%pd: invalid physical address 0x%"PRI_mfn"\n",
- d, mfn_x(mfn_add(maddr_to_mfn(pbase), i)));
- return -EINVAL;
- }
+ ret = parse_shm_property(d, shm_node, &paddr_assigned,
+ &pbase, &gbase, &psize);
+ if ( ret )
+ return ret;
/*
* "role" property is optional and if it is defined explicitly,
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 09/13] xen/arm: refine docs about static shared memory
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (7 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 08/13] xen/arm: use paddr_assigned to indicate whether host address is provided Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 10/13] xen/arm: introduce "xen,offset" feature Penny Zheng
` (3 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
This commit amends docs(booting.txt) to include the new scenario where host
address is not provided in "xen,shared-mem" property, and we also add a new
example to explain in details.
We also fix some buggy info in the docs, like SHMID is "my-shared-mem-1",
not "0x1".
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
docs/misc/arm/device-tree/booting.txt | 52 ++++++++++++++++++++-------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 87eaa3e254..287898ef03 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -553,7 +553,7 @@ communication.
An array takes a physical address, which is the base address of the
shared memory region in host physical address space, a size, and a guest
physical address, as the target address of the mapping.
- e.g. xen,shared-mem = < [host physical address] [guest address] [size] >
+ e.g. xen,shared-mem = < [host physical address] [guest address] [size] >;
It shall also meet the following criteria:
1) If the SHM ID matches with an existing region, the address range of the
@@ -564,8 +564,8 @@ communication.
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.
- Host physical address is optional, when missing Xen decides the location
- (currently unimplemented).
+ Host physical address is optional, when missing Xen decides the location.
+ e.g. xen,shared-mem = < [guest address] [size] >
- role (Optional)
@@ -592,7 +592,7 @@ chosen {
role = "owner";
xen,shm-id = "my-shared-mem-0";
xen,shared-mem = <0x10000000 0x10000000 0x10000000>;
- }
+ };
domU1 {
compatible = "xen,domain";
@@ -603,25 +603,36 @@ chosen {
vpl011;
/*
- * shared memory region identified as 0x0(xen,shm-id = <0x0>)
- * is shared between Dom0 and DomU1.
+ * shared memory region "my-shared-mem-0" is shared
+ * between Dom0 and DomU1.
*/
domU1-shared-mem@10000000 {
compatible = "xen,domain-shared-memory-v1";
role = "borrower";
xen,shm-id = "my-shared-mem-0";
xen,shared-mem = <0x10000000 0x50000000 0x10000000>;
- }
+ };
/*
- * shared memory region identified as 0x1(xen,shm-id = <0x1>)
- * is shared between DomU1 and DomU2.
+ * shared memory region "my-shared-mem-1" is shared between
+ * DomU1 and DomU2.
*/
domU1-shared-mem@50000000 {
compatible = "xen,domain-shared-memory-v1";
xen,shm-id = "my-shared-mem-1";
xen,shared-mem = <0x50000000 0x60000000 0x20000000>;
- }
+ };
+
+ /*
+ * shared memory region "my-shared-mem-2" is shared between
+ * DomU1 and DomU2.
+ */
+ domU1-shared-mem-2 {
+ compatible = "xen,domain-shared-memory-v1";
+ xen,shm-id = "my-shared-mem-2";
+ role = "owner";
+ xen,shared-mem = <0x80000000 0x20000000>;
+ };
......
@@ -635,14 +646,21 @@ chosen {
cpus = <1>;
/*
- * shared memory region identified as 0x1(xen,shm-id = <0x1>)
- * is shared between domU1 and domU2.
+ * shared memory region "my-shared-mem-1" is shared between
+ * domU1 and domU2.
*/
domU2-shared-mem@50000000 {
compatible = "xen,domain-shared-memory-v1";
xen,shm-id = "my-shared-mem-1";
xen,shared-mem = <0x50000000 0x70000000 0x20000000>;
- }
+ };
+
+ domU2-shared-mem-2 {
+ compatible = "xen,domain-shared-memory-v1";
+ xen,shm-id = "my-shared-mem-2";
+ role = "borrower";
+ xen,shared-mem = <0x90000000 0x20000000>;
+ };
......
};
@@ -662,3 +680,11 @@ shared between DomU1 and DomU2. It will get mapped at 0x60000000 in DomU1 guest
physical address space, and at 0x70000000 in DomU2 guest physical address space.
DomU1 and DomU2 are both the borrower domain, the owner domain is the default
owner domain DOMID_IO.
+
+For the static shared memory region "my-shared-mem-2", since host physical
+address is not provided by user, Xen will automatically allocate 512MB
+from heap as static shared memory to be shared between DomU1 and DomU2.
+The automatically allocated static shared memory will get mapped at
+0x80000000 in DomU1 guest physical address space, and at 0x90000000 in DomU2
+guest physical address space. DomU1 is explicitly defined as the owner domain,
+and DomU2 is the borrower domain.
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 10/13] xen/arm: introduce "xen,offset" feature
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (8 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 09/13] xen/arm: refine docs about static shared memory Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 11/13] xen/arm: implement "xen,offset" feature when host address provided Penny Zheng
` (2 subsequent siblings)
12 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
We introduce "xen,offset" to handle the case where memory from the owner is
shared with multiple borrowers. Each borrower would have its own offset within
the region shared by the owner.
Add relative check when parsing static shared memory node, to make sure
that "xen,offset" could be only provided in borrower domain, and the value
must be smaller than size. And include according docs in booting.txt.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
docs/misc/arm/device-tree/booting.txt | 19 ++++++++++++++--
xen/arch/arm/bootfdt.c | 32 ++++++++++++++++++++++++++-
2 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 287898ef03..183c41e3c2 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -577,6 +577,16 @@ communication.
If not specified, the default value is "borrower" and owner is
DOMID_IO, a system domain.
+- xen,offset (Optional)
+
+ A 64-bit integer specifying the offset within a shared memory region.
+ When "xen,offset" is provided, only partial shared memory will be
+ mapped to the borrower domain, starting at the offset and the size
+ being "size - offset".
+
+ "xen,offset" could be only provided in borrower domain, and the value
+ must be smaller than size.
+
As an example:
chosen {
@@ -660,6 +670,7 @@ chosen {
xen,shm-id = "my-shared-mem-2";
role = "borrower";
xen,shared-mem = <0x90000000 0x20000000>;
+ xen,offset = <0x0 0x10000000>;
};
......
@@ -686,5 +697,9 @@ address is not provided by user, Xen will automatically allocate 512MB
from heap as static shared memory to be shared between DomU1 and DomU2.
The automatically allocated static shared memory will get mapped at
0x80000000 in DomU1 guest physical address space, and at 0x90000000 in DomU2
-guest physical address space. DomU1 is explicitly defined as the owner domain,
-and DomU2 is the borrower domain.
+guest physical address space.
+For borrower domain DomU2, only partial static shared memory region
+"my-shared-mem-2" gets mapped, starting at offset 0x10000000 with size of
+256MB.
+DomU1 is explicitly defined as the owner domain, and DomU2 is the borrower
+domain.
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index efaf49fd56..e642e72f30 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -380,7 +380,7 @@ static int __init process_domain_node(const void *fdt, int node,
static int __init process_shm_node(const void *fdt, int node,
uint32_t address_cells, uint32_t size_cells)
{
- const struct fdt_property *prop, *prop_id, *prop_role;
+ const struct fdt_property *prop, *prop_id, *prop_role, *prop_offset;
const __be32 *cell;
paddr_t paddr, gaddr, size;
struct meminfo *mem = &bootinfo.reserved_mem;
@@ -389,6 +389,7 @@ static int __init process_shm_node(const void *fdt, int node,
int len;
bool owner = false, paddr_assigned = true;
const char *shm_id;
+ uint64_t offset;
if ( address_cells < 1 || size_cells < 1 )
{
@@ -464,6 +465,35 @@ static int __init process_shm_node(const void *fdt, int node,
return -EINVAL;
}
+ /*
+ * If "xen,offset" is provided, then only partial shared memory
+ * shall be mapped to borrower domain. The size will be
+ * "psize - offset".
+ * "xen,offset" is a 64-bit integer and an optional property
+ */
+ prop_offset = fdt_get_property(fdt, node, "xen,offset", NULL);
+ if ( prop_offset )
+ {
+ /*
+ * "xen,offset" could be only provided in borrower domain,
+ * and the offset must be smaller than size.
+ */
+ if ( prop_role && !strcmp(prop_role->data, "owner") )
+ {
+ printk("fdt: \"xen,offset\" could not be provided in owner domain\n");
+ return -EINVAL;
+ }
+
+ cell = (const __be32 *)prop_offset->data;
+ offset = dt_next_cell(2, &cell);
+
+ if ( offset >= size )
+ {
+ printk("fdt: invalid \"xen,offset\" value\n");
+ return -EINVAL;
+ }
+ }
+
for ( i = 0; i < shm_mem->nr_banks; i++ )
{
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 11/13] xen/arm: implement "xen,offset" feature when host address provided
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (9 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 10/13] xen/arm: introduce "xen,offset" feature Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 12/13] xen/arm: implement "xen,offset" feature when host address not provided Penny Zheng
2022-11-15 2:52 ` [PATCH v1 13/13] xen: make static shared memory supported in SUPPORT.md Penny Zheng
12 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
When host address is provided in "xen,shared-mem" property([pbase,
gbase, psize]), it is easy to decide where to map to the borrower
domain if "xen,offset" is also provided.
The partially-shared region shall be starting at pbase + offset, and
ending at pbase + size.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 80d37245a8..95600c640c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1112,7 +1112,7 @@ static int __init assign_shared_memory(struct domain *d,
static int __init borrower_physmap_add_memory(struct domain *d,
struct shm_membank *shm_membank,
- paddr_t gbase)
+ paddr_t gbase, paddr_t offset)
{
int ret;
unsigned int i;
@@ -1141,8 +1141,8 @@ static int __init borrower_physmap_add_memory(struct domain *d,
}
else
{
- start = shm_membank->mem.bank->start;
- size = shm_membank->mem.bank->size;
+ start = shm_membank->mem.bank->start + offset;
+ size = shm_membank->mem.bank->size - offset;
sgfn = _gfn(PFN_UP(gbase));
/* Set up P2M foreign mapping for borrower domain. */
@@ -1157,7 +1157,7 @@ static int __init borrower_physmap_add_memory(struct domain *d,
static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
paddr_t start, paddr_t size,
- const char *shm_id)
+ const char *shm_id, paddr_t offset)
{
struct membank *membank;
@@ -1170,7 +1170,7 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank = membank;
kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank->start = start;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank->size = size;
+ kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].mem.bank->size = size - offset;
safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
kinfo->shm_mem.nr_banks++;
@@ -1265,6 +1265,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
const char *shm_id;
bool owner_dom_io = true, paddr_assigned = true;
struct shm_membank *shm_membank;
+ uint64_t offset = 0UL;
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
@@ -1288,6 +1289,19 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
}
BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
+ /*
+ * If "xen,offset" is provided, then only partial shared memory
+ * shall be mapped to borrower domain.
+ * "xen,offset" is a 64-bit integer and an optional property
+ */
+ dt_property_read_u64(shm_node, "xen,offset", &offset);
+ if ( !IS_ALIGNED(offset, PAGE_SIZE) )
+ {
+ printk("%pd: \"xen,offset\" 0x%lx is not suitably aligned\n",
+ d, offset);
+ return -EINVAL;
+ }
+
shm_membank = acquire_shm_membank(shm_id);
if ( !shm_membank )
{
@@ -1331,7 +1345,8 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
{
- ret = borrower_physmap_add_memory(d, shm_membank, gbase);
+ ret = borrower_physmap_add_memory(d, shm_membank, gbase,
+ (paddr_t)offset);
if ( ret )
{
printk(XENLOG_ERR
@@ -1345,7 +1360,8 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
* Record static shared memory region info for later setting
* up shm-node in guest device tree.
*/
- ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id);
+ ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id,
+ (paddr_t)offset);
if ( ret )
return ret;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 12/13] xen/arm: implement "xen,offset" feature when host address not provided
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (10 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 11/13] xen/arm: implement "xen,offset" feature when host address provided Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 13/13] xen: make static shared memory supported in SUPPORT.md Penny Zheng
12 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk, Penny Zheng
When host address is not provided in "xen,shared-mem" property, shared
memory region is allocated from heap by Xen. It is normally not
contiguous and consisted of multiple memory blocks.
Under above scenario, when "xen,offset" is also offered, we need to find at
which memory block the offset locates, and the borrower memory map shall start
at this block offset.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
xen/arch/arm/domain_build.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 95600c640c..494f6aff2e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -48,6 +48,10 @@ boolean_param("ext_regions", opt_ext_regions);
static u64 __initdata dom0_mem;
static bool __initdata dom0_mem_set;
+#ifdef CONFIG_STATIC_SHM
+static uint64_t __initdata allocated = 0UL;
+#endif
+
static int __init parse_dom0_mem(const char *s)
{
dom0_mem_set = true;
@@ -1110,6 +1114,26 @@ static int __init assign_shared_memory(struct domain *d,
return ret;
}
+static bool __init find_anchor_offset(paddr_t *bank_start, paddr_t *bank_size,
+ uint64_t offset)
+{
+ uint64_t bank_offset;
+
+ allocated += *bank_size;
+ if ( allocated <= offset )
+ return false;
+
+ /* Find the bank when offset locates */
+ bank_offset = *bank_size - (allocated - offset);
+ *bank_start += bank_offset;
+ *bank_size = allocated - offset;
+
+ /* Reset after finding the anchor */
+ allocated = 0UL;
+
+ return true;
+}
+
static int __init borrower_physmap_add_memory(struct domain *d,
struct shm_membank *shm_membank,
paddr_t gbase, paddr_t offset)
@@ -1123,12 +1147,17 @@ static int __init borrower_physmap_add_memory(struct domain *d,
/* Host address is not provided in "xen,shared-mem" */
if ( shm_membank->mem.banks.meminfo )
{
+ bool found = false;
meminfo = shm_membank->mem.banks.meminfo;
+ sgfn = _gfn(PFN_UP(gbase));
+
for ( i = 0; i < meminfo->nr_banks; i++ )
{
start = meminfo->bank[i].start;
size = meminfo->bank[i].size;
- sgfn = _gfn(PFN_UP(gbase));
+
+ if ( offset && !found )
+ found = find_anchor_offset(&start, &size, offset);
/* Set up P2M foreign mapping for borrower domain. */
ret = map_regions_p2mt(d, sgfn, PFN_DOWN(size), _mfn(PFN_UP(start)),
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* [PATCH v1 13/13] xen: make static shared memory supported in SUPPORT.md
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
` (11 preceding siblings ...)
2022-11-15 2:52 ` [PATCH v1 12/13] xen/arm: implement "xen,offset" feature when host address not provided Penny Zheng
@ 2022-11-15 2:52 ` Penny Zheng
2023-01-08 13:18 ` Julien Grall
12 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2022-11-15 2:52 UTC (permalink / raw)
To: xen-devel
Cc: wei.chen, Penny Zheng, Andrew Cooper, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu, Bertrand Marquis,
Volodymyr Babchuk, Penny Zheng
After patching previous commits, we could make feature of "static shared memory"
supported in SUPPORT.md.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
SUPPORT.md | 2 +-
xen/arch/arm/Kconfig | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/SUPPORT.md b/SUPPORT.md
index ab71464cf6..c9fe7daf56 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -316,7 +316,7 @@ Allow sharing of identical pages between guests
Allow to statically set up shared memory on dom0less system,
enabling domains to do shm-based communication
- Status, ARM: Tech Preview
+ Status, ARM: Supported
### Memory Paging
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 52a05f704d..e0ea6e1762 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -141,7 +141,7 @@ config TEE
source "arch/arm/tee/Kconfig"
config STATIC_SHM
- bool "Statically shared memory on a dom0less system" if UNSUPPORTED
+ bool "Statically shared memory on a dom0less system"
depends on STATIC_MEMORY
help
This option enables statically shared memory on a dom0less system.
--
2.25.1
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter
2022-11-15 2:52 ` [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter Penny Zheng
@ 2022-11-15 6:58 ` Jeungwoo Yoo
2023-01-08 11:56 ` Julien Grall
1 sibling, 0 replies; 38+ messages in thread
From: Jeungwoo Yoo @ 2022-11-15 6:58 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hello,
Reading this patch, I found one place that can be improved.
On 11/15/22 03:52, Penny Zheng wrote:
> Instead of using multiple function parameters to deliver various shm-related
> info, like host physical address, SHMID, etc, and with the introduction
> of new struct "shm_membank", we could switch to use "shm_membank" as
> function parameter to replace them all, to make codes more clear and
> tidy.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c0fd13f6ed..d2b9e60b5c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain *d,
> }
>
> #ifdef CONFIG_STATIC_SHM
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> - unsigned long *nr_borrowers)
> +static struct shm_membank * __init acquire_shm_membank(const char *shm_id)
> {
> unsigned int bank;
>
> /* Iterate static shared memory to find requested shm bank. */
> for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
> - {
> - paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
> - paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
> -
> - if ( (pbase == bank_start) && (psize == bank_size) )
> + if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )
The target shared memory is found and the bank can be returned
directly here (return &bootinfo.shm_mem.bank[bank];).
Therefore, the out-of-bounds condition check can be removed below.
> break;
> - }
>
> if ( bank == bootinfo.shm_mem.nr_banks )
This can be removed, but only return NULL because the target memory is
not found.
> - return -ENOENT;
> -
> - *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
> + return NULL;
>
> - return 0;
> + return &bootinfo.shm_mem.bank[bank];
> }
>
> /*
> * This function checks whether the static shared memory region is
> * already allocated to dom_io.
> */
> -static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> +static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
> {
> struct page_info *page;
> struct domain *d;
>
> - page = maddr_to_page(pbase);
> + page = maddr_to_page(shm_membank->membank->start);
> d = page_get_owner_and_reference(page);
> if ( d == NULL )
> return false;
> @@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> }
>
> static int __init assign_shared_memory(struct domain *d,
> - uint32_t addr_cells, uint32_t size_cells,
> - paddr_t pbase, paddr_t psize,
> + struct shm_membank *shm_membank,
> paddr_t gbase)
> {
> mfn_t smfn;
> int ret = 0;
> unsigned long nr_pages, nr_borrowers, i;
> struct page_info *page;
> + paddr_t pbase, psize;
> +
> + pbase = shm_membank->membank->start;
> + psize = shm_membank->membank->size;
>
> printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> d, pbase, pbase + psize);
> @@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
> * Get the right amount of references per page, which is the number of
> * borrower domains.
> */
> - ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> - if ( ret )
> - return ret;
> + nr_borrowers = shm_membank->nr_shm_borrowers;
>
> /*
> * Instead of letting borrower domain get a page ref, we add as many
> @@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const char *role_str;
> const char *shm_id;
> bool owner_dom_io = true;
> + struct shm_membank *shm_membank;
>
> if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> continue;
> @@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> }
> BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
>
> + shm_membank = acquire_shm_membank(shm_id);
> + if ( !shm_membank )
> + {
> + printk("%pd: failed to acquire %s shared memory bank\n",
> + d, shm_id);
> + return -ENOENT;
> + }
> +
> /*
> * DOMID_IO is a fake domain and is not described in the Device-Tree.
> * Therefore when the owner of the shared region is DOMID_IO, we will
> * only find the borrowers.
> */
> - if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> + if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
> (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> {
> /*
> @@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> * specified, so they should be assigned to dom_io.
> */
> ret = assign_shared_memory(owner_dom_io ? dom_io : d,
> - addr_cells, size_cells,
> - pbase, psize, gbase);
> + shm_membank, gbase);
> if ( ret )
> return ret;
> }
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
2022-11-15 2:52 ` [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region Penny Zheng
@ 2023-01-08 11:44 ` Julien Grall
2023-01-09 7:48 ` Penny Zheng
2023-02-07 20:55 ` Stewart Hildebrand
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2023-01-08 11:44 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
> This commit re-arranges the static shared memory regions into a separate array
> shm_meminfo. And static shared memory region now would have its own structure
> 'shm_membank' to hold all shm-related members, like shm_id, etc, and a pointer
> to the normal memory bank 'membank'. This will avoid continuing to grow
> 'membank'.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
> xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
> xen/arch/arm/include/asm/kernel.h | 2 +-
> xen/arch/arm/include/asm/setup.h | 16 +++++++++----
> 4 files changed, 59 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 6014c0f852..ccf281cd37 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt, int node,
> const __be32 *cell;
> paddr_t paddr, gaddr, size;
> struct meminfo *mem = &bootinfo.reserved_mem;
After this patch, 'mem' is barely going to be used. So I would recommend
to remove it or restrict the scope.
This will make easier to confirm that most of the use of 'mem' have been
replaced with 'shm_mem' and reduce the risk of confusion between the two
(the name are quite similar).
[...]
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index bd30d3798c..c0fd13f6ed 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -757,20 +757,20 @@ static int __init acquire_nr_borrower_domain(struct domain *d,
> {
> unsigned int bank;
>
> - /* Iterate reserved memory to find requested shm bank. */
> - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> + /* Iterate static shared memory to find requested shm bank. */
> + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
> {
> - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
> + paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be
removed. But it looks like there was none. I guess that was a mistake in
the existing code?
>
> if ( (pbase == bank_start) && (psize == bank_size) )
> break;
> }
>
> - if ( bank == bootinfo.reserved_mem.nr_banks )
> + if ( bank == bootinfo.shm_mem.nr_banks )
> return -ENOENT;
>
> - *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
> + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
>
> return 0;
> }
> @@ -907,11 +907,18 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
> paddr_t start, paddr_t size,
> const char *shm_id)
> {
> + struct membank *membank;
> +
> if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
> return -ENOMEM;
>
> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
> + membank = xmalloc_bytes(sizeof(struct membank));
You allocate memory but never free it. However, I think it would be
better to avoid the dynamic allocation. So I would consider to not use
the structure shm_membank and instead create a specific one for the
domain construction.
> + if ( membank == NULL )
> + return -ENOMEM;
> +
> + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank = membank;
> + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start = start;
> + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size = size;
The last two could be replaced with:
membank->start = start;
membank->size = size;
This would make the code more readable. Also, while you are modifying
the code, I would consider to introduce a local variable that points to
kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].
[...]
> struct meminfo {
> @@ -61,6 +57,17 @@ struct meminfo {
> struct membank bank[NR_MEM_BANKS];
> };
>
> +struct shm_membank {
> + char shm_id[MAX_SHM_ID_LENGTH];
> + unsigned int nr_shm_borrowers;
> + struct membank *membank;
After the change I suggest above, I would expect that the field of
membank will not be updated. So I would add const here.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter
2022-11-15 2:52 ` [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter Penny Zheng
2022-11-15 6:58 ` Jeungwoo Yoo
@ 2023-01-08 11:56 ` Julien Grall
1 sibling, 0 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-08 11:56 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
> Instead of using multiple function parameters to deliver various shm-related
> info, like host physical address, SHMID, etc, and with the introduction
> of new struct "shm_membank", we could switch to use "shm_membank" as
> function parameter to replace them all, to make codes more clear and
> tidy.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 46 ++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c0fd13f6ed..d2b9e60b5c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -751,40 +751,31 @@ static void __init assign_static_memory_11(struct domain *d,
> }
>
> #ifdef CONFIG_STATIC_SHM
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> - unsigned long *nr_borrowers)
> +static struct shm_membank * __init acquire_shm_membank(const char *shm_id)
You are returning bootinfo. AFAICT, nobody you modify it after it was
populated. So can this be const?
Also, I think the function wants to be renamed because this is too
similar to the function acquire_shared_memory_bank().
I would rename it to find_shared_memory_bank().
> {
> unsigned int bank;
>
> /* Iterate static shared memory to find requested shm bank. */
> for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
> - {
> - paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank->start;
> - paddr_t bank_size = bootinfo.shm_mem.bank[bank].membank->size;
> -
> - if ( (pbase == bank_start) && (psize == bank_size) )
> + if ( strcmp(shm_id, bootinfo.shm_mem.bank[bank].shm_id) == 0 )
> break;
> - }
>
> if ( bank == bootinfo.shm_mem.nr_banks )
> - return -ENOENT;
> -
> - *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
> + return NULL;
>
> - return 0;
> + return &bootinfo.shm_mem.bank[bank];
> }
>
> /*
> * This function checks whether the static shared memory region is
> * already allocated to dom_io.
> */
> -static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> +static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
AFAICT, the function will not modify shm_membank. So please use const.
> {
> struct page_info *page;
> struct domain *d;
>
> - page = maddr_to_page(pbase);
> + page = maddr_to_page(shm_membank->membank->start);
> d = page_get_owner_and_reference(page);
> if ( d == NULL )
> return false;
> @@ -835,14 +826,17 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> }
>
> static int __init assign_shared_memory(struct domain *d,
> - uint32_t addr_cells, uint32_t size_cells,
> - paddr_t pbase, paddr_t psize,
> + struct shm_membank *shm_membank,
Same here.
> paddr_t gbase)
> {
> mfn_t smfn;
> int ret = 0;
> unsigned long nr_pages, nr_borrowers, i;
> struct page_info *page;
> + paddr_t pbase, psize;
> +
> + pbase = shm_membank->membank->start;
> + psize = shm_membank->membank->size;
>
> printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> d, pbase, pbase + psize);
> @@ -871,9 +865,7 @@ static int __init assign_shared_memory(struct domain *d,
> * Get the right amount of references per page, which is the number of
> * borrower domains.
> */
> - ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> - if ( ret )
> - return ret;
> + nr_borrowers = shm_membank->nr_shm_borrowers;
>
> /*
> * Instead of letting borrower domain get a page ref, we add as many
> @@ -941,6 +933,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const char *role_str;
> const char *shm_id;
> bool owner_dom_io = true;
> + struct shm_membank *shm_membank;
same here.
>
> if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> continue;
> @@ -991,12 +984,20 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> }
> BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
>
> + shm_membank = acquire_shm_membank(shm_id);
> + if ( !shm_membank )
> + {
> + printk("%pd: failed to acquire %s shared memory bank\n",
> + d, shm_id);
> + return -ENOENT;
> + }
> +
> /*
> * DOMID_IO is a fake domain and is not described in the Device-Tree.
> * Therefore when the owner of the shared region is DOMID_IO, we will
> * only find the borrowers.
> */
> - if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> + if ( (owner_dom_io && !is_shm_allocated_to_domio(shm_membank)) ||
> (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> {
> /*
> @@ -1004,8 +1005,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> * specified, so they should be assigned to dom_io.
> */
> ret = assign_shared_memory(owner_dom_io ? dom_io : d,
> - addr_cells, size_cells,
> - pbase, psize, gbase);
> + shm_membank, gbase);
> if ( ret )
> return ret;
> }
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address
2022-11-15 2:52 ` [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address Penny Zheng
@ 2023-01-08 12:13 ` Julien Grall
2023-01-08 12:54 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2023-01-08 12:13 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
> When host address is not provided in "xen,shared-mem", we let Xen
> automatically allocate requested static shared memory from heap, and it
> stands good chance of having multiple host memory banks allocated for the
> requested static shared memory as a result. Therefore current membank is not
> going to cover it.
>
> This commit introduces a new field "mem" to cover both scenarios.
> "struct membank" is used when host address is provided, whereas
> "struct meminfo" shall be used when host address not provided.
From this patch, it is not clear to me how a user can know which part
of the union should be used.
However... I am not entirely sure why you need to create a union because
in your new structure you can fit one bank.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided
2022-11-15 2:52 ` [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided Penny Zheng
@ 2023-01-08 12:22 ` Julien Grall
2023-01-09 7:50 ` Penny Zheng
2023-02-07 20:57 ` Stewart Hildebrand
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2023-01-08 12:22 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
> when host address is not provided in "xen,shared-mem", we let Xen
> allocate requested shared memory from heap, and once the shared memory is
> allocated, it will be marked as static(PGC_static), which means that it will be
> reserved as static memory, and will not go back to heap even on freeing.
Please don't move pages from the {xen,dom}heap to the static heap. If
you need to keep the pages for longer, then get an extra reference so
they will not be released.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 83 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index fbb196d8a4..3de96882a5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -835,6 +835,72 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
> return true;
> }
>
> +static int __init mark_shared_memory_static(struct shm_membank *shm_membank)
> +{
> + unsigned int bank;
> + unsigned long i, nr_mfns;
> + struct page_info *pg;
> + struct meminfo *meminfo;
> +
> + BUG_ON(!shm_membank->mem.banks.meminfo);
> + meminfo = shm_membank->mem.banks.meminfo;
> + for ( bank = 0; bank < meminfo->nr_banks; bank++ )
> + {
> + pg = mfn_to_page(maddr_to_mfn(meminfo->bank[bank].start));
mfn_to_page(maddr_to_mfn(...)) is equivalent to maddr_to_page(..).
> + nr_mfns = PFN_DOWN(meminfo->bank[bank].size);
> +
> + for ( i = 0; i < nr_mfns; i++ )
> + {
> + /* The page should be already allocated from heap. */
> + if ( !pg[i].count_info & PGC_state_inuse )
I don't think this is doing what you want because ! will take precedence
over &. You likely want to add parenthese:
!(... & ...)
> + {
> + printk(XENLOG_ERR
> + "pg[%lu] MFN %"PRI_mfn" c=%#lx\n",
> + i, mfn_x(page_to_mfn(pg)) + i, pg[i].count_info);
> + goto fail;
> + }
> +
> + pg[i].count_info |= PGC_static;
> + }
> + }
> +
> + return 0;
> +
> + fail:
> + while ( bank >= 0 )
> + {
> + while ( --i >= 0 )
> + pg[i].count_info &= ~PGC_static;
> + i = PFN_DOWN(meminfo->bank[--bank].size);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int __init allocate_shared_memory(struct shm_membank *shm_membank,
> + paddr_t psize)
> +{
> + struct meminfo *banks;
> + int ret;
> +
> + BUG_ON(shm_membank->mem.banks.meminfo != NULL);
> +
> + banks = xmalloc_bytes(sizeof(struct meminfo));
Where is this freed?
> + if ( banks == NULL )
> + return -ENOMEM;
> + shm_membank->mem.banks.meminfo = banks;
> + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct meminfo));
> +
> + if ( !allocate_domheap_memory(NULL, psize, shm_membank->mem.banks.meminfo) )
> + return -EINVAL;
> +
> + ret = mark_shared_memory_static(shm_membank);
> + if ( ret )
> + return ret;
> +
> + return 0;
> +}
> +
> static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> paddr_t pbase, paddr_t psize)
> {
> @@ -975,7 +1041,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> unsigned int i;
> const char *role_str;
> const char *shm_id;
> - bool owner_dom_io = true;
> + bool owner_dom_io = true, paddr_assigned = true;
> struct shm_membank *shm_membank;
>
> if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> @@ -1035,6 +1101,21 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> return -ENOENT;
> }
>
> + /*
> + * When host address is not provided in "xen,shared-mem",
> + * we let Xen allocate requested memory from heap at first domain.
> + */
> + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo )
> + {
> + ret = allocate_shared_memory(shm_membank, psize);
> + if ( ret )
> + {
> + printk("%pd: failed to allocate shared memory bank(%"PRIpaddr"MB) from heap: %d\n",
> + d, psize >> 20, ret);
> + return ret;
> + }
> + }
> +
> /*
> * DOMID_IO is a fake domain and is not described in the Device-Tree.
> * Therefore when the owner of the shared region is DOMID_IO, we will
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2022-11-15 2:52 ` [PATCH v1 06/13] xen/arm: assign shared memory to owner " Penny Zheng
@ 2023-01-08 12:52 ` Julien Grall
2023-01-09 7:49 ` Penny Zheng
2023-02-07 20:58 ` Stewart Hildebrand
1 sibling, 1 reply; 38+ messages in thread
From: Julien Grall @ 2023-01-08 12:52 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 15/11/2022 02:52, Penny Zheng wrote:
> @@ -922,33 +927,82 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> d->max_pages += nr_pfns;
>
> smfn = maddr_to_mfn(pbase);
> - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> - if ( res )
> + page = mfn_to_page(smfn);
> + /*
> + * If page is allocated from heap as static shared memory, then we just
> + * assign it to the owner domain
> + */
> + if ( page->count_info == (PGC_state_inuse | PGC_static) )
I am a bit confused how this can help differentiating
becaPGC_state_inuse is 0. So effectively, you are checking that
count_info is equal to PGC_static.
But as I wrote in a previous patch, I don't think you should convert
{xen,dom}heap pages to a static pages.
[...]
> +static int __init assign_shared_memory(struct domain *d,
> + struct shm_membank *shm_membank,
> + paddr_t gbase)
> +{
> + int ret = 0;
> + unsigned long nr_pages, nr_borrowers;
> + struct page_info *page;
> + unsigned int i;
> + struct meminfo *meminfo;
> +
> + /* Host address is not provided in "xen,shared-mem" */
> + if ( shm_membank->mem.banks.meminfo )
> + {
> + meminfo = shm_membank->mem.banks.meminfo;
> + for ( i = 0; i < meminfo->nr_banks; i++ )
> + {
> + ret = acquire_shared_memory(d,
> + meminfo->bank[i].start,
> + meminfo->bank[i].size,
> + gbase);
> + if ( ret )
> + return ret;
> +
> + gbase += meminfo->bank[i].size;
> + }
> + }
> + else
> + {
> + ret = acquire_shared_memory(d,
> + shm_membank->mem.bank->start,
> + shm_membank->mem.bank->size, gbase);
> + if ( ret )
> + return ret;
> + }
Looking at this change and...
> +
> /*
> * Get the right amount of references per page, which is the number of
> * borrower domains.
> @@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct domain *d,
> * So if the borrower is created first, it will cause adding pages
> * in the P2M without reference.
> */
> - page = mfn_to_page(smfn);
> - for ( i = 0; i < nr_pages; i++ )
> + if ( shm_membank->mem.banks.meminfo )
> {
> - if ( !get_page_nr(page + i, d, nr_borrowers) )
> + meminfo = shm_membank->mem.banks.meminfo;
> + for ( i = 0; i < meminfo->nr_banks; i++ )
> {
> - printk(XENLOG_ERR
> - "Failed to add %lu references to page %"PRI_mfn".\n",
> - nr_borrowers, mfn_x(smfn) + i);
> - goto fail;
> + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> + nr_pages = PFN_DOWN(meminfo->bank[i].size);
> + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> + if ( ret )
> + goto fail;
> }
> }
> + else
> + {
> + page = mfn_to_page(
> + maddr_to_mfn(shm_membank->mem.bank->start));
> + nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
> + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> + if ( ret )
> + return ret;
> + }
... this one. The code to deal with a bank is exactly the same. But you
need the duplication because you special case "one bank".
As I wrote in a previous patch, I don't think we should special case it.
If the concern is memory usage, then we should look at reworking meminfo
instead (or using a different structure).
>
> return 0;
>
> fail:
> while ( --i >= 0 )
> - put_page_nr(page + i, nr_borrowers);
> + {
> + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> + nr_pages = PFN_DOWN(meminfo->bank[i].size);
> + remove_shared_memory_ref(page, nr_pages, nr_borrowers);
> + }
> return ret;
> }
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address
2023-01-08 12:13 ` Julien Grall
@ 2023-01-08 12:54 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-08 12:54 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 08/01/2023 12:13, Julien Grall wrote:
> Hi Penny,
>
> On 15/11/2022 02:52, Penny Zheng wrote:
>> When host address is not provided in "xen,shared-mem", we let Xen
>> automatically allocate requested static shared memory from heap, and it
>> stands good chance of having multiple host memory banks allocated for the
>> requested static shared memory as a result. Therefore current membank
>> is not
>> going to cover it.
>>
>> This commit introduces a new field "mem" to cover both scenarios.
>> "struct membank" is used when host address is provided, whereas
>> "struct meminfo" shall be used when host address not provided.
>
> From this patch, it is not clear to me how a user can know which part
> of the union should be used.
Ah it is a struct rather than an union. Yet...
>
> However... I am not entirely sure why you need to create a union because
> in your new structure you can fit one bank.
... my point here stands.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 13/13] xen: make static shared memory supported in SUPPORT.md
2022-11-15 2:52 ` [PATCH v1 13/13] xen: make static shared memory supported in SUPPORT.md Penny Zheng
@ 2023-01-08 13:18 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-08 13:18 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini, Wei Liu, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 15/11/2022 02:52, Penny Zheng wrote:
> After patching previous commits, we could make feature of "static shared memory"
Are you referring to the patch in this series? If so, they seem to had
new features which I don't think are necessary to mark the "static
shared memory".
Instead, "static shared memory" could be marked as supported if we
believe that the new code has no security hole.
Looking below, the STATIC_SHM depends on STATIC_MEMORY which is
currently unsupported. So it seems a bit strange to mark one supported
but not the other.
Now, in order to support them, we need to make sure that the XENMEM_*
operations are working as intended. I know there are some works that
were done in the past, but I can't exactly remember if we fixed
everything. So what happen if the domain (consider the case where the
domain is directmapped or not):
1) Remove the page?
2) Remove the page twice? (Only in the directmap case)
3) Request to map the page?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
2023-01-08 11:44 ` Julien Grall
@ 2023-01-09 7:48 ` Penny Zheng
2023-01-09 10:01 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2023-01-09 7:48 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Sunday, January 8, 2023 7:44 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory
> region
>
> Hi Penny,
>
Hi Julien
> On 15/11/2022 02:52, Penny Zheng wrote:
> > This commit re-arranges the static shared memory regions into a
> > separate array shm_meminfo. And static shared memory region now
> would
> > have its own structure 'shm_membank' to hold all shm-related members,
> > like shm_id, etc, and a pointer to the normal memory bank 'membank'.
> > This will avoid continuing to grow 'membank'.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
> > xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
> > xen/arch/arm/include/asm/kernel.h | 2 +-
> > xen/arch/arm/include/asm/setup.h | 16 +++++++++----
> > 4 files changed, 59 insertions(+), 34 deletions(-)
> >
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> > 6014c0f852..ccf281cd37 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt,
> int node,
> > const __be32 *cell;
> > paddr_t paddr, gaddr, size;
> > struct meminfo *mem = &bootinfo.reserved_mem;
>
> After this patch, 'mem' is barely going to be used. So I would recommend to
> remove it or restrict the scope.
>
Hope I understand correctly, you are saying that all static shared memory bank will be
described as "struct shm_membank". That's right.
However when host address is provided, we still need an instance of "struct membank"
to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in
as static pages.
That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the
same object.
If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like
Init_staticmem_pages, dt_unreserved_regions, etc
> This will make easier to confirm that most of the use of 'mem' have been
> replaced with 'shm_mem' and reduce the risk of confusion between the two
> (the name are quite similar).
>
> [...]
>
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index bd30d3798c..c0fd13f6ed 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -757,20 +757,20 @@ static int __init
> acquire_nr_borrower_domain(struct domain *d,
> > {
> > unsigned int bank;
> >
> > - /* Iterate reserved memory to find requested shm bank. */
> > - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> > + /* Iterate static shared memory to find requested shm bank. */
> > + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
> > {
> > - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> > - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> > + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank-
> >start;
> > + paddr_t bank_size =
> > + bootinfo.shm_mem.bank[bank].membank->size;
>
> I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be
> removed. But it looks like there was none. I guess that was a mistake in the
> existing code?
>
Oh, you're right, the type shall also be checked.
> >
> > if ( (pbase == bank_start) && (psize == bank_size) )
> > break;
> > }
> >
> > - if ( bank == bootinfo.reserved_mem.nr_banks )
> > + if ( bank == bootinfo.shm_mem.nr_banks )
> > return -ENOENT;
> >
> > - *nr_borrowers =
> bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
> > + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
> >
> > return 0;
> > }
> > @@ -907,11 +907,18 @@ static int __init
> append_shm_bank_to_domain(struct kernel_info *kinfo,
> > paddr_t start, paddr_t size,
> > const char *shm_id)
> > {
> > + struct membank *membank;
> > +
> > if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
> > return -ENOMEM;
> >
> > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
> > - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
> > + membank = xmalloc_bytes(sizeof(struct membank));
>
> You allocate memory but never free it. However, I think it would be better to
> avoid the dynamic allocation. So I would consider to not use the structure
> shm_membank and instead create a specific one for the domain construction.
>
True, a local variable "struct meminfo shm_banks" could be introduced only
for domain construction in function construct_domU
> > + if ( membank == NULL )
> > + return -ENOMEM;
> > +
> > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank =
> membank;
> > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->start =
> start;
> > + kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].membank->size =
> > + size;
>
> The last two could be replaced with:
>
> membank->start = start;
> membank->size = size;
>
> This would make the code more readable. Also, while you are modifying the
> code, I would consider to introduce a local variable that points to
> kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].
>
> [...]
>
> > struct meminfo {
> > @@ -61,6 +57,17 @@ struct meminfo {
> > struct membank bank[NR_MEM_BANKS];
> > };
> >
> > +struct shm_membank {
> > + char shm_id[MAX_SHM_ID_LENGTH];
> > + unsigned int nr_shm_borrowers;
> > + struct membank *membank;
>
> After the change I suggest above, I would expect that the field of
> membank will not be updated. So I would add const here.
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-08 12:52 ` Julien Grall
@ 2023-01-09 7:49 ` Penny Zheng
2023-01-09 10:57 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2023-01-09 7:49 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
Happy new year~~~~
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Sunday, January 8, 2023 8:53 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> when host address not provided
>
> Hi,
>
> On 15/11/2022 02:52, Penny Zheng wrote:
> > @@ -922,33 +927,82 @@ static mfn_t __init
> acquire_shared_memory_bank(struct domain *d,
> > d->max_pages += nr_pfns;
> >
> > smfn = maddr_to_mfn(pbase);
> > - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> > - if ( res )
> > + page = mfn_to_page(smfn);
> > + /*
> > + * If page is allocated from heap as static shared memory, then we just
> > + * assign it to the owner domain
> > + */
> > + if ( page->count_info == (PGC_state_inuse | PGC_static) )
> I am a bit confused how this can help differentiating becaPGC_state_inuse is
> 0. So effectively, you are checking that count_info is equal to PGC_static.
>
When host address is provided, the host address range defined in
xen,static-mem will be stored as a "struct membank" with type
of "MEMBANK_STATIC_DOMAIN" in "bootinfo.reserved_mem"
Then it will be initialized as static memory through "init_staticmem_pages"
So here its page->count_info is PGC_state_free |PGC_static.
For pages allocated from heap, its page state is different, being PGC_state_inuse.
So actually, we are checking page state to tell the difference .
> But as I wrote in a previous patch, I don't think you should convert
> {xen,dom}heap pages to a static pages.
>
I agree that taking reference could also prevent giving these pages back to heap.
But may I ask what is your concern on converting {xen,dom}heap pages to a static pages?
> [...]
>
> > +static int __init assign_shared_memory(struct domain *d,
> > + struct shm_membank *shm_membank,
> > + paddr_t gbase) {
> > + int ret = 0;
> > + unsigned long nr_pages, nr_borrowers;
> > + struct page_info *page;
> > + unsigned int i;
> > + struct meminfo *meminfo;
> > +
> > + /* Host address is not provided in "xen,shared-mem" */
> > + if ( shm_membank->mem.banks.meminfo )
> > + {
> > + meminfo = shm_membank->mem.banks.meminfo;
> > + for ( i = 0; i < meminfo->nr_banks; i++ )
> > + {
> > + ret = acquire_shared_memory(d,
> > + meminfo->bank[i].start,
> > + meminfo->bank[i].size,
> > + gbase);
> > + if ( ret )
> > + return ret;
> > +
> > + gbase += meminfo->bank[i].size;
> > + }
> > + }
> > + else
> > + {
> > + ret = acquire_shared_memory(d,
> > + shm_membank->mem.bank->start,
> > + shm_membank->mem.bank->size, gbase);
> > + if ( ret )
> > + return ret;
> > + }
>
> Looking at this change and...
>
> > +
> > /*
> > * Get the right amount of references per page, which is the number of
> > * borrower domains.
> > @@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct
> domain *d,
> > * So if the borrower is created first, it will cause adding pages
> > * in the P2M without reference.
> > */
> > - page = mfn_to_page(smfn);
> > - for ( i = 0; i < nr_pages; i++ )
> > + if ( shm_membank->mem.banks.meminfo )
> > {
> > - if ( !get_page_nr(page + i, d, nr_borrowers) )
> > + meminfo = shm_membank->mem.banks.meminfo;
> > + for ( i = 0; i < meminfo->nr_banks; i++ )
> > {
> > - printk(XENLOG_ERR
> > - "Failed to add %lu references to page %"PRI_mfn".\n",
> > - nr_borrowers, mfn_x(smfn) + i);
> > - goto fail;
> > + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> > + nr_pages = PFN_DOWN(meminfo->bank[i].size);
> > + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> > + if ( ret )
> > + goto fail;
> > }
> > }
> > + else
> > + {
> > + page = mfn_to_page(
> > + maddr_to_mfn(shm_membank->mem.bank->start));
> > + nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
> > + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> > + if ( ret )
> > + return ret;
> > + }
>
> ... this one. The code to deal with a bank is exactly the same. But you need
> the duplication because you special case "one bank".
>
> As I wrote in a previous patch, I don't think we should special case it.
> If the concern is memory usage, then we should look at reworking meminfo
> instead (or using a different structure).
>
A few concerns explained why I didn't choose "struct meminfo" over two
pointers "struct membank*" and "struct meminfo*".
1) memory usage is the main reason.
If we use "struct meminfo" over the current "struct membank*" and "struct meminfo*",
"struct shm_meminfo" will become a array of 256 "struct shm_membank", with
"struct shm_membank" being also an 256-item array, that is 256 * 256, too big for
a structure and If I remembered clearly, it will lead to "more than PAGE_SIZE" compiling error.
FWIT, either reworking meminfo or using a different structure, are both leading to sizing down
the array, hmmm, I don't know which size is suitable. That's why I prefer pointer
and dynamic allocation.
2) If we use "struct meminfo*" over the current "struct membank*" and "struct meminfo*",
we will need a new flag to differentiate two scenarios(host address provided or not), As
the special case "struct membank*" is already helping us differentiate.
And since when host address is provided, the related "struct membank" also needs to be
reserved in "bootinfo.reserved_mem". That's why I used pointer " struct membank*" to
avoid memory waste.
For the duplication, I could extract the common codes to mitigate the impact.
> >
> > return 0;
>
>
> > fail:
> > while ( --i >= 0 )
> > - put_page_nr(page + i, nr_borrowers);
> > + {
> > + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> > + nr_pages = PFN_DOWN(meminfo->bank[i].size);
> > + remove_shared_memory_ref(page, nr_pages, nr_borrowers);
> > + }
> > return ret;
> > }
> >
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided
2023-01-08 12:22 ` Julien Grall
@ 2023-01-09 7:50 ` Penny Zheng
2023-01-09 10:07 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2023-01-09 7:50 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Sunday, January 8, 2023 8:23 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap
> when host address not provided
>
> Hi Penny,
>
Hi Julien,
> On 15/11/2022 02:52, Penny Zheng wrote:
> > when host address is not provided in "xen,shared-mem", we let Xen
> > allocate requested shared memory from heap, and once the shared
> memory
> > is allocated, it will be marked as static(PGC_static), which means
> > that it will be reserved as static memory, and will not go back to heap even
> on freeing.
>
> Please don't move pages from the {xen,dom}heap to the static heap. If you
> need to keep the pages for longer, then get an extra reference so they will
> not be released.
>
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > xen/arch/arm/domain_build.c | 83
> ++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 82 insertions(+), 1 deletion(-)
> >
> > +static int __init allocate_shared_memory(struct shm_membank
> *shm_membank,
> > + paddr_t psize) {
> > + struct meminfo *banks;
> > + int ret;
> > +
> > + BUG_ON(shm_membank->mem.banks.meminfo != NULL);
> > +
> > + banks = xmalloc_bytes(sizeof(struct meminfo));
>
> Where is this freed?
>
These kinds of info will be only used in boot-time, so maybe I shall
free them in init_done()?
Or just after process_shm() ?
> > + if ( banks == NULL )
> > + return -ENOMEM;
> > + shm_membank->mem.banks.meminfo = banks;
> > + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct
> > + meminfo));
> > +
> > + if ( !allocate_domheap_memory(NULL, psize, shm_membank-
> >mem.banks.meminfo) )
> > + return -EINVAL;
> > +
> > + ret = mark_shared_memory_static(shm_membank);
> > + if ( ret )
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> > paddr_t pbase, paddr_t psize)
> > {
> > @@ -975,7 +1041,7 @@ static int __init process_shm(struct domain *d,
> struct kernel_info *kinfo,
> > unsigned int i;
> > const char *role_str;
> > const char *shm_id;
> > - bool owner_dom_io = true;
> > + bool owner_dom_io = true, paddr_assigned = true;
> > struct shm_membank *shm_membank;
> >
> > if ( !dt_device_is_compatible(shm_node,
> > "xen,domain-shared-memory-v1") ) @@ -1035,6 +1101,21 @@ static int
> __init process_shm(struct domain *d, struct kernel_info *kinfo,
> > return -ENOENT;
> > }
> >
> > + /*
> > + * When host address is not provided in "xen,shared-mem",
> > + * we let Xen allocate requested memory from heap at first domain.
> > + */
> > + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo )
> > + {
> > + ret = allocate_shared_memory(shm_membank, psize);
> > + if ( ret )
> > + {
> > + printk("%pd: failed to allocate shared memory
> bank(%"PRIpaddr"MB) from heap: %d\n",
> > + d, psize >> 20, ret);
> > + return ret;
> > + }
> > + }
> > +
> > /*
> > * DOMID_IO is a fake domain and is not described in the Device-Tree.
> > * Therefore when the owner of the shared region is
> > DOMID_IO, we will
>
> Cheers,
>
> --
> Julien Grall
Cheers,
--
Penny Zheng
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
2023-01-09 7:48 ` Penny Zheng
@ 2023-01-09 10:01 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-09 10:01 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 09/01/2023 07:48, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Sunday, January 8, 2023 7:44 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory
>> region
>> On 15/11/2022 02:52, Penny Zheng wrote:
>>> This commit re-arranges the static shared memory regions into a
>>> separate array shm_meminfo. And static shared memory region now
>> would
>>> have its own structure 'shm_membank' to hold all shm-related members,
>>> like shm_id, etc, and a pointer to the normal memory bank 'membank'.
>>> This will avoid continuing to grow 'membank'.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> xen/arch/arm/bootfdt.c | 40 +++++++++++++++++++------------
>>> xen/arch/arm/domain_build.c | 35 ++++++++++++++++-----------
>>> xen/arch/arm/include/asm/kernel.h | 2 +-
>>> xen/arch/arm/include/asm/setup.h | 16 +++++++++----
>>> 4 files changed, 59 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
>>> 6014c0f852..ccf281cd37 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -384,6 +384,7 @@ static int __init process_shm_node(const void *fdt,
>> int node,
>>> const __be32 *cell;
>>> paddr_t paddr, gaddr, size;
>>> struct meminfo *mem = &bootinfo.reserved_mem;
>>
>> After this patch, 'mem' is barely going to be used. So I would recommend to
>> remove it or restrict the scope.
>>
>
> Hope I understand correctly, you are saying that all static shared memory bank will be
> described as "struct shm_membank". That's right.
> However when host address is provided, we still need an instance of "struct membank"
> to refer to in "bootinfo.reserved_mem". Only in this way, it could be initialized properly in
> as static pages.
> That's why I put a "struct membank*" pointer in "struct shm_membank" to refer to the
> same object.
I wasn't talking about the field in "struct shm_membank". Instead, I was
referring to the local variable:
struct meminfo *mem = &bootinfo.reserved_mem;
AFAICT, the only use after this patch is when you add a new bank in
shm_mem. So you could restrict the scope of the local variable.
> If I removed instance in bootinfo.reserved_mem, a few more path needs to be updated, like
> Init_staticmem_pages, dt_unreserved_regions, etc
>
>> This will make easier to confirm that most of the use of 'mem' have been
>> replaced with 'shm_mem' and reduce the risk of confusion between the two
>> (the name are quite similar).
>>
>> [...]
>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index bd30d3798c..c0fd13f6ed 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -757,20 +757,20 @@ static int __init
>> acquire_nr_borrower_domain(struct domain *d,
>>> {
>>> unsigned int bank;
>>>
>>> - /* Iterate reserved memory to find requested shm bank. */
>>> - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
>>> + /* Iterate static shared memory to find requested shm bank. */
>>> + for ( bank = 0 ; bank < bootinfo.shm_mem.nr_banks; bank++ )
>>> {
>>> - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
>>> - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
>>> + paddr_t bank_start = bootinfo.shm_mem.bank[bank].membank-
>>> start;
>>> + paddr_t bank_size =
>>> + bootinfo.shm_mem.bank[bank].membank->size;
>>
>> I was expecting a "if (type == MEMBANK_STATIC_DOMAIN) ..." to be
>> removed. But it looks like there was none. I guess that was a mistake in the
>> existing code?
>>
>
> Oh, you're right, the type shall also be checked.
Just to clarify, with this patch you don't need to check the type. I was
pointing out a latent error in the existing code.
>
>>>
>>> if ( (pbase == bank_start) && (psize == bank_size) )
>>> break;
>>> }
>>>
>>> - if ( bank == bootinfo.reserved_mem.nr_banks )
>>> + if ( bank == bootinfo.shm_mem.nr_banks )
>>> return -ENOENT;
>>>
>>> - *nr_borrowers =
>> bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
>>> + *nr_borrowers = bootinfo.shm_mem.bank[bank].nr_shm_borrowers;
>>>
>>> return 0;
>>> }
>>> @@ -907,11 +907,18 @@ static int __init
>> append_shm_bank_to_domain(struct kernel_info *kinfo,
>>> paddr_t start, paddr_t size,
>>> const char *shm_id)
>>> {
>>> + struct membank *membank;
>>> +
>>> if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
>>> return -ENOMEM;
>>>
>>> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
>>> - kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
>>> + membank = xmalloc_bytes(sizeof(struct membank));
>>
>> You allocate memory but never free it. However, I think it would be better to
>> avoid the dynamic allocation. So I would consider to not use the structure
>> shm_membank and instead create a specific one for the domain construction.
>>
>
> True, a local variable "struct meminfo shm_banks" could be introduced only
> for domain construction in function construct_domU
Hmmm... I didn't suggest to introduce a local variable. I would still
much prefer if we keep using 'kinfo' because we keep all the domain
building information in one place.
So ``struct meninfo`` should want to be defined in ``kinfo``.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided
2023-01-09 7:50 ` Penny Zheng
@ 2023-01-09 10:07 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-09 10:07 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 09/01/2023 07:50, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Sunday, January 8, 2023 8:23 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap
>> when host address not provided
>>
>> Hi Penny,
>>
>
> Hi Julien,
>
>> On 15/11/2022 02:52, Penny Zheng wrote:
>>> when host address is not provided in "xen,shared-mem", we let Xen
>>> allocate requested shared memory from heap, and once the shared
>> memory
>>> is allocated, it will be marked as static(PGC_static), which means
>>> that it will be reserved as static memory, and will not go back to heap even
>> on freeing.
>>
>> Please don't move pages from the {xen,dom}heap to the static heap. If you
>> need to keep the pages for longer, then get an extra reference so they will
>> not be released.
>>
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> xen/arch/arm/domain_build.c | 83
>> ++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 82 insertions(+), 1 deletion(-)
>>>
>>> +static int __init allocate_shared_memory(struct shm_membank
>> *shm_membank,
>>> + paddr_t psize) {
>>> + struct meminfo *banks;
>>> + int ret;
>>> +
>>> + BUG_ON(shm_membank->mem.banks.meminfo != NULL);
>>> +
>>> + banks = xmalloc_bytes(sizeof(struct meminfo));
>>
>> Where is this freed?
>>
>
> These kinds of info will be only used in boot-time, so maybe I shall
> free them in init_done()?
I don't think you can free it in init_done() because we don't keep a
pointer to kinfo past construct_dom*().
> Or just after process_shm() ?
This might work. But I think it would be better to avoid the dynamic
memory allocation if we can.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-09 7:49 ` Penny Zheng
@ 2023-01-09 10:57 ` Julien Grall
2023-01-09 11:58 ` Penny Zheng
2023-01-18 6:09 ` Penny Zheng
0 siblings, 2 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-09 10:57 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
On 09/01/2023 07:49, Penny Zheng wrote:
> Hi Julien
Hi Penny,
> Happy new year~~~~
Happy new year too!
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Sunday, January 8, 2023 8:53 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
>> when host address not provided
>>
>> Hi,
>>
>> On 15/11/2022 02:52, Penny Zheng wrote:
>>> @@ -922,33 +927,82 @@ static mfn_t __init
>> acquire_shared_memory_bank(struct domain *d,
>>> d->max_pages += nr_pfns;
>>>
>>> smfn = maddr_to_mfn(pbase);
>>> - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
>>> - if ( res )
>>> + page = mfn_to_page(smfn);
>>> + /*
>>> + * If page is allocated from heap as static shared memory, then we just
>>> + * assign it to the owner domain
>>> + */
>>> + if ( page->count_info == (PGC_state_inuse | PGC_static) )
>> I am a bit confused how this can help differentiating becaPGC_state_inuse is
>> 0. So effectively, you are checking that count_info is equal to PGC_static.
>>
>
> When host address is provided, the host address range defined in
> xen,static-mem will be stored as a "struct membank" with type
> of "MEMBANK_STATIC_DOMAIN" in "bootinfo.reserved_mem"
> Then it will be initialized as static memory through "init_staticmem_pages"
> So here its page->count_info is PGC_state_free |PGC_static.
> For pages allocated from heap, its page state is different, being PGC_state_inuse.
> So actually, we are checking page state to tell the difference .
Ok. This is definitely not obvious from the code. But I think this is a
very fragile assumption.
Instead, it would be better if we allocate the memory in
acquire_shared_memory_bank() when the host address is not provided.
>
>> But as I wrote in a previous patch, I don't think you should convert
>> {xen,dom}heap pages to a static pages.
>>
>
> I agree that taking reference could also prevent giving these pages back to heap.
> But may I ask what is your concern on converting {xen,dom}heap pages to a static pages?
A few reasons:
1) I consider them as two distinct allocators. So far they have the
same behavior, but in the future this may change.
2) If the page is freed you really don't want the domain to be able to
re-use the page for a different purpose.
I realize that 2) is already a problem today with static pages. So I
think the best is to ensure that pages allocated for shared memory never
reach the any of the allocators.
>
>> [...]
>>
>>> +static int __init assign_shared_memory(struct domain *d,
>>> + struct shm_membank *shm_membank,
>>> + paddr_t gbase) {
>>> + int ret = 0;
>>> + unsigned long nr_pages, nr_borrowers;
>>> + struct page_info *page;
>>> + unsigned int i;
>>> + struct meminfo *meminfo;
>>> +
>>> + /* Host address is not provided in "xen,shared-mem" */
>>> + if ( shm_membank->mem.banks.meminfo )
>>> + {
>>> + meminfo = shm_membank->mem.banks.meminfo;
>>> + for ( i = 0; i < meminfo->nr_banks; i++ )
>>> + {
>>> + ret = acquire_shared_memory(d,
>>> + meminfo->bank[i].start,
>>> + meminfo->bank[i].size,
>>> + gbase);
>>> + if ( ret )
>>> + return ret;
>>> +
>>> + gbase += meminfo->bank[i].size;
>>> + }
>>> + }
>>> + else
>>> + {
>>> + ret = acquire_shared_memory(d,
>>> + shm_membank->mem.bank->start,
>>> + shm_membank->mem.bank->size, gbase);
>>> + if ( ret )
>>> + return ret;
>>> + }
>>
>> Looking at this change and...
>>
>>> +
>>> /*
>>> * Get the right amount of references per page, which is the number of
>>> * borrower domains.
>>> @@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct
>> domain *d,
>>> * So if the borrower is created first, it will cause adding pages
>>> * in the P2M without reference.
>>> */
>>> - page = mfn_to_page(smfn);
>>> - for ( i = 0; i < nr_pages; i++ )
>>> + if ( shm_membank->mem.banks.meminfo )
>>> {
>>> - if ( !get_page_nr(page + i, d, nr_borrowers) )
>>> + meminfo = shm_membank->mem.banks.meminfo;
>>> + for ( i = 0; i < meminfo->nr_banks; i++ )
>>> {
>>> - printk(XENLOG_ERR
>>> - "Failed to add %lu references to page %"PRI_mfn".\n",
>>> - nr_borrowers, mfn_x(smfn) + i);
>>> - goto fail;
>>> + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
>>> + nr_pages = PFN_DOWN(meminfo->bank[i].size);
>>> + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
>>> + if ( ret )
>>> + goto fail;
>>> }
>>> }
>>> + else
>>> + {
>>> + page = mfn_to_page(
>>> + maddr_to_mfn(shm_membank->mem.bank->start));
>>> + nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
>>> + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
>>> + if ( ret )
>>> + return ret;
>>> + }
>>
>> ... this one. The code to deal with a bank is exactly the same. But you need
>> the duplication because you special case "one bank".
>>
>> As I wrote in a previous patch, I don't think we should special case it.
>> If the concern is memory usage, then we should look at reworking meminfo
>> instead (or using a different structure).
>>
>
> A few concerns explained why I didn't choose "struct meminfo" over two
> pointers "struct membank*" and "struct meminfo*".
> 1) memory usage is the main reason.
> If we use "struct meminfo" over the current "struct membank*" and "struct meminfo*",
> "struct shm_meminfo" will become a array of 256 "struct shm_membank", with
> "struct shm_membank" being also an 256-item array, that is 256 * 256, too big for
> a structure and If I remembered clearly, it will lead to "more than PAGE_SIZE" compiling error.
I am not aware of any place where we would restrict the size of kinfo in
upstream. Can you give me a pointer?
> FWIT, either reworking meminfo or using a different structure, are both leading to sizing down
> the array, hmmm, I don't know which size is suitable. That's why I prefer pointer
> and dynamic allocation.
I would expect that in most cases, you will need only one bank when the
host address is not provided. So I think this is a bit odd to me to
impose a "large" allocation for them.
> 2) If we use "struct meminfo*" over the current "struct membank*" and "struct meminfo*",
> we will need a new flag to differentiate two scenarios(host address provided or not), As
> the special case "struct membank*" is already helping us differentiate.
> And since when host address is provided, the related "struct membank" also needs to be
> reserved in "bootinfo.reserved_mem". That's why I used pointer " struct membank*" to
> avoid memory waste.
I am confused... Today you are defining as:
struct {
struct membank *;
struct {
struct meminfo *;
unsigned long total_size;
}
}
So on arm64 host, you would use 24 bytes. If we were using an union
instead. We would use 16 bytes. Adding a 32-bit fields, would bring to
20 bytes (assuming we can re-use a padding).
Therefore, there is no memory waste here with a flag...
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-09 10:57 ` Julien Grall
@ 2023-01-09 11:58 ` Penny Zheng
2023-01-09 18:23 ` Julien Grall
2023-01-18 6:09 ` Penny Zheng
1 sibling, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2023-01-09 11:58 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, January 9, 2023 6:58 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> when host address not provided
>
>
>
> On 09/01/2023 07:49, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> > Happy new year~~~~
>
> Happy new year too!
>
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Sunday, January 8, 2023 8:53 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>
> >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> >> when host address not provided
> >>
> >> Hi,
> >>
> >
> > A few concerns explained why I didn't choose "struct meminfo" over two
> > pointers "struct membank*" and "struct meminfo*".
> > 1) memory usage is the main reason.
> > If we use "struct meminfo" over the current "struct membank*" and
> > "struct meminfo*", "struct shm_meminfo" will become a array of 256
> > "struct shm_membank", with "struct shm_membank" being also an 256-
> item
> > array, that is 256 * 256, too big for a structure and If I remembered clearly,
> it will lead to "more than PAGE_SIZE" compiling error.
>
> I am not aware of any place where we would restrict the size of kinfo in
> upstream. Can you give me a pointer?
>
If I remembered correctly, my first version of "struct shm_meminfo" is this
"big"(256 * 256) structure, and it leads to the whole xen binary is bigger than 2MB. ;\
> > FWIT, either reworking meminfo or using a different structure, are
> > both leading to sizing down the array, hmmm, I don't know which size
> > is suitable. That's why I prefer pointer and dynamic allocation.
>
> I would expect that in most cases, you will need only one bank when the host
> address is not provided. So I think this is a bit odd to me to impose a "large"
> allocation for them.
>
Only if user is not defining size as something like (2^a + 2^b + 2^c + ...). ;\
So maybe 8 or 16 is enough?
struct new_meminfo {
unsigned int nr_banks;
struct membank bank[8];
};
Correct me if I'm wrong:
The "struct shm_membank" you are suggesting is looking like this, right?
struct shm_membank {
char shm_id[MAX_SHM_ID_LENGTH];
unsigned int nr_shm_borrowers;
struct new_meminfo shm_banks;
unsigned long total_size;
};
Let me try~
> > 2) If we use "struct meminfo*" over the current "struct membank*" and
> > "struct meminfo*", we will need a new flag to differentiate two
> > scenarios(host address provided or not), As the special case "struct
> membank*" is already helping us differentiate.
> > And since when host address is provided, the related "struct membank"
> > also needs to be reserved in "bootinfo.reserved_mem". That's why I
> > used pointer " struct membank*" to avoid memory waste.
>
> I am confused... Today you are defining as:
>
> struct {
> struct membank *;
> struct {
> struct meminfo *;
> unsigned long total_size;
> }
> }
>
> So on arm64 host, you would use 24 bytes. If we were using an union instead.
> We would use 16 bytes. Adding a 32-bit fields, would bring to
> 20 bytes (assuming we can re-use a padding).
>
> Therefore, there is no memory waste here with a flag...
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-09 11:58 ` Penny Zheng
@ 2023-01-09 18:23 ` Julien Grall
2023-01-10 3:38 ` Penny Zheng
0 siblings, 1 reply; 38+ messages in thread
From: Julien Grall @ 2023-01-09 18:23 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 09/01/2023 11:58, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Monday, January 9, 2023 6:58 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
>> when host address not provided
>>
>>
>>
>> On 09/01/2023 07:49, Penny Zheng wrote:
>>> Hi Julien
>>
>> Hi Penny,
>>
>>> Happy new year~~~~
>>
>> Happy new year too!
>>
>>>> -----Original Message-----
>>>> From: Julien Grall <julien@xen.org>
>>>> Sent: Sunday, January 8, 2023 8:53 PM
>>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
>> devel@lists.xenproject.org
>>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; Bertrand Marquis
>>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>>>> <Volodymyr_Babchuk@epam.com>
>>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
>>>> when host address not provided
>>>>
>>>> Hi,
>>>>
>>>
>>> A few concerns explained why I didn't choose "struct meminfo" over two
>>> pointers "struct membank*" and "struct meminfo*".
>>> 1) memory usage is the main reason.
>>> If we use "struct meminfo" over the current "struct membank*" and
>>> "struct meminfo*", "struct shm_meminfo" will become a array of 256
>>> "struct shm_membank", with "struct shm_membank" being also an 256-
>> item
>>> array, that is 256 * 256, too big for a structure and If I remembered clearly,
>> it will lead to "more than PAGE_SIZE" compiling error.
>>
>> I am not aware of any place where we would restrict the size of kinfo in
>> upstream. Can you give me a pointer?
>>
>
> If I remembered correctly, my first version of "struct shm_meminfo" is this
> "big"(256 * 256) structure, and it leads to the whole xen binary is bigger than 2MB. ;\
Ah so the problem is because shm_mem is used in bootinfo. Then I think
we should create a distinct structure when dealing with domain information.
>
>>> FWIT, either reworking meminfo or using a different structure, are
>>> both leading to sizing down the array, hmmm, I don't know which size
>>> is suitable. That's why I prefer pointer and dynamic allocation.
>>
>> I would expect that in most cases, you will need only one bank when the host
>> address is not provided. So I think this is a bit odd to me to impose a "large"
>> allocation for them.
>>
>
> Only if user is not defining size as something like (2^a + 2^b + 2^c + ...). ;\
> So maybe 8 or 16 is enough?
> struct new_meminfo {
"new" is a bit strange. The name would want to be changed. Or maybe
better the structure been defined within the next structure and anonymized.
> unsigned int nr_banks;
> struct membank bank[8];
> };
>
> Correct me if I'm wrong:
> The "struct shm_membank" you are suggesting is looking like this, right?
> struct shm_membank {
> char shm_id[MAX_SHM_ID_LENGTH];
> unsigned int nr_shm_borrowers;
> struct new_meminfo shm_banks;
> unsigned long total_size;
> };
AFAIU, shm_membank would still be used to get the information from the
host device-tree. If so, then I am afraid this is not an option to me
because it would make the code to reserve memory more complex.
Instead, we should create a separate structure that will only be used
for domain shared memory information.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-09 18:23 ` Julien Grall
@ 2023-01-10 3:38 ` Penny Zheng
2023-01-10 12:47 ` Julien Grall
0 siblings, 1 reply; 38+ messages in thread
From: Penny Zheng @ 2023-01-10 3:38 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, January 10, 2023 2:23 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> when host address not provided
>
> Hi Penny,
Hi Julien,
>
> On 09/01/2023 11:58, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Monday, January 9, 2023 6:58 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>
> >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> >> when host address not provided
> >>
> >>
> >>
> >> On 09/01/2023 07:49, Penny Zheng wrote:
> >>> Hi Julien
> >>
> >> Hi Penny,
> >>
> >>> Happy new year~~~~
> >>
> >> Happy new year too!
> >>
> >>>> -----Original Message-----
> >>>> From: Julien Grall <julien@xen.org>
> >>>> Sent: Sunday, January 8, 2023 8:53 PM
> >>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> >> devel@lists.xenproject.org
> >>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >>>> <sstabellini@kernel.org>; Bertrand Marquis
> >>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >>>> <Volodymyr_Babchuk@epam.com>
> >>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to
> >>>> owner when host address not provided
> >>>>
> >>>> Hi,
> >>>>
> >>>
> >>> A few concerns explained why I didn't choose "struct meminfo" over
> >>> two pointers "struct membank*" and "struct meminfo*".
> >>> 1) memory usage is the main reason.
> >>> If we use "struct meminfo" over the current "struct membank*" and
> >>> "struct meminfo*", "struct shm_meminfo" will become a array of 256
> >>> "struct shm_membank", with "struct shm_membank" being also an 256-
> >> item
> >>> array, that is 256 * 256, too big for a structure and If I
> >>> remembered clearly,
> >> it will lead to "more than PAGE_SIZE" compiling error.
> >>
> >> I am not aware of any place where we would restrict the size of kinfo
> >> in upstream. Can you give me a pointer?
> >>
> >
> > If I remembered correctly, my first version of "struct shm_meminfo" is
> > this
> > "big"(256 * 256) structure, and it leads to the whole xen binary is
> >. ;\
>
> Ah so the problem is because shm_mem is used in bootinfo. Then I think we
> should create a distinct structure when dealing with domain information.
>
Yes, If I use the latter "struct shm_info", keeping the shm memory info out of the bootinfo,
I think we could avoid "bigger than 2MB" error.
Hmm, out of curiosity, The way to create "distinct" structure is like creating another section
for these distinct structures in lds, just like the existing .dtb section?
> >
> >>> FWIT, either reworking meminfo or using a different structure, are
> >>> both leading to sizing down the array, hmmm, I don't know which size
> >>> is suitable. That's why I prefer pointer and dynamic allocation.
> >>
> >> I would expect that in most cases, you will need only one bank when
> >> the host address is not provided. So I think this is a bit odd to me to
> impose a "large"
> >> allocation for them.
> >>
> >
> > Only if user is not defining size as something like (2^a + 2^b + 2^c +
> > ...). ;\ So maybe 8 or 16 is enough?
> > struct new_meminfo {
>
> "new" is a bit strange. The name would want to be changed. Or maybe better
> the structure been defined within the next structure and anonymized.
>
> > unsigned int nr_banks;
> > struct membank bank[8];
> > };
> >
> > Correct me if I'm wrong:
> > The "struct shm_membank" you are suggesting is looking like this, right?
> > struct shm_membank {
> > char shm_id[MAX_SHM_ID_LENGTH];
> > unsigned int nr_shm_borrowers;
> > struct new_meminfo shm_banks;
> > unsigned long total_size;
> > };
>
> AFAIU, shm_membank would still be used to get the information from the
> host device-tree. If so, then I am afraid this is not an option to me because it
> would make the code to reserve memory more complex.
>
> Instead, we should create a separate structure that will only be used for
> domain shared memory information.
>
Ah, so you are suggesting we should extract the domain shared memory information only
when dealing with the information from the host device-tree, something like this:
struct shm_info {
char shm_id[MAX_SHM_ID_LENGTH];
unsigned int nr_shm_borrowers;
}
I'll try and may *bother* you when encountering any problem~
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-10 3:38 ` Penny Zheng
@ 2023-01-10 12:47 ` Julien Grall
0 siblings, 0 replies; 38+ messages in thread
From: Julien Grall @ 2023-01-10 12:47 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Penny,
On 10/01/2023 03:38, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Tuesday, January 10, 2023 2:23 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
>> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
>> when host address not provided
>>
>> Hi Penny,
>
> Hi Julien,
>
>>
>> On 09/01/2023 11:58, Penny Zheng wrote:
>>>> -----Original Message-----
>>>> From: Julien Grall <julien@xen.org>
>>>> Sent: Monday, January 9, 2023 6:58 PM
>>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
>> devel@lists.xenproject.org
>>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; Bertrand Marquis
>>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>>>> <Volodymyr_Babchuk@epam.com>
>>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
>>>> when host address not provided
>>>>
>>>>
>>>>
>>>> On 09/01/2023 07:49, Penny Zheng wrote:
>>>>> Hi Julien
>>>>
>>>> Hi Penny,
>>>>
>>>>> Happy new year~~~~
>>>>
>>>> Happy new year too!
>>>>
>>>>>> -----Original Message-----
>>>>>> From: Julien Grall <julien@xen.org>
>>>>>> Sent: Sunday, January 8, 2023 8:53 PM
>>>>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
>>>> devel@lists.xenproject.org
>>>>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
>>>>>> <sstabellini@kernel.org>; Bertrand Marquis
>>>>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
>>>>>> <Volodymyr_Babchuk@epam.com>
>>>>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to
>>>>>> owner when host address not provided
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>
>>>>> A few concerns explained why I didn't choose "struct meminfo" over
>>>>> two pointers "struct membank*" and "struct meminfo*".
>>>>> 1) memory usage is the main reason.
>>>>> If we use "struct meminfo" over the current "struct membank*" and
>>>>> "struct meminfo*", "struct shm_meminfo" will become a array of 256
>>>>> "struct shm_membank", with "struct shm_membank" being also an 256-
>>>> item
>>>>> array, that is 256 * 256, too big for a structure and If I
>>>>> remembered clearly,
>>>> it will lead to "more than PAGE_SIZE" compiling error.
>>>>
>>>> I am not aware of any place where we would restrict the size of kinfo
>>>> in upstream. Can you give me a pointer?
>>>>
>>>
>>> If I remembered correctly, my first version of "struct shm_meminfo" is
>>> this
>>> "big"(256 * 256) structure, and it leads to the whole xen binary is
>>> . ;\
>>
>> Ah so the problem is because shm_mem is used in bootinfo. Then I think we
>> should create a distinct structure when dealing with domain information.
>>
>
> Yes, If I use the latter "struct shm_info", keeping the shm memory info out of the bootinfo,
> I think we could avoid "bigger than 2MB" error.
>
> Hmm, out of curiosity, The way to create "distinct" structure is like creating another section
> for these distinct structures in lds, just like the existing .dtb section?
No. I meant defining a new structure (i.e. struct {}) that would be used
in kernel_info. So you don't grow the one used by bootinfo.
>
>>>
>>>>> FWIT, either reworking meminfo or using a different structure, are
>>>>> both leading to sizing down the array, hmmm, I don't know which size
>>>>> is suitable. That's why I prefer pointer and dynamic allocation.
>>>>
>>>> I would expect that in most cases, you will need only one bank when
>>>> the host address is not provided. So I think this is a bit odd to me to
>> impose a "large"
>>>> allocation for them.
>>>>
>>>
>>> Only if user is not defining size as something like (2^a + 2^b + 2^c +
>>> ...). ;\ So maybe 8 or 16 is enough?
>>> struct new_meminfo {
>>
>> "new" is a bit strange. The name would want to be changed. Or maybe better
>> the structure been defined within the next structure and anonymized.
>>
>>> unsigned int nr_banks;
>>> struct membank bank[8];
>>> };
>>>
>>> Correct me if I'm wrong:
>>> The "struct shm_membank" you are suggesting is looking like this, right?
>>> struct shm_membank {
>>> char shm_id[MAX_SHM_ID_LENGTH];
>>> unsigned int nr_shm_borrowers;
>>> struct new_meminfo shm_banks;
>>> unsigned long total_size;
>>> };
>>
>> AFAIU, shm_membank would still be used to get the information from the
>> host device-tree. If so, then I am afraid this is not an option to me because it
>> would make the code to reserve memory more complex.
>>
>> Instead, we should create a separate structure that will only be used for
>> domain shared memory information.
>>
>
> Ah, so you are suggesting we should extract the domain shared memory information only
> when dealing with the information from the host device-tree, something like this:
> struct shm_info {
> char shm_id[MAX_SHM_ID_LENGTH];
> unsigned int nr_shm_borrowers;
> }
I am not entirely sure what you are suggesting. So I will wait for the
code to understand.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2023-01-09 10:57 ` Julien Grall
2023-01-09 11:58 ` Penny Zheng
@ 2023-01-18 6:09 ` Penny Zheng
1 sibling, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2023-01-18 6:09 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Wei Chen, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, January 9, 2023 6:58 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> when host address not provided
>
>
>
> On 09/01/2023 07:49, Penny Zheng wrote:
> > Hi Julien
>
> Hi Penny,
>
> > Happy new year~~~~
>
> Happy new year too!
>
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Sunday, January 8, 2023 8:53 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Bertrand Marquis
> >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> >> <Volodymyr_Babchuk@epam.com>
> >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner
> >> when host address not provided
> >>
> >> Hi,
> >>
> >> On 15/11/2022 02:52, Penny Zheng wrote:
> >>> @@ -922,33 +927,82 @@ static mfn_t __init
> >> acquire_shared_memory_bank(struct domain *d,
> >>> d->max_pages += nr_pfns;
> >>>
> >>> smfn = maddr_to_mfn(pbase);
> >>> - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> >>> - if ( res )
> >>> + page = mfn_to_page(smfn);
> >>> + /*
> >>> + * If page is allocated from heap as static shared memory, then we
> just
> >>> + * assign it to the owner domain
> >>> + */
> >>> + if ( page->count_info == (PGC_state_inuse | PGC_static) )
> >> I am a bit confused how this can help differentiating
> >> becaPGC_state_inuse is 0. So effectively, you are checking that count_info
> is equal to PGC_static.
> >>
> >
> > When host address is provided, the host address range defined in
> > xen,static-mem will be stored as a "struct membank" with type of
> > "MEMBANK_STATIC_DOMAIN" in "bootinfo.reserved_mem"
> > Then it will be initialized as static memory through "init_staticmem_pages"
> > So here its page->count_info is PGC_state_free |PGC_static.
> > For pages allocated from heap, its page state is different, being
> PGC_state_inuse.
> > So actually, we are checking page state to tell the
> difference .
>
> Ok. This is definitely not obvious from the code. But I think this is a very
> fragile assumption.
>
> Instead, it would be better if we allocate the memory in
> acquire_shared_memory_bank() when the host address is not provided.
>
Right now, acquire_shared_memory_bank is called only when domain is owner domain.
It is applicable when host address is provided, we could still do guest physmap when
borrower accessed before owner, as address is provided.
However when host address is not provided, we must allocate memory at first domain.
So allocating the memory shall be called outside acquire_shared_memory_bank
> >
> >> But as I wrote in a previous patch, I don't think you should convert
> >> {xen,dom}heap pages to a static pages.
> >>
> >
> > I agree that taking reference could also prevent giving these pages back to
> heap.
> > But may I ask what is your concern on converting {xen,dom}heap pages to
> a static pages?
>
> A few reasons:
> 1) I consider them as two distinct allocators. So far they have the same
> behavior, but in the future this may change.
> 2) If the page is freed you really don't want the domain to be able to re-use
> the page for a different purpose.
>
>
> I realize that 2) is already a problem today with static pages. So I think the
> best is to ensure that pages allocated for shared memory never reach the
> any of the allocators.
>
>
> Cheers,
>
> --
> Julien Grall
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
2022-11-15 2:52 ` [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region Penny Zheng
2023-01-08 11:44 ` Julien Grall
@ 2023-02-07 20:55 ` Stewart Hildebrand
2023-02-14 9:56 ` Penny Zheng
1 sibling, 1 reply; 38+ messages in thread
From: Stewart Hildebrand @ 2023-02-07 20:55 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Penny,
On 11/14/22 21:52, Penny Zheng wrote:
> ...
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index fdbf68aadc..2d4ae0f00a 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -50,10 +50,6 @@ struct membank {
> paddr_t start;
> paddr_t size;
> enum membank_type type;
> -#ifdef CONFIG_STATIC_SHM
> - char shm_id[MAX_SHM_ID_LENGTH];
> - unsigned int nr_shm_borrowers;
> -#endif
> };
>
> struct meminfo {
> @@ -61,6 +57,17 @@ struct meminfo {
> struct membank bank[NR_MEM_BANKS];
> };
>
> +struct shm_membank {
> + char shm_id[MAX_SHM_ID_LENGTH];
> + unsigned int nr_shm_borrowers;
> + struct membank *membank;
> +};
> +
> +struct shm_meminfo {
> + unsigned int nr_banks;
> + struct shm_membank bank[NR_MEM_BANKS];
> +};
> +
> /*
> * The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
> * The purpose of the domU flag is to avoid getting confused in
> @@ -105,6 +112,7 @@ struct bootinfo {
> struct meminfo acpi;
> #endif
> bool static_heap;
> + struct shm_meminfo shm_mem;
> };
>
> struct map_range_data
The reduction in the sizeof struct membank is a welcome improvement, in my opinion, because it is multiplied by NR_MEM_BANKS, and IIRC we only have 32k of boot stack to play with.
Before this patch:
sizeof(struct kernel_info): 20648
sizeof(struct meminfo): 10248
sizeof(struct shm_meminfo): 10248
When building with EXTRA_CFLAGS_XEN_CORE="Wstack-usage=4096 -Wno-error=stack-usage=":
arch/arm/domain_build.c: In function ‘construct_domU’:
arch/arm/domain_build.c:3747:19: warning: stack usage is 20720 bytes [-Wstack-usage=]
3747 | static int __init construct_domU(struct domain *d,
| ^~~~~~~~~~~~~~
arch/arm/domain_build.c: In function ‘construct_dom0’:
arch/arm/domain_build.c:3979:19: warning: stack usage is 20688 bytes [-Wstack-usage=]
3979 | static int __init construct_dom0(struct domain *d)
| ^~~~~~~~~~~~~~
After this patch:
sizeof(struct kernel_info): 14504
sizeof(struct meminfo): 6152
sizeof(struct shm_meminfo): 8200
arch/arm/domain_build.c: In function ‘construct_domU’:
arch/arm/domain_build.c:3754:19: warning: stack usage is 14576 bytes [-Wstack-usage=]
3754 | static int __init construct_domU(struct domain *d,
| ^~~~~~~~~~~~~~
arch/arm/domain_build.c: In function ‘construct_dom0’:
arch/arm/domain_build.c:3986:19: warning: stack usage is 14544 bytes [-Wstack-usage=]
3986 | static int __init construct_dom0(struct domain *d)
| ^~~~~~~~~~~~~~
A later patch in this series will increase it again slightly. Note that I'm not expecting this series to address these particular warnings, I'm merely pointing out the (positive) impact of the change.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory
2022-11-15 2:52 ` [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory Penny Zheng
@ 2023-02-07 20:56 ` Stewart Hildebrand
0 siblings, 0 replies; 38+ messages in thread
From: Stewart Hildebrand @ 2023-02-07 20:56 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Penny,
On 11/14/22 21:52, Penny Zheng wrote:
> We split the codes of allocate_bank_memory into two parts,
> allocate_domheap_memory and guest_physmap_memory.
>
> One is about allocating guest RAM from heap, which could be re-used later for
> allocating static shared memory from heap when host address is not provided.
>
> The other is building up guest P2M mapping.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 93 +++++++++++++++++++++++++++----------
> 1 file changed, 68 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d2b9e60b5c..92763e96fc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -390,34 +390,18 @@ static void __init allocate_memory_11(struct domain *d,
> }
> }
>
> -static bool __init allocate_bank_memory(struct domain *d,
> - struct kernel_info *kinfo,
> - gfn_t sgfn,
> - paddr_t tot_size)
> +static bool __init allocate_domheap_memory(struct domain *d,
> + paddr_t tot_size,
> + struct meminfo *mem)
> {
> - int res;
> struct page_info *pg;
> - struct membank *bank;
> unsigned int max_order = ~0;
>
> - /*
> - * allocate_bank_memory can be called with a tot_size of zero for
> - * the second memory bank. It is not an error and we can safely
> - * avoid creating a zero-size memory bank.
> - */
> - if ( tot_size == 0 )
> - return true;
> -
> - bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> - bank->start = gfn_to_gaddr(sgfn);
> - bank->size = tot_size;
> -
> while ( tot_size > 0 )
> {
> unsigned int order = get_allocation_size(tot_size);
>
> order = min(max_order, order);
> -
> pg = alloc_domheap_pages(d, order, 0);
> if ( !pg )
> {
> @@ -437,15 +421,74 @@ static bool __init allocate_bank_memory(struct domain *d,
> continue;
> }
>
> - res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
> - if ( res )
> - {
> - dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> + if ( mem->nr_banks == NR_MEM_BANKS )
> return false;
> - }
> +
> + mem->bank[mem->nr_banks].start = mfn_to_maddr(page_to_mfn(pg));
> + mem->bank[mem->nr_banks].size = 1UL << (PAGE_SHIFT + order);
> + mem->nr_banks++;
> + tot_size -= (1UL << (PAGE_SHIFT + order));
Why the change from 1ULL to 1UL?
> + }
> +
> + return true;
> +}
> +
> +static int __init guest_physmap_memory(struct domain *d,
> + const struct meminfo *mem, gfn_t sgfn)
> +{
> + unsigned int i;
> + int res;
> +
> + for ( i = 0; i < mem->nr_banks; i++ )
> + {
> + paddr_t size = mem->bank[i].size;
> + unsigned int order = get_order_from_bytes(size);
> +
> + /* Size must be power of two */
> + BUG_ON(!size || (size & (size - 1)));
> + res = guest_physmap_add_page(d, sgfn, maddr_to_mfn(mem->bank[i].start),
> + order);
> + if ( res )
> + return res;
>
> sgfn = gfn_add(sgfn, 1UL << order);
> - tot_size -= (1ULL << (PAGE_SHIFT + order));
> + }
> +
> + return 0;
> +}
> +
> +static bool __init allocate_bank_memory(struct domain *d,
> + struct kernel_info *kinfo,
> + gfn_t sgfn,
> + paddr_t total_size)
> +{
> + struct membank *bank;
> + struct meminfo host = {0};
This function uses 6k of stack, largely due to the sizeof struct meminfo.
arch/arm/domain_build.c: In function ‘allocate_bank_memory’:
arch/arm/domain_build.c:461:20: warning: stack usage is 6224 bytes [-Wstack-usage=]
461 | static bool __init allocate_bank_memory(struct domain *d,
| ^~~~~~~~~~~~~~~~~~~~
It may be fine for now, but I wanted to at least check if anyone else had an opinion on allocating "struct meminfo host" by some other means, such as xzalloc/free or by making it static:
static struct meminfo __initdata host;
memset(&host, 0, sizeof(struct meminfo));
Particularly if we ever plan on increasing NR_MEM_BANKS again in the future.
> +
> + /*
> + * allocate_bank_memory can be called with a total_size of zero for
> + * the second memory bank. It is not an error and we can safely
> + * avoid creating a zero-size memory bank.
> + */
> + if ( total_size == 0 )
> + return true;
> +
> + bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> + bank->start = gfn_to_gaddr(sgfn);
> + bank->size = total_size;
> +
> + if ( !allocate_domheap_memory(d, total_size, &host) )
> + {
> + printk(XENLOG_ERR "Failed to allocate (%"PRIpaddr"MB) pages to %pd\n",
> + total_size >> 20, d);
> + return false;
> + }
> +
> + if ( guest_physmap_memory(d, &host, sgfn) )
> + {
> + printk(XENLOG_ERR "Failed to map (%"PRIpaddr"MB) pages to %pd\n",
> + total_size >> 20, d);
> + return false;
> }
>
> kinfo->mem.nr_banks++;
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided
2022-11-15 2:52 ` [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided Penny Zheng
2023-01-08 12:22 ` Julien Grall
@ 2023-02-07 20:57 ` Stewart Hildebrand
1 sibling, 0 replies; 38+ messages in thread
From: Stewart Hildebrand @ 2023-02-07 20:57 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
On 11/14/22 21:52, Penny Zheng wrote:
> when host address is not provided in "xen,shared-mem", we let Xen
> allocate requested shared memory from heap, and once the shared memory is
> allocated, it will be marked as static(PGC_static), which means that it will be
> reserved as static memory, and will not go back to heap even on freeing.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 83 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index fbb196d8a4..3de96882a5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -835,6 +835,72 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
> return true;
> }
>
> +static int __init mark_shared_memory_static(struct shm_membank *shm_membank)
> +{
> + unsigned int bank;
> + unsigned long i, nr_mfns;
> + struct page_info *pg;
> + struct meminfo *meminfo;
> +
> + BUG_ON(!shm_membank->mem.banks.meminfo);
> + meminfo = shm_membank->mem.banks.meminfo;
> + for ( bank = 0; bank < meminfo->nr_banks; bank++ )
> + {
> + pg = mfn_to_page(maddr_to_mfn(meminfo->bank[bank].start));
> + nr_mfns = PFN_DOWN(meminfo->bank[bank].size);
> +
> + for ( i = 0; i < nr_mfns; i++ )
> + {
> + /* The page should be already allocated from heap. */
> + if ( !pg[i].count_info & PGC_state_inuse )
> + {
> + printk(XENLOG_ERR
> + "pg[%lu] MFN %"PRI_mfn" c=%#lx\n",
> + i, mfn_x(page_to_mfn(pg)) + i, pg[i].count_info);
> + goto fail;
> + }
> +
> + pg[i].count_info |= PGC_static;
> + }
> + }
> +
> + return 0;
> +
> + fail:
> + while ( bank >= 0 )
When building with EXTRA_CFLAGS_XEN_CORE="-Wtype-limits -Wno-error=type-limits", we get the following warning:
arch/arm/domain_build.c: In function ‘mark_shared_memory_static’:
arch/arm/domain_build.c:879:18: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
879 | while ( bank >= 0 )
| ^~
> + {
> + while ( --i >= 0 )
Similarly here:
arch/arm/domain_build.c:881:21: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
881 | while ( --i >= 0 )
| ^~
> + pg[i].count_info &= ~PGC_static;
> + i = PFN_DOWN(meminfo->bank[--bank].size);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int __init allocate_shared_memory(struct shm_membank *shm_membank,
> + paddr_t psize)
> +{
> + struct meminfo *banks;
> + int ret;
> +
> + BUG_ON(shm_membank->mem.banks.meminfo != NULL);
> +
> + banks = xmalloc_bytes(sizeof(struct meminfo));
> + if ( banks == NULL )
> + return -ENOMEM;
> + shm_membank->mem.banks.meminfo = banks;
> + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct meminfo));
> +
> + if ( !allocate_domheap_memory(NULL, psize, shm_membank->mem.banks.meminfo) )
> + return -EINVAL;
> +
> + ret = mark_shared_memory_static(shm_membank);
> + if ( ret )
> + return ret;
> +
> + return 0;
> +}
> +
> static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> paddr_t pbase, paddr_t psize)
> {
> @@ -975,7 +1041,7 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> unsigned int i;
> const char *role_str;
> const char *shm_id;
> - bool owner_dom_io = true;
> + bool owner_dom_io = true, paddr_assigned = true;
> struct shm_membank *shm_membank;
>
> if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> @@ -1035,6 +1101,21 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> return -ENOENT;
> }
>
> + /*
> + * When host address is not provided in "xen,shared-mem",
> + * we let Xen allocate requested memory from heap at first domain.
> + */
> + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo )
> + {
> + ret = allocate_shared_memory(shm_membank, psize);
> + if ( ret )
> + {
> + printk("%pd: failed to allocate shared memory bank(%"PRIpaddr"MB) from heap: %d\n",
> + d, psize >> 20, ret);
> + return ret;
> + }
> + }
> +
> /*
> * DOMID_IO is a fake domain and is not described in the Device-Tree.
> * Therefore when the owner of the shared region is DOMID_IO, we will
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner when host address not provided
2022-11-15 2:52 ` [PATCH v1 06/13] xen/arm: assign shared memory to owner " Penny Zheng
2023-01-08 12:52 ` Julien Grall
@ 2023-02-07 20:58 ` Stewart Hildebrand
1 sibling, 0 replies; 38+ messages in thread
From: Stewart Hildebrand @ 2023-02-07 20:58 UTC (permalink / raw)
To: Penny Zheng, xen-devel
Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Penny,
On 11/14/22 21:52, Penny Zheng wrote:
> With the introduction of new scenario where host address is not provided
> in "xen,shared-mem", the function "assign_shared_memory" shall be adapted
> to it too.
>
> Shared memory will already be allocated from heap, when calling
> "assign_shared_memory" with unprovided host address.
> So in "assign_shared_memory", we just need to assign these static shared pages
> to its owner domain using function "assign_pages", and add as many
> additional reference as the number of borrowers.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> xen/arch/arm/domain_build.c | 160 ++++++++++++++++++++++++++++++------
> 1 file changed, 133 insertions(+), 27 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 3de96882a5..faf0784bb0 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -817,8 +817,12 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank)
> {
> struct page_info *page;
> struct domain *d;
> + paddr_t pbase;
>
> - page = maddr_to_page(shm_membank->mem.bank->start);
> + pbase = shm_membank->mem.banks.meminfo ?
> + shm_membank->mem.banks.meminfo->bank[0].start :
> + shm_membank->mem.bank->start;
> + page = maddr_to_page(pbase);
> d = page_get_owner_and_reference(page);
> if ( d == NULL )
> return false;
> @@ -907,6 +911,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> mfn_t smfn;
> unsigned long nr_pfns;
> int res;
> + struct page_info *page;
>
> /*
> * Pages of statically shared memory shall be included
> @@ -922,33 +927,82 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> d->max_pages += nr_pfns;
>
> smfn = maddr_to_mfn(pbase);
> - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> - if ( res )
> + page = mfn_to_page(smfn);
> + /*
> + * If page is allocated from heap as static shared memory, then we just
> + * assign it to the owner domain
> + */
> + if ( page->count_info == (PGC_state_inuse | PGC_static) )
> {
> - printk(XENLOG_ERR
> - "%pd: failed to acquire static memory: %d.\n", d, res);
> - d->max_pages -= nr_pfns;
> - return INVALID_MFN;
> + res = assign_pages(page, nr_pfns, d, 0);
> + if ( res )
> + {
> + printk(XENLOG_ERR
> + "%pd: failed to assign static memory: %d.\n", d, res);
> + return INVALID_MFN;
> + }
> + }
> + else
> + {
> + res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> + if ( res )
> + {
> + printk(XENLOG_ERR
> + "%pd: failed to acquire static memory: %d.\n", d, res);
> + d->max_pages -= nr_pfns;
> + return INVALID_MFN;
> + }
> }
>
> return smfn;
> }
>
> -static int __init assign_shared_memory(struct domain *d,
> - struct shm_membank *shm_membank,
> - paddr_t gbase)
> +static void __init remove_shared_memory_ref(struct page_info *page,
> + unsigned long nr_pages,
> + unsigned long nr_borrowers)
> {
> - mfn_t smfn;
> - int ret = 0;
> - unsigned long nr_pages, nr_borrowers, i;
> - struct page_info *page;
> - paddr_t pbase, psize;
> + while ( --nr_pages >= 0 )
arch/arm/domain_build.c: In function ‘remove_shared_memory_ref’:
arch/arm/domain_build.c:969:24: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
969 | while ( --nr_pages >= 0 )
| ^~
> + put_page_nr(page + nr_pages, nr_borrowers);
> +}
>
> - pbase = shm_membank->mem.bank->start;
> - psize = shm_membank->mem.bank->size;
> +static int __init add_shared_memory_ref(struct domain *d, struct page_info *page,
> + unsigned long nr_pages,
> + unsigned long nr_borrowers)
> +{
> + unsigned int i;
>
> - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> - d, pbase, pbase + psize);
> + /*
> + * Instead of letting borrower domain get a page ref, we add as many
> + * additional reference as the number of borrowers when the owner
> + * is allocated, since there is a chance that owner is created
> + * after borrower.
> + * So if the borrower is created first, it will cause adding pages
> + * in the P2M without reference.
> + */
> + for ( i = 0; i < nr_pages; i++ )
> + {
> + if ( !get_page_nr(page + i, d, nr_borrowers) )
> + {
> + printk(XENLOG_ERR
> + "Failed to add %lu references to page %"PRI_mfn".\n",
> + nr_borrowers, mfn_x(page_to_mfn(page)) + i);
> + goto fail;
> + }
> + }
> + return 0;
> +
> + fail:
> + remove_shared_memory_ref(page, i, nr_borrowers);
> + return -EINVAL;
> +}
> +
> +static int __init acquire_shared_memory(struct domain *d,
> + paddr_t pbase, paddr_t psize,
> + paddr_t gbase)
> +{
> + mfn_t smfn;
> + int ret = 0;
> + unsigned long nr_pages;
When building with EXTRA_CFLAGS_XEN_CORE="-Wunused-but-set-variable -Wno-error=unused-but-set-variable"
arch/arm/domain_build.c: In function ‘acquire_shared_memory’:
arch/arm/domain_build.c:1010:19: warning: variable ‘nr_pages’ set but not used [-Wunused-but-set-variable]
1010 | unsigned long nr_pages;
| ^~~~~~~~
>
> smfn = acquire_shared_memory_bank(d, pbase, psize);
> if ( mfn_eq(smfn, INVALID_MFN) )
> @@ -970,6 +1024,44 @@ static int __init assign_shared_memory(struct domain *d,
> }
> }
>
> + return 0;
> +}
> +
> +static int __init assign_shared_memory(struct domain *d,
> + struct shm_membank *shm_membank,
> + paddr_t gbase)
> +{
> + int ret = 0;
> + unsigned long nr_pages, nr_borrowers;
> + struct page_info *page;
> + unsigned int i;
> + struct meminfo *meminfo;
> +
> + /* Host address is not provided in "xen,shared-mem" */
> + if ( shm_membank->mem.banks.meminfo )
> + {
> + meminfo = shm_membank->mem.banks.meminfo;
> + for ( i = 0; i < meminfo->nr_banks; i++ )
> + {
> + ret = acquire_shared_memory(d,
> + meminfo->bank[i].start,
> + meminfo->bank[i].size,
> + gbase);
> + if ( ret )
> + return ret;
> +
> + gbase += meminfo->bank[i].size;
> + }
> + }
> + else
> + {
> + ret = acquire_shared_memory(d,
> + shm_membank->mem.bank->start,
> + shm_membank->mem.bank->size, gbase);
> + if ( ret )
> + return ret;
> + }
> +
> /*
> * Get the right amount of references per page, which is the number of
> * borrower domains.
> @@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct domain *d,
> * So if the borrower is created first, it will cause adding pages
> * in the P2M without reference.
> */
> - page = mfn_to_page(smfn);
> - for ( i = 0; i < nr_pages; i++ )
> + if ( shm_membank->mem.banks.meminfo )
> {
> - if ( !get_page_nr(page + i, d, nr_borrowers) )
> + meminfo = shm_membank->mem.banks.meminfo;
> + for ( i = 0; i < meminfo->nr_banks; i++ )
> {
> - printk(XENLOG_ERR
> - "Failed to add %lu references to page %"PRI_mfn".\n",
> - nr_borrowers, mfn_x(smfn) + i);
> - goto fail;
> + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> + nr_pages = PFN_DOWN(meminfo->bank[i].size);
> + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> + if ( ret )
> + goto fail;
> }
> }
> + else
> + {
> + page = mfn_to_page(
> + maddr_to_mfn(shm_membank->mem.bank->start));
> + nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT;
> + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers);
> + if ( ret )
> + return ret;
> + }
>
> return 0;
>
> fail:
> while ( --i >= 0 )
I'm aware that this line is unchanged, and I'm not trying to introduce scope creep, but I still wanted to point this out because (1) it is similar in nature to other occurrences introduced in this series and (2) the body of the loop is changed:
arch/arm/domain_build.c: In function ‘assign_shared_memory’:
arch/arm/domain_build.c:1109:17: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
1109 | while ( --i >= 0 )
| ^~
> - put_page_nr(page + i, nr_borrowers);
> + {
> + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start));
> + nr_pages = PFN_DOWN(meminfo->bank[i].size);
> + remove_shared_memory_ref(page, nr_pages, nr_borrowers);
> + }
> return ret;
> }
>
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region
2023-02-07 20:55 ` Stewart Hildebrand
@ 2023-02-14 9:56 ` Penny Zheng
0 siblings, 0 replies; 38+ messages in thread
From: Penny Zheng @ 2023-02-14 9:56 UTC (permalink / raw)
To: Stewart Hildebrand, xen-devel
Cc: Wei Chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
> -----Original Message-----
> From: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Sent: Wednesday, February 8, 2023 4:55 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>
> Subject: Re: [PATCH v1 01/13] xen/arm: re-arrange the static shared memory
> region
>
> Hi Penny,
>
Hi, Stewart
Sorry for the late response, got sidetracked by a few internal projects
> On 11/14/22 21:52, Penny Zheng wrote:
> > ...
> > diff --git a/xen/arch/arm/include/asm/setup.h
> > b/xen/arch/arm/include/asm/setup.h
> > index fdbf68aadc..2d4ae0f00a 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -50,10 +50,6 @@ struct membank {
> > paddr_t start;
> > paddr_t size;
> > enum membank_type type;
> > -#ifdef CONFIG_STATIC_SHM
> > - char shm_id[MAX_SHM_ID_LENGTH];
> > - unsigned int nr_shm_borrowers;
> > -#endif
> > };
> >
> > struct meminfo {
> > @@ -61,6 +57,17 @@ struct meminfo {
> > struct membank bank[NR_MEM_BANKS]; };
> >
> > +struct shm_membank {
> > + char shm_id[MAX_SHM_ID_LENGTH];
> > + unsigned int nr_shm_borrowers;
> > + struct membank *membank;
> > +};
> > +
> > +struct shm_meminfo {
> > + unsigned int nr_banks;
> > + struct shm_membank bank[NR_MEM_BANKS]; };
> > +
> > /*
> > * The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
> > * The purpose of the domU flag is to avoid getting confused in @@
> > -105,6 +112,7 @@ struct bootinfo {
> > struct meminfo acpi;
> > #endif
> > bool static_heap;
> > + struct shm_meminfo shm_mem;
> > };
> >
> > struct map_range_data
>
> The reduction in the sizeof struct membank is a welcome improvement, in
> my opinion, because it is multiplied by NR_MEM_BANKS, and IIRC we only
> have 32k of boot stack to play with.
>
> Before this patch:
> sizeof(struct kernel_info): 20648
> sizeof(struct meminfo): 10248
> sizeof(struct shm_meminfo): 10248
>
> When building with EXTRA_CFLAGS_XEN_CORE="Wstack-usage=4096 -Wno-
> error=stack-usage=":
Learnt! Thx!
> arch/arm/domain_build.c: In function ‘construct_domU’:
> arch/arm/domain_build.c:3747:19: warning: stack usage is 20720 bytes [-
> Wstack-usage=]
> 3747 | static int __init construct_domU(struct domain *d,
> | ^~~~~~~~~~~~~~
> arch/arm/domain_build.c: In function ‘construct_dom0’:
> arch/arm/domain_build.c:3979:19: warning: stack usage is 20688 bytes [-
> Wstack-usage=]
> 3979 | static int __init construct_dom0(struct domain *d)
> | ^~~~~~~~~~~~~~
>
>
>
> After this patch:
> sizeof(struct kernel_info): 14504
> sizeof(struct meminfo): 6152
> sizeof(struct shm_meminfo): 8200
>
> arch/arm/domain_build.c: In function ‘construct_domU’:
> arch/arm/domain_build.c:3754:19: warning: stack usage is 14576 bytes [-
> Wstack-usage=]
> 3754 | static int __init construct_domU(struct domain *d,
> | ^~~~~~~~~~~~~~
> arch/arm/domain_build.c: In function ‘construct_dom0’:
> arch/arm/domain_build.c:3986:19: warning: stack usage is 14544 bytes [-
> Wstack-usage=]
> 3986 | static int __init construct_dom0(struct domain *d)
> | ^~~~~~~~~~~~~~
>
> A later patch in this series will increase it again slightly. Note that I'm not
> expecting this series to address these particular warnings, I'm merely
> pointing out the (positive) impact of the change.
I agreed that NR_MEM_BANKS could be a large multiplier, and if we make
"struct shm_meminfo" like "struct meminfo", to have a array of NR_MEM_BANKS
items, it will make Xen binary exceed 20MB... We have discussed it here[1].
However, I'm afraid that dynamic allocation is also not a preferred option here,
where to free the data could be a problem.
So in next serie, which will come very soon, I'll introduce:
```
#define NR_SHM_BANKS 16
/*
* A static shared memory node could be banked with a statically
* configured host memory bank, or a set of arbitrary host memory
* banks allocated from heap.
*/
struct shm_meminfo {
unsigned int nr_banks;
struct membank bank[NR_SHM_BANKS];
paddr_t tot_size;
};
```
Taking your previous instructions, compiling with "EXTRA_CFLAGS_XEN_CORE="-Wstack-usage=4096 -Wno-error=stack-usage=",
boot stack usage for "construct_domU" and " construct_dom0" will be like:
"
arch/arm/domain_build.c: In function ‘construct_domU’:
arch/arm/domain_build.c:4127:19: warning: stack usage is 16800 bytes [-Wstack-usage=]
4127 | static int __init construct_domU(struct domain *d,
| ^~~~~~~~~~~~~~
arch/arm/domain_build.c: In function ‘construct_dom0’:
arch/arm/domain_build.c:4359:19: warning: stack usage is 16640 bytes [-Wstack-usage=]
4359 | static int __init construct_dom0(struct domain *d)
| ^~~~~~~~~~~~~~
"
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-01/msg00449.html
Cheers,
---
Penny Zheng
^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2023-02-14 9:56 UTC | newest]
Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-15 2:52 [PATCH V1 00/13 for 4.17-post] Follow-up static shared memory Penny Zheng
2022-11-15 2:52 ` [PATCH v1 01/13] xen/arm: re-arrange the static shared memory region Penny Zheng
2023-01-08 11:44 ` Julien Grall
2023-01-09 7:48 ` Penny Zheng
2023-01-09 10:01 ` Julien Grall
2023-02-07 20:55 ` Stewart Hildebrand
2023-02-14 9:56 ` Penny Zheng
2022-11-15 2:52 ` [PATCH v1 02/13] xen/arm: switch to use shm_membank as function parameter Penny Zheng
2022-11-15 6:58 ` Jeungwoo Yoo
2023-01-08 11:56 ` Julien Grall
2022-11-15 2:52 ` [PATCH v1 03/13] xen/arm: introduce allocate_domheap_memory and guest_physmap_memory Penny Zheng
2023-02-07 20:56 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 04/13] xen/arm: expand shm_membank for unprovided host address Penny Zheng
2023-01-08 12:13 ` Julien Grall
2023-01-08 12:54 ` Julien Grall
2022-11-15 2:52 ` [PATCH v1 05/13] xen/arm: allocate shared memory from heap when host address not provided Penny Zheng
2023-01-08 12:22 ` Julien Grall
2023-01-09 7:50 ` Penny Zheng
2023-01-09 10:07 ` Julien Grall
2023-02-07 20:57 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 06/13] xen/arm: assign shared memory to owner " Penny Zheng
2023-01-08 12:52 ` Julien Grall
2023-01-09 7:49 ` Penny Zheng
2023-01-09 10:57 ` Julien Grall
2023-01-09 11:58 ` Penny Zheng
2023-01-09 18:23 ` Julien Grall
2023-01-10 3:38 ` Penny Zheng
2023-01-10 12:47 ` Julien Grall
2023-01-18 6:09 ` Penny Zheng
2023-02-07 20:58 ` Stewart Hildebrand
2022-11-15 2:52 ` [PATCH v1 07/13] xen/arm: map shared memory to borrower " Penny Zheng
2022-11-15 2:52 ` [PATCH v1 08/13] xen/arm: use paddr_assigned to indicate whether host address is provided Penny Zheng
2022-11-15 2:52 ` [PATCH v1 09/13] xen/arm: refine docs about static shared memory Penny Zheng
2022-11-15 2:52 ` [PATCH v1 10/13] xen/arm: introduce "xen,offset" feature Penny Zheng
2022-11-15 2:52 ` [PATCH v1 11/13] xen/arm: implement "xen,offset" feature when host address provided Penny Zheng
2022-11-15 2:52 ` [PATCH v1 12/13] xen/arm: implement "xen,offset" feature when host address not provided Penny Zheng
2022-11-15 2:52 ` [PATCH v1 13/13] xen: make static shared memory supported in SUPPORT.md Penny Zheng
2023-01-08 13:18 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.