[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v11 03/12] xen/arm: permit non direct-mapped Dom0 construction
Hi Michal,
On 07/12/2024 15:04, Michal Orzel wrote:
On 06/12/2024 19:37, Julien Grall wrote:
Hi,
Sorry for the late answer.
On 05/12/2024 09:40, Michal Orzel wrote:
On 02/12/2024 17:59, Carlo Nonato wrote:
Cache coloring requires Dom0 not to be direct-mapped because of its non
contiguous mapping nature, so allocate_memory() is needed in this case.
8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate module")
moved allocate_memory() in dom0less_build.c. In order to use it
in Dom0 construction bring it back to domain_build.c and declare it in
domain_build.h.
Take the opportunity to adapt the implementation of allocate_memory() so
that it uses the host layout when called on the hwdom, via
find_unallocated_memory().
Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
---
v11:
- GUEST_RAM_BANKS instead of hardcoding the number of banks in allocate_memory()
- hwdom_ext_regions -> hwdom_free_mem in allocate_memory()
- added a comment in allocate_memory() when skipping small banks
v10:
- fixed a compilation bug that happened when dom0less support was disabled
v9:
- no changes
v8:
- patch adapted to new changes to allocate_memory()
v7:
- allocate_memory() now uses the host layout when called on the hwdom
v6:
- new patch
---
xen/arch/arm/dom0less-build.c | 44 -----------
xen/arch/arm/domain_build.c | 97 ++++++++++++++++++++++++-
xen/arch/arm/include/asm/domain_build.h | 1 +
3 files changed, 94 insertions(+), 48 deletions(-)
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index d93a85434e..67b1503647 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -49,50 +49,6 @@ bool __init is_dom0less_mode(void)
return ( !dom0found && domUfound );
}
-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;
-
- printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
- /* Don't want format this as PRIpaddr (16 digit hex) */
- (unsigned long)(kinfo->unassigned_mem >> 20), d);
-
- mem->nr_banks = 0;
- bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
- if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
- bank_size) )
- goto fail;
-
- bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
- if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
- bank_size) )
- goto fail;
-
- if ( kinfo->unassigned_mem )
- goto fail;
-
- for( i = 0; i < mem->nr_banks; i++ )
- {
- printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
- d,
- i,
- mem->bank[i].start,
- mem->bank[i].start + mem->bank[i].size,
- /* Don't want format this as PRIpaddr (16 digit hex) */
- (unsigned long)(mem->bank[i].size >> 20));
- }
-
- return;
-
-fail:
- panic("Failed to allocate requested domain memory."
- /* Don't want format this as PRIpaddr (16 digit hex) */
- " %ldKB unallocated. Fix the VMs configurations.\n",
- (unsigned long)kinfo->unassigned_mem >> 10);
-}
-
#ifdef CONFIG_VGICV2
static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
{
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2c30792de8..2b8cba9b2f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -416,7 +416,6 @@ static void __init allocate_memory_11(struct domain *d,
}
}
-#ifdef CONFIG_DOM0LESS_BOOT
bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
alloc_domheap_mem_cb cb, void *extra)
{
@@ -508,7 +507,6 @@ bool __init allocate_bank_memory(struct kernel_info *kinfo,
gfn_t sgfn,
return true;
}
-#endif
/*
* When PCI passthrough is available we want to keep the
@@ -1003,6 +1001,94 @@ out:
return res;
}
+void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
+{
+ struct membanks *mem = kernel_info_get_mem(kinfo);
+ unsigned int i, nr_banks = GUEST_RAM_BANKS;
+ paddr_t bank_start, bank_size;
Limit the scope
+ struct membanks *hwdom_free_mem = NULL;
+ const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+ const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
Limit the scope
+
+ printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
+ /* Don't want format this as PRIpaddr (16 digit hex) */
+ (unsigned long)(kinfo->unassigned_mem >> 20), d);
+
+ mem->nr_banks = 0;
+ /*
+ * Use host memory layout for hwdom. Only case for this is when LLC
coloring
+ * is enabled.
+ */
+ if ( is_hardware_domain(d) )
+ {
+ ASSERT(llc_coloring_enabled);
This patch does not build because of declaration not being visible. You must include
<xen/llc-coloring.h>.
Piggying back on this comment. AFAICT, the code below would work also in
the non cache coloring case. So what's the assert is for?
+
+ hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
+ NR_MEM_BANKS);
+ if ( !hwdom_free_mem )
+ goto fail;
+
+ hwdom_free_mem->max_banks = NR_MEM_BANKS;
+
+ if ( find_unallocated_memory(kinfo, hwdom_free_mem) )
My remarks for the use of find_unallocated_memory() 1:1 have not been
addressed. You did not even
change the comments inside the function. The problem is that the function is
specifically designed
for finding extended regions and assumes being called at certain point i.e.
dom0 RAM allocated, gnttab
region allocated, etc.
So I agree that the function should be updated if we plan to use it for
other purpose.
My opinion is that we should attempt to make the function generic so
that in your
case you can choose which regions to exclude, define even your own function to
grab free regions (at the moment
add_ext_regions grabs banks >= 64M but you still discards banks >= 128M, so
it's a bit wasteful.
My very short attempt to make the function as generic as possible in the first
iteration:
https://paste.debian.net/1338334/
This looks better, but I wonder why we need still need to exclude the
static regions? Wouldn't it be sufficient to exclude just reserved regions?
Static shared memory banks are not part of reserved memory (i.e.
bootinfo.reserved_mem) if that's what you're asking.
They are stored in bootinfo.shmem, hence we need to take them into account when
searching for unused address space.
Oh I missed the fact you now pass "mem_banks" as a parameter. I thought
they would still get excluded for cache coloring case.
If you and Carlo are ok with my proposed solution for making the function
generic, I can send a patch as a prerequisite
patch for Carlo series.
I am fine with the approach.
Cheers,
--
Julien Grall
|
