Re: [PATCH 01/12] qemu: Drop KVM assignment

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

On 8/20/19 1:52 PM, Daniel Henrique Barboza wrote:
> On 8/20/19 11:30 AM, Michal Privoznik wrote:
> > KVM style of PCI devices assignment was dropped in kernel in
> > favor of vfio pci (see kernel commit v4.12-rc1~68^2~65). Since
> > vfio is around for quite some time now and is far superior
> > discourage people in using KVM style.
> >
> > Ideally, I'd make QEMU_CAPS_VFIO_PCI implicitly assumed but turns
> > out qemu-3.0.0 doesn't support vfio-pci device for RISC-V.
> >
> > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > ---


Tested-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>


After taking care of that commit message nit I've pointed out below:


Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>


> >   src/qemu/qemu_capabilities.c |  6 -----
> >   src/qemu/qemu_command.c      | 26 ++-------------------
> >   src/qemu/qemu_command.h      |  1 -
> >   src/qemu/qemu_driver.c       | 14 ++++--------
> >   src/qemu/qemu_hostdev.c      | 44 +++---------------------------------
> >   src/qemu/qemu_hostdev.h      |  1 -
> >   src/qemu/qemu_hotplug.c      | 20 ++--------------
> >   tests/domaincapstest.c       |  3 +--
> >   8 files changed, 12 insertions(+), 103 deletions(-)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index c9677315ab..73300128ea 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -5338,7 +5338,6 @@ static int
> >   virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps,
> > virDomainCapsDeviceHostdevPtr hostdev)
> >   {
> > -    bool supportsPassthroughKVM = 
> > qemuHostdevHostSupportsPassthroughLegacy();
> >       bool supportsPassthroughVFIO = 
> > qemuHostdevHostSupportsPassthroughVFIO();
> >         hostdev->supported = VIR_TRISTATE_BOOL_YES;
> > @@ -5374,11 +5373,6 @@ 
> > virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps,
> > VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO);
> >       }
> >   -    if (supportsPassthroughKVM) {
> > -        VIR_DOMAIN_CAPS_ENUM_SET(hostdev->pciBackend,
> > - VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT,
> > - VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM);
> > -    }
> >       return 0;
> >   }
> >   diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index e3f4ef624b..f7b5430db8 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4904,7 +4904,6 @@ char *
> >   qemuBuildPCIHostdevDevStr(const virDomainDef *def,
> >                             virDomainHostdevDefPtr dev,
> >                             unsigned int bootIndex, /* used iff 
> > dev->info->bootIndex == 0 */
> > -                          const char *configfd,
> >                             virQEMUCapsPtr qemuCaps)
> >   {
> >       virBuffer buf = VIR_BUFFER_INITIALIZER;
> > @@ -4913,16 +4912,11 @@ qemuBuildPCIHostdevDevStr(const virDomainDef 
> > *def,
> >         /* caller has to assign proper passthrough backend type */
> >       switch ((virDomainHostdevSubsysPCIBackendType)backend) {
> > -    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> > -        virBufferAddLit(&buf, "pci-assign");
> > -        if (configfd && *configfd)
> > -            virBufferAsprintf(&buf, ",configfd=%s", configfd);
> > -        break;
> > -
> >       case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO:
> >           virBufferAddLit(&buf, "vfio-pci");
> >           break;
> >   +    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> >       case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT:
> >       case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
> >       case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
> > @@ -5676,7 +5670,6 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> >                   }
> >               }
> >   -            char *configfd_name = NULL;
> >               unsigned int bootIndex = hostdev->info->bootIndex;
> >                 /* bootNet will be non-0 if boot order was set and no 
> > other
> > @@ -5686,27 +5679,12 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> >                   bootIndex = *bootHostdevNet;
> >                   *bootHostdevNet = 0;
> >               }
> > -            if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> > -                int configfd = qemuOpenPCIConfig(hostdev);
> > -
> > -                if (configfd >= 0) {
> > -                    if (virAsprintf(&configfd_name, "%d", configfd) 
> > < 0) {
> > -                        VIR_FORCE_CLOSE(configfd);
> > -                        return -1;
> > -                    }
> > -
> > -                    virCommandPassFD(cmd, configfd,
> > - VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> > -                }
> > -            }
> >                 if (qemuCommandAddExtDevice(cmd, hostdev->info) < 0)
> >                   return -1;
> >                 virCommandAddArg(cmd, "-device");
> > -            devstr = qemuBuildPCIHostdevDevStr(def, hostdev, bootIndex,
> > -                                               configfd_name, 
> > qemuCaps);
> > -            VIR_FREE(configfd_name);
> > +            devstr = qemuBuildPCIHostdevDevStr(def, hostdev, 
> > bootIndex, qemuCaps);
> >               if (!devstr)
> >                   return -1;
> >               virCommandAddArg(cmd, devstr);
> > diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> > index 7e2dc5a60a..e3983bedb2 100644
> > --- a/src/qemu/qemu_command.h
> > +++ b/src/qemu/qemu_command.h
> > @@ -148,7 +148,6 @@ char 
> > *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem,
> >   char *qemuBuildPCIHostdevDevStr(const virDomainDef *def,
> >                                   virDomainHostdevDefPtr dev,
> >                                   unsigned int bootIndex,
> > -                                const char *configfd,
> >                                   virQEMUCapsPtr qemuCaps);
> >     char *qemuBuildRNGDevStr(const virDomainDef *def,
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 11f97dbc65..eb373c14d7 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -13517,7 +13517,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
> >       int ret = -1;
> >       virNodeDeviceDefPtr def = NULL;
> >       char *xml = NULL;
> > -    bool legacy = qemuHostdevHostSupportsPassthroughLegacy();
> >       bool vfio = qemuHostdevHostSupportsPassthroughVFIO();
> >       virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> >   @@ -13544,8 +13543,6 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr 
> > dev,
> >       if (!driverName) {
> >           if (vfio) {
> >               driverName = "vfio";
> > -        } else if (legacy) {
> > -            driverName = "kvm";
> >           } else {
> >               virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> >                              _("neither VFIO nor KVM device 
> > assignment is "
>
> Right here comes the error message:
>
>             virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>                            _("neither VFIO nor KVM device assignment is "
>                              "currently supported on this system"));
>             goto cleanup;
>
> I think this message should refer only to VFIO in this case - there's
> already a KVM device assignment message down below.
>
>
> Thanks,
>
>
> DHB
>
>
> > @@ -13563,13 +13560,10 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr 
> > dev,
> >           }
> >           virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_VFIO);
> >       } else if (STREQ(driverName, "kvm")) {
> > -        if (!legacy) {
> > -            virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> > -                           _("KVM device assignment is currently not "
> > -                             "supported on this system"));
> > -            goto cleanup;
> > -        }
> > -        virPCIDeviceSetStubDriver(pci, VIR_PCI_STUB_DRIVER_KVM);
> > +        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
> > +                       _("KVM device assignment is currently not "
> > +                         "supported on this system"));
> > +        goto cleanup;
> >       } else {
> >           virReportError(VIR_ERR_INVALID_ARG,
> >                          _("unknown driver name '%s'"), driverName);
> > diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> > index 92b037e1ed..af41c32679 100644
> > --- a/src/qemu/qemu_hostdev.c
> > +++ b/src/qemu/qemu_hostdev.c
> > @@ -133,44 +133,11 @@ qemuHostdevHostSupportsPassthroughVFIO(void)
> >   }
> >     -#if HAVE_LINUX_KVM_H
> > -# include <linux/kvm.h>
> > -bool
> > -qemuHostdevHostSupportsPassthroughLegacy(void)
> > -{
> > -    int kvmfd = -1;
> > -    bool ret = false;
> > -
> > -    if ((kvmfd = open("/dev/kvm", O_RDONLY)) < 0)
> > -        goto cleanup;
> > -
> > -# ifdef KVM_CAP_IOMMU
> > -    if ((ioctl(kvmfd, KVM_CHECK_EXTENSION, KVM_CAP_IOMMU)) <= 0)
> > -        goto cleanup;
> > -
> > -    ret = true;
> > -# endif
> > -
> > - cleanup:
> > -    VIR_FORCE_CLOSE(kvmfd);
> > -
> > -    return ret;
> > -}
> > -#else
> > -bool
> > -qemuHostdevHostSupportsPassthroughLegacy(void)
> > -{
> > -    return false;
> > -}
> > -#endif
> > -
> > -
> >   static bool
> > qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDefPtr 
> > *hostdevs,
> >                                            size_t nhostdevs,
> >                                            virQEMUCapsPtr qemuCaps)
> >   {
> > -    bool supportsPassthroughKVM = 
> > qemuHostdevHostSupportsPassthroughLegacy();
> >       bool supportsPassthroughVFIO = 
> > qemuHostdevHostSupportsPassthroughVFIO();
> >       size_t i;
> >   @@ -189,8 +156,6 @@ 
> > qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDefPtr 
> > *hostdevs,
> >               if (supportsPassthroughVFIO &&
> >                   virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VFIO_PCI)) {
> >                   *backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO;
> > -            } else if (supportsPassthroughKVM) {
> > -                *backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM;
> >               } else {
> >                   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> >                                  _("host doesn't support passthrough 
> > of "
> > @@ -209,12 +174,9 @@ 
> > qemuHostdevPreparePCIDevicesCheckSupport(virDomainHostdevDefPtr 
> > *hostdevs,
> >               break;
> >             case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
> > -            if (!supportsPassthroughKVM) {
> > -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                               _("host doesn't support legacy PCI 
> > passthrough"));
> > -                return false;
> > -            }
> > -
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("host doesn't support legacy PCI 
> > passthrough"));
> > +            return false;
> >               break;
> >             case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
> > diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
> > index f6d76c1c2a..e99c204961 100644
> > --- a/src/qemu/qemu_hostdev.h
> > +++ b/src/qemu/qemu_hostdev.h
> > @@ -24,7 +24,6 @@
> >   #include "qemu_conf.h"
> >   #include "domain_conf.h"
> >   -bool qemuHostdevHostSupportsPassthroughLegacy(void);
> >   bool qemuHostdevHostSupportsPassthroughVFIO(void);
> >     int qemuHostdevUpdateActiveMediatedDevices(virQEMUDriverPtr driver,
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index d8be63b71c..63acb9c451 100644
> > --- a/src/qemu/qemu_hotplug.c
> > +++ b/src/qemu/qemu_hotplug.c
> > @@ -1465,8 +1465,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr 
> > driver,
> >       virDomainDeviceInfoPtr info = hostdev->info;
> >       int ret;
> >       char *devstr = NULL;
> > -    int configfd = -1;
> > -    char *configfd_name = NULL;
> >       bool releaseaddr = false;
> >       bool teardowncgroup = false;
> >       bool teardownlabel = false;
> > @@ -1547,13 +1545,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr 
> > driver,
> >       if (qemuDomainEnsurePCIAddress(vm, &dev, driver) < 0)
> >           goto error;
> >       releaseaddr = true;
> > -    if (backend != VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO) {
> > -        configfd = qemuOpenPCIConfig(hostdev);
> > -        if (configfd >= 0) {
> > -            if (virAsprintf(&configfd_name, "fd-%s", info->alias) < 0)
> > -                goto error;
> > -        }
> > -    }
> >         if (!virDomainObjIsActive(vm)) {
> >           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > @@ -1561,8 +1552,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr 
> > driver,
> >           goto error;
> >       }
> >   -    if (!(devstr = qemuBuildPCIHostdevDevStr(vm->def, hostdev, 0,
> > -                                             configfd_name, 
> > priv->qemuCaps)))
> > +    if (!(devstr = qemuBuildPCIHostdevDevStr(vm->def, hostdev, 0, 
> > priv->qemuCaps)))
> >           goto error;
> >         qemuDomainObjEnterMonitor(driver, vm);
> > @@ -1570,10 +1560,8 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr 
> > driver,
> >       if ((ret = qemuDomainAttachExtensionDevice(priv->mon, 
> > hostdev->info)) < 0)
> >           goto exit_monitor;
> >   -    if ((ret = qemuMonitorAddDeviceWithFd(priv->mon, devstr,
> > -                                          configfd, configfd_name)) 
> > < 0) {
> > +    if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0)
> > ignore_value(qemuDomainDetachExtensionDevice(priv->mon, hostdev->info));
> > -    }
> >      exit_monitor:
> >       if (qemuDomainObjExitMonitor(driver, vm) < 0)
> > @@ -1586,8 +1574,6 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr 
> > driver,
> >       vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
> >         VIR_FREE(devstr);
> > -    VIR_FREE(configfd_name);
> > -    VIR_FORCE_CLOSE(configfd);
> >         return 0;
> >   @@ -1607,8 +1593,6 @@ 
> > qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver,
> >       qemuHostdevReAttachPCIDevices(driver, vm->def->name, &hostdev, 1);
> >         VIR_FREE(devstr);
> > -    VIR_FREE(configfd_name);
> > -    VIR_FORCE_CLOSE(configfd);
> >      cleanup:
> >       return -1;
> > diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c
> > index 45ecfe61ac..77acefa6b0 100644
> > --- a/tests/domaincapstest.c
> > +++ b/tests/domaincapstest.c
> > @@ -111,8 +111,7 @@ fillQemuCaps(virDomainCapsPtr domCaps,
> >                                     cfg->nfirmwares) < 0)
> >           goto cleanup;
> >   -    /* The function above tries to query host's KVM & VFIO 
> > capabilities by
> > -     * calling qemuHostdevHostSupportsPassthroughLegacy() and
> > +    /* The function above tries to query host's VFIO capabilities by 
> > calling
> >        * qemuHostdevHostSupportsPassthroughVFIO() which, however, 
> > can't be
> >        * successfully mocked as they are not exposed as internal 
> > APIs. Therefore,
> >        * instead of mocking set the expected values here by hand. */

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