7:37 a.m.
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.
Since v1:
- adjusted the error msg in patch 1 according to reviewer
- rephrased the commit message for patch 2 to make it less confusing
Erik Skultety (2):
util: mdev: Improve the error msg on non-existent mdev prior to VM
start
qemu: hostdev: Fix the error on VM start 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
7:37 a.m.
New subject: [libvirt] [PATCH v2 1/2] util: mdev: Improve the error msg on non-existent mdev prior to VM start
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(a)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..27541cf34 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)) {
+ virReportError(VIR_ERR_DEVICE_MISSING,
+ _("mediated device '%s' not found"), uuidstr);
+ 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
7:48 a.m.
New subject: [libvirt] [PATCH v2 1/2] util: mdev: Improve the error msg on non-existent mdev prior to VM start
On Fri, Mar 16, 2018 at 13:37:53 +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(a)redhat.com>
---
src/util/virmdev.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
ACK
7:37 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: hostdev: Fix the error on VM start with an mdev when IOMMU is off
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 serious, since as of yet, libvirt mandates
mdevs to be pre-created prior to a domain's launch - if it is,
everything does work smoothly, because the parent device will ensure the
iommu groups we try to access exist. However, if the mdev didn't exist,
one would see the following error:
"unsupported configuration: Mediated host device assignment requires VFIO
support"
The error msg above is simply wrong and doesn't even reflect the IOMMU
reality, so after applying this patch one would rather hit the following
error:
"device not found: mediated device '<UUID>' not found"
Signed-off-by: Erik Skultety <eskultet(a)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
12:14 p.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: hostdev: Fix the error on VM start with an mdev when IOMMU is off
On Fri, Mar 16, 2018 at 13:37:54 +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
I'd drop the word IOMMU here.
presence is enough to successfully start a VM.
Luckily, this issue is not serious, since as of yet, libvirt mandates
mdevs to be pre-created prior to a domain's launch - if it is,
everything does work smoothly, because the parent device will ensure the
iommu groups we try to access exist. However, if the mdev didn't exist,
one would see the following error:
That is true only if the mdev does not exist (since it creates an
iommu/isolation group) and also no other iommu groups are present on the
host.
If there are any other iommu group then qemuHostdevHostSupportsPassthroughVFIO
will return true and you'll still get the proper error that you report
below.
"unsupported configuration: Mediated host device assignment
requires VFIO
support"
In that case this error would be wrong.
The error msg above is simply wrong and doesn't even reflect the IOMMU
reality, so after applying this patch one would rather hit the following
error:
"device not found: mediated device '<UUID>' not found"
This is okay as long as you point out that the above case would happen
only if no other iommu groups were present on the host.
Signed-off-by: Erik Skultety <eskultet(a)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) {
ACK with the reworded commit message
--
2.13.6
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:16 a.m.
New subject: [libvirt] [PATCH v2 2/2] qemu: hostdev: Fix the error on VM start with an mdev when IOMMU is off
On Fri, Mar 16, 2018 at 06:14:56PM +0100, Peter Krempa wrote:
On Fri, Mar 16, 2018 at 13:37:54 +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
I'd drop the word IOMMU here.
> presence is enough to successfully start a VM.
> Luckily, this issue is not serious, since as of yet, libvirt mandates
> mdevs to be pre-created prior to a domain's launch - if it is,
> everything does work smoothly, because the parent device will ensure the
> iommu groups we try to access exist. However, if the mdev didn't exist,
> one would see the following error:
That is true only if the mdev does not exist (since it creates an
iommu/isolation group) and also no other iommu groups are present on the
host.
If there are any other iommu group then qemuHostdevHostSupportsPassthroughVFIO
will return true and you'll still get the proper error that you report
below.
> "unsupported configuration: Mediated host device assignment requires VFIO
> support"
In that case this error would be wrong.
>
> The error msg above is simply wrong and doesn't even reflect the IOMMU
> reality, so after applying this patch one would rather hit the following
> error:
>
> "device not found: mediated device '<UUID>' not found"
This is okay as long as you point out that the above case would happen
only if no other iommu groups were present on the host.
>
> Signed-off-by: Erik Skultety <eskultet(a)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) {
ACK with the reworded commit message
Reworded with a description of the one specific case where the error message
would be wrong without the patch and pushed both patches, thanks for review.
Erik
2485
days inactive
2488
days old
5 comments
2 participants
participants (2)
-
Erik Skultety -
Peter Krempa