* [PATCH v2 00/13] Static shared memory followup v2 - pt1
@ 2024-04-09 11:45 Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 01/13] xen/arm: remove stale addr_cells/size_cells in assign_shared_memory Luca Fancellu
` (12 more replies)
0 siblings, 13 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Roger Pau Monné
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, there will be a following serie addressing the
last missing feature soon.
From v1:
- Add new patches, moved the patch related to the static memory helper.
Luca Fancellu (9):
xen/arm: Pass struct kernel_info parameter to make_resv_memory_node
xen/arm: Introduce a generic way to access memory bank structures
xen/arm: Conditional compilation of kernel_info.shm_mem member
xen/arm: Avoid code duplication in find_unallocated_memory
xen/arm: Avoid code duplication in check_reserved_regions_overlap
xen/arm: Introduce helper for static memory pages
xen/arm: Reduce struct membank size on static shared memory
xen/device_tree: Introduce function to merge overlapping intervals
xen/arm: List static shared memory regions as /memory nodes
Penny Zheng (4):
xen/arm: remove stale addr_cells/size_cells in assign_shared_memory
xen/arm: avoid repetitive checking in process_shm_node
xen/arm: remove shm holes from extended regions
xen/arm: fix duplicate /reserved-memory node in Dom0
xen/arch/arm/acpi/domain_build.c | 6 +-
xen/arch/arm/arm32/mmu/mm.c | 68 ++++--
xen/arch/arm/arm64/mmu/mm.c | 4 +-
xen/arch/arm/bootfdt.c | 45 ++--
xen/arch/arm/dom0less-build.c | 19 +-
xen/arch/arm/domain_build.c | 226 +++++++++++---------
xen/arch/arm/efi/efi-boot.h | 8 +-
xen/arch/arm/efi/efi-dom0.c | 13 +-
xen/arch/arm/include/asm/domain_build.h | 6 +-
xen/arch/arm/include/asm/kernel.h | 27 ++-
xen/arch/arm/include/asm/setup.h | 81 ++++++-
xen/arch/arm/include/asm/static-memory.h | 13 ++
xen/arch/arm/include/asm/static-shmem.h | 62 +++++-
xen/arch/arm/io.c | 11 +-
xen/arch/arm/kernel.c | 12 +-
xen/arch/arm/setup.c | 97 ++++++---
xen/arch/arm/static-memory.c | 35 ++-
xen/arch/arm/static-shmem.c | 260 ++++++++++++++++++-----
xen/arch/x86/extable.c | 5 +-
xen/common/device_tree.c | 140 ++++++++++++
xen/include/xen/device_tree.h | 19 ++
xen/include/xen/sort.h | 14 +-
22 files changed, 881 insertions(+), 290 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2 01/13] xen/arm: remove stale addr_cells/size_cells in assign_shared_memory
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 02/13] xen/arm: avoid repetitive checking in process_shm_node Luca Fancellu
` (11 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 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>
Function parameters {addr_cells,size_cells} are stale parameters in
assign_shared_memory, so we shall remove them.
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:
- no change
v1:
- This is this patch: https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-2-Penny.Zheng@arm.com/
---
---
xen/arch/arm/static-shmem.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 9097bc8b1511..cb268cd2edf1 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -90,7 +90,6 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d,
}
static int __init assign_shared_memory(struct domain *d,
- uint32_t addr_cells, uint32_t size_cells,
paddr_t pbase, paddr_t psize,
paddr_t gbase)
{
@@ -252,7 +251,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
* specified, so they should be assigned to dom_io.
*/
ret = assign_shared_memory(owner_dom_io ? dom_io : d,
- addr_cells, size_cells,
pbase, psize, gbase);
if ( ret )
return ret;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 02/13] xen/arm: avoid repetitive checking in process_shm_node
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 01/13] xen/arm: remove stale addr_cells/size_cells in assign_shared_memory Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node Luca Fancellu
` (10 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 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>
Putting overlap and overflow checking in the loop is causing repetitive
operation, so this commit extracts both checking outside the loop.
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:
- Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-3-Penny.Zheng@arm.com/
- use strncmp to match the branch above
- drop Michal R-by given the change
---
---
xen/arch/arm/static-shmem.c | 39 +++++++++++++++----------------------
1 file changed, 16 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index cb268cd2edf1..40a0e860c79d 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -349,7 +349,7 @@ 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;
+ paddr_t paddr, gaddr, size, end;
struct meminfo *mem = &bootinfo.reserved_mem;
unsigned int i;
int len;
@@ -422,6 +422,13 @@ 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++ )
{
/*
@@ -441,30 +448,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
return -EINVAL;
}
}
+ else if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) != 0 )
+ continue;
else
{
- paddr_t end = paddr + size;
- paddr_t bank_end = mem->bank[i].start + mem->bank[i].size;
-
- if ( (end <= paddr) || (bank_end <= mem->bank[i].start) )
- {
- printk("fdt: static shared memory region %s overflow\n", shm_id);
- return -EINVAL;
- }
-
- if ( check_reserved_regions_overlap(paddr, size) )
- return -EINVAL;
- else
- {
- if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
- continue;
- else
- {
- printk("fdt: different shared memory region could not share the same shm ID %s\n",
- shm_id);
- return -EINVAL;
- }
- }
+ printk("fdt: different shared memory region could not share the same shm ID %s\n",
+ shm_id);
+ return -EINVAL;
}
}
@@ -472,6 +462,9 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
{
if ( i < NR_MEM_BANKS )
{
+ if ( check_reserved_regions_overlap(paddr, size) )
+ return -EINVAL;
+
/* Static shared memory shall be reserved from any other use. */
safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
mem->bank[mem->nr_banks].start = paddr;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 01/13] xen/arm: remove stale addr_cells/size_cells in assign_shared_memory Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 02/13] xen/arm: avoid repetitive checking in process_shm_node Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 13:18 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures Luca Fancellu
` (9 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
The struct domain parameter is not used in make_resv_memory_node
and in its called function make_shm_memory_node, so drop it from
both function, also, take the occasion to pass directly
struct kernel_info, from which we can infer other parameter
passed to the functions and drop them as well.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- new patch
---
---
xen/arch/arm/dom0less-build.c | 3 +--
xen/arch/arm/domain_build.c | 3 +--
xen/arch/arm/include/asm/static-shmem.h | 9 ++++-----
xen/arch/arm/static-shmem.c | 16 +++++++++-------
4 files changed, 15 insertions(+), 16 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index fb63ec6fd111..0edc5357caef 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -645,8 +645,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
if ( ret )
goto err;
- ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
- &kinfo->shm_mem);
+ ret = make_resv_memory_node(kinfo, addrcells, sizecells);
if ( ret )
goto err;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 085d88671ebc..b497fb6a3090 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1770,8 +1770,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
return res;
}
- res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
- &kinfo->shm_mem);
+ res = make_resv_memory_node(kinfo, addrcells, sizecells);
if ( res )
return res;
}
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 1536ff18b895..680594b6843d 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -7,8 +7,8 @@
#ifdef CONFIG_STATIC_SHM
-int make_resv_memory_node(const struct domain *d, void *fdt, int addrcells,
- int sizecells, const struct meminfo *mem);
+int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
+ int sizecells);
int process_shm(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node);
@@ -26,9 +26,8 @@ int process_shm_node(const void *fdt, int node, uint32_t address_cells,
#else /* !CONFIG_STATIC_SHM */
-static inline int make_resv_memory_node(const struct domain *d, void *fdt,
- int addrcells, int sizecells,
- const struct meminfo *mem)
+static inline int make_resv_memory_node(const struct kernel_info *kinfo,
+ int addrcells, int sizecells)
{
return 0;
}
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 40a0e860c79d..349b85667684 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -277,10 +277,11 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
return 0;
}
-static int __init make_shm_memory_node(const struct domain *d, void *fdt,
- int addrcells, int sizecells,
- const struct meminfo *mem)
+static int __init make_shm_memory_node(const struct kernel_info *kinfo,
+ int addrcells, int sizecells)
{
+ const struct meminfo *mem = &kinfo->shm_mem;
+ void *fdt = kinfo->fdt;
unsigned int i = 0;
int res = 0;
@@ -488,10 +489,11 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
return 0;
}
-int __init make_resv_memory_node(const struct domain *d, void *fdt,
- int addrcells, int sizecells,
- const struct meminfo *mem)
+int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
+ int sizecells)
{
+ const struct meminfo *mem = &kinfo->shm_mem;
+ void *fdt = kinfo->fdt;
int res = 0;
/* Placeholder for reserved-memory\0 */
const char resvbuf[16] = "reserved-memory";
@@ -518,7 +520,7 @@ int __init make_resv_memory_node(const struct domain *d, void *fdt,
if ( res )
return res;
- res = make_shm_memory_node(d, fdt, addrcells, sizecells, mem);
+ res = make_shm_memory_node(kinfo, addrcells, sizecells);
if ( res )
return res;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (2 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 13:24 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 05/13] xen/arm: Conditional compilation of kernel_info.shm_mem member Luca Fancellu
` (8 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Currently the 'stuct meminfo' is defining a static defined array of
'struct membank' of NR_MEM_BANKS elements, some feature like
shared memory don't require such amount of memory allocation but
might want to reuse existing code to manipulate this kind of
structure that is just as 'struct meminfo' but less bulky.
For this reason introduce a generic way to access this kind of
structure using a new structure 'struct membanks', which implements
all the fields needed by a structure related to memory banks
without the need to specify at build time the size of the
'struct membank' array, using a flexible array member.
Modify 'struct meminfo' to implement the field related to the new
introduced structure, given the change all usage of this structure
are updated in this way:
- code accessing bootinfo.{mem,reserved_mem,acpi} field now uses
3 new introduced static inline helpers to access the new field
of 'struct meminfo' named 'common'.
- code accessing 'struct kernel_info *' member 'mem' now use the
new introduced macro 'kernel_info_get_mem(...)' to access the
new field of 'struct meminfo' named 'common'.
Constify pointers where needed.
Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Fixed typos in commit message and mention flexible array member
- Add static assert for struct membanks
- use static inline for the kernel_info structure instead of macro
- use xzalloc_flex_struct inside make_hypervisor_node instead of
xzalloc
- Fix trailing backslash style
v1:
- new patch
---
---
xen/arch/arm/acpi/domain_build.c | 6 +-
xen/arch/arm/arm32/mmu/mm.c | 44 +++++-----
xen/arch/arm/arm64/mmu/mm.c | 2 +-
xen/arch/arm/bootfdt.c | 39 ++++++---
xen/arch/arm/dom0less-build.c | 16 ++--
xen/arch/arm/domain_build.c | 103 +++++++++++++-----------
xen/arch/arm/efi/efi-boot.h | 8 +-
xen/arch/arm/efi/efi-dom0.c | 13 +--
xen/arch/arm/include/asm/domain_build.h | 2 +-
xen/arch/arm/include/asm/kernel.h | 17 ++++
xen/arch/arm/include/asm/setup.h | 40 ++++++++-
xen/arch/arm/kernel.c | 12 +--
xen/arch/arm/setup.c | 58 +++++++------
xen/arch/arm/static-memory.c | 27 ++++---
xen/arch/arm/static-shmem.c | 34 ++++----
15 files changed, 258 insertions(+), 163 deletions(-)
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
index b58389ce9e9f..2ce75543d004 100644
--- a/xen/arch/arm/acpi/domain_build.c
+++ b/xen/arch/arm/acpi/domain_build.c
@@ -444,14 +444,14 @@ static int __init acpi_create_fadt(struct domain *d, struct membank tbl_add[])
}
static int __init estimate_acpi_efi_size(struct domain *d,
- struct kernel_info *kinfo)
+ const struct kernel_info *kinfo)
{
size_t efi_size, acpi_size, madt_size;
u64 addr;
struct acpi_table_rsdp *rsdp_tbl;
struct acpi_table_header *table;
- efi_size = estimate_efi_size(kinfo->mem.nr_banks);
+ efi_size = estimate_efi_size(kernel_info_get_mem_const(kinfo)->nr_banks);
acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8);
acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8);
@@ -546,7 +546,7 @@ int __init prepare_acpi(struct domain *d, struct kernel_info *kinfo)
acpi_map_other_tables(d);
acpi_create_efi_system_table(d, tbl_add);
- acpi_create_efi_mmap_table(d, &kinfo->mem, tbl_add);
+ acpi_create_efi_mmap_table(d, kernel_info_get_mem(kinfo), tbl_add);
/* Map the EFI and ACPI tables to Dom0 */
rc = map_regions_p2mt(d,
diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index cb441ca87c0d..e6bb5d934c16 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -41,6 +41,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
uint32_t size, paddr_t align,
int first_mod)
{
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
const struct bootmodules *mi = &bootinfo.modules;
int i;
int nr;
@@ -99,15 +100,14 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
* possible kinds of bootmodules.
*
* When retrieving the corresponding reserved-memory addresses, we
- * need to index the bootinfo.reserved_mem bank starting from 0, and
- * only counting the reserved-memory modules. Hence, we need to use
- * i - nr.
+ * need to index the reserved_mem bank starting from 0, and only counting
+ * the reserved-memory modules. Hence, we need to use i - nr.
*/
nr += mi->nr_mods;
- for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+ for ( ; i - nr < reserved_mem->nr_banks; i++ )
{
- paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
- paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
+ paddr_t r_s = reserved_mem->bank[i - nr].start;
+ paddr_t r_e = r_s + reserved_mem->bank[i - nr].size;
if ( s < r_e && r_s < e )
{
@@ -128,17 +128,18 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
*/
static paddr_t __init fit_xenheap_in_static_heap(uint32_t size, paddr_t align)
{
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
unsigned int i;
paddr_t end = 0, aligned_start, aligned_end;
paddr_t bank_start, bank_size, bank_end;
- for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+ for ( i = 0 ; i < reserved_mem->nr_banks; i++ )
{
- if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+ if ( reserved_mem->bank[i].type != MEMBANK_STATIC_HEAP )
continue;
- bank_start = bootinfo.reserved_mem.bank[i].start;
- bank_size = bootinfo.reserved_mem.bank[i].size;
+ bank_start = reserved_mem->bank[i].start;
+ bank_size = reserved_mem->bank[i].size;
bank_end = bank_start + bank_size;
if ( bank_size < size )
@@ -161,13 +162,14 @@ static paddr_t __init fit_xenheap_in_static_heap(uint32_t size, paddr_t align)
void __init setup_mm(void)
{
+ const struct membanks *mem = bootinfo_get_mem();
paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
paddr_t static_heap_end = 0, static_heap_size = 0;
unsigned long heap_pages, xenheap_pages, domheap_pages;
unsigned int i;
const uint32_t ctr = READ_CP32(CTR);
- if ( !bootinfo.mem.nr_banks )
+ if ( !mem->nr_banks )
panic("No memory bank\n");
/* We only supports instruction caches implementing the IVIPT extension. */
@@ -176,14 +178,14 @@ void __init setup_mm(void)
init_pdx();
- ram_start = bootinfo.mem.bank[0].start;
- ram_size = bootinfo.mem.bank[0].size;
+ ram_start = mem->bank[0].start;
+ ram_size = mem->bank[0].size;
ram_end = ram_start + ram_size;
- for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
+ for ( i = 1; i < mem->nr_banks; i++ )
{
- bank_start = bootinfo.mem.bank[i].start;
- bank_size = bootinfo.mem.bank[i].size;
+ bank_start = mem->bank[i].start;
+ bank_size = mem->bank[i].size;
bank_end = bank_start + bank_size;
ram_size = ram_size + bank_size;
@@ -195,13 +197,15 @@ void __init setup_mm(void)
if ( bootinfo.static_heap )
{
- for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+
+ for ( i = 0 ; i < reserved_mem->nr_banks; i++ )
{
- if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+ if ( reserved_mem->bank[i].type != MEMBANK_STATIC_HEAP )
continue;
- bank_start = bootinfo.reserved_mem.bank[i].start;
- bank_size = bootinfo.reserved_mem.bank[i].size;
+ bank_start = reserved_mem->bank[i].start;
+ bank_size = reserved_mem->bank[i].size;
bank_end = bank_start + bank_size;
static_heap_size += bank_size;
diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
index d2651c948698..f8aaf4ac18be 100644
--- a/xen/arch/arm/arm64/mmu/mm.c
+++ b/xen/arch/arm/arm64/mmu/mm.c
@@ -194,7 +194,7 @@ static void __init setup_directmap_mappings(unsigned long base_mfn,
void __init setup_mm(void)
{
- const struct meminfo *banks = &bootinfo.mem;
+ const struct membanks *banks = bootinfo_get_mem();
paddr_t ram_start = INVALID_PADDR;
paddr_t ram_end = 0;
paddr_t ram_size = 0;
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 35dbdf3384cb..4c80962f79d4 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -17,6 +17,18 @@
#include <asm/setup.h>
#include <asm/static-shmem.h>
+static void __init __maybe_unused build_assertions(void)
+{
+ /*
+ * Check that no padding is between struct membanks "bank" flexible array
+ * member and struct meminfo "bank" member
+ */
+ BUILD_BUG_ON((offsetof(struct membanks, bank) !=
+ offsetof(struct meminfo, bank)));
+ /* Ensure "struct membanks" is 8-byte aligned */
+ BUILD_BUG_ON(alignof(struct membanks) != 8);
+}
+
static bool __init device_tree_node_is_available(const void *fdt, int node)
{
const char *status;
@@ -107,14 +119,14 @@ void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells,
static int __init device_tree_get_meminfo(const void *fdt, int node,
const char *prop_name,
u32 address_cells, u32 size_cells,
- void *data, enum membank_type type)
+ struct membanks *mem,
+ enum membank_type type)
{
const struct fdt_property *prop;
unsigned int i, banks;
const __be32 *cell;
u32 reg_cells = address_cells + size_cells;
paddr_t start, size;
- struct meminfo *mem = data;
if ( !device_tree_node_is_available(fdt, node) )
return 0;
@@ -133,10 +145,10 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
cell = (const __be32 *)prop->data;
banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
- for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
+ for ( i = 0; i < banks && mem->nr_banks < mem->max_banks; i++ )
{
device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
- if ( mem == &bootinfo.reserved_mem &&
+ if ( mem == bootinfo_get_reserved_mem() &&
check_reserved_regions_overlap(start, size) )
return -EINVAL;
/* Some DT may describe empty bank, ignore them */
@@ -231,10 +243,10 @@ int __init device_tree_for_each_node(const void *fdt, int node,
static int __init process_memory_node(const void *fdt, int node,
const char *name, int depth,
u32 address_cells, u32 size_cells,
- void *data)
+ struct membanks *mem)
{
return device_tree_get_meminfo(fdt, node, "reg", address_cells, size_cells,
- data, MEMBANK_DEFAULT);
+ mem, MEMBANK_DEFAULT);
}
static int __init process_reserved_memory_node(const void *fdt, int node,
@@ -259,7 +271,7 @@ static int __init process_reserved_memory(const void *fdt, int node,
{
return device_tree_for_each_node(fdt, node,
process_reserved_memory_node,
- &bootinfo.reserved_mem);
+ bootinfo_get_reserved_mem());
}
static void __init process_multiboot_node(const void *fdt, int node,
@@ -358,7 +370,7 @@ static int __init process_chosen_node(const void *fdt, int node,
rc = device_tree_get_meminfo(fdt, node, "xen,static-heap",
address_cells, size_cells,
- &bootinfo.reserved_mem,
+ bootinfo_get_reserved_mem(),
MEMBANK_STATIC_HEAP);
if ( rc )
return rc;
@@ -420,7 +432,7 @@ static int __init process_domain_node(const void *fdt, int node,
return 0;
return device_tree_get_meminfo(fdt, node, "xen,static-mem", address_cells,
- size_cells, &bootinfo.reserved_mem,
+ size_cells, bootinfo_get_reserved_mem(),
MEMBANK_STATIC_DOMAIN);
}
@@ -438,7 +450,7 @@ static int __init early_scan_node(const void *fdt,
if ( !efi_enabled(EFI_BOOT) &&
device_tree_node_matches(fdt, node, "memory") )
rc = process_memory_node(fdt, node, name, depth,
- address_cells, size_cells, &bootinfo.mem);
+ address_cells, size_cells, bootinfo_get_mem());
else if ( depth == 1 && !dt_node_cmp(name, "reserved-memory") )
rc = process_reserved_memory(fdt, node, name, depth,
address_cells, size_cells);
@@ -459,8 +471,8 @@ static int __init early_scan_node(const void *fdt,
static void __init early_print_info(void)
{
- struct meminfo *mi = &bootinfo.mem;
- struct meminfo *mem_resv = &bootinfo.reserved_mem;
+ const struct membanks *mi = bootinfo_get_mem();
+ const struct membanks *mem_resv = bootinfo_get_reserved_mem();
struct bootmodules *mods = &bootinfo.modules;
struct bootcmdlines *cmds = &bootinfo.cmdlines;
unsigned int i, j;
@@ -537,6 +549,7 @@ static void __init swap_memory_node(void *_a, void *_b, size_t size)
*/
size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
{
+ struct membanks *mem = bootinfo_get_mem();
int ret;
ret = fdt_check_header(fdt);
@@ -554,7 +567,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
* bank in memory first. There is no requirement that the DT will provide
* the banks sorted in ascending order. So sort them through.
*/
- sort(bootinfo.mem.bank, bootinfo.mem.nr_banks, sizeof(struct membank),
+ sort(mem->bank, mem->nr_banks, sizeof(struct membank),
cmp_memory_node, swap_memory_node);
early_print_info();
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 0edc5357caef..51cf03221d56 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -50,6 +50,7 @@ bool __init is_dom0less_mode(void)
static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
unsigned int i;
paddr_t bank_size;
@@ -57,7 +58,7 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
/* Don't want format this as PRIpaddr (16 digit hex) */
(unsigned long)(kinfo->unassigned_mem >> 20), d);
- kinfo->mem.nr_banks = 0;
+ 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),
bank_size) )
@@ -71,15 +72,15 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
if ( kinfo->unassigned_mem )
goto fail;
- for( i = 0; i < kinfo->mem.nr_banks; i++ )
+ for( i = 0; i < mem->nr_banks; i++ )
{
printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
d,
i,
- kinfo->mem.bank[i].start,
- kinfo->mem.bank[i].start + kinfo->mem.bank[i].size,
+ mem->bank[i].start,
+ mem->bank[i].start + mem->bank[i].size,
/* Don't want format this as PRIpaddr (16 digit hex) */
- (unsigned long)(kinfo->mem.bank[i].size >> 20));
+ (unsigned long)(mem->bank[i].size >> 20));
}
return;
@@ -641,7 +642,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
if ( ret )
goto err;
- ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
+ ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ kernel_info_get_mem(kinfo));
if ( ret )
goto err;
@@ -740,7 +742,7 @@ static int __init alloc_xenstore_evtchn(struct domain *d)
static int __init construct_domU(struct domain *d,
const struct dt_device_node *node)
{
- struct kernel_info kinfo = {};
+ struct kernel_info kinfo = KERNEL_INFO_INIT;
const char *dom0less_enhanced;
int rc;
u64 mem;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b497fb6a3090..57cf92668ae6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -142,6 +142,7 @@ static bool __init insert_11_bank(struct domain *d,
struct page_info *pg,
unsigned int order)
{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
unsigned int i;
int res;
mfn_t smfn;
@@ -158,9 +159,9 @@ static bool __init insert_11_bank(struct domain *d,
(unsigned long)(kinfo->unassigned_mem >> 20),
order);
- if ( kinfo->mem.nr_banks > 0 &&
+ if ( mem->nr_banks > 0 &&
size < MB(128) &&
- start + size < kinfo->mem.bank[0].start )
+ start + size < mem->bank[0].start )
{
D11PRINT("Allocation below bank 0 is too small, not using\n");
goto fail;
@@ -172,17 +173,17 @@ static bool __init insert_11_bank(struct domain *d,
kinfo->unassigned_mem -= size;
- if ( kinfo->mem.nr_banks == 0 )
+ if ( mem->nr_banks == 0 )
{
- kinfo->mem.bank[0].start = start;
- kinfo->mem.bank[0].size = size;
- kinfo->mem.nr_banks = 1;
+ mem->bank[0].start = start;
+ mem->bank[0].size = size;
+ mem->nr_banks = 1;
return true;
}
- for( i = 0; i < kinfo->mem.nr_banks; i++ )
+ for( i = 0; i < mem->nr_banks; i++ )
{
- struct membank *bank = &kinfo->mem.bank[i];
+ struct membank *bank = &mem->bank[i];
/* If possible merge new memory into the start of the bank */
if ( bank->start == start+size )
@@ -205,24 +206,24 @@ static bool __init insert_11_bank(struct domain *d,
* could have inserted the memory into/before we would already
* have done so, so this must be the right place.
*/
- if ( start + size < bank->start && kinfo->mem.nr_banks < NR_MEM_BANKS )
+ if ( start + size < bank->start && mem->nr_banks < mem->max_banks )
{
memmove(bank + 1, bank,
- sizeof(*bank) * (kinfo->mem.nr_banks - i));
- kinfo->mem.nr_banks++;
+ sizeof(*bank) * (mem->nr_banks - i));
+ mem->nr_banks++;
bank->start = start;
bank->size = size;
return true;
}
}
- if ( i == kinfo->mem.nr_banks && kinfo->mem.nr_banks < NR_MEM_BANKS )
+ if ( i == mem->nr_banks && mem->nr_banks < mem->max_banks )
{
- struct membank *bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
+ struct membank *bank = &mem->bank[mem->nr_banks];
bank->start = start;
bank->size = size;
- kinfo->mem.nr_banks++;
+ mem->nr_banks++;
return true;
}
@@ -294,6 +295,7 @@ static void __init allocate_memory_11(struct domain *d,
const unsigned int min_low_order =
get_order_from_bytes(min_t(paddr_t, dom0_mem, MB(128)));
const unsigned int min_order = get_order_from_bytes(MB(4));
+ struct membanks *mem = kernel_info_get_mem(kinfo);
struct page_info *pg;
unsigned int order = get_allocation_size(kinfo->unassigned_mem);
unsigned int i;
@@ -312,7 +314,7 @@ static void __init allocate_memory_11(struct domain *d,
/* Don't want format this as PRIpaddr (16 digit hex) */
(unsigned long)(kinfo->unassigned_mem >> 20));
- kinfo->mem.nr_banks = 0;
+ mem->nr_banks = 0;
/*
* First try and allocate the largest thing we can as low as
@@ -350,7 +352,7 @@ static void __init allocate_memory_11(struct domain *d,
* continue allocating from above the lowmem and fill in banks.
*/
order = get_allocation_size(kinfo->unassigned_mem);
- while ( kinfo->unassigned_mem && kinfo->mem.nr_banks < NR_MEM_BANKS )
+ while ( kinfo->unassigned_mem && mem->nr_banks < mem->max_banks )
{
pg = alloc_domheap_pages(d, order,
lowmem ? MEMF_bits(lowmem_bitsize) : 0);
@@ -374,7 +376,7 @@ static void __init allocate_memory_11(struct domain *d,
if ( !insert_11_bank(d, kinfo, pg, order) )
{
- if ( kinfo->mem.nr_banks == NR_MEM_BANKS )
+ if ( mem->nr_banks == mem->max_banks )
/* Nothing more we can do. */
break;
@@ -404,14 +406,14 @@ static void __init allocate_memory_11(struct domain *d,
panic("Failed to allocate requested dom0 memory. %ldMB unallocated\n",
(unsigned long)kinfo->unassigned_mem >> 20);
- for( i = 0; i < kinfo->mem.nr_banks; i++ )
+ for( i = 0; i < mem->nr_banks; i++ )
{
printk("BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
i,
- kinfo->mem.bank[i].start,
- kinfo->mem.bank[i].start + kinfo->mem.bank[i].size,
+ mem->bank[i].start,
+ mem->bank[i].start + mem->bank[i].size,
/* Don't want format this as PRIpaddr (16 digit hex) */
- (unsigned long)(kinfo->mem.bank[i].size >> 20));
+ (unsigned long)(mem->bank[i].size >> 20));
}
}
@@ -419,6 +421,7 @@ static void __init allocate_memory_11(struct domain *d,
bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
gfn_t sgfn, paddr_t tot_size)
{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
int res;
struct page_info *pg;
struct membank *bank;
@@ -432,7 +435,7 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
if ( tot_size == 0 )
return true;
- bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
+ bank = &mem->bank[mem->nr_banks];
bank->start = gfn_to_gaddr(sgfn);
bank->size = tot_size;
@@ -472,7 +475,7 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
tot_size -= (1ULL << (PAGE_SHIFT + order));
}
- kinfo->mem.nr_banks++;
+ mem->nr_banks++;
kinfo->unassigned_mem -= bank->size;
return true;
@@ -757,7 +760,7 @@ int __init domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit)
int __init make_memory_node(const struct domain *d,
void *fdt,
int addrcells, int sizecells,
- struct meminfo *mem)
+ const struct membanks *mem)
{
unsigned int i;
int res, reg_size = addrcells + sizecells;
@@ -817,12 +820,12 @@ int __init make_memory_node(const struct domain *d,
static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
void *data)
{
- struct meminfo *ext_regions = data;
+ struct membanks *ext_regions = data;
paddr_t start, size;
paddr_t s = pfn_to_paddr(s_gfn);
paddr_t e = pfn_to_paddr(e_gfn);
- if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) )
+ if ( ext_regions->nr_banks >= ext_regions->max_banks )
return 0;
/*
@@ -864,9 +867,11 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
* - grant table space
*/
static int __init find_unallocated_memory(const struct kernel_info *kinfo,
- struct meminfo *ext_regions)
+ struct membanks *ext_regions)
{
- const struct meminfo *assign_mem = &kinfo->mem;
+ const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
+ const struct membanks *mem = bootinfo_get_mem();
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
struct rangeset *unalloc_mem;
paddr_t start, end;
unsigned int i;
@@ -879,10 +884,10 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
return -ENOMEM;
/* Start with all available RAM */
- for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
+ for ( i = 0; i < mem->nr_banks; i++ )
{
- start = bootinfo.mem.bank[i].start;
- end = bootinfo.mem.bank[i].start + bootinfo.mem.bank[i].size;
+ start = mem->bank[i].start;
+ end = mem->bank[i].start + mem->bank[i].size;
res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
PFN_DOWN(end - 1));
if ( res )
@@ -894,10 +899,10 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
}
/* Remove RAM assigned to Dom0 */
- for ( i = 0; i < assign_mem->nr_banks; i++ )
+ for ( i = 0; i < kinfo_mem->nr_banks; i++ )
{
- start = assign_mem->bank[i].start;
- end = assign_mem->bank[i].start + assign_mem->bank[i].size;
+ start = kinfo_mem->bank[i].start;
+ end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
PFN_DOWN(end - 1));
if ( res )
@@ -909,11 +914,10 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
}
/* Remove reserved-memory regions */
- for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
+ for ( i = 0; i < reserved_mem->nr_banks; i++ )
{
- start = bootinfo.reserved_mem.bank[i].start;
- end = bootinfo.reserved_mem.bank[i].start +
- bootinfo.reserved_mem.bank[i].size;
+ start = reserved_mem->bank[i].start;
+ end = reserved_mem->bank[i].start + reserved_mem->bank[i].size;
res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
PFN_DOWN(end - 1));
if ( res )
@@ -991,7 +995,7 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
* - PCI aperture
*/
static int __init find_memory_holes(const struct kernel_info *kinfo,
- struct meminfo *ext_regions)
+ struct membanks *ext_regions)
{
struct dt_device_node *np;
struct rangeset *mem_holes;
@@ -1081,19 +1085,20 @@ out:
}
static int __init find_domU_holes(const struct kernel_info *kinfo,
- struct meminfo *ext_regions)
+ struct membanks *ext_regions)
{
unsigned int i;
uint64_t bankend;
const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+ const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
int res = -ENOENT;
for ( i = 0; i < GUEST_RAM_BANKS; i++ )
{
struct membank *ext_bank = &(ext_regions->bank[ext_regions->nr_banks]);
- ext_bank->start = ROUNDUP(bankbase[i] + kinfo->mem.bank[i].size, SZ_2M);
+ ext_bank->start = ROUNDUP(bankbase[i] + kinfo_mem->bank[i].size, SZ_2M);
bankend = ~0ULL >> (64 - p2m_ipa_bits);
bankend = min(bankend, bankbase[i] + banksize[i] - 1);
@@ -1121,7 +1126,7 @@ int __init make_hypervisor_node(struct domain *d,
gic_interrupt_t intr;
int res;
void *fdt = kinfo->fdt;
- struct meminfo *ext_regions = NULL;
+ struct membanks *ext_regions = NULL;
unsigned int i, nr_ext_regions;
dt_dprintk("Create hypervisor node\n");
@@ -1157,10 +1162,12 @@ int __init make_hypervisor_node(struct domain *d,
}
else
{
- ext_regions = xzalloc(struct meminfo);
+ ext_regions = xzalloc_flex_struct(struct membanks, bank, NR_MEM_BANKS);
if ( !ext_regions )
return -ENOMEM;
+ ext_regions->max_banks = NR_MEM_BANKS;
+
if ( is_domain_direct_mapped(d) )
{
if ( !is_iommu_enabled(d) )
@@ -1729,6 +1736,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
if ( node == dt_host )
{
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
int addrcells = dt_child_n_addr_cells(node);
int sizecells = dt_child_n_size_cells(node);
@@ -1754,7 +1762,8 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
if ( res )
return res;
- res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
+ res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ kernel_info_get_mem(kinfo));
if ( res )
return res;
@@ -1762,10 +1771,10 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
* Create a second memory node to store the ranges covering
* reserved-memory regions.
*/
- if ( bootinfo.reserved_mem.nr_banks > 0 )
+ if ( reserved_mem->nr_banks > 0 )
{
res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
- &bootinfo.reserved_mem);
+ reserved_mem);
if ( res )
return res;
}
@@ -2039,7 +2048,7 @@ int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
static int __init construct_dom0(struct domain *d)
{
- struct kernel_info kinfo = {};
+ struct kernel_info kinfo = KERNEL_INFO_INIT;
int rc;
/* Sanity! */
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 0cb29f90a066..199f5260229d 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -157,14 +157,14 @@ static void __init *lookup_fdt_config_table(EFI_SYSTEM_TABLE *sys_table)
return fdt;
}
-static bool __init meminfo_add_bank(struct meminfo *mem,
+static bool __init meminfo_add_bank(struct membanks *mem,
EFI_MEMORY_DESCRIPTOR *desc)
{
struct membank *bank;
paddr_t start = desc->PhysicalStart;
paddr_t size = desc->NumberOfPages * EFI_PAGE_SIZE;
- if ( mem->nr_banks >= NR_MEM_BANKS )
+ if ( mem->nr_banks >= mem->max_banks )
return false;
#ifdef CONFIG_ACPI
if ( check_reserved_regions_overlap(start, size) )
@@ -198,7 +198,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
(desc_ptr->Type == EfiBootServicesCode ||
desc_ptr->Type == EfiBootServicesData))) )
{
- if ( !meminfo_add_bank(&bootinfo.mem, desc_ptr) )
+ if ( !meminfo_add_bank(bootinfo_get_mem(), desc_ptr) )
{
PrintStr(L"Warning: All " __stringify(NR_MEM_BANKS)
" bootinfo mem banks exhausted.\r\n");
@@ -208,7 +208,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
#ifdef CONFIG_ACPI
else if ( desc_ptr->Type == EfiACPIReclaimMemory )
{
- if ( !meminfo_add_bank(&bootinfo.acpi, desc_ptr) )
+ if ( !meminfo_add_bank(bootinfo_get_acpi(), desc_ptr) )
{
PrintStr(L"Error: All " __stringify(NR_MEM_BANKS)
" acpi meminfo mem banks exhausted.\r\n");
diff --git a/xen/arch/arm/efi/efi-dom0.c b/xen/arch/arm/efi/efi-dom0.c
index aae0f979112a..baee8ab716cb 100644
--- a/xen/arch/arm/efi/efi-dom0.c
+++ b/xen/arch/arm/efi/efi-dom0.c
@@ -44,7 +44,7 @@ size_t __init estimate_efi_size(unsigned int mem_nr_banks)
unsigned int acpi_mem_nr_banks = 0;
if ( !acpi_disabled )
- acpi_mem_nr_banks = bootinfo.acpi.nr_banks;
+ acpi_mem_nr_banks = bootinfo_get_acpi()->nr_banks;
size = ROUNDUP(est_size + ect_size + fw_vendor_size, 8);
/* plus 1 for new created tables */
@@ -107,9 +107,10 @@ static void __init fill_efi_memory_descriptor(EFI_MEMORY_DESCRIPTOR *desc,
}
void __init acpi_create_efi_mmap_table(struct domain *d,
- const struct meminfo *mem,
+ const struct membanks *mem,
struct membank tbl_add[])
{
+ const struct membanks *acpi = bootinfo_get_acpi();
EFI_MEMORY_DESCRIPTOR *desc;
unsigned int i;
u8 *base_ptr;
@@ -122,10 +123,10 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
fill_efi_memory_descriptor(desc, EfiConventionalMemory,
mem->bank[i].start, mem->bank[i].size);
- for ( i = 0; i < bootinfo.acpi.nr_banks; i++, desc++ )
+ for ( i = 0; i < acpi->nr_banks; i++, desc++ )
fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
- bootinfo.acpi.bank[i].start,
- bootinfo.acpi.bank[i].size);
+ acpi->bank[i].start,
+ acpi->bank[i].size);
fill_efi_memory_descriptor(desc, EfiACPIReclaimMemory,
d->arch.efi_acpi_gpa, d->arch.efi_acpi_len);
@@ -133,7 +134,7 @@ void __init acpi_create_efi_mmap_table(struct domain *d,
tbl_add[TBL_MMAP].start = d->arch.efi_acpi_gpa
+ acpi_get_table_offset(tbl_add, TBL_MMAP);
tbl_add[TBL_MMAP].size = sizeof(EFI_MEMORY_DESCRIPTOR)
- * (mem->nr_banks + bootinfo.acpi.nr_banks + 1);
+ * (mem->nr_banks + acpi->nr_banks + 1);
}
/* Create /hypervisor/uefi node for efi properties. */
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index da9e6025f37c..a6f276cc4263 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -15,7 +15,7 @@ int make_cpus_node(const struct domain *d, void *fdt);
int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
int addrcells, int sizecells);
int make_memory_node(const struct domain *d, void *fdt, int addrcells,
- int sizecells, struct meminfo *mem);
+ int sizecells, const struct membanks *mem);
int make_psci_node(void *fdt);
int make_timer_node(const struct kernel_info *kinfo);
void evtchn_allocate(struct domain *d);
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 0a23e86c2d37..1730854d7f3c 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -78,6 +78,23 @@ struct kernel_info {
};
};
+static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo)
+{
+ return &kinfo->mem.common;
+}
+
+static inline const struct membanks *
+kernel_info_get_mem_const(const struct kernel_info *kinfo)
+{
+ return &kinfo->mem.common;
+}
+
+#define KERNEL_INFO_INIT \
+{ \
+ .mem.common.max_banks = NR_MEM_BANKS, \
+ .shm_mem.common.max_banks = NR_MEM_BANKS, \
+}
+
/*
* Probe the kernel to detemine its type and select a loader.
*
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index d15a88d2e0d1..696f55db86b1 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -56,8 +56,14 @@ struct membank {
#endif
};
-struct meminfo {
+struct membanks {
unsigned int nr_banks;
+ unsigned int max_banks;
+ struct membank bank[];
+};
+
+struct meminfo {
+ struct membanks common;
struct membank bank[NR_MEM_BANKS];
};
@@ -107,6 +113,19 @@ struct bootinfo {
bool static_heap;
};
+#ifdef CONFIG_ACPI
+#define BOOTINFO_ACPI_INIT .acpi.common.max_banks = NR_MEM_BANKS,
+#else
+#define BOOTINFO_ACPI_INIT
+#endif
+
+#define BOOTINFO_INIT \
+{ \
+ .mem.common.max_banks = NR_MEM_BANKS, \
+ .reserved_mem.common.max_banks = NR_MEM_BANKS, \
+ BOOTINFO_ACPI_INIT \
+}
+
struct map_range_data
{
struct domain *d;
@@ -122,6 +141,23 @@ extern struct bootinfo bootinfo;
extern domid_t max_init_domid;
+static inline struct membanks *bootinfo_get_mem(void)
+{
+ return &bootinfo.mem.common;
+}
+
+static inline struct membanks *bootinfo_get_reserved_mem(void)
+{
+ return &bootinfo.reserved_mem.common;
+}
+
+#ifdef CONFIG_ACPI
+static inline struct membanks *bootinfo_get_acpi(void)
+{
+ return &bootinfo.acpi.common;
+}
+#endif
+
void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
size_t estimate_efi_size(unsigned int mem_nr_banks);
@@ -130,7 +166,7 @@ void acpi_create_efi_system_table(struct domain *d,
struct membank tbl_add[]);
void acpi_create_efi_mmap_table(struct domain *d,
- const struct meminfo *mem,
+ const struct membanks *mem,
struct membank tbl_add[]);
int acpi_make_efi_nodes(void *fdt, struct membank tbl_add[]);
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index bc3e5bd6f940..674388fa11a2 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -45,13 +45,14 @@ static void __init place_modules(struct kernel_info *info,
{
/* Align DTB and initrd size to 2Mb. Linux only requires 4 byte alignment */
const struct bootmodule *mod = info->initrd_bootmodule;
+ const struct membanks *mem = kernel_info_get_mem(info);
const paddr_t initrd_len = ROUNDUP(mod ? mod->size : 0, MB(2));
const paddr_t dtb_len = ROUNDUP(fdt_totalsize(info->fdt), MB(2));
const paddr_t modsize = initrd_len + dtb_len;
/* Convenient */
- const paddr_t rambase = info->mem.bank[0].start;
- const paddr_t ramsize = info->mem.bank[0].size;
+ const paddr_t rambase = mem->bank[0].start;
+ const paddr_t ramsize = mem->bank[0].size;
const paddr_t ramend = rambase + ramsize;
const paddr_t kernsize = ROUNDUP(kernend, MB(2)) - kernbase;
const paddr_t ram128mb = rambase + MB(128);
@@ -96,11 +97,12 @@ static void __init place_modules(struct kernel_info *info,
static paddr_t __init kernel_zimage_place(struct kernel_info *info)
{
+ const struct membanks *mem = kernel_info_get_mem(info);
paddr_t load_addr;
#ifdef CONFIG_ARM_64
if ( (info->type == DOMAIN_64BIT) && (info->zimage.start == 0) )
- return info->mem.bank[0].start + info->zimage.text_offset;
+ return mem->bank[0].start + info->zimage.text_offset;
#endif
/*
@@ -113,8 +115,8 @@ static paddr_t __init kernel_zimage_place(struct kernel_info *info)
{
paddr_t load_end;
- load_end = info->mem.bank[0].start + info->mem.bank[0].size;
- load_end = MIN(info->mem.bank[0].start + MB(128), load_end);
+ load_end = mem->bank[0].start + mem->bank[0].size;
+ load_end = MIN(mem->bank[0].start + MB(128), load_end);
load_addr = load_end - info->zimage.len;
/* Align to 2MB */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 424744ad5e1a..02bd27eb0c69 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -48,7 +48,7 @@
#include <xsm/xsm.h>
#include <asm/acpi.h>
-struct bootinfo __initdata bootinfo;
+struct bootinfo __initdata bootinfo = BOOTINFO_INIT;
/*
* Sanitized version of cpuinfo containing only features available on all
@@ -207,6 +207,7 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
void (*cb)(paddr_t ps, paddr_t pe),
unsigned int first)
{
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
unsigned int i, nr;
int rc;
@@ -240,14 +241,14 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
* kinds.
*
* When retrieving the corresponding reserved-memory addresses
- * below, we need to index the bootinfo.reserved_mem bank starting
+ * below, we need to index the reserved_mem->bank starting
* from 0, and only counting the reserved-memory modules. Hence,
* we need to use i - nr.
*/
- for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+ for ( ; i - nr < reserved_mem->nr_banks; i++ )
{
- paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
- paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
+ paddr_t r_s = reserved_mem->bank[i - nr].start;
+ paddr_t r_e = r_s + reserved_mem->bank[i - nr].size;
if ( s < r_e && r_s < e )
{
@@ -264,18 +265,18 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
* TODO: '*_end' could be 0 if the bank/region is at the end of the physical
* address space. This is for now not handled as it requires more rework.
*/
-static bool __init meminfo_overlap_check(struct meminfo *meminfo,
+static bool __init meminfo_overlap_check(const struct membanks *mem,
paddr_t region_start,
paddr_t region_size)
{
paddr_t bank_start = INVALID_PADDR, bank_end = 0;
paddr_t region_end = region_start + region_size;
- unsigned int i, bank_num = meminfo->nr_banks;
+ unsigned int i, bank_num = mem->nr_banks;
for ( i = 0; i < bank_num; i++ )
{
- bank_start = meminfo->bank[i].start;
- bank_end = bank_start + meminfo->bank[i].size;
+ bank_start = mem->bank[i].start;
+ bank_end = bank_start + mem->bank[i].size;
if ( region_end <= bank_start || region_start >= bank_end )
continue;
@@ -339,8 +340,11 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
bool __init check_reserved_regions_overlap(paddr_t region_start,
paddr_t region_size)
{
- /* Check if input region is overlapping with bootinfo.reserved_mem banks */
- if ( meminfo_overlap_check(&bootinfo.reserved_mem,
+ /*
+ * Check if input region is overlapping with bootinfo_get_reserved_mem()
+ * banks
+ */
+ if ( meminfo_overlap_check(bootinfo_get_reserved_mem(),
region_start, region_size) )
return true;
@@ -351,7 +355,7 @@ bool __init check_reserved_regions_overlap(paddr_t region_start,
#ifdef CONFIG_ACPI
/* Check if input region is overlapping with ACPI EfiACPIReclaimMemory */
- if ( meminfo_overlap_check(&bootinfo.acpi, region_start, region_size) )
+ if ( meminfo_overlap_check(bootinfo_get_acpi(), region_start, region_size) )
return true;
#endif
@@ -580,6 +584,7 @@ static paddr_t __init next_module(paddr_t s, paddr_t *end)
void __init init_pdx(void)
{
+ const struct membanks *mem = bootinfo_get_mem();
paddr_t bank_start, bank_size, bank_end;
/*
@@ -592,18 +597,18 @@ void __init init_pdx(void)
uint64_t mask = pdx_init_mask(0x0);
int bank;
- for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
+ for ( bank = 0 ; bank < mem->nr_banks; bank++ )
{
- bank_start = bootinfo.mem.bank[bank].start;
- bank_size = bootinfo.mem.bank[bank].size;
+ bank_start = mem->bank[bank].start;
+ bank_size = mem->bank[bank].size;
mask |= bank_start | pdx_region_mask(bank_start, bank_size);
}
- for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
+ for ( bank = 0 ; bank < mem->nr_banks; bank++ )
{
- bank_start = bootinfo.mem.bank[bank].start;
- bank_size = bootinfo.mem.bank[bank].size;
+ bank_start = mem->bank[bank].start;
+ bank_size = mem->bank[bank].size;
if (~mask & pdx_region_mask(bank_start, bank_size))
mask = 0;
@@ -611,10 +616,10 @@ void __init init_pdx(void)
pfn_pdx_hole_setup(mask >> PAGE_SHIFT);
- for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
+ for ( bank = 0 ; bank < mem->nr_banks; bank++ )
{
- bank_start = bootinfo.mem.bank[bank].start;
- bank_size = bootinfo.mem.bank[bank].size;
+ bank_start = mem->bank[bank].start;
+ bank_size = mem->bank[bank].size;
bank_end = bank_start + bank_size;
set_pdx_range(paddr_to_pfn(bank_start),
@@ -636,18 +641,19 @@ void __init init_pdx(void)
void __init populate_boot_allocator(void)
{
unsigned int i;
- const struct meminfo *banks = &bootinfo.mem;
+ const struct membanks *banks = bootinfo_get_mem();
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
paddr_t s, e;
if ( bootinfo.static_heap )
{
- for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
+ for ( i = 0 ; i < reserved_mem->nr_banks; i++ )
{
- if ( bootinfo.reserved_mem.bank[i].type != MEMBANK_STATIC_HEAP )
+ if ( reserved_mem->bank[i].type != MEMBANK_STATIC_HEAP )
continue;
- s = bootinfo.reserved_mem.bank[i].start;
- e = s + bootinfo.reserved_mem.bank[i].size;
+ s = reserved_mem->bank[i].start;
+ e = s + reserved_mem->bank[i].size;
#ifdef CONFIG_ARM_32
/* Avoid the xenheap, note that the xenheap cannot across a bank */
if ( s <= mfn_to_maddr(directmap_mfn_start) &&
diff --git a/xen/arch/arm/static-memory.c b/xen/arch/arm/static-memory.c
index cffbab7241b7..34bd12696a53 100644
--- a/xen/arch/arm/static-memory.c
+++ b/xen/arch/arm/static-memory.c
@@ -85,6 +85,7 @@ static int __init parse_static_mem_prop(const struct dt_device_node *node,
void __init allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
u32 addr_cells, size_cells, reg_cells;
unsigned int nr_banks, gbank, bank = 0;
const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
@@ -106,7 +107,7 @@ void __init allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
*/
gbank = 0;
gsize = ramsize[gbank];
- kinfo->mem.bank[gbank].start = rambase[gbank];
+ mem->bank[gbank].start = rambase[gbank];
nr_banks = length / (reg_cells * sizeof (u32));
for ( ; bank < nr_banks; bank++ )
@@ -122,7 +123,7 @@ void __init allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
while ( 1 )
{
/* Map as much as possible the static range to the guest bank */
- if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn,
+ if ( !append_static_memory_to_bank(d, &mem->bank[gbank], smfn,
min(psize, gsize)) )
goto fail;
@@ -153,14 +154,14 @@ void __init allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
/* Update to the next guest bank. */
gbank++;
gsize = ramsize[gbank];
- kinfo->mem.bank[gbank].start = rambase[gbank];
+ mem->bank[gbank].start = rambase[gbank];
}
}
tot_size += psize;
}
- kinfo->mem.nr_banks = ++gbank;
+ mem->nr_banks = ++gbank;
kinfo->unassigned_mem -= tot_size;
/*
@@ -190,6 +191,7 @@ void __init allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
void __init assign_static_memory_11(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
u32 addr_cells, size_cells, reg_cells;
unsigned int nr_banks, bank = 0;
const __be32 *cell;
@@ -206,7 +208,7 @@ void __init assign_static_memory_11(struct domain *d, struct kernel_info *kinfo,
reg_cells = addr_cells + size_cells;
nr_banks = length / (reg_cells * sizeof(u32));
- if ( nr_banks > NR_MEM_BANKS )
+ if ( nr_banks > mem->max_banks )
{
printk(XENLOG_ERR
"%pd: exceed max number of supported guest memory banks.\n", d);
@@ -224,15 +226,15 @@ void __init assign_static_memory_11(struct domain *d, struct kernel_info *kinfo,
d, bank, pbase, pbase + psize);
/* One guest memory bank is matched with one physical memory bank. */
- kinfo->mem.bank[bank].start = pbase;
- if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[bank],
+ mem->bank[bank].start = pbase;
+ if ( !append_static_memory_to_bank(d, &mem->bank[bank],
smfn, psize) )
goto fail;
kinfo->unassigned_mem -= psize;
}
- kinfo->mem.nr_banks = nr_banks;
+ mem->nr_banks = nr_banks;
/*
* The property 'memory' should match the amount of memory given to
@@ -257,14 +259,15 @@ void __init assign_static_memory_11(struct domain *d, struct kernel_info *kinfo,
/* Static memory initialization */
void __init init_staticmem_pages(void)
{
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
unsigned int bank;
- for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
+ for ( bank = 0 ; bank < reserved_mem->nr_banks; bank++ )
{
- if ( bootinfo.reserved_mem.bank[bank].type == MEMBANK_STATIC_DOMAIN )
+ if ( reserved_mem->bank[bank].type == MEMBANK_STATIC_DOMAIN )
{
- mfn_t bank_start = _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
- unsigned long bank_pages = PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
+ mfn_t bank_start = _mfn(PFN_UP(reserved_mem->bank[bank].start));
+ unsigned long bank_pages = PFN_DOWN(reserved_mem->bank[bank].size);
mfn_t bank_end = mfn_add(bank_start, bank_pages);
if ( mfn_x(bank_end) <= mfn_x(bank_start) )
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 349b85667684..11f3c0da81a4 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -10,22 +10,23 @@ static int __init acquire_nr_borrower_domain(struct domain *d,
paddr_t pbase, paddr_t psize,
unsigned long *nr_borrowers)
{
+ const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
unsigned int bank;
/* Iterate reserved memory to find requested shm bank. */
- for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
+ for ( bank = 0 ; bank < reserved_mem->nr_banks; bank++ )
{
- paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
- paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
+ paddr_t bank_start = reserved_mem->bank[bank].start;
+ paddr_t bank_size = reserved_mem->bank[bank].size;
if ( (pbase == bank_start) && (psize == bank_size) )
break;
}
- if ( bank == bootinfo.reserved_mem.nr_banks )
+ if ( bank == reserved_mem->nr_banks )
return -ENOENT;
- *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
+ *nr_borrowers = reserved_mem->bank[bank].nr_shm_borrowers;
return 0;
}
@@ -157,17 +158,17 @@ static int __init assign_shared_memory(struct domain *d,
return ret;
}
-static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
+static int __init append_shm_bank_to_domain(struct membanks *shm_mem,
paddr_t start, paddr_t size,
const char *shm_id)
{
- if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
+ if ( shm_mem->nr_banks >= shm_mem->max_banks )
return -ENOMEM;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
- kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
- safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
- kinfo->shm_mem.nr_banks++;
+ shm_mem->bank[shm_mem->nr_banks].start = start;
+ shm_mem->bank[shm_mem->nr_banks].size = size;
+ safe_strcpy(shm_mem->bank[shm_mem->nr_banks].shm_id, shm_id);
+ shm_mem->nr_banks++;
return 0;
}
@@ -269,7 +270,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
* Record static shared memory region info for later setting
* up shm-node in guest device tree.
*/
- ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id);
+ ret = append_shm_bank_to_domain(&kinfo->shm_mem.common, gbase, psize,
+ shm_id);
if ( ret )
return ret;
}
@@ -280,7 +282,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
static int __init make_shm_memory_node(const struct kernel_info *kinfo,
int addrcells, int sizecells)
{
- const struct meminfo *mem = &kinfo->shm_mem;
+ const struct membanks *mem = &kinfo->shm_mem.common;
void *fdt = kinfo->fdt;
unsigned int i = 0;
int res = 0;
@@ -351,7 +353,7 @@ 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;
- struct meminfo *mem = &bootinfo.reserved_mem;
+ struct membanks *mem = bootinfo_get_reserved_mem();
unsigned int i;
int len;
bool owner = false;
@@ -461,7 +463,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
if ( i == mem->nr_banks )
{
- if ( i < NR_MEM_BANKS )
+ if (i < mem->max_banks)
{
if ( check_reserved_regions_overlap(paddr, size) )
return -EINVAL;
@@ -492,7 +494,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
int sizecells)
{
- const struct meminfo *mem = &kinfo->shm_mem;
+ const struct membanks *mem = &kinfo->shm_mem.common;
void *fdt = kinfo->fdt;
int res = 0;
/* Placeholder for reserved-memory\0 */
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 05/13] xen/arm: Conditional compilation of kernel_info.shm_mem member
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (3 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory Luca Fancellu
` (7 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
The user of shm_mem member of the 'struct kernel_info' is only
the code managing the static shared memory feature, which can be
compiled out using CONFIG_STATIC_SHM, so in case the feature is
not requested, that member won't be used and will waste memory
space.
To address this issue, protect the member with the Kconfig parameter
and modify the signature of the only function using it to remove
any reference to the member from outside the static-shmem module.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
v2:
- add Michal R-by
- Removed the signature modification of make_resv_memory_node from
this patch
v1:
- new patch
---
---
xen/arch/arm/include/asm/kernel.h | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 1730854d7f3c..85e1625cc989 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -39,7 +39,9 @@ struct kernel_info {
void *fdt; /* flat device tree */
paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
struct meminfo mem;
+#ifdef CONFIG_STATIC_SHM
struct meminfo shm_mem;
+#endif
/* kernel entry point */
paddr_t entry;
@@ -89,10 +91,16 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
return &kinfo->mem.common;
}
+#ifdef CONFIG_STATIC_SHM
+#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_MEM_BANKS,
+#else
+#define KERNEL_INFO_SHM_MEM_INIT
+#endif
+
#define KERNEL_INFO_INIT \
{ \
.mem.common.max_banks = NR_MEM_BANKS, \
- .shm_mem.common.max_banks = NR_MEM_BANKS, \
+ KERNEL_INFO_SHM_MEM_INIT \
}
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (4 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 05/13] xen/arm: Conditional compilation of kernel_info.shm_mem member Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 13:38 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap Luca Fancellu
` (6 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
The function find_unallocated_memory is using the same code to
loop through 3 structure of the same type, in order to avoid
code duplication, rework the code to have only one loop that
goes through all the structures.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Add comment in the loop inside find_unallocated_memory to
improve readability
v1:
- new patch
---
---
xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------
1 file changed, 25 insertions(+), 45 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 57cf92668ae6..269aaff4d067 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
static int __init find_unallocated_memory(const struct kernel_info *kinfo,
struct membanks *ext_regions)
{
- const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
- const struct membanks *mem = bootinfo_get_mem();
- const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+ const struct membanks *mem_banks[] = {
+ bootinfo_get_mem(),
+ kernel_info_get_mem_const(kinfo),
+ bootinfo_get_reserved_mem(),
+ };
struct rangeset *unalloc_mem;
paddr_t start, end;
- unsigned int i;
+ unsigned int i, j;
int res;
dt_dprintk("Find unallocated memory for extended regions\n");
@@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
if ( !unalloc_mem )
return -ENOMEM;
- /* Start with all available RAM */
- for ( i = 0; i < mem->nr_banks; i++ )
- {
- start = mem->bank[i].start;
- end = mem->bank[i].start + mem->bank[i].size;
- res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
- PFN_DOWN(end - 1));
- if ( res )
+ /*
+ * Exclude the following regions, in order:
+ * 1) Start with all available RAM
+ * 2) Remove RAM assigned to Dom0
+ * 3) Remove reserved memory
+ * The order comes from the initialization of the variable "mem_banks"
+ * above
+ */
+ for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
+ for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
{
- printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
- start, end);
- goto out;
- }
- }
-
- /* Remove RAM assigned to Dom0 */
- for ( i = 0; i < kinfo_mem->nr_banks; i++ )
- {
- start = kinfo_mem->bank[i].start;
- end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
- res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
+ start = mem_banks[i]->bank[j].start;
+ end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
+ res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
PFN_DOWN(end - 1));
- if ( res )
- {
- printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
- start, end);
- goto out;
- }
- }
-
- /* Remove reserved-memory regions */
- for ( i = 0; i < reserved_mem->nr_banks; i++ )
- {
- start = reserved_mem->bank[i].start;
- end = reserved_mem->bank[i].start + reserved_mem->bank[i].size;
- res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
- PFN_DOWN(end - 1));
- if ( res )
- {
- printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n",
- start, end);
- goto out;
+ if ( res )
+ {
+ printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
+ start, end);
+ goto out;
+ }
}
- }
/* Remove grant table region */
if ( kinfo->gnttab_size )
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (5 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-10 7:40 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 08/13] xen/arm: Introduce helper for static memory pages Luca Fancellu
` (5 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
The function check_reserved_regions_overlap is calling
'meminfo_overlap_check' on the same type of structure, this code
can be written in a way to avoid code duplication, so rework the
function to do that.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- no changes
v1:
- new patch
---
---
xen/arch/arm/setup.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 02bd27eb0c69..cc719d508d63 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -340,25 +340,27 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
bool __init check_reserved_regions_overlap(paddr_t region_start,
paddr_t region_size)
{
+ const struct membanks *mem_banks[] = {
+ bootinfo_get_reserved_mem(),
+#ifdef CONFIG_ACPI
+ bootinfo_get_acpi(),
+#endif
+ };
+ unsigned int i;
+
/*
- * Check if input region is overlapping with bootinfo_get_reserved_mem()
- * banks
+ * Check if input region is overlapping with reserved memory banks or
+ * ACPI EfiACPIReclaimMemory (when ACPI feature is enabled)
*/
- if ( meminfo_overlap_check(bootinfo_get_reserved_mem(),
- region_start, region_size) )
- return true;
+ for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
+ if ( meminfo_overlap_check(mem_banks[i], region_start, region_size) )
+ return true;
/* Check if input region is overlapping with bootmodules */
if ( bootmodules_overlap_check(&bootinfo.modules,
region_start, region_size) )
return true;
-#ifdef CONFIG_ACPI
- /* Check if input region is overlapping with ACPI EfiACPIReclaimMemory */
- if ( meminfo_overlap_check(bootinfo_get_acpi(), region_start, region_size) )
- return true;
-#endif
-
return false;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 08/13] xen/arm: Introduce helper for static memory pages
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (6 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory Luca Fancellu
` (4 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Introduce a new helper function in the static-memory module
that can be called to manage static memory banks, this is
done to reuse the code when other modules would like to
manage static memory banks that are not part of the
reserved_mem structure, this is done because the static
shared memory banks will be removed from reserved_mem.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
v2:
- Add Michal R-by
- Changed commit msg
v1:
- new patch
---
---
xen/arch/arm/include/asm/static-memory.h | 13 +++++++++++++
xen/arch/arm/static-memory.c | 12 +-----------
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/include/asm/static-memory.h b/xen/arch/arm/include/asm/static-memory.h
index 3e3efd70c38d..804166e541ef 100644
--- a/xen/arch/arm/include/asm/static-memory.h
+++ b/xen/arch/arm/include/asm/static-memory.h
@@ -3,10 +3,23 @@
#ifndef __ASM_STATIC_MEMORY_H_
#define __ASM_STATIC_MEMORY_H_
+#include <xen/pfn.h>
#include <asm/kernel.h>
#ifdef CONFIG_STATIC_MEMORY
+static inline void init_staticmem_bank(const struct membank *bank)
+{
+ mfn_t bank_start = _mfn(PFN_UP(bank->start));
+ unsigned long bank_pages = PFN_DOWN(bank->size);
+ mfn_t bank_end = mfn_add(bank_start, bank_pages);
+
+ if ( mfn_x(bank_end) <= mfn_x(bank_start) )
+ return;
+
+ unprepare_staticmem_pages(mfn_to_page(bank_start), bank_pages, false);
+}
+
void allocate_static_memory(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node);
void assign_static_memory_11(struct domain *d, struct kernel_info *kinfo,
diff --git a/xen/arch/arm/static-memory.c b/xen/arch/arm/static-memory.c
index 34bd12696a53..d4585c5a0633 100644
--- a/xen/arch/arm/static-memory.c
+++ b/xen/arch/arm/static-memory.c
@@ -265,17 +265,7 @@ void __init init_staticmem_pages(void)
for ( bank = 0 ; bank < reserved_mem->nr_banks; bank++ )
{
if ( reserved_mem->bank[bank].type == MEMBANK_STATIC_DOMAIN )
- {
- mfn_t bank_start = _mfn(PFN_UP(reserved_mem->bank[bank].start));
- unsigned long bank_pages = PFN_DOWN(reserved_mem->bank[bank].size);
- mfn_t bank_end = mfn_add(bank_start, bank_pages);
-
- if ( mfn_x(bank_end) <= mfn_x(bank_start) )
- return;
-
- unprepare_staticmem_pages(mfn_to_page(bank_start),
- bank_pages, false);
- }
+ init_staticmem_bank(&reserved_mem->bank[bank]);
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (7 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 08/13] xen/arm: Introduce helper for static memory pages Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-10 10:01 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 10/13] xen/arm: remove shm holes from extended regions Luca Fancellu
` (3 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Currently the memory footprint of the static shared memory feature
is impacting all the struct meminfo instances with memory space
that is not going to be used.
To solve this issue, rework the static shared memory extra
information linked to the memory bank to another structure,
struct shmem_membank_extra, and exploit the struct membank
padding to host a pointer to that structure in a union with the
enum membank_type, with this trick the 'struct membank' has the
same size with or without the static shared memory, given that
the 'type' and 'shmem_extra' are never used at the same time,
hence the 'struct membank' won't grow in size.
Afterwards, create a new structure 'struct shared_meminfo' which
has the same interface of 'struct meminfo', but requires less
banks and hosts the extra information for the static shared memory.
The fields 'bank' and 'extra' of this structure are meant to be
linked by the index (e.g. extra[idx] will have the information for
the bank[idx], for i=0..NR_SHMEM_BANKS), the convinient pointer
'shmem_extra' of 'struct membank' is then linked to the related
'extra' bank to ease the fruition when a function has access only
to the 'struct membanks common' of 'struct shared_meminfo'.
The last part of this work is to move the allocation of the
static shared memory banks from the 'reserved_mem' to a new
'shmem' member of the 'struct bootinfo'.
Change also the 'shm_mem' member type to be 'struct shared_meminfo'
in order to match the above changes and allow a memory space
reduction also in 'struct kernel_info'.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Made clear that the struct membank is not growing in size in the
commit message.
- Add static assert for the struct shared_meminfo bank field.
v1:
- new patch
---
---
xen/arch/arm/arm32/mmu/mm.c | 24 ++++++++
xen/arch/arm/arm64/mmu/mm.c | 2 +
xen/arch/arm/bootfdt.c | 1 +
xen/arch/arm/domain_build.c | 5 ++
xen/arch/arm/include/asm/kernel.h | 4 +-
xen/arch/arm/include/asm/setup.h | 41 ++++++++++++-
xen/arch/arm/include/asm/static-shmem.h | 8 +++
xen/arch/arm/setup.c | 25 +++++++-
xen/arch/arm/static-shmem.c | 79 +++++++++++++++++++------
9 files changed, 165 insertions(+), 24 deletions(-)
diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
index e6bb5d934c16..45e42b307e20 100644
--- a/xen/arch/arm/arm32/mmu/mm.c
+++ b/xen/arch/arm/arm32/mmu/mm.c
@@ -7,6 +7,7 @@
#include <xen/pfn.h>
#include <asm/fixmap.h>
#include <asm/static-memory.h>
+#include <asm/static-shmem.h>
static unsigned long opt_xenheap_megabytes __initdata;
integer_param("xenheap_megabytes", opt_xenheap_megabytes);
@@ -42,6 +43,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
int first_mod)
{
const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+#ifdef CONFIG_STATIC_SHM
+ const struct membanks *shmem = bootinfo_get_shmem();
+#endif
const struct bootmodules *mi = &bootinfo.modules;
int i;
int nr;
@@ -118,6 +122,25 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
return consider_modules(s, r_s, size, align, i + 1);
}
}
+
+#ifdef CONFIG_STATIC_SHM
+ 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;
+
+ if ( s < r_e && r_s < e )
+ {
+ r_e = consider_modules(r_e, e, size, align, i + 1);
+ if ( r_e )
+ return r_e;
+
+ return consider_modules(s, r_s, size, align, i + 1);
+ }
+ }
+#endif
+
return e;
}
@@ -294,6 +317,7 @@ void __init setup_mm(void)
mfn_to_maddr(directmap_mfn_end));
init_staticmem_pages();
+ init_sharedmem_pages();
}
/*
diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
index f8aaf4ac18be..293acb67e09c 100644
--- a/xen/arch/arm/arm64/mmu/mm.c
+++ b/xen/arch/arm/arm64/mmu/mm.c
@@ -6,6 +6,7 @@
#include <asm/setup.h>
#include <asm/static-memory.h>
+#include <asm/static-shmem.h>
/* Override macros from asm/page.h to make them work with mfn_t */
#undef virt_to_mfn
@@ -236,6 +237,7 @@ void __init setup_mm(void)
max_page = PFN_DOWN(ram_end);
init_staticmem_pages();
+ init_sharedmem_pages();
}
/*
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 4c80962f79d4..4d708442a19e 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -511,6 +511,7 @@ static void __init early_print_info(void)
mem_resv->bank[j].start,
mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
}
+ early_print_info_shmem();
printk("\n");
for ( i = 0 ; i < cmds->nr_mods; i++ )
printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 269aaff4d067..01d66fbde92b 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -864,6 +864,7 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
* regions we exclude every region assigned to Dom0 from the Host RAM:
* - domain RAM
* - reserved-memory
+ * - static shared memory
* - grant table space
*/
static int __init find_unallocated_memory(const struct kernel_info *kinfo,
@@ -873,6 +874,9 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
bootinfo_get_mem(),
kernel_info_get_mem_const(kinfo),
bootinfo_get_reserved_mem(),
+#ifdef CONFIG_STATIC_SHM
+ bootinfo_get_shmem(),
+#endif
};
struct rangeset *unalloc_mem;
paddr_t start, end;
@@ -890,6 +894,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
* 1) Start with all available RAM
* 2) Remove RAM assigned to Dom0
* 3) Remove reserved memory
+ * 4) Remove static shared memory
* The order comes from the initialization of the variable "mem_banks"
* above
*/
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 85e1625cc989..7663d9f18d02 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -40,7 +40,7 @@ struct kernel_info {
paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
struct meminfo mem;
#ifdef CONFIG_STATIC_SHM
- struct meminfo shm_mem;
+ struct shared_meminfo shm_mem;
#endif
/* kernel entry point */
@@ -92,7 +92,7 @@ kernel_info_get_mem_const(const struct kernel_info *kinfo)
}
#ifdef CONFIG_STATIC_SHM
-#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_MEM_BANKS,
+#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS,
#else
#define KERNEL_INFO_SHM_MEM_INIT
#endif
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 696f55db86b1..28fb659fe946 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -9,6 +9,7 @@
#define MAX_FDT_SIZE SZ_2M
#define NR_MEM_BANKS 256
+#define NR_SHMEM_BANKS 32
#define MAX_MODULES 32 /* Current maximum useful modules */
@@ -46,14 +47,20 @@ enum membank_type {
/* Indicates the maximum number of characters(\0 included) for shm_id */
#define MAX_SHM_ID_LENGTH 16
+struct shmem_membank_extra {
+ char shm_id[MAX_SHM_ID_LENGTH];
+ unsigned int nr_shm_borrowers;
+};
+
struct membank {
paddr_t start;
paddr_t size;
- enum membank_type type;
+ union {
+ enum membank_type type;
#ifdef CONFIG_STATIC_SHM
- char shm_id[MAX_SHM_ID_LENGTH];
- unsigned int nr_shm_borrowers;
+ struct shmem_membank_extra *shmem_extra;
#endif
+ };
};
struct membanks {
@@ -67,6 +74,12 @@ struct meminfo {
struct membank bank[NR_MEM_BANKS];
};
+struct shared_meminfo {
+ struct membanks common;
+ struct membank bank[NR_SHMEM_BANKS];
+ struct shmem_membank_extra extra[NR_SHMEM_BANKS];
+};
+
/*
* The domU flag is set for kernels and ramdisks of "xen,domain" nodes.
* The purpose of the domU flag is to avoid getting confused in
@@ -109,6 +122,9 @@ struct bootinfo {
struct bootcmdlines cmdlines;
#ifdef CONFIG_ACPI
struct meminfo acpi;
+#endif
+#ifdef CONFIG_STATIC_SHM
+ struct shared_meminfo shmem;
#endif
bool static_heap;
};
@@ -119,11 +135,18 @@ struct bootinfo {
#define BOOTINFO_ACPI_INIT
#endif
+#ifdef CONFIG_STATIC_SHM
+#define BOOTINFO_SHMEM_INIT .shmem.common.max_banks = NR_SHMEM_BANKS,
+#else
+#define BOOTINFO_SHMEM_INIT
+#endif
+
#define BOOTINFO_INIT \
{ \
.mem.common.max_banks = NR_MEM_BANKS, \
.reserved_mem.common.max_banks = NR_MEM_BANKS, \
BOOTINFO_ACPI_INIT \
+ BOOTINFO_SHMEM_INIT \
}
struct map_range_data
@@ -158,6 +181,18 @@ static inline struct membanks *bootinfo_get_acpi(void)
}
#endif
+#ifdef CONFIG_STATIC_SHM
+static inline struct membanks *bootinfo_get_shmem(void)
+{
+ return &bootinfo.shmem.common;
+}
+
+static inline struct shmem_membank_extra *bootinfo_get_shmem_extra(void)
+{
+ return bootinfo.shmem.extra;
+}
+#endif
+
void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
size_t estimate_efi_size(unsigned int mem_nr_banks);
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 680594b6843d..90aafc81e740 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -24,6 +24,10 @@ static inline int process_shm_chosen(struct domain *d,
int process_shm_node(const void *fdt, int node, uint32_t address_cells,
uint32_t size_cells);
+void early_print_info_shmem(void);
+
+void init_sharedmem_pages(void);
+
#else /* !CONFIG_STATIC_SHM */
static inline int make_resv_memory_node(const struct kernel_info *kinfo,
@@ -51,6 +55,10 @@ static inline int process_shm_node(const void *fdt, int node,
return -EINVAL;
}
+static inline void early_print_info_shmem(void) {};
+
+static inline void init_sharedmem_pages(void) {};
+
#endif /* CONFIG_STATIC_SHM */
#endif /* __ASM_STATIC_SHMEM_H_ */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index cc719d508d63..111172a8c4b1 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -208,6 +208,9 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
unsigned int first)
{
const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+#ifdef CONFIG_STATIC_SHM
+ const struct membanks *shmem = bootinfo_get_shmem();
+#endif
unsigned int i, nr;
int rc;
@@ -258,6 +261,22 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
}
}
+#ifdef CONFIG_STATIC_SHM
+ 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;
+
+ if ( s < r_e && r_s < e )
+ {
+ dt_unreserved_regions(r_e, e, cb, i + 1);
+ dt_unreserved_regions(s, r_s, cb, i + 1);
+ return;
+ }
+ }
+#endif
+
cb(s, e);
}
@@ -344,13 +363,17 @@ bool __init check_reserved_regions_overlap(paddr_t region_start,
bootinfo_get_reserved_mem(),
#ifdef CONFIG_ACPI
bootinfo_get_acpi(),
+#endif
+#ifdef CONFIG_STATIC_SHM
+ bootinfo_get_shmem(),
#endif
};
unsigned int i;
/*
* Check if input region is overlapping with reserved memory banks or
- * ACPI EfiACPIReclaimMemory (when ACPI feature is enabled)
+ * ACPI EfiACPIReclaimMemory (when ACPI feature is enabled) or static
+ * shared memory banks (when static shared memory feature is enabled)
*/
for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
if ( meminfo_overlap_check(mem_banks[i], region_start, region_size) )
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 11f3c0da81a4..fcf8227bf404 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -4,29 +4,40 @@
#include <xen/sched.h>
#include <asm/domain_build.h>
+#include <asm/static-memory.h>
#include <asm/static-shmem.h>
+static void __init __maybe_unused build_assertions(void)
+{
+ /*
+ * Check that no padding is between struct membanks "bank" flexible array
+ * member and struct shared_meminfo "bank" member
+ */
+ BUILD_BUG_ON((offsetof(struct membanks, bank) !=
+ 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)
{
- const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
+ const struct membanks *shmem = bootinfo_get_shmem();
unsigned int bank;
/* Iterate reserved memory to find requested shm bank. */
- for ( bank = 0 ; bank < reserved_mem->nr_banks; bank++ )
+ for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
{
- paddr_t bank_start = reserved_mem->bank[bank].start;
- paddr_t bank_size = reserved_mem->bank[bank].size;
+ paddr_t bank_start = shmem->bank[bank].start;
+ paddr_t bank_size = shmem->bank[bank].size;
if ( (pbase == bank_start) && (psize == bank_size) )
break;
}
- if ( bank == reserved_mem->nr_banks )
+ if ( bank == shmem->nr_banks )
return -ENOENT;
- *nr_borrowers = reserved_mem->bank[bank].nr_shm_borrowers;
+ *nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
return 0;
}
@@ -158,16 +169,22 @@ static int __init assign_shared_memory(struct domain *d,
return ret;
}
-static int __init append_shm_bank_to_domain(struct membanks *shm_mem,
- paddr_t start, paddr_t size,
- const char *shm_id)
+static int __init
+append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start,
+ paddr_t size, const char *shm_id)
{
+ struct membanks *shm_mem = &kinfo_shm_mem->common;
+ struct shmem_membank_extra *shm_mem_extra;
+
if ( shm_mem->nr_banks >= shm_mem->max_banks )
return -ENOMEM;
+ shm_mem_extra = &kinfo_shm_mem->extra[shm_mem->nr_banks];
+
shm_mem->bank[shm_mem->nr_banks].start = start;
shm_mem->bank[shm_mem->nr_banks].size = size;
- safe_strcpy(shm_mem->bank[shm_mem->nr_banks].shm_id, shm_id);
+ safe_strcpy(shm_mem_extra->shm_id, shm_id);
+ shm_mem->bank[shm_mem->nr_banks].shmem_extra = shm_mem_extra;
shm_mem->nr_banks++;
return 0;
@@ -270,7 +287,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
* Record static shared memory region info for later setting
* up shm-node in guest device tree.
*/
- ret = append_shm_bank_to_domain(&kinfo->shm_mem.common, gbase, psize,
+ ret = append_shm_bank_to_domain(&kinfo->shm_mem, gbase, psize,
shm_id);
if ( ret )
return ret;
@@ -325,7 +342,8 @@ static int __init make_shm_memory_node(const struct kernel_info *kinfo,
dt_dprintk("Shared memory bank %u: %#"PRIx64"->%#"PRIx64"\n",
i, start, start + size);
- res = fdt_property_string(fdt, "xen,id", mem->bank[i].shm_id);
+ res = fdt_property_string(fdt, "xen,id",
+ mem->bank[i].shmem_extra->shm_id);
if ( res )
return res;
@@ -353,7 +371,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;
- struct membanks *mem = bootinfo_get_reserved_mem();
+ struct membanks *mem = bootinfo_get_shmem();
+ struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra();
unsigned int i;
int len;
bool owner = false;
@@ -442,7 +461,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
*/
if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
{
- if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) == 0 )
+ if ( strncmp(shm_id, shmem_extra[i].shm_id,
+ MAX_SHM_ID_LENGTH) == 0 )
break;
else
{
@@ -451,7 +471,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
return -EINVAL;
}
}
- else if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) != 0 )
+ else if ( strncmp(shm_id, shmem_extra[i].shm_id,
+ MAX_SHM_ID_LENGTH) != 0 )
continue;
else
{
@@ -469,10 +490,10 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
return -EINVAL;
/* Static shared memory shall be reserved from any other use. */
- safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id);
+ safe_strcpy(shmem_extra[mem->nr_banks].shm_id, shm_id);
mem->bank[mem->nr_banks].start = paddr;
mem->bank[mem->nr_banks].size = size;
- mem->bank[mem->nr_banks].type = MEMBANK_STATIC_DOMAIN;
+ mem->bank[mem->nr_banks].shmem_extra = &shmem_extra[mem->nr_banks];
mem->nr_banks++;
}
else
@@ -486,7 +507,7 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells,
* to calculate the reference count.
*/
if ( !owner )
- mem->bank[i].nr_shm_borrowers++;
+ shmem_extra[i].nr_shm_borrowers++;
return 0;
}
@@ -531,6 +552,28 @@ int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
return res;
}
+void __init early_print_info_shmem(void)
+{
+ const struct membanks *shmem = bootinfo_get_shmem();
+ unsigned int bank;
+
+ 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);
+ }
+}
+
+void __init init_sharedmem_pages(void)
+{
+ const struct membanks *shmem = bootinfo_get_shmem();
+ unsigned int bank;
+
+ for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
+ init_staticmem_bank(&shmem->bank[bank]);
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (8 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-10 11:15 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 11/13] xen/arm: fix duplicate /reserved-memory node in Dom0 Luca Fancellu
` (2 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 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>
Static shared memory acts as reserved memory in guest, so it shall be
excluded from extended regions.
Extended regions are taken care of under three different scenarios:
normal DomU, direct-map domain with iommu on, and direct-map domain
with iommu off.
For normal DomU, we create a new function "remove_shm_holes_for_domU",
to firstly transfer original outputs into the format of
"struct rangeset", then use "remove_shm_from_rangeset" to remove static
shm from them.
For direct-map domain with iommu on, after we get guest shm info from "kinfo",
we use "remove_shm_from_rangeset" to remove static shm.
For direct-map domain with iommu off, as static shm has already been taken
care of through reserved memory banks, we do nothing.
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- Fixed commit title, fixed comment, moved call of remove_shm_from_rangeset
after populating rangeset in find_memory_holes, print error code when
possible
- used PFN_DOWN where needed
v1:
- Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-Penny.Zheng@arm.com/
---
---
xen/arch/arm/domain_build.c | 16 ++++-
xen/arch/arm/include/asm/domain_build.h | 2 +
xen/arch/arm/include/asm/static-shmem.h | 18 +++++
xen/arch/arm/static-shmem.c | 88 +++++++++++++++++++++++++
4 files changed, 121 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 01d66fbde92b..9b36d6bb70c9 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -817,8 +817,8 @@ int __init make_memory_node(const struct domain *d,
return res;
}
-static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
- void *data)
+int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
+ void *data)
{
struct membanks *ext_regions = data;
paddr_t start, size;
@@ -978,6 +978,8 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
* - MMIO
* - Host RAM
* - PCI aperture
+ * - Static shared memory regions, which are described by special property
+ * "xen,shared-mem"
*/
static int __init find_memory_holes(const struct kernel_info *kinfo,
struct membanks *ext_regions)
@@ -1005,6 +1007,11 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
goto out;
}
+ /* Remove static shared memory regions */
+ res = remove_shm_from_rangeset(kinfo, mem_holes);
+ if ( res )
+ goto out;
+
/*
* Remove regions described by "reg" and "ranges" properties where
* the memory is addressable (MMIO, RAM, PCI BAR, etc).
@@ -1097,7 +1104,10 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
res = 0;
}
- return res;
+ if ( res )
+ return res;
+
+ return remove_shm_holes_for_domU(kinfo, ext_regions);
}
int __init make_hypervisor_node(struct domain *d,
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index a6f276cc4263..026d975da28e 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -51,6 +51,8 @@ static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
#endif
+int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
+
#endif
/*
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 90aafc81e740..2e8b138eb989 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -28,6 +28,12 @@ void early_print_info_shmem(void);
void init_sharedmem_pages(void);
+int remove_shm_from_rangeset(const struct kernel_info *kinfo,
+ struct rangeset *rangeset);
+
+int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
+ struct membanks *ext_regions);
+
#else /* !CONFIG_STATIC_SHM */
static inline int make_resv_memory_node(const struct kernel_info *kinfo,
@@ -59,6 +65,18 @@ static inline void early_print_info_shmem(void) {};
static inline void init_sharedmem_pages(void) {};
+static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo,
+ struct rangeset *rangeset)
+{
+ return 0;
+}
+
+static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
+ struct membanks *ext_regions)
+{
+ return 0;
+}
+
#endif /* CONFIG_STATIC_SHM */
#endif /* __ASM_STATIC_SHMEM_H_ */
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index fcf8227bf404..a08413ff44e2 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0-only */
#include <xen/libfdt/libfdt.h>
+#include <xen/rangeset.h>
#include <xen/sched.h>
#include <asm/domain_build.h>
@@ -574,6 +575,93 @@ void __init init_sharedmem_pages(void)
init_staticmem_bank(&shmem->bank[bank]);
}
+int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
+ struct rangeset *rangeset)
+{
+ const struct membanks *shm_mem = &kinfo->shm_mem.common;
+ unsigned int i;
+
+ /* Remove static shared memory regions */
+ for ( i = 0; i < shm_mem->nr_banks; i++ )
+ {
+ paddr_t start, end;
+ int res;
+
+ start = shm_mem->bank[i].start;
+ end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
+ res = rangeset_remove_range(rangeset, PFN_DOWN(start), PFN_DOWN(end));
+ if ( res )
+ {
+ printk(XENLOG_ERR
+ "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr", error: %d\n",
+ start, end, res);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
+ struct membanks *ext_regions)
+{
+ const struct membanks *shm_mem = &kinfo->shm_mem.common;
+ struct rangeset *guest_holes;
+ unsigned int i;
+ paddr_t start;
+ paddr_t end;
+ int res;
+
+ /* No static shared memory region. */
+ if ( shm_mem->nr_banks == 0 )
+ return 0;
+
+ dt_dprintk("Remove static shared memory holes from extended regions of DomU\n");
+
+ guest_holes = rangeset_new(NULL, NULL, 0);
+ if ( !guest_holes )
+ return -ENOMEM;
+
+ /* Copy extended regions sets into the rangeset */
+ for ( i = 0; i < ext_regions->nr_banks; i++ )
+ {
+ start = ext_regions->bank[i].start;
+ end = start + ext_regions->bank[i].size - 1;
+
+ res = rangeset_add_range(guest_holes, PFN_DOWN(start), PFN_DOWN(end));
+ if ( res )
+ {
+ printk(XENLOG_ERR
+ "Failed to add: %#"PRIpaddr"->%#"PRIpaddr", error: %d\n",
+ start, end, res);
+ goto out;
+ }
+ }
+
+ /* Remove static shared memory regions */
+ res = remove_shm_from_rangeset(kinfo, guest_holes);
+ if ( res )
+ goto out;
+
+ i = ext_regions->nr_banks - 1;
+ start = ext_regions->bank[0].start;
+ end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
+
+ /* Reset original extended regions to hold new value */
+ ext_regions->nr_banks = 0;
+ res = rangeset_report_ranges(guest_holes, PFN_DOWN(start), PFN_DOWN(end),
+ add_ext_regions, ext_regions);
+ if ( res )
+ ext_regions->nr_banks = 0;
+ else if ( !ext_regions->nr_banks )
+ res = -ENOENT;
+
+ out:
+ rangeset_destroy(guest_holes);
+
+ return res;
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 11/13] xen/arm: fix duplicate /reserved-memory node in Dom0
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (9 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 10/13] xen/arm: remove shm holes from extended regions Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes Luca Fancellu
12 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 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>
In case there is a /reserved-memory node already present in the host
dtb, current Xen codes would create yet another /reserved-memory node
when the static shared memory feature is enabled and static shared
memory regions are present.
This would result in an incorrect device tree generation and hwdom
would not be able to detect the static shared memory region.
Avoid this issue by checking the presence of the /reserved-memory
node and appending the nodes instead of generating a duplicate
/reserved-memory.
Make make_shm_memory_node externally visible and rename it to
make_shm_resv_memory_node to make clear it produces childs for
/reserved-memory.
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:
- fix comment, remove function signature change, fixed commit msg
- rename make_shm_memory_node to make_shm_resv_memory_node in order
to make clear that it produces childs for /reserved-memory
- Add Michal R-by
v1:
- Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-11-Penny.Zheng@arm.com/
---
xen/arch/arm/domain_build.c | 23 ++++++++++++++++++++---
xen/arch/arm/include/asm/static-shmem.h | 9 +++++++++
xen/arch/arm/static-shmem.c | 8 ++++----
3 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9b36d6bb70c9..71eebfcf7e03 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1628,6 +1628,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
DT_MATCH_PATH("/hypervisor"),
{ /* sentinel */ },
};
+ static __initdata bool res_mem_node_found = false;
struct dt_device_node *child;
int res, i, nirq, irq_id;
const char *name;
@@ -1722,6 +1723,19 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
if ( res )
return res;
+ if ( dt_node_path_is_equal(node, "/reserved-memory") )
+ {
+ res_mem_node_found = true;
+ /*
+ * Avoid duplicate /reserved-memory nodes in Device Tree, so add the
+ * static shared memory nodes there.
+ */
+ res = make_shm_resv_memory_node(kinfo, dt_n_addr_cells(node),
+ dt_n_size_cells(node));
+ if ( res )
+ return res;
+ }
+
for ( child = node->child; child != NULL; child = child->sibling )
{
res = handle_node(d, kinfo, child, p2mt);
@@ -1774,9 +1788,12 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
return res;
}
- res = make_resv_memory_node(kinfo, addrcells, sizecells);
- if ( res )
- return res;
+ if ( !res_mem_node_found )
+ {
+ res = make_resv_memory_node(kinfo, addrcells, sizecells);
+ if ( res )
+ return res;
+ }
}
res = fdt_end_node(kinfo->fdt);
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 2e8b138eb989..7495a91e7a31 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -34,6 +34,9 @@ int remove_shm_from_rangeset(const struct kernel_info *kinfo,
int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
struct membanks *ext_regions);
+int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
+ int sizecells);
+
#else /* !CONFIG_STATIC_SHM */
static inline int make_resv_memory_node(const struct kernel_info *kinfo,
@@ -77,6 +80,12 @@ static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
return 0;
}
+static inline int make_shm_resv_memory_node(const struct kernel_info *kinfo,
+ int addrcells, int sizecells)
+{
+ return 0;
+}
+
#endif /* CONFIG_STATIC_SHM */
#endif /* __ASM_STATIC_SHMEM_H_ */
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index a08413ff44e2..5ad6f1269c48 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -297,8 +297,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo,
return 0;
}
-static int __init make_shm_memory_node(const struct kernel_info *kinfo,
- int addrcells, int sizecells)
+int __init make_shm_resv_memory_node(const struct kernel_info *kinfo,
+ int addrcells, int sizecells)
{
const struct membanks *mem = &kinfo->shm_mem.common;
void *fdt = kinfo->fdt;
@@ -306,7 +306,7 @@ static int __init make_shm_memory_node(const struct kernel_info *kinfo,
int res = 0;
if ( mem->nr_banks == 0 )
- return -ENOENT;
+ return 0;
/*
* For each shared memory region, a range is exposed under
@@ -544,7 +544,7 @@ int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
if ( res )
return res;
- res = make_shm_memory_node(kinfo, addrcells, sizecells);
+ res = make_shm_resv_memory_node(kinfo, addrcells, sizecells);
if ( res )
return res;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (10 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 11/13] xen/arm: fix duplicate /reserved-memory node in Dom0 Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-11 9:50 ` Luca Fancellu
2024-04-18 6:28 ` Jan Beulich
2024-04-09 11:45 ` [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes Luca Fancellu
12 siblings, 2 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Roger Pau Monné
Introduce a function that given an array of cells containing
(address,size) intervals, merges the overlapping ones, returning
an array with no overlapping intervals.
The algorithm needs to sort the intervals by ascending order
address, so the sort() function already included in the codebase
is used, however in this case additional data is needed for the
compare function, to be able to extract the address from the
interval.
So add one argument to the sort() function and its compare
callback to have additional data and be able to pass, in this
case, the address length. In case the argument is not needed,
NULL can be provided.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- new patch
---
---
xen/arch/arm/bootfdt.c | 5 +-
xen/arch/arm/io.c | 11 ++-
xen/arch/x86/extable.c | 5 +-
xen/common/device_tree.c | 140 ++++++++++++++++++++++++++++++++++
xen/include/xen/device_tree.h | 19 +++++
xen/include/xen/sort.h | 14 ++--
6 files changed, 181 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 4d708442a19e..a2aba67b45e7 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -521,7 +521,8 @@ static void __init early_print_info(void)
}
/* This function assumes that memory regions are not overlapped */
-static int __init cmp_memory_node(const void *key, const void *elem)
+static int __init cmp_memory_node(const void *key, const void *elem,
+ const void *data)
{
const struct membank *handler0 = key;
const struct membank *handler1 = elem;
@@ -569,7 +570,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
* the banks sorted in ascending order. So sort them through.
*/
sort(mem->bank, mem->nr_banks, sizeof(struct membank),
- cmp_memory_node, swap_memory_node);
+ cmp_memory_node, swap_memory_node, NULL);
early_print_info();
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 96c740d5636c..c1814491fec4 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -57,7 +57,7 @@ static enum io_state handle_write(const struct mmio_handler *handler,
}
/* This function assumes that mmio regions are not overlapped */
-static int cmp_mmio_handler(const void *key, const void *elem)
+static int cmp_mmio_handler(const void *key, const void *elem, const void *data)
{
const struct mmio_handler *handler0 = key;
const struct mmio_handler *handler1 = elem;
@@ -71,6 +71,11 @@ static int cmp_mmio_handler(const void *key, const void *elem)
return 0;
}
+static int bsearch_cmp_mmio_handler(const void *key, const void *elem)
+{
+ return cmp_mmio_handler(key, elem, NULL);
+}
+
static void swap_mmio_handler(void *_a, void *_b, size_t size)
{
struct mmio_handler *a = _a, *b = _b;
@@ -87,7 +92,7 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d,
read_lock(&vmmio->lock);
handler = bsearch(&key, vmmio->handlers, vmmio->num_entries,
- sizeof(*handler), cmp_mmio_handler);
+ sizeof(*handler), bsearch_cmp_mmio_handler);
read_unlock(&vmmio->lock);
return handler;
@@ -219,7 +224,7 @@ void register_mmio_handler(struct domain *d,
/* Sort mmio handlers in ascending order based on base address */
sort(vmmio->handlers, vmmio->num_entries, sizeof(struct mmio_handler),
- cmp_mmio_handler, swap_mmio_handler);
+ cmp_mmio_handler, swap_mmio_handler, NULL);
write_unlock(&vmmio->lock);
}
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index 8415cd1fa249..589e251b29b9 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -23,7 +23,8 @@ static inline unsigned long ex_cont(const struct exception_table_entry *x)
return EX_FIELD(x, cont);
}
-static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b)
+static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b,
+ const void *data)
{
const struct exception_table_entry *l = a, *r = b;
unsigned long lip = ex_addr(l);
@@ -53,7 +54,7 @@ void init_or_livepatch sort_exception_table(struct exception_table_entry *start,
const struct exception_table_entry *stop)
{
sort(start, stop - start,
- sizeof(struct exception_table_entry), cmp_ex, swap_ex);
+ sizeof(struct exception_table_entry), cmp_ex, swap_ex, NULL);
}
void __init sort_exception_tables(void)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 8d1017a49d80..24914a80d03b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -18,6 +18,7 @@
#include <xen/lib.h>
#include <xen/libfdt/libfdt.h>
#include <xen/mm.h>
+#include <xen/sort.h>
#include <xen/stdarg.h>
#include <xen/string.h>
#include <xen/cpumask.h>
@@ -2243,6 +2244,145 @@ int dt_get_pci_domain_nr(struct dt_device_node *node)
return (u16)domain;
}
+static int __init cmp_mem_reg_cell(const void *key, const void *elem,
+ const void *data)
+{
+ const __be32 *cell0 = key;
+ const __be32 *cell1 = elem;
+ const int *addrcells = data;
+ u64 addr0, addr1;
+
+ /* Same address, same element */
+ if ( cell0 == cell1 )
+ return 0;
+
+ BUG_ON(!addrcells || !*addrcells || *addrcells > 2);
+ addr0 = dt_read_number(cell0, *addrcells);
+ addr1 = dt_read_number(cell1, *addrcells);
+
+ if ( addr0 < addr1 )
+ return -1;
+
+ if ( addr0 > addr1 )
+ return 1;
+
+ return 0;
+}
+
+static void __init swap_mem_reg_cell(void *_a, void *_b, size_t size)
+{
+ __be32 tmp[4];
+ __be32 *cell0 = _a;
+ __be32 *cell1 = _b;
+
+ BUG_ON(size > (4 * sizeof(__be32)));
+
+ /* Don't swap the same element */
+ if ( cell0 == cell1 )
+ return;
+
+ /* Swap cell0 and cell1 */
+ memcpy(tmp, cell0, size);
+ memcpy(cell0, cell1, size);
+ memcpy(cell1, tmp, size);
+}
+
+int __init dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells,
+ int addrcells,
+ int sizecells)
+{
+ int reg_size = addrcells + sizecells;
+ u64 start_last, size_last, end_last;
+ unsigned int count;
+ unsigned int i = 1;
+ __be32 *last_cell = reg;
+
+ BUG_ON(!nr_cells || !reg);
+
+ if ( (addrcells < 1) || (addrcells > 2) || (sizecells < 1) ||
+ (sizecells > 2) )
+ return -EINVAL;
+
+ count = *nr_cells / reg_size;
+ /* Early stop, only one interval in the array */
+ if ( count == 1 )
+ return 0;
+
+ /* Sort cells by ascending address */
+ sort(reg, count, reg_size * sizeof(__be32), cmp_mem_reg_cell,
+ swap_mem_reg_cell, &addrcells);
+
+ /*
+ * Algorithm to merge overlapping intervals in place, prerequisite for the
+ * intervals is that they must be sorted with ascending order address
+ */
+ start_last = dt_read_number(last_cell, addrcells);
+ size_last = dt_read_number(last_cell + addrcells, sizecells);
+ end_last = start_last + size_last;
+
+ /* The sum is too big */
+ if ( end_last < start_last )
+ return -ERANGE;
+
+ while ( i < count )
+ {
+ __be32 *current_cell = ®[i * reg_size];
+ u64 start_current = dt_read_number(current_cell, addrcells);
+ u64 size_current = dt_read_number(current_cell + addrcells, sizecells);
+ u64 end_current = start_current + size_current;
+ bool overlap = end_last >= start_current;
+ u64 new_size;
+
+ /* The sum is too big */
+ if ( end_current < start_current )
+ return -ERANGE;
+
+ new_size = MAX(end_last, end_current) - start_last;
+
+ /*
+ * If the last interval end is not connected with the current one, or
+ * if they are connected but the new computed size would not be
+ * representable given the input sizecells, don't merge and advance the
+ * last of one position.
+ */
+ if ( !overlap ||
+ (overlap && (sizecells < 2) && (new_size > UINT32_MAX)) )
+ {
+ /* last element doesn't overlap with the current, advance it */
+ last_cell = last_cell + reg_size;
+ start_last = dt_read_number(last_cell, addrcells);
+ size_last = dt_read_number(last_cell + addrcells, sizecells);
+ end_last = start_last + size_last;
+ }
+ else
+ {
+ /* Temporary pointer because dt_set_cell modifies it */
+ __be32 *tmp_last_cell_size = last_cell + addrcells;
+
+ dt_set_cell(&tmp_last_cell_size, sizecells, new_size);
+ /*
+ * This current interval is merged with the last one, so remove this
+ * interval and shift left all the remaining elements
+ */
+ memmove(current_cell, current_cell + reg_size,
+ (reg_size * (count - i)) * sizeof(__be32));
+ /* Now the array has less element since we merged two intervals */
+ count--;
+ /*
+ * Next iteration needs to start from the current index, skip
+ * increment
+ */
+ continue;
+ }
+ /* Point to the next element in the array */
+ i++;
+ }
+
+ /* Now count holds the number of intervals in the array */
+ *nr_cells = count * reg_size;
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index e6287305a7b5..95a88a0d3bc9 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -946,6 +946,25 @@ int dt_get_pci_domain_nr(struct dt_device_node *node);
struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
+/**
+ * dt_merge_overlapping_addr_size_intervals - Given an array of (address, size)
+ * cells intervals, returns an array with the overlapping intervals merged.
+ * @reg: Array of (address, size) cells.
+ * @nr_cells: Total number of cells in the array.
+ * @addrcells: Size of the "address" in number of cells.
+ * @sizecells: Size of the "size" in number of cells.
+ *
+ * Return:
+ * * 0 - On success.
+ * * -ERANGE - The interval computation results are not representable.
+ * (address + size results in truncation overflow).
+ * * -EINVAL - addrcells or sizecells are outside the interval [1, 2]
+ *
+ * Returns in nr_cells the new number of cells in the array.
+ */
+int dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells,
+ int addrcells, int sizecells);
+
#ifdef CONFIG_DEVICE_TREE_DEBUG
#define dt_dprintk(fmt, args...) \
printk(XENLOG_DEBUG fmt, ## args)
diff --git a/xen/include/xen/sort.h b/xen/include/xen/sort.h
index b95328628465..1bd4420457c0 100644
--- a/xen/include/xen/sort.h
+++ b/xen/include/xen/sort.h
@@ -23,8 +23,8 @@
extern gnu_inline
#endif
void sort(void *base, size_t num, size_t size,
- int (*cmp)(const void *a, const void *b),
- void (*swap)(void *a, void *b, size_t size))
+ int (*cmp)(const void *a, const void *b, const void *data),
+ void (*swap)(void *a, void *b, size_t size), const void *cmp_data)
{
/* pre-scale counters for performance */
size_t i = (num / 2) * size, n = num * size, c, r;
@@ -35,9 +35,10 @@ void sort(void *base, size_t num, size_t size,
for ( r = i -= size; r * 2 + size < n; r = c )
{
c = r * 2 + size;
- if ( (c < n - size) && (cmp(base + c, base + c + size) < 0) )
+ if ( (c < n - size) &&
+ (cmp(base + c, base + c + size, cmp_data) < 0) )
c += size;
- if ( cmp(base + r, base + c) >= 0 )
+ if ( cmp(base + r, base + c, cmp_data) >= 0 )
break;
swap(base + r, base + c, size);
}
@@ -51,9 +52,10 @@ void sort(void *base, size_t num, size_t size,
for ( r = 0; r * 2 + size < i; r = c )
{
c = r * 2 + size;
- if ( (c < i - size) && (cmp(base + c, base + c + size) < 0) )
+ if ( (c < i - size) &&
+ (cmp(base + c, base + c + size, cmp_data) < 0) )
c += size;
- if ( cmp(base + r, base + c) >= 0 )
+ if ( cmp(base + r, base + c, cmp_data) >= 0 )
break;
swap(base + r, base + c, size);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
` (11 preceding siblings ...)
2024-04-09 11:45 ` [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals Luca Fancellu
@ 2024-04-09 11:45 ` Luca Fancellu
2024-04-15 18:41 ` Julien Grall
12 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 11:45 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Currently Xen is not exporting the static shared memory regions
to the device tree as /memory node, this commit is fixing this
issue.
The static shared memory banks can be part of the memory range
available for the domain, so if they are overlapping with the
normal memory banks, they need to be merged together in order
to produce a /memory node with non overlapping ranges in 'reg'.
Given that now make_memory_node needs a parameter 'struct kernel_info'
in order to call the new function shm_mem_node_merge_reg_range,
take the occasion to remove the unused struct domain parameter.
Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
---
v2:
- try to use make_memory_node, don't add overlapping ranges of
memory, commit message changed.
v1:
- new patch
---
---
xen/arch/arm/dom0less-build.c | 2 +-
xen/arch/arm/domain_build.c | 38 ++++++++++++++++++-------
xen/arch/arm/include/asm/domain_build.h | 2 +-
xen/arch/arm/include/asm/static-shmem.h | 18 ++++++++++++
xen/arch/arm/static-shmem.c | 26 +++++++++++++++++
5 files changed, 73 insertions(+), 13 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 51cf03221d56..74f053c242f4 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -642,7 +642,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
if ( ret )
goto err;
- ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ ret = make_memory_node(kinfo, addrcells, sizecells,
kernel_info_get_mem(kinfo));
if ( ret )
goto err;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 71eebfcf7e03..f9b749d0a068 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -757,15 +757,14 @@ int __init domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit)
return fdt_begin_node(fdt, buf);
}
-int __init make_memory_node(const struct domain *d,
- void *fdt,
- int addrcells, int sizecells,
- const struct membanks *mem)
+int __init make_memory_node(const struct kernel_info *kinfo, int addrcells,
+ int sizecells, const struct membanks *mem)
{
+ void *fdt = kinfo->fdt;
unsigned int i;
int res, reg_size = addrcells + sizecells;
int nr_cells = 0;
- __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
+ __be32 reg[DT_MEM_NODE_REG_RANGE_SIZE];
__be32 *cells;
if ( mem->nr_banks == 0 )
@@ -798,14 +797,32 @@ int __init make_memory_node(const struct domain *d,
if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN )
continue;
- dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n",
- i, start, start + size);
-
nr_cells += reg_size;
BUG_ON(nr_cells >= ARRAY_SIZE(reg));
dt_child_set_range(&cells, addrcells, sizecells, start, size);
}
+ /*
+ * static shared memory banks need to be listed as /memory node, so when
+ * this function is handling the normal memory, merge the banks.
+ */
+ if ( mem == kernel_info_get_mem_const(kinfo) )
+ {
+ res = shm_mem_node_merge_reg_range(kinfo, reg, &nr_cells, addrcells,
+ sizecells);
+ if ( res )
+ return res;
+ }
+
+ for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size )
+ {
+ u64 start = dt_read_number(cells, addrcells);
+ u64 size = dt_read_number(cells + addrcells, sizecells);
+
+ dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n",
+ i, start, start + size);
+ }
+
dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells);
res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
@@ -1771,7 +1788,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
if ( res )
return res;
- res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+ res = make_memory_node(kinfo, addrcells, sizecells,
kernel_info_get_mem(kinfo));
if ( res )
return res;
@@ -1782,8 +1799,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
*/
if ( reserved_mem->nr_banks > 0 )
{
- res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
- reserved_mem);
+ res = make_memory_node(kinfo, addrcells, sizecells, reserved_mem);
if ( res )
return res;
}
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index 026d975da28e..45936212ca21 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -14,7 +14,7 @@ int make_chosen_node(const struct kernel_info *kinfo);
int make_cpus_node(const struct domain *d, void *fdt);
int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
int addrcells, int sizecells);
-int make_memory_node(const struct domain *d, void *fdt, int addrcells,
+int make_memory_node(const struct kernel_info *kinfo, int addrcells,
int sizecells, const struct membanks *mem);
int make_psci_node(void *fdt);
int make_timer_node(const struct kernel_info *kinfo);
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 7495a91e7a31..bb5624c801af 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -3,10 +3,15 @@
#ifndef __ASM_STATIC_SHMEM_H_
#define __ASM_STATIC_SHMEM_H_
+#include <xen/types.h>
#include <asm/kernel.h>
+#include <asm/setup.h>
#ifdef CONFIG_STATIC_SHM
+/* Worst case /memory node reg element: (addrcells + sizecells) */
+#define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4)
+
int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
int sizecells);
@@ -37,8 +42,14 @@ int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
int sizecells);
+int shm_mem_node_merge_reg_range(const struct kernel_info *kinfo, __be32 *reg,
+ int *nr_cells, int addrcells, int sizecells);
+
#else /* !CONFIG_STATIC_SHM */
+/* Worst case /memory node reg element: (addrcells + sizecells) */
+#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4)
+
static inline int make_resv_memory_node(const struct kernel_info *kinfo,
int addrcells, int sizecells)
{
@@ -86,6 +97,13 @@ static inline int make_shm_resv_memory_node(const struct kernel_info *kinfo,
return 0;
}
+static inline int shm_mem_node_merge_reg_range(const struct kernel_info *kinfo,
+ __be32 *reg, int *nr_cells,
+ int addrcells, int sizecells)
+{
+ return 0;
+}
+
#endif /* CONFIG_STATIC_SHM */
#endif /* __ASM_STATIC_SHMEM_H_ */
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 5ad6f1269c48..61fcbe217c61 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-only */
+#include <xen/device_tree.h>
#include <xen/libfdt/libfdt.h>
#include <xen/rangeset.h>
#include <xen/sched.h>
@@ -662,6 +663,31 @@ int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
return res;
}
+int __init shm_mem_node_merge_reg_range(const struct kernel_info *kinfo,
+ __be32 *reg, int *nr_cells,
+ int addrcells, int sizecells)
+{
+ const struct membanks *mem = &kinfo->shm_mem.common;
+ unsigned int i;
+ __be32 *cells;
+
+ BUG_ON(!nr_cells || !reg);
+
+ cells = ®[*nr_cells];
+ for ( i = 0; i < mem->nr_banks; i++ )
+ {
+ u64 start = mem->bank[i].start;
+ u64 size = mem->bank[i].size;
+
+ *nr_cells += addrcells + sizecells;
+ BUG_ON(*nr_cells >= DT_MEM_NODE_REG_RANGE_SIZE);
+ dt_child_set_range(&cells, addrcells, sizecells, start, size);
+ }
+
+ return dt_merge_overlapping_addr_size_intervals(reg, nr_cells, addrcells,
+ sizecells);
+}
+
/*
* Local variables:
* mode: C
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node
2024-04-09 11:45 ` [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node Luca Fancellu
@ 2024-04-09 13:18 ` Michal Orzel
0 siblings, 0 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-09 13:18 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Luca,
The title should be "xen/arm: Pass struct kernel_info parameter to make_{resv,shm}_memory_node
given that you're modifying both functions.
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> The struct domain parameter is not used in make_resv_memory_node
> and in its called function make_shm_memory_node, so drop it from
> both function, also, take the occasion to pass directly
s/function/functions
> struct kernel_info, from which we can infer other parameter
s/parameter/parameters
> passed to the functions and drop them as well.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures
2024-04-09 11:45 ` [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures Luca Fancellu
@ 2024-04-09 13:24 ` Michal Orzel
0 siblings, 0 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-09 13:24 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> Currently the 'stuct meminfo' is defining a static defined array of
s/stuct/struct
> 'struct membank' of NR_MEM_BANKS elements, some feature like
s/feature/features
> shared memory don't require such amount of memory allocation but
> might want to reuse existing code to manipulate this kind of
> structure that is just as 'struct meminfo' but less bulky.
>
> For this reason introduce a generic way to access this kind of
> structure using a new structure 'struct membanks', which implements
> all the fields needed by a structure related to memory banks
> without the need to specify at build time the size of the
> 'struct membank' array, using a flexible array member.
>
> Modify 'struct meminfo' to implement the field related to the new
> introduced structure, given the change all usage of this structure
> are updated in this way:
> - code accessing bootinfo.{mem,reserved_mem,acpi} field now uses
> 3 new introduced static inline helpers to access the new field
> of 'struct meminfo' named 'common'.
> - code accessing 'struct kernel_info *' member 'mem' now use the
> new introduced macro 'kernel_info_get_mem(...)' to access the
> new field of 'struct meminfo' named 'common'.
I would also mention introduction of KERNEL_INFO_INIT, BOOTINFO_INIT that should be used
from now onwards to initialize corresponding structs (.max_banks member).
>
> Constify pointers where needed.
>
> Suggested-by: Julien Grall <julien@xen.org>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2:
> - Fixed typos in commit message and mention flexible array member
> - Add static assert for struct membanks
> - use static inline for the kernel_info structure instead of macro
> - use xzalloc_flex_struct inside make_hypervisor_node instead of
> xzalloc
> - Fix trailing backslash style
[...]
>
> +static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo)
> +{
> + return &kinfo->mem.common;
> +}
> +
> +static inline const struct membanks *
> +kernel_info_get_mem_const(const struct kernel_info *kinfo)
I'm not a fan of having 2 helpers named this way. Maybe macro (as in v1) would be better here.
Other than that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory
2024-04-09 11:45 ` [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory Luca Fancellu
@ 2024-04-09 13:38 ` Michal Orzel
2024-04-09 14:37 ` Luca Fancellu
0 siblings, 1 reply; 39+ messages in thread
From: Michal Orzel @ 2024-04-09 13:38 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> The function find_unallocated_memory is using the same code to
> loop through 3 structure of the same type, in order to avoid
> code duplication, rework the code to have only one loop that
> goes through all the structures.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2:
> - Add comment in the loop inside find_unallocated_memory to
> improve readability
> v1:
> - new patch
> ---
> ---
> xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------
> 1 file changed, 25 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 57cf92668ae6..269aaff4d067 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
> struct membanks *ext_regions)
> {
> - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
> - const struct membanks *mem = bootinfo_get_mem();
> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> + const struct membanks *mem_banks[] = {
> + bootinfo_get_mem(),
> + kernel_info_get_mem_const(kinfo),
> + bootinfo_get_reserved_mem(),
> + };
> struct rangeset *unalloc_mem;
> paddr_t start, end;
> - unsigned int i;
> + unsigned int i, j;
> int res;
>
> dt_dprintk("Find unallocated memory for extended regions\n");
> @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
> if ( !unalloc_mem )
> return -ENOMEM;
>
> - /* Start with all available RAM */
> - for ( i = 0; i < mem->nr_banks; i++ )
> - {
> - start = mem->bank[i].start;
> - end = mem->bank[i].start + mem->bank[i].size;
> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
> - PFN_DOWN(end - 1));
> - if ( res )
> + /*
> + * Exclude the following regions, in order:
> + * 1) Start with all available RAM
> + * 2) Remove RAM assigned to Dom0
> + * 3) Remove reserved memory
Given this commit and the previous code, I expect one call to rangeset_add_range() and
3 calls to rangeset_remove_range(). However ...
> + * The order comes from the initialization of the variable "mem_banks"
> + * above
> + */
> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
> {
> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
> - start, end);
> - goto out;
> - }
> - }
> -
> - /* Remove RAM assigned to Dom0 */
> - for ( i = 0; i < kinfo_mem->nr_banks; i++ )
> - {
> - start = kinfo_mem->bank[i].start;
> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
> + start = mem_banks[i]->bank[j].start;
> + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
> + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
... here you always call rangeset_add_range() which is wrong. For direct mapped domain
you would e.g. register its RAM region as extended region.
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory
2024-04-09 13:38 ` Michal Orzel
@ 2024-04-09 14:37 ` Luca Fancellu
0 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-09 14:37 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
> On 9 Apr 2024, at 14:38, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Hi Luca,
>
> On 09/04/2024 13:45, Luca Fancellu wrote:
>>
>>
>> The function find_unallocated_memory is using the same code to
>> loop through 3 structure of the same type, in order to avoid
>> code duplication, rework the code to have only one loop that
>> goes through all the structures.
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> ---
>> v2:
>> - Add comment in the loop inside find_unallocated_memory to
>> improve readability
>> v1:
>> - new patch
>> ---
>> ---
>> xen/arch/arm/domain_build.c | 70 +++++++++++++------------------------
>> 1 file changed, 25 insertions(+), 45 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 57cf92668ae6..269aaff4d067 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>> struct membanks *ext_regions)
>> {
>> - const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
>> - const struct membanks *mem = bootinfo_get_mem();
>> - const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
>> + const struct membanks *mem_banks[] = {
>> + bootinfo_get_mem(),
>> + kernel_info_get_mem_const(kinfo),
>> + bootinfo_get_reserved_mem(),
>> + };
>> struct rangeset *unalloc_mem;
>> paddr_t start, end;
>> - unsigned int i;
>> + unsigned int i, j;
>> int res;
>>
>> dt_dprintk("Find unallocated memory for extended regions\n");
>> @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>> if ( !unalloc_mem )
>> return -ENOMEM;
>>
>> - /* Start with all available RAM */
>> - for ( i = 0; i < mem->nr_banks; i++ )
>> - {
>> - start = mem->bank[i].start;
>> - end = mem->bank[i].start + mem->bank[i].size;
>> - res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
>> - PFN_DOWN(end - 1));
>> - if ( res )
>> + /*
>> + * Exclude the following regions, in order:
>> + * 1) Start with all available RAM
>> + * 2) Remove RAM assigned to Dom0
>> + * 3) Remove reserved memory
> Given this commit and the previous code, I expect one call to rangeset_add_range() and
> 3 calls to rangeset_remove_range(). However ...
>> + * The order comes from the initialization of the variable "mem_banks"
>> + * above
>> + */
>> + for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
>> + for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
>> {
>> - printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
>> - start, end);
>> - goto out;
>> - }
>> - }
>> -
>> - /* Remove RAM assigned to Dom0 */
>> - for ( i = 0; i < kinfo_mem->nr_banks; i++ )
>> - {
>> - start = kinfo_mem->bank[i].start;
>> - end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
>> - res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
>> + start = mem_banks[i]->bank[j].start;
>> + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
>> + res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
> ... here you always call rangeset_add_range() which is wrong. For direct mapped domain
> you would e.g. register its RAM region as extended region.
Right, I read it wrong initially, my mistake, here we are adding all available ram and later removing the dom0 regions
and reserved regions. Will fix
>
> ~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap
2024-04-09 11:45 ` [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap Luca Fancellu
@ 2024-04-10 7:40 ` Michal Orzel
0 siblings, 0 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-10 7:40 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> The function check_reserved_regions_overlap is calling
> 'meminfo_overlap_check' on the same type of structure, this code
> can be written in a way to avoid code duplication, so rework the
> function to do that.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
2024-04-09 11:45 ` [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory Luca Fancellu
@ 2024-04-10 10:01 ` Michal Orzel
2024-04-10 10:56 ` Luca Fancellu
0 siblings, 1 reply; 39+ messages in thread
From: Michal Orzel @ 2024-04-10 10:01 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> Currently the memory footprint of the static shared memory feature
> is impacting all the struct meminfo instances with memory space
> that is not going to be used.
>
> To solve this issue, rework the static shared memory extra
> information linked to the memory bank to another structure,
> struct shmem_membank_extra, and exploit the struct membank
> padding to host a pointer to that structure in a union with the
> enum membank_type, with this trick the 'struct membank' has the
> same size with or without the static shared memory, given that
> the 'type' and 'shmem_extra' are never used at the same time,
> hence the 'struct membank' won't grow in size.
>
> Afterwards, create a new structure 'struct shared_meminfo' which
> has the same interface of 'struct meminfo', but requires less
I would expect some justification for selecting 32 as the max number of shmem banks
> banks and hosts the extra information for the static shared memory.
> The fields 'bank' and 'extra' of this structure are meant to be
> linked by the index (e.g. extra[idx] will have the information for
> the bank[idx], for i=0..NR_SHMEM_BANKS), the convinient pointer
> 'shmem_extra' of 'struct membank' is then linked to the related
> 'extra' bank to ease the fruition when a function has access only
> to the 'struct membanks common' of 'struct shared_meminfo'.
>
> The last part of this work is to move the allocation of the
> static shared memory banks from the 'reserved_mem' to a new
> 'shmem' member of the 'struct bootinfo'.
> Change also the 'shm_mem' member type to be 'struct shared_meminfo'
> in order to match the above changes and allow a memory space
> reduction also in 'struct kernel_info'.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
With the find_unallocated_memory() issue fixed:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
2024-04-10 10:01 ` Michal Orzel
@ 2024-04-10 10:56 ` Luca Fancellu
2024-04-10 11:01 ` Michal Orzel
0 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-10 10:56 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Michal,
> On 10 Apr 2024, at 11:01, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Hi Luca,
>
> On 09/04/2024 13:45, Luca Fancellu wrote:
>>
>>
>> Currently the memory footprint of the static shared memory feature
>> is impacting all the struct meminfo instances with memory space
>> that is not going to be used.
>>
>> To solve this issue, rework the static shared memory extra
>> information linked to the memory bank to another structure,
>> struct shmem_membank_extra, and exploit the struct membank
>> padding to host a pointer to that structure in a union with the
>> enum membank_type, with this trick the 'struct membank' has the
>> same size with or without the static shared memory, given that
>> the 'type' and 'shmem_extra' are never used at the same time,
>> hence the 'struct membank' won't grow in size.
>>
>> Afterwards, create a new structure 'struct shared_meminfo' which
>> has the same interface of 'struct meminfo', but requires less
> I would expect some justification for selecting 32 as the max number of shmem banks
So I have to say I picked up a value I thought was ok for the amount of shared memory
Banks, do you think it is too low? The real intention here was to decouple the number
of shared memory banks from the number of generic memory banks, and I felt 32 was enough,
but if you think it might be an issue I could bump it, or we could have a Kconfig...
>>
>>
>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> With the find_unallocated_memory() issue fixed:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Thanks, I took the opportunity to improve the comment in that function in this way,
adding “ (when the feature is enabled)":
* 3) Remove static shared memory (when the feature is enabled)
Please tell me if that works for you so I will keep your R-by
Cheers,
Luca
>
> ~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
2024-04-10 10:56 ` Luca Fancellu
@ 2024-04-10 11:01 ` Michal Orzel
2024-04-10 11:19 ` Luca Fancellu
0 siblings, 1 reply; 39+ messages in thread
From: Michal Orzel @ 2024-04-10 11:01 UTC (permalink / raw)
To: Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
On 10/04/2024 12:56, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>> On 10 Apr 2024, at 11:01, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Hi Luca,
>>
>> On 09/04/2024 13:45, Luca Fancellu wrote:
>>>
>>>
>>> Currently the memory footprint of the static shared memory feature
>>> is impacting all the struct meminfo instances with memory space
>>> that is not going to be used.
>>>
>>> To solve this issue, rework the static shared memory extra
>>> information linked to the memory bank to another structure,
>>> struct shmem_membank_extra, and exploit the struct membank
>>> padding to host a pointer to that structure in a union with the
>>> enum membank_type, with this trick the 'struct membank' has the
>>> same size with or without the static shared memory, given that
>>> the 'type' and 'shmem_extra' are never used at the same time,
>>> hence the 'struct membank' won't grow in size.
>>>
>>> Afterwards, create a new structure 'struct shared_meminfo' which
>>> has the same interface of 'struct meminfo', but requires less
>> I would expect some justification for selecting 32 as the max number of shmem banks
>
> So I have to say I picked up a value I thought was ok for the amount of shared memory
> Banks, do you think it is too low? The real intention here was to decouple the number
> of shared memory banks from the number of generic memory banks, and I felt 32 was enough,
> but if you think it might be an issue I could bump it, or we could have a Kconfig...
No need for Kconfig. 32 is enough for now but I expect a paragraph in commit msg that you select
32 which should be enough for current use cases and can be bumped in the future in case there is a need.
>
>>>
>>>
>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
>> With the find_unallocated_memory() issue fixed:
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> Thanks, I took the opportunity to improve the comment in that function in this way,
> adding “ (when the feature is enabled)":
>
> * 3) Remove static shared memory (when the feature is enabled)
>
> Please tell me if that works for you so I will keep your R-by
You can retain Rb.
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
2024-04-09 11:45 ` [PATCH v2 10/13] xen/arm: remove shm holes from extended regions Luca Fancellu
@ 2024-04-10 11:15 ` Michal Orzel
2024-04-10 14:08 ` Luca Fancellu
2024-04-10 14:12 ` Luca Fancellu
0 siblings, 2 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-10 11:15 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Penny Zheng, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> From: Penny Zheng <Penny.Zheng@arm.com>
>
> Static shared memory acts as reserved memory in guest, so it shall be
> excluded from extended regions.
>
> Extended regions are taken care of under three different scenarios:
> normal DomU, direct-map domain with iommu on, and direct-map domain
> with iommu off.
>
> For normal DomU, we create a new function "remove_shm_holes_for_domU",
> to firstly transfer original outputs into the format of
> "struct rangeset", then use "remove_shm_from_rangeset" to remove static
> shm from them.
>
> For direct-map domain with iommu on, after we get guest shm info from "kinfo",
> we use "remove_shm_from_rangeset" to remove static shm.
>
> For direct-map domain with iommu off, as static shm has already been taken
> care of through reserved memory banks, we do nothing.
Stale info given that shmem is no longer part of reserved memory banks. It's been taken care
of by removing shmem regions in find_unallocated_memory()
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> v2:
> - Fixed commit title, fixed comment, moved call of remove_shm_from_rangeset
> after populating rangeset in find_memory_holes, print error code when
> possible
> - used PFN_DOWN where needed
> v1:
> - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-Penny.Zheng@arm.com/
> ---
> ---
> xen/arch/arm/domain_build.c | 16 ++++-
> xen/arch/arm/include/asm/domain_build.h | 2 +
> xen/arch/arm/include/asm/static-shmem.h | 18 +++++
> xen/arch/arm/static-shmem.c | 88 +++++++++++++++++++++++++
> 4 files changed, 121 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 01d66fbde92b..9b36d6bb70c9 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -817,8 +817,8 @@ int __init make_memory_node(const struct domain *d,
> return res;
> }
>
> -static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> - void *data)
> +int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> + void *data)
> {
> struct membanks *ext_regions = data;
> paddr_t start, size;
> @@ -978,6 +978,8 @@ static int __init handle_pci_range(const struct dt_device_node *dev,
> * - MMIO
> * - Host RAM
> * - PCI aperture
> + * - Static shared memory regions, which are described by special property
> + * "xen,shared-mem"
> */
> static int __init find_memory_holes(const struct kernel_info *kinfo,
> struct membanks *ext_regions)
> @@ -1005,6 +1007,11 @@ static int __init find_memory_holes(const struct kernel_info *kinfo,
> goto out;
> }
>
> + /* Remove static shared memory regions */
> + res = remove_shm_from_rangeset(kinfo, mem_holes);
> + if ( res )
> + goto out;
> +
> /*
> * Remove regions described by "reg" and "ranges" properties where
> * the memory is addressable (MMIO, RAM, PCI BAR, etc).
> @@ -1097,7 +1104,10 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
> res = 0;
> }
>
> - return res;
> + if ( res )
> + return res;
> +
> + return remove_shm_holes_for_domU(kinfo, ext_regions);
> }
>
> int __init make_hypervisor_node(struct domain *d,
> diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
> index a6f276cc4263..026d975da28e 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -51,6 +51,8 @@ static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
> int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
> #endif
>
> +int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
> +
> #endif
>
> /*
> diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
> index 90aafc81e740..2e8b138eb989 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -28,6 +28,12 @@ void early_print_info_shmem(void);
>
> void init_sharedmem_pages(void);
>
> +int remove_shm_from_rangeset(const struct kernel_info *kinfo,
> + struct rangeset *rangeset);
> +
> +int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
> + struct membanks *ext_regions);
> +
> #else /* !CONFIG_STATIC_SHM */
>
> static inline int make_resv_memory_node(const struct kernel_info *kinfo,
> @@ -59,6 +65,18 @@ static inline void early_print_info_shmem(void) {};
>
> static inline void init_sharedmem_pages(void) {};
>
> +static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo,
> + struct rangeset *rangeset)
> +{
> + return 0;
> +}
> +
> +static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
> + struct membanks *ext_regions)
> +{
> + return 0;
> +}
> +
> #endif /* CONFIG_STATIC_SHM */
>
> #endif /* __ASM_STATIC_SHMEM_H_ */
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index fcf8227bf404..a08413ff44e2 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
>
> #include <xen/libfdt/libfdt.h>
> +#include <xen/rangeset.h>
> #include <xen/sched.h>
>
> #include <asm/domain_build.h>
> @@ -574,6 +575,93 @@ void __init init_sharedmem_pages(void)
> init_staticmem_bank(&shmem->bank[bank]);
> }
>
> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
> + struct rangeset *rangeset)
> +{
> + const struct membanks *shm_mem = &kinfo->shm_mem.common;
> + unsigned int i;
> +
> + /* Remove static shared memory regions */
> + for ( i = 0; i < shm_mem->nr_banks; i++ )
> + {
> + paddr_t start, end;
> + int res;
> +
> + start = shm_mem->bank[i].start;
> + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
If you look at other rangeset_remove_range use cases and error messages, 1 is subtracted
in PFN_DOWN() so that the error message contains end unchanged. Please adhere to that so that
printed messages are consistent.
> + res = rangeset_remove_range(rangeset, PFN_DOWN(start), PFN_DOWN(end));
> + if ( res )
> + {
> + printk(XENLOG_ERR
> + "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr", error: %d\n",
> + start, end, res);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
> + struct membanks *ext_regions)
> +{
> + const struct membanks *shm_mem = &kinfo->shm_mem.common;
> + struct rangeset *guest_holes;
> + unsigned int i;
> + paddr_t start;
> + paddr_t end;
> + int res;
> +
> + /* No static shared memory region. */
> + if ( shm_mem->nr_banks == 0 )
> + return 0;
> +
> + dt_dprintk("Remove static shared memory holes from extended regions of DomU\n");
> +
> + guest_holes = rangeset_new(NULL, NULL, 0);
> + if ( !guest_holes )
> + return -ENOMEM;
> +
> + /* Copy extended regions sets into the rangeset */
> + for ( i = 0; i < ext_regions->nr_banks; i++ )
> + {
> + start = ext_regions->bank[i].start;
> + end = start + ext_regions->bank[i].size - 1;
> +
> + res = rangeset_add_range(guest_holes, PFN_DOWN(start), PFN_DOWN(end));
same remark about the end
> + if ( res )
> + {
> + printk(XENLOG_ERR
> + "Failed to add: %#"PRIpaddr"->%#"PRIpaddr", error: %d\n",
> + start, end, res);
> + goto out;
> + }
> + }
> +
> + /* Remove static shared memory regions */
> + res = remove_shm_from_rangeset(kinfo, guest_holes);
> + if ( res )
> + goto out;
> +
Could you please add a comment explaining here what's done below?
> + i = ext_regions->nr_banks - 1;
> + start = ext_regions->bank[0].start;
> + end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
> +
> + /* Reset original extended regions to hold new value */
> + ext_regions->nr_banks = 0;
> + res = rangeset_report_ranges(guest_holes, PFN_DOWN(start), PFN_DOWN(end),
> + add_ext_regions, ext_regions);
> + if ( res )
> + ext_regions->nr_banks = 0;
> + else if ( !ext_regions->nr_banks )
> + res = -ENOENT;
> +
> + out:
> + rangeset_destroy(guest_holes);
> +
> + return res;
> +}
> +
> /*
> * Local variables:
> * mode: C
> --
> 2.34.1
>
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
2024-04-10 11:01 ` Michal Orzel
@ 2024-04-10 11:19 ` Luca Fancellu
2024-04-10 11:23 ` Michal Orzel
0 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-10 11:19 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
>>>>
>>>> Afterwards, create a new structure 'struct shared_meminfo' which
>>>> has the same interface of 'struct meminfo', but requires less
>>> I would expect some justification for selecting 32 as the max number of shmem banks
>>
>> So I have to say I picked up a value I thought was ok for the amount of shared memory
>> Banks, do you think it is too low? The real intention here was to decouple the number
>> of shared memory banks from the number of generic memory banks, and I felt 32 was enough,
>> but if you think it might be an issue I could bump it, or we could have a Kconfig...
> No need for Kconfig. 32 is enough for now but I expect a paragraph in commit msg that you select
> 32 which should be enough for current use cases and can be bumped in the future in case there is a need.
What do you think of this proposal:
[...]
hence the 'struct membank' won't grow in size.
Afterwards, create a new structure 'struct shared_meminfo' which
has the same interface of 'struct meminfo', but requires less
banks, defined by the number in NR_SHMEM_BANKS, which is 32 at the
moment and should be enough for the current use cases, the value
might be increased in te future if needed.
Finally, this structure hosts also the extra information for the
static shared memory banks.
The fields 'bank' and 'extra' of this structure are meant to be
[...]
Cheers,
Luca
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
2024-04-10 11:19 ` Luca Fancellu
@ 2024-04-10 11:23 ` Michal Orzel
0 siblings, 0 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-10 11:23 UTC (permalink / raw)
To: Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk
On 10/04/2024 13:19, Luca Fancellu wrote:
>
>
>>>>>
>>>>> Afterwards, create a new structure 'struct shared_meminfo' which
>>>>> has the same interface of 'struct meminfo', but requires less
>>>> I would expect some justification for selecting 32 as the max number of shmem banks
>>>
>>> So I have to say I picked up a value I thought was ok for the amount of shared memory
>>> Banks, do you think it is too low? The real intention here was to decouple the number
>>> of shared memory banks from the number of generic memory banks, and I felt 32 was enough,
>>> but if you think it might be an issue I could bump it, or we could have a Kconfig...
>> No need for Kconfig. 32 is enough for now but I expect a paragraph in commit msg that you select
>> 32 which should be enough for current use cases and can be bumped in the future in case there is a need.
>
> What do you think of this proposal:
>
> [...]
> hence the 'struct membank' won't grow in size.
>
> Afterwards, create a new structure 'struct shared_meminfo' which
> has the same interface of 'struct meminfo', but requires less
> banks, defined by the number in NR_SHMEM_BANKS, which is 32 at the
> moment and should be enough for the current use cases, the value
> might be increased in te future if needed.
> Finally, this structure hosts also the extra information for the
> static shared memory banks.
> The fields 'bank' and 'extra' of this structure are meant to be
> [...]
reads ok
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
2024-04-10 11:15 ` Michal Orzel
@ 2024-04-10 14:08 ` Luca Fancellu
2024-04-10 14:58 ` Michal Orzel
2024-04-10 14:12 ` Luca Fancellu
1 sibling, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-10 14:08 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
>>
>> For direct-map domain with iommu on, after we get guest shm info from "kinfo",
>> we use "remove_shm_from_rangeset" to remove static shm.
>>
>> For direct-map domain with iommu off, as static shm has already been taken
>> care of through reserved memory banks, we do nothing.
> Stale info given that shmem is no longer part of reserved memory banks. It's been taken care
> of by removing shmem regions in find_unallocated_memory()
Sure, will amend for this:
>>
>> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>> + struct rangeset *rangeset)
>> +{
>> + const struct membanks *shm_mem = &kinfo->shm_mem.common;
>> + unsigned int i;
>> +
>> + /* Remove static shared memory regions */
>> + for ( i = 0; i < shm_mem->nr_banks; i++ )
>> + {
>> + paddr_t start, end;
>> + int res;
>> +
>> + start = shm_mem->bank[i].start;
>> + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
> If you look at other rangeset_remove_range use cases and error messages, 1 is subtracted
> in PFN_DOWN() so that the error message contains end unchanged. Please adhere to that so that
> printed messages are consistent.
Yes I will change it to have -1 inside PFN_DOWN(), here and in the other occurrences
>>
>> + /* Remove static shared memory regions */
>> + res = remove_shm_from_rangeset(kinfo, guest_holes);
>> + if ( res )
>> + goto out;
>> +
> Could you please add a comment explaining here what's done below?
Is it ok something like this:
/*
* Take the interval of memory starting from the first extended region bank
* start address and ending to the end of the last extended region bank.
* The interval will be passed to rangeset_report_ranges to allow it to
* create, by the add_ext_regions callback, a set of extended memory region
* banks from the guest_holes rangeset, which contains the original extended
* memory region ranges where the static shared memory ranges are carved
* out.
*/
>> + i = ext_regions->nr_banks - 1;
>> + start = ext_regions->bank[0].start;
>> + end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
>> +
>> + /* Reset original extended regions to hold new value */
>> + ext_regions->nr_banks = 0;
>> + res = rangeset_report_ranges(guest_holes, PFN_DOWN(start), PFN_DOWN(end),
>> + add_ext_regions, ext_regions);
>> + if ( res )
>> + ext_regions->nr_banks = 0;
>> + else if ( !ext_regions->nr_banks )
>> + res = -ENOENT;
>> +
>> + out:
>> + rangeset_destroy(guest_holes);
>> +
>> + return res;
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> --
>> 2.34.1
>>
>
> ~Michal
>
Cheers,
Luca
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
2024-04-10 11:15 ` Michal Orzel
2024-04-10 14:08 ` Luca Fancellu
@ 2024-04-10 14:12 ` Luca Fancellu
1 sibling, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-10 14:12 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
>>
>> For direct-map domain with iommu off, as static shm has already been taken
>> care of through reserved memory banks, we do nothing.
> Stale info given that shmem is no longer part of reserved memory banks. It's been taken care
> of by removing shmem regions in find_unallocated_memory()
Sorry, in the previous mail it didn’t paste my proposed fix:
For direct-map domain with iommu off, as static shm has already been taken
care of through find_unallocated_memory, we do nothing.
Cheers,
Luca
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
2024-04-10 14:08 ` Luca Fancellu
@ 2024-04-10 14:58 ` Michal Orzel
0 siblings, 0 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-10 14:58 UTC (permalink / raw)
To: Luca Fancellu
Cc: Xen-devel, Penny Zheng, Stefano Stabellini, Julien Grall,
Bertrand Marquis, Volodymyr Babchuk
Hi Luca,
On 10/04/2024 16:08, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>>>
>>> For direct-map domain with iommu on, after we get guest shm info from "kinfo",
>>> we use "remove_shm_from_rangeset" to remove static shm.
>>>
>>> For direct-map domain with iommu off, as static shm has already been taken
>>> care of through reserved memory banks, we do nothing.
>> Stale info given that shmem is no longer part of reserved memory banks. It's been taken care
>> of by removing shmem regions in find_unallocated_memory()
>
> Sure, will amend for this:
>
>>>
>>> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>>> + struct rangeset *rangeset)
>>> +{
>>> + const struct membanks *shm_mem = &kinfo->shm_mem.common;
>>> + unsigned int i;
>>> +
>>> + /* Remove static shared memory regions */
>>> + for ( i = 0; i < shm_mem->nr_banks; i++ )
>>> + {
>>> + paddr_t start, end;
>>> + int res;
>>> +
>>> + start = shm_mem->bank[i].start;
>>> + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
>> If you look at other rangeset_remove_range use cases and error messages, 1 is subtracted
>> in PFN_DOWN() so that the error message contains end unchanged. Please adhere to that so that
>> printed messages are consistent.
>
> Yes I will change it to have -1 inside PFN_DOWN(), here and in the other occurrences
>>>
>>> + /* Remove static shared memory regions */
>>> + res = remove_shm_from_rangeset(kinfo, guest_holes);
>>> + if ( res )
>>> + goto out;
>>> +
>> Could you please add a comment explaining here what's done below?
>
> Is it ok something like this:
I'm ok with your proposal in the other e-mail.
>
> /*
> * Take the interval of memory starting from the first extended region bank
> * start address and ending to the end of the last extended region bank.
I would stop here. The rest reads quite difficult.
> * The interval will be passed to rangeset_report_ranges to allow it to
> * create, by the add_ext_regions callback, a set of extended memory region
> * banks from the guest_holes rangeset, which contains the original extended
> * memory region ranges where the static shared memory ranges are carved
> * out.
> */
>
>>> + i = ext_regions->nr_banks - 1;
>>> + start = ext_regions->bank[0].start;
>>> + end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
>>> +
>>> + /* Reset original extended regions to hold new value */
>>> + ext_regions->nr_banks = 0;
>>> + res = rangeset_report_ranges(guest_holes, PFN_DOWN(start), PFN_DOWN(end),
>>> + add_ext_regions, ext_regions);
>>> + if ( res )
>>> + ext_regions->nr_banks = 0;
>>> + else if ( !ext_regions->nr_banks )
>>> + res = -ENOENT;
>>> +
>>> + out:
>>> + rangeset_destroy(guest_holes);
>>> +
>>> + return res;
>>> +}
>>> +
>>> /*
>>> * Local variables:
>>> * mode: C
>>> --
>>> 2.34.1
>>>
>>
>> ~Michal
>>
>
> Cheers,
> Luca
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
2024-04-09 11:45 ` [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals Luca Fancellu
@ 2024-04-11 9:50 ` Luca Fancellu
2024-04-11 13:36 ` Luca Fancellu
2024-04-18 6:28 ` Jan Beulich
1 sibling, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-11 9:50 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Roger Pau Monné
> On 9 Apr 2024, at 12:45, Luca Fancellu <Luca.Fancellu@arm.com> wrote:
>
> Introduce a function that given an array of cells containing
> (address,size) intervals, merges the overlapping ones, returning
> an array with no overlapping intervals.
>
> The algorithm needs to sort the intervals by ascending order
> address, so the sort() function already included in the codebase
> is used, however in this case additional data is needed for the
> compare function, to be able to extract the address from the
> interval.
> So add one argument to the sort() function and its compare
> callback to have additional data and be able to pass, in this
> case, the address length. In case the argument is not needed,
> NULL can be provided.
>
> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
Hi all,
I’ve just spotted an issue with the algorithm, the fix is this one:
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 24914a80d03b..262385a041a8 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -2360,6 +2360,10 @@ int __init dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells,
__be32 *tmp_last_cell_size = last_cell + addrcells;
dt_set_cell(&tmp_last_cell_size, sizecells, new_size);
+
+ /* Last interval updated, so the end is changed */
+ end_last = start_last + size_last;
+
/*
* This current interval is merged with the last one, so remove this
* interval and shift left all the remaining elements
--------------------------------
Now, I would like to write something about the algorithm to ease the reviewers,
the problem is that we have some intervals and we would like to merge the overlapping
ones, a simple algorithm can be found here: https://www.interviewbit.com/blog/merge-intervals/
Limitation now is that when merging the intervals, we don’t want to exceed the space needed
to store the information, for example:
sizecells: 1 (meaning one __be32, 4 byte)
Int1: start 0x0, size 0xFFFFFFFF
Int2: start 0xFFFFFFFF, size 0x1000
We can’t merge them because the new size would be over 4 byte.
During the development of this algorithm I’ve prototyped it in Python, I’ll attach my script here
so that it’s easier to understand:
#!/usr/bin/env python3
def merge_intervals_inplace(intervals, size_limit):
merged = intervals[:]
last_idx = 0
i = 1
count = len(merged)
if count == 1:
return merged
last_cell = merged[last_idx]
start_last = last_cell[0]
size_last = last_cell[1]
end_last = start_last + size_last
while i < count:
start_current = merged[i][0]
size_current = merged[i][1]
end_current = start_current + size_current
overlap = end_last >= start_current
new_size = max(end_last, end_current) - start_last
#print((f"last ({start_last},{end_last}),"
# f" curr ({start_current},{end_current}),"
# f" newsize: {new_size}"
# ))
# If the current interval doesn't overlap with the last one, or even if
# they overlap but the computed new size would be over the imposed
# limit, then advance the last element by one position
if (not overlap) or (overlap and new_size > size_limit):
#print("advance last")
last_idx += 1
last_cell = merged[last_idx]
start_last = last_cell[0]
size_last = last_cell[1]
end_last = start_last + size_last
else:
#print("merge")
# Set new size for the last element, merging the last interval with
# the current one
merged[last_idx] = (start_last, new_size)
# Update last elem interval end
end_last = start_last + new_size
# The current interval (i) is merged with the last, so remove it and
# shift left all the remaining intervals
merged = merged[:i] + merged[i+1:]
# Now the array has less element since we merged two intervals
count -= 1
# Next iteration needs to start from the current index, skip
# increment
continue
i += 1
return merged
def print_interval(intervals):
print("[", end='')
for interval in intervals:
s = interval[0]
sz = interval[1]
print(f" ({s},{sz}) ", end='')
print("] -> ", end='')
print("[", end='')
for interval in intervals:
s = interval[0]
e = interval[0] + interval[1]
print(f" ({s},{e}) ", end='')
print("]")
def main(argv):
limit=20
# Array of intervals (start address, size)
#banks = [(0,2), (5,1), (0,10), (10,7), (2,6)]
banks = [(0,20), (20,5), (10,15), (5,15)]
for interval in banks:
if interval[1] > limit:
raise Exception(f"{interval} size > limit ({limit})")
# Sort by start address ascending order
banks.sort(key=lambda a: a[0])
print("IN (sorted) [(start,size)] -> [(start,end)]")
print_interval(banks)
banks = merge_intervals_inplace(banks, limit)
print("OUT [(start,size)] -> [(start,end)]")
print_interval(banks)
if __name__ == "__main__":
main(sys.argv[1:])
Cheers,
Luca
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
2024-04-11 9:50 ` Luca Fancellu
@ 2024-04-11 13:36 ` Luca Fancellu
0 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-11 13:36 UTC (permalink / raw)
To: Xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Roger Pau Monné
>
> I’ve just spotted an issue with the algorithm, the fix is this one:
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 24914a80d03b..262385a041a8 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -2360,6 +2360,10 @@ int __init dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells,
> __be32 *tmp_last_cell_size = last_cell + addrcells;
>
> dt_set_cell(&tmp_last_cell_size, sizecells, new_size);
> +
> + /* Last interval updated, so the end is changed */
> + end_last = start_last + size_last;
> +
> /*
> * This current interval is merged with the last one, so remove this
> * interval and shift left all the remaining elements
>
Apologies, this is the fix:
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 24914a80d03b..9a2f5b27aa9b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -2360,6 +2360,10 @@ int __init dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells,
__be32 *tmp_last_cell_size = last_cell + addrcells;
dt_set_cell(&tmp_last_cell_size, sizecells, new_size);
+
+ /* Last interval updated, so the end is changed */
+ end_last = start_last + new_size;
+
/*
* This current interval is merged with the last one, so remove this
* interval and shift left all the remaining elements
So instead of “size_last” -> “new_size”.
Sorry for the noise.
Cheers,
Luca
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-09 11:45 ` [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes Luca Fancellu
@ 2024-04-15 18:41 ` Julien Grall
2024-04-16 6:27 ` Luca Fancellu
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2024-04-15 18:41 UTC (permalink / raw)
To: Luca Fancellu, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Michal Orzel, Volodymyr Babchuk
Hi Luca,
On 09/04/2024 12:45, Luca Fancellu wrote:
> Currently Xen is not exporting the static shared memory regions
> to the device tree as /memory node, this commit is fixing this
> issue.
>
> The static shared memory banks can be part of the memory range
> available for the domain, so if they are overlapping with the
> normal memory banks, they need to be merged together in order
> to produce a /memory node with non overlapping ranges in 'reg'.
Before reviewing the code in more details, I would like to understand a
bit more the use case and whether it should be valid.
From my understanding, the case you are trying to prevent is the
following setup:
1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
The underlying Host Physical regions may be different. Xen doesn't
guarantee in which order the regions will be mapped, So whether the
overlapped region will point to the memory or the shared region is
unknown (we don't guarantee the order of the mapping). So nothing good
will happen to the guest.
Did I understand correctly? If so, shouldn't this be a configuration we
should forbid?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-15 18:41 ` Julien Grall
@ 2024-04-16 6:27 ` Luca Fancellu
2024-04-16 8:50 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-16 6:27 UTC (permalink / raw)
To: Julien Grall
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
Hi Julien,
> On 15 Apr 2024, at 19:41, Julien Grall <julien@xen.org> wrote:
>
> Hi Luca,
>
> On 09/04/2024 12:45, Luca Fancellu wrote:
>> Currently Xen is not exporting the static shared memory regions
>> to the device tree as /memory node, this commit is fixing this
>> issue.
>> The static shared memory banks can be part of the memory range
>> available for the domain, so if they are overlapping with the
>> normal memory banks, they need to be merged together in order
>> to produce a /memory node with non overlapping ranges in 'reg'.
>
> Before reviewing the code in more details, I would like to understand a bit more the use case and whether it should be valid.
>
> From my understanding, the case you are trying to prevent is the following setup:
> 1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
> 2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
So far, it was possible to map guest physical regions inside the memory range given to the guest,
so the above configuration was allowed and the underlying host physical regions were of course
different and enforced with checks. So I’m not trying to prevent this behaviour, however ...
>
> The underlying Host Physical regions may be different. Xen doesn't guarantee in which order the regions will be mapped, So whether the overlapped region will point to the memory or the shared region is unknown (we don't guarantee the order of the mapping). So nothing good will happen to the guest.
... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that
guest physical regions for static shared memory are outside the memory given to the guest?
>
> Did I understand correctly? If so, shouldn't this be a configuration we should forbid?
>
> Cheers,
>
> --
> Julien Grall
Cheers,
Luca
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-16 6:27 ` Luca Fancellu
@ 2024-04-16 8:50 ` Julien Grall
2024-04-16 8:57 ` Michal Orzel
2024-04-16 8:59 ` Luca Fancellu
0 siblings, 2 replies; 39+ messages in thread
From: Julien Grall @ 2024-04-16 8:50 UTC (permalink / raw)
To: Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
On 16/04/2024 07:27, Luca Fancellu wrote:
> Hi Julien,
Hi Luca,
>
>> On 15 Apr 2024, at 19:41, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Luca,
>>
>> On 09/04/2024 12:45, Luca Fancellu wrote:
>>> Currently Xen is not exporting the static shared memory regions
>>> to the device tree as /memory node, this commit is fixing this
>>> issue.
>>> The static shared memory banks can be part of the memory range
>>> available for the domain, so if they are overlapping with the
>>> normal memory banks, they need to be merged together in order
>>> to produce a /memory node with non overlapping ranges in 'reg'.
>>
>> Before reviewing the code in more details, I would like to understand a bit more the use case and whether it should be valid.
>>
>> From my understanding, the case you are trying to prevent is the following setup:
>> 1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
>> 2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
>
> So far, it was possible to map guest physical regions inside the memory range given to the guest,
> so the above configuration was allowed and the underlying host physical regions were of course
> different and enforced with checks. So I’m not trying to prevent this behaviour, however ...
>
>>
>> The underlying Host Physical regions may be different. Xen doesn't guarantee in which order the regions will be mapped, So whether the overlapped region will point to the memory or the shared region is unknown (we don't guarantee the order of the mapping). So nothing good will happen to the guest.
>
> ... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that
> guest physical regions for static shared memory are outside the memory given to the guest?
Nothing good will happen if you are trying to overwrite mappings. So I
think this should be enforced. However, this is a more general problem.
At the moment, this is pretty much as mess because you can overwrite any
mapping (e.g. map MMIO on top of the RAM).
I think the easiest way to enforce is to do it in the P2M code like x86
does for certain mappings.
Anyway, I don't think the problem should be solved here or by you (this
is likely going to be a can of worms). For now, I would consider to
simply drop the patches that are trying to do the merge.
Any thoughts?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-16 8:50 ` Julien Grall
@ 2024-04-16 8:57 ` Michal Orzel
2024-04-16 8:59 ` Luca Fancellu
1 sibling, 0 replies; 39+ messages in thread
From: Michal Orzel @ 2024-04-16 8:57 UTC (permalink / raw)
To: Julien Grall, Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk
Hi Julien,
On 16/04/2024 10:50, Julien Grall wrote:
>
>
> On 16/04/2024 07:27, Luca Fancellu wrote:
>> Hi Julien,
>
> Hi Luca,
>
>>
>>> On 15 Apr 2024, at 19:41, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Luca,
>>>
>>> On 09/04/2024 12:45, Luca Fancellu wrote:
>>>> Currently Xen is not exporting the static shared memory regions
>>>> to the device tree as /memory node, this commit is fixing this
>>>> issue.
>>>> The static shared memory banks can be part of the memory range
>>>> available for the domain, so if they are overlapping with the
>>>> normal memory banks, they need to be merged together in order
>>>> to produce a /memory node with non overlapping ranges in 'reg'.
>>>
>>> Before reviewing the code in more details, I would like to understand a bit more the use case and whether it should be valid.
>>>
>>> From my understanding, the case you are trying to prevent is the following setup:
>>> 1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
>>> 2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
>>
>> So far, it was possible to map guest physical regions inside the memory range given to the guest,
>> so the above configuration was allowed and the underlying host physical regions were of course
>> different and enforced with checks. So I’m not trying to prevent this behaviour, however ...
>>
>>>
>>> The underlying Host Physical regions may be different. Xen doesn't guarantee in which order the regions will be mapped, So whether the overlapped region will point to the memory or the shared region is unknown (we don't guarantee the order of the mapping). So nothing good will happen to the guest.
>>
>> ... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that
>> guest physical regions for static shared memory are outside the memory given to the guest?
>
> Nothing good will happen if you are trying to overwrite mappings. So I
> think this should be enforced. However, this is a more general problem.
> At the moment, this is pretty much as mess because you can overwrite any
> mapping (e.g. map MMIO on top of the RAM).
>
> I think the easiest way to enforce is to do it in the P2M code like x86
> does for certain mappings.
>
> Anyway, I don't think the problem should be solved here or by you (this
> is likely going to be a can of worms). For now, I would consider to
> simply drop the patches that are trying to do the merge.
>
> Any thoughts?
I agree with your analysis. For now, let's drop this patch.
@Luca: This means I reviewed your series completely and you can send a respin.
~Michal
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-16 8:50 ` Julien Grall
2024-04-16 8:57 ` Michal Orzel
@ 2024-04-16 8:59 ` Luca Fancellu
2024-04-16 9:06 ` Julien Grall
1 sibling, 1 reply; 39+ messages in thread
From: Luca Fancellu @ 2024-04-16 8:59 UTC (permalink / raw)
To: Julien Grall
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
> On 16 Apr 2024, at 09:50, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 16/04/2024 07:27, Luca Fancellu wrote:
>> Hi Julien,
>
> Hi Luca,
>
>>> On 15 Apr 2024, at 19:41, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Luca,
>>>
>>> On 09/04/2024 12:45, Luca Fancellu wrote:
>>>> Currently Xen is not exporting the static shared memory regions
>>>> to the device tree as /memory node, this commit is fixing this
>>>> issue.
>>>> The static shared memory banks can be part of the memory range
>>>> available for the domain, so if they are overlapping with the
>>>> normal memory banks, they need to be merged together in order
>>>> to produce a /memory node with non overlapping ranges in 'reg'.
>>>
>>> Before reviewing the code in more details, I would like to understand a bit more the use case and whether it should be valid.
>>>
>>> From my understanding, the case you are trying to prevent is the following setup:
>>> 1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
>>> 2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
>> So far, it was possible to map guest physical regions inside the memory range given to the guest,
>> so the above configuration was allowed and the underlying host physical regions were of course
>> different and enforced with checks. So I’m not trying to prevent this behaviour, however ...
>>>
>>> The underlying Host Physical regions may be different. Xen doesn't guarantee in which order the regions will be mapped, So whether the overlapped region will point to the memory or the shared region is unknown (we don't guarantee the order of the mapping). So nothing good will happen to the guest.
>> ... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that
>> guest physical regions for static shared memory are outside the memory given to the guest?
>
> Nothing good will happen if you are trying to overwrite mappings. So I think this should be enforced. However, this is a more general problem. At the moment, this is pretty much as mess because you can overwrite any mapping (e.g. map MMIO on top of the RAM).
>
> I think the easiest way to enforce is to do it in the P2M code like x86 does for certain mappings.
>
> Anyway, I don't think the problem should be solved here or by you (this is likely going to be a can of worms). For now, I would consider to simply drop the patches that are trying to do the merge.
>
> Any thoughts?
Yes I agree with you, I’ll drop the patch that tries to do the merge, I was thinking about checking that the guest phys static mem region is
inside these boundaries:
#define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
#define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
and also that doesn’t overlap with (struct kernel_info).mem, does it sounds right to you?
So in this patch I will only populate the /memory nodes with the static shared memory banks.
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-16 8:59 ` Luca Fancellu
@ 2024-04-16 9:06 ` Julien Grall
2024-04-16 10:50 ` Luca Fancellu
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2024-04-16 9:06 UTC (permalink / raw)
To: Luca Fancellu
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
On 16/04/2024 09:59, Luca Fancellu wrote:
>
>
>> On 16 Apr 2024, at 09:50, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 16/04/2024 07:27, Luca Fancellu wrote:
>>> Hi Julien,
>>
>> Hi Luca,
>>
>>>> On 15 Apr 2024, at 19:41, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Luca,
>>>>
>>>> On 09/04/2024 12:45, Luca Fancellu wrote:
>>>>> Currently Xen is not exporting the static shared memory regions
>>>>> to the device tree as /memory node, this commit is fixing this
>>>>> issue.
>>>>> The static shared memory banks can be part of the memory range
>>>>> available for the domain, so if they are overlapping with the
>>>>> normal memory banks, they need to be merged together in order
>>>>> to produce a /memory node with non overlapping ranges in 'reg'.
>>>>
>>>> Before reviewing the code in more details, I would like to understand a bit more the use case and whether it should be valid.
>>>>
>>>> From my understanding, the case you are trying to prevent is the following setup:
>>>> 1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
>>>> 2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
>>> So far, it was possible to map guest physical regions inside the memory range given to the guest,
>>> so the above configuration was allowed and the underlying host physical regions were of course
>>> different and enforced with checks. So I’m not trying to prevent this behaviour, however ...
>>>>
>>>> The underlying Host Physical regions may be different. Xen doesn't guarantee in which order the regions will be mapped, So whether the overlapped region will point to the memory or the shared region is unknown (we don't guarantee the order of the mapping). So nothing good will happen to the guest.
>>> ... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that
>>> guest physical regions for static shared memory are outside the memory given to the guest?
>>
>> Nothing good will happen if you are trying to overwrite mappings. So I think this should be enforced. However, this is a more general problem. At the moment, this is pretty much as mess because you can overwrite any mapping (e.g. map MMIO on top of the RAM).
>>
>> I think the easiest way to enforce is to do it in the P2M code like x86 does for certain mappings.
>>
>> Anyway, I don't think the problem should be solved here or by you (this is likely going to be a can of worms). For now, I would consider to simply drop the patches that are trying to do the merge.
>>
>> Any thoughts?
>
> Yes I agree with you, I’ll drop the patch that tries to do the merge, I was thinking about checking that the guest phys static mem region is
> inside these boundaries:
>
> #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
> #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>
> and also that doesn’t overlap with (struct kernel_info).mem, does it sounds right to you?
I don't fully understand what you want to do. But as I wrote before, the
overlaps happen with many different regions (what if you try to use the
GIC Distributor regions for the shared memory?).
So if we want some overlaps check, then it has to be generic.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
2024-04-16 9:06 ` Julien Grall
@ 2024-04-16 10:50 ` Luca Fancellu
0 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-16 10:50 UTC (permalink / raw)
To: Julien Grall
Cc: Xen-devel, Stefano Stabellini, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk
> On 16 Apr 2024, at 10:06, Julien Grall <julien@xen.org> wrote:
>
>
>
> On 16/04/2024 09:59, Luca Fancellu wrote:
>>> On 16 Apr 2024, at 09:50, Julien Grall <julien@xen.org> wrote:
>>>
>>>
>>>
>>> On 16/04/2024 07:27, Luca Fancellu wrote:
>>>> Hi Julien,
>>>
>>> Hi Luca,
>>>
>>>>> On 15 Apr 2024, at 19:41, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>> Hi Luca,
>>>>>
>>>>> On 09/04/2024 12:45, Luca Fancellu wrote:
>>>>>> Currently Xen is not exporting the static shared memory regions
>>>>>> to the device tree as /memory node, this commit is fixing this
>>>>>> issue.
>>>>>> The static shared memory banks can be part of the memory range
>>>>>> available for the domain, so if they are overlapping with the
>>>>>> normal memory banks, they need to be merged together in order
>>>>>> to produce a /memory node with non overlapping ranges in 'reg'.
>>>>>
>>>>> Before reviewing the code in more details, I would like to understand a bit more the use case and whether it should be valid.
>>>>>
>>>>> From my understanding, the case you are trying to prevent is the following setup:
>>>>> 1. The Guest Physical region 0x0000 to 0x8000 is used for RAM
>>>>> 2. The Guest Physical region 0x0000 to 0x4000 is used for static memory
>>>> So far, it was possible to map guest physical regions inside the memory range given to the guest,
>>>> so the above configuration was allowed and the underlying host physical regions were of course
>>>> different and enforced with checks. So I’m not trying to prevent this behaviour, however ...
>>>>>
>>>>> The underlying Host Physical regions may be different. Xen doesn't guarantee in which order the regions will be mapped, So whether the overlapped region will point to the memory or the shared region is unknown (we don't guarantee the order of the mapping). So nothing good will happen to the guest.
>>>> ... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that
>>>> guest physical regions for static shared memory are outside the memory given to the guest?
>>>
>>> Nothing good will happen if you are trying to overwrite mappings. So I think this should be enforced. However, this is a more general problem. At the moment, this is pretty much as mess because you can overwrite any mapping (e.g. map MMIO on top of the RAM).
>>>
>>> I think the easiest way to enforce is to do it in the P2M code like x86 does for certain mappings.
>>>
>>> Anyway, I don't think the problem should be solved here or by you (this is likely going to be a can of worms). For now, I would consider to simply drop the patches that are trying to do the merge.
>>>
>>> Any thoughts?
>> Yes I agree with you, I’ll drop the patch that tries to do the merge, I was thinking about checking that the guest phys static mem region is
>> inside these boundaries:
>> #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
>> #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>> and also that doesn’t overlap with (struct kernel_info).mem, does it sounds right to you?
>
> I don't fully understand what you want to do. But as I wrote before, the overlaps happen with many different regions (what if you try to use the GIC Distributor regions for the shared memory?).
>
> So if we want some overlaps check, then it has to be generic.
After a chat in Matrix now I understand what you mean, thanks, I will just drop the patch 12 and update this one.
Cheers,
Luca
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
2024-04-09 11:45 ` [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals Luca Fancellu
2024-04-11 9:50 ` Luca Fancellu
@ 2024-04-18 6:28 ` Jan Beulich
2024-04-18 7:44 ` Luca Fancellu
1 sibling, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2024-04-18 6:28 UTC (permalink / raw)
To: Luca Fancellu
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Roger Pau Monné,
xen-devel
On 09.04.2024 13:45, Luca Fancellu wrote:
> --- a/xen/arch/x86/extable.c
> +++ b/xen/arch/x86/extable.c
> @@ -23,7 +23,8 @@ static inline unsigned long ex_cont(const struct exception_table_entry *x)
> return EX_FIELD(x, cont);
> }
>
> -static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b)
> +static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b,
> + const void *data)
> {
> const struct exception_table_entry *l = a, *r = b;
> unsigned long lip = ex_addr(l);
> @@ -53,7 +54,7 @@ void init_or_livepatch sort_exception_table(struct exception_table_entry *start,
> const struct exception_table_entry *stop)
> {
> sort(start, stop - start,
> - sizeof(struct exception_table_entry), cmp_ex, swap_ex);
> + sizeof(struct exception_table_entry), cmp_ex, swap_ex, NULL);
> }
Not the least because of this addition of an entirely useless parameter / argument
I'm not in favor of ...
> --- a/xen/include/xen/sort.h
> +++ b/xen/include/xen/sort.h
> @@ -23,8 +23,8 @@
> extern gnu_inline
> #endif
> void sort(void *base, size_t num, size_t size,
> - int (*cmp)(const void *a, const void *b),
> - void (*swap)(void *a, void *b, size_t size))
> + int (*cmp)(const void *a, const void *b, const void *data),
> + void (*swap)(void *a, void *b, size_t size), const void *cmp_data)
> {
... this change. Consider you were doing this on a C library you cannot change.
You'd have to find a different solution anyway. And the way we have sort()
right now is matching the C spec. The change to do renders things unexpected to
anyone wanting to use this function in a spec-compliant way. One approach may
be to make an adjustment to data representation, such that the extra reference
data is accessible through the pointers already being passed.
Jan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
2024-04-18 6:28 ` Jan Beulich
@ 2024-04-18 7:44 ` Luca Fancellu
0 siblings, 0 replies; 39+ messages in thread
From: Luca Fancellu @ 2024-04-18 7:44 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Michal Orzel,
Volodymyr Babchuk, Andrew Cooper, George Dunlap,
Roger Pau Monné,
xen-devel
> On 18 Apr 2024, at 07:28, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 09.04.2024 13:45, Luca Fancellu wrote:
>> --- a/xen/arch/x86/extable.c
>> +++ b/xen/arch/x86/extable.c
>> @@ -23,7 +23,8 @@ static inline unsigned long ex_cont(const struct exception_table_entry *x)
>> return EX_FIELD(x, cont);
>> }
>>
>> -static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b)
>> +static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b,
>> + const void *data)
>> {
>> const struct exception_table_entry *l = a, *r = b;
>> unsigned long lip = ex_addr(l);
>> @@ -53,7 +54,7 @@ void init_or_livepatch sort_exception_table(struct exception_table_entry *start,
>> const struct exception_table_entry *stop)
>> {
>> sort(start, stop - start,
>> - sizeof(struct exception_table_entry), cmp_ex, swap_ex);
>> + sizeof(struct exception_table_entry), cmp_ex, swap_ex, NULL);
>> }
>
> Not the least because of this addition of an entirely useless parameter / argument
Well it’s not useless in this patch, given that without it I couldn’t know the size of the address
element, however ...
> I'm not in favor of ...
>
>> --- a/xen/include/xen/sort.h
>> +++ b/xen/include/xen/sort.h
>> @@ -23,8 +23,8 @@
>> extern gnu_inline
>> #endif
>> void sort(void *base, size_t num, size_t size,
>> - int (*cmp)(const void *a, const void *b),
>> - void (*swap)(void *a, void *b, size_t size))
>> + int (*cmp)(const void *a, const void *b, const void *data),
>> + void (*swap)(void *a, void *b, size_t size), const void *cmp_data)
>> {
>
> ... this change. Consider you were doing this on a C library you cannot change.
> You'd have to find a different solution anyway.
I get your point here, we should not change standard functions.
> And the way we have sort()
> right now is matching the C spec. The change to do renders things unexpected to
> anyone wanting to use this function in a spec-compliant way. One approach may
> be to make an adjustment to data representation, such that the extra reference
> data is accessible through the pointers already being passed.
>
> Jan
>
Anyway in the end this patch was dropped for other reasons.
Cheers,
Luca
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2024-04-18 7:44 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-09 11:45 [PATCH v2 00/13] Static shared memory followup v2 - pt1 Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 01/13] xen/arm: remove stale addr_cells/size_cells in assign_shared_memory Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 02/13] xen/arm: avoid repetitive checking in process_shm_node Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node Luca Fancellu
2024-04-09 13:18 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures Luca Fancellu
2024-04-09 13:24 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 05/13] xen/arm: Conditional compilation of kernel_info.shm_mem member Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory Luca Fancellu
2024-04-09 13:38 ` Michal Orzel
2024-04-09 14:37 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap Luca Fancellu
2024-04-10 7:40 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 08/13] xen/arm: Introduce helper for static memory pages Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory Luca Fancellu
2024-04-10 10:01 ` Michal Orzel
2024-04-10 10:56 ` Luca Fancellu
2024-04-10 11:01 ` Michal Orzel
2024-04-10 11:19 ` Luca Fancellu
2024-04-10 11:23 ` Michal Orzel
2024-04-09 11:45 ` [PATCH v2 10/13] xen/arm: remove shm holes from extended regions Luca Fancellu
2024-04-10 11:15 ` Michal Orzel
2024-04-10 14:08 ` Luca Fancellu
2024-04-10 14:58 ` Michal Orzel
2024-04-10 14:12 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 11/13] xen/arm: fix duplicate /reserved-memory node in Dom0 Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals Luca Fancellu
2024-04-11 9:50 ` Luca Fancellu
2024-04-11 13:36 ` Luca Fancellu
2024-04-18 6:28 ` Jan Beulich
2024-04-18 7:44 ` Luca Fancellu
2024-04-09 11:45 ` [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes Luca Fancellu
2024-04-15 18:41 ` Julien Grall
2024-04-16 6:27 ` Luca Fancellu
2024-04-16 8:50 ` Julien Grall
2024-04-16 8:57 ` Michal Orzel
2024-04-16 8:59 ` Luca Fancellu
2024-04-16 9:06 ` Julien Grall
2024-04-16 10:50 ` Luca Fancellu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.