On Mon, 09 Dec 2013 11:02:29 +0200
Marcel Apfelbaum <marce...@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 <imamm...@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)
>
>
>
