On Sep 9 11:43, Hannes Reinecke wrote: > With commit 5ffbaeed16 ("hw/nvme: fix controller hot unplugging") > namespaces get moved from the controller to the subsystem if one > is specified. > That keeps the namespaces alive after a controller hot-unplug, but > after a controller hotplug we have to reconnect the namespaces > from the subsystem to the controller. > > Fixes: 5ffbaeed16 ("hw/nvme: fix controller hot unplugging") > Cc: Klaus Jensen <k.jen...@samsung.com> > Signed-off-by: Hannes Reinecke <h...@suse.de> > --- > hw/nvme/subsys.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/nvme/subsys.c b/hw/nvme/subsys.c > index 93c35950d6..a9404f2b5e 100644 > --- a/hw/nvme/subsys.c > +++ b/hw/nvme/subsys.c > @@ -14,7 +14,7 @@ > int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp) > { > NvmeSubsystem *subsys = n->subsys; > - int cntlid; > + int cntlid, nsid; > > for (cntlid = 0; cntlid < ARRAY_SIZE(subsys->ctrls); cntlid++) { > if (!subsys->ctrls[cntlid]) { > @@ -29,12 +29,18 @@ int nvme_subsys_register_ctrl(NvmeCtrl *n, Error **errp) > > subsys->ctrls[cntlid] = n; > > + for (nsid = 0; nsid < ARRAY_SIZE(subsys->namespaces); nsid++) { > + if (subsys->namespaces[nsid]) { > + nvme_attach_ns(n, subsys->namespaces[nsid]); > + }
Thanks Hannes! I like it, keeping things simple. But we should only attach namespaces that have the shared property or have ns->attached == 0. Non-shared namespaces may already be attached to another controller in the subsystem. However... The spec says that "The attach and detach operations are persistent across all reset events.". This means that we should track those events in the subsystem and only reattach namespaces that were attached at the time of the "reset" event. Currently, we don't have anything mapping that state. But the device already has to take some liberties with regard to stuff that is considered persistent by the spec (SMART log etc.) since we do not have any way to store stuff persistently across qemu invocations, so I think the above is an acceptable compromise. A potential (as good as it gets) fix would be to keep a map/list of "persistently" attached controllers on the namespaces and re-attach according to that when we see that controller joining the subsystem again. CNTLID would be the obvious choice for the key here, but problem is that we cant really use it since we assign it sequentially from the subsystem, which now looks like a pretty bad choice. CNTLID should have been a required property of the nvme device when subsystems are involved. Maybe we can fix up the CNTLID assignment to take the serial into account (we know that is defined and *should* be unique) and not reuse CNTLIDs. This limits the subsystem to NVME_MAX_CONTROLLERS unique controllers, but I think that is fair enough. Sigh. Need to think this through. Bottomline I think I'm partial to just accepting your patch (with the addition of taking the shared property into account) and documenting the limitation wrt. persistency of attach/detach events. No matter how spec-compliant we do it on a live system, we still break compliance across QEMU invocations.
signature.asc
Description: PGP signature