On 11/08/2016 01:26 PM, Eric Farman wrote:
Adjust the device string that is built for vhost-scsi devices so that it
can be invoked from hotplug.
>From the QEMU command line, the file descriptors are expect to be numeric only.
s/>//
Looks like a cut-n-paste carryover
However, for hotplug, the file descriptors are expected to begin with at least
one alphabetic character else this error occurs:
# virsh attach-device guest_0001 ~/vhost.xml
error: Failed to attach device from /root/vhost.xml
error: internal error: unable to execute QEMU command 'getfd':
Parameter 'fdname' expects a name not starting with a digit
We also close the file descriptor in this case, so that shutting down the
guest cleans up the host cgroup entries and allows future guests to use
vhost-scsi devices. (Otherwise the guest will silently end.)
See you're following the lead of qemuDomainAttachHostPCIDevice for the
'configfd' processing
Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
---
src/qemu/qemu_hotplug.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 158 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 2d6b086..d503212 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -2425,6 +2425,120 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
goto cleanup;
}
+static int
+qemuDomainAttachHostSCSIHostDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr hostdev)
+{
+ int ret = -1;
+ qemuDomainObjPrivatePtr priv = vm->privateData;
virErrorPtr orig_err;
+ virDomainCCWAddressSetPtr ccwaddrs = NULL;
+ char *vhostfdName = NULL;
+ int vhostfd = -1;
+ char *devstr = NULL;
+ bool teardowncgroup = false;
+ bool teardownlabel = false;
+ bool releaseaddr = false;
+
+ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("SCSI passthrough is not supported by this version of qemu"));
+ goto cleanup;
+ }
Still not clear why SCSI_GENERIC is required - what is the guest device?
+
+ if (qemuHostdevPrepareHostDevices(driver, vm->def->name, &hostdev, 1) < 0) {
+ virDomainHostdevSubsysHostPtr hostsrc = &hostdev->source.subsys.u.host;
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to prepare scsi_host hostdev: %s"),
+ hostsrc->wwpn);
+ goto cleanup;
+ }
+
+ if (qemuSetupHostdevCgroup(vm, hostdev) < 0)
+ goto cleanup;
+ teardowncgroup = true;
+
+ if (virSecurityManagerSetHostdevLabel(driver->securityManager,
+ vm->def, hostdev, NULL) < 0)
+ goto cleanup;
+ teardownlabel = true;
+
+ if (virHostOpenVhostSCSI(&vhostfd) < 0)
+ goto cleanup;
+
+ if (virAsprintf(&vhostfdName, "vhostfd-%d", vhostfd) < 0)
+ goto cleanup;
+
+ if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
+ if (qemuDomainMachineIsS390CCW(vm->def) &&
+ virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
+ hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
+ }
+
+ if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
+ hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
+ if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, hostdev->info) < 0)
Here we are doing the PCI address thing, but I don't see patch7
addressing that... That is - no address is defined on the command line
(as seen the the patch9 .args file)
+ goto cleanup;
+ } else if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
+ if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
+ goto cleanup;
+ if (virDomainCCWAddressAssign(hostdev->info, ccwaddrs,
+ !hostdev->info->addr.ccw.assigned) < 0)
+ goto cleanup;
+ }
+ releaseaddr = true;
+
+ if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
+ goto cleanup;
+
+ if (!(devstr = qemuBuildHostHostdevDevStr(vm->def,
+ hostdev,
+ priv->qemuCaps,
+ vhostfdName)))
If I look at the only other caller of qemuMonitorAddDeviceWithFd I note
that it does this slightly differently... You may want to check that
out as they should be consistent.
+ goto cleanup;
+
+ qemuDomainObjEnterMonitor(driver, vm);
+
+ ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr, vhostfd, vhostfdName);
if (qemuMonitorAddDeviceWithFd(...) < 0)
goto exit_monitor;
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+ ret = -1;
+ goto cleanup;
s/cleanup/audit [1]
The ret = -1 would be pointless and we should just able to alter the
subsequent lines a bit... [1] (see redirdevs for my example)
+ }
+
+ virDomainAuditHostdev(vm, hostdev, "attach", (ret == 0));
+
+ if (ret < 0)
+ goto cleanup;
+
+ if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
+ goto cleanup;
Hmmm... so if this fails now, we have the device added to the domain,
but we're failing so we should remove the device... See the problem?
That's why other functions do their REALLOC_N before monitor
interactions... then just add it (as follows) - so move the above 2
lines before the EnterMonitor
+
+ vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
+
[1]
vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
ret = 0;
audit:
virDomainAuditHostdev(vm, hostdev, "attach", (ret == 0));
cleanup:
+ virDomainCCWAddressSetFree(ccwaddrs);
+
+ VIR_FORCE_CLOSE(vhostfd);
+ VIR_FREE(vhostfdName);
+ VIR_FREE(devstr);
+ return 0;
+
+ cleanup:
s/cleanup/exit_monitor
orig_err = virSaveLastError();
+ if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
+ VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail");
+ if (teardownlabel &&
+ virSecurityManagerRestoreHostdevLabel(driver->securityManager,
+ vm->def, hostdev, NULL) < 0)
+ VIR_WARN("Unable to restore host device labelling on hotplug fail");
+ if (releaseaddr)
+ qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
+
if (orig_err) {
virSetError(orig_err);
virFreeError(orig_err);
}
goto audit;
Meaning the next 4 aren't necessary.
John
+ VIR_FORCE_CLOSE(vhostfd);
+ VIR_FREE(vhostfdName);
+ VIR_FREE(devstr);
+ return ret;
+}
+
int
qemuDomainAttachHostDevice(virConnectPtr conn,
@@ -2458,6 +2572,11 @@ qemuDomainAttachHostDevice(virConnectPtr conn,
goto error;
break;
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
+ if (qemuDomainAttachHostSCSIHostDevice(driver, vm, hostdev) < 0)
+ goto error;
+ break;
+
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hostdev subsys type '%s' not supported"),
@@ -3549,6 +3668,14 @@ qemuDomainRemoveSCSIHostDevice(virQEMUDriverPtr driver,
qemuHostdevReAttachSCSIDevices(driver, vm->def->name, &hostdev, 1);
}
+static void
+qemuDomainRemoveHostSCSIHostDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr hostdev)
+{
+ qemuHostdevReAttachHostDevices(driver, vm->def->name, &hostdev, 1);
+}
+
static int
qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
@@ -3627,6 +3754,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
qemuDomainRemoveSCSIHostDevice(driver, vm, hostdev);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
+ qemuDomainRemoveHostSCSIHostDevice(driver, vm, hostdev);
break;
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST:
break;
@@ -4477,6 +4605,31 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver,
}
static int
+qemuDomainDetachHostSCSIHostDevice(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainHostdevDefPtr detach)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ int ret = -1;
+
+ if (!detach->info->alias) {
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ "%s", _("device cannot be detached without a device alias"));
+ return -1;
+ }
+
+ qemuDomainMarkDeviceForRemoval(vm, detach->info);
+
+ qemuDomainObjEnterMonitor(driver, vm);
+ ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0)
+ return -1;
+
+ return ret;
+}
+
+static int
qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr detach)
@@ -4498,6 +4651,9 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach);
break;
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
+ ret = qemuDomainDetachHostSCSIHostDevice(driver, vm, detach);
+ break;
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hostdev subsys type '%s' not supported"),
@@ -4575,6 +4731,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
}
break;
}
+ case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
+ break;
default:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected hostdev type %d"), subsys->type);