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

Re: [libvirt] [RFC PATCH v2] qemu: Add suport for pci-assign.configfd option



On Thu, May 20, 2010 at 10:02:10AM -0400, Alex Williamson wrote:
> This allows libvirt to open the PCI device sysfs config file prior
> to dropping privileges so qemu can access the full config space.
> Without this, a de-privileged qemu can only access the first 64
> bytes of config space.
> 
> Signed-off-by: Alex Williamson <alex williamson redhat com>
> ---
> 
> v2:
>  - fix qemuargs, two separate args (thanks Chris)
>  - don't hardcode pci segment
>  - error message of open() fail
> 
>  src/qemu/qemu_conf.c   |   89 +++++++++++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_conf.h   |    8 ++++
>  src/qemu/qemu_driver.c |    2 +
>  3 files changed, 95 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index d7bc798..24c360e 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1345,6 +1345,48 @@ fail:
>      return -1;
>  }
>  
> +void qemudParsePCIDeviceStrs(const char *qemu, unsigned long long *flags)

Minor point, that this function can be declared static.

> +{
> +    const char *const qemuarg[] = { qemu, "-device", "pci-assign,?", NULL };
> +    const char *const qemuenv[] = { "LC_ALL=C", NULL };
> +    pid_t child;
> +    int status;
> +    int newstderr = -1;
> +
> +    if (virExec(qemuarg, qemuenv, NULL,
> +                &child, -1, NULL, &newstderr, VIR_EXEC_CLEAR_CAPS) < 0)
> +        return;
> +
> +    char *pciassign = NULL;
> +    enum { MAX_PCI_OUTPUT_SIZE = 1024*4 };
> +    int len = virFileReadLimFD(newstderr, MAX_PCI_OUTPUT_SIZE, &pciassign);
> +    if (len < 0) {
> +        virReportSystemError(errno,
> +                             _("Unable to read %s pci-assign device output"),
> +                             qemu);
> +        goto cleanup;
> +    }
> +
> +    if (strstr(pciassign, "pci-assign.configfd"))
> +        *flags |= QEMUD_CMD_FLAG_PCI_CONFIGFD;
> +
> +cleanup:
> +    VIR_FREE(pciassign);
> +    close(newstderr);
> +rewait:
> +    if (waitpid(child, &status, 0) != child) {
> +        if (errno == EINTR)
> +            goto rewait;
> +
> +        VIR_ERROR(_("Unexpected exit status from qemu %d pid %lu"),
> +                  WEXITSTATUS(status), (unsigned long)child);
> +    }
> +    if (WEXITSTATUS(status) != 0) {
> +        VIR_WARN("Unexpected exit status '%d', qemu probably failed",
> +                 WEXITSTATUS(status));
> +    }
> +}
> +
>  int qemudExtractVersionInfo(const char *qemu,
>                              unsigned int *retversion,
>                              unsigned long long *retflags) {
> @@ -1378,6 +1420,9 @@ int qemudExtractVersionInfo(const char *qemu,
>                            &version, &is_kvm, &kvm_version) == -1)
>          goto cleanup2;
>  
> +    if (flags & QEMUD_CMD_FLAG_DEVICE)
> +        qemudParsePCIDeviceStrs(qemu, &flags);
> +
>      if (retversion)
>          *retversion = version;
>      if (retflags)
> @@ -2887,8 +2932,33 @@ error:
>  }
>  
>  
> +int
> +qemudOpenPCIConfig(virDomainHostdevDefPtr dev)
> +{
> +    char *path = NULL;
> +    int configfd = -1;
> +
> +    if (virAsprintf(&path, "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/config",
> +                    dev->source.subsys.u.pci.domain,
> +                    dev->source.subsys.u.pci.bus,
> +                    dev->source.subsys.u.pci.slot,
> +                    dev->source.subsys.u.pci.function) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    configfd = open(path, O_RDWR, 0);
> +
> +    if (configfd < 0)
> +        virReportSystemError(errno, _("Failed opening %s"), path);
> +
> +    VIR_FREE(path);
> +
> +    return configfd;
> +}
> +
>  char *
> -qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev)
> +qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, int configfd)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>  
> @@ -2898,6 +2968,8 @@ qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev)
>                        dev->source.subsys.u.pci.slot,
>                        dev->source.subsys.u.pci.function);
>      virBufferVSprintf(&buf, ",id=%s", dev->info.alias);
> +    if (configfd >= 0)
> +        virBufferVSprintf(&buf, ",configfd=%d", configfd);

For hotplug to work, it will actually be neccessary to make this take
a const char * string and use %s. For CLI args the string will just be
the FD number formatted normally, for hotplug the string will be a 
symbolic FD name.

> @@ -4600,8 +4672,21 @@ int qemudBuildCommandLine(virConnectPtr conn,
>          if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
>              hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI) {
>              if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) {
> +                int configfd = -1;

const char configfdstr[20];

> +                if (qemuCmdFlags & QEMUD_CMD_FLAG_PCI_CONFIGFD) {
> +                    configfd = qemudOpenPCIConfig(hostdev);
> +
> +                    if (configfd >= 0) {

   snprintf(configfdstr, sizeof(configfdstr), "%d", configfd);

> +                        if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) {
> +                            close(configfd);
> +                            goto no_memory;
> +                        }
> +
> +                        (*vmfds)[(*nvmfds)++] = configfd;
> +                    }
> +                }
>                  ADD_ARG_LIT("-device");
> -                if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev)))
> +                if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd)))

And pass the configfdstr here.

>                      goto error;
>                  ADD_ARG(devstr);
>              } else if (qemuCmdFlags & QEMUD_CMD_FLAG_PCIDEVICE) {
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index a101e47..64fab60 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -88,6 +88,7 @@ enum qemud_cmd_flags {
>      QEMUD_CMD_FLAG_NO_HPET       = (1LL << 33), /* -no-hpet flag is supported */
>      QEMUD_CMD_FLAG_NO_KVM_PIT    = (1LL << 34), /* -no-kvm-pit-reinjection supported */
>      QEMUD_CMD_FLAG_TDF           = (1LL << 35), /* -tdf flag (user-mode pit catchup) */
> +    QEMUD_CMD_FLAG_PCI_CONFIGFD  = (1LL << 36), /* pci-assign.configfd */
>  };
>  
>  /* Main driver state */
> @@ -190,6 +191,9 @@ int         qemudParseHelpStr           (const char *qemu,
>                                           unsigned int *is_kvm,
>                                           unsigned int *kvm_version);
>  
> +void        qemudParsePCIDeviceStrs     (const char *qemu,
> +                                         unsigned long long *qemuCmdFlags);
> +
>  int         qemudBuildCommandLine       (virConnectPtr conn,
>                                           struct qemud_driver *driver,
>                                           virDomainDefPtr def,
> @@ -242,7 +246,9 @@ char * qemuBuildSoundDevStr(virDomainSoundDefPtr sound);
>  /* Legacy, pre device support */
>  char * qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev);
>  /* Current, best practice */
> -char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev);
> +char * qemuBuildPCIHostdevDevStr(virDomainHostdevDefPtr dev, int configfd);
> +
> +int qemudOpenPCIConfig(virDomainHostdevDefPtr dev);
>  
>  /* Current, best practice */
>  char * qemuBuildChrChardevStr(virDomainChrDefPtr dev);
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index dd5bd24..219a973 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7567,7 +7567,7 @@ static int qemudDomainAttachHostPciDevice(struct qemud_driver *driver,
>          if (qemuDomainPCIAddressEnsureAddr(priv->pciaddrs, &hostdev->info) < 0)
>              goto error;
>  
> -        if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev)))
> +        if (!(devstr = qemuBuildPCIHostdevDevStr(hostdev, -1)))
>              goto error;
>      }


This is the only thing still needing work. On the libvirt side there is a
function qemuMonitorSendFileHandle() that can be used to give QEMU the
pre-opened file handle. You the 'fdname' parameter is what's passed into
the qemuBuildPCIHostdevDevStr() earlier as  the named file handle.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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