On 18.01.19 15:37, Igor Mammedov wrote:
> On Fri, 18 Jan 2019 13:53:14 +0100
> David Hildenbrand <da...@redhat.com> wrote:
>
>> On 18.01.19 11:20, Igor Mammedov wrote:
>>> On Wed, 16 Jan 2019 12:35:22 +0100
>>> David Hildenbrand <da...@redhat.com> wrote:
>>>
>>>> Override the PCI device hotplug handler to properly handle the
>>>> memory device part from the machine hotplug handler and forward to the
>>>> actual PCI bus hotplug handler.
>>>>
>>>> As PCI hotplug has not been properly factored out into hotplug handlers,
>>>> most magic is performed in the (un)realize functions. Also some buses
>>>> don't
>>> ^^^^^^^^ pls,
>>> be more specific
>>
>> Last time I checked there were some TYPE_PCI_HOST_BRIDGE devices in QEMU
>> that would not have a hotplug handler. I *think* they are all unrelated
>> to x86/pc. But as I was not sure if I am missing something, I introduced
>> a simple error check.
>>
>>>
>>>> have a PCI hotplug handler at all yet (not sure if they are actually used
>>>> on x86, but just to be sure).
>>>>
>>>> Signed-off-by: David Hildenbrand <da...@redhat.com>
>>>> ---
>>>> hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 89 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index fd0cb29ba9..62f83859fa 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -75,6 +75,7 @@
>>>> #include "hw/usb.h"
>>>> #include "hw/i386/intel_iommu.h"
>>>> #include "hw/net/ne2000-isa.h"
>>>> +#include "hw/mem/memory-device.h"
>>>>
>>>> /* debug PC/ISA interrupts */
>>>> //#define DEBUG_IRQ
>>>> @@ -2224,6 +2225,84 @@ static void pc_cpu_pre_plug(HotplugHandler
>>>> *hotplug_dev,
>>>> numa_cpu_pre_plug(cpu_slot, dev, errp);
>>>> }
>>>>
>>>> +static void pc_pci_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> + Error **errp)
>>>> +{
>>>> + HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> + Error *local_err = NULL;
>>>> +
>>>> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> + if (!hotplug_dev2) {
>>>> + /*
>>>> + * Without a bus hotplug handler, we cannot control the
>>>> plug/unplug
>>>> + * order. Disallow memory devices on such buses.
>>>> + */
>>>> + error_setg(errp, "Memory devices not supported on this bus.");
>>>> + return;
>>>> + }
>>>> + memory_device_pre_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev),
>>>> + NULL, errp);
>>>> + }
>>>> +
>>>> + if (hotplug_dev2) {
>>>> + hotplug_handler_pre_plug(hotplug_dev2, dev, errp);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return;
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> +static void pc_pci_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>> + Error **errp)
>>>> +{
>>>> + HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> + Error *local_err = NULL;
>>>> +
>>>> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> + memory_device_plug(MEMORY_DEVICE(dev), MACHINE(hotplug_dev));
>>>> + }
>>>> +
>>>> + if (hotplug_dev2) {
>>>> + hotplug_handler_plug(hotplug_dev2, dev, &local_err);
>>>> + }
>>>> +
>>>> + if (local_err) {
>>>> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> + memory_device_unplug(MEMORY_DEVICE(dev),
>>>> MACHINE(hotplug_dev));
>>>> + }
>>>> + }
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>> +
>>>> +static void pc_pci_unplug_request(HotplugHandler *hotplug_dev,
>>>> DeviceState *dev,
>>>> + Error **errp)
>>>> +{
>>>> + HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> +
>>>> + if (hotplug_dev2) {
>>>> + hotplug_handler_unplug_request(hotplug_dev2, dev, errp);
>>>> + }
>>>> +}
>>>> +
>>>> +static void pc_pci_device_unplug(HotplugHandler *hotplug_dev, DeviceState
>>>> *dev,
>>>> + Error **errp)
>>>> +{
>>>> + HotplugHandler *hotplug_dev2 = qdev_get_bus_hotplug_handler(dev);
>>>> + Error *local_err = NULL;
>>>> +
>>>> + if (hotplug_dev2) {
>>>> + hotplug_handler_unplug(hotplug_dev2, dev, &local_err);
>>>> + }
>>>> +
>>>> + if (!local_err) {
>>>> + if (object_dynamic_cast(OBJECT(dev), TYPE_MEMORY_DEVICE)) {
>>>> + memory_device_unplug(MEMORY_DEVICE(dev),
>>>> MACHINE(hotplug_dev));
>>>> + }
>>>> + }
>>>> + error_propagate(errp, local_err);
>>>> +}
>>>> +
>>>> static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>>> DeviceState *dev, Error **errp)
>>>> {
>>>> @@ -2231,6 +2310,8 @@ static void
>>>> pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>>>> pc_memory_pre_plug(hotplug_dev, dev, errp);
>>>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> pc_cpu_pre_plug(hotplug_dev, dev, errp);
>>>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>>> + pc_pci_pre_plug(hotplug_dev, dev, errp);
>>>> }
>>>> }
>>>>
>>>> @@ -2241,6 +2322,8 @@ static void pc_machine_device_plug_cb(HotplugHandler
>>>> *hotplug_dev,
>>>> pc_memory_plug(hotplug_dev, dev, errp);
>>>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> pc_cpu_plug(hotplug_dev, dev, errp);
>>>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>>> + pc_pci_plug(hotplug_dev, dev, errp);
>>>> }
>>>> }
>>>>
>>>> @@ -2251,6 +2334,8 @@ static void
>>>> pc_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>>> pc_memory_unplug_request(hotplug_dev, dev, errp);
>>>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> pc_cpu_unplug_request_cb(hotplug_dev, dev, errp);
>>>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>>> + pc_pci_unplug_request(hotplug_dev, dev, errp);
>>>> } else {
>>>> error_setg(errp, "acpi: device unplug request for not supported
>>>> device"
>>>> " type: %s", object_get_typename(OBJECT(dev)));
>>>> @@ -2264,6 +2349,8 @@ static void
>>>> pc_machine_device_unplug_cb(HotplugHandler *hotplug_dev,
>>>> pc_memory_unplug(hotplug_dev, dev, errp);
>>>> } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> pc_cpu_unplug_cb(hotplug_dev, dev, errp);
>>>> + } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>>> + pc_pci_device_unplug(hotplug_dev, dev, errp);
>>>> } else {
>>>> error_setg(errp, "acpi: device unplug for not supported device"
>>>> " type: %s", object_get_typename(OBJECT(dev)));
>>>> @@ -2274,7 +2361,8 @@ static HotplugHandler
>>>> *pc_get_hotpug_handler(MachineState *machine,
>>>> DeviceState *dev)
>>>> {
>>>> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) ||
>>>> - object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>>> + object_dynamic_cast(OBJECT(dev), TYPE_CPU) ||
>>>> + object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>>> why all PCI devices instead of virtio-pmem-pci one?
>>>
>>
>> Last time I tried to include "hw/virtio/virtio-pci.h" in pc.c it
>> resulted in an inclusion error madness. But this should be easy to fix I
>> guess. (the basic idea of this patch is the important bit)
>>
>> And I just tried to include and it seems to result in no errors right now
> I'd prefer override for concrete devices only and the rest PCI devices
> handled as they used to. That also should simplify hotplug handlers you
> are adding here.
Yes, will do for the next round as long as gates to inclusion hell don't
open up again :)
Thanks!
--
Thanks,
David / dhildenb
