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.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
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
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 <david@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdroth@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
>
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 <david@gibson.dropbear.id.au>
>
> Reviewed-by: Michael Roth <mdroth@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
> >
>
>
On Wed, Jun 07, 2017 at 05:49:06PM -0500, Michael Roth wrote: > 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. Ah, ok. > 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. I see your point, but I don't think it's really worth worrying aboutg. For the DR-indicator, I don't particularly care about the state of a virtual LED in weird edge cases like this. From the guest POV it seems bogus to me to adjust the LED state before unisolating the device, but whatever. If we do ever implement explicit power control (unlikely) then the detach()/attach() would either not be permitted with device power on, or it would revert power to off, in which case the attempt to move to UNISOLATE would fail. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
© 2016 - 2025 Red Hat, Inc.