Re: [PATCH kernel] powerpc/pci/of: Parse unassigned resources

[Sam Bobroff <sbobroff@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3749 bytes --]

On Fri, Jun 14, 2019 at 01:18:28PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> On 14/06/2019 12:59, Alexey Kardashevskiy wrote:
> > The pseries platform uses the PCI_PROBE_DEVTREE method of PCI probing
> > which is basically reading "assigned-addresses" of every PCI device.
> > However if the property is missing or zero sized, then there is
> > no fallback of any kind and the PCI resources remain undiscovered, i.e.
> > pdev->resource[] array is empty.
> > 
> > This adds a fallback which parses the "reg" property in pretty much same
> > way except it marks resources as "unset" which later makes Linux assign
> > those resources with proper addresses.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > 
> > This is an attempts to boot linux directly under QEMU without slof/rtas;
> > the aim is to use petitboot instead and let the guest kernel configure
> > devices.
> > 
> > QEMU does not allocate resources, it creates correct "reg" and zero length
> > "assigned-addresses" (which is probably a bug on its own) which is
> > normally populated by SLOF later but not during this exercise.

Hi Alexey,

This patch (fixed, as you point out below) also seems to improve hotplug
on pSeries.

Currently, the PCI hotplug driver for pSeries (rpaphp) uses generic PCI
scanning to add hot plugged devices, rather than slot power control,
because the slot power control method doesn't work.

AFAIK one of the reasons that slot power control doesn't work is that
the assigned-addresses node isn't populated by QEMU during hotplug, so I
tested this patch on a guest that has been modified to use that method.

In combination with a QEMU change to prevent PCI_PROBE_ONLY being set
(necessary to allow pcibios_finish_adding_to_bus() to do resource
allocation -- I assume you are using a similar change), I was able to
successfully hot plug a few devices!

So this change seems to be a step in the right direction.

(I also tested it with an unmodified guest, and it doesn't seem to harm
hotpluging via generic PCI scanning.)

One nit: I think that calling the variable "unset" is a bit confusing.
What about calling it "aa_missing" or something like that?

Cheers,
Sam.

> > ---
> >  arch/powerpc/kernel/pci_of_scan.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> > index 64ad92016b63..cfe6ec3c6aaf 100644
> > --- a/arch/powerpc/kernel/pci_of_scan.c
> > +++ b/arch/powerpc/kernel/pci_of_scan.c
> > @@ -82,10 +82,18 @@ static void of_pci_parse_addrs(struct device_node *node, struct pci_dev *dev)
> >  	const __be32 *addrs;
> >  	u32 i;
> >  	int proplen;
> > +	bool unset = false;
> >  
> >  	addrs = of_get_property(node, "assigned-addresses", &proplen);
> >  	if (!addrs)
> >  		return;
> 
> 
> Ah. Of course, these 2 lines above should go, my bad. I'll repost if
> there are no other (and bigger) problems with this.
> 
> 
> 
> > +	if (!addrs || !proplen) {
> > +		addrs = of_get_property(node, "reg", &proplen);
> > +		if (!addrs || !proplen)
> > +			return;
> > +		unset = true;
> > +	}
> > +
> >  	pr_debug("    parse addresses (%d bytes) @ %p\n", proplen, addrs);
> >  	for (; proplen >= 20; proplen -= 20, addrs += 5) {
> >  		flags = pci_parse_of_flags(of_read_number(addrs, 1), 0);
> > @@ -110,6 +118,8 @@ static void of_pci_parse_addrs(struct device_node *node, struct pci_dev *dev)
> >  			continue;
> >  		}
> >  		res->flags = flags;
> > +		if (unset)
> > +			res->flags |= IORESOURCE_UNSET;
> >  		res->name = pci_name(dev);
> >  		region.start = base;
> >  		region.end = base + size - 1;
> > 
> 
> -- 
> Alexey
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]