Re: [Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

[Peter Crosthwaite]

On Tue, Dec 17, 2013 at 9:26 AM, Anthony Liguori <address@hidden> wrote:
> Igor Mammedov <address@hidden> 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
>> link<foo...> 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
>