| Message ID | 20220401003847.38393-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dom0less PV drivers | expand |
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
Hi, 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,
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");