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

[David Gibson]

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

signature.asc
Description: PGP signature