Re: [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 09 Dec 2013 11:02:29 +0200
Marcel Apfelbaum <marcel.a@redhat.com> wrote:

> On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > Split piix4_device_hotplug() into hotplug/unplug callbacks
> > and register them as "hotplug-device" interface implementation of
> > PIIX4_PM device.
> > 
> > Replace pci_bus_hotplug() wiring with setting link on
> > PCI BUS "hotplug-device" property to PIIX4_PM device.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/piix4.c |   73 +++++++++++++++++++++++++++++++------------------------
> >  1 files changed, 41 insertions(+), 32 deletions(-)
> > 
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 8084a60..8327d80 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -30,6 +30,8 @@
> >  #include "hw/nvram/fw_cfg.h"
> >  #include "exec/address-spaces.h"
> >  #include "hw/acpi/piix4.h"
> > +#include "qapi/qmp/qerror.h"
> > +#include "hw/hotplug.h"
> >  
> >  //#define DEBUG
> >  
> > @@ -107,7 +109,7 @@ typedef struct PIIX4PMState {
> >      OBJECT_CHECK(PIIX4PMState, (obj), TYPE_PIIX4_PM)
> >  
> >  static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > -                                           PCIBus *bus, PIIX4PMState *s);
> > +                                           BusState *bus, PIIX4PMState *s);
> >  
> >  #define ACPI_ENABLE 0xf1
> >  #define ACPI_DISABLE 0xf0
> > @@ -478,7 +480,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
> >      qemu_add_machine_init_done_notifier(&s->machine_ready);
> >      qemu_register_reset(piix4_reset, s);
> >  
> > -    piix4_acpi_system_hot_add_init(pci_address_space_io(dev), dev->bus, s);
> > +    piix4_acpi_system_hot_add_init(pci_address_space_io(dev), BUS(dev->bus), s);
> >  
> >      piix4_pm_add_propeties(s);
> >      return 0;
> > @@ -664,13 +666,11 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque)
> >      piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
> >  }
> >  
> > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> > -                                PCIHotplugState state);
> > -
> >  static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > -                                           PCIBus *bus, PIIX4PMState *s)
> > +                                           BusState *bus, PIIX4PMState *s)
> >  {
> >      CPUState *cpu;
> > +    Error *local_error = NULL;
> >  
> >      memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> >                            "acpi-gpe0", GPE_LEN);
> > @@ -680,7 +680,14 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> >                            "acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
> >      memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
> >                                  &s->io_pci);
> > -    pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
> > +    object_property_set_link(OBJECT(bus), OBJECT(s),
> > +                             QDEV_HOTPLUG_DEVICE_PROPERTY, &local_error);
> > +    if (error_is_set(&local_error)) {
> > +        qerror_report_err(local_error);
> > +        error_free(local_error);
> > +        abort();
> > +    }
> > +    bus->allow_hotplug = 1;
> I saw the exact same lines in this patch and the next two,
> I would "recycle" the pci_bus_hotplug (changing the signature
> and implementation) in order to avoid code duplication.
> What do you think?
it's not exactly the same, in shpc case we do not abort.
I've left it with a bit of code duplication for reason that in abort case 
error check code disappears one Peter's abort_error patch is merged 
(it's on my todo list to switch to  abort_error once available).

And as for "bus->allow_hotplug = 1" it should also be removed once all
current users are switched to common hotplug interface, then empty link could
serve the same purpose as allow_hotplug.

I'm not sure that creating function to replace essentially 2-liner in 2 places
justifies trouble considering that it should be obsoleted anyway.


> Thanks,
> Marcel
> 
> >  
> >      CPU_FOREACH(cpu) {
> >          CPUClass *cc = CPU_GET_CLASS(cpu);
> > @@ -696,41 +703,36 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> >      qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
> >  }
> >  
> > -static void enable_device(PIIX4PMState *s, int slot)
> > +static void piix4_pci_device_hotplug_cb(DeviceState *hotplug_dev,
> > +                                        DeviceState *dev, Error **errp)
> >  {
> > -    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> > -    s->pci0_slot_device_present |= (1U << slot);
> > -}
> > -
> > -static void disable_device(PIIX4PMState *s, int slot)
> > -{
> > -    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> > -    s->pci0_status.down |= (1U << slot);
> > -}
> > -
> > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> > -				PCIHotplugState state)
> > -{
> > -    int slot = PCI_SLOT(dev->devfn);
> > -    PIIX4PMState *s = PIIX4_PM(qdev);
> > +    PCIDevice *pci_dev = PCI_DEVICE(dev);
> > +    int slot = PCI_SLOT(pci_dev->devfn);
> > +    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> >  
> >      /* Don't send event when device is enabled during qemu machine creation:
> >       * it is present on boot, no hotplug event is necessary. We do send an
> >       * event when the device is disabled later. */
> > -    if (state == PCI_COLDPLUG_ENABLED) {
> > +    if (!dev->hotplugged) {
> >          s->pci0_slot_device_present |= (1U << slot);
> > -        return 0;
> > -    }
> > -
> > -    if (state == PCI_HOTPLUG_ENABLED) {
> > -        enable_device(s, slot);
> > -    } else {
> > -        disable_device(s, slot);
> > +        return;
> >      }
> >  
> > +    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> > +    s->pci0_slot_device_present |= (1U << slot);
> >      pm_update_sci(s);
> > +}
> >  
> > -    return 0;
> > +static void piix4_pci_device_hot_unplug_cb(DeviceState *hotplug_dev,
> > +                                           DeviceState *dev, Error **errp)
> > +{
> > +    PCIDevice *pci_dev = PCI_DEVICE(dev);
> > +    int slot = PCI_SLOT(pci_dev->devfn);
> > +    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
> > +
> > +    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> > +    s->pci0_status.down |= (1U << slot);
> > +    pm_update_sci(s);
> >  }
> >  
> >  static Property piix4_pm_properties[] = {
> > @@ -745,6 +747,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> >      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +    HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass);
> >  
> >      k->no_hotplug = 1;
> >      k->init = piix4_pm_initfn;
> > @@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> >      dc->no_user = 1;
> >      dc->vmsd = &vmstate_acpi;
> >      dc->props = piix4_pm_properties;
> > +    hc->hotplug = piix4_pci_device_hotplug_cb;
> > +    hc->hot_unplug = piix4_pci_device_hot_unplug_cb;
> >  }
> >  
> >  static const TypeInfo piix4_pm_info = {
> > @@ -764,6 +769,10 @@ static const TypeInfo piix4_pm_info = {
> >      .parent        = TYPE_PCI_DEVICE,
> >      .instance_size = sizeof(PIIX4PMState),
> >      .class_init    = piix4_pm_class_init,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_HOTPLUG_DEVICE },
> > +        { }
> > +    }
> >  };
> >  
> >  static void piix4_pm_register_types(void)
> 
> 
>