[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: address@hidden
Cc: David Gibson <address@hidden>
Cc: Bharata B Rao <address@hidden>
Signed-off-by: Michael Roth <address@hidden>
---
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