Re: [Qemu-ppc] [Qemu-devel] [FIX PATCH] spapr_drc: Return correct state for logical DR in entity_sense()

[David Gibson]

On Wed, Sep 09, 2015 at 09:32:32AM +0530, Bharata B Rao wrote:
> On Tue, Sep 08, 2015 at 05:03:25PM -0500, Michael Roth wrote:
> > Quoting Michael Roth (2015-09-08 16:03:56)
> > > Quoting David Gibson (2015-09-07 20:22:50)
> > > > On Mon, Sep 07, 2015 at 11:37:04AM +0530, Bharata B Rao wrote:
> > > > > When drmgr is run in the guest to add a device for which device_add
> > > > > hasn't been issued in QEMU, configure-connector call fails.
> > > > > When configure-connector call fails, the guest would release (*)
> > > > > the previously acquired DRC by setting back the DRC isolation state
> > > > > to ISOLATED and allocation state to UNUSABLE. These calls will be 
> > > > > issued
> > > > > only if get-sensor-state call returns PRESENT state. However 
> > > > > currently for
> > > > > a logical DR, entity_sense() would unconditinally return UNUSABLE
> > > > > state only. This prevents any subsequent hotplug of the device with
> > > > > that DRC.
> > > 
> > > This seems a little odd. I think we default to ALLOCATION_STATE_UNUSABLE
> > > for logical DR, and it's up the guest to transition to USABLE, which
> > > probably happens prior to the configure-connector calls. So I think the
> > > net effect of this fix is that guest will see these unallocated/unattached
> > > resources the same way they would a resource that was actually attached
> > > via device_add, and all we're really doing is working around the
> > > eventual configuration failure that that will lead to by pretending a
> > > resource was actually there.
> > > 
> > > According to PAPR+ 2.7:
> > > 
> > > 13.7.3.1 Acquire Logical Resource from Resource Pool:
> > > 
> > >   If the state is “unusable” the OS issues set-indicator 
> > > (allocation-state, usable) to attempt to allocate the re-
> > >   source. Similarly, if the state is “available for exchange” the OS 
> > > issues set-indicator (allocation-state, ex-
> > >   change) to attempt to allocate the resource, and if the state is 
> > > “available for recovery” the OS issues
> > >   set-indicator (allocation-state, recover) to attempt to allocate the 
> > > resource.
> > > 
> > > and
> > > 
> > > 13.7 Logical Resource Dynamic Reconfiguration (LRDR):
> > > 
> > >   The OS may use the get-sensor-state RTAS call with the dr-entity-sense 
> > > token to deter-
> > > mine if a given drc-index refers to a connector that is currently usable 
> > > for DR operations. If the connector is not
> > > currently usable the return state is “DR entity unusable” (2). A 
> > > set-indicator (isolation state) RTAS call to an unusable
> > > connector or (dr-indicator) to any logical resource connector results in 
> > > a “No such indicator implemented” return sta-
> > > tus.  
> > > 
> > > So I think maybe the proper fix is to make sure that
> > > drc->set_indicator_state() fails with an error that indicates to RTAS to
> > > return NO_SENSOR (-3) for cases where we haven't attached a resource
> > > to the DRC via device_add.
> > 
> > Patch incoming:
> > 
> >   spapr_drc: don't allow 'empty' DRCs to be unisolated
> > 
> > applies to spapr-next but requires revert of this patch.
> 
> Yes, please revert this patch in favour of Michael's.

Done.

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

pgpqPLEvWM0Sg.pgp
Description: PGP signature