Re: [PATCH V2 2/3] xen/arm: Add handling of extended regions for Dom0

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 22 Sep 2021, Oleksandr wrote:
> > > > > You will also need to cover "ranges" that will describe the BARs for
> > > > > the PCI
> > > > > devices.
> > > > Good point.
> > > Yes, very good point!
> > > 
> > > 
> > > > Could you please clarify how to recognize whether it is a PCI
> > > > device as long as PCI support is not merged? Or just to find any device
> > > > nodes
> > > > with non-empty "ranges" property
> > > > and retrieve addresses?
> > > Normally any bus can have a ranges property with the aperture and
> > > possible address translations, including /amba (compatible =
> > > "simple-bus"). However, in these cases dt_device_get_address already
> > > takes care of it, see xen/common/device_tree.c:dt_device_get_address.
> > > 
> > > The PCI bus is special for 2 reasons:
> > > - the ranges property has a different format
> > > - the bus is hot-pluggable
> > > 
> > > So I think the only one that we need to treat specially is PCI.
> > > 
> > > As far as I am aware PCI is the only bus (or maybe just the only bus
> > > that we support?) where ranges means the aperture.
> > Thank you for the clarification. I need to find device node with non-empty
> > ranges property
> > (and make sure that device_type property is "pci"), after that I need to
> > read the context of ranges property and translate it.
> > 
> > 
> 
> OK, I experimented with that and managed to parse ranges property for PCI host
> bridge node.
> 
> I tested on my setup where the host device tree contains two PCI host bridge
> nodes with the following:
> 
> pcie@fe000000 {
> ...
>             ranges = <0x1000000 0x0 0x0 0x0 0xfe100000 0x0 0x100000 0x2000000
> 0x0 0xfe200000 0x0 0xfe200000 0x0 0x200000 0x2000000 0x0 0x30000000 0x0
> 0x30000000 0x0 0x8000000 0x42000000 0x0 0x38000000 0x0 0x38000000 0x0
> 0x8000000>;
> ...
> };
> 
> pcie@ee800000 {
> ...
>             ranges = <0x1000000 0x0 0x0 0x0 0xee900000 0x0 0x100000 0x2000000
> 0x0 0xeea00000 0x0 0xeea00000 0x0 0x200000 0x2000000 0x0 0xc0000000 0x0
> 0xc0000000 0x0 0x8000000 0x42000000 0x0 0xc8000000 0x0 0xc8000000 0x0
> 0x8000000>;
> ...
> };
> 
> So Xen retrieves the *CPU addresses* from the ranges:
> 
> (XEN) dev /soc/pcie@fe000000 range_size 7 nr_ranges 4
> (XEN) 0: addr=fe100000, size=100000
> (XEN) 1: addr=fe200000, size=200000
> (XEN) 2: addr=30000000, size=8000000
> (XEN) 3: addr=38000000, size=8000000
> (XEN) dev /soc/pcie@ee800000 range_size 7 nr_ranges 4
> (XEN) 0: addr=ee900000, size=100000
> (XEN) 1: addr=eea00000, size=200000
> (XEN) 2: addr=c0000000, size=8000000
> (XEN) 3: addr=c8000000, size=8000000
> 
> The code below covers ranges property in the context of finding memory holes
> (to be squashed with current patch):
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d37156a..7d20c10 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -834,6 +834,8 @@ static int __init find_memory_holes(struct meminfo
> *ext_regions)
>      {
>          unsigned int naddr;
>          u64 addr, size;
> +        const __be32 *ranges;
> +        u32 len;
> 
>          naddr = dt_number_of_address(np);
> 
> @@ -857,6 +859,41 @@ static int __init find_memory_holes(struct meminfo
> *ext_regions)
>                  goto out;
>              }
>          }
> +
> +        /*
> +         * Also looking for non-empty ranges property which would likely mean
> +         * that we deal with PCI host bridge device and the property here
> +         * describes the BARs for the PCI devices.
> +         */

One thing to be careful is that ranges with a valid parameter is not
only present in PCI busses. It can be present in amba and other
simple-busses too. In that case the format for ranges in simpler as it
doesn't have a "memory type" like PCI.

When you get addresses from reg, bus ranges properties are automatically
handled for you.

All of this to say that a check on "ranges" is not enough because it
might capture other non-PCI busses that have a different, simpler,
ranges format. You want to check for "ranges" under a device_type =
"pci"; node.


> +        ranges = dt_get_property(np, "ranges", &len);
> +        if ( ranges && len )
> +        {
> +            unsigned int range_size, nr_ranges;
> +            int na, ns, pna;
> +
> +            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;
> +                }
> +            }
> +        }
>      }