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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@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?
> 
> (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);
>>>
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]