[libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests

[Andrea Bolognani]

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> +static int
> +qemuDomainAttachExtensionDevice(qemuMonitorPtr mon,
> +                                virDomainDeviceInfoPtr info)
> +{
> +    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
> +        return qemuDomainAttachZPCIDevice(mon, info);
> +
> +    return 0;

Same comment as with qemuBuildExtensionCommandLine().

[...]
> +static int
> +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
> +                                virDomainDeviceInfoPtr info)
> +{
> +    if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
> +        return qemuDomainDetachZPCIDevice(mon, info);
> +
> +    return 0;

Here, too.

[...]
> @@ -805,8 +869,13 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver,
>      if (qemuHotplugDiskSourceAttach(priv->mon, diskdata) < 0)
>          goto exit_monitor;
>  
> -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> +    if (qemuDomainAttachExtensionDevice(priv->mon, &disk->info) < 0)
> +        goto exit_monitor;
> +

Since the zpci device refers to the underlying device through the
target= option, does it make sense to attach the zpci device first
and the target device second? I would expect it to fail unless you
attach them the other way around...

[...]
> @@ -913,7 +982,15 @@ int qemuDomainAttachControllerDevice(virQEMUDriverPtr driver,
>          goto cleanup;
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> -    ret = qemuMonitorAddDevice(priv->mon, devstr);
> +
> +    if (qemuDomainAttachExtensionDevice(priv->mon, &controller->info) < 0) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }

I'm not sure this is entirely safe. Perhaps you should introduce an
exit_monitor label that ensures failure to exit the monitor is also
taken into account, and jump to that one instead?

[...]
> +    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
> +                                          configfd, configfd_name)) < 0)
> +        ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));

Parentheses around here, please.

[...]
> @@ -3236,8 +3347,17 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
>          goto cleanup;
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> -    if (qemuMonitorAddDevice(priv->mon, devstr) < 0)
> +
> +    if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO) {
> +        if (qemuDomainAttachExtensionDevice(priv->mon, &input->info) < 0)
> +            goto exit_monitor;
> +    }
> +
> +    if (qemuMonitorAddDevice(priv->mon, devstr) < 0) {
> +        if (input->bus == VIR_DOMAIN_INPUT_BUS_VIRTIO)
> +            ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &input->info));
>          goto exit_monitor;
> +    }

Shouldn't you rather check for the address type here? IIUC an input
device with BUS_VIRTIO could be virtio-ccw or virtio-mmio, for
example, and you don't want to try adding a zpci device in those
cases.

[...]
> @@ -5301,6 +5427,11 @@ int qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>          qemuDomainMarkDeviceForRemoval(vm, &detach->info);
>  
>      qemuDomainObjEnterMonitor(driver, vm);
> +    if (detach->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI &&
> +        qemuDomainDetachExtensionDevice(priv->mon, &detach->info)) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }
>      if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
>          ignore_value(qemuDomainObjExitMonitor(driver, vm));
>          goto cleanup;

This one too looks like it would benefit from an exit_monitor lable.

-- 
Andrea Bolognani / Red Hat / Virtualization