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

[Michael Roth]

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