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

[David Gibson]

On Tue, Sep 15, 2015 at 07:03:59PM -0500, Michael Roth wrote:
> 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.

Ugh, yes, I think you're right.

I earlier made the call that not backporting the qemu-generated
properties stuff was the lower-risk path, but based on the above, I've
realised I was mistaken.

Laurent,  can you have a look today at how much is involved in
backporting that.

> 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.


-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/slof/attachments/20150916/2eec2b81/attachment.sig>