Re: [libvirt] [PATCH v3 06/10] qemu: Allow hotplug of vhost-scsi device

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(a)linux.vnet.ibm.com&gt; > --- > 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); >