Re: [Qemu-devel] [PATCH v5 15/16] spapr_pci: enable basic hotplug operations

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Feb 26, 2015 at 02:06:04PM -0600, Michael Roth wrote:
> Quoting Michael Roth (2015-02-25 00:40:26)
> > Quoting Michael Roth (2015-02-24 23:17:24)
> > > Quoting David Gibson (2015-02-24 21:11:29)
> > > > On Mon, Feb 16, 2015 at 08:27:51AM -0600, Michael Roth wrote:
[snip]
> > > > > +        }
> > > > > +
> > > > > +        assigned = &rp->assigned.fields[assigned_idx++];
> > > > > +        assigned->phys_hi = cpu_to_be32(reg->phys_hi | b_n(1));
> > > > > +        assigned->phys_mid = cpu_to_be32(d->io_regions[i].addr >> 32);
> > > > > +        assigned->phys_lo = cpu_to_be32(d->io_regions[i].addr);
> > > > 
> > > > Not sure if I understood the comment above properly; should these
> > > > addresses actually be translated through an mappings above the PHB?
> > > > Not that there are any such translations on sPAPR, but...
> > > 
> > > The io_regions[i].addr values are relative to the IO/MEM address spaces for
> > > the device, which correspond to IO/MEM windows for the PHB. So I think the
> > > memory API should handle any translation above that...
> > > 
> > > Or do you mean because of the n=0/relocatable IO regions described by 'reg'?
> > > 
> > > IIUC, when n=0, reg->phys_{mid,lo} can be used to encode a starting offset,
> > > so a guest can re-assign/re-program an IO region that's already been assigned
> > > and described via the correspond fields in 'assigned-addresses', so long as
> > > uses an addr above reg->phys{mid,lo}.
> > > 
> > > So, we could use those reg->phys_{mid,lo} values as an alternative to the
> > > PHBs IO/MEM windows (I guess for platforms that don't provide these windows
> > > and just expose the full address space?).
> > > 
> > > But since we have those windows, we end up not needing this, so we always do
> > > n=0, reg->phys_mid = reg->phys_lo = 0, so the values in
> > > assigned->phys_{mid,lo} end up just being offsets into the IO/MEM windows,
> > > which correspond directly to io_regions[n].addr.
> > 
> > Argh, not sure what I was thinking. io_regions[n].addr is relative to the
> > IO/MEM window, so from the guest perspective .addr is indeed a different
> > value...
> > 
> > So maybe reg->phys_mid/reg->phys_lo do in fact need to reflect the window
> > offsets so that the relocatable region assignments are offset
> > accordingly...
> > 
> > Will look into it more.
> 
> So, maybe my first response wasn't as misguided as I thought. I haven't
> found any documentation that lays out the specifics, but the kernel code makes
> it fairly clear that the IO/MEM addresses in assigned-addresses are relative
> to PHB's IO/MEM windows.
> 
> The code basically does this:
> 
>   arch/powerpc/kernel/pci_of_scan.c: of_pci_parse_addrs()
>     addrs = of_get('assigned-addresses')
>     config_offset = addrs[0] && 0xff (phys.hi)
>     pci_bar_num = (config_offset - PCI_BASE_ADDRESS_0) / 4
>     base = &addrs[1:2] (phys.mid/phys.lo)
>     size = &addrs[3:4] (size.hi, size.lo)
>     region.start = base
>     region.end = base + size -1
>     res = &dev->resource[pci_bar_num];
>     pcibios_bus_to_resource(dev->bus, res, &region)
>   drivers/pci/host-bridge.c:pcibios_bus_to_resource(bus, bar_res, bar_region)
>     bridge = pci_host_bridge(bus)
>     for window in bridge->windows:
>       # skip if window type (IO/MEM), doesn't match device BAR's res)
>       bus_region.start = window->res->start - window->offset
>       bus_region.end = window->res->end - window->offset
>       if bus_region contains device BAR region:
>         offset = window->offset
>         break
>     //take the region, add the window offset, and use that as the resource addr
>     res->start = region->start + offset
>     res->end = region->end + offset
> 
> I tested this with the rpaphp hotplug module, which honors
> assigned-addresses (current guest hotplug uses generic pci rescan instead
> due to a long-standing issue with rpaphp only being able to handle
> 1 slot per PHB if there are devices present at module-load time), by adding
> some test code to pre-assign the BARs during hotplug (with addresses relative
> to PHB's IO/MEM windows) to make sure assigned-addresses values get properly
> translated to the correct offset in guest's io/system address space.
> 
> Will add a note in the comments.

Ok, sounds like the only problem was a confusing description in the
comment that made it seem like the assigned-address values were GPAs
rather than relative to the PHB windows.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]