[libvirt] [PATCH 0/2] Don't report a VFIO error for mdev when IOMMU is disabled
Unlike GPU assignment, mdev doesn't need the IOMMU to be enabled within kernel, since the physical parent device takes care of the isolation, i.e. there's an IOMMU group entry for each mdev created under sysfs, thus a VM can start successfully Erik Skultety (2): util:mdev: Improve the error msg on non-existent mdev prior to VM start qemu: hostdev: Don't error out on domain with an mdev when IOMMU is off src/qemu/qemu_hostdev.c | 7 ++++++- src/util/virmdev.c | 16 +++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) -- 2.13.6
What one currently gets is: failed to read '/sys/bus/mdev/devices/<UUID>/mdev_type/device_api': No such file or directory This indicates that something is missing within the device's sysfs tree which likely might be not be the case here because the device simply doesn't exist yet. So, when creating our internal mdev obj, let's check whether the device exists first prior to trying to verify the user-provided model within domain XML. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index e4816cf20..2e3769aa6 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) { virMediatedDevicePtr ret = NULL; virMediatedDevicePtr dev = NULL; + char *sysfspath = NULL; + + if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) + goto cleanup; + + if (!virFileExists(sysfspath)) { + virReportSystemError(errno, _("failed to read device '%s'"), + sysfspath); + goto cleanup; + } if (VIR_ALLOC(dev) < 0) - return NULL; - - if (!(dev->path = virMediatedDeviceGetSysfsPath(uuidstr))) goto cleanup; + VIR_STEAL_PTR(dev->path, sysfspath); + /* Check whether the user-provided model corresponds with the actually * supported mediated device's API. */ @@ -167,6 +176,7 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) VIR_STEAL_PTR(ret, dev); cleanup: + VIR_FREE(sysfspath); virMediatedDeviceFree(dev); return ret; } -- 2.13.6
missing space in summary On Fri, Mar 16, 2018 at 11:50:01 +0100, Erik Skultety wrote:
What one currently gets is: failed to read '/sys/bus/mdev/devices/<UUID>/mdev_type/device_api': No such file or directory
This indicates that something is missing within the device's sysfs tree which likely might be not be the case here because the device simply doesn't exist yet. So, when creating our internal mdev obj, let's check whether the device exists first prior to trying to verify the user-provided model within domain XML.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c index e4816cf20..2e3769aa6 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) { virMediatedDevicePtr ret = NULL; virMediatedDevicePtr dev = NULL; + char *sysfspath = NULL; + + if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) + goto cleanup; + + if (!virFileExists(sysfspath)) { + virReportSystemError(errno, _("failed to read device '%s'"), + sysfspath);
You did not try to read the device at this point. It merely does not exist.
On Fri, Mar 16, 2018 at 12:08:46PM +0100, Peter Krempa wrote:
missing space in summary
On Fri, Mar 16, 2018 at 11:50:01 +0100, Erik Skultety wrote:
What one currently gets is: failed to read '/sys/bus/mdev/devices/<UUID>/mdev_type/device_api': No such file or directory
This indicates that something is missing within the device's sysfs tree which likely might be not be the case here because the device simply doesn't exist yet. So, when creating our internal mdev obj, let's check whether the device exists first prior to trying to verify the user-provided model within domain XML.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c index e4816cf20..2e3769aa6 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) { virMediatedDevicePtr ret = NULL; virMediatedDevicePtr dev = NULL; + char *sysfspath = NULL; + + if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) + goto cleanup; + + if (!virFileExists(sysfspath)) { + virReportSystemError(errno, _("failed to read device '%s'"), + sysfspath);
You did not try to read the device at this point. It merely does not exist.
Would you like "device '%s' not found" better? Erik
On Fri, Mar 16, 2018 at 13:13:41 +0100, Erik Skultety wrote:
On Fri, Mar 16, 2018 at 12:08:46PM +0100, Peter Krempa wrote:
missing space in summary
On Fri, Mar 16, 2018 at 11:50:01 +0100, Erik Skultety wrote:
What one currently gets is: failed to read '/sys/bus/mdev/devices/<UUID>/mdev_type/device_api': No such file or directory
This indicates that something is missing within the device's sysfs tree which likely might be not be the case here because the device simply doesn't exist yet. So, when creating our internal mdev obj, let's check whether the device exists first prior to trying to verify the user-provided model within domain XML.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/util/virmdev.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/util/virmdev.c b/src/util/virmdev.c index e4816cf20..2e3769aa6 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -150,13 +150,22 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) { virMediatedDevicePtr ret = NULL; virMediatedDevicePtr dev = NULL; + char *sysfspath = NULL; + + if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) + goto cleanup; + + if (!virFileExists(sysfspath)) { + virReportSystemError(errno, _("failed to read device '%s'"), + sysfspath);
You did not try to read the device at this point. It merely does not exist.
Would you like "device '%s' not found" better?
How about: "mediated device '%s' not found", uuidstr ? I don't think that adding the full sysfs path is any helpful since it's generated from the uuid anyways.
Commit b4c2ac8d56 made a false assumption that IOMMU support necessary for an mdev device to be assigned to a VM. Unlike direct PCI assignment, IOMMU support is not needed for mediated devices, as the physical parent device provides the IOMMU isolation, therefore, simply checking for VFIO presence is enough to successfully start a VM. Luckily, this issue is not very serious, since as of yet, libvirt mandates the mdevs to be pre-created prior to a domain's launch, so this patch will merely change the error the end user is going to see. Previously: unsupported configuration: Mediated host device assignment requires VFIO support Now: failed to read device '/sys/bus/mdev/devices/<uuid>/': No such file or directory Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hostdev.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 73d26f4c6..afe445d4e 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -330,9 +330,14 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr driver, int nhostdevs) { virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - bool supportsVFIO = qemuHostdevHostSupportsPassthroughVFIO(); + bool supportsVFIO; size_t i; + /* Checking for VFIO only is fine with mdev, as IOMMU isolation is achieved + * by the physical parent device. + */ + supportsVFIO = virFileExists("/dev/vfio/vfio"); + for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdevs[i]->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) { -- 2.13.6
On Fri, Mar 16, 2018 at 11:50:02 +0100, Erik Skultety wrote:
Commit b4c2ac8d56 made a false assumption that IOMMU support necessary for an mdev device to be assigned to a VM. Unlike direct PCI assignment, IOMMU support is not needed for mediated devices, as the physical parent device provides the IOMMU isolation, therefore, simply checking for VFIO presence is enough to successfully start a VM. Luckily, this issue is not very serious, since as of yet, libvirt mandates the mdevs to be pre-created prior to a domain's launch, so this patch will merely change the error the end user is going to see.
Previously: unsupported configuration: Mediated host device assignment requires VFIO support
Now: failed to read device '/sys/bus/mdev/devices/<uuid>/': No such file or directory
I'm not sure I understood what you wanted to say in the commit message. You are not requiring the IOMMU to bepresent for the mediated device since isolation is inherent. This makes sense. You are then giving an example of error message what would occur if the iommu groups are not present on the host. As a 'fixed' result you provide an error message if the user did not create a MDEV prior to starting the VM. How is that relevant to this patch? The VM should start just fine in that case, shouldn't it?
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_hostdev.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 73d26f4c6..afe445d4e 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -330,9 +330,14 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr driver, int nhostdevs) { virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; - bool supportsVFIO = qemuHostdevHostSupportsPassthroughVFIO(); + bool supportsVFIO; size_t i;
+ /* Checking for VFIO only is fine with mdev, as IOMMU isolation is achieved + * by the physical parent device. + */ + supportsVFIO = virFileExists("/dev/vfio/vfio"); + for (i = 0; i < nhostdevs; i++) { if (hostdevs[i]->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS && hostdevs[i]->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV) { -- 2.13.6
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 16, 2018 at 12:07:50PM +0100, Peter Krempa wrote:
On Fri, Mar 16, 2018 at 11:50:02 +0100, Erik Skultety wrote:
Commit b4c2ac8d56 made a false assumption that IOMMU support necessary for an mdev device to be assigned to a VM. Unlike direct PCI assignment, IOMMU support is not needed for mediated devices, as the physical parent device provides the IOMMU isolation, therefore, simply checking for VFIO presence is enough to successfully start a VM. Luckily, this issue is not very serious, since as of yet, libvirt mandates the mdevs to be pre-created prior to a domain's launch, so this patch will merely change the error the end user is going to see.
Previously: unsupported configuration: Mediated host device assignment requires VFIO support
Now: failed to read device '/sys/bus/mdev/devices/<uuid>/': No such file or directory
I'm not sure I understood what you wanted to say in the commit message.
You are not requiring the IOMMU to bepresent for the mediated device since isolation is inherent. This makes sense.
You are then giving an example of error message what would occur if the iommu groups are not present on the host.
I should have been more clear about that. So, if you're referencing a non-existent mdev within the domain XML, the error you get indicates something that is not true, but you're right in what you deduced that you won't come across this weird error if the device you're referencing already exists. Therefore I mentioned in the patch that the issue isn't serious since everything works if used correctly, only when you don't you'd experience some odd messages, that's all, I'll rephrase the commit message. Erik
participants (2)
-
Erik Skultety -
Peter Krempa