Re: [Qemu-devel] [RFC 2/3] spapr: Start hotplugged PCI devices in ISOLATED state

[Michael Roth]

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 <address@hidden>
> 
> Reviewed-by: Michael Roth <address@hidden>
> 
> > ---
> >  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
> > 
> 
>