Re: [PATCH] hw/block/nvme: re-enable NVMe PCI hotplug

[Daniel Wagner <wagi@monom.org>]

On Tue, May 11, 2021 at 06:12:47PM +0200, Hannes Reinecke wrote:
> On 5/11/21 6:03 PM, Klaus Jensen wrote:
> > On May 11 16:54, Hannes Reinecke wrote:
> > > On 5/11/21 3:37 PM, Klaus Jensen wrote:
> > > > On May 11 15:12, Hannes Reinecke wrote:
> > > > > On 5/11/21 2:22 PM, Klaus Jensen wrote:
> > > [ .. ]
> > > > > > The hotplug fix looks good - I'll post a series that
> > > > > > tries to integrate
> > > > > > both.
> > > > > > 
> > > > > Ta.
> > > > > 
> > > > > The more I think about it, the more I think we should be looking into
> > > > > reparenting the namespaces to the subsystem.
> > > > > That would have the _immediate_ benefit that 'device_del' and
> > > > > 'device_add' becomes symmetric (ie one doesn't have to do a separate
> > > > > 'device_add nvme-ns'), as the nvme namespace is not affected by the
> > > > > hotplug event.
> > > > > 
> > > > 
> > > > I have that working, but I'm struggling with a QEMU API technicality in
> > > > that I apparently cannot simply move the NvmeBus creation to the
> > > > nvme-subsys device. For some reason the bus is not available for the
> > > > nvme-ns devices. That is, if one does something like this:
> > > > 
> > > >   -device nvme-subsys,...
> > > >   -device nvme-ns,...
> > > > 
> > > > Then I get an error that "no 'nvme-bus' bus found for device 'nvme'ns".
> > > > This is probably just me not grok'ing the qdev well enough, so I'll keep
> > > > trying to fix that. What works now is to have the regular setup:
> > > > 
> > > _Normally_ the 'id' of the parent device spans a bus, so the syntax
> > > should be
> > > 
> > > -device nvme-subsys,id=subsys1,...
> > > -device nvme-ns,bus=subsys1,...
> > 
> > Yeah, I know, I just oversimplified the example. This *is* how I wanted
> > it to work ;)
> > 
> > > 
> > > As for the nvme device I would initially expose any namespace from the
> > > subsystem to the controller; the nvme spec has some concept of 'active'
> > > or 'inactive' namespaces which would allow us to blank out individual
> > > namespaces on a per-controller basis, but I fear that's not easy to
> > > model with qdev and the structure above.
> > > 
> > 
> > The nvme-ns device already supports the boolean 'detached' parameter to
> > support the concept of an inactive namespace.
> > 
> Yeah, but that doesn't really work if we move the namespace to the
> subsystem; the 'detached' parameter is for the controller<->namespace
> relationship.
> That's why I meant we have to have some sort of NSID map for the controller
> such that the controller knows with NSID to access.
> I guess we can copy the trick with the NSID array, and reverse the operation
> we have now wrt subsystem; keep the main NSID array in the subsystem, and
> per-controller NSID arrays holding those which can be accessed.
> 
> And ignore the commandline for now; figure that one out later.
> 
> Cheers,
> 
> Hannes
> > > >   -device nvme-subsys,...
> > > >   -device nvme,...
> > > >   -device nvme-ns,...
> > > > 
> > > > And the nvme-ns device will then reparent to the NvmeBus on nvme-subsys
> > > > (which magically now IS available when nvme-ns is realized). This has
> > > > the same end result, but I really would like that the namespaces could
> > > > be specified as children of the subsys directly.
> > > > 
> > > Shudder.
> > > Automatic reparenting.
> > > To my understanding from qdev that shouldn't even be possible.
> > > Please don't.
> > > 
> > 
> > It's perfectly possible with the API and used to implement stuff like
> > failover. We are not changing the parent object, we are changing the
> > parent bus. hw/sd does something like this (but does mention that its a
> > bit of a hack). In this case I'd say we could argue to get away with it
> > as well.
> > Allowing the nvme-ns device to be a child of the controller allows the
> > initially attached controller of non-shared namespaces to be easily
> > expressible. But I agree, the approach is a bit wacky, which is why I
> > havnt posted anything yet.
> 
> Yep, I did try to implement multipathing for SCSI at one point, and that
> became patently horrible as it would run against qdev where everything is
> ordered within a tree.

Sorry to ask but has there been any progress on this topic? Just run
into the same issue that adding nvme device during runtime is not
showing any namespace.

Thanks,
Daniel