[Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[Michael Roth]

Logical resources start with allocation-state:UNUSABLE /
isolation-state:ISOLATED. During hotplug, guests will transition
them to allocate-state:USABLE, and then to isolate-state:UNISOLATED.
The former transition does not seem to have any failure path for
cases where a DRC does not have any resources associated with it to
allocate for guest, but instead relies on the subsequent
isolation-state:UNISOLATED transition to indicate failure in this
situation.

Currently DRC code does not implement this logic, but instead
tries to indicate failure by refusing the allocation-state:USABLE
transition. Unfortunately, since that's not a documented failure
path, guests continue undeterred, causing undefined behavior in
QEMU and guest code.

Fix this by handling things as PAPR defines (13.7 and 13.7.3.1).

Cc: qemu-ppc@nongnu.org
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
v2:
 - actually include the full changeset in the patch
---
 hw/ppc/spapr_drc.c         | 12 ++++++++++++
 hw/ppc/spapr_rtas.c        |  9 +++++++--
 include/hw/ppc/spapr.h     |  1 +
 include/hw/ppc/spapr_drc.h |  2 ++
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 9ce844a..c1f664f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -66,6 +66,18 @@ static int set_isolation_state(sPAPRDRConnector *drc,
 
     DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
 
+    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
+        /* cannot unisolate a non-existant resource. this generally
+         * happens for logical resources where transitions from
+         * allocation-state:UNUSABLE to allocation-state:USABLE are
+         * unguarded, but instead rely on a subsequent
+         * isolation-state:UNISOLATED transition to indicate failure
+         */
+        if (!drc->dev) {
+            return -1;
+        }
+    }
+
     drc->isolation_state = state;
 
     if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 3b7b20b..0ddedca 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -372,6 +372,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     uint32_t sensor_type;
     uint32_t sensor_index;
     uint32_t sensor_state;
+    int drc_ret, ret = RTAS_OUT_SUCCESS;
     sPAPRDRConnector *drc;
     sPAPRDRConnectorClass *drck;
 
@@ -413,7 +414,11 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                 spapr_ccs_remove(spapr, ccs);
             }
         }
-        drck->set_isolation_state(drc, sensor_state);
+        drc_ret = drck->set_isolation_state(drc, sensor_state);
+        if (drc_ret != 0) {
+            ret = (drc_ret == -1) ? RTAS_OUT_NO_SUCH_INDICATOR
+                                  : RTAS_OUT_HW_ERROR;
+        }
         break;
     case RTAS_SENSOR_TYPE_DR:
         drck->set_indicator_state(drc, sensor_state);
@@ -425,7 +430,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         goto out_unimplemented;
     }
 
-    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    rtas_st(rets, 0, ret);
     return;
 
 out_unimplemented:
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index c75cc5e..ffb108d 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -412,6 +412,7 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_OUT_BUSY               -2
 #define RTAS_OUT_PARAM_ERROR        -3
 #define RTAS_OUT_NOT_SUPPORTED      -3
+#define RTAS_OUT_NO_SUCH_INDICATOR  -3
 #define RTAS_OUT_NOT_AUTHORIZED     -9002
 
 /* RTAS tokens */
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 28ffeae..b2c1209 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -165,6 +165,8 @@ typedef struct sPAPRDRConnectorClass {
     /*< public >*/
 
     /* accessors for guest-visible (generally via RTAS) DR state */
+
+    /* returns -1 if DRC cannot be set to requested isolation state */
     int (*set_isolation_state)(sPAPRDRConnector *drc,
                                sPAPRDRIsolationState state);
     int (*set_indicator_state)(sPAPRDRConnector *drc,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[Bharata B Rao]

On Tue, Sep 08, 2015 at 06:44:55PM -0500, Michael Roth wrote:
> Logical resources start with allocation-state:UNUSABLE /
> isolation-state:ISOLATED. During hotplug, guests will transition
> them to allocate-state:USABLE, and then to isolate-state:UNISOLATED.
> The former transition does not seem to have any failure path for
> cases where a DRC does not have any resources associated with it to
> allocate for guest, but instead relies on the subsequent
> isolation-state:UNISOLATED transition to indicate failure in this
> situation.
> 
> Currently DRC code does not implement this logic, but instead
> tries to indicate failure by refusing the allocation-state:USABLE
> transition. Unfortunately, since that's not a documented failure
> path, guests continue undeterred, causing undefined behavior in
> QEMU and guest code.
> 
> Fix this by handling things as PAPR defines (13.7 and 13.7.3.1).
> 
> Cc: qemu-ppc@nongnu.org
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Tested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]

On Tue, Sep 08, 2015 at 06:44:55PM -0500, Michael Roth wrote:
> Logical resources start with allocation-state:UNUSABLE /
> isolation-state:ISOLATED. During hotplug, guests will transition
> them to allocate-state:USABLE, and then to isolate-state:UNISOLATED.
> The former transition does not seem to have any failure path for
> cases where a DRC does not have any resources associated with it to
> allocate for guest, but instead relies on the subsequent
> isolation-state:UNISOLATED transition to indicate failure in this
> situation.
> 
> Currently DRC code does not implement this logic, but instead
> tries to indicate failure by refusing the allocation-state:USABLE
> transition. Unfortunately, since that's not a documented failure
> path, guests continue undeterred, causing undefined behavior in
> QEMU and guest code.
> 
> Fix this by handling things as PAPR defines (13.7 and 13.7.3.1).
> 
> Cc: qemu-ppc@nongnu.org
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
> v2:
>  - actually include the full changeset in the patch

Several queries for clarification:

 * Is this intended to replace Bharata's patch "spapr_drc:
   Return correct state for logical DR in entity_sense()" or to apply
   on top of it?

 * If I'm understanding correctly, the problem here is that although
   the guest is supposed to check for failures to set the allocation
   state, it's actually not?  This patch is to make qemu gracefully
   handle the guest's failure to do this?  Is that right?
   
> ---
>  hw/ppc/spapr_drc.c         | 12 ++++++++++++
>  hw/ppc/spapr_rtas.c        |  9 +++++++--
>  include/hw/ppc/spapr.h     |  1 +
>  include/hw/ppc/spapr_drc.h |  2 ++
>  4 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 9ce844a..c1f664f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -66,6 +66,18 @@ static int set_isolation_state(sPAPRDRConnector *drc,
>  
>      DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
>  
> +    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> +        /* cannot unisolate a non-existant resource. this generally
> +         * happens for logical resources where transitions from
> +         * allocation-state:UNUSABLE to allocation-state:USABLE are
> +         * unguarded, but instead rely on a subsequent
> +         * isolation-state:UNISOLATED transition to indicate failure
> +         */
> +        if (!drc->dev) {
> +            return -1;
> +        }
> +    }
> +
>      drc->isolation_state = state;
>  
>      if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 3b7b20b..0ddedca 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -372,6 +372,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      uint32_t sensor_type;
>      uint32_t sensor_index;
>      uint32_t sensor_state;
> +    int drc_ret, ret = RTAS_OUT_SUCCESS;
>      sPAPRDRConnector *drc;
>      sPAPRDRConnectorClass *drck;
>  
> @@ -413,7 +414,11 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                  spapr_ccs_remove(spapr, ccs);
>              }
>          }
> -        drck->set_isolation_state(drc, sensor_state);
> +        drc_ret = drck->set_isolation_state(drc, sensor_state);
> +        if (drc_ret != 0) {
> +            ret = (drc_ret == -1) ? RTAS_OUT_NO_SUCH_INDICATOR
> +                                  : RTAS_OUT_HW_ERROR;
> +        }
>          break;
>      case RTAS_SENSOR_TYPE_DR:
>          drck->set_indicator_state(drc, sensor_state);
> @@ -425,7 +430,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>          goto out_unimplemented;
>      }
>  
> -    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +    rtas_st(rets, 0, ret);
>      return;
>  
>  out_unimplemented:
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index c75cc5e..ffb108d 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -412,6 +412,7 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>  #define RTAS_OUT_BUSY               -2
>  #define RTAS_OUT_PARAM_ERROR        -3
>  #define RTAS_OUT_NOT_SUPPORTED      -3
> +#define RTAS_OUT_NO_SUCH_INDICATOR  -3
>  #define RTAS_OUT_NOT_AUTHORIZED     -9002
>  
>  /* RTAS tokens */
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 28ffeae..b2c1209 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -165,6 +165,8 @@ typedef struct sPAPRDRConnectorClass {
>      /*< public >*/
>  
>      /* accessors for guest-visible (generally via RTAS) DR state */
> +
> +    /* returns -1 if DRC cannot be set to requested isolation state */
>      int (*set_isolation_state)(sPAPRDRConnector *drc,
>                                 sPAPRDRIsolationState state);
>      int (*set_indicator_state)(sPAPRDRConnector *drc,

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[Michael Roth]

Quoting David Gibson (2015-09-08 23:10:41)
> On Tue, Sep 08, 2015 at 06:44:55PM -0500, Michael Roth wrote:
> > Logical resources start with allocation-state:UNUSABLE /
> > isolation-state:ISOLATED. During hotplug, guests will transition
> > them to allocate-state:USABLE, and then to isolate-state:UNISOLATED.
> > The former transition does not seem to have any failure path for
> > cases where a DRC does not have any resources associated with it to
> > allocate for guest, but instead relies on the subsequent
> > isolation-state:UNISOLATED transition to indicate failure in this
> > situation.
> > 
> > Currently DRC code does not implement this logic, but instead
> > tries to indicate failure by refusing the allocation-state:USABLE
> > transition. Unfortunately, since that's not a documented failure
> > path, guests continue undeterred, causing undefined behavior in
> > QEMU and guest code.
> > 
> > Fix this by handling things as PAPR defines (13.7 and 13.7.3.1).
> > 
> > Cc: qemu-ppc@nongnu.org
> > Cc: David Gibson <david@gibson.dropbear.id.au>
> > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> > v2:
> >  - actually include the full changeset in the patch
> 
> Several queries for clarification:

Looks like you've already picked this up, but just in case:

> 
>  * Is this intended to replace Bharata's patch "spapr_drc:
>    Return correct state for logical DR in entity_sense()" or to apply
>    on top of it?

Yes this should replace that patch.

> 
>  * If I'm understanding correctly, the problem here is that although
>    the guest is supposed to check for failures to set the allocation
>    state, it's actually not?  This patch is to make qemu gracefully
>    handle the guest's failure to do this?  Is that right?

The root issue is that allocation state setting doesn't actually have
a documented failure path. Instead, the subsequent isolation state
setting is where we surface an error if we're unable to actually
provide the device to the guest (due to it not being attached
to the DRC in question).

We weren't surfacing that error in the isolation state setting, so the
guest was continuing on with configuring devices that aren't actually
present. This patch should correct that.

Personally it seems like allocation state setting is where that failure
should occur, since that's the earliest point to surface the error, but
that's how PAPR has it documented.

>    
> > ---
> >  hw/ppc/spapr_drc.c         | 12 ++++++++++++
> >  hw/ppc/spapr_rtas.c        |  9 +++++++--
> >  include/hw/ppc/spapr.h     |  1 +
> >  include/hw/ppc/spapr_drc.h |  2 ++
> >  4 files changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 9ce844a..c1f664f 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -66,6 +66,18 @@ static int set_isolation_state(sPAPRDRConnector *drc,
> >  
> >      DPRINTFN("drc: %x, set_isolation_state: %x", get_index(drc), state);
> >  
> > +    if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
> > +        /* cannot unisolate a non-existant resource. this generally
> > +         * happens for logical resources where transitions from
> > +         * allocation-state:UNUSABLE to allocation-state:USABLE are
> > +         * unguarded, but instead rely on a subsequent
> > +         * isolation-state:UNISOLATED transition to indicate failure
> > +         */
> > +        if (!drc->dev) {
> > +            return -1;
> > +        }
> > +    }
> > +
> >      drc->isolation_state = state;
> >  
> >      if (drc->isolation_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 3b7b20b..0ddedca 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -372,6 +372,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      uint32_t sensor_type;
> >      uint32_t sensor_index;
> >      uint32_t sensor_state;
> > +    int drc_ret, ret = RTAS_OUT_SUCCESS;
> >      sPAPRDRConnector *drc;
> >      sPAPRDRConnectorClass *drck;
> >  
> > @@ -413,7 +414,11 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                  spapr_ccs_remove(spapr, ccs);
> >              }
> >          }
> > -        drck->set_isolation_state(drc, sensor_state);
> > +        drc_ret = drck->set_isolation_state(drc, sensor_state);
> > +        if (drc_ret != 0) {
> > +            ret = (drc_ret == -1) ? RTAS_OUT_NO_SUCH_INDICATOR
> > +                                  : RTAS_OUT_HW_ERROR;
> > +        }
> >          break;
> >      case RTAS_SENSOR_TYPE_DR:
> >          drck->set_indicator_state(drc, sensor_state);
> > @@ -425,7 +430,7 @@ static void rtas_set_indicator(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >          goto out_unimplemented;
> >      }
> >  
> > -    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > +    rtas_st(rets, 0, ret);
> >      return;
> >  
> >  out_unimplemented:
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index c75cc5e..ffb108d 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -412,6 +412,7 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
> >  #define RTAS_OUT_BUSY               -2
> >  #define RTAS_OUT_PARAM_ERROR        -3
> >  #define RTAS_OUT_NOT_SUPPORTED      -3
> > +#define RTAS_OUT_NO_SUCH_INDICATOR  -3
> >  #define RTAS_OUT_NOT_AUTHORIZED     -9002
> >  
> >  /* RTAS tokens */
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 28ffeae..b2c1209 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -165,6 +165,8 @@ typedef struct sPAPRDRConnectorClass {
> >      /*< public >*/
> >  
> >      /* accessors for guest-visible (generally via RTAS) DR state */
> > +
> > +    /* returns -1 if DRC cannot be set to requested isolation state */
> >      int (*set_isolation_state)(sPAPRDRConnector *drc,
> >                                 sPAPRDRIsolationState state);
> >      int (*set_indicator_state)(sPAPRDRConnector *drc,
> 
> -- 
> 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

Re: [Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]

On Wed, Sep 09, 2015 at 12:19:22PM -0500, Michael Roth wrote:
> Quoting David Gibson (2015-09-08 23:10:41)
> > On Tue, Sep 08, 2015 at 06:44:55PM -0500, Michael Roth wrote:
> > > Logical resources start with allocation-state:UNUSABLE /
> > > isolation-state:ISOLATED. During hotplug, guests will transition
> > > them to allocate-state:USABLE, and then to isolate-state:UNISOLATED.
> > > The former transition does not seem to have any failure path for
> > > cases where a DRC does not have any resources associated with it to
> > > allocate for guest, but instead relies on the subsequent
> > > isolation-state:UNISOLATED transition to indicate failure in this
> > > situation.
> > > 
> > > Currently DRC code does not implement this logic, but instead
> > > tries to indicate failure by refusing the allocation-state:USABLE
> > > transition. Unfortunately, since that's not a documented failure
> > > path, guests continue undeterred, causing undefined behavior in
> > > QEMU and guest code.
> > > 
> > > Fix this by handling things as PAPR defines (13.7 and 13.7.3.1).
> > > 
> > > Cc: qemu-ppc@nongnu.org
> > > Cc: David Gibson <david@gibson.dropbear.id.au>
> > > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > ---
> > > v2:
> > >  - actually include the full changeset in the patch
> > 
> > Several queries for clarification:
> 
> Looks like you've already picked this up, but just in case:
> 
> > 
> >  * Is this intended to replace Bharata's patch "spapr_drc:
> >    Return correct state for logical DR in entity_sense()" or to apply
> >    on top of it?
> 
> Yes this should replace that patch.
> 
> > 
> >  * If I'm understanding correctly, the problem here is that although
> >    the guest is supposed to check for failures to set the allocation
> >    state, it's actually not?  This patch is to make qemu gracefully
> >    handle the guest's failure to do this?  Is that right?
> 
> The root issue is that allocation state setting doesn't actually have
> a documented failure path. Instead, the subsequent isolation state
> setting is where we surface an error if we're unable to actually
> provide the device to the guest (due to it not being attached
> to the DRC in question).

Um.. 13.7.3.1 (step 2) implies checking for failures on setting
allocation state, and 13.5.3.5 Table 177 says "If the transition to
the usable state is not possible the -3 (no such indicator
implemented) status is returned."

How is that not a documented failure path?

> We weren't surfacing that error in the isolation state setting, so the
> guest was continuing on with configuring devices that aren't actually
> present. This patch should correct that.

So, to be clear, I think the check in set isolation-state is a good
idea as well, to properly handle a misbehaving guest.

> Personally it seems like allocation state setting is where that failure
> should occur, since that's the earliest point to surface the error, but
> that's how PAPR has it documented.

.. but I'm not seeing how PAPR is documenting it as happening in set
isolation state rather than set allocation-state.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2] spapr_drc: don't allow 'empty' DRCs to be unisolated

[Michael Roth]

Quoting David Gibson (2015-09-09 20:18:01)
> On Wed, Sep 09, 2015 at 12:19:22PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2015-09-08 23:10:41)
> > > On Tue, Sep 08, 2015 at 06:44:55PM -0500, Michael Roth wrote:
> > > > Logical resources start with allocation-state:UNUSABLE /
> > > > isolation-state:ISOLATED. During hotplug, guests will transition
> > > > them to allocate-state:USABLE, and then to isolate-state:UNISOLATED.
> > > > The former transition does not seem to have any failure path for
> > > > cases where a DRC does not have any resources associated with it to
> > > > allocate for guest, but instead relies on the subsequent
> > > > isolation-state:UNISOLATED transition to indicate failure in this
> > > > situation.
> > > > 
> > > > Currently DRC code does not implement this logic, but instead
> > > > tries to indicate failure by refusing the allocation-state:USABLE
> > > > transition. Unfortunately, since that's not a documented failure
> > > > path, guests continue undeterred, causing undefined behavior in
> > > > QEMU and guest code.
> > > > 
> > > > Fix this by handling things as PAPR defines (13.7 and 13.7.3.1).
> > > > 
> > > > Cc: qemu-ppc@nongnu.org
> > > > Cc: David Gibson <david@gibson.dropbear.id.au>
> > > > Cc: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > ---
> > > > v2:
> > > >  - actually include the full changeset in the patch
> > > 
> > > Several queries for clarification:
> > 
> > Looks like you've already picked this up, but just in case:
> > 
> > > 
> > >  * Is this intended to replace Bharata's patch "spapr_drc:
> > >    Return correct state for logical DR in entity_sense()" or to apply
> > >    on top of it?
> > 
> > Yes this should replace that patch.
> > 
> > > 
> > >  * If I'm understanding correctly, the problem here is that although
> > >    the guest is supposed to check for failures to set the allocation
> > >    state, it's actually not?  This patch is to make qemu gracefully
> > >    handle the guest's failure to do this?  Is that right?
> > 
> > The root issue is that allocation state setting doesn't actually have
> > a documented failure path. Instead, the subsequent isolation state
> > setting is where we surface an error if we're unable to actually
> > provide the device to the guest (due to it not being attached
> > to the DRC in question).
> 
> Um.. 13.7.3.1 (step 2) implies checking for failures on setting
> allocation state, and 13.5.3.5 Table 177 says "If the transition to
> the usable state is not possible the -3 (no such indicator
> implemented) status is returned."
> 
> How is that not a documented failure path?

Hmm, I think I misunderpreted this statement from 13.7:

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

To me it seemed to suggest that isolation-state is where we fail
attempts to bring up an UNUSABLE resource, but the statement doesn't
actually preclude us from returning error on setting allocation-state.

I'm not sure how I misinterpreted 13.7.3.1 to support this assumption,
it does seem to clearly indicate setting allocation-state can return
error (and in fact mentions nothing about accounting for isolation-state
errors in that case), and 13.5.3.5 makes it clear it should also be
-3 in that case.

I'll send a v3 with the set-allocation logic in place. I'll also look
into returning RTAS errors directly. Thanks for the catch!

> 
> > We weren't surfacing that error in the isolation state setting, so the
> > guest was continuing on with configuring devices that aren't actually
> > present. This patch should correct that.
> 
> So, to be clear, I think the check in set isolation-state is a good
> idea as well, to properly handle a misbehaving guest.
> 
> > Personally it seems like allocation state setting is where that failure
> > should occur, since that's the earliest point to surface the error, but
> > that's how PAPR has it documented.
> 
> .. but I'm not seeing how PAPR is documenting it as happening in set
> isolation state rather than set allocation-state.
> 
> -- 
> 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