| Message ID | 1633519346-3686-4-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space") | expand |
On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > This is a follow-up of > "b6fe410 xen/arm: Add handling of extended regions for Dom0" > > Add various in-code comments, update Xen hypervisor device tree > bindings text, change the log level for some prints and clarify > format specifier, reuse dt_for_each_range() to avoid open-coding > in find_memory_holes(). > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Thanks for the patch, it looks like you addressed all Julien's comments well. A couple of minor issues below. > --- > New patch > --- > docs/misc/arm/device-tree/guest.txt | 12 ++-- > xen/arch/arm/domain_build.c | 108 ++++++++++++++++++++++-------------- > 2 files changed, 73 insertions(+), 47 deletions(-) > > diff --git a/docs/misc/arm/device-tree/guest.txt b/docs/misc/arm/device-tree/guest.txt > index 418f1e9..c115751 100644 > --- a/docs/misc/arm/device-tree/guest.txt > +++ b/docs/misc/arm/device-tree/guest.txt > @@ -7,10 +7,14 @@ the following properties: > compatible = "xen,xen-<version>", "xen,xen"; > where <version> is the version of the Xen ABI of the platform. > > -- reg: specifies the base physical address and size of a region in > - memory where the grant table should be mapped to, using an > - HYPERVISOR_memory_op hypercall. The memory region is large enough to map > - the whole grant table (it is larger or equal to gnttab_max_grant_frames()). > +- reg: specifies the base physical address and size of the regions in memory > + where the special resources should be mapped to, using an HYPERVISOR_memory_op > + hypercall. > + Region 0 is reserved for mapping grant table, it must be always present. > + The memory region is large enough to map the whole grant table (it is larger > + or equal to gnttab_max_grant_frames()). > + Regions 1...N are extended regions (unused address space) for mapping foreign > + GFNs and grants, they might be absent if there is nothing to expose. > This property is unnecessary when booting Dom0 using ACPI. > > - interrupts: the interrupt used by Xen to inject event notifications. > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index c5afbe2..d9f40d4 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -898,7 +898,10 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) > if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) ) > return 0; > > - /* Both start and size of the extended region should be 2MB aligned */ > + /* > + * Both start and size of the extended region should be 2MB aligned to > + * potentially allow superpage mapping. > + */ > start = (s + SZ_2M - 1) & ~(SZ_2M - 1); > if ( start > e ) > return 0; > @@ -909,6 +912,12 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) > */ > e += 1; > size = (e - start) & ~(SZ_2M - 1); > + > + /* > + * Reasonable size. Not too little to pick up small ranges which are > + * not quite useful itself but increase bookkeeping and not too much ^ remove itself ^ large > + * to skip a large proportion of unused address space. > + */ > if ( size < MB(64) ) > return 0; > > @@ -919,6 +928,14 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) > return 0; > } > > +/* > + * Find unused regions of Host address space which can be exposed to Dom0 > + * as extended regions for the special memory mappings. In order to calculate > + * regions we exclude every assigned to Dom0 region from the Host RAM: ^ region assigned ^ remove > + * - domain RAM > + * - reserved-memory > + * - grant table space > + */ > static int __init find_unallocated_memory(const struct kernel_info *kinfo, > struct meminfo *ext_regions) > { > @@ -942,7 +959,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > res = rangeset_add_range(unalloc_mem, start, end - 1); > if ( res ) > { > - printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n", > + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", > start, end); > goto out; > } > @@ -956,7 +973,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > res = rangeset_remove_range(unalloc_mem, start, end - 1); > if ( res ) > { > - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", > + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > start, end); > goto out; > } > @@ -971,7 +988,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > res = rangeset_remove_range(unalloc_mem, start, end - 1); > if ( res ) > { > - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", > + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > start, end); > goto out; > } > @@ -983,7 +1000,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > res = rangeset_remove_range(unalloc_mem, start, end - 1); > if ( res ) > { > - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", > + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > start, end); > goto out; > } > @@ -1003,6 +1020,35 @@ out: > return res; > } > > +static int __init handle_pci_range(const struct dt_device_node *dev, > + u64 addr, u64 len, void *data) > +{ > + struct rangeset *mem_holes = data; > + paddr_t start, end; > + int res; > + > + start = addr & PAGE_MASK; > + end = PAGE_ALIGN(addr + len); > + res = rangeset_remove_range(mem_holes, start, end - 1); > + if ( res ) > + { > + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > + start, end); > + return res; > + } > + > + return 0; > +} > + > +/* > + * Find the holes in the Host DT which can be exposed to Dom0 as extended > + * regions for the special memory mappings. In order to calculate regions > + * we exclude every addressable memory region described by "reg" and "ranges" > + * properties from the maximum possible addressable physical memory range: > + * - MMIO > + * - Host RAM > + * - PCI bar ^ PCI aperture > + */ > static int __init find_memory_holes(const struct kernel_info *kinfo, > struct meminfo *ext_regions) > { > @@ -1024,7 +1070,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > res = rangeset_add_range(mem_holes, start, end); > if ( res ) > { > - printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n", > + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", > start, end); > goto out; > } > @@ -1055,49 +1101,25 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > res = rangeset_remove_range(mem_holes, start, end - 1); > if ( res ) > { > - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", > + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > start, end); > goto out; > } > } > > - if ( dt_device_type_is_equal(np, "pci" ) ) > + if ( dt_device_type_is_equal(np, "pci") ) > { > - unsigned int range_size, nr_ranges; > - int na, ns, pna; > - const __be32 *ranges; > - u32 len; > - > /* > - * Looking for non-empty ranges property which in this context > - * describes the PCI host bridge aperture. > + * The ranges property in this context describes the PCI host > + * bridge aperture. It shall be absent if no addresses are mapped > + * through the bridge. > */ > - ranges = dt_get_property(np, "ranges", &len); > - if ( !ranges || !len ) > + if ( !dt_get_property(np, "ranges", NULL) ) > continue; > > - pna = dt_n_addr_cells(np); > - na = dt_child_n_addr_cells(np); > - ns = dt_child_n_size_cells(np); > - range_size = pna + na + ns; > - nr_ranges = len / sizeof(__be32) / range_size; > - > - for ( i = 0; i < nr_ranges; i++, ranges += range_size ) > - { > - /* Skip the child address and get the parent (CPU) address */ > - addr = dt_read_number(ranges + na, pna); > - size = dt_read_number(ranges + na + pna, ns); > - > - start = addr & PAGE_MASK; > - end = PAGE_ALIGN(addr + size); > - res = rangeset_remove_range(mem_holes, start, end - 1); > - if ( res ) > - { > - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", > - start, end); > - goto out; > - } > - } > + res = dt_for_each_range(np, &handle_pci_range, mem_holes); > + if ( res ) > + goto out; > } > } > > @@ -1152,12 +1174,12 @@ static int __init make_hypervisor_node(struct domain *d, > > if ( !opt_ext_regions ) > { > - printk(XENLOG_DEBUG "The extended regions support is disabled\n"); > + printk(XENLOG_INFO "The extended regions support is disabled\n"); > nr_ext_regions = 0; > } > else if ( is_32bit_domain(d) ) > { > - printk(XENLOG_DEBUG "The extended regions are only supported for 64-bit guest currently\n"); > + printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n"); > nr_ext_regions = 0; > } > else > @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain *d, > u64 start = ext_regions->bank[i].start; > u64 size = ext_regions->bank[i].size; > > - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", > - i, start, start + size); > + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", > + i, start, start + size); Also should be PRIpaddr
On 07.10.21 04:50, Stefano Stabellini wrote: Hi Stefano > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> This is a follow-up of >> "b6fe410 xen/arm: Add handling of extended regions for Dom0" >> >> Add various in-code comments, update Xen hypervisor device tree >> bindings text, change the log level for some prints and clarify >> format specifier, reuse dt_for_each_range() to avoid open-coding >> in find_memory_holes(). >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Thanks for the patch, it looks like you addressed all Julien's comments > well. I believe so) > A couple of minor issues below. > > >> --- >> New patch >> --- >> docs/misc/arm/device-tree/guest.txt | 12 ++-- >> xen/arch/arm/domain_build.c | 108 ++++++++++++++++++++++-------------- >> 2 files changed, 73 insertions(+), 47 deletions(-) >> >> diff --git a/docs/misc/arm/device-tree/guest.txt b/docs/misc/arm/device-tree/guest.txt >> index 418f1e9..c115751 100644 >> --- a/docs/misc/arm/device-tree/guest.txt >> +++ b/docs/misc/arm/device-tree/guest.txt >> @@ -7,10 +7,14 @@ the following properties: >> compatible = "xen,xen-<version>", "xen,xen"; >> where <version> is the version of the Xen ABI of the platform. >> >> -- reg: specifies the base physical address and size of a region in >> - memory where the grant table should be mapped to, using an >> - HYPERVISOR_memory_op hypercall. The memory region is large enough to map >> - the whole grant table (it is larger or equal to gnttab_max_grant_frames()). >> +- reg: specifies the base physical address and size of the regions in memory >> + where the special resources should be mapped to, using an HYPERVISOR_memory_op >> + hypercall. >> + Region 0 is reserved for mapping grant table, it must be always present. >> + The memory region is large enough to map the whole grant table (it is larger >> + or equal to gnttab_max_grant_frames()). >> + Regions 1...N are extended regions (unused address space) for mapping foreign >> + GFNs and grants, they might be absent if there is nothing to expose. >> This property is unnecessary when booting Dom0 using ACPI. >> >> - interrupts: the interrupt used by Xen to inject event notifications. >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index c5afbe2..d9f40d4 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -898,7 +898,10 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) >> if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) ) >> return 0; >> >> - /* Both start and size of the extended region should be 2MB aligned */ >> + /* >> + * Both start and size of the extended region should be 2MB aligned to >> + * potentially allow superpage mapping. >> + */ >> start = (s + SZ_2M - 1) & ~(SZ_2M - 1); >> if ( start > e ) >> return 0; >> @@ -909,6 +912,12 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) >> */ >> e += 1; >> size = (e - start) & ~(SZ_2M - 1); >> + >> + /* >> + * Reasonable size. Not too little to pick up small ranges which are >> + * not quite useful itself but increase bookkeeping and not too much > ^ remove itself ^ large ok > >> + * to skip a large proportion of unused address space. >> + */ >> if ( size < MB(64) ) >> return 0; >> >> @@ -919,6 +928,14 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) >> return 0; >> } >> >> +/* >> + * Find unused regions of Host address space which can be exposed to Dom0 >> + * as extended regions for the special memory mappings. In order to calculate >> + * regions we exclude every assigned to Dom0 region from the Host RAM: > ^ region assigned ^ remove ok > > >> + * - domain RAM >> + * - reserved-memory >> + * - grant table space >> + */ >> static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> struct meminfo *ext_regions) >> { >> @@ -942,7 +959,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> res = rangeset_add_range(unalloc_mem, start, end - 1); >> if ( res ) >> { >> - printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n", >> + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", >> start, end); >> goto out; >> } >> @@ -956,7 +973,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> res = rangeset_remove_range(unalloc_mem, start, end - 1); >> if ( res ) >> { >> - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", >> + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", >> start, end); >> goto out; >> } >> @@ -971,7 +988,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> res = rangeset_remove_range(unalloc_mem, start, end - 1); >> if ( res ) >> { >> - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", >> + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", >> start, end); >> goto out; >> } >> @@ -983,7 +1000,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, >> res = rangeset_remove_range(unalloc_mem, start, end - 1); >> if ( res ) >> { >> - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", >> + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", >> start, end); >> goto out; >> } >> @@ -1003,6 +1020,35 @@ out: >> return res; >> } >> >> +static int __init handle_pci_range(const struct dt_device_node *dev, >> + u64 addr, u64 len, void *data) >> +{ >> + struct rangeset *mem_holes = data; >> + paddr_t start, end; >> + int res; >> + >> + start = addr & PAGE_MASK; >> + end = PAGE_ALIGN(addr + len); >> + res = rangeset_remove_range(mem_holes, start, end - 1); >> + if ( res ) >> + { >> + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", >> + start, end); >> + return res; >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Find the holes in the Host DT which can be exposed to Dom0 as extended >> + * regions for the special memory mappings. In order to calculate regions >> + * we exclude every addressable memory region described by "reg" and "ranges" >> + * properties from the maximum possible addressable physical memory range: >> + * - MMIO >> + * - Host RAM >> + * - PCI bar > ^ PCI aperture ok > > >> + */ >> static int __init find_memory_holes(const struct kernel_info *kinfo, >> struct meminfo *ext_regions) >> { >> @@ -1024,7 +1070,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, >> res = rangeset_add_range(mem_holes, start, end); >> if ( res ) >> { >> - printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n", >> + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", >> start, end); >> goto out; >> } >> @@ -1055,49 +1101,25 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, >> res = rangeset_remove_range(mem_holes, start, end - 1); >> if ( res ) >> { >> - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", >> + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", >> start, end); >> goto out; >> } >> } >> >> - if ( dt_device_type_is_equal(np, "pci" ) ) >> + if ( dt_device_type_is_equal(np, "pci") ) >> { >> - unsigned int range_size, nr_ranges; >> - int na, ns, pna; >> - const __be32 *ranges; >> - u32 len; >> - >> /* >> - * Looking for non-empty ranges property which in this context >> - * describes the PCI host bridge aperture. >> + * The ranges property in this context describes the PCI host >> + * bridge aperture. It shall be absent if no addresses are mapped >> + * through the bridge. >> */ >> - ranges = dt_get_property(np, "ranges", &len); >> - if ( !ranges || !len ) >> + if ( !dt_get_property(np, "ranges", NULL) ) >> continue; >> >> - pna = dt_n_addr_cells(np); >> - na = dt_child_n_addr_cells(np); >> - ns = dt_child_n_size_cells(np); >> - range_size = pna + na + ns; >> - nr_ranges = len / sizeof(__be32) / range_size; >> - >> - for ( i = 0; i < nr_ranges; i++, ranges += range_size ) >> - { >> - /* Skip the child address and get the parent (CPU) address */ >> - addr = dt_read_number(ranges + na, pna); >> - size = dt_read_number(ranges + na + pna, ns); >> - >> - start = addr & PAGE_MASK; >> - end = PAGE_ALIGN(addr + size); >> - res = rangeset_remove_range(mem_holes, start, end - 1); >> - if ( res ) >> - { >> - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", >> - start, end); >> - goto out; >> - } >> - } >> + res = dt_for_each_range(np, &handle_pci_range, mem_holes); >> + if ( res ) >> + goto out; >> } >> } >> >> @@ -1152,12 +1174,12 @@ static int __init make_hypervisor_node(struct domain *d, >> >> if ( !opt_ext_regions ) >> { >> - printk(XENLOG_DEBUG "The extended regions support is disabled\n"); >> + printk(XENLOG_INFO "The extended regions support is disabled\n"); >> nr_ext_regions = 0; >> } >> else if ( is_32bit_domain(d) ) >> { >> - printk(XENLOG_DEBUG "The extended regions are only supported for 64-bit guest currently\n"); >> + printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n"); >> nr_ext_regions = 0; >> } >> else >> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain *d, >> u64 start = ext_regions->bank[i].start; >> u64 size = ext_regions->bank[i].size; >> >> - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >> - i, start, start + size); >> + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >> + i, start, start + size); > Also should be PRIpaddr I thought I needed to change specifier only for variables of type "paddr_t", but here "u64".
On Thu, 7 Oct 2021, Oleksandr wrote: > On 07.10.21 04:50, Stefano Stabellini wrote: > > Hi Stefano > > > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > > > > > This is a follow-up of > > > "b6fe410 xen/arm: Add handling of extended regions for Dom0" > > > > > > Add various in-code comments, update Xen hypervisor device tree > > > bindings text, change the log level for some prints and clarify > > > format specifier, reuse dt_for_each_range() to avoid open-coding > > > in find_memory_holes(). > > > > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > Thanks for the patch, it looks like you addressed all Julien's comments > > well. > > I believe so) [...] > > > @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain > > > *d, > > > u64 start = ext_regions->bank[i].start; > > > u64 size = ext_regions->bank[i].size; > > > - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", > > > - i, start, start + size); > > > + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", > > > + i, start, start + size); > > Also should be PRIpaddr > > I thought I needed to change specifier only for variables of type "paddr_t", > but here "u64". Sorry, you are right. I added my reviewed-by and made the small typo changes on commit.
On 07.10.21 23:06, Stefano Stabellini wrote: Hi Stefano > On Thu, 7 Oct 2021, Oleksandr wrote: >> On 07.10.21 04:50, Stefano Stabellini wrote: >> >> Hi Stefano >> >>> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: >>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> >>>> This is a follow-up of >>>> "b6fe410 xen/arm: Add handling of extended regions for Dom0" >>>> >>>> Add various in-code comments, update Xen hypervisor device tree >>>> bindings text, change the log level for some prints and clarify >>>> format specifier, reuse dt_for_each_range() to avoid open-coding >>>> in find_memory_holes(). >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> Thanks for the patch, it looks like you addressed all Julien's comments >>> well. >> I believe so) > > [...] > >>>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain >>>> *d, >>>> u64 start = ext_regions->bank[i].start; >>>> u64 size = ext_regions->bank[i].size; >>>> - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >>>> - i, start, start + size); >>>> + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >>>> + i, start, start + size); >>> Also should be PRIpaddr >> I thought I needed to change specifier only for variables of type "paddr_t", >> but here "u64". > Sorry, you are right. > > I added my reviewed-by and made the small typo changes on commit. Thanks! In case if you haven't committed the patch yet, let's please wait for Julien (who asked for this follow-up) to review it. In any case, I will be able to do another follow-up if needed.
On Thu, 7 Oct 2021, Oleksandr wrote: > On 07.10.21 23:06, Stefano Stabellini wrote: > > On Thu, 7 Oct 2021, Oleksandr wrote: > > > On 07.10.21 04:50, Stefano Stabellini wrote: > > > > > > Hi Stefano > > > > > > > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: > > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > > > > > > > > > This is a follow-up of > > > > > "b6fe410 xen/arm: Add handling of extended regions for Dom0" > > > > > > > > > > Add various in-code comments, update Xen hypervisor device tree > > > > > bindings text, change the log level for some prints and clarify > > > > > format specifier, reuse dt_for_each_range() to avoid open-coding > > > > > in find_memory_holes(). > > > > > > > > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > > > Thanks for the patch, it looks like you addressed all Julien's comments > > > > well. > > > I believe so) > > > > [...] > > > > > > > @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct > > > > > domain > > > > > *d, > > > > > u64 start = ext_regions->bank[i].start; > > > > > u64 size = ext_regions->bank[i].size; > > > > > - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", > > > > > - i, start, start + size); > > > > > + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", > > > > > + i, start, start + size); > > > > Also should be PRIpaddr > > > I thought I needed to change specifier only for variables of type > > > "paddr_t", > > > but here "u64". > > Sorry, you are right. > > > > I added my reviewed-by and made the small typo changes on commit. > > Thanks! In case if you haven't committed the patch yet, let's please wait for > Julien (who asked for this follow-up) to review it. > > In any case, I will be able to do another follow-up if needed. I committed it as I would like to squeeze as many runs out of OSSTest and Gitlab-CI as possible as we are getting closer and closer to the release. I am trying to avoid the last minute rush to commit 150 patches one day before code freeze :-) The more intermediate runs we get, the easier is to pinpoint (and fix) regressions. But also, this patch doesn't affect external interfances, it is just internal and mostly comments, so it is super-easy to do follow-ups.
On 07.10.21 23:42, Stefano Stabellini wrote: Hi Stefano > On Thu, 7 Oct 2021, Oleksandr wrote: >> On 07.10.21 23:06, Stefano Stabellini wrote: >>> On Thu, 7 Oct 2021, Oleksandr wrote: >>>> On 07.10.21 04:50, Stefano Stabellini wrote: >>>> >>>> Hi Stefano >>>> >>>>> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: >>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>>> >>>>>> This is a follow-up of >>>>>> "b6fe410 xen/arm: Add handling of extended regions for Dom0" >>>>>> >>>>>> Add various in-code comments, update Xen hypervisor device tree >>>>>> bindings text, change the log level for some prints and clarify >>>>>> format specifier, reuse dt_for_each_range() to avoid open-coding >>>>>> in find_memory_holes(). >>>>>> >>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>> Thanks for the patch, it looks like you addressed all Julien's comments >>>>> well. >>>> I believe so) >>> [...] >>> >>>>>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct >>>>>> domain >>>>>> *d, >>>>>> u64 start = ext_regions->bank[i].start; >>>>>> u64 size = ext_regions->bank[i].size; >>>>>> - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >>>>>> - i, start, start + size); >>>>>> + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >>>>>> + i, start, start + size); >>>>> Also should be PRIpaddr >>>> I thought I needed to change specifier only for variables of type >>>> "paddr_t", >>>> but here "u64". >>> Sorry, you are right. >>> >>> I added my reviewed-by and made the small typo changes on commit. >> Thanks! In case if you haven't committed the patch yet, let's please wait for >> Julien (who asked for this follow-up) to review it. >> >> In any case, I will be able to do another follow-up if needed. > > I committed it as I would like to squeeze as many runs out of OSSTest > and Gitlab-CI as possible as we are getting closer and closer to the > release. I am trying to avoid the last minute rush to commit 150 patches > one day before code freeze :-) > > The more intermediate runs we get, the easier is to pinpoint (and fix) > regressions. I got it, thank you for the explanation. > > But also, this patch doesn't affect external interfances, it is just > internal and mostly comments, so it is super-easy to do follow-ups. Yes, agree.
Hi Oleksandr, On 07/10/2021 21:29, Oleksandr wrote: >>>>> @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct >>>>> domain >>>>> *d, >>>>> u64 start = ext_regions->bank[i].start; >>>>> u64 size = ext_regions->bank[i].size; >>>>> - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >>>>> - i, start, start + size); >>>>> + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", >>>>> + i, start, start + size); >>>> Also should be PRIpaddr >>> I thought I needed to change specifier only for variables of type >>> "paddr_t", >>> but here "u64". >> Sorry, you are right. >> >> I added my reviewed-by and made the small typo changes on commit. > > Thanks! In case if you haven't committed the patch yet, let's please > wait for Julien (who asked for this follow-up) to review it. I went through it. The change looks good to me. No need for... > > In any case, I will be able to do another follow-up if needed. ... another follow-up. Thanks for sending a patch to handle my requests! :) Cheers,
diff --git a/docs/misc/arm/device-tree/guest.txt b/docs/misc/arm/device-tree/guest.txt index 418f1e9..c115751 100644 --- a/docs/misc/arm/device-tree/guest.txt +++ b/docs/misc/arm/device-tree/guest.txt @@ -7,10 +7,14 @@ the following properties: compatible = "xen,xen-<version>", "xen,xen"; where <version> is the version of the Xen ABI of the platform. -- reg: specifies the base physical address and size of a region in - memory where the grant table should be mapped to, using an - HYPERVISOR_memory_op hypercall. The memory region is large enough to map - the whole grant table (it is larger or equal to gnttab_max_grant_frames()). +- reg: specifies the base physical address and size of the regions in memory + where the special resources should be mapped to, using an HYPERVISOR_memory_op + hypercall. + Region 0 is reserved for mapping grant table, it must be always present. + The memory region is large enough to map the whole grant table (it is larger + or equal to gnttab_max_grant_frames()). + Regions 1...N are extended regions (unused address space) for mapping foreign + GFNs and grants, they might be absent if there is nothing to expose. This property is unnecessary when booting Dom0 using ACPI. - interrupts: the interrupt used by Xen to inject event notifications. diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index c5afbe2..d9f40d4 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -898,7 +898,10 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) if ( ext_regions->nr_banks >= ARRAY_SIZE(ext_regions->bank) ) return 0; - /* Both start and size of the extended region should be 2MB aligned */ + /* + * Both start and size of the extended region should be 2MB aligned to + * potentially allow superpage mapping. + */ start = (s + SZ_2M - 1) & ~(SZ_2M - 1); if ( start > e ) return 0; @@ -909,6 +912,12 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) */ e += 1; size = (e - start) & ~(SZ_2M - 1); + + /* + * Reasonable size. Not too little to pick up small ranges which are + * not quite useful itself but increase bookkeeping and not too much + * to skip a large proportion of unused address space. + */ if ( size < MB(64) ) return 0; @@ -919,6 +928,14 @@ static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) return 0; } +/* + * Find unused regions of Host address space which can be exposed to Dom0 + * as extended regions for the special memory mappings. In order to calculate + * regions we exclude every assigned to Dom0 region from the Host RAM: + * - domain RAM + * - reserved-memory + * - grant table space + */ static int __init find_unallocated_memory(const struct kernel_info *kinfo, struct meminfo *ext_regions) { @@ -942,7 +959,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, res = rangeset_add_range(unalloc_mem, start, end - 1); if ( res ) { - printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n", + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", start, end); goto out; } @@ -956,7 +973,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, res = rangeset_remove_range(unalloc_mem, start, end - 1); if ( res ) { - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", start, end); goto out; } @@ -971,7 +988,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, res = rangeset_remove_range(unalloc_mem, start, end - 1); if ( res ) { - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", start, end); goto out; } @@ -983,7 +1000,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, res = rangeset_remove_range(unalloc_mem, start, end - 1); if ( res ) { - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", start, end); goto out; } @@ -1003,6 +1020,35 @@ out: return res; } +static int __init handle_pci_range(const struct dt_device_node *dev, + u64 addr, u64 len, void *data) +{ + struct rangeset *mem_holes = data; + paddr_t start, end; + int res; + + start = addr & PAGE_MASK; + end = PAGE_ALIGN(addr + len); + res = rangeset_remove_range(mem_holes, start, end - 1); + if ( res ) + { + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", + start, end); + return res; + } + + return 0; +} + +/* + * Find the holes in the Host DT which can be exposed to Dom0 as extended + * regions for the special memory mappings. In order to calculate regions + * we exclude every addressable memory region described by "reg" and "ranges" + * properties from the maximum possible addressable physical memory range: + * - MMIO + * - Host RAM + * - PCI bar + */ static int __init find_memory_holes(const struct kernel_info *kinfo, struct meminfo *ext_regions) { @@ -1024,7 +1070,7 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, res = rangeset_add_range(mem_holes, start, end); if ( res ) { - printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n", + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", start, end); goto out; } @@ -1055,49 +1101,25 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, res = rangeset_remove_range(mem_holes, start, end - 1); if ( res ) { - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", start, end); goto out; } } - if ( dt_device_type_is_equal(np, "pci" ) ) + if ( dt_device_type_is_equal(np, "pci") ) { - unsigned int range_size, nr_ranges; - int na, ns, pna; - const __be32 *ranges; - u32 len; - /* - * Looking for non-empty ranges property which in this context - * describes the PCI host bridge aperture. + * The ranges property in this context describes the PCI host + * bridge aperture. It shall be absent if no addresses are mapped + * through the bridge. */ - ranges = dt_get_property(np, "ranges", &len); - if ( !ranges || !len ) + if ( !dt_get_property(np, "ranges", NULL) ) continue; - pna = dt_n_addr_cells(np); - na = dt_child_n_addr_cells(np); - ns = dt_child_n_size_cells(np); - range_size = pna + na + ns; - nr_ranges = len / sizeof(__be32) / range_size; - - for ( i = 0; i < nr_ranges; i++, ranges += range_size ) - { - /* Skip the child address and get the parent (CPU) address */ - addr = dt_read_number(ranges + na, pna); - size = dt_read_number(ranges + na + pna, ns); - - start = addr & PAGE_MASK; - end = PAGE_ALIGN(addr + size); - res = rangeset_remove_range(mem_holes, start, end - 1); - if ( res ) - { - printk(XENLOG_ERR "Failed to remove: %#"PRIx64"->%#"PRIx64"\n", - start, end); - goto out; - } - } + res = dt_for_each_range(np, &handle_pci_range, mem_holes); + if ( res ) + goto out; } } @@ -1152,12 +1174,12 @@ static int __init make_hypervisor_node(struct domain *d, if ( !opt_ext_regions ) { - printk(XENLOG_DEBUG "The extended regions support is disabled\n"); + printk(XENLOG_INFO "The extended regions support is disabled\n"); nr_ext_regions = 0; } else if ( is_32bit_domain(d) ) { - printk(XENLOG_DEBUG "The extended regions are only supported for 64-bit guest currently\n"); + printk(XENLOG_WARNING "The extended regions are only supported for 64-bit guest currently\n"); nr_ext_regions = 0; } else @@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct domain *d, u64 start = ext_regions->bank[i].start; u64 size = ext_regions->bank[i].size; - dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", - i, start, start + size); + printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); dt_child_set_range(&cells, addrcells, sizecells, start, size); }