Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
Il 17/12/2013 20:38, Anthony Liguori ha scritto: On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 00:26, Anthony Liguori ha scritto: Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. Can you explain what you mean? The question is whether hotpluggable as a property applies to all devices or not. But hotplug is strictly a bus level concept. It's a sequence of events that correspond to what happens when you add a new device to a bus after power on. Hotplugging a device is a special case of plugging a device. If a bus or device only supports cold-plug, that can be done using bc-allow_hotplug = false or dc-hotpluggable = false. Igor's interface applies just as well to the case of plugging a device at startup; I think separating the two makes little sense. And once you have cold-plug and hot-plug in qdev core, it makes sense to add unplug as well. Also because we already have surprise removal in qdev core (that's unparent) and we have some kind of unplug request support (device_del/dc-unplug). One possibility that remains is to put cold/hot-plug in a BusDevice class rather than in the core qdev: Device BusDevice-- can be cold/hot-plugged but this adds more complication. For example, the same CPU can be hotpluggable or not depending on the board model, should the superclass be Device or BusDevice. And if we ever have multi-CPU targets, with the core CPU not hotpluggable and additional hotpluggable ones (e.g. for GPUs) what would be the superclass of the common CPU superclass? The question is whether there can be code sharing without touching the base class. You could certainly have a HotpluggableBusState and then a HotpluggableDeviceState. Interfaces would be another option too. Interfaces are fine, but the question is who finds them and calls them. In this case, the discovery mechanism is a link property, and the calling mechanism is an explicit hook in the realized property. If we had aspect-oriented programming, we would be marking join points instead of writing if (dev-foo) bar(dev-foo) conditionals. But the idea is the same. The general concern is about polluting widely used base classes. It's better if we can avoid adding things to DeviceState and Object whenever possible. I agree. At the same time we should make base classes as small as possible, but not smaller than that. Paolo
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Wed, 18 Dec 2013 11:36:52 +0100 Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 20:38, Anthony Liguori ha scritto: On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 00:26, Anthony Liguori ha scritto: Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. Can you explain what you mean? The question is whether hotpluggable as a property applies to all devices or not. I think Andreas asked me to provide hotpluggable property to distinguish hotpluggable vs not hotpluggable DimmDevice via qom interface. But hotplug is strictly a bus level concept. It's a sequence of events that correspond to what happens when you add a new device to a bus after power on. Hotplugging a device is a special case of plugging a device. If a bus or device only supports cold-plug, that can be done using bc-allow_hotplug = false or dc-hotpluggable = false. Do we need per instance ability to set hotpluggable property? For example board might want to mark some CPUs as not hotpluggable. Igor's interface applies just as well to the case of plugging a device at startup; I think separating the two makes little sense. And once you have cold-plug and hot-plug in qdev core, it makes sense to add unplug as well. Also because we already have surprise removal in qdev core (that's unparent) and we have some kind of unplug request support (device_del/dc-unplug). One possibility that remains is to put cold/hot-plug in a BusDevice class rather than in the core qdev: Device BusDevice-- can be cold/hot-plugged but this adds more complication. For example, the same CPU can be hotpluggable or not depending on the board model, should the superclass be Device or BusDevice. And if we ever have multi-CPU targets, with the core CPU not hotpluggable and additional hotpluggable ones (e.g. for GPUs) what would be the superclass of the common CPU superclass? The question is whether there can be code sharing without touching the base class. You could certainly have a HotpluggableBusState and then a HotpluggableDeviceState. Interfaces would be another option too. Interfaces are fine, but the question is who finds them and calls them. In this case, the discovery mechanism is a link property, and the calling mechanism is an explicit hook in the realized property. If we don't need per instance hotpluggable state and we can call interfaces from generic qdev/device code, then we would need at first only TYPE_HOTPLUGGABLE_BUS_DEVICE_IF and later for link based hotplug we could add just TYPE_HOTPLUGGABLE_DEVICE_IF. Difference would be in the way they get access to hotplug device link, former one will use bus for it and second some other way. If we had aspect-oriented programming, we would be marking join points instead of writing if (dev-foo) bar(dev-foo) conditionals. But the idea is the same. The general concern is about polluting widely used base classes. It's better if we can avoid adding things to DeviceState and Object whenever possible. I agree. At the same time we should make base classes as small as possible, but not smaller than that. Paolo
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
Il 18/12/2013 16:48, Igor Mammedov ha scritto: Hotplugging a device is a special case of plugging a device. If a bus or device only supports cold-plug, that can be done using bc-allow_hotplug = false or dc-hotpluggable = false. Do we need per instance ability to set hotpluggable property? For example board might want to mark some CPUs as not hotpluggable. I think such fine-grained control could be handled from dc-unplug. Let's not do anything more than we need, until we need it. Right now, all we need to model is the fact that a device X can act as hotplug controller for multiple buses, X is not the parent of those buses, and we need to tell those buses about X. This is hotplug-handler in BusState. We also like to distinguish devices that only support -device (or are even only board-instantiatable) from devices that support device_add. This is dc-hotpluggable. It is not absolutely necessary, but it makes sense if plugging gets a more central place in qdev. This is the case after you add hotplug-handler. Interfaces would be another option too. Interfaces are fine, but the question is who finds them and calls them. In this case, the discovery mechanism is a link property, and the calling mechanism is an explicit hook in the realized property. If we don't need per instance hotpluggable state and we can call interfaces from generic qdev/device code, then we would need at first only TYPE_HOTPLUGGABLE_BUS_DEVICE_IF and later for link based hotplug we could add just TYPE_HOTPLUGGABLE_DEVICE_IF. Difference would be in the way they get access to hotplug device link, former one will use bus for it and second some other way. I think this is overengineered. What you have is flexible and decently clean. I don't think there's any need to go from the device to the bus and from there optionally to the handler. Most of the time you couldn't do anything really in the bus, you would have to call some sort of bus-specific callback (like SCSIBusInfo). It is then equivalent to set the bus's parent device as a (hot)plug handler and go straight from the device to the handler, as in your patches. Paolo
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Wed, Dec 18, 2013 at 04:48:09PM +0100, Igor Mammedov wrote: On Wed, 18 Dec 2013 11:36:52 +0100 Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 20:38, Anthony Liguori ha scritto: On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 00:26, Anthony Liguori ha scritto: Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. Can you explain what you mean? The question is whether hotpluggable as a property applies to all devices or not. I think Andreas asked me to provide hotpluggable property to distinguish hotpluggable vs not hotpluggable DimmDevice via qom interface. But hotplug is strictly a bus level concept. It's a sequence of events that correspond to what happens when you add a new device to a bus after power on. Hotplugging a device is a special case of plugging a device. If a bus or device only supports cold-plug, that can be done using bc-allow_hotplug = false or dc-hotpluggable = false. Do we need per instance ability to set hotpluggable property? For example board might want to mark some CPUs as not hotpluggable. It could be useful. In real life same device can be on-board or on a plugin card. But it's not a must, we survived without this so far. So maybe start not supporting it, add later? Igor's interface applies just as well to the case of plugging a device at startup; I think separating the two makes little sense. And once you have cold-plug and hot-plug in qdev core, it makes sense to add unplug as well. Also because we already have surprise removal in qdev core (that's unparent) and we have some kind of unplug request support (device_del/dc-unplug). One possibility that remains is to put cold/hot-plug in a BusDevice class rather than in the core qdev: Device BusDevice-- can be cold/hot-plugged but this adds more complication. For example, the same CPU can be hotpluggable or not depending on the board model, should the superclass be Device or BusDevice. And if we ever have multi-CPU targets, with the core CPU not hotpluggable and additional hotpluggable ones (e.g. for GPUs) what would be the superclass of the common CPU superclass? The question is whether there can be code sharing without touching the base class. You could certainly have a HotpluggableBusState and then a HotpluggableDeviceState. Interfaces would be another option too. Interfaces are fine, but the question is who finds them and calls them. In this case, the discovery mechanism is a link property, and the calling mechanism is an explicit hook in the realized property. If we don't need per instance hotpluggable state and we can call interfaces from generic qdev/device code, then we would need at first only TYPE_HOTPLUGGABLE_BUS_DEVICE_IF and later for link based hotplug we could add just TYPE_HOTPLUGGABLE_DEVICE_IF. Difference would be in the way they get access to hotplug device link, former one will use bus for it and second some other way. If we had aspect-oriented programming, we would be marking join points instead of writing if (dev-foo) bar(dev-foo) conditionals. But the idea is the same. The general concern is about polluting widely used base classes. It's better if we can avoid adding things to DeviceState and Object whenever possible. I agree. At the same time we should make base classes as small as possible, but not smaller than that. Paolo
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Wed, 18 Dec 2013 18:26:07 +0200 Michael S. Tsirkin m...@redhat.com wrote: On Wed, Dec 18, 2013 at 04:48:09PM +0100, Igor Mammedov wrote: On Wed, 18 Dec 2013 11:36:52 +0100 Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 20:38, Anthony Liguori ha scritto: On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 00:26, Anthony Liguori ha scritto: Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. Can you explain what you mean? The question is whether hotpluggable as a property applies to all devices or not. I think Andreas asked me to provide hotpluggable property to distinguish hotpluggable vs not hotpluggable DimmDevice via qom interface. But hotplug is strictly a bus level concept. It's a sequence of events that correspond to what happens when you add a new device to a bus after power on. Hotplugging a device is a special case of plugging a device. If a bus or device only supports cold-plug, that can be done using bc-allow_hotplug = false or dc-hotpluggable = false. Do we need per instance ability to set hotpluggable property? For example board might want to mark some CPUs as not hotpluggable. It could be useful. In real life same device can be on-board or on a plugin card. But it's not a must, we survived without this so far. So maybe start not supporting it, add later? Yes, that's surely could be done later Paolo
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Wed, 18 Dec 2013 16:59:02 +0100 Paolo Bonzini pbonz...@redhat.com wrote: Il 18/12/2013 16:48, Igor Mammedov ha scritto: Hotplugging a device is a special case of plugging a device. If a bus or device only supports cold-plug, that can be done using bc-allow_hotplug = false or dc-hotpluggable = false. Do we need per instance ability to set hotpluggable property? For example board might want to mark some CPUs as not hotpluggable. I think such fine-grained control could be handled from dc-unplug. Let's not do anything more than we need, until we need it. I had in mind a lightweight conversion of initial memory to DimmDevices for i386 target and disabling its unplug until initial memory unplug would be properly handled by board and SeaBIOS. Paolo
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Mon, 16 Dec 2013 15:26:37 -0800 Anthony Liguori anth...@codemonkey.ws wrote: Igor Mammedov imamm...@redhat.com writes: changes since v2: * s/hotplugable/hotpluggable/ * move hotplug check to an earlier patch: qdev: add hotpluggable property to Device -- Refactor PCI specific hotplug API to a more generic/reusable one. Model it after SCSI-BUS like hotplug API replacing single hotplug callback with hotplug/hot_unplug pair of callbacks as suggested by Paolo. Difference between SCSI-BUS and this approach is that the former is BUS centric while the latter is device centred. Which is evolved from the fact that hotplug callbacks used by ACPI/SHPC/PCIE are implemented by devices rather than by bus and bus serves only as a proxy to forward event to hotplug device. Memory hotplug also exposes tha same usage pattern hence an attempt to generalize hotplug API. Refactoring also simplifies wiring of a hotplug device with a bus, all it needs is to set hotplug-device link on bus, which would potentially allow to do it from configuration file, there is not need to setup hotplug device callbacks on bus synce it can get them via HOTPLUG_DEVICE API of hotplug-device target. In addition device centred hotplug API may be used by bus-less hotplug implementations as well if it's decided to use linkfoo... instead of bus. I'm having a hard time parsing this description. Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. we could add TYPE_BUS_DEVICE as Peter suggests but let me explain why I've chosen DEVICE for it. All current hotplug code depends on hotpluggable bus but periodically there were suggestions to use links for hotplug instead of bus. Making hotplug the bus device only concept will not allow to move in direction of hotplug with links which are/were supposed to replace buses in future. As side effect series lays ground work for link based hotplug, it's not complete but a step towards it. It still doesn't allow link based hotplug, since purpose was to have unified hotplug interface across PCI/SCSI/virtio devices/buses. Maybe we need TYPE_HOTPLUGGABLE_DEVICE so it could be abstracted from bus as well and eventually it could be used with bussless devices? The series is a net add of code so I don't think we're winning anything by generalizing here. it will allow to remove different implementations of hotplug mechanism in SCSI/virtio and maybe USB buses in addition to converted PCI hotplug. Is there a use-case this enables that isn't possible today? It was prompted by memory hotplug, alternative was that it can re-implement hotplug mechanism in its own way or duplicating code from scsi or pci buses/devices. Neither alternatives look nice when there is a common usage pattern. Regards, Anthony Liguori Patches 8-11 are should be merged as one and are split only for simplifying review (they compile fine but PCI hotplug is broken until the last patch is applyed). git tree for testing: https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3 tested only ACPI and PCIE hotplug. Hervé Poussineau (1): qom: detect bad reentrance during object_class_foreach Igor Mammedov (9): define hotplug interface qdev: add to BusState hotplug-handler link qdev: add hotpluggable property to Device hw/acpi: move typeinfo to the file end qdev:pci: refactor PCIDevice to use generic hotpluggable property acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API pci/shpc: convert SHPC hotplug to use hotplug-handler API pci/pcie: convert PCIE hotplug to use hotplug-handler API hw/pci: switch to a generic hotplug handling for PCIDevice Paolo Bonzini (1): qom: do not register interface types in the type table hw/acpi/piix4.c| 151 ++--- hw/core/Makefile.objs | 1 + hw/core/hotplug.c | 48 + hw/core/qdev.c | 50 -- hw/display/cirrus_vga.c| 2 +- hw/display/qxl.c | 2 +- hw/display/vga-pci.c | 2 +- hw/display/vmware_vga.c| 2 +- hw/i386/acpi-build.c | 6 +- hw/ide/piix.c | 4 +- hw/isa/piix4.c | 2 +- hw/pci-bridge/pci_bridge_dev.c | 9 +++ hw/pci-host/piix.c | 6 +- hw/pci/pci.c | 40 +-- hw/pci/pcie.c | 73 +--- hw/pci/pcie_port.c | 8 +++ hw/pci/shpc.c | 133 +++- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- include/hw/hotplug.h | 75 include/hw/pci/pci.h | 13
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
Il 17/12/2013 00:26, Anthony Liguori ha scritto: Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. Can you explain what you mean? The series is a net add of code so I don't think we're winning anything by generalizing here. Any generalization that's used just once will be a net add of code (and this code will be reused by SCSI and x86 memory hotplug at least; perhaps x86 CPU hotplug too). Any generalization that requires some boilerplate code will be a net add of code, too. QEMU being written in C, we unfortunately cannot avoid that. So I don't think that lines of code are a good metric. Paolo Is there a use-case this enables that isn't possible today? Regards, Anthony Liguori Patches 8-11 are should be merged as one and are split only for simplifying review (they compile fine but PCI hotplug is broken until the last patch is applyed). git tree for testing: https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3 tested only ACPI and PCIE hotplug. Hervé Poussineau (1): qom: detect bad reentrance during object_class_foreach Igor Mammedov (9): define hotplug interface qdev: add to BusState hotplug-handler link qdev: add hotpluggable property to Device hw/acpi: move typeinfo to the file end qdev:pci: refactor PCIDevice to use generic hotpluggable property acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API pci/shpc: convert SHPC hotplug to use hotplug-handler API pci/pcie: convert PCIE hotplug to use hotplug-handler API hw/pci: switch to a generic hotplug handling for PCIDevice Paolo Bonzini (1): qom: do not register interface types in the type table hw/acpi/piix4.c| 151 ++--- hw/core/Makefile.objs | 1 + hw/core/hotplug.c | 48 + hw/core/qdev.c | 50 -- hw/display/cirrus_vga.c| 2 +- hw/display/qxl.c | 2 +- hw/display/vga-pci.c | 2 +- hw/display/vmware_vga.c| 2 +- hw/i386/acpi-build.c | 6 +- hw/ide/piix.c | 4 +- hw/isa/piix4.c | 2 +- hw/pci-bridge/pci_bridge_dev.c | 9 +++ hw/pci-host/piix.c | 6 +- hw/pci/pci.c | 40 +-- hw/pci/pcie.c | 73 +--- hw/pci/pcie_port.c | 8 +++ hw/pci/shpc.c | 133 +++- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- include/hw/hotplug.h | 75 include/hw/pci/pci.h | 13 include/hw/pci/pci_bus.h | 2 - include/hw/pci/pcie.h | 5 ++ include/hw/pci/shpc.h | 8 +++ include/hw/qdev-core.h | 8 +++ qom/object.c | 17 - 28 files changed, 455 insertions(+), 220 deletions(-) create mode 100644 hw/core/hotplug.c create mode 100644 include/hw/hotplug.h -- 1.8.3.1
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini pbonz...@redhat.com wrote: Il 17/12/2013 00:26, Anthony Liguori ha scritto: Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. Can you explain what you mean? The question is whether hotpluggable as a property applies to all devices or not. But hotplug is strictly a bus level concept. It's a sequence of events that correspond to what happens when you add a new device to a bus after power on. The series is a net add of code so I don't think we're winning anything by generalizing here. Any generalization that's used just once will be a net add of code (and this code will be reused by SCSI and x86 memory hotplug at least; perhaps x86 CPU hotplug too). The question is whether there can be code sharing without touching the base class. You could certainly have a HotpluggableBusState and then a HotpluggableDeviceState. Interfaces would be another option too. Any generalization that requires some boilerplate code will be a net add of code, too. QEMU being written in C, we unfortunately cannot avoid that. So I don't think that lines of code are a good metric. The general concern is about polluting widely used base classes. It's better if we can avoid adding things to DeviceState and Object whenever possible. Regards, Anthony Liguori Paolo Is there a use-case this enables that isn't possible today? Regards, Anthony Liguori Patches 8-11 are should be merged as one and are split only for simplifying review (they compile fine but PCI hotplug is broken until the last patch is applyed). git tree for testing: https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3 tested only ACPI and PCIE hotplug. Hervé Poussineau (1): qom: detect bad reentrance during object_class_foreach Igor Mammedov (9): define hotplug interface qdev: add to BusState hotplug-handler link qdev: add hotpluggable property to Device hw/acpi: move typeinfo to the file end qdev:pci: refactor PCIDevice to use generic hotpluggable property acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API pci/shpc: convert SHPC hotplug to use hotplug-handler API pci/pcie: convert PCIE hotplug to use hotplug-handler API hw/pci: switch to a generic hotplug handling for PCIDevice Paolo Bonzini (1): qom: do not register interface types in the type table hw/acpi/piix4.c| 151 ++--- hw/core/Makefile.objs | 1 + hw/core/hotplug.c | 48 + hw/core/qdev.c | 50 -- hw/display/cirrus_vga.c| 2 +- hw/display/qxl.c | 2 +- hw/display/vga-pci.c | 2 +- hw/display/vmware_vga.c| 2 +- hw/i386/acpi-build.c | 6 +- hw/ide/piix.c | 4 +- hw/isa/piix4.c | 2 +- hw/pci-bridge/pci_bridge_dev.c | 9 +++ hw/pci-host/piix.c | 6 +- hw/pci/pci.c | 40 +-- hw/pci/pcie.c | 73 +--- hw/pci/pcie_port.c | 8 +++ hw/pci/shpc.c | 133 +++- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- include/hw/hotplug.h | 75 include/hw/pci/pci.h | 13 include/hw/pci/pci_bus.h | 2 - include/hw/pci/pcie.h | 5 ++ include/hw/pci/shpc.h | 8 +++ include/hw/qdev-core.h | 8 +++ qom/object.c | 17 - 28 files changed, 455 insertions(+), 220 deletions(-) create mode 100644 hw/core/hotplug.c create mode 100644 include/hw/hotplug.h -- 1.8.3.1
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
Igor Mammedov imamm...@redhat.com writes: changes since v2: * s/hotplugable/hotpluggable/ * move hotplug check to an earlier patch: qdev: add hotpluggable property to Device -- Refactor PCI specific hotplug API to a more generic/reusable one. Model it after SCSI-BUS like hotplug API replacing single hotplug callback with hotplug/hot_unplug pair of callbacks as suggested by Paolo. Difference between SCSI-BUS and this approach is that the former is BUS centric while the latter is device centred. Which is evolved from the fact that hotplug callbacks used by ACPI/SHPC/PCIE are implemented by devices rather than by bus and bus serves only as a proxy to forward event to hotplug device. Memory hotplug also exposes tha same usage pattern hence an attempt to generalize hotplug API. Refactoring also simplifies wiring of a hotplug device with a bus, all it needs is to set hotplug-device link on bus, which would potentially allow to do it from configuration file, there is not need to setup hotplug device callbacks on bus synce it can get them via HOTPLUG_DEVICE API of hotplug-device target. In addition device centred hotplug API may be used by bus-less hotplug implementations as well if it's decided to use linkfoo... instead of bus. I'm having a hard time parsing this description. Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. The series is a net add of code so I don't think we're winning anything by generalizing here. Is there a use-case this enables that isn't possible today? Regards, Anthony Liguori Patches 8-11 are should be merged as one and are split only for simplifying review (they compile fine but PCI hotplug is broken until the last patch is applyed). git tree for testing: https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3 tested only ACPI and PCIE hotplug. Hervé Poussineau (1): qom: detect bad reentrance during object_class_foreach Igor Mammedov (9): define hotplug interface qdev: add to BusState hotplug-handler link qdev: add hotpluggable property to Device hw/acpi: move typeinfo to the file end qdev:pci: refactor PCIDevice to use generic hotpluggable property acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API pci/shpc: convert SHPC hotplug to use hotplug-handler API pci/pcie: convert PCIE hotplug to use hotplug-handler API hw/pci: switch to a generic hotplug handling for PCIDevice Paolo Bonzini (1): qom: do not register interface types in the type table hw/acpi/piix4.c| 151 ++--- hw/core/Makefile.objs | 1 + hw/core/hotplug.c | 48 + hw/core/qdev.c | 50 -- hw/display/cirrus_vga.c| 2 +- hw/display/qxl.c | 2 +- hw/display/vga-pci.c | 2 +- hw/display/vmware_vga.c| 2 +- hw/i386/acpi-build.c | 6 +- hw/ide/piix.c | 4 +- hw/isa/piix4.c | 2 +- hw/pci-bridge/pci_bridge_dev.c | 9 +++ hw/pci-host/piix.c | 6 +- hw/pci/pci.c | 40 +-- hw/pci/pcie.c | 73 +--- hw/pci/pcie_port.c | 8 +++ hw/pci/shpc.c | 133 +++- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- include/hw/hotplug.h | 75 include/hw/pci/pci.h | 13 include/hw/pci/pci_bus.h | 2 - include/hw/pci/pcie.h | 5 ++ include/hw/pci/shpc.h | 8 +++ include/hw/qdev-core.h | 8 +++ qom/object.c | 17 - 28 files changed, 455 insertions(+), 220 deletions(-) create mode 100644 hw/core/hotplug.c create mode 100644 include/hw/hotplug.h -- 1.8.3.1
Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
On Tue, Dec 17, 2013 at 9:26 AM, Anthony Liguori anth...@codemonkey.ws wrote: Igor Mammedov imamm...@redhat.com writes: changes since v2: * s/hotplugable/hotpluggable/ * move hotplug check to an earlier patch: qdev: add hotpluggable property to Device -- Refactor PCI specific hotplug API to a more generic/reusable one. Model it after SCSI-BUS like hotplug API replacing single hotplug callback with hotplug/hot_unplug pair of callbacks as suggested by Paolo. Difference between SCSI-BUS and this approach is that the former is BUS centric while the latter is device centred. Which is evolved from the fact that hotplug callbacks used by ACPI/SHPC/PCIE are implemented by devices rather than by bus and bus serves only as a proxy to forward event to hotplug device. Memory hotplug also exposes tha same usage pattern hence an attempt to generalize hotplug API. Refactoring also simplifies wiring of a hotplug device with a bus, all it needs is to set hotplug-device link on bus, which would potentially allow to do it from configuration file, there is not need to setup hotplug device callbacks on bus synce it can get them via HOTPLUG_DEVICE API of hotplug-device target. In addition device centred hotplug API may be used by bus-less hotplug implementations as well if it's decided to use linkfoo... instead of bus. I'm having a hard time parsing this description. Sharing hot plug code is a good thing. Making hotplug a qdev-level concept seems like a bad thing to me. So hotplug seems sane as a bus level concept to me. The problem is busses are a collection of general devices so the bus concept is lost when you move from the container to the contained. Are we missing an absraction layer here - TYPE_BUS_DEVICE? The series is a net add of code so I don't think we're winning anything by generalizing here. Is there a use-case this enables that isn't possible today? Has some potential embedded applications is we want to use it for power-down/up emulation. Exactly what such a sysbus hotplug handler would look like is an open question. Regards, Peter Regards, Anthony Liguori Patches 8-11 are should be merged as one and are split only for simplifying review (they compile fine but PCI hotplug is broken until the last patch is applyed). git tree for testing: https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3 tested only ACPI and PCIE hotplug. Hervé Poussineau (1): qom: detect bad reentrance during object_class_foreach Igor Mammedov (9): define hotplug interface qdev: add to BusState hotplug-handler link qdev: add hotpluggable property to Device hw/acpi: move typeinfo to the file end qdev:pci: refactor PCIDevice to use generic hotpluggable property acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API pci/shpc: convert SHPC hotplug to use hotplug-handler API pci/pcie: convert PCIE hotplug to use hotplug-handler API hw/pci: switch to a generic hotplug handling for PCIDevice Paolo Bonzini (1): qom: do not register interface types in the type table hw/acpi/piix4.c| 151 ++--- hw/core/Makefile.objs | 1 + hw/core/hotplug.c | 48 + hw/core/qdev.c | 50 -- hw/display/cirrus_vga.c| 2 +- hw/display/qxl.c | 2 +- hw/display/vga-pci.c | 2 +- hw/display/vmware_vga.c| 2 +- hw/i386/acpi-build.c | 6 +- hw/ide/piix.c | 4 +- hw/isa/piix4.c | 2 +- hw/pci-bridge/pci_bridge_dev.c | 9 +++ hw/pci-host/piix.c | 6 +- hw/pci/pci.c | 40 +-- hw/pci/pcie.c | 73 +--- hw/pci/pcie_port.c | 8 +++ hw/pci/shpc.c | 133 +++- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- include/hw/hotplug.h | 75 include/hw/pci/pci.h | 13 include/hw/pci/pci_bus.h | 2 - include/hw/pci/pcie.h | 5 ++ include/hw/pci/shpc.h | 8 +++ include/hw/qdev-core.h | 8 +++ qom/object.c | 17 - 28 files changed, 455 insertions(+), 220 deletions(-) create mode 100644 hw/core/hotplug.c create mode 100644 include/hw/hotplug.h -- 1.8.3.1
[Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
changes since v2: * s/hotplugable/hotpluggable/ * move hotplug check to an earlier patch: qdev: add hotpluggable property to Device -- Refactor PCI specific hotplug API to a more generic/reusable one. Model it after SCSI-BUS like hotplug API replacing single hotplug callback with hotplug/hot_unplug pair of callbacks as suggested by Paolo. Difference between SCSI-BUS and this approach is that the former is BUS centric while the latter is device centred. Which is evolved from the fact that hotplug callbacks used by ACPI/SHPC/PCIE are implemented by devices rather than by bus and bus serves only as a proxy to forward event to hotplug device. Memory hotplug also exposes tha same usage pattern hence an attempt to generalize hotplug API. Refactoring also simplifies wiring of a hotplug device with a bus, all it needs is to set hotplug-device link on bus, which would potentially allow to do it from configuration file, there is not need to setup hotplug device callbacks on bus synce it can get them via HOTPLUG_DEVICE API of hotplug-device target. In addition device centred hotplug API may be used by bus-less hotplug implementations as well if it's decided to use linkfoo... instead of bus. Patches 8-11 are should be merged as one and are split only for simplifying review (they compile fine but PCI hotplug is broken until the last patch is applyed). git tree for testing: https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3 tested only ACPI and PCIE hotplug. Hervé Poussineau (1): qom: detect bad reentrance during object_class_foreach Igor Mammedov (9): define hotplug interface qdev: add to BusState hotplug-handler link qdev: add hotpluggable property to Device hw/acpi: move typeinfo to the file end qdev:pci: refactor PCIDevice to use generic hotpluggable property acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API pci/shpc: convert SHPC hotplug to use hotplug-handler API pci/pcie: convert PCIE hotplug to use hotplug-handler API hw/pci: switch to a generic hotplug handling for PCIDevice Paolo Bonzini (1): qom: do not register interface types in the type table hw/acpi/piix4.c| 151 ++--- hw/core/Makefile.objs | 1 + hw/core/hotplug.c | 48 + hw/core/qdev.c | 50 -- hw/display/cirrus_vga.c| 2 +- hw/display/qxl.c | 2 +- hw/display/vga-pci.c | 2 +- hw/display/vmware_vga.c| 2 +- hw/i386/acpi-build.c | 6 +- hw/ide/piix.c | 4 +- hw/isa/piix4.c | 2 +- hw/pci-bridge/pci_bridge_dev.c | 9 +++ hw/pci-host/piix.c | 6 +- hw/pci/pci.c | 40 +-- hw/pci/pcie.c | 73 +--- hw/pci/pcie_port.c | 8 +++ hw/pci/shpc.c | 133 +++- hw/usb/hcd-ehci-pci.c | 2 +- hw/usb/hcd-ohci.c | 2 +- hw/usb/hcd-uhci.c | 2 +- hw/usb/hcd-xhci.c | 2 +- include/hw/hotplug.h | 75 include/hw/pci/pci.h | 13 include/hw/pci/pci_bus.h | 2 - include/hw/pci/pcie.h | 5 ++ include/hw/pci/shpc.h | 8 +++ include/hw/qdev-core.h | 8 +++ qom/object.c | 17 - 28 files changed, 455 insertions(+), 220 deletions(-) create mode 100644 hw/core/hotplug.c create mode 100644 include/hw/hotplug.h -- 1.8.3.1
