From: Stefano Stabellini <stefano.stabellini@xilinx.com>
Implement extended regions for dom0less domUs. The implementation is
based on the libxl implementation.
Signed-off-by: Stefano Stabellini <stefano.stabellini@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)))
+
+static int __init find_domU_holes(const struct kernel_info *kinfo,
+ struct meminfo *ext_regions)
+{
+ unsigned int i;
+ uint64_t bankend[GUEST_RAM_BANKS];
+ 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 =
+ 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)
+ ext_regions->bank[ext_regions->nr_banks].size =
+ bankend[i] - ext_regions->bank[ext_regions->nr_banks].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++;
+ }
+ return 0;
+}
+
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) )
+ {
+ 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);
+ {
+ res = find_domU_holes(kinfo, ext_regions);
+ }
if ( res )
printk(XENLOG_WARNING "Failed to allocate extended regions\n");
--
2.25.1Hi,
On 01/04/2022 01:38, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> Implement extended regions for dom0less domUs. The implementation is
> based on the libxl implementation.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@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
Hi Stefano,
> +#define ALIGN_UP_TO_2MB(x) (((x) + MB(2) - 1) & (~(MB(2) - 1)))
> +
> +static int __init find_domU_holes(const struct kernel_info *kinfo,
> + struct meminfo *ext_regions)
> +{
> + unsigned int i;
> + uint64_t bankend[GUEST_RAM_BANKS];
> + 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 =
> + 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)
Just a code style issue, the if needs a space before and after the condition
With this fixed:
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Cheers,
Luca
© 2016 - 2024 Red Hat, Inc.