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

[Paolo Bonzini]

Il 25/11/2013 13:49, Paolo Bonzini ha scritto:
> 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).
> 
> 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.

I reviewed the user now.

I understand why you want to reuse the same idiom.  Perhaps you can move
this to include/hw/hotplug.h instead?  I would still prefer DimmBus or
BusClass to use the more common hotplug/hot_unplug interface, but
ACPIHotpluggableDimmBus can map it to an hotplug_fn.

Paolo