Re: [PATCH v2 4/5] qemu: Allow hotplug of vhost-scsi device

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

 




On 09/06/2016 08:58 AM, 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.
> 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.)
> 
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
> ---
>  src/conf/domain_conf.c  |   6 ++
>  src/qemu/qemu_hotplug.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 154 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 45b643b..123bbcb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13720,6 +13720,12 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
>              return virDomainHostdevMatchSubsysSCSIiSCSI(a, b);
>          else
>              return virDomainHostdevMatchSubsysSCSIHost(a, b);
> +    case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> +        if (a->source.subsys.u.host.protocol !=
> +            b->source.subsys.u.host.protocol)
> +            return 0;
> +        if (STREQ(a->source.subsys.u.host.wwpn, b->source.subsys.u.host.wwpn))
> +            return 1;
>      }
>      return 0;
>  }

This hunk seems to be out of place in this patch...  Although I assume
this is because of the remove device searches.

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e9140a9..5f8d411 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2165,6 +2165,119 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
>      goto cleanup;
>  }
>  
> +static int
> +qemuDomainAttachHostSCSIHostDevice(virQEMUDriverPtr driver,
> +                                   virDomainObjPtr vm,
> +                                   virDomainHostdevDefPtr hostdev)
> +{
> +    int ret = -1;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainCCWAddressSetPtr ccwaddrs = NULL;
> +    char *vhostfdName = NULL;
> +    int vhostfd = -1;
> +    size_t vhostfdSize = 1;
> +    char *devstr = NULL;
> +    bool teardowncgroup = false;
> +    bool teardownlabel = false;
> +    bool releaseaddr = false;
> +
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {

Here again the capabilities conundrum.

> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("SCSI passthrough is not supported by this version of qemu"));
> +        goto cleanup;
> +    }
> +
> +    if (qemuSetupHostdevCgroup(vm, hostdev) < 0)
> +        goto cleanup;

Oh, um, didn't I comment on this in patch 1 - there's no
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST in qemuSetupHostdevCgroup


> +    teardowncgroup = true;
> +
> +    if (virSecurityManagerSetHostdevLabel(driver->securityManager,
> +                                          vm->def, hostdev, NULL) < 0)
> +        goto cleanup;
> +    teardownlabel = true;

Likewise there's really not much going on...

> +
> +    if (virSCSIOpenVhost(&vhostfd, &vhostfdSize) < 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)

'info' is already a virDomainDeviceInfoPtr (unlike where you
cut-n-paste'd from), so no need for the '&' here (besides it induces a
build failure).

> +            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;
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +
> +    if (hostdev->source.subsys.u.host.protocol == VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_VHOST) {

Long line > 80 cols

Ironically we do nothing if the protocol isn't VHOST...

> +        if (!(devstr = qemuBuildHostHostdevDevStr(vm->def,
> +                                                  hostdev,
> +                                                  priv->qemuCaps,
> +                                                  vhostfdName,
> +                                                  vhostfdSize)))

This can be done outside holding the monitor lock

> +            goto exit_monitor;
> +
> +        if ((ret = qemuMonitorAddDeviceWithFd(priv->mon,
> +                                              devstr,
> +                                              vhostfd,
> +                                              vhostfdName)) < 0) {
> +            goto exit_monitor;
> +        }
> +    }
> +
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;
> +
> +    virDomainAuditHostdev(vm, hostdev, "attach", true);

s/true/(ret == 0)/

Since we've set up the addresses above - the auditing code should print
more than "?"

Here should be a "if (ret < 0) goto cleanup;" (on two lines).

> +
> +    if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
> +        goto cleanup;
> +
> +    vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;

Other code will increase the size of hostdevs before the monitor calls,
then just insert.
> +
> +    virDomainCCWAddressSetFree(ccwaddrs);
> +
> +    VIR_FORCE_CLOSE(vhostfd);

This I would think would only be done on *failure* to AddDeviceWithFd,
but I see you're following qemuDomainAttachHostPCIDevice

> +    VIR_FREE(vhostfdName);
> +    VIR_FREE(devstr);
> +    return 0;
> +
> + exit_monitor:
> +    ignore_value(qemuDomainObjExitMonitor(driver, vm)); 
> +
> + cleanup:
> +    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);
> +
> +    VIR_FORCE_CLOSE(vhostfd);
> +    VIR_FREE(vhostfdName);
> +    VIR_FREE(devstr);
> +    return ret;
> +}
> +
>  
>  int
>  qemuDomainAttachHostDevice(virConnectPtr conn,
> @@ -2198,6 +2311,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"),
> @@ -3960,6 +4078,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)
> @@ -3981,6 +4124,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"),
> @@ -4058,6 +4204,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,

Ah - and this is where that pesky virDomainHostdevFind causes the need
for the domain_conf.c code above...

>              }
>              break;
>          }
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:

But we need an error here otherwise we get a generic failed with some
error message.

John
> +            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]