Re: [libvirt] [PATCH v5 11/13] qemu: Add hotpluging support for PCI devices on S390 guests
在 2018/9/11 下午11:21, 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().
yes.
[...]
> +static int
> +qemuDomainDetachExtensionDevice(qemuMonitorPtr mon,
> + virDomainDeviceInfoPtr info)
> +{
> + if (!virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci))
> + return qemuDomainDetachZPCIDevice(mon, info);
> +
> + return 0;
Here, too.
I posted my idea in previous mail. Let's see that.
[...]
> @@ -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...
This attaching order is right. Qemu requires
that attach zpci first and
target device second.
In Qemu, zpci is created firstly and ready to build connection with
target device, and after
target device is plugged, there's a mapping built for zpci and target
device. If the order is
reversed, there would be error.
[...]
> @@ -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?
I see there're a lot of code
ignoring monitor exit failure. Of course,
taking it into account
is proper. Do you strongly suggest to add that?
[...]
> + if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
> + configfd, configfd_name)) < 0)
> + ignore_value(qemuDomainDetachExtensionDevice(priv->mon,
hostdev->info));
Parentheses around here, please.
ok.
[...]
> @@ -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.
Yes, you're right. As my idea regarding using switch in attach***() and
detach***(),
we could move the check there.
[...]
> @@ -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.
Exactly. I will reorganise the code.
--
Yi Min