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@linux.vnet.ibm.com> --- 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. In particular, the configfd_name is a combination of "fd-%s" where %s is the alias (e.g. perhaps "fd-hostdev4"; whereas, this would seem to generate "vhostfd-hostdev4"
+ 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);