[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support



On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> 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@xxxxxxxx>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.