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. Which also kind of re-opens the discussion of whether or not drc->set_indicator_state() should return RTAS errors directly. I'd still stray away from that for now but maybe if we get more cases like this it'll start becoming more practical. > > > > Fix this by returning the right state in entity_sense() by checking > > the allocation_state of DRC. > > > > (*) > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-September/133430.html > > > > Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com> > > Cc: Michael Roth <mdr...@linux.vnet.ibm.com> > > Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> > > and applied to my tree. > > > --- > > hw/ppc/spapr_drc.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > > index 9ce844a..2586065 100644 > > --- a/hw/ppc/spapr_drc.c > > +++ b/hw/ppc/spapr_drc.c > > @@ -186,7 +186,11 @@ static sPAPRDREntitySense > > entity_sense(sPAPRDRConnector *drc) > > */ > > state = SPAPR_DR_ENTITY_SENSE_EMPTY; > > } else { > > - state = SPAPR_DR_ENTITY_SENSE_UNUSABLE; > > + if (drc->allocation_state == > > SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { > > + state = SPAPR_DR_ENTITY_SENSE_UNUSABLE; > > + } else { > > + state = SPAPR_DR_ENTITY_SENSE_PRESENT; > > + } > > } > > } > > > > -- > 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
