4:02 p.m.
On 11/14/2016 09:08 AM, Eric Farman wrote:
On 11/11/2016 04:47 PM, John Ferlan wrote:
>
> 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>
>> ---
>> 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?
(jumping out of order in my replies... You brought this up earlier, but
there's less to digest in this one...)
What's visible in the guest is one or more /dev/sdX devices and their
/dev/sgX counterparts, just as there would be if using virtio-scsi. The
difference is that there'd be one <hostdev> tag for each disk for
virtio, but here a single <hostdev> can contain many disks within it.
Ah - OK. I hadn't got around to "seeing" this working on my test system
yet (it's still out of commission). Your explanation makes sense.
Now, the question of why the check has to be made twice? Er, well,
uh... I don't have an answer for that.
yeah well I do this too... It's easy to get lost in what you've done or
haven't done
John
>
>> +
>> + 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.
Okay.
>
> 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"
I see that I started this from the net code that has both tapfd and
vhostfd, but calls Monitor via qemuMonitorAddNetdev instead of how
configfd handles things. So I presumed that the string was meant to be
more descriptive, than following a particular naming convention. Will
change with suggestions below.
- Eric
>
>> + 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);
>>