[Qemu-devel] [RFC 0/3] spapr: DRC cleanups (part IV)

[Qemu-devel] [RFC 0/3] spapr: DRC cleanups (part IV)

[David Gibson]

This is an RFC draft of the fourth batch of DRC cleanup patches.

This is RFC because patches 2&3 are making the first real changes to
the DRC state management, and I'm substantially less confident about
this than the earlier more straightforward cleanups.  Especially since
I haven't had a chance to test this yet.

This gets rid of 'signalled', one of what looks to me like an
excessive number of state variables we have currently.  AFAICT it's
there only as a workaround for a pre-existing oddity in state
management.

David Gibson (3):
  spapr: Fold spapr_phb_add_pci_device() into its only caller
  spapr: Start hotplugged PCI devices in ISOLATED state
  spapr: Eliminate DRC 'signalled' state variable

 hw/ppc/spapr_drc.c         | 55 +---------------------------------------------
 hw/ppc/spapr_events.c      | 10 ---------
 hw/ppc/spapr_pci.c         | 53 +++++++++++++++++++-------------------------
 include/hw/ppc/spapr_drc.h |  2 --
 4 files changed, 23 insertions(+), 97 deletions(-)

-- 
2.9.4

[Qemu-devel] [RFC 1/3] spapr: Fold spapr_phb_add_pci_device() into its only caller

[David Gibson]

This function is fairly short, and so is its only caller.  There's no
particular logical distinction between them, so fold them together.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 53 ++++++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 46e736d..a216f61 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1344,30 +1344,6 @@ static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
     return offset;
 }
 
-static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
-                                     sPAPRPHBState *phb,
-                                     PCIDevice *pdev,
-                                     Error **errp)
-{
-    DeviceState *dev = DEVICE(pdev);
-    void *fdt = NULL;
-    int fdt_start_offset = 0, fdt_size;
-
-    fdt = create_device_tree(&fdt_size);
-    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
-    if (!fdt_start_offset) {
-        error_setg(errp, "Failed to create pci child device tree node");
-        goto out;
-    }
-
-    spapr_drc_attach(drc, DEVICE(pdev),
-                     fdt, fdt_start_offset, !dev->hotplugged, errp);
-out:
-    if (*errp) {
-        g_free(fdt);
-    }
-}
-
 /* Callback to be called during DRC release. */
 void spapr_phb_remove_pci_device_cb(DeviceState *dev)
 {
@@ -1429,6 +1405,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
     Error *local_err = NULL;
     PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
     uint32_t slotnr = PCI_SLOT(pdev->devfn);
+    void *fdt = NULL;
+    int fdt_start_offset, fdt_size;
 
     /* if DR is disabled we don't need to do anything in the case of
      * hotplug or coldplug callbacks
@@ -1438,10 +1416,10 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
          * we need to let them know it's not enabled
          */
         if (plugged_dev->hotplugged) {
-            error_setg(errp, QERR_BUS_NO_HOTPLUG,
+            error_setg(&local_err, QERR_BUS_NO_HOTPLUG,
                        object_get_typename(OBJECT(phb)));
         }
-        return;
+        goto out;
     }
 
     g_assert(drc);
@@ -1452,16 +1430,23 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
      */
     if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
         PCI_FUNC(pdev->devfn) != 0) {
-        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
+        error_setg(&local_err, "PCI: slot %d function 0 already ocuppied by %s,"
                    " additional functions can no longer be exposed to guest.",
                    slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
-        return;
+        goto out;
     }
 
-    spapr_phb_add_pci_device(drc, phb, pdev, &local_err);
+    fdt = create_device_tree(&fdt_size);
+    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
+    if (!fdt_start_offset) {
+        error_setg(&local_err, "Failed to create pci child device tree node");
+        goto out;
+    }
+
+    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
+                     !plugged_dev->hotplugged, &local_err);
     if (local_err) {
-        error_propagate(errp, local_err);
-        return;
+        goto out;
     }
 
     /* If this is function 0, signal hotplug for all the device functions.
@@ -1485,6 +1470,12 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
             }
         }
     }
+
+out:
+    if (local_err) {
+        error_propagate(errp, local_err);
+        g_free(fdt);
+    }
 }
 
 static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
-- 
2.9.4

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

[David Gibson]

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.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index c73fae0..22f9224 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -291,16 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     }
     g_assert(fdt || coldplug);
 
-    /* NOTE: setting initial isolation state to UNISOLATED means we can't
-     * detach unless guest has a userspace/kernel that moves this state
-     * back to ISOLATED in response to an unplug event, or this is done
-     * manually by the admin prior. if we force things while the guest
-     * may be accessing the device, we can easily crash the guest, so we
-     * we defer completion of removal in such cases to the reset() hook.
-     */
-    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
-        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
-    }
     drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
 
     drc->dev = d;
-- 
2.9.4

[Qemu-devel] [RFC 3/3] spapr: Eliminate DRC 'signalled' state variable

[David Gibson]

The 'signalled' field in the DRC appears to be entirely a torturous
workaround for the fact that PCI devices were started in UNISOLATED state
for unclear reasons.

1) 'signalled' is already meaningless for logical (so far, all non PCI)
DRCs.  It's always set to true (at least at any point it might be tested),
and can't be assigned any real meaning due to the way signalling works for
logical DRCs.

2) For PCI DRCs, the only time signalled would be false is when non-zero
functions of a multifunction device are hotplugged, followed by function
zero (the other way around is explicitly not permitted). In that case the
secondary function DRCs are attached, but the notification isn't sent to
the guest until function 0 is plugged.

3) signalled being false is used to allow a DRC detach to switch mode
back to ISOLATED state, which allows a secondary function to be hotplugged
then unplugged with function 0 never inserted.  Without this a secondary
function starting in UNISOLATED state couldn't be detached again without
function 0 being inserted, all the functions configured by the guest, then
sent back to ISOLATED state.

4) But now that PCI DRCs start in ISOLATED state, there's nothing to be
done.  If the guest doesn't get the notification, it won't switch the
device to UNISOLATED state, so nothing prevents it from being unplugged.
If the guest does move it to UNISOLATED state without the signal (due to
a manual drmgr call, for instance) then it really isn't safe to unplug it.


So, this patch removes the signalled variable and all code related to it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 45 +--------------------------------------------
 hw/ppc/spapr_events.c      | 10 ----------
 include/hw/ppc/spapr_drc.h |  2 --
 3 files changed, 1 insertion(+), 56 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 22f9224..7c778cf 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -155,12 +155,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc,
     return RTAS_OUT_SUCCESS;
 }
 
-/* has the guest been notified of device attachment? */
-static void set_signalled(sPAPRDRConnector *drc)
-{
-    drc->signalled = true;
-}
-
 /*
  * dr-entity-sense sensor value
  * returned via get-sensor-state RTAS calls
@@ -297,17 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     drc->fdt = fdt;
     drc->fdt_start_offset = fdt_start_offset;
     drc->configured = coldplug;
-    /* 'logical' DR resources such as memory/cpus are in some cases treated
-     * as a pool of resources from which the guest is free to choose from
-     * based on only a count. for resources that can be assigned in this
-     * fashion, we must assume the resource is signalled immediately
-     * since a single hotplug request might make an arbitrary number of
-     * such attached resources available to the guest, as opposed to
-     * 'physical' DR resources such as PCI where each device/resource is
-     * signalled individually.
-     */
-    drc->signalled = (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI)
-                     ? true : coldplug;
 
     if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
         drc->awaiting_allocation = true;
@@ -323,26 +306,6 @@ void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
 {
     trace_spapr_drc_detach(spapr_drc_index(drc));
 
-    /* if we've signalled device presence to the guest, or if the guest
-     * has gone ahead and configured the device (via manually-executed
-     * device add via drmgr in guest, namely), we need to wait
-     * for the guest to quiesce the device before completing detach.
-     * Otherwise, we can assume the guest hasn't seen it and complete the
-     * detach immediately. Note that there is a small race window
-     * just before, or during, configuration, which is this context
-     * refers mainly to fetching the device tree via RTAS.
-     * During this window the device access will be arbitrated by
-     * associated DRC, which will simply fail the RTAS calls as invalid.
-     * This is recoverable within guest and current implementations of
-     * drmgr should be able to cope.
-     */
-    if (!drc->signalled && !drc->configured) {
-        /* if the guest hasn't seen the device we can't rely on it to
-         * set it back to an isolated state via RTAS, so do it here manually
-         */
-        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
-    }
-
     if (drc->isolation_state != SPAPR_DR_ISOLATION_STATE_ISOLATED) {
         trace_spapr_drc_awaiting_isolated(spapr_drc_index(drc));
         drc->awaiting_release = true;
@@ -431,10 +394,6 @@ static void reset(DeviceState *d)
             drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UNUSABLE);
         }
     }
-
-    if (drck->dr_entity_sense(drc) == SPAPR_DR_ENTITY_SENSE_PRESENT) {
-        drck->set_signalled(drc);
-    }
 }
 
 static bool spapr_drc_needed(void *opaque)
@@ -459,7 +418,7 @@ static bool spapr_drc_needed(void *opaque)
     case SPAPR_DR_CONNECTOR_TYPE_LMB:
         rc = !((drc->isolation_state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) &&
                (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) &&
-               drc->configured && drc->signalled && !drc->awaiting_release);
+               drc->configured && !drc->awaiting_release);
         break;
     case SPAPR_DR_CONNECTOR_TYPE_PHB:
     case SPAPR_DR_CONNECTOR_TYPE_VIO:
@@ -481,7 +440,6 @@ static const VMStateDescription vmstate_spapr_drc = {
         VMSTATE_BOOL(configured, sPAPRDRConnector),
         VMSTATE_BOOL(awaiting_release, sPAPRDRConnector),
         VMSTATE_BOOL(awaiting_allocation, sPAPRDRConnector),
-        VMSTATE_BOOL(signalled, sPAPRDRConnector),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -594,7 +552,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     drck->set_isolation_state = set_isolation_state;
     drck->set_allocation_state = set_allocation_state;
     drck->release_pending = release_pending;
-    drck->set_signalled = set_signalled;
     /*
      * Reason: it crashes FIXME find and document the real reason
      */
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 171aedc..587a3da 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -475,13 +475,6 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
                                                        RTAS_LOG_TYPE_EPOW)));
 }
 
-static void spapr_hotplug_set_signalled(uint32_t drc_index)
-{
-    sPAPRDRConnector *drc = spapr_drc_by_index(drc_index);
-    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
-    drck->set_signalled(drc);
-}
-
 static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
                                     sPAPRDRConnectorType drc_type,
                                     union drc_identifier *drc_id)
@@ -528,9 +521,6 @@ static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action,
     switch (drc_type) {
     case SPAPR_DR_CONNECTOR_TYPE_PCI:
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
-        if (hp->hotplug_action == RTAS_LOG_V6_HP_ACTION_ADD) {
-            spapr_hotplug_set_signalled(drc_id->index);
-        }
         break;
     case SPAPR_DR_CONNECTOR_TYPE_LMB:
         hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_MEMORY;
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index ec0256b..b68e8a6 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -200,7 +200,6 @@ typedef struct sPAPRDRConnector {
     sPAPRConfigureConnectorState *ccs;
 
     bool awaiting_release;
-    bool signalled;
     bool awaiting_allocation;
     bool awaiting_allocation_skippable;
 
@@ -226,7 +225,6 @@ typedef struct sPAPRDRConnectorClass {
 
     /* QEMU interfaces for managing hotplug operations */
     bool (*release_pending)(sPAPRDRConnector *drc);
-    void (*set_signalled)(sPAPRDRConnector *drc);
 } sPAPRDRConnectorClass;
 
 uint32_t spapr_drc_index(sPAPRDRConnector *drc);
-- 
2.9.4

Re: [Qemu-devel] [RFC 1/3] spapr: Fold spapr_phb_add_pci_device() into its only caller

[Michael Roth]

Quoting David Gibson (2017-06-06 08:05:32)
> This function is fairly short, and so is its only caller.  There's no
> particular logical distinction between them, so fold them together.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 53 ++++++++++++++++++++++-------------------------------
>  1 file changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 46e736d..a216f61 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1344,30 +1344,6 @@ static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
>      return offset;
>  }
> 
> -static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> -                                     sPAPRPHBState *phb,
> -                                     PCIDevice *pdev,
> -                                     Error **errp)
> -{
> -    DeviceState *dev = DEVICE(pdev);
> -    void *fdt = NULL;
> -    int fdt_start_offset = 0, fdt_size;
> -
> -    fdt = create_device_tree(&fdt_size);
> -    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> -    if (!fdt_start_offset) {
> -        error_setg(errp, "Failed to create pci child device tree node");
> -        goto out;
> -    }
> -
> -    spapr_drc_attach(drc, DEVICE(pdev),
> -                     fdt, fdt_start_offset, !dev->hotplugged, errp);
> -out:
> -    if (*errp) {
> -        g_free(fdt);
> -    }
> -}
> -
>  /* Callback to be called during DRC release. */
>  void spapr_phb_remove_pci_device_cb(DeviceState *dev)
>  {
> @@ -1429,6 +1405,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>      Error *local_err = NULL;
>      PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
>      uint32_t slotnr = PCI_SLOT(pdev->devfn);
> +    void *fdt = NULL;
> +    int fdt_start_offset, fdt_size;
> 
>      /* if DR is disabled we don't need to do anything in the case of
>       * hotplug or coldplug callbacks
> @@ -1438,10 +1416,10 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>           * we need to let them know it's not enabled
>           */
>          if (plugged_dev->hotplugged) {
> -            error_setg(errp, QERR_BUS_NO_HOTPLUG,
> +            error_setg(&local_err, QERR_BUS_NO_HOTPLUG,
>                         object_get_typename(OBJECT(phb)));
>          }
> -        return;
> +        goto out;
>      }
> 
>      g_assert(drc);
> @@ -1452,16 +1430,23 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>       */
>      if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
>          PCI_FUNC(pdev->devfn) != 0) {
> -        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> +        error_setg(&local_err, "PCI: slot %d function 0 already ocuppied by %s,"
>                     " additional functions can no longer be exposed to guest.",
>                     slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> -        return;
> +        goto out;
>      }
> 
> -    spapr_phb_add_pci_device(drc, phb, pdev, &local_err);

Since we never used local_err outside propagating it and immediately
bailing, and since we bail on errp in all prior callers, maybe we
should just drop local_err completely in favor errp.

> +    fdt = create_device_tree(&fdt_size);
> +    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> +    if (!fdt_start_offset) {
> +        error_setg(&local_err, "Failed to create pci child device tree node");
> +        goto out;
> +    }
> +
> +    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
> +                     !plugged_dev->hotplugged, &local_err);
>      if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> +        goto out;
>      }
> 
>      /* If this is function 0, signal hotplug for all the device functions.
> @@ -1485,6 +1470,12 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>              }
>          }
>      }
> +
> +out:
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        g_free(fdt);
> +    }
>  }
> 
>  static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [RFC 1/3] spapr: Fold spapr_phb_add_pci_device() into its only caller

[David Gibson]

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

On Tue, Jun 06, 2017 at 04:37:27PM -0500, Michael Roth wrote:
> Quoting David Gibson (2017-06-06 08:05:32)
> > This function is fairly short, and so is its only caller.  There's no
> > particular logical distinction between them, so fold them together.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c | 53 ++++++++++++++++++++++-------------------------------
> >  1 file changed, 22 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 46e736d..a216f61 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1344,30 +1344,6 @@ static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
> >      return offset;
> >  }
> > 
> > -static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> > -                                     sPAPRPHBState *phb,
> > -                                     PCIDevice *pdev,
> > -                                     Error **errp)
> > -{
> > -    DeviceState *dev = DEVICE(pdev);
> > -    void *fdt = NULL;
> > -    int fdt_start_offset = 0, fdt_size;
> > -
> > -    fdt = create_device_tree(&fdt_size);
> > -    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > -    if (!fdt_start_offset) {
> > -        error_setg(errp, "Failed to create pci child device tree node");
> > -        goto out;
> > -    }
> > -
> > -    spapr_drc_attach(drc, DEVICE(pdev),
> > -                     fdt, fdt_start_offset, !dev->hotplugged, errp);
> > -out:
> > -    if (*errp) {
> > -        g_free(fdt);
> > -    }
> > -}
> > -
> >  /* Callback to be called during DRC release. */
> >  void spapr_phb_remove_pci_device_cb(DeviceState *dev)
> >  {
> > @@ -1429,6 +1405,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> >      Error *local_err = NULL;
> >      PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> >      uint32_t slotnr = PCI_SLOT(pdev->devfn);
> > +    void *fdt = NULL;
> > +    int fdt_start_offset, fdt_size;
> > 
> >      /* if DR is disabled we don't need to do anything in the case of
> >       * hotplug or coldplug callbacks
> > @@ -1438,10 +1416,10 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> >           * we need to let them know it's not enabled
> >           */
> >          if (plugged_dev->hotplugged) {
> > -            error_setg(errp, QERR_BUS_NO_HOTPLUG,
> > +            error_setg(&local_err, QERR_BUS_NO_HOTPLUG,
> >                         object_get_typename(OBJECT(phb)));
> >          }
> > -        return;
> > +        goto out;
> >      }
> > 
> >      g_assert(drc);
> > @@ -1452,16 +1430,23 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> >       */
> >      if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
> >          PCI_FUNC(pdev->devfn) != 0) {
> > -        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> > +        error_setg(&local_err, "PCI: slot %d function 0 already ocuppied by %s,"
> >                     " additional functions can no longer be exposed to guest.",
> >                     slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> > -        return;
> > +        goto out;
> >      }
> > 
> > -    spapr_phb_add_pci_device(drc, phb, pdev, &local_err);
> 
> Since we never used local_err outside propagating it and immediately
> bailing, and since we bail on errp in all prior callers, maybe we
> should just drop local_err completely in favor errp.

That doesn't quite work.  The reason for the local_err pattern is so
that we can tell locally if the error was triggered (errp might be
NULL, so checking *errp isn't safe).

> 
> > +    fdt = create_device_tree(&fdt_size);
> > +    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > +    if (!fdt_start_offset) {
> > +        error_setg(&local_err, "Failed to create pci child device tree node");
> > +        goto out;
> > +    }
> > +
> > +    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
> > +                     !plugged_dev->hotplugged, &local_err);
> >      if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > +        goto out;
> >      }
> > 
> >      /* If this is function 0, signal hotplug for all the device functions.
> > @@ -1485,6 +1470,12 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> >              }
> >          }
> >      }
> > +
> > +out:
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        g_free(fdt);
> > +    }
> >  }
> > 
> >  static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> 

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Michael Roth]

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.

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.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>  hw/ppc/spapr_drc.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index c73fae0..22f9224 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -291,16 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      }
>      g_assert(fdt || coldplug);
> 
> -    /* NOTE: setting initial isolation state to UNISOLATED means we can't
> -     * detach unless guest has a userspace/kernel that moves this state
> -     * back to ISOLATED in response to an unplug event, or this is done
> -     * manually by the admin prior. if we force things while the guest
> -     * may be accessing the device, we can easily crash the guest, so we
> -     * we defer completion of removal in such cases to the reset() hook.
> -     */
> -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> -        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> -    }
>      drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> 
>      drc->dev = d;
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [RFC 1/3] spapr: Fold spapr_phb_add_pci_device() into its only caller

[Michael Roth]

Quoting David Gibson (2017-06-06 20:33:07)
> On Tue, Jun 06, 2017 at 04:37:27PM -0500, Michael Roth wrote:
> > Quoting David Gibson (2017-06-06 08:05:32)
> > > This function is fairly short, and so is its only caller.  There's no
> > > particular logical distinction between them, so fold them together.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr_pci.c | 53 ++++++++++++++++++++++-------------------------------
> > >  1 file changed, 22 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index 46e736d..a216f61 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -1344,30 +1344,6 @@ static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
> > >      return offset;
> > >  }
> > > 
> > > -static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
> > > -                                     sPAPRPHBState *phb,
> > > -                                     PCIDevice *pdev,
> > > -                                     Error **errp)
> > > -{
> > > -    DeviceState *dev = DEVICE(pdev);
> > > -    void *fdt = NULL;
> > > -    int fdt_start_offset = 0, fdt_size;
> > > -
> > > -    fdt = create_device_tree(&fdt_size);
> > > -    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > > -    if (!fdt_start_offset) {
> > > -        error_setg(errp, "Failed to create pci child device tree node");
> > > -        goto out;
> > > -    }
> > > -
> > > -    spapr_drc_attach(drc, DEVICE(pdev),
> > > -                     fdt, fdt_start_offset, !dev->hotplugged, errp);
> > > -out:
> > > -    if (*errp) {
> > > -        g_free(fdt);
> > > -    }
> > > -}
> > > -
> > >  /* Callback to be called during DRC release. */
> > >  void spapr_phb_remove_pci_device_cb(DeviceState *dev)
> > >  {
> > > @@ -1429,6 +1405,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > >      Error *local_err = NULL;
> > >      PCIBus *bus = PCI_BUS(qdev_get_parent_bus(DEVICE(pdev)));
> > >      uint32_t slotnr = PCI_SLOT(pdev->devfn);
> > > +    void *fdt = NULL;
> > > +    int fdt_start_offset, fdt_size;
> > > 
> > >      /* if DR is disabled we don't need to do anything in the case of
> > >       * hotplug or coldplug callbacks
> > > @@ -1438,10 +1416,10 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > >           * we need to let them know it's not enabled
> > >           */
> > >          if (plugged_dev->hotplugged) {
> > > -            error_setg(errp, QERR_BUS_NO_HOTPLUG,
> > > +            error_setg(&local_err, QERR_BUS_NO_HOTPLUG,
> > >                         object_get_typename(OBJECT(phb)));
> > >          }
> > > -        return;
> > > +        goto out;
> > >      }
> > > 
> > >      g_assert(drc);
> > > @@ -1452,16 +1430,23 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > >       */
> > >      if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] &&
> > >          PCI_FUNC(pdev->devfn) != 0) {
> > > -        error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s,"
> > > +        error_setg(&local_err, "PCI: slot %d function 0 already ocuppied by %s,"
> > >                     " additional functions can no longer be exposed to guest.",
> > >                     slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name);
> > > -        return;
> > > +        goto out;
> > >      }
> > > 
> > > -    spapr_phb_add_pci_device(drc, phb, pdev, &local_err);
> > 
> > Since we never used local_err outside propagating it and immediately
> > bailing, and since we bail on errp in all prior callers, maybe we
> > should just drop local_err completely in favor errp.
> 
> That doesn't quite work.  The reason for the local_err pattern is so
> that we can tell locally if the error was triggered (errp might be
> NULL, so checking *errp isn't safe).

Ah, of course, not sure how I keep forgetting that detail.

> 
> > 
> > > +    fdt = create_device_tree(&fdt_size);
> > > +    fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > > +    if (!fdt_start_offset) {
> > > +        error_setg(&local_err, "Failed to create pci child device tree node");
> > > +        goto out;
> > > +    }
> > > +
> > > +    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
> > > +                     !plugged_dev->hotplugged, &local_err);
> > >      if (local_err) {
> > > -        error_propagate(errp, local_err);
> > > -        return;
> > > +        goto out;
> > >      }
> > > 
> > >      /* If this is function 0, signal hotplug for all the device functions.
> > > @@ -1485,6 +1470,12 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
> > >              }
> > >          }
> > >      }
> > > +
> > > +out:
> > > +    if (local_err) {
> > > +        error_propagate(errp, local_err);
> > > +        g_free(fdt);
> > > +    }
> > >  }
> > > 
> > >  static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> > 
> 
> -- 
> 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] [RFC 2/3] spapr: Start hotplugged PCI devices in ISOLATED state

[Michael Roth]

Quoting Michael Roth (2017-06-07 17:49:06)
> 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.
> 
> 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

Actually, since we set it explicitly in attach(), dr-indicator does end
up being correct, but correct for the wrong reasons still.

> 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.
> 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> > ---
> >  hw/ppc/spapr_drc.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index c73fae0..22f9224 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -291,16 +291,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> >      }
> >      g_assert(fdt || coldplug);
> > 
> > -    /* NOTE: setting initial isolation state to UNISOLATED means we can't
> > -     * detach unless guest has a userspace/kernel that moves this state
> > -     * back to ISOLATED in response to an unplug event, or this is done
> > -     * manually by the admin prior. if we force things while the guest
> > -     * may be accessing the device, we can easily crash the guest, so we
> > -     * we defer completion of removal in such cases to the reset() hook.
> > -     */
> > -    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
> > -        drc->isolation_state = SPAPR_DR_ISOLATION_STATE_UNISOLATED;
> > -    }
> >      drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> > 
> >      drc->dev = d;
> > -- 
> > 2.9.4
> > 
> 
> 

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

[David Gibson]

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

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

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