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. Paolo