Re: [Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 25 Nov 2013 13:49:29 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/11/2013 03:38, Igor Mammedov ha scritto:
> > +typedef enum {
> > +    HOTPLUG_DISABLED,
> > +    HOTPLUG_ENABLED,
> > +    COLDPLUG_ENABLED,
> > +} HotplugState;
> > +
> > +typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev,
> > +                          HotplugState state);
> 
> I don't think this is a particularly useful interface, because it
> reinvents a lot of what already exists in qdev/qbus.
> 
> The cold/hotplug_enabled differentiation is not particularly useful
> when dev->hotplugged already exists, and the interface leaves one to
> wonder why COLDPLUG_ENABLED doesn't have a matching COLDPLUG_DISABLED
> (until one looks at the code).
Yes, it's a bit weird but I've opted in favor of unifying currently
used by piix4/ich9_acpi approach so it could be reused vs rewriting
yet another part of QEMU and Michael also uses this in his new PCIEHP
patches.
Maybe rewriting it as you suggest should be made as a separate series.


But I have unrelated to this question, regarding patch
 "[PATCH 01/27] acpi: factor out common pm_update_sci() into acpi core"

in old pm_update_sci() we have:
    sci_level = (((pmsts & s->ar.pm1.evt.en) &
                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                   ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
          (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);

where we mask out currently not supported GPE status bits,
can we just drop (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS) mask
altogether and rely on gpe.sts & gpe.en only for rising SCI?
Why there were need to mask SCI at all?

> Take for example this:
> 
> static void enable_device(PIIX4PMState *s, int slot)
> {
>     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);
> 
>     /* 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) {
>         s->pci0_slot_device_present |= (1U << slot);
>         return 0;
>     }
> 
>     if (state == PCI_HOTPLUG_ENABLED) {
>         enable_device(s, slot);
>     } else {
>         disable_device(s, slot);
>     }
> 
>     pm_update_sci(s);
> 
>     return 0;
> }
> 
> 
> In my opinion, it is much clearer this way---separating enable/disabled
> rather than hotplug/coldplug:
> 
> static void piix4_pci_send_event(PIIX4PMState *s)
> {
>     s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
>     pm_update_sci(s);
> }
> 
> static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev)
> {
>     PCIDevice *pci = PCI_DEVICE(dev);
>     int slot = PCI_SLOT(pci->devfn);
>     PIIX4PMState *s = PIIX4_PM(qdev);
> 
>     s->pci0_slot_device_present |= (1U << slot);
>     /* 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 (dev->hotplugged) {
>         piix4_pci_send_event(s);
>     }
>     return 0;
> }
> 
> static int piix4_device_hot_unplug(DeviceState *qdev, DeviceState *dev)
> {
>     PCIDevice *pci = PCI_DEVICE(dev);
>     int slot = PCI_SLOT(pci->devfn);
>     PIIX4PMState *s = PIIX4_PM(qdev);
> 
>     s->pci0_status.down |= (1U << slot);
>     piix4_pci_send_event(s);
>     return 0;
> }
> 
> This is of course just an example, I'm not asking you to reimplement hotplug
> for all buses in QEMU.  However, my point is that this shouldn't be a "core"
> enum and interface.  If you want to have a core interface in BusClass, I'd
> rather add ->hotplug and ->hot_unplug to BusClass, similar to how the SCSI
> bus does it (note: I only maintain it, I didn't write that code) and so
> that BusClass's methods match ->init/->exit on the device side.
> 
> Paolo