[Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part V)

[Qemu-devel] [PATCH 0/5] spapr: DRC cleanups (part V)

[David Gibson]

This fifth set of cleanups to the DRC code mostly deals with removing
unnecessary differences between different cases on the various hot
plug and unplug paths.

David Gibson (5):
  spapr: Leave DR-indicator management to the guest
  spapr: Uniform DRC reset paths
  spapr: Add DRC release method
  spapr: Remove unnecessary differences between hotplug and coldplug
    paths
  spapr: Use unplug_request for PCI hot unplug

 hw/ppc/spapr.c             | 44 +++++++-------------------------------------
 hw/ppc/spapr_drc.c         | 37 ++++++++++++-------------------------
 hw/ppc/spapr_pci.c         | 10 +++++-----
 include/hw/ppc/spapr_drc.h |  3 ++-
 4 files changed, 26 insertions(+), 68 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/5] spapr: Leave DR-indicator management to the guest

[David Gibson]

The DR-indicator is essentially a "virtual LED" attached to a hotpluggable
device, which the guest can set to various states for the attention of
the operator or management layers.

It's mostly guest managed, except that we once-off set it to
ACTIVE/INACTIVE in the attach/detach path.  While that makes certain sense,
there's no indication in PAPR that the hypervisor should do this, and the
drmgr code on the guest side doesn't appear to need it (it will already set
the indicator to ACTIVE on hotplug, and INACTIVE on remove).

So, leave the DR-indicator entirely to the guest; the only thing we need
to do is ensure it's in a sane state on reset.

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

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd9e07d..0bc9046 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -353,8 +353,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     }
     g_assert(fdt || coldplug);
 
-    drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
-
     drc->dev = d;
     drc->fdt = fdt;
     drc->fdt_start_offset = fdt_start_offset;
@@ -372,8 +370,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
 
 static void spapr_drc_release(sPAPRDRConnector *drc)
 {
-    drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
-
     /* Calling release callbacks based on spapr_drc_type(drc). */
     switch (spapr_drc_type(drc)) {
     case SPAPR_DR_CONNECTOR_TYPE_CPU:
@@ -452,12 +448,14 @@ static void reset(DeviceState *d)
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
             drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
         }
+        drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
     } else {
         /* Otherwise device is absent, but might be hotplugged */
         drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
         if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
             drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
         }
+        drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
     }
 }
 
-- 
2.9.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/5] spapr: Leave DR-indicator management to the guest

[Laurent Vivier]

On 20/06/2017 03:53, David Gibson wrote:
> The DR-indicator is essentially a "virtual LED" attached to a hotpluggable
> device, which the guest can set to various states for the attention of
> the operator or management layers.
> 
> It's mostly guest managed, except that we once-off set it to
> ACTIVE/INACTIVE in the attach/detach path.  While that makes certain sense,
> there's no indication in PAPR that the hypervisor should do this, and the
> drmgr code on the guest side doesn't appear to need it (it will already set
> the indicator to ACTIVE on hotplug, and INACTIVE on remove).
> 
> So, leave the DR-indicator entirely to the guest; the only thing we need
> to do is ensure it's in a sane state on reset.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fd9e07d..0bc9046 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -353,8 +353,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      }
>      g_assert(fdt || coldplug);
>  
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> -
>      drc->dev = d;
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
> @@ -372,8 +370,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void spapr_drc_release(sPAPRDRConnector *drc)
>  {
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> -
>      /* Calling release callbacks based on spapr_drc_type(drc). */
>      switch (spapr_drc_type(drc)) {
>      case SPAPR_DR_CONNECTOR_TYPE_CPU:
> @@ -452,12 +448,14 @@ static void reset(DeviceState *d)
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>      } else {
>          /* Otherwise device is absent, but might be hotplugged */
>          drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
>      }
>  }
>  
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH 1/5] spapr: Leave DR-indicator management to the guest

[Greg Kurz]

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

On Tue, 20 Jun 2017 09:53:28 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> The DR-indicator is essentially a "virtual LED" attached to a hotpluggable
> device, which the guest can set to various states for the attention of
> the operator or management layers.
> 
> It's mostly guest managed, except that we once-off set it to
> ACTIVE/INACTIVE in the attach/detach path.  While that makes certain sense,
> there's no indication in PAPR that the hypervisor should do this, and the
> drmgr code on the guest side doesn't appear to need it (it will already set
> the indicator to ACTIVE on hotplug, and INACTIVE on remove).
> 
> So, leave the DR-indicator entirely to the guest; the only thing we need
> to do is ensure it's in a sane state on reset.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_drc.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index fd9e07d..0bc9046 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -353,8 +353,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      }
>      g_assert(fdt || coldplug);
>  
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
> -
>      drc->dev = d;
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
> @@ -372,8 +370,6 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void spapr_drc_release(sPAPRDRConnector *drc)
>  {
> -    drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
> -
>      /* Calling release callbacks based on spapr_drc_type(drc). */
>      switch (spapr_drc_type(drc)) {
>      case SPAPR_DR_CONNECTOR_TYPE_CPU:
> @@ -452,12 +448,14 @@ static void reset(DeviceState *d)
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_USABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_ACTIVE;
>      } else {
>          /* Otherwise device is absent, but might be hotplugged */
>          drc->isolation_state = SPAPR_DR_ISOLATION_STATE_ISOLATED;
>          if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>              drc->allocation_state = SPAPR_DR_ALLOCATION_STATE_UNUSABLE;
>          }
> +        drc->dr_indicator = SPAPR_DR_INDICATOR_INACTIVE;
>      }
>  }
>  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[Qemu-devel] [PATCH 2/5] spapr: Uniform DRC reset paths

[David Gibson]

DRC objects have a regular device reset method.  However, it only gets
called in the usual way for PCI DRCs.  Because of where CPU and LMB DRCs
are in the QOM tree, their device reset method isn't automatically called.
So, the machine manually registers reset handlers to call device_reset().

This patch removes the device reset method, and instead always explicitly
registers the reset handler from realize().  This means the callers don't
have to worry about the two cases, and we always get proper resets.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c     | 31 ++++---------------------------
 hw/ppc/spapr_drc.c |  6 +++---
 2 files changed, 7 insertions(+), 30 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8a0c247..f12bc4d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1967,24 +1967,6 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
     machine->boot_order = g_strdup(boot_device);
 }
 
-/*
- * Reset routine for LMB DR devices.
- *
- * Unlike PCI DR devices, LMB DR devices explicitly register this reset
- * routine. Reset for PCI DR devices will be handled by PHB reset routine
- * when it walks all its children devices. LMB devices reset occurs
- * as part of spapr_ppc_reset().
- */
-static void spapr_drc_reset(void *opaque)
-{
-    sPAPRDRConnector *drc = opaque;
-    DeviceState *d = DEVICE(drc);
-
-    if (d) {
-        device_reset(d);
-    }
-}
-
 static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
 {
     MachineState *machine = MACHINE(spapr);
@@ -1993,13 +1975,11 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
     int i;
 
     for (i = 0; i < nr_lmbs; i++) {
-        sPAPRDRConnector *drc;
         uint64_t addr;
 
         addr = i * lmb_size + spapr->hotplug_memory.base;
-        drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
-                                     addr/lmb_size);
-        qemu_register_reset(spapr_drc_reset, drc);
+        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
+                               addr / lmb_size);
     }
 }
 
@@ -2093,11 +2073,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
         int core_id = i * smp_threads;
 
         if (mc->has_hotpluggable_cpus) {
-            sPAPRDRConnector *drc =
-                spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
-                                       (core_id / smp_threads) * smt);
-
-            qemu_register_reset(spapr_drc_reset, drc);
+            spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
+                                   (core_id / smp_threads) * smt);
         }
 
         if (i < boot_cores_nr) {
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 0bc9046..e5dff16 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -426,9 +426,9 @@ static bool release_pending(sPAPRDRConnector *drc)
     return drc->awaiting_release;
 }
 
-static void reset(DeviceState *d)
+static void drc_reset(void *opaque)
 {
-    sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
+    sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
 
     trace_spapr_drc_reset(spapr_drc_index(drc));
 
@@ -536,6 +536,7 @@ static void realize(DeviceState *d, Error **errp)
     g_free(child_name);
     vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
                      drc);
+    qemu_register_reset(drc_reset, drc);
     trace_spapr_drc_realize_complete(spapr_drc_index(drc));
 }
 
@@ -594,7 +595,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
     DeviceClass *dk = DEVICE_CLASS(k);
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
 
-    dk->reset = reset;
     dk->realize = realize;
     dk->unrealize = unrealize;
     drck->release_pending = release_pending;
-- 
2.9.4

Re: [Qemu-devel] [PATCH 2/5] spapr: Uniform DRC reset paths

[Greg Kurz]

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

On Tue, 20 Jun 2017 09:53:29 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> DRC objects have a regular device reset method.  However, it only gets
> called in the usual way for PCI DRCs.  Because of where CPU and LMB DRCs
> are in the QOM tree, their device reset method isn't automatically called.
> So, the machine manually registers reset handlers to call device_reset().
> 
> This patch removes the device reset method, and instead always explicitly
> registers the reset handler from realize().  This means the callers don't
> have to worry about the two cases, and we always get proper resets.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

This indeed simplifies the code. It also has the interesting effect that
nobody cares for the return value of spapr_dr_connector_new() anymore.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c     | 31 ++++---------------------------
>  hw/ppc/spapr_drc.c |  6 +++---
>  2 files changed, 7 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8a0c247..f12bc4d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1967,24 +1967,6 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> -/*
> - * Reset routine for LMB DR devices.
> - *
> - * Unlike PCI DR devices, LMB DR devices explicitly register this reset
> - * routine. Reset for PCI DR devices will be handled by PHB reset routine
> - * when it walks all its children devices. LMB devices reset occurs
> - * as part of spapr_ppc_reset().
> - */
> -static void spapr_drc_reset(void *opaque)
> -{
> -    sPAPRDRConnector *drc = opaque;
> -    DeviceState *d = DEVICE(drc);
> -
> -    if (d) {
> -        device_reset(d);
> -    }
> -}
> -
>  static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>  {
>      MachineState *machine = MACHINE(spapr);
> @@ -1993,13 +1975,11 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>      int i;
>  
>      for (i = 0; i < nr_lmbs; i++) {
> -        sPAPRDRConnector *drc;
>          uint64_t addr;
>  
>          addr = i * lmb_size + spapr->hotplug_memory.base;
> -        drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> -                                     addr/lmb_size);
> -        qemu_register_reset(spapr_drc_reset, drc);
> +        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> +                               addr / lmb_size);
>      }
>  }
>  
> @@ -2093,11 +2073,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
>          int core_id = i * smp_threads;
>  
>          if (mc->has_hotpluggable_cpus) {
> -            sPAPRDRConnector *drc =
> -                spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> -                                       (core_id / smp_threads) * smt);
> -
> -            qemu_register_reset(spapr_drc_reset, drc);
> +            spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> +                                   (core_id / smp_threads) * smt);
>          }
>  
>          if (i < boot_cores_nr) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 0bc9046..e5dff16 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -426,9 +426,9 @@ static bool release_pending(sPAPRDRConnector *drc)
>      return drc->awaiting_release;
>  }
>  
> -static void reset(DeviceState *d)
> +static void drc_reset(void *opaque)
>  {
> -    sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> +    sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
>  
>      trace_spapr_drc_reset(spapr_drc_index(drc));
>  
> @@ -536,6 +536,7 @@ static void realize(DeviceState *d, Error **errp)
>      g_free(child_name);
>      vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>                       drc);
> +    qemu_register_reset(drc_reset, drc);
>      trace_spapr_drc_realize_complete(spapr_drc_index(drc));
>  }
>  
> @@ -594,7 +595,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      DeviceClass *dk = DEVICE_CLASS(k);
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>  
> -    dk->reset = reset;
>      dk->realize = realize;
>      dk->unrealize = unrealize;
>      drck->release_pending = release_pending;

 	 	

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 2/5] spapr: Uniform DRC reset paths

[David Gibson]

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

On Tue, Jun 20, 2017 at 06:32:49PM +0200, Greg Kurz wrote:
> On Tue, 20 Jun 2017 09:53:29 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > DRC objects have a regular device reset method.  However, it only gets
> > called in the usual way for PCI DRCs.  Because of where CPU and LMB DRCs
> > are in the QOM tree, their device reset method isn't automatically called.
> > So, the machine manually registers reset handlers to call device_reset().
> > 
> > This patch removes the device reset method, and instead always explicitly
> > registers the reset handler from realize().  This means the callers don't
> > have to worry about the two cases, and we always get proper resets.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> This indeed simplifies the code. It also has the interesting effect that
> nobody cares for the return value of spapr_dr_connector_new()
> anymore.

True.  I think I'll leave it there for now, though, since *_new()
functions generally return the object they created.

-- 
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] [Qemu-ppc] [PATCH 2/5] spapr: Uniform DRC reset paths

[Laurent Vivier]

On 20/06/2017 03:53, David Gibson wrote:
> DRC objects have a regular device reset method.  However, it only gets
> called in the usual way for PCI DRCs.  Because of where CPU and LMB DRCs
> are in the QOM tree, their device reset method isn't automatically called.
> So, the machine manually registers reset handlers to call device_reset().
> 
> This patch removes the device reset method, and instead always explicitly
> registers the reset handler from realize().  This means the callers don't
> have to worry about the two cases, and we always get proper resets.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr.c     | 31 ++++---------------------------
>  hw/ppc/spapr_drc.c |  6 +++---
>  2 files changed, 7 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8a0c247..f12bc4d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1967,24 +1967,6 @@ static void spapr_boot_set(void *opaque, const char *boot_device,
>      machine->boot_order = g_strdup(boot_device);
>  }
>  
> -/*
> - * Reset routine for LMB DR devices.
> - *
> - * Unlike PCI DR devices, LMB DR devices explicitly register this reset
> - * routine. Reset for PCI DR devices will be handled by PHB reset routine
> - * when it walks all its children devices. LMB devices reset occurs
> - * as part of spapr_ppc_reset().
> - */
> -static void spapr_drc_reset(void *opaque)
> -{
> -    sPAPRDRConnector *drc = opaque;
> -    DeviceState *d = DEVICE(drc);
> -
> -    if (d) {
> -        device_reset(d);
> -    }
> -}
> -
>  static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>  {
>      MachineState *machine = MACHINE(spapr);
> @@ -1993,13 +1975,11 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>      int i;
>  
>      for (i = 0; i < nr_lmbs; i++) {
> -        sPAPRDRConnector *drc;
>          uint64_t addr;
>  
>          addr = i * lmb_size + spapr->hotplug_memory.base;
> -        drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> -                                     addr/lmb_size);
> -        qemu_register_reset(spapr_drc_reset, drc);
> +        spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
> +                               addr / lmb_size);
>      }
>  }
>  
> @@ -2093,11 +2073,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
>          int core_id = i * smp_threads;
>  
>          if (mc->has_hotpluggable_cpus) {
> -            sPAPRDRConnector *drc =
> -                spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> -                                       (core_id / smp_threads) * smt);
> -
> -            qemu_register_reset(spapr_drc_reset, drc);
> +            spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> +                                   (core_id / smp_threads) * smt);
>          }
>  
>          if (i < boot_cores_nr) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 0bc9046..e5dff16 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -426,9 +426,9 @@ static bool release_pending(sPAPRDRConnector *drc)
>      return drc->awaiting_release;
>  }
>  
> -static void reset(DeviceState *d)
> +static void drc_reset(void *opaque)
>  {
> -    sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> +    sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(opaque);
>  
>      trace_spapr_drc_reset(spapr_drc_index(drc));
>  
> @@ -536,6 +536,7 @@ static void realize(DeviceState *d, Error **errp)
>      g_free(child_name);
>      vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>                       drc);
> +    qemu_register_reset(drc_reset, drc);
>      trace_spapr_drc_realize_complete(spapr_drc_index(drc));
>  }
>  
> @@ -594,7 +595,6 @@ static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
>      DeviceClass *dk = DEVICE_CLASS(k);
>      sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
>  
> -    dk->reset = reset;
>      dk->realize = realize;
>      dk->unrealize = unrealize;
>      drck->release_pending = release_pending;
> 

[Qemu-devel] [PATCH 3/5] spapr: Add DRC release method

[David Gibson]

At the moment, spapr_drc_release() has an ugly switch on the DRC type to
call the right, device-specific release function.  This cleans it up by
doing that via a proper QOM method.

It's still arguably an abstraction violation for the DRC code to call into
the specific device code, but one mess at a time.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_drc.c         | 22 ++++++----------------
 include/hw/ppc/spapr_drc.h |  1 +
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index e5dff16..32e39f2 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -370,22 +370,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
 
 static void spapr_drc_release(sPAPRDRConnector *drc)
 {
-    /* Calling release callbacks based on spapr_drc_type(drc). */
-    switch (spapr_drc_type(drc)) {
-    case SPAPR_DR_CONNECTOR_TYPE_CPU:
-        spapr_core_release(drc->dev);
-        break;
-    case SPAPR_DR_CONNECTOR_TYPE_PCI:
-        spapr_phb_remove_pci_device_cb(drc->dev);
-        break;
-    case SPAPR_DR_CONNECTOR_TYPE_LMB:
-        spapr_lmb_release(drc->dev);
-        break;
-    case SPAPR_DR_CONNECTOR_TYPE_PHB:
-    case SPAPR_DR_CONNECTOR_TYPE_VIO:
-    default:
-        g_assert(false);
-    }
+    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+
+    drck->release(drc->dev);
 
     drc->awaiting_release = false;
     g_free(drc->fdt);
@@ -629,6 +616,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
     drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
     drck->typename = "CPU";
     drck->drc_name_prefix = "CPU ";
+    drck->release = spapr_core_release;
 }
 
 static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
@@ -638,6 +626,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
     drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
     drck->typename = "28";
     drck->drc_name_prefix = "C";
+    drck->release = spapr_phb_remove_pci_device_cb;
 }
 
 static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
@@ -647,6 +636,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
     drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
     drck->typename = "MEM";
     drck->drc_name_prefix = "LMB ";
+    drck->release = spapr_lmb_release;
 }
 
 static const TypeInfo spapr_dr_connector_info = {
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index d9cacb3..6fd84d1 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -217,6 +217,7 @@ typedef struct sPAPRDRConnectorClass {
     sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
     uint32_t (*isolate)(sPAPRDRConnector *drc);
     uint32_t (*unisolate)(sPAPRDRConnector *drc);
+    void (*release)(DeviceState *dev);
 
     /* QEMU interfaces for managing hotplug operations */
     bool (*release_pending)(sPAPRDRConnector *drc);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 3/5] spapr: Add DRC release method

[Greg Kurz]

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

On Tue, 20 Jun 2017 09:53:30 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> call the right, device-specific release function.  This cleans it up by
> doing that via a proper QOM method.
> 
> It's still arguably an abstraction violation for the DRC code to call into
> the specific device code, but one mess at a time.
> 

Maybe a second step would be to move DRC subclasses to spapr.c (CPU, LMB) and
spapr_pci.c (PCI) ? This would allow each device-specific release function to
have local scope at least.

Reviewed-by: Greg Kurz <groug@kaod.org>

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_drc.c         | 22 ++++++----------------
>  include/hw/ppc/spapr_drc.h |  1 +
>  2 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index e5dff16..32e39f2 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -370,22 +370,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void spapr_drc_release(sPAPRDRConnector *drc)
>  {
> -    /* Calling release callbacks based on spapr_drc_type(drc). */
> -    switch (spapr_drc_type(drc)) {
> -    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> -        spapr_core_release(drc->dev);
> -        break;
> -    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> -        spapr_phb_remove_pci_device_cb(drc->dev);
> -        break;
> -    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> -        spapr_lmb_release(drc->dev);
> -        break;
> -    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> -    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> -    default:
> -        g_assert(false);
> -    }
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> +    drck->release(drc->dev);
>  
>      drc->awaiting_release = false;
>      g_free(drc->fdt);
> @@ -629,6 +616,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
>      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
>      drck->typename = "CPU";
>      drck->drc_name_prefix = "CPU ";
> +    drck->release = spapr_core_release;
>  }
>  
>  static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> @@ -638,6 +626,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
>      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
>      drck->typename = "28";
>      drck->drc_name_prefix = "C";
> +    drck->release = spapr_phb_remove_pci_device_cb;
>  }
>  
>  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> @@ -647,6 +636,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
>      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
>      drck->typename = "MEM";
>      drck->drc_name_prefix = "LMB ";
> +    drck->release = spapr_lmb_release;
>  }
>  
>  static const TypeInfo spapr_dr_connector_info = {
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index d9cacb3..6fd84d1 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -217,6 +217,7 @@ typedef struct sPAPRDRConnectorClass {
>      sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
>      uint32_t (*isolate)(sPAPRDRConnector *drc);
>      uint32_t (*unisolate)(sPAPRDRConnector *drc);
> +    void (*release)(DeviceState *dev);
>  
>      /* QEMU interfaces for managing hotplug operations */
>      bool (*release_pending)(sPAPRDRConnector *drc);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 3/5] spapr: Add DRC release method

[Michael Roth]

Quoting Greg Kurz (2017-06-20 11:51:45)
> On Tue, 20 Jun 2017 09:53:30 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> > call the right, device-specific release function.  This cleans it up by
> > doing that via a proper QOM method.
> > 
> > It's still arguably an abstraction violation for the DRC code to call into
> > the specific device code, but one mess at a time.
> > 
> 
> Maybe a second step would be to move DRC subclasses to spapr.c (CPU, LMB) and
> spapr_pci.c (PCI) ? This would allow each device-specific release function to
> have local scope at least.

I kind of prefer the proposed approach of registering a callback
function via drc_new(). This would make it easier to implement
unit tests without needing to rely on stub functions, and keeps the
type-specific handling constrained to matters relating to the DRC
itself and not the resources associated with them.

> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_drc.c         | 22 ++++++----------------
> >  include/hw/ppc/spapr_drc.h |  1 +
> >  2 files changed, 7 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index e5dff16..32e39f2 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -370,22 +370,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> >  
> >  static void spapr_drc_release(sPAPRDRConnector *drc)
> >  {
> > -    /* Calling release callbacks based on spapr_drc_type(drc). */
> > -    switch (spapr_drc_type(drc)) {
> > -    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > -        spapr_core_release(drc->dev);
> > -        break;
> > -    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > -        spapr_phb_remove_pci_device_cb(drc->dev);
> > -        break;
> > -    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> > -        spapr_lmb_release(drc->dev);
> > -        break;
> > -    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> > -    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> > -    default:
> > -        g_assert(false);
> > -    }
> > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > +    drck->release(drc->dev);
> >  
> >      drc->awaiting_release = false;
> >      g_free(drc->fdt);
> > @@ -629,6 +616,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> >      drck->typename = "CPU";
> >      drck->drc_name_prefix = "CPU ";
> > +    drck->release = spapr_core_release;
> >  }
> >  
> >  static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > @@ -638,6 +626,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> >      drck->typename = "28";
> >      drck->drc_name_prefix = "C";
> > +    drck->release = spapr_phb_remove_pci_device_cb;
> >  }
> >  
> >  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > @@ -647,6 +636,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> >      drck->typename = "MEM";
> >      drck->drc_name_prefix = "LMB ";
> > +    drck->release = spapr_lmb_release;
> >  }
> >  
> >  static const TypeInfo spapr_dr_connector_info = {
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index d9cacb3..6fd84d1 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -217,6 +217,7 @@ typedef struct sPAPRDRConnectorClass {
> >      sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
> >      uint32_t (*isolate)(sPAPRDRConnector *drc);
> >      uint32_t (*unisolate)(sPAPRDRConnector *drc);
> > +    void (*release)(DeviceState *dev);
> >  
> >      /* QEMU interfaces for managing hotplug operations */
> >      bool (*release_pending)(sPAPRDRConnector *drc);
> 

Re: [Qemu-devel] [PATCH 3/5] spapr: Add DRC release method

[David Gibson]

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

On Tue, Jun 20, 2017 at 02:24:08PM -0500, Michael Roth wrote:
> Quoting Greg Kurz (2017-06-20 11:51:45)
> > On Tue, 20 Jun 2017 09:53:30 +0800
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> > > call the right, device-specific release function.  This cleans it up by
> > > doing that via a proper QOM method.
> > > 
> > > It's still arguably an abstraction violation for the DRC code to call into
> > > the specific device code, but one mess at a time.
> > > 
> > 
> > Maybe a second step would be to move DRC subclasses to spapr.c (CPU, LMB) and
> > spapr_pci.c (PCI) ? This would allow each device-specific release function to
> > have local scope at least.

I was already thinking of doing that in a future patch :)

> I kind of prefer the proposed approach of registering a callback
> function via drc_new(). This would make it easier to implement
> unit tests without needing to rely on stub functions, and keeps the
> type-specific handling constrained to matters relating to the DRC
> itself and not the resources associated with them.

True.  We went from the callback to this ugly switch when we made the
DRCs migratable (well, sort of, except for the many remaining
migration problems these cleanups are addressing amongst other
things).  But moving the callback registration from attach time to new
time would do just as well.  Well I'll think about it and do one or
the other in future.

-- 
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] [PATCH 3/5] spapr: Add DRC release method

[Greg Kurz]

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

On Tue, 20 Jun 2017 14:24:08 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Greg Kurz (2017-06-20 11:51:45)
> > On Tue, 20 Jun 2017 09:53:30 +0800
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> > > call the right, device-specific release function.  This cleans it up by
> > > doing that via a proper QOM method.
> > > 
> > > It's still arguably an abstraction violation for the DRC code to call into
> > > the specific device code, but one mess at a time.
> > >   
> > 
> > Maybe a second step would be to move DRC subclasses to spapr.c (CPU, LMB) and
> > spapr_pci.c (PCI) ? This would allow each device-specific release function to
> > have local scope at least.  
> 
> I kind of prefer the proposed approach of registering a callback
> function via drc_new(). This would make it easier to implement
> unit tests without needing to rely on stub functions, and keeps the
> type-specific handling constrained to matters relating to the DRC
> itself and not the resources associated with them.
> 

Yeah, this could work too and it would address David's concern about
abstraction violation in a clean way.

> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >   
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr_drc.c         | 22 ++++++----------------
> > >  include/hw/ppc/spapr_drc.h |  1 +
> > >  2 files changed, 7 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index e5dff16..32e39f2 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -370,22 +370,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > >  
> > >  static void spapr_drc_release(sPAPRDRConnector *drc)
> > >  {
> > > -    /* Calling release callbacks based on spapr_drc_type(drc). */
> > > -    switch (spapr_drc_type(drc)) {
> > > -    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> > > -        spapr_core_release(drc->dev);
> > > -        break;
> > > -    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > > -        spapr_phb_remove_pci_device_cb(drc->dev);
> > > -        break;
> > > -    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> > > -        spapr_lmb_release(drc->dev);
> > > -        break;
> > > -    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> > > -    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> > > -    default:
> > > -        g_assert(false);
> > > -    }
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +
> > > +    drck->release(drc->dev);
> > >  
> > >      drc->awaiting_release = false;
> > >      g_free(drc->fdt);
> > > @@ -629,6 +616,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
> > >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> > >      drck->typename = "CPU";
> > >      drck->drc_name_prefix = "CPU ";
> > > +    drck->release = spapr_core_release;
> > >  }
> > >  
> > >  static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > > @@ -638,6 +626,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> > >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> > >      drck->typename = "28";
> > >      drck->drc_name_prefix = "C";
> > > +    drck->release = spapr_phb_remove_pci_device_cb;
> > >  }
> > >  
> > >  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > > @@ -647,6 +636,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> > >      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> > >      drck->typename = "MEM";
> > >      drck->drc_name_prefix = "LMB ";
> > > +    drck->release = spapr_lmb_release;
> > >  }
> > >  
> > >  static const TypeInfo spapr_dr_connector_info = {
> > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > > index d9cacb3..6fd84d1 100644
> > > --- a/include/hw/ppc/spapr_drc.h
> > > +++ b/include/hw/ppc/spapr_drc.h
> > > @@ -217,6 +217,7 @@ typedef struct sPAPRDRConnectorClass {
> > >      sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
> > >      uint32_t (*isolate)(sPAPRDRConnector *drc);
> > >      uint32_t (*unisolate)(sPAPRDRConnector *drc);
> > > +    void (*release)(DeviceState *dev);
> > >  
> > >      /* QEMU interfaces for managing hotplug operations */
> > >      bool (*release_pending)(sPAPRDRConnector *drc);  
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] spapr: Add DRC release method

[Laurent Vivier]

On 20/06/2017 03:53, David Gibson wrote:
> At the moment, spapr_drc_release() has an ugly switch on the DRC type to
> call the right, device-specific release function.  This cleans it up by
> doing that via a proper QOM method.
> 
> It's still arguably an abstraction violation for the DRC code to call into
> the specific device code, but one mess at a time.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr_drc.c         | 22 ++++++----------------
>  include/hw/ppc/spapr_drc.h |  1 +
>  2 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index e5dff16..32e39f2 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -370,22 +370,9 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>  
>  static void spapr_drc_release(sPAPRDRConnector *drc)
>  {
> -    /* Calling release callbacks based on spapr_drc_type(drc). */
> -    switch (spapr_drc_type(drc)) {
> -    case SPAPR_DR_CONNECTOR_TYPE_CPU:
> -        spapr_core_release(drc->dev);
> -        break;
> -    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> -        spapr_phb_remove_pci_device_cb(drc->dev);
> -        break;
> -    case SPAPR_DR_CONNECTOR_TYPE_LMB:
> -        spapr_lmb_release(drc->dev);
> -        break;
> -    case SPAPR_DR_CONNECTOR_TYPE_PHB:
> -    case SPAPR_DR_CONNECTOR_TYPE_VIO:
> -    default:
> -        g_assert(false);
> -    }
> +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> +    drck->release(drc->dev);
>  
>      drc->awaiting_release = false;
>      g_free(drc->fdt);
> @@ -629,6 +616,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void *data)
>      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
>      drck->typename = "CPU";
>      drck->drc_name_prefix = "CPU ";
> +    drck->release = spapr_core_release;
>  }
>  
>  static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> @@ -638,6 +626,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
>      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
>      drck->typename = "28";
>      drck->drc_name_prefix = "C";
> +    drck->release = spapr_phb_remove_pci_device_cb;
>  }
>  
>  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> @@ -647,6 +636,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
>      drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
>      drck->typename = "MEM";
>      drck->drc_name_prefix = "LMB ";
> +    drck->release = spapr_lmb_release;
>  }
>  
>  static const TypeInfo spapr_dr_connector_info = {
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index d9cacb3..6fd84d1 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -217,6 +217,7 @@ typedef struct sPAPRDRConnectorClass {
>      sPAPRDREntitySense (*dr_entity_sense)(sPAPRDRConnector *drc);
>      uint32_t (*isolate)(sPAPRDRConnector *drc);
>      uint32_t (*unisolate)(sPAPRDRConnector *drc);
> +    void (*release)(DeviceState *dev);
>  
>      /* QEMU interfaces for managing hotplug operations */
>      bool (*release_pending)(sPAPRDRConnector *drc);
> 

[Qemu-devel] [PATCH 4/5] spapr: Remove unnecessary differences between hotplug and coldplug paths

[David Gibson]

spapr_drc_attach() has a 'coldplug' parameter which sets the DRC into
configured state initially, instead of the usual ISOLATED/UNUSABLE state.
It turns out this is unnecessary: although coldplugged devices do need to
be in CONFIGURED state once the guest starts, that will already be
accomplished by the reset code which will move DRCs for already plugged
devices into a coldplug equivalent state.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c             | 13 +++----------
 hw/ppc/spapr_drc.c         |  5 ++---
 hw/ppc/spapr_pci.c         |  3 +--
 include/hw/ppc/spapr_drc.h |  2 +-
 4 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f12bc4d..35f2d4b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2611,7 +2611,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
         fdt_offset = spapr_populate_memory_node(fdt, node, addr,
                                                 SPAPR_MEMORY_BLOCK_SIZE);
 
-        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
+        spapr_drc_attach(drc, dev, fdt, fdt_offset, errp);
         addr += SPAPR_MEMORY_BLOCK_SIZE;
     }
     /* send hotplug notification to the
@@ -2955,17 +2955,10 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 
     g_assert(drc || !mc->has_hotpluggable_cpus);
 
-    /*
-     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
-     * coldplugged CPUs DT entries are setup in spapr_build_fdt().
-     */
-    if (dev->hotplugged) {
-        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
-    }
+    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
 
     if (drc) {
-        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged,
-                         &local_err);
+        spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
         if (local_err) {
             g_free(fdt);
             error_propagate(errp, local_err);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 32e39f2..e70879a 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -340,7 +340,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
 }
 
 void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
-                      int fdt_start_offset, bool coldplug, Error **errp)
+                      int fdt_start_offset, Error **errp)
 {
     trace_spapr_drc_attach(spapr_drc_index(drc));
 
@@ -351,12 +351,11 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
     if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
         g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
     }
-    g_assert(fdt || coldplug);
+    g_assert(fdt);
 
     drc->dev = d;
     drc->fdt = fdt;
     drc->fdt_start_offset = fdt_start_offset;
-    drc->configured = coldplug;
 
     if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
         drc->awaiting_allocation = true;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 0b447f2..f2543ef 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1435,8 +1435,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
         goto out;
     }
 
-    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
-                     !plugged_dev->hotplugged, &local_err);
+    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset, &local_err);
     if (local_err) {
         goto out;
     }
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 6fd84d1..d15e9eb 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -234,7 +234,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
                           uint32_t drc_type_mask);
 
 void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
-                      int fdt_start_offset, bool coldplug, Error **errp);
+                      int fdt_start_offset, Error **errp);
 void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
 
 #endif /* HW_SPAPR_DRC_H */
-- 
2.9.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/5] spapr: Remove unnecessary differences between hotplug and coldplug paths

[Laurent Vivier]

On 20/06/2017 03:53, David Gibson wrote:
> spapr_drc_attach() has a 'coldplug' parameter which sets the DRC into
> configured state initially, instead of the usual ISOLATED/UNUSABLE state.
> It turns out this is unnecessary: although coldplugged devices do need to
> be in CONFIGURED state once the guest starts, that will already be
> accomplished by the reset code which will move DRCs for already plugged
> devices into a coldplug equivalent state.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr.c             | 13 +++----------
>  hw/ppc/spapr_drc.c         |  5 ++---
>  hw/ppc/spapr_pci.c         |  3 +--
>  include/hw/ppc/spapr_drc.h |  2 +-
>  4 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f12bc4d..35f2d4b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2611,7 +2611,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>          fdt_offset = spapr_populate_memory_node(fdt, node, addr,
>                                                  SPAPR_MEMORY_BLOCK_SIZE);
>  
> -        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> +        spapr_drc_attach(drc, dev, fdt, fdt_offset, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>      /* send hotplug notification to the
> @@ -2955,17 +2955,10 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      g_assert(drc || !mc->has_hotpluggable_cpus);
>  
> -    /*
> -     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> -     * coldplugged CPUs DT entries are setup in spapr_build_fdt().
> -     */
> -    if (dev->hotplugged) {
> -        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> -    }
> +    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
>  
>      if (drc) {
> -        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged,
> -                         &local_err);
> +        spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
>          if (local_err) {
>              g_free(fdt);
>              error_propagate(errp, local_err);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 32e39f2..e70879a 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -340,7 +340,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>  }
>  
>  void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> -                      int fdt_start_offset, bool coldplug, Error **errp)
> +                      int fdt_start_offset, Error **errp)
>  {
>      trace_spapr_drc_attach(spapr_drc_index(drc));
>  
> @@ -351,12 +351,11 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
>          g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
>      }
> -    g_assert(fdt || coldplug);
> +    g_assert(fdt);
>  
>      drc->dev = d;
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
> -    drc->configured = coldplug;
>  
>      if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>          drc->awaiting_allocation = true;
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 0b447f2..f2543ef 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1435,8 +1435,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>          goto out;
>      }
>  
> -    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
> -                     !plugged_dev->hotplugged, &local_err);
> +    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset, &local_err);
>      if (local_err) {
>          goto out;
>      }
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 6fd84d1..d15e9eb 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -234,7 +234,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>                            uint32_t drc_type_mask);
>  
>  void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> -                      int fdt_start_offset, bool coldplug, Error **errp);
> +                      int fdt_start_offset, Error **errp);
>  void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
>  
>  #endif /* HW_SPAPR_DRC_H */
> 

Re: [Qemu-devel] [PATCH 4/5] spapr: Remove unnecessary differences between hotplug and coldplug paths

[Greg Kurz]

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

On Tue, 20 Jun 2017 09:53:31 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> spapr_drc_attach() has a 'coldplug' parameter which sets the DRC into
> configured state initially, instead of the usual ISOLATED/UNUSABLE state.
> It turns out this is unnecessary: although coldplugged devices do need to
> be in CONFIGURED state once the guest starts, that will already be
> accomplished by the reset code which will move DRCs for already plugged
> devices into a coldplug equivalent state.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c             | 13 +++----------
>  hw/ppc/spapr_drc.c         |  5 ++---
>  hw/ppc/spapr_pci.c         |  3 +--
>  include/hw/ppc/spapr_drc.h |  2 +-
>  4 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f12bc4d..35f2d4b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2611,7 +2611,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>          fdt_offset = spapr_populate_memory_node(fdt, node, addr,
>                                                  SPAPR_MEMORY_BLOCK_SIZE);
>  
> -        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> +        spapr_drc_attach(drc, dev, fdt, fdt_offset, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
>      }
>      /* send hotplug notification to the
> @@ -2955,17 +2955,10 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      g_assert(drc || !mc->has_hotpluggable_cpus);
>  
> -    /*
> -     * Setup CPU DT entries only for hotplugged CPUs. For boot time or
> -     * coldplugged CPUs DT entries are setup in spapr_build_fdt().
> -     */
> -    if (dev->hotplugged) {
> -        fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> -    }
> +    fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
>  
>      if (drc) {
> -        spapr_drc_attach(drc, dev, fdt, fdt_offset, !dev->hotplugged,
> -                         &local_err);
> +        spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
>          if (local_err) {
>              g_free(fdt);
>              error_propagate(errp, local_err);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 32e39f2..e70879a 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -340,7 +340,7 @@ static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>  }
>  
>  void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> -                      int fdt_start_offset, bool coldplug, Error **errp)
> +                      int fdt_start_offset, Error **errp)
>  {
>      trace_spapr_drc_attach(spapr_drc_index(drc));
>  
> @@ -351,12 +351,11 @@ void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
>      if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_PCI) {
>          g_assert(drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE);
>      }
> -    g_assert(fdt || coldplug);
> +    g_assert(fdt);
>  
>      drc->dev = d;
>      drc->fdt = fdt;
>      drc->fdt_start_offset = fdt_start_offset;
> -    drc->configured = coldplug;
>  
>      if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>          drc->awaiting_allocation = true;
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 0b447f2..f2543ef 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1435,8 +1435,7 @@ static void spapr_phb_hot_plug_child(HotplugHandler *plug_handler,
>          goto out;
>      }
>  
> -    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset,
> -                     !plugged_dev->hotplugged, &local_err);
> +    spapr_drc_attach(drc, DEVICE(pdev), fdt, fdt_start_offset, &local_err);
>      if (local_err) {
>          goto out;
>      }
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 6fd84d1..d15e9eb 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -234,7 +234,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, Object *owner,
>                            uint32_t drc_type_mask);
>  
>  void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> -                      int fdt_start_offset, bool coldplug, Error **errp);
> +                      int fdt_start_offset, Error **errp);
>  void spapr_drc_detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp);
>  
>  #endif /* HW_SPAPR_DRC_H */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[Qemu-devel] [PATCH 5/5] spapr: Use unplug_request for PCI hot unplug

[David Gibson]

AIUI, ->unplug_request in the HotplugHandler is used for "soft"
unplug, where acknowledgement from the guest is required before
completing the unplug, whereas ->unplug is used for "hard" unplug
where qemu unilaterally removes the device, and the guest just has to
cope with its sudden absence.  For spapr we (correctly) use
->unplug_request for CPU and memory hot unplug but we use ->unplug for
PCI.

While I think it might be possible to support "hard" PCI unplug within
the PAPR model, that's not how it actually works now.  Although it's
called from ->unplug, the PCI unplug path will usually just mark the
device for removal, with completion of the unplug delayed until
userspace responds to the unplug notification. If the guest doesn't
respond as expected, that could delay the unplug completiong
arbitrarily long.

To reflect that, change the PCI unplug path to be called from
->unplug_request.

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

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f2543ef..bda8938 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1469,8 +1469,8 @@ out:
     }
 }
 
-static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
-                                       DeviceState *plugged_dev, Error **errp)
+static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
+                                     DeviceState *plugged_dev, Error **errp)
 {
     sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
     PCIDevice *pdev = PCI_DEVICE(plugged_dev);
@@ -1485,6 +1485,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
     }
 
     g_assert(drc);
+    g_assert(drc->dev == plugged_dev);
 
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
     if (!drck->release_pending(drc)) {
@@ -1973,7 +1974,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = true;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hp->plug = spapr_phb_hot_plug_child;
-    hp->unplug = spapr_phb_hot_unplug_child;
+    hp->unplug_request = spapr_pci_unplug_request;
 }
 
 static const TypeInfo spapr_phb_info = {
-- 
2.9.4

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/5] spapr: Use unplug_request for PCI hot unplug

[Laurent Vivier]

On 20/06/2017 03:53, David Gibson wrote:
> AIUI, ->unplug_request in the HotplugHandler is used for "soft"
> unplug, where acknowledgement from the guest is required before
> completing the unplug, whereas ->unplug is used for "hard" unplug
> where qemu unilaterally removes the device, and the guest just has to
> cope with its sudden absence.  For spapr we (correctly) use
> ->unplug_request for CPU and memory hot unplug but we use ->unplug for
> PCI.
> 
> While I think it might be possible to support "hard" PCI unplug within
> the PAPR model, that's not how it actually works now.  Although it's
> called from ->unplug, the PCI unplug path will usually just mark the
> device for removal, with completion of the unplug delayed until
> userspace responds to the unplug notification. If the guest doesn't
> respond as expected, that could delay the unplug completiong
> arbitrarily long.
> 
> To reflect that, change the PCI unplug path to be called from
> ->unplug_request.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>  hw/ppc/spapr_pci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index f2543ef..bda8938 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1469,8 +1469,8 @@ out:
>      }
>  }
>  
> -static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> -                                       DeviceState *plugged_dev, Error **errp)
> +static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> +                                     DeviceState *plugged_dev, Error **errp)
>  {
>      sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
>      PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> @@ -1485,6 +1485,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
>      }
>  
>      g_assert(drc);
> +    g_assert(drc->dev == plugged_dev);
>  
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      if (!drck->release_pending(drc)) {
> @@ -1973,7 +1974,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>      dc->user_creatable = true;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      hp->plug = spapr_phb_hot_plug_child;
> -    hp->unplug = spapr_phb_hot_unplug_child;
> +    hp->unplug_request = spapr_pci_unplug_request;
>  }
>  
>  static const TypeInfo spapr_phb_info = {
> 

Re: [Qemu-devel] [PATCH 5/5] spapr: Use unplug_request for PCI hot unplug

[Greg Kurz]

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

On Tue, 20 Jun 2017 09:53:32 +0800
David Gibson <david@gibson.dropbear.id.au> wrote:

> AIUI, ->unplug_request in the HotplugHandler is used for "soft"
> unplug, where acknowledgement from the guest is required before
> completing the unplug, whereas ->unplug is used for "hard" unplug
> where qemu unilaterally removes the device, and the guest just has to
> cope with its sudden absence.  For spapr we (correctly) use
> ->unplug_request for CPU and memory hot unplug but we use ->unplug for  
> PCI.
> 
> While I think it might be possible to support "hard" PCI unplug within
> the PAPR model, that's not how it actually works now.  Although it's
> called from ->unplug, the PCI unplug path will usually just mark the
> device for removal, with completion of the unplug delayed until
> userspace responds to the unplug notification. If the guest doesn't
> respond as expected, that could delay the unplug completiong
> arbitrarily long.
> 
> To reflect that, change the PCI unplug path to be called from
> ->unplug_request.  
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index f2543ef..bda8938 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1469,8 +1469,8 @@ out:
>      }
>  }
>  
> -static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> -                                       DeviceState *plugged_dev, Error **errp)
> +static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> +                                     DeviceState *plugged_dev, Error **errp)
>  {
>      sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
>      PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> @@ -1485,6 +1485,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
>      }
>  
>      g_assert(drc);
> +    g_assert(drc->dev == plugged_dev);
>  
>      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>      if (!drck->release_pending(drc)) {
> @@ -1973,7 +1974,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>      dc->user_creatable = true;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      hp->plug = spapr_phb_hot_plug_child;

Maybe you can rename spapr_phb_hot_plug_child() to spapr_pci_plug() for
consistency ?

Anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

> -    hp->unplug = spapr_phb_hot_unplug_child;
> +    hp->unplug_request = spapr_pci_unplug_request;
>  }
>  
>  static const TypeInfo spapr_phb_info = {


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 5/5] spapr: Use unplug_request for PCI hot unplug

[David Gibson]

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

On Wed, Jun 21, 2017 at 11:50:08AM +0200, Greg Kurz wrote:
> On Tue, 20 Jun 2017 09:53:32 +0800
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > AIUI, ->unplug_request in the HotplugHandler is used for "soft"
> > unplug, where acknowledgement from the guest is required before
> > completing the unplug, whereas ->unplug is used for "hard" unplug
> > where qemu unilaterally removes the device, and the guest just has to
> > cope with its sudden absence.  For spapr we (correctly) use
> > ->unplug_request for CPU and memory hot unplug but we use ->unplug for  
> > PCI.
> > 
> > While I think it might be possible to support "hard" PCI unplug within
> > the PAPR model, that's not how it actually works now.  Although it's
> > called from ->unplug, the PCI unplug path will usually just mark the
> > device for removal, with completion of the unplug delayed until
> > userspace responds to the unplug notification. If the guest doesn't
> > respond as expected, that could delay the unplug completiong
> > arbitrarily long.
> > 
> > To reflect that, change the PCI unplug path to be called from
> > ->unplug_request.  
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index f2543ef..bda8938 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1469,8 +1469,8 @@ out:
> >      }
> >  }
> >  
> > -static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> > -                                       DeviceState *plugged_dev, Error **errp)
> > +static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
> > +                                     DeviceState *plugged_dev, Error **errp)
> >  {
> >      sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> >      PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> > @@ -1485,6 +1485,7 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler,
> >      }
> >  
> >      g_assert(drc);
> > +    g_assert(drc->dev == plugged_dev);
> >  
> >      drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >      if (!drck->release_pending(drc)) {
> > @@ -1973,7 +1974,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
> >      dc->user_creatable = true;
> >      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> >      hp->plug = spapr_phb_hot_plug_child;
> 
> Maybe you can rename spapr_phb_hot_plug_child() to spapr_pci_plug() for
> consistency ?

Good idea.  I've made that change.

> 
> Anyway,
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > -    hp->unplug = spapr_phb_hot_unplug_child;
> > +    hp->unplug_request = spapr_pci_unplug_request;
> >  }
> >  
> >  static const TypeInfo spapr_phb_info = {
> 



-- 
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] [PATCH 0/5] spapr: DRC cleanups (part V)

[David Gibson]

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

On Tue, Jun 20, 2017 at 09:53:27AM +0800, David Gibson wrote:
> This fifth set of cleanups to the DRC code mostly deals with removing
> unnecessary differences between different cases on the various hot
> plug and unplug paths.

With the assorted R-bs accumulated, I've merged this batch into
ppc-for-2.10.

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