Quoting Michael Roth (2017-06-07 17:49:06) > Quoting David Gibson (2017-06-06 08:05:33) > > PCI DRCs, and only PCI DRCs, are immediately moved to UNISOLATED isolation > > state once the device is attached. This has been there from the initial > > implementation, and it's not clear why. > > > > The state diagram in PAPR 13.4 suggests PCI devices should start in > > ISOLATED state until the guest moves them into UNISOLATED, and the code in > > the guest-side drmgr tool seems to work that way too. > > I think this was a misguided attempt to disallow detach() to finalize a > device immediately after attach(), but up until v1.3.3 drmgr actually > explicitly put the device right back into ISOLATED before doing > entity-sense, then back to UNISOLATED right before calling > configure-connector and eventually bringing the device to a configured > state. So I doesn't seem like this would have had any effect up until > drmgr v1.3.3+. > > For drmgr v1.3.3+, this patch would have the effect of allowing detach() > during the initial entity-sense/set-power-level RTAS calls the guest > might be doing in response to attach(), but the guest code seems capable > of dealing with that particular case gracefully. > > I'm a bit concerned if we have *multiple* attach()/detach() pairs being > executed while drmgr is processing a single hotplug add event though: > > host guest > > attach() > notify > rtas-set-indicator(DR_INDICATOR_ON) > rtas-entity-sense => device_present > rtas-set-power-level(on) > detach() > attach() > rtas-set-sensor-state(ISOLATION_STATE_UNISOLATED) > rtas-configure-connector => configured > guest actually onlines device, but dr-indicator and > power domain aren't necessarily in the expected > state > > In practice, this one isn't a big issue since we emulate an > 'automatic' power domain in QEMU that makes any rtas-set-power-level > calls a no-op, and we don't rely on the dr-indicator (anymore, at > least), but it does end up being the wrong value, and makes me think
Actually, since we set it explicitly in attach(), dr-indicator does end up being correct, but correct for the wrong reasons still. > we should find some way to disallow immediate detach() after attach() > outside of the scenarios set_signalled() was added for. So I think > this patch seems reasonable, but maybe patch 3 should instead > repurpose set_signalled to handle this, or be replaced with some > other alternative that has the same effect. > > > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > Reviewed-by: Michael Roth <mdr...@linux.vnet.ibm.com> > > > --- > > hw/ppc/spapr_drc.c | 10 ---------- > > 1 file changed, 10 deletions(-) > > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > > index c73fae0..22f9224 100644 > > --- a/hw/ppc/spapr_drc.c > > +++ b/hw/ppc/spapr_drc.c > > @@ -291,16 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, > > DeviceState *d, void *fdt, > > } > > g_assert(fdt || coldplug); > > > > - /* NOTE: setting initial isolation state to UNISOLATED means we can't > > - * detach unless guest has a userspace/kernel that moves this state > > - * back to ISOLATED in response to an unplug event, or this is done > > - * manually by the admin prior. if we force things while the guest > > - * may be accessing the device, we can easily crash the guest, so we > > - * we defer completion of removal in such cases to the reset() hook. > > - */ > > - if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) { > > - drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED; > > - } > > drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE; > > > > drc->dev = d; > > -- > > 2.9.4 > > > >