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
