* [PATCH v2 0/7] Static shared memory followup v2 - pt2
@ 2024-05-15 14:26 Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping Luca Fancellu
` (6 more replies)
0 siblings, 7 replies; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
This serie is a partial rework of this other serie:
https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-Penny.Zheng@arm.com/
The original serie is addressing an issue of the static shared memory feature
that impacts the memory footprint of other component when the feature is
enabled, another issue impacts the device tree generation for the guests when
the feature is enabled and used and the last one is a missing feature that is
the option to have a static shared memory region that is not from the host
address space.
This serie is handling some comment on the original serie and it is splitting
the rework in two part, this first part is addressing the memory footprint issue
and the device tree generation and currently is fully merged
(https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fancellu@arm.com/),
this serie is addressing the static shared memory allocation from the Xen heap.
Luca Fancellu (5):
xen/arm: Lookup bootinfo shm bank during the mapping
xen/arm: Wrap shared memory mapping code in one function
xen/arm: Parse xen,shared-mem when host phys address is not provided
xen/arm: Rework heap page allocation outside allocate_bank_memory
xen/arm: Implement the logic for static shared memory from Xen heap
Penny Zheng (2):
xen/p2m: put reference for level 2 superpage
xen/docs: Describe static shared memory when host address is not
provided
docs/misc/arm/device-tree/booting.txt | 52 ++-
xen/arch/arm/arm32/mmu/mm.c | 11 +-
xen/arch/arm/dom0less-build.c | 4 +-
xen/arch/arm/domain_build.c | 84 +++--
xen/arch/arm/include/asm/domain_build.h | 9 +-
xen/arch/arm/mmu/p2m.c | 57 +++-
xen/arch/arm/setup.c | 14 +-
xen/arch/arm/static-shmem.c | 435 +++++++++++++++++-------
8 files changed, 482 insertions(+), 184 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
2024-05-16 13:05 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function Luca Fancellu
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
The current static shared memory code is using bootinfo banks when it
needs to find the number of borrowers, so every time assign_shared_memory
is called, the bank is searched in the bootinfo.shmem structure.
There is nothing wrong with it, however the bank can be used also to
retrieve the start address and size and also to pass less argument to
assign_shared_memory. When retrieving the information from the bootinfo
bank, it's also possible to move the checks on alignment to
process_shm_node in the early stages.
So create a new function find_shm_bank_by_id() which takes a
'struct shared_meminfo' structure and the shared memory ID, to look for a
bank with a matching ID, take the physical host address and size from the
bank, pass the bank to assign_shared_memory() removing the now unnecessary
arguments and finally remove the acquire_nr_borrower_domain() function
since now the information can be extracted from the passed bank.
Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
case of errors (unlikely), as said above, move the checks on alignment
to process_shm_node.
Drawback of this change is that now the bootinfo are used also when the
bank doesn't need to be allocated, however it will be convinient later
to use it as an argument for assign_shared_memory when dealing with
the use case where the Host physical address is not supplied by the user.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
- fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(),
swap region size check different from zero and size alignment, remove
not necessary BUGON(). (Michal)
---
xen/arch/arm/static-shmem.c | 101 +++++++++++++++++++-----------------
1 file changed, 54 insertions(+), 47 deletions(-)
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 78881dd1d3f7..0afc86c43f85 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -19,29 +19,22 @@ static void __init __maybe_unused build_assertions(void)
offsetof(struct shared_meminfo, bank)));
}
-static int __init acquire_nr_borrower_domain(struct domain *d,
- paddr_t pbase, paddr_t psize,
- unsigned long *nr_borrowers)
+static const struct membank __init *
+find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id)
{
- const struct membanks *shmem = bootinfo_get_shmem();
unsigned int bank;
- /* Iterate reserved memory to find requested shm bank. */
for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
{
- paddr_t bank_start = shmem->bank[bank].start;
- paddr_t bank_size = shmem->bank[bank].size;
-
- if ( (pbase == bank_start) && (psize == bank_size) )
+ if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
+ MAX_SHM_ID_LENGTH) == 0 )
break;
}
if ( bank == shmem->nr_banks )
- return -ENOENT;
+ return NULL;
- *nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
-
- return 0;
+ return &shmem->bank[bank];
}
/*
@@ -103,14 +96,18 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
return smfn;
}
-static int __init assign_shared_memory(struct domain *d,
- paddr_t pbase, paddr_t psize,
- paddr_t gbase)
+static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
+ const struct membank *shm_bank)
{
mfn_t smfn;
int ret = 0;
unsigned long nr_pages, nr_borrowers, i;
struct page_info *page;
+ paddr_t pbase, psize;
+
+ pbase = shm_bank->start;
+ psize = shm_bank->size;
+ nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
d, pbase, pbase + psize);
@@ -135,14 +132,6 @@ 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;
-
/*
* Instead of letting borrower domain get a page ref, we add as many
* additional reference as the number of borrowers when the owner
@@ -199,6 +188,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
dt_for_each_child_node(node, shm_node)
{
+ const struct membank *boot_shm_bank;
const struct dt_property *prop;
const __be32 *cells;
uint32_t addr_cells, size_cells;
@@ -212,6 +202,23 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
+ if ( dt_property_read_string(shm_node, "xen,shm-id", &shm_id) )
+ {
+ printk("%pd: invalid \"xen,shm-id\" property", d);
+ return -EINVAL;
+ }
+ BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
+
+ boot_shm_bank = find_shm_bank_by_id(bootinfo_get_shmem(), shm_id);
+ if ( !boot_shm_bank )
+ {
+ printk("%pd: static shared memory bank not found: '%s'", d, shm_id);
+ return -ENOENT;
+ }
+
+ pbase = boot_shm_bank->start;
+ psize = boot_shm_bank->size;
+
/*
* xen,shared-mem = <pbase, gbase, size>;
* TODO: pbase is optional.
@@ -221,20 +228,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
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_paddr(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;
- }
+ gbase = dt_read_paddr(cells + addr_cells, addr_cells);
for ( i = 0; i < PFN_DOWN(psize); i++ )
if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
@@ -251,13 +245,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
owner_dom_io = false;
- if ( dt_property_read_string(shm_node, "xen,shm-id", &shm_id) )
- {
- printk("%pd: invalid \"xen,shm-id\" property", d);
- return -EINVAL;
- }
- BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH));
-
/*
* 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
@@ -270,8 +257,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
* We found the first borrower of the region, the owner was not
* specified, so they should be assigned to dom_io.
*/
- ret = assign_shared_memory(owner_dom_io ? dom_io : d,
- pbase, psize, gbase);
+ ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
+ boot_shm_bank);
if ( ret )
return ret;
}
@@ -439,12 +426,32 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
size = dt_next_cell(size_cells, &cell);
+ if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
+ {
+ printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
+ paddr);
+ return -EINVAL;
+ }
+
+ if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
+ {
+ printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n",
+ gaddr);
+ return -EINVAL;
+ }
+
if ( !size )
{
printk("fdt: the size for static shared memory region can not be zero\n");
return -EINVAL;
}
+ if ( !IS_ALIGNED(size, PAGE_SIZE) )
+ {
+ printk("fdt: size 0x%"PRIpaddr" is not suitably aligned\n", size);
+ return -EINVAL;
+ }
+
end = paddr + size;
if ( end <= paddr )
{
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
2024-05-16 13:19 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage Luca Fancellu
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Wrap the code and logic that is calling assign_shared_memory
and map_regions_p2mt into a new function 'handle_shared_mem_bank',
it will become useful later when the code will allow the user to
don't pass the host physical address.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
- add blank line, move owner_dom_io computation inside
handle_shared_mem_bank in order to reduce args count, remove
not needed BUGON(). (Michal)
---
xen/arch/arm/static-shmem.c | 87 ++++++++++++++++++++++---------------
1 file changed, 53 insertions(+), 34 deletions(-)
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 0afc86c43f85..8a14d120690c 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
return 0;
}
+static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
+ const char *role_str,
+ const struct membank *shm_bank)
+{
+ bool owner_dom_io = true;
+ paddr_t pbase, psize;
+ int ret;
+
+ pbase = shm_bank->start;
+ psize = shm_bank->size;
+
+ /*
+ * "role" property is optional and if it is defined explicitly,
+ * then the owner domain is not the default "dom_io" domain.
+ */
+ if ( role_str != NULL )
+ owner_dom_io = false;
+
+ /*
+ * 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)) ||
+ (!owner_dom_io && strcmp(role_str, "owner") == 0) )
+ {
+ /*
+ * We found the first borrower of the region, the owner was not
+ * specified, so they should be assigned to dom_io.
+ */
+ ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
+ if ( ret )
+ return ret;
+ }
+
+ if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
+ {
+ /* Set up P2M foreign mapping for borrower domain. */
+ ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
+ _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
+ if ( ret )
+ return ret;
+ }
+
+ return 0;
+}
+
int __init process_shm(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
@@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
paddr_t gbase, pbase, psize;
int ret = 0;
unsigned int i;
- const char *role_str;
+ const char *role_str = NULL;
const char *shm_id;
- bool owner_dom_io = true;
if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
continue;
@@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
return -EINVAL;
}
- /*
- * "role" property is optional and if it is defined explicitly,
- * then the owner domain is not the default "dom_io" domain.
- */
- if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
- owner_dom_io = false;
+ /* "role" property is optional */
+ dt_property_read_string(shm_node, "role", &role_str);
- /*
- * 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)) ||
- (!owner_dom_io && strcmp(role_str, "owner") == 0) )
- {
- /*
- * We found the first borrower of the region, the owner was not
- * specified, so they should be assigned to dom_io.
- */
- ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
- boot_shm_bank);
- if ( ret )
- return ret;
- }
-
- if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
- {
- /* Set up P2M foreign mapping for borrower domain. */
- ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
- _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
- if ( ret )
- return ret;
- }
+ ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
+ if ( ret )
+ return ret;
/*
* Record static shared memory region info for later setting
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
2024-05-16 13:42 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided Luca Fancellu
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Penny Zheng
From: Penny Zheng <Penny.Zheng@arm.com>
We are doing foreign memory mapping for static shared memory, and
there is a great possibility that it could be super mapped.
But today, p2m_put_l3_page could not handle superpages.
This commits implements a new function p2m_put_l2_superpage to handle
2MB superpages, specifically for helping put extra references for
foreign superpages.
Modify relinquish_p2m_mapping as well to take into account preemption
when type is foreign memory and order is above 9 (2MB).
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Do not handle 1GB super page as there might be some issue where
a lot of calls to put_page(...) might be issued which could lead
to free memory that is a long operation.
v1:
- patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-Penny.Zheng@arm.com/
---
xen/arch/arm/mmu/p2m.c | 57 +++++++++++++++++++++++++++++-------------
1 file changed, 40 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
index 41fcca011cf4..29fdb3b87dd0 100644
--- a/xen/arch/arm/mmu/p2m.c
+++ b/xen/arch/arm/mmu/p2m.c
@@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
return rc;
}
-/*
- * Put any references on the single 4K page referenced by pte.
- * TODO: Handle superpages, for now we only take special references for leaf
- * pages (specifically foreign ones, which can't be super mapped today).
- */
-static void p2m_put_l3_page(const lpae_t pte)
+/* Put any references on the single 4K page referenced by mfn. */
+static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type)
{
- mfn_t mfn = lpae_get_mfn(pte);
-
- ASSERT(p2m_is_valid(pte));
-
/*
* TODO: Handle other p2m types
*
@@ -771,16 +763,44 @@ static void p2m_put_l3_page(const lpae_t pte)
* flush the TLBs if the page is reallocated before the end of
* this loop.
*/
- if ( p2m_is_foreign(pte.p2m.type) )
+ if ( p2m_is_foreign(type) )
{
ASSERT(mfn_valid(mfn));
put_page(mfn_to_page(mfn));
}
/* Detect the xenheap page and mark the stored GFN as invalid. */
- else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
+ else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
}
+/* Put any references on the superpage referenced by mfn. */
+static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type)
+{
+ unsigned int i;
+ unsigned int level_order = XEN_PT_LEVEL_ORDER(3);
+
+ for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
+ {
+ p2m_put_l3_page(mfn, type);
+
+ mfn = mfn_add(mfn, 1 << level_order);
+ }
+}
+
+/* Put any references on the page referenced by pte. */
+static void p2m_put_page(const lpae_t pte, unsigned int level)
+{
+ mfn_t mfn = lpae_get_mfn(pte);
+
+ ASSERT(p2m_is_valid(pte));
+
+ /* We have a second level 2M superpage */
+ if ( p2m_is_superpage(pte, level) && (level == 2) )
+ return p2m_put_l2_superpage(mfn, pte.p2m.type);
+ else if ( level == 3 )
+ return p2m_put_l3_page(mfn, pte.p2m.type);
+}
+
/* Free lpae sub-tree behind an entry */
static void p2m_free_entry(struct p2m_domain *p2m,
lpae_t entry, unsigned int level)
@@ -809,9 +829,10 @@ static void p2m_free_entry(struct p2m_domain *p2m,
#endif
p2m->stats.mappings[level]--;
- /* Nothing to do if the entry is a super-page. */
- if ( level == 3 )
- p2m_put_l3_page(entry);
+ /* TODO: Currently we don't handle 1GB super-page. */
+ if ( level >= 2 )
+ p2m_put_page(entry, level);
+
return;
}
@@ -1558,9 +1579,11 @@ int relinquish_p2m_mapping(struct domain *d)
count++;
/*
- * Arbitrarily preempt every 512 iterations.
+ * Arbitrarily preempt every 512 iterations or when type is foreign
+ * mapping and the order is above 9 (2MB).
*/
- if ( !(count % 512) && hypercall_preempt_check() )
+ if ( (!(count % 512) || (p2m_is_foreign(t) && (order > 9))) &&
+ hypercall_preempt_check() )
{
rc = -ERESTART;
break;
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
` (2 preceding siblings ...)
2024-05-15 14:26 ` [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
2024-05-20 9:34 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory Luca Fancellu
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Handle the parsing of the 'xen,shared-mem' property when the host physical
address is not provided, this commit is introducing the logic to parse it,
but the functionality is still not implemented and will be part of future
commits.
Rework the logic inside process_shm_node to check the shm_id before doing
the other checks, because it ease the logic itself, add more comment on
the logic.
Now when the host physical address is not provided, the value
INVALID_PADDR is chosen to signal this condition and it is stored as
start of the bank, due to that change also early_print_info_shmem and
init_sharedmem_pages are changed, to don't handle banks with start equal
to INVALID_PADDR.
Another change is done inside meminfo_overlap_check, to skip banks that
are starting with the start address INVALID_PADDR, that function is used
to check banks from reserved memory, shared memory and ACPI and since
the comment above the function states that wrapping around is not handled,
it's unlikely for these bank to have the start address as INVALID_PADDR.
Same change is done inside consider_modules, find_unallocated_memory and
dt_unreserved_regions functions, in order to skip banks that starts with
INVALID_PADDR from any computation.
The changes above holds because of this consideration.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
- fix comments, add parenthesis to some conditions, remove unneeded
variables, remove else branch, increment counter in the for loop,
skip INVALID_PADDR start banks from also consider_modules,
find_unallocated_memory and dt_unreserved_regions. (Michal)
---
xen/arch/arm/arm32/mmu/mm.c | 11 +++-
xen/arch/arm/domain_build.c | 5 ++
xen/arch/arm/setup.c | 14 +++-
xen/arch/arm/static-shmem.c | 125 +++++++++++++++++++++++++-----------
4 files changed, 111 insertions(+), 44 deletions(-)
diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index 23150122f7c4..3d46f32831bf 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -128,8 +128,15 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
nr += reserved_mem->nr_banks;
for ( ; i - nr < shmem->nr_banks; i++ )
{
- paddr_t r_s = shmem->bank[i - nr].start;
- paddr_t r_e = r_s + shmem->bank[i - nr].size;
+ paddr_t r_s, r_e;
+
+ r_s = shmem->bank[i - nr].start;
+
+ /* Shared memory banks can contain INVALID_PADDR as start */
+ if ( INVALID_PADDR == r_s )
+ continue;
+
+ r_e = r_s + shmem->bank[i - nr].size;
if ( s < r_e && r_s < e )
{
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f6550809cfdf..4febba69ef63 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -926,6 +926,11 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
{
start = mem_banks[i]->bank[j].start;
+
+ /* Shared memory banks can contain INVALID_PADDR as start */
+ if ( INVALID_PADDR == start )
+ continue;
+
end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
PFN_DOWN(end - 1));
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index d242674381d4..016859f6982a 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -265,8 +265,15 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
nr += reserved_mem->nr_banks;
for ( ; i - nr < shmem->nr_banks; i++ )
{
- paddr_t r_s = shmem->bank[i - nr].start;
- paddr_t r_e = r_s + shmem->bank[i - nr].size;
+ paddr_t r_s, r_e;
+
+ r_s = shmem->bank[i - nr].start;
+
+ /* Shared memory banks can contain INVALID_PADDR as start */
+ if ( INVALID_PADDR == r_s )
+ continue;
+
+ r_e = r_s + shmem->bank[i - nr].size;
if ( s < r_e && r_s < e )
{
@@ -297,7 +304,8 @@ static bool __init meminfo_overlap_check(const struct membanks *mem,
bank_start = mem->bank[i].start;
bank_end = bank_start + mem->bank[i].size;
- if ( region_end <= bank_start || region_start >= bank_end )
+ if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
+ region_start >= bank_end )
continue;
else
{
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 8a14d120690c..ddaacbc77740 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -265,6 +265,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
pbase = boot_shm_bank->start;
psize = boot_shm_bank->size;
+ if ( INVALID_PADDR == pbase )
+ {
+ printk("%pd: host physical address must be chosen by users at the moment", d);
+ return -EINVAL;
+ }
+
/*
* xen,shared-mem = <pbase, gbase, size>;
* TODO: pbase is optional.
@@ -377,7 +383,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
{
const struct fdt_property *prop, *prop_id, *prop_role;
const __be32 *cell;
- paddr_t paddr, gaddr, size, end;
+ paddr_t paddr = INVALID_PADDR;
+ paddr_t gaddr, size, end;
struct membanks *mem = bootinfo_get_shmem();
struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra();
unsigned int i;
@@ -432,24 +439,37 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
if ( !prop )
return -ENOENT;
+ cell = (const __be32 *)prop->data;
if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) )
{
- 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;
+ if ( len == dt_cells_to_size(address_cells + size_cells) )
+ device_tree_get_reg(&cell, address_cells, size_cells, &gaddr,
+ &size);
+ else
+ {
+ printk("fdt: invalid `xen,shared-mem` property.\n");
+ return -EINVAL;
+ }
}
+ else
+ {
+ device_tree_get_reg(&cell, address_cells, address_cells, &paddr,
+ &gaddr);
+ size = dt_next_cell(size_cells, &cell);
- cell = (const __be32 *)prop->data;
- device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
- size = dt_next_cell(size_cells, &cell);
+ if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
+ {
+ printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
+ paddr);
+ return -EINVAL;
+ }
- if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
- {
- printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n",
- paddr);
- return -EINVAL;
+ end = paddr + size;
+ if ( end <= paddr )
+ {
+ printk("fdt: static shared memory region %s overflow\n", shm_id);
+ return -EINVAL;
+ }
}
if ( !IS_ALIGNED(gaddr, PAGE_SIZE) )
@@ -471,39 +491,64 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
return -EINVAL;
}
- end = paddr + size;
- if ( end <= paddr )
- {
- printk("fdt: static shared memory region %s overflow\n", shm_id);
- return -EINVAL;
- }
-
for ( i = 0; i < mem->nr_banks; i++ )
{
/*
* Meet the following check:
- * 1) The shm ID matches and the region exactly match
- * 2) The shm ID doesn't match and the region doesn't overlap
- * with an existing one
+ * - 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 == mem->bank[i].start && size == mem->bank[i].size )
+ bool paddr_assigned = (INVALID_PADDR == paddr);
+
+ if ( strncmp(shm_id, shmem_extra[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
{
- if ( strncmp(shm_id, shmem_extra[i].shm_id,
- MAX_SHM_ID_LENGTH) == 0 )
+ /*
+ * Regions have same shm_id (cases):
+ * 1) physical host address is supplied:
+ * - OK: paddr is equal and size is equal (same region)
+ * - Fail: paddr doesn't match or size doesn't match (there
+ * cannot exists two shmem regions with same shm_id)
+ * 2) physical host address is NOT supplied:
+ * - OK: size is equal (same region)
+ * - Fail: size is not equal (same shm_id must identify only one
+ * region, there can't be two different regions with same
+ * shm_id)
+ */
+ bool start_match = paddr_assigned ? (paddr == mem->bank[i].start) :
+ true;
+
+ if ( start_match && (size == mem->bank[i].size) )
break;
else
{
- printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n",
+ printk("fdt: different shared memory region could not share the same shm ID %s\n",
shm_id);
return -EINVAL;
}
}
- else if ( strncmp(shm_id, shmem_extra[i].shm_id,
- MAX_SHM_ID_LENGTH) != 0 )
+
+ /*
+ * Regions have different shm_id (cases):
+ * 1) physical host address is supplied:
+ * - OK: paddr different, or size different (case where paddr
+ * is equal but psize is different are wrong, but they
+ * are handled later when checking for overlapping)
+ * - Fail: paddr equal and size equal (the same region can't be
+ * identified with different shm_id)
+ * 2) physical host address is NOT supplied:
+ * - OK: Both have different shm_id so even with same size they
+ * can exists
+ */
+ if ( !paddr_assigned || (paddr != mem->bank[i].start) ||
+ (size != mem->bank[i].size) )
continue;
else
{
- printk("fdt: different shared memory region could not share the same shm ID %s\n",
+ printk("fdt: xen,shm-id %s does not match for all the nodes using the same region\n",
shm_id);
return -EINVAL;
}
@@ -513,7 +558,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
{
if (i < mem->max_banks)
{
- if ( check_reserved_regions_overlap(paddr, size) )
+ if ( (paddr != INVALID_PADDR) &&
+ check_reserved_regions_overlap(paddr, size) )
return -EINVAL;
/* Static shared memory shall be reserved from any other use. */
@@ -583,13 +629,13 @@ void __init early_print_info_shmem(void)
{
const struct membanks *shmem = bootinfo_get_shmem();
unsigned int bank;
+ unsigned int printed = 0;
- for ( bank = 0; bank < shmem->nr_banks; bank++ )
- {
- printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", bank,
- shmem->bank[bank].start,
- shmem->bank[bank].start + shmem->bank[bank].size - 1);
- }
+ for ( bank = 0; bank < shmem->nr_banks; bank++, printed++ )
+ if ( shmem->bank[bank].start != INVALID_PADDR )
+ printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", printed,
+ shmem->bank[bank].start,
+ shmem->bank[bank].start + shmem->bank[bank].size - 1);
}
void __init init_sharedmem_pages(void)
@@ -598,7 +644,8 @@ void __init init_sharedmem_pages(void)
unsigned int bank;
for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
- init_staticmem_bank(&shmem->bank[bank]);
+ if ( shmem->bank[bank].start != INVALID_PADDR )
+ init_staticmem_bank(&shmem->bank[bank]);
}
int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
` (3 preceding siblings ...)
2024-05-15 14:26 ` [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
2024-05-20 9:42 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 7/7] xen/docs: Describe static shared memory when host address is not provided Luca Fancellu
6 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
The function allocate_bank_memory allocates pages from the heap and
maps them to the guest using guest_physmap_add_page.
As a preparation work to support static shared memory bank when the
host physical address is not provided, Xen needs to allocate memory
from the heap, so rework allocate_bank_memory moving out the page
allocation in a new function called allocate_domheap_memory.
The function allocate_domheap_memory takes a callback function and
a pointer to some extra information passed to the callback and this
function will be called for every region, until a defined size is
reached.
In order to keep allocate_bank_memory functionality, the callback
passed to allocate_domheap_memory is a wrapper for
guest_physmap_add_page.
Let allocate_domheap_memory be externally visible, in order to use
it in the future from the static shared memory module.
Take the opportunity to change the signature of allocate_bank_memory
and remove the 'struct domain' parameter, which can be retrieved from
'struct kernel_info'.
No functional changes is intended.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Reduced scope of pg var in allocate_domheap_memory, removed not
necessary BUG_ON(), changed callback to return bool and fix
comment. (Michal)
---
xen/arch/arm/dom0less-build.c | 4 +-
xen/arch/arm/domain_build.c | 79 +++++++++++++++++--------
xen/arch/arm/include/asm/domain_build.h | 9 ++-
3 files changed, 62 insertions(+), 30 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 74f053c242f4..20ddf6f8f250 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
mem->nr_banks = 0;
bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
- if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
+ if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
bank_size) )
goto fail;
bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
- if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
+ if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
bank_size) )
goto fail;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4febba69ef63..fd8babacb9df 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -417,30 +417,15 @@ static void __init allocate_memory_11(struct domain *d,
}
#ifdef CONFIG_DOM0LESS_BOOT
-bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
- gfn_t sgfn, paddr_t tot_size)
+bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
+ alloc_domheap_mem_cb cb, void *extra)
{
- struct membanks *mem = kernel_info_get_mem(kinfo);
- 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 = &mem->bank[mem->nr_banks];
- bank->start = gfn_to_gaddr(sgfn);
- bank->size = tot_size;
+ unsigned int max_order = UINT_MAX;
while ( tot_size > 0 )
{
unsigned int order = get_allocation_size(tot_size);
+ struct page_info *pg;
order = min(max_order, order);
@@ -463,17 +448,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
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 ( !cb(d, pg, order, extra) )
return false;
- }
- sgfn = gfn_add(sgfn, 1UL << order);
tot_size -= (1ULL << (PAGE_SHIFT + order));
}
+ return true;
+}
+
+static bool __init guest_map_pages(struct domain *d, struct page_info *pg,
+ unsigned int order, void *extra)
+{
+ gfn_t *sgfn = (gfn_t *)extra;
+ int res;
+
+ BUG_ON(!sgfn);
+ res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order);
+ if ( res )
+ {
+ dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+ return false;
+ }
+
+ *sgfn = gfn_add(*sgfn, 1UL << order);
+
+ return true;
+}
+
+bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
+ paddr_t tot_size)
+{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
+ struct domain *d = kinfo->d;
+ struct membank *bank;
+
+ /*
+ * 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 = &mem->bank[mem->nr_banks];
+ bank->start = gfn_to_gaddr(sgfn);
+ bank->size = tot_size;
+
+ /*
+ * Allocate pages from the heap until tot_size is zero and map them to the
+ * guest using guest_map_pages, passing the starting gfn as extra parameter
+ * for the map operation.
+ */
+ if ( !allocate_domheap_memory(d, tot_size, guest_map_pages, &sgfn) )
+ return false;
+
mem->nr_banks++;
kinfo->unassigned_mem -= bank->size;
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index 45936212ca21..e712afbc7fbf 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -5,9 +5,12 @@
#include <asm/kernel.h>
typedef __be32 gic_interrupt_t[3];
-
-bool allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
- gfn_t sgfn, paddr_t tot_size);
+typedef bool (*alloc_domheap_mem_cb)(struct domain *d, struct page_info *pg,
+ unsigned int order, void *extra);
+bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
+ alloc_domheap_mem_cb cb, void *extra);
+bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
+ paddr_t tot_size);
int construct_domain(struct domain *d, struct kernel_info *kinfo);
int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
int make_chosen_node(const struct kernel_info *kinfo);
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
` (4 preceding siblings ...)
2024-05-15 14:26 ` [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
2024-05-20 11:16 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 7/7] xen/docs: Describe static shared memory when host address is not provided Luca Fancellu
6 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
This commit implements the logic to have the static shared memory banks
from the Xen heap instead of having the host physical address passed from
the user.
When the host physical address is not supplied, the physical memory is
taken from the Xen heap using allocate_domheap_memory, the allocation
needs to occur at the first handled DT node and the allocated banks
need to be saved somewhere, so introduce the 'shm_heap_banks' static
global variable of type 'struct meminfo' that will hold the banks
allocated from the heap, its field .shmem_extra will be used to point
to the bootinfo shared memory banks .shmem_extra space, so that there
is not further allocation of memory and every bank in shm_heap_banks
can be safely identified by the shm_id to reconstruct its traceability
and if it was allocated or not.
A search into 'shm_heap_banks' will reveal if the banks were allocated
or not, in case the host address is not passed, and the callback given
to allocate_domheap_memory will store the banks in the structure and
map them to the current domain, to do that, some changes to
acquire_shared_memory_bank are made to let it differentiate if the bank
is from the heap and if it is, then assign_pages is called for every
bank.
When the bank is already allocated, for every bank allocated with the
corresponding shm_id, handle_shared_mem_bank is called and the mapping
are done.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2 changes:
- add static inline get_shmem_heap_banks(), given the changes to the
struct membanks interface. Rebase changes due to removal of
owner_dom_io arg from handle_shared_mem_bank.
Change save_map_heap_pages return type given the changes to the
allocate_domheap_memory callback type.
---
xen/arch/arm/static-shmem.c | 186 ++++++++++++++++++++++++++++++------
1 file changed, 155 insertions(+), 31 deletions(-)
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index ddaacbc77740..9c3a83042d8b 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -9,6 +9,22 @@
#include <asm/static-memory.h>
#include <asm/static-shmem.h>
+typedef struct {
+ struct domain *d;
+ paddr_t gbase;
+ const char *role_str;
+ struct shmem_membank_extra *bank_extra_info;
+} alloc_heap_pages_cb_extra;
+
+static struct meminfo __initdata shm_heap_banks = {
+ .common.max_banks = NR_MEM_BANKS
+};
+
+static inline struct membanks *get_shmem_heap_banks(void)
+{
+ return container_of(&shm_heap_banks.common, struct membanks, common);
+}
+
static void __init __maybe_unused build_assertions(void)
{
/*
@@ -64,7 +80,8 @@ static bool __init is_shm_allocated_to_domio(paddr_t pbase)
}
static mfn_t __init acquire_shared_memory_bank(struct domain *d,
- paddr_t pbase, paddr_t psize)
+ paddr_t pbase, paddr_t psize,
+ bool bank_from_heap)
{
mfn_t smfn;
unsigned long nr_pfns;
@@ -84,19 +101,31 @@ 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 ( bank_from_heap )
+ /*
+ * When host address is not provided, static shared memory is
+ * allocated from heap and shall be assigned to owner domain.
+ */
+ res = assign_pages(maddr_to_page(pbase), nr_pfns, d, 0);
+ 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;
+ printk(XENLOG_ERR "%pd: failed to %s static memory: %d.\n", d,
+ bank_from_heap ? "assign" : "acquire", res);
+ goto fail;
}
return smfn;
+
+ fail:
+ d->max_pages -= nr_pfns;
+ return INVALID_MFN;
}
static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
+ bool bank_from_heap,
const struct membank *shm_bank)
{
mfn_t smfn;
@@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
psize = shm_bank->size;
nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
- printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
- d, pbase, pbase + psize);
-
- smfn = acquire_shared_memory_bank(d, pbase, psize);
+ smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
if ( mfn_eq(smfn, INVALID_MFN) )
return -EINVAL;
@@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
const char *role_str,
+ bool bank_from_heap,
const struct membank *shm_bank)
{
bool owner_dom_io = true;
@@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
pbase = shm_bank->start;
psize = shm_bank->size;
+ printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
+ d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
+
/*
* "role" property is optional and if it is defined explicitly,
* then the owner domain is not the default "dom_io" domain.
@@ -211,7 +241,8 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
* We found the first borrower of the region, the owner was not
* specified, so they should be assigned to dom_io.
*/
- ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
+ ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
+ bank_from_heap, shm_bank);
if ( ret )
return ret;
}
@@ -228,6 +259,39 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
return 0;
}
+static bool __init save_map_heap_pages(struct domain *d, struct page_info *pg,
+ unsigned int order, void *extra)
+{
+ alloc_heap_pages_cb_extra *b_extra = (alloc_heap_pages_cb_extra *)extra;
+ int idx = shm_heap_banks.common.nr_banks;
+ int ret = -ENOSPC;
+
+ BUG_ON(!b_extra);
+
+ if ( idx < shm_heap_banks.common.max_banks )
+ {
+ shm_heap_banks.bank[idx].start = page_to_maddr(pg);
+ shm_heap_banks.bank[idx].size = (1ULL << (PAGE_SHIFT + order));
+ shm_heap_banks.bank[idx].shmem_extra = b_extra->bank_extra_info;
+ shm_heap_banks.common.nr_banks++;
+
+ ret = handle_shared_mem_bank(b_extra->d, b_extra->gbase,
+ b_extra->role_str, true,
+ &shm_heap_banks.bank[idx]);
+ if ( !ret )
+ {
+ /* Increment guest physical address for next mapping */
+ b_extra->gbase += shm_heap_banks.bank[idx].size;
+ return true;
+ }
+ }
+
+ printk("Failed to allocate static shared memory from Xen heap: (%d)\n",
+ ret);
+
+ return false;
+}
+
int __init process_shm(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
@@ -265,37 +329,97 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
pbase = boot_shm_bank->start;
psize = boot_shm_bank->size;
- if ( INVALID_PADDR == pbase )
- {
- printk("%pd: host physical address must be chosen by users at the moment", d);
- return -EINVAL;
- }
+ /* "role" property is optional */
+ dt_property_read_string(shm_node, "role", &role_str);
/*
- * xen,shared-mem = <pbase, gbase, size>;
- * TODO: pbase is optional.
+ * xen,shared-mem = <[pbase,] gbase, size>;
+ * 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;
- gbase = dt_read_paddr(cells + addr_cells, addr_cells);
- 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;
- }
+ if ( pbase != INVALID_PADDR )
+ {
+ /* guest phys address is after host phys address */
+ gbase = dt_read_paddr(cells + addr_cells, addr_cells);
+
+ 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;
+ }
+
+ /* The host physical address is supplied by the user */
+ ret = handle_shared_mem_bank(d, gbase, role_str, false,
+ boot_shm_bank);
+ if ( ret )
+ return ret;
+ }
+ else
+ {
+ /*
+ * The host physical address is not supplied by the user, so it
+ * means that the banks needs to be allocated from the Xen heap,
+ * look into the already allocated banks from the heap.
+ */
+ const struct membank *alloc_bank =
+ find_shm_bank_by_id(get_shmem_heap_banks(), shm_id);
- /* "role" property is optional */
- dt_property_read_string(shm_node, "role", &role_str);
+ /* guest phys address is right at the beginning */
+ gbase = dt_read_paddr(cells, addr_cells);
- ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
- if ( ret )
- return ret;
+ if ( !alloc_bank )
+ {
+ alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
+ boot_shm_bank->shmem_extra };
+
+ /* shm_id identified bank is not yet allocated */
+ if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages,
+ &cb_arg) )
+ {
+ printk(XENLOG_ERR
+ "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n",
+ psize >> 20);
+ return -EINVAL;
+ }
+ }
+ else
+ {
+ /* shm_id identified bank is already allocated */
+ const struct membank *end_bank =
+ &shm_heap_banks.bank[shm_heap_banks.common.nr_banks];
+ paddr_t gbase_bank = gbase;
+
+ /*
+ * Static shared memory banks that are taken from the Xen heap
+ * are allocated sequentially in shm_heap_banks, so starting
+ * from the first bank found identified by shm_id, the code can
+ * just advance by one bank at the time until it reaches the end
+ * of the array or it finds another bank NOT identified by
+ * shm_id
+ */
+ for ( ; alloc_bank < end_bank; alloc_bank++ )
+ {
+ if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
+ MAX_SHM_ID_LENGTH) != 0 )
+ break;
+
+ ret = handle_shared_mem_bank(d, gbase_bank, role_str, true,
+ alloc_bank);
+ if ( ret )
+ return ret;
+
+ /* Increment guest physical address for next mapping */
+ gbase_bank += alloc_bank->size;
+ }
+ }
+ }
/*
* Record static shared memory region info for later setting
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 7/7] xen/docs: Describe static shared memory when host address is not provided
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
` (5 preceding siblings ...)
2024-05-15 14:26 ` [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap Luca Fancellu
@ 2024-05-15 14:26 ` Luca Fancellu
6 siblings, 0 replies; 19+ messages in thread
From: Luca Fancellu @ 2024-05-15 14:26 UTC (permalink / raw
To: xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Michal Orzel, Volodymyr Babchuk, Penny Zheng
From: Penny Zheng <Penny.Zheng@arm.com>
This commit describe the new scenario where host address is not provided
in "xen,shared-mem" property and a new example is added to the page to
explain in details.
Take the occasion to fix some typos in the page.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
v2:
- Add Michal R-by
v1:
- patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-10-Penny.Zheng@arm.com/
with some changes in the commit message.
---
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 bbd955e9c2f6..ac4bad6fe5e0 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -590,7 +590,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
@@ -601,8 +601,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)
@@ -629,7 +629,7 @@ chosen {
role = "owner";
xen,shm-id = "my-shared-mem-0";
xen,shared-mem = <0x10000000 0x10000000 0x10000000>;
- }
+ };
domU1 {
compatible = "xen,domain";
@@ -640,25 +640,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>;
+ };
......
@@ -672,14 +683,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>;
+ };
......
};
@@ -699,3 +717,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.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
2024-05-15 14:26 ` [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping Luca Fancellu
@ 2024-05-16 13:05 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2024-05-16 13:05 UTC (permalink / raw
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> The current static shared memory code is using bootinfo banks when it
> needs to find the number of borrowers, so every time assign_shared_memory
> is called, the bank is searched in the bootinfo.shmem structure.
>
> There is nothing wrong with it, however the bank can be used also to
> retrieve the start address and size and also to pass less argument to
s/argument/arguments
> assign_shared_memory. When retrieving the information from the bootinfo
> bank, it's also possible to move the checks on alignment to
> process_shm_node in the early stages.
>
> So create a new function find_shm_bank_by_id() which takes a
> 'struct shared_meminfo' structure and the shared memory ID, to look for a
> bank with a matching ID, take the physical host address and size from the
> bank, pass the bank to assign_shared_memory() removing the now unnecessary
> arguments and finally remove the acquire_nr_borrower_domain() function
> since now the information can be extracted from the passed bank.
> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
> case of errors (unlikely), as said above, move the checks on alignment
> to process_shm_node.
>
> Drawback of this change is that now the bootinfo are used also when the
> bank doesn't need to be allocated, however it will be convinient later
s/convinient/convenient
> to use it as an argument for assign_shared_memory when dealing with
> the use case where the Host physical address is not supplied by the user.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2 changes:
> - fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(),
> swap region size check different from zero and size alignment, remove
> not necessary BUGON(). (Michal)
> ---
> xen/arch/arm/static-shmem.c | 101 +++++++++++++++++++-----------------
> 1 file changed, 54 insertions(+), 47 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 78881dd1d3f7..0afc86c43f85 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -19,29 +19,22 @@ static void __init __maybe_unused build_assertions(void)
> offsetof(struct shared_meminfo, bank)));
> }
>
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> - unsigned long *nr_borrowers)
> +static const struct membank __init *
> +find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id)
> {
> - const struct membanks *shmem = bootinfo_get_shmem();
> unsigned int bank;
>
> - /* Iterate reserved memory to find requested shm bank. */
> for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
> {
> - paddr_t bank_start = shmem->bank[bank].start;
> - paddr_t bank_size = shmem->bank[bank].size;
> -
> - if ( (pbase == bank_start) && (psize == bank_size) )
> + if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
Does it really need to be strncmp? You validated it a few times already.
Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function
2024-05-15 14:26 ` [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function Luca Fancellu
@ 2024-05-16 13:19 ` Michal Orzel
2024-05-16 13:24 ` Luca Fancellu
0 siblings, 1 reply; 19+ messages in thread
From: Michal Orzel @ 2024-05-16 13:19 UTC (permalink / raw
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> Wrap the code and logic that is calling assign_shared_memory
> and map_regions_p2mt into a new function 'handle_shared_mem_bank',
> it will become useful later when the code will allow the user to
> don't pass the host physical address.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2 changes:
> - add blank line, move owner_dom_io computation inside
> handle_shared_mem_bank in order to reduce args count, remove
> not needed BUGON(). (Michal)
> ---
> xen/arch/arm/static-shmem.c | 87 ++++++++++++++++++++++---------------
> 1 file changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 0afc86c43f85..8a14d120690c 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
> return 0;
> }
>
> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> + const char *role_str,
> + const struct membank *shm_bank)
> +{
> + bool owner_dom_io = true;
> + paddr_t pbase, psize;
> + int ret;
> +
> + pbase = shm_bank->start;
> + psize = shm_bank->size;
> +
> + /*
> + * "role" property is optional and if it is defined explicitly,
> + * then the owner domain is not the default "dom_io" domain.
> + */
> + if ( role_str != NULL )
> + owner_dom_io = false;
> +
> + /*
> + * 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)) ||
> + (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> + {
> + /*
> + * We found the first borrower of the region, the owner was not
> + * specified, so they should be assigned to dom_io.
> + */
> + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
> + if ( ret )
> + return ret;
> + }
> +
> + if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
> + {
> + /* Set up P2M foreign mapping for borrower domain. */
> + ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
> + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
> + if ( ret )
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> paddr_t gbase, pbase, psize;
> int ret = 0;
> unsigned int i;
> - const char *role_str;
> + const char *role_str = NULL;
> const char *shm_id;
> - bool owner_dom_io = true;
>
> if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") )
> continue;
> @@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> return -EINVAL;
> }
>
> - /*
> - * "role" property is optional and if it is defined explicitly,
> - * then the owner domain is not the default "dom_io" domain.
> - */
> - if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
> - owner_dom_io = false;
> + /* "role" property is optional */
> + dt_property_read_string(shm_node, "role", &role_str);
This now violates a MISRA rule saying that if a function returns a value, this value needs to be checked.
I think you should check if return value is non zero and if such, assign role_str NULL (thus removing such
assignment from a definition).
Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function
2024-05-16 13:19 ` Michal Orzel
@ 2024-05-16 13:24 ` Luca Fancellu
0 siblings, 0 replies; 19+ messages in thread
From: Luca Fancellu @ 2024-05-16 13:24 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Michal,
>>
>> - /*
>> - * "role" property is optional and if it is defined explicitly,
>> - * then the owner domain is not the default "dom_io" domain.
>> - */
>> - if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
>> - owner_dom_io = false;
>> + /* "role" property is optional */
>> + dt_property_read_string(shm_node, "role", &role_str);
> This now violates a MISRA rule saying that if a function returns a value, this value needs to be checked.
> I think you should check if return value is non zero and if such, assign role_str NULL (thus removing such
> assignment from a definition).
Sure, I’ll do it.
>
> Other than that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> ~Michal
Cheers,
Luca
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage
2024-05-15 14:26 ` [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage Luca Fancellu
@ 2024-05-16 13:42 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2024-05-16 13:42 UTC (permalink / raw
To: Luca Fancellu, xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> From: Penny Zheng <Penny.Zheng@arm.com>
>
> We are doing foreign memory mapping for static shared memory, and
> there is a great possibility that it could be super mapped.
> But today, p2m_put_l3_page could not handle superpages.
s/could/can
>
> This commits implements a new function p2m_put_l2_superpage to handle
> 2MB superpages, specifically for helping put extra references for
> foreign superpages.
>
> Modify relinquish_p2m_mapping as well to take into account preemption
> when type is foreign memory and order is above 9 (2MB).
I don't see the reasoning why we don't handle 1GB super pages. It would be beneficial
to include such in commit msg as well as in the code.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2:
> - Do not handle 1GB super page as there might be some issue where
> a lot of calls to put_page(...) might be issued which could lead
> to free memory that is a long operation.
> v1:
> - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-Penny.Zheng@arm.com/
> ---
> xen/arch/arm/mmu/p2m.c | 57 +++++++++++++++++++++++++++++-------------
> 1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 41fcca011cf4..29fdb3b87dd0 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn,
> return rc;
> }
>
> -/*
> - * Put any references on the single 4K page referenced by pte.
> - * TODO: Handle superpages, for now we only take special references for leaf
> - * pages (specifically foreign ones, which can't be super mapped today).
> - */
> -static void p2m_put_l3_page(const lpae_t pte)
> +/* Put any references on the single 4K page referenced by mfn. */
> +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type)
> {
> - mfn_t mfn = lpae_get_mfn(pte);
> -
> - ASSERT(p2m_is_valid(pte));
> -
> /*
> * TODO: Handle other p2m types
> *
> @@ -771,16 +763,44 @@ static void p2m_put_l3_page(const lpae_t pte)
> * flush the TLBs if the page is reallocated before the end of
> * this loop.
> */
> - if ( p2m_is_foreign(pte.p2m.type) )
> + if ( p2m_is_foreign(type) )
> {
> ASSERT(mfn_valid(mfn));
> put_page(mfn_to_page(mfn));
> }
> /* Detect the xenheap page and mark the stored GFN as invalid. */
> - else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
> + else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
> page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
> }
>
> +/* Put any references on the superpage referenced by mfn. */
> +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type)
> +{
> + unsigned int i;
> + unsigned int level_order = XEN_PT_LEVEL_ORDER(3);
If we know the third level order is always 0 no matter the page shift, does it make sense to have this variable?
> +
> + for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
> + {
> + p2m_put_l3_page(mfn, type);
> +
> + mfn = mfn_add(mfn, 1 << level_order);
> + }
> +}
> +
> +/* Put any references on the page referenced by pte. */
> +static void p2m_put_page(const lpae_t pte, unsigned int level)
> +{
> + mfn_t mfn = lpae_get_mfn(pte);
> +
> + ASSERT(p2m_is_valid(pte));
> +
> + /* We have a second level 2M superpage */
> + if ( p2m_is_superpage(pte, level) && (level == 2) )
> + return p2m_put_l2_superpage(mfn, pte.p2m.type);
> + else if ( level == 3 )
> + return p2m_put_l3_page(mfn, pte.p2m.type);
> +}
> +
> /* Free lpae sub-tree behind an entry */
> static void p2m_free_entry(struct p2m_domain *p2m,
> lpae_t entry, unsigned int level)
> @@ -809,9 +829,10 @@ static void p2m_free_entry(struct p2m_domain *p2m,
> #endif
>
> p2m->stats.mappings[level]--;
> - /* Nothing to do if the entry is a super-page. */
> - if ( level == 3 )
> - p2m_put_l3_page(entry);
> + /* TODO: Currently we don't handle 1GB super-page. */
As a future reader of the code it would be beneficial to say why.
> + if ( level >= 2 )
> + p2m_put_page(entry, level);
> +
> return;
> }
>
> @@ -1558,9 +1579,11 @@ int relinquish_p2m_mapping(struct domain *d)
>
> count++;
> /*
> - * Arbitrarily preempt every 512 iterations.
> + * Arbitrarily preempt every 512 iterations or when type is foreign
> + * mapping and the order is above 9 (2MB).
> */
> - if ( !(count % 512) && hypercall_preempt_check() )
> + if ( (!(count % 512) || (p2m_is_foreign(t) && (order > 9))) &&
Instead of (order > 9), use XEN_PT_LEVEL_ORDER(2)
> + hypercall_preempt_check() )
> {
> rc = -ERESTART;
> break;
> --
> 2.34.1
>
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
2024-05-15 14:26 ` [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided Luca Fancellu
@ 2024-05-20 9:34 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2024-05-20 9:34 UTC (permalink / raw
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> Handle the parsing of the 'xen,shared-mem' property when the host physical
> address is not provided, this commit is introducing the logic to parse it,
> but the functionality is still not implemented and will be part of future
> commits.
>
> Rework the logic inside process_shm_node to check the shm_id before doing
> the other checks, because it ease the logic itself, add more comment on
> the logic.
> Now when the host physical address is not provided, the value
> INVALID_PADDR is chosen to signal this condition and it is stored as
> start of the bank, due to that change also early_print_info_shmem and
> init_sharedmem_pages are changed, to don't handle banks with start equal
s/don't/not/
> to INVALID_PADDR.
>
> Another change is done inside meminfo_overlap_check, to skip banks that
> are starting with the start address INVALID_PADDR, that function is used
> to check banks from reserved memory, shared memory and ACPI and since
> the comment above the function states that wrapping around is not handled,
> it's unlikely for these bank to have the start address as INVALID_PADDR.
> Same change is done inside consider_modules, find_unallocated_memory and
> dt_unreserved_regions functions, in order to skip banks that starts with
> INVALID_PADDR from any computation.
> The changes above holds because of this consideration.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Note:
This patch will need rebasing since it does not apply cleanly on staging.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
2024-05-15 14:26 ` [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory Luca Fancellu
@ 2024-05-20 9:42 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2024-05-20 9:42 UTC (permalink / raw
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> The function allocate_bank_memory allocates pages from the heap and
> maps them to the guest using guest_physmap_add_page.
>
> As a preparation work to support static shared memory bank when the
> host physical address is not provided, Xen needs to allocate memory
> from the heap, so rework allocate_bank_memory moving out the page
> allocation in a new function called allocate_domheap_memory.
>
> The function allocate_domheap_memory takes a callback function and
> a pointer to some extra information passed to the callback and this
> function will be called for every region, until a defined size is
> reached.
>
> In order to keep allocate_bank_memory functionality, the callback
> passed to allocate_domheap_memory is a wrapper for
> guest_physmap_add_page.
>
> Let allocate_domheap_memory be externally visible, in order to use
> it in the future from the static shared memory module.
>
> Take the opportunity to change the signature of allocate_bank_memory
> and remove the 'struct domain' parameter, which can be retrieved from
> 'struct kernel_info'.
>
> No functional changes is intended.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
2024-05-15 14:26 ` [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap Luca Fancellu
@ 2024-05-20 11:16 ` Michal Orzel
2024-05-20 12:44 ` Luca Fancellu
0 siblings, 1 reply; 19+ messages in thread
From: Michal Orzel @ 2024-05-20 11:16 UTC (permalink / raw
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> This commit implements the logic to have the static shared memory banks
> from the Xen heap instead of having the host physical address passed from
> the user.
>
> When the host physical address is not supplied, the physical memory is
> taken from the Xen heap using allocate_domheap_memory, the allocation
> needs to occur at the first handled DT node and the allocated banks
> need to be saved somewhere, so introduce the 'shm_heap_banks' static
> global variable of type 'struct meminfo' that will hold the banks
> allocated from the heap, its field .shmem_extra will be used to point
> to the bootinfo shared memory banks .shmem_extra space, so that there
> is not further allocation of memory and every bank in shm_heap_banks
> can be safely identified by the shm_id to reconstruct its traceability
> and if it was allocated or not.
NIT for the future: it's better to split 10 lines long sentence into multiple ones.
Otherwise it reads difficult.
>
> A search into 'shm_heap_banks' will reveal if the banks were allocated
> or not, in case the host address is not passed, and the callback given
> to allocate_domheap_memory will store the banks in the structure and
> map them to the current domain, to do that, some changes to
> acquire_shared_memory_bank are made to let it differentiate if the bank
> is from the heap and if it is, then assign_pages is called for every
> bank.
>
> When the bank is already allocated, for every bank allocated with the
> corresponding shm_id, handle_shared_mem_bank is called and the mapping
> are done.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2 changes:
> - add static inline get_shmem_heap_banks(), given the changes to the
> struct membanks interface. Rebase changes due to removal of
> owner_dom_io arg from handle_shared_mem_bank.
> Change save_map_heap_pages return type given the changes to the
> allocate_domheap_memory callback type.
> ---
> xen/arch/arm/static-shmem.c | 186 ++++++++++++++++++++++++++++++------
> 1 file changed, 155 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index ddaacbc77740..9c3a83042d8b 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -9,6 +9,22 @@
> #include <asm/static-memory.h>
> #include <asm/static-shmem.h>
>
> +typedef struct {
> + struct domain *d;
> + paddr_t gbase;
> + const char *role_str;
You could swap role_str and gbase to avoid a 4B hole on arm32
> + struct shmem_membank_extra *bank_extra_info;
> +} alloc_heap_pages_cb_extra;
> +
> +static struct meminfo __initdata shm_heap_banks = {
> + .common.max_banks = NR_MEM_BANKS
Do we expect that many banks?
> +};
> +
> +static inline struct membanks *get_shmem_heap_banks(void)
> +{
> + return container_of(&shm_heap_banks.common, struct membanks, common);
> +}
> +
> static void __init __maybe_unused build_assertions(void)
> {
> /*
> @@ -64,7 +80,8 @@ static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> }
>
> static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> - paddr_t pbase, paddr_t psize)
> + paddr_t pbase, paddr_t psize,
> + bool bank_from_heap)
> {
> mfn_t smfn;
> unsigned long nr_pfns;
> @@ -84,19 +101,31 @@ 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 ( bank_from_heap )
> + /*
> + * When host address is not provided, static shared memory is
> + * allocated from heap and shall be assigned to owner domain.
> + */
> + res = assign_pages(maddr_to_page(pbase), nr_pfns, d, 0);
> + 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;
> + printk(XENLOG_ERR "%pd: failed to %s static memory: %d.\n", d,
> + bank_from_heap ? "assign" : "acquire", res);
> + goto fail;
> }
>
> return smfn;
> +
> + fail:
> + d->max_pages -= nr_pfns;
> + return INVALID_MFN;
> }
>
> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
> + bool bank_from_heap,
> const struct membank *shm_bank)
> {
> mfn_t smfn;
> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
> psize = shm_bank->size;
> nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>
> - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
> - d, pbase, pbase + psize);
> -
> - smfn = acquire_shared_memory_bank(d, pbase, psize);
> + smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
> if ( mfn_eq(smfn, INVALID_MFN) )
> return -EINVAL;
>
> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
>
> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> const char *role_str,
> + bool bank_from_heap,
> const struct membank *shm_bank)
> {
> bool owner_dom_io = true;
> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> pbase = shm_bank->start;
> psize = shm_bank->size;
>
> + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
> + d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
This looks more like a debug print since I don't expect user to want to see a machine address.
> +
> /*
> * "role" property is optional and if it is defined explicitly,
> * then the owner domain is not the default "dom_io" domain.
> @@ -211,7 +241,8 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> * We found the first borrower of the region, the owner was not
> * specified, so they should be assigned to dom_io.
> */
> - ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank);
> + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
> + bank_from_heap, shm_bank);
> if ( ret )
> return ret;
> }
> @@ -228,6 +259,39 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> return 0;
> }
>
> +static bool __init save_map_heap_pages(struct domain *d, struct page_info *pg,
> + unsigned int order, void *extra)
> +{
> + alloc_heap_pages_cb_extra *b_extra = (alloc_heap_pages_cb_extra *)extra;
> + int idx = shm_heap_banks.common.nr_banks;
> + int ret = -ENOSPC;
> +
> + BUG_ON(!b_extra);
> +
> + if ( idx < shm_heap_banks.common.max_banks )
> + {
> + shm_heap_banks.bank[idx].start = page_to_maddr(pg);
> + shm_heap_banks.bank[idx].size = (1ULL << (PAGE_SHIFT + order));
> + shm_heap_banks.bank[idx].shmem_extra = b_extra->bank_extra_info;
> + shm_heap_banks.common.nr_banks++;
> +
> + ret = handle_shared_mem_bank(b_extra->d, b_extra->gbase,
> + b_extra->role_str, true,
> + &shm_heap_banks.bank[idx]);
> + if ( !ret )
> + {
> + /* Increment guest physical address for next mapping */
> + b_extra->gbase += shm_heap_banks.bank[idx].size;
> + return true;
> + }
> + }
> +
> + printk("Failed to allocate static shared memory from Xen heap: (%d)\n",
> + ret);
> +
> + return false;
> +}
> +
> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -265,37 +329,97 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> pbase = boot_shm_bank->start;
> psize = boot_shm_bank->size;
>
> - if ( INVALID_PADDR == pbase )
> - {
> - printk("%pd: host physical address must be chosen by users at the moment", d);
> - return -EINVAL;
> - }
> + /* "role" property is optional */
> + dt_property_read_string(shm_node, "role", &role_str);
This function returns a value but you seem to ignore it
>
> /*
> - * xen,shared-mem = <pbase, gbase, size>;
> - * TODO: pbase is optional.
> + * xen,shared-mem = <[pbase,] gbase, size>;
> + * 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;
> - gbase = dt_read_paddr(cells + addr_cells, addr_cells);
>
> - 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;
> - }
> + if ( pbase != INVALID_PADDR )
> + {
> + /* guest phys address is after host phys address */
> + gbase = dt_read_paddr(cells + addr_cells, addr_cells);
> +
> + 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;
> + }
> +
> + /* The host physical address is supplied by the user */
> + ret = handle_shared_mem_bank(d, gbase, role_str, false,
> + boot_shm_bank);
> + if ( ret )
> + return ret;
> + }
> + else
> + {
> + /*
> + * The host physical address is not supplied by the user, so it
> + * means that the banks needs to be allocated from the Xen heap,
> + * look into the already allocated banks from the heap.
> + */
> + const struct membank *alloc_bank =
> + find_shm_bank_by_id(get_shmem_heap_banks(), shm_id);
>
> - /* "role" property is optional */
> - dt_property_read_string(shm_node, "role", &role_str);
> + /* guest phys address is right at the beginning */
> + gbase = dt_read_paddr(cells, addr_cells);
>
> - ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
> - if ( ret )
> - return ret;
> + if ( !alloc_bank )
> + {
> + alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
> + boot_shm_bank->shmem_extra };
> +
> + /* shm_id identified bank is not yet allocated */
> + if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages,
> + &cb_arg) )
> + {
> + printk(XENLOG_ERR
> + "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n",
Why limiting to MB?
> + psize >> 20);
> + return -EINVAL;
> + }
> + }
> + else
> + {
> + /* shm_id identified bank is already allocated */
> + const struct membank *end_bank =
> + &shm_heap_banks.bank[shm_heap_banks.common.nr_banks];
> + paddr_t gbase_bank = gbase;
> +
> + /*
> + * Static shared memory banks that are taken from the Xen heap
> + * are allocated sequentially in shm_heap_banks, so starting
> + * from the first bank found identified by shm_id, the code can
> + * just advance by one bank at the time until it reaches the end
> + * of the array or it finds another bank NOT identified by
> + * shm_id
> + */
> + for ( ; alloc_bank < end_bank; alloc_bank++ )
> + {
> + if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
> + MAX_SHM_ID_LENGTH) != 0 )
shm_id has been already validated above, hence no need for a safe version of strcmp
> + break;
> +
> + ret = handle_shared_mem_bank(d, gbase_bank, role_str, true,
> + alloc_bank);
> + if ( ret )
> + return ret;
> +
> + /* Increment guest physical address for next mapping */
> + gbase_bank += alloc_bank->size;
> + }
> + }
> + }
>
> /*
> * Record static shared memory region info for later setting
> --
> 2.34.1
>
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
2024-05-20 11:16 ` Michal Orzel
@ 2024-05-20 12:44 ` Luca Fancellu
2024-05-20 13:01 ` Michal Orzel
0 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-20 12:44 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Michal,
> On 20 May 2024, at 12:16, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Hi Luca,
>
> On 15/05/2024 16:26, Luca Fancellu wrote:
>>
>>
>> This commit implements the logic to have the static shared memory banks
>> from the Xen heap instead of having the host physical address passed from
>> the user.
>>
>> When the host physical address is not supplied, the physical memory is
>> taken from the Xen heap using allocate_domheap_memory, the allocation
>> needs to occur at the first handled DT node and the allocated banks
>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>> global variable of type 'struct meminfo' that will hold the banks
>> allocated from the heap, its field .shmem_extra will be used to point
>> to the bootinfo shared memory banks .shmem_extra space, so that there
>> is not further allocation of memory and every bank in shm_heap_banks
>> can be safely identified by the shm_id to reconstruct its traceability
>> and if it was allocated or not.
> NIT for the future: it's better to split 10 lines long sentence into multiple ones.
> Otherwise it reads difficult.
I’ll do,
>>
>> xen/arch/arm/static-shmem.c | 186 ++++++++++++++++++++++++++++++------
>> 1 file changed, 155 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>> index ddaacbc77740..9c3a83042d8b 100644
>> --- a/xen/arch/arm/static-shmem.c
>> +++ b/xen/arch/arm/static-shmem.c
>> @@ -9,6 +9,22 @@
>> #include <asm/static-memory.h>
>> #include <asm/static-shmem.h>
>>
>> +typedef struct {
>> + struct domain *d;
>> + paddr_t gbase;
>> + const char *role_str;
> You could swap role_str and gbase to avoid a 4B hole on arm32
Sure I will,
>
>> + struct shmem_membank_extra *bank_extra_info;
>> +} alloc_heap_pages_cb_extra;
>> +
>> +static struct meminfo __initdata shm_heap_banks = {
>> + .common.max_banks = NR_MEM_BANKS
> Do we expect that many banks?
Not really, but I was trying to don’t introduce another type, do you think it’s better instead to
introduce a new type only here, with a lower amount of banks?
Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ member.
>>
>> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>> + bool bank_from_heap,
>> const struct membank *shm_bank)
>> {
>> mfn_t smfn;
>> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>> psize = shm_bank->size;
>> nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>>
>> - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
>> - d, pbase, pbase + psize);
>> -
>> - smfn = acquire_shared_memory_bank(d, pbase, psize);
>> + smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
>> if ( mfn_eq(smfn, INVALID_MFN) )
>> return -EINVAL;
>>
>> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
>>
>> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>> const char *role_str,
>> + bool bank_from_heap,
>> const struct membank *shm_bank)
>> {
>> bool owner_dom_io = true;
>> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>> pbase = shm_bank->start;
>> psize = shm_bank->size;
>>
>> + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
>> + d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
> This looks more like a debug print since I don't expect user to want to see a machine address.
printk(XENLOG_DEBUG ?
>>
>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>> const struct dt_device_node *node)
>> {
>> @@ -265,37 +329,97 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>> pbase = boot_shm_bank->start;
>> psize = boot_shm_bank->size;
>>
>> - if ( INVALID_PADDR == pbase )
>> - {
>> - printk("%pd: host physical address must be chosen by users at the moment", d);
>> - return -EINVAL;
>> - }
>> + /* "role" property is optional */
>> + dt_property_read_string(shm_node, "role", &role_str);
> This function returns a value but you seem to ignore it
Sure, I’ll handle that
>>
>> - ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
>> - if ( ret )
>> - return ret;
>> + if ( !alloc_bank )
>> + {
>> + alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
>> + boot_shm_bank->shmem_extra };
>> +
>> + /* shm_id identified bank is not yet allocated */
>> + if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages,
>> + &cb_arg) )
>> + {
>> + printk(XENLOG_ERR
>> + "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n",
> Why limiting to MB?
I think I used it from domain_build.c, do you think it’s better to limit it on KB instead?
>>
>> + for ( ; alloc_bank < end_bank; alloc_bank++ )
>> + {
>> + if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
>> + MAX_SHM_ID_LENGTH) != 0 )
> shm_id has been already validated above, hence no need for a safe version of strcmp
>
I always try to use the safe version, even when redundant, I feel that if someone is copying part of the code,
at least it would copy a safe version. Anyway I will change it if it’s not desirable.
Cheers,
Luca
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
2024-05-20 12:44 ` Luca Fancellu
@ 2024-05-20 13:01 ` Michal Orzel
2024-05-20 13:11 ` Luca Fancellu
0 siblings, 1 reply; 19+ messages in thread
From: Michal Orzel @ 2024-05-20 13:01 UTC (permalink / raw
To: Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
On 20/05/2024 14:44, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>> On 20 May 2024, at 12:16, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Hi Luca,
>>
>> On 15/05/2024 16:26, Luca Fancellu wrote:
>>>
>>>
>>> This commit implements the logic to have the static shared memory banks
>>> from the Xen heap instead of having the host physical address passed from
>>> the user.
>>>
>>> When the host physical address is not supplied, the physical memory is
>>> taken from the Xen heap using allocate_domheap_memory, the allocation
>>> needs to occur at the first handled DT node and the allocated banks
>>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>>> global variable of type 'struct meminfo' that will hold the banks
>>> allocated from the heap, its field .shmem_extra will be used to point
>>> to the bootinfo shared memory banks .shmem_extra space, so that there
>>> is not further allocation of memory and every bank in shm_heap_banks
>>> can be safely identified by the shm_id to reconstruct its traceability
>>> and if it was allocated or not.
>> NIT for the future: it's better to split 10 lines long sentence into multiple ones.
>> Otherwise it reads difficult.
>
> I’ll do,
>
>>>
>>> xen/arch/arm/static-shmem.c | 186 ++++++++++++++++++++++++++++++------
>>> 1 file changed, 155 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>>> index ddaacbc77740..9c3a83042d8b 100644
>>> --- a/xen/arch/arm/static-shmem.c
>>> +++ b/xen/arch/arm/static-shmem.c
>>> @@ -9,6 +9,22 @@
>>> #include <asm/static-memory.h>
>>> #include <asm/static-shmem.h>
>>>
>>> +typedef struct {
>>> + struct domain *d;
>>> + paddr_t gbase;
>>> + const char *role_str;
>> You could swap role_str and gbase to avoid a 4B hole on arm32
>
> Sure I will,
>
>>
>>> + struct shmem_membank_extra *bank_extra_info;
>>> +} alloc_heap_pages_cb_extra;
>>> +
>>> +static struct meminfo __initdata shm_heap_banks = {
>>> + .common.max_banks = NR_MEM_BANKS
>> Do we expect that many banks?
>
> Not really, but I was trying to don’t introduce another type, do you think it’s better instead to
> introduce a new type only here, with a lower amount of banks?
I'd be ok with meminfo provided you add a reasoning behind this being meminfo and not shared_meminfo.
>
> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ member.
Would it result in a smaller footprint overall?
>
>>>
>>> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>>> + bool bank_from_heap,
>>> const struct membank *shm_bank)
>>> {
>>> mfn_t smfn;
>>> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>>> psize = shm_bank->size;
>>> nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>>>
>>> - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n",
>>> - d, pbase, pbase + psize);
>>> -
>>> - smfn = acquire_shared_memory_bank(d, pbase, psize);
>>> + smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
>>> if ( mfn_eq(smfn, INVALID_MFN) )
>>> return -EINVAL;
>>>
>>> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start,
>>>
>>> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>> const char *role_str,
>>> + bool bank_from_heap,
>>> const struct membank *shm_bank)
>>> {
>>> bool owner_dom_io = true;
>>> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>> pbase = shm_bank->start;
>>> psize = shm_bank->size;
>>>
>>> + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
>>> + d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
>> This looks more like a debug print since I don't expect user to want to see a machine address.
>
> printk(XENLOG_DEBUG ?
Why would you want user to know the machine physical address? It's a heap address.
>
>
>>>
>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>> const struct dt_device_node *node)
>>> {
>>> @@ -265,37 +329,97 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>> pbase = boot_shm_bank->start;
>>> psize = boot_shm_bank->size;
>>>
>>> - if ( INVALID_PADDR == pbase )
>>> - {
>>> - printk("%pd: host physical address must be chosen by users at the moment", d);
>>> - return -EINVAL;
>>> - }
>>> + /* "role" property is optional */
>>> + dt_property_read_string(shm_node, "role", &role_str);
>> This function returns a value but you seem to ignore it
>
> Sure, I’ll handle that
>
>>>
>>> - ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
>>> - if ( ret )
>>> - return ret;
>>> + if ( !alloc_bank )
>>> + {
>>> + alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
>>> + boot_shm_bank->shmem_extra };
>>> +
>>> + /* shm_id identified bank is not yet allocated */
>>> + if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages,
>>> + &cb_arg) )
>>> + {
>>> + printk(XENLOG_ERR
>>> + "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n",
>> Why limiting to MB?
>
> I think I used it from domain_build.c, do you think it’s better to limit it on KB instead?
User can request static shmem region of as little as 1 page.
>
>>>
>>> + for ( ; alloc_bank < end_bank; alloc_bank++ )
>>> + {
>>> + if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
>>> + MAX_SHM_ID_LENGTH) != 0 )
>> shm_id has been already validated above, hence no need for a safe version of strcmp
>>
>
> I always try to use the safe version, even when redundant, I feel that if someone is copying part of the code,
> at least it would copy a safe version. Anyway I will change it if it’s not desirable.
>
> Cheers,
> Luca
>
>
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
2024-05-20 13:01 ` Michal Orzel
@ 2024-05-20 13:11 ` Luca Fancellu
2024-05-20 13:13 ` Michal Orzel
0 siblings, 1 reply; 19+ messages in thread
From: Luca Fancellu @ 2024-05-20 13:11 UTC (permalink / raw
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
>>
>>>
>>>> + struct shmem_membank_extra *bank_extra_info;
>>>> +} alloc_heap_pages_cb_extra;
>>>> +
>>>> +static struct meminfo __initdata shm_heap_banks = {
>>>> + .common.max_banks = NR_MEM_BANKS
>>> Do we expect that many banks?
>>
>> Not really, but I was trying to don’t introduce another type, do you think it’s better instead to
>> introduce a new type only here, with a lower amount of banks?
> I'd be ok with meminfo provided you add a reasoning behind this being meminfo and not shared_meminfo.
>
>>
>> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ member.
> Would it result in a smaller footprint overall?
Well overall yes, meminfo now is 255 banks, shared_meminfo is 64 in total, even if we use 32 of them and
32 are wasted.
Otherwise, as I said, I could do something like this in this module:
static struct shared_heap_meminfo {
struct membanks_hdr common;
struct membank bank[NR_SHMEM_BANKS];
} __initdata shm_heap_banks = {
.common.max_banks = NR_SHMEM_BANKS
};
>>>>
>>>> bool owner_dom_io = true;
>>>> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>>> pbase = shm_bank->start;
>>>> psize = shm_bank->size;
>>>>
>>>> + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
>>>> + d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
>>> This looks more like a debug print since I don't expect user to want to see a machine address.
>>
>> printk(XENLOG_DEBUG ?
> Why would you want user to know the machine physical address? It's a heap address.
Oh ok your comment was about removing it, ok I don’t have strong objections to that.
>>
>>
>>>>
>>>> - ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
>>>> - if ( ret )
>>>> - return ret;
>>>> + if ( !alloc_bank )
>>>> + {
>>>> + alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
>>>> + boot_shm_bank->shmem_extra };
>>>> +
>>>> + /* shm_id identified bank is not yet allocated */
>>>> + if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages,
>>>> + &cb_arg) )
>>>> + {
>>>> + printk(XENLOG_ERR
>>>> + "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n",
>>> Why limiting to MB?
>>
>> I think I used it from domain_build.c, do you think it’s better to limit it on KB instead?
> User can request static shmem region of as little as 1 page.
Ok I’ll change to KB
>
>>
>>>>
>>>> + for ( ; alloc_bank < end_bank; alloc_bank++ )
>>>> + {
>>>> + if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
>>>> + MAX_SHM_ID_LENGTH) != 0 )
>>> shm_id has been already validated above, hence no need for a safe version of strcmp
>>>
>>
>> I always try to use the safe version, even when redundant, I feel that if someone is copying part of the code,
>> at least it would copy a safe version. Anyway I will change it if it’s not desirable.
>>
>> Cheers,
>> Luca
>>
>>
>
> ~Michal
Cheers,
Luca
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
2024-05-20 13:11 ` Luca Fancellu
@ 2024-05-20 13:13 ` Michal Orzel
0 siblings, 0 replies; 19+ messages in thread
From: Michal Orzel @ 2024-05-20 13:13 UTC (permalink / raw
To: Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
On 20/05/2024 15:11, Luca Fancellu wrote:
>
>
>>>
>>>>
>>>>> + struct shmem_membank_extra *bank_extra_info;
>>>>> +} alloc_heap_pages_cb_extra;
>>>>> +
>>>>> +static struct meminfo __initdata shm_heap_banks = {
>>>>> + .common.max_banks = NR_MEM_BANKS
>>>> Do we expect that many banks?
>>>
>>> Not really, but I was trying to don’t introduce another type, do you think it’s better instead to
>>> introduce a new type only here, with a lower amount of banks?
>> I'd be ok with meminfo provided you add a reasoning behind this being meminfo and not shared_meminfo.
>>
>>>
>>> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ member.
>> Would it result in a smaller footprint overall?
>
> Well overall yes, meminfo now is 255 banks, shared_meminfo is 64 in total, even if we use 32 of them and
> 32 are wasted.
>
> Otherwise, as I said, I could do something like this in this module:
>
> static struct shared_heap_meminfo {
> struct membanks_hdr common;
> struct membank bank[NR_SHMEM_BANKS];
> } __initdata shm_heap_banks = {
> .common.max_banks = NR_SHMEM_BANKS
> };
If that's all it takes to avoid defining unnecessarily big variable, then I'd be ok with it.
~Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-05-20 13:14 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-15 14:26 [PATCH v2 0/7] Static shared memory followup v2 - pt2 Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping Luca Fancellu
2024-05-16 13:05 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function Luca Fancellu
2024-05-16 13:19 ` Michal Orzel
2024-05-16 13:24 ` Luca Fancellu
2024-05-15 14:26 ` [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage Luca Fancellu
2024-05-16 13:42 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided Luca Fancellu
2024-05-20 9:34 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory Luca Fancellu
2024-05-20 9:42 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap Luca Fancellu
2024-05-20 11:16 ` Michal Orzel
2024-05-20 12:44 ` Luca Fancellu
2024-05-20 13:01 ` Michal Orzel
2024-05-20 13:11 ` Luca Fancellu
2024-05-20 13:13 ` Michal Orzel
2024-05-15 14:26 ` [PATCH v2 7/7] xen/docs: Describe static shared memory when host address is not provided Luca Fancellu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).