Hi,
On 01/04/2022 01:38, Stefano Stabellini wrote:
From: Stefano Stabellini <stefano.stabell...@xilinx.com>
Implement extended regions for dom0less domUs. The implementation is
based on the libxl implementation.
Signed-off-by: Stefano Stabellini <stefano.stabell...@xilinx.com>
---
xen/arch/arm/domain_build.c | 42 ++++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de..b6189b935d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1324,6 +1324,35 @@ out:
return res;
}
+#define ALIGN_UP_TO_2MB(x) (((x) + MB(2) - 1) & (~(MB(2) - 1)))
I think this is the same as ROUNDUP(x, SZ_2M).
+
+static int __init find_domU_holes(const struct kernel_info *kinfo,
+ struct meminfo *ext_regions)
+{
+ unsigned int i;
+ uint64_t bankend[GUEST_RAM_BANKS];
Looking, you only seem to use one bankend at the time. So why do you
need to store all the bankend?
This should also be s/uint64_t/paddr_t/. Same for two other instances below.
+ const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+ const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+
+ for ( i = 0; i < GUEST_RAM_BANKS; i++ )
+ {
+ ext_regions->bank[ext_regions->nr_banks].start =
The code would be easier to read if you define a local variable ext_bank
that points to &(ext_regions->bank[ext_regions->nr_banks]).
+ ALIGN_UP_TO_2MB(bankbase[i] + kinfo->mem.bank[i].size);
+
+ bankend[i] = ~0ULL >> (64 - p2m_ipa_bits);
+ bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1); > + if
(bankend[i] > ext_regions->bank[ext_regions->nr_banks].start)
Coding style:
if ( ... )
+ ext_regions->bank[ext_regions->nr_banks].size =
+ bankend[i] - ext_regions->bank[ext_regions->nr_banks].start +
1;
This is one of the line that could greatly benefits from the local
variable I suggested above. It would look like:
ext_bank->size = bankend[i] - ext_bank->start + 1;
+
+ /* 64MB is the minimum size of an extended region */
+ if ( ext_regions->bank[ext_regions->nr_banks].size < MB(64) )
+ continue;
+ ext_regions->nr_banks++;
+ }
NIT: We tend to add a newline before the last return.
+ return 0;
find_memory_holes() and find_unallocated_memory() will return an error
if there are no banks allocated. I think we should do the same here at
least for consistency.
In which case, the check should be moved in make_hypervisor_node().
+}
+
static int __init make_hypervisor_node(struct domain *d,
const struct kernel_info *kinfo,
int addrcells, int sizecells)
@@ -1374,10 +1403,17 @@ static int __init make_hypervisor_node(struct domain *d,
if ( !ext_regions )
return -ENOMEM;
- if ( !is_iommu_enabled(d) )
- res = find_unallocated_memory(kinfo, ext_regions);
+ if ( is_domain_direct_mapped(d) )
I believe the code in the 'if' part would work properly for a dom0 that
is not direct mapped (e.g. in the cache coloring case).
If it doesn't, I think we need...
+ {
+ if ( !is_iommu_enabled(d) )
+ res = find_unallocated_memory(kinfo, ext_regions);
+ else
+ res = find_memory_holes(kinfo, ext_regions);
+ }
else
- res = find_memory_holes(kinfo, ext_regions);
+ {
... and ASSERT() here so we the person that will introduce non direct
mapped dom0 can easily notice before the domain get corrupted.
+ res = find_domU_holes(kinfo, ext_regions);
+ }
if ( res )
printk(XENLOG_WARNING "Failed to allocate extended regions\n");
This printk() and the others in the function should print the domain.
Cheers,
--
Julien Grall