diff mbox series

[v5,06/18] vfio-user: add HotplugHandler for remote machine

Message ID fa3282607f7fed7736bfdf3c1ae9f7fce466ed44.1642626515.git.jag.raman@oracle.com (mailing list archive)
State New, archived
Headers show
Series vfio-user server in QEMU | expand

Commit Message

Jag Raman Jan. 19, 2022, 9:41 p.m. UTC 
Allow hotplugging of PCI(e) devices to remote machine

Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
---
 hw/remote/machine.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Stefan Hajnoczi Jan. 25, 2022, 10:32 a.m. UTC | #1 
On Wed, Jan 19, 2022 at 04:41:55PM -0500, Jagannathan Raman wrote:
> Allow hotplugging of PCI(e) devices to remote machine
> 
> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> ---
>  hw/remote/machine.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Why is this code necessary? I expected the default hotplug behavior to
pretty much handle this case - hotplugging device types that the bus
doesn't support should fail and unplug should already unparent/unrealize
the device.

> 
> diff --git a/hw/remote/machine.c b/hw/remote/machine.c
> index 952105eab5..220ff01aa9 100644
> --- a/hw/remote/machine.c
> +++ b/hw/remote/machine.c
> @@ -54,14 +54,39 @@ static void remote_machine_init(MachineState *machine)
>  
>      pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
>                   &s->iohub, REMOTE_IOHUB_NB_PIRQS);
> +
> +    qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));
> +}
> +
> +static void remote_machine_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                       DeviceState *dev, Error **errp)
> +{
> +    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        error_setg(errp, "Only allowing PCI hotplug");
> +    }
> +}
> +
> +static void remote_machine_unplug_cb(HotplugHandler *hotplug_dev,
> +                                     DeviceState *dev, Error **errp)
> +{
> +    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        error_setg(errp, "Only allowing PCI hot-unplug");
> +        return;
> +    }
> +
> +    qdev_unrealize(dev);
>  }
>  
>  static void remote_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>  
>      mc->init = remote_machine_init;
>      mc->desc = "Experimental remote machine";
> +
> +    hc->pre_plug = remote_machine_pre_plug_cb;
> +    hc->unplug = remote_machine_unplug_cb;
>  }
>  
>  static const TypeInfo remote_machine = {
> @@ -69,6 +94,10 @@ static const TypeInfo remote_machine = {
>      .parent = TYPE_MACHINE,
>      .instance_size = sizeof(RemoteMachineState),
>      .class_init = remote_machine_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  static void remote_machine_register_types(void)
> -- 
> 2.20.1
>
Jag Raman Jan. 25, 2022, 6:12 p.m. UTC | #2 
> On Jan 25, 2022, at 5:32 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Wed, Jan 19, 2022 at 04:41:55PM -0500, Jagannathan Raman wrote:
>> Allow hotplugging of PCI(e) devices to remote machine
>> 
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> ---
>> hw/remote/machine.c | 29 +++++++++++++++++++++++++++++
>> 1 file changed, 29 insertions(+)
> 
> Why is this code necessary? I expected the default hotplug behavior to

I just discovered that TYPE_REMOTE_MACHINE wasn't setting up a hotplug
handler for the root PCI bus.

Looks like, some of the machines don’t support hotplugging PCI devices. I see
that the ‘pc’ machine does support hotplug, whereas ‘q35’ does not.

We didn’t check hotplug in multiprocess-qemu previously because it was limited
to one device per process, and the use cases attached the devices via
command line.

> pretty much handle this case - hotplugging device types that the bus
> doesn't support should fail and unplug should already unparent/unrealize
> the device.

OK, that makes sense. We don’t need to test the device type during
plug and unplug.

Therefore, I don’t think we need a callback for the plug operation. We
could set HotplugHandlerClass->unplug callback to the default
qdev_simple_device_unplug_cb() callback.

—
Jag

> 
>> 
>> diff --git a/hw/remote/machine.c b/hw/remote/machine.c
>> index 952105eab5..220ff01aa9 100644
>> --- a/hw/remote/machine.c
>> +++ b/hw/remote/machine.c
>> @@ -54,14 +54,39 @@ static void remote_machine_init(MachineState *machine)
>> 
>>     pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
>>                  &s->iohub, REMOTE_IOHUB_NB_PIRQS);
>> +
>> +    qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));
>> +}
>> +
>> +static void remote_machine_pre_plug_cb(HotplugHandler *hotplug_dev,
>> +                                       DeviceState *dev, Error **errp)
>> +{
>> +    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        error_setg(errp, "Only allowing PCI hotplug");
>> +    }
>> +}
>> +
>> +static void remote_machine_unplug_cb(HotplugHandler *hotplug_dev,
>> +                                     DeviceState *dev, Error **errp)
>> +{
>> +    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
>> +        error_setg(errp, "Only allowing PCI hot-unplug");
>> +        return;
>> +    }
>> +
>> +    qdev_unrealize(dev);
>> }
>> 
>> static void remote_machine_class_init(ObjectClass *oc, void *data)
>> {
>>     MachineClass *mc = MACHINE_CLASS(oc);
>> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
>> 
>>     mc->init = remote_machine_init;
>>     mc->desc = "Experimental remote machine";
>> +
>> +    hc->pre_plug = remote_machine_pre_plug_cb;
>> +    hc->unplug = remote_machine_unplug_cb;
>> }
>> 
>> static const TypeInfo remote_machine = {
>> @@ -69,6 +94,10 @@ static const TypeInfo remote_machine = {
>>     .parent = TYPE_MACHINE,
>>     .instance_size = sizeof(RemoteMachineState),
>>     .class_init = remote_machine_class_init,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_HOTPLUG_HANDLER },
>> +        { }
>> +    }
>> };
>> 
>> static void remote_machine_register_types(void)
>> -- 
>> 2.20.1
>>
Stefan Hajnoczi Jan. 26, 2022, 9:35 a.m. UTC | #3 
On Tue, Jan 25, 2022 at 06:12:48PM +0000, Jag Raman wrote:
> 
> 
> > On Jan 25, 2022, at 5:32 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > On Wed, Jan 19, 2022 at 04:41:55PM -0500, Jagannathan Raman wrote:
> >> Allow hotplugging of PCI(e) devices to remote machine
> >> 
> >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> >> ---
> >> hw/remote/machine.c | 29 +++++++++++++++++++++++++++++
> >> 1 file changed, 29 insertions(+)
> > 
> > Why is this code necessary? I expected the default hotplug behavior to
> 
> I just discovered that TYPE_REMOTE_MACHINE wasn't setting up a hotplug
> handler for the root PCI bus.
> 
> Looks like, some of the machines don’t support hotplugging PCI devices. I see
> that the ‘pc’ machine does support hotplug, whereas ‘q35’ does not.

Hotplug is definitely possible with q35. I'm not familiar with the
hotplug code though so I don't know how exactly that works for q35.

> We didn’t check hotplug in multiprocess-qemu previously because it was limited
> to one device per process, and the use cases attached the devices via
> command line.
> 
> > pretty much handle this case - hotplugging device types that the bus
> > doesn't support should fail and unplug should already unparent/unrealize
> > the device.
> 
> OK, that makes sense. We don’t need to test the device type during
> plug and unplug.
> 
> Therefore, I don’t think we need a callback for the plug operation. We
> could set HotplugHandlerClass->unplug callback to the default
> qdev_simple_device_unplug_cb() callback.

Great!

Stefan
Jag Raman Jan. 26, 2022, 3:20 p.m. UTC | #4 
> On Jan 26, 2022, at 4:35 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> On Tue, Jan 25, 2022 at 06:12:48PM +0000, Jag Raman wrote:
>> 
>> 
>>> On Jan 25, 2022, at 5:32 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> 
>>> On Wed, Jan 19, 2022 at 04:41:55PM -0500, Jagannathan Raman wrote:
>>>> Allow hotplugging of PCI(e) devices to remote machine
>>>> 
>>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>>>> ---
>>>> hw/remote/machine.c | 29 +++++++++++++++++++++++++++++
>>>> 1 file changed, 29 insertions(+)
>>> 
>>> Why is this code necessary? I expected the default hotplug behavior to
>> 
>> I just discovered that TYPE_REMOTE_MACHINE wasn't setting up a hotplug
>> handler for the root PCI bus.
>> 
>> Looks like, some of the machines don’t support hotplugging PCI devices. I see
>> that the ‘pc’ machine does support hotplug, whereas ‘q35’ does not.
> 
> Hotplug is definitely possible with q35. I'm not familiar with the
> hotplug code though so I don't know how exactly that works for q35.

I was referring to the root PCI bus, other buses in Q35 probably support
hotplug. Please see error message below:

QEMU 6.2.50 monitor - type 'help' for more information
(qemu) device_add lsi53c895a,id=lsi2
Error: Bus 'pcie.0' does not support hotplugging

--
Jag

> 
>> We didn’t check hotplug in multiprocess-qemu previously because it was limited
>> to one device per process, and the use cases attached the devices via
>> command line.
>> 
>>> pretty much handle this case - hotplugging device types that the bus
>>> doesn't support should fail and unplug should already unparent/unrealize
>>> the device.
>> 
>> OK, that makes sense. We don’t need to test the device type during
>> plug and unplug.
>> 
>> Therefore, I don’t think we need a callback for the plug operation. We
>> could set HotplugHandlerClass->unplug callback to the default
>> qdev_simple_device_unplug_cb() callback.
> 
> Great!
> 
> Stefan
Stefan Hajnoczi Jan. 26, 2022, 3:43 p.m. UTC | #5 
On Wed, Jan 26, 2022 at 03:20:35PM +0000, Jag Raman wrote:
> 
> 
> > On Jan 26, 2022, at 4:35 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > 
> > On Tue, Jan 25, 2022 at 06:12:48PM +0000, Jag Raman wrote:
> >> 
> >> 
> >>> On Jan 25, 2022, at 5:32 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>> 
> >>> On Wed, Jan 19, 2022 at 04:41:55PM -0500, Jagannathan Raman wrote:
> >>>> Allow hotplugging of PCI(e) devices to remote machine
> >>>> 
> >>>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> >>>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> >>>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
> >>>> ---
> >>>> hw/remote/machine.c | 29 +++++++++++++++++++++++++++++
> >>>> 1 file changed, 29 insertions(+)
> >>> 
> >>> Why is this code necessary? I expected the default hotplug behavior to
> >> 
> >> I just discovered that TYPE_REMOTE_MACHINE wasn't setting up a hotplug
> >> handler for the root PCI bus.
> >> 
> >> Looks like, some of the machines don’t support hotplugging PCI devices. I see
> >> that the ‘pc’ machine does support hotplug, whereas ‘q35’ does not.
> > 
> > Hotplug is definitely possible with q35. I'm not familiar with the
> > hotplug code though so I don't know how exactly that works for q35.
> 
> I was referring to the root PCI bus, other buses in Q35 probably support
> hotplug. Please see error message below:
> 
> QEMU 6.2.50 monitor - type 'help' for more information
> (qemu) device_add lsi53c895a,id=lsi2
> Error: Bus 'pcie.0' does not support hotplugging

Yes, I think that's because it's PCIe and not PCI.

Stefan
diff mbox series

Patch

diff --git a/hw/remote/machine.c b/hw/remote/machine.c
index 952105eab5..220ff01aa9 100644
--- a/hw/remote/machine.c
+++ b/hw/remote/machine.c
@@ -54,14 +54,39 @@  static void remote_machine_init(MachineState *machine)
 
     pci_bus_irqs(pci_host->bus, remote_iohub_set_irq, remote_iohub_map_irq,
                  &s->iohub, REMOTE_IOHUB_NB_PIRQS);
+
+    qbus_set_hotplug_handler(BUS(pci_host->bus), OBJECT(s));
+}
+
+static void remote_machine_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        error_setg(errp, "Only allowing PCI hotplug");
+    }
+}
+
+static void remote_machine_unplug_cb(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
+{
+    if (!object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        error_setg(errp, "Only allowing PCI hot-unplug");
+        return;
+    }
+
+    qdev_unrealize(dev);
 }
 
 static void remote_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     mc->init = remote_machine_init;
     mc->desc = "Experimental remote machine";
+
+    hc->pre_plug = remote_machine_pre_plug_cb;
+    hc->unplug = remote_machine_unplug_cb;
 }
 
 static const TypeInfo remote_machine = {
@@ -69,6 +94,10 @@  static const TypeInfo remote_machine = {
     .parent = TYPE_MACHINE,
     .instance_size = sizeof(RemoteMachineState),
     .class_init = remote_machine_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void remote_machine_register_types(void)