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

[Jag Raman]

> 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
>>