[SLOF] [PATCH] Correctly set ibm,my-drc-index

[Michael Roth]

Quoting David Gibson (2015-09-13 22:57:15)
> On Sun, 13 Sep 2015 21:25:17 -0500
> Michael Roth <mdroth at linux.vnet.ibm.com> wrote:
> 
> > Quoting David Gibson (2015-09-13 18:45:00)
> > > On Fri, 11 Sep 2015 18:48:19 +0200
> > > Laurent Vivier <lvivier at redhat.com> wrote:
> > > 
> > > > The current calculation of DRC index for PCI card doesn't take
> > > > into account the PHB bridge index, so with several bridges we
> > > > can have several devices with the same DRC index.
> > > > 
> > > > This is a problem when we want to unplug a PCI card with
> > > > drmgr which is based on the DRC index.
> > > > 
> > > > Signed-off-by: Laurent Vivier <lvivier at redhat.com>
> > > 
> > > Actually, I think the safer fix is just for SLOF not to touch the
> > > ibm,my-drc-index property if it's already present.  QEMU is already
> > > setting it correctly, and SLOF is overwriting it  :(.
> > 
> > This patch makes sense and fixes a real bug, but my understanding is that
> > SLOF only fills in properties missing from device node rather than
> > overwriting ones provided by QEMU. On my end, on the second PHB, I see
> > this for a hotplugged device in slot 1 (where prop comes from QEMU via
> > RTAS):
> > 
> > mdroth at ubuntu:/proc/device-tree/pci at 800000020000001/pci at 2$ od -x
> > ibm,my-drc-index
> > 0000000 0140 1000
> > 0000004
> > 
> > And after reboot (where prop comes from QEMU via SLOF):
> > 
> > mdroth at ubuntu:/proc/device-tree/pci at 800000020000001/ethernet at 2$ od -x
> > ibm,my-drc-index
> > 0000000 0140 1000
> > 0000004
> > 
> > If you're getting the buggy values (0040 1000, i.e. 40000010 be32 value)
> > it makes me think you're not using the QEMU-generated DTs for device
> > nodes. Older SLOF that doesn't have Nikunj's updates possibly? I think
> > in that case values might get overwritten/thrown out.
> 
> 
> Ah.  Good point, yes, the downstream qemu doesn't have the code to set
> my-drc-index.
> 
> So the question is, do we fix this in SLOF, or do we pull down the qemu
> changes to move more device node creation into qemu, making it moot.

Personally, I think it's best to pull in QEMU changes and relevant SLOF
updates to make that work. Upstream hotplug code was done with those
changes in place, and I think re-implemented on SLOF-generated device
properties will require a bit more work than this patch suggests.

Off-hand there's at least a couple more things you'd need to get this
working as it is with upstream QEMU/SLOF:

1) Hotplug of individual functions for multi-function devices was added
   after this SLOF code, so to get that working I think you'd need something
   like:

   40000000h + $phbindex << 16 + $bus << 8 + $slot << 3 + $function

2) For emulated devices, I'm not sure ibm,loc-code value matters all
   that much, but it probably at least needs to be unique, and so should
   incorporate the device fn similar to above and as we do with QEMU
   device nodes.

3) For VFIO devices, there's some fairly gnarly loc-code handling that
   was added in QEMU commit 16b0ea1d852873cf17630133d86df6a68e23f38c.
   I'm not 100% sure on why it's required, maybe Nikunj can comment,
   but that will be probably be a nightmare to implement in SLOF unless
   you have QEMU pass it down, which is somewhat self-defeating if the
   goal is to avoid the QEMU property generation.

   Buried in that update is also a subtle fix for VFIO devices that
   don't have a host-generated loc-code/devspec (like SR-IOV VFs)

With all these changes in place I *think* you can get an old SLOF up
to the task, but I'm not certain.

OTOH, Nikunj's SLOF patch to use QEMU-generated properties is pretty
straightforward, and most of the QEMU code to generate properties
will already be there as part of any backported PCI hotplug code, so
I think that's the less painful approach for downstreams that want
hotplug support.

I agree on the points about not keeping around broken code, and do
think we should eventually start pulling out device-tree stuff from
SLOF, but QEMU 2.4 is the first release with QEMU-generated device
properties so I think it makes sense to keep the code around for
a while. I don't think that needs to imply that we need to fully
support hotplug on top of SLOF-generated props though, since the
idea is that this might be older QEMUs (no hotplug anyway) using
newer SLOF to pull in other bug fixes. If hotplug is needed I
think it's best to assume there's a tight coupling with
QEMU-generated device props.

> 
> 
> > 
> > > 
> > > Adding Mike Roth to CC, since git blame says he wrote this bit of SLOF
> > > code.
> > > 
> > > >  slof/fs/pci-properties.fs | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/slof/fs/pci-properties.fs b/slof/fs/pci-properties.fs
> > > > index 4f13402..d132426 100644
> > > > --- a/slof/fs/pci-properties.fs
> > > > +++ b/slof/fs/pci-properties.fs
> > > > @@ -599,9 +599,10 @@
> > > >          pci-reg-props
> > > >          pci-hotplug-enabled IF
> > > >              \ QEMU uses static assignments for my-drc-index:
> > > > -            \ 40000000h + $bus << 8 + $slot << 3
> > > > +            \ 40000000h + $phbindex << 16 + $bus << 8 + $slot << 3
> > > >              dup dup pci-addr2bus 8 lshift
> > > >              swap pci-addr2dev 3 lshift or
> > > > +            puid ff and 10 lshift or
> > > >              40000000 + encode-int s" ibm,my-drc-index" property
> > > >              \ QEMU uses "Slot $bus*32$slotno" for loc-code
> > 
> > I think loc-code may need to be updated to match QEMU as well.
> > 
> > > >              dup dup pci-addr2bus 20 *
> > > > -- 
> > > > 2.1.0
> > > > 
> > > 
> > > 
> > > -- 
> > > David Gibson <dgibson at redhat.com>
> > > Senior Software Engineer, Virtualization, Red Hat
> > 
> 
> 
> -- 
> David Gibson <dgibson at redhat.com>
> Senior Software Engineer, Virtualization, Red Hat