| Message ID | 20190116113523.9213-10-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qdev: Hotplug handler chaining + virtio-pmem | expand |
On Wed, 16 Jan 2019 12:35:22 +0100 David Hildenbrand <david@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 > 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 <david@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? > return HOTPLUG_HANDLER(machine); > } >
On 18.01.19 11:20, Igor Mammedov wrote: > On Wed, 16 Jan 2019 12:35:22 +0100 > David Hildenbrand <david@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 <david@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. >> return HOTPLUG_HANDLER(machine); >> } >> >
On Fri, 18 Jan 2019 13:53:14 +0100 David Hildenbrand <david@redhat.com> wrote: > On 18.01.19 11:20, Igor Mammedov wrote: > > On Wed, 16 Jan 2019 12:35:22 +0100 > > David Hildenbrand <david@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 <david@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. > > >> return HOTPLUG_HANDLER(machine); > >> } > >> > > > >
On 18.01.19 15:37, Igor Mammedov wrote: > On Fri, 18 Jan 2019 13:53:14 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 18.01.19 11:20, Igor Mammedov wrote: >>> On Wed, 16 Jan 2019 12:35:22 +0100 >>> David Hildenbrand <david@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 <david@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!
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)) { return HOTPLUG_HANDLER(machine); }
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 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 <david@redhat.com> --- hw/i386/pc.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-)