[libvirt] [PATCH 0/2] Don't perform the vfio-ap mdev validation in post parse
Move the respective bits to QEMU validation code instead. Erik Skultety (2): qemu: Extract MDEV VFIO PCI validation code into a separate helper conf: Move VFIO AP validation from post parse to QEMU validation code src/conf/domain_conf.c | 28 ------------------- src/qemu/qemu_domain.c | 61 +++++++++++++++++++++++++++++++++++++----- 2 files changed, 55 insertions(+), 34 deletions(-) -- 2.19.1
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } - if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("<hostdev> attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, } +static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + + switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + break; + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + break; + case VIR_MDEV_MODEL_TYPE_LAST: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def, -- 2.19.1
On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety <eskultet@redhat.com> wrote:
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; }
- if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("<hostdev> attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; }
- if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, }
+static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + + switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + break; + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + break; + case VIR_MDEV_MODEL_TYPE_LAST: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + return -1; + }
default case is missing? Otherwise looking good.
+ + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def, -- 2.19.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Kind regards / Beste Grüße Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Nov 12, 2018 at 03:03:34PM +0100, Marc Hartmayer wrote:
On Mon, Nov 12, 2018 at 01:14 PM +0100, Erik Skultety <eskultet@redhat.com> wrote:
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; }
- if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("<hostdev> attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; }
- if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, }
+static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + + switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + break; + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + break; + case VIR_MDEV_MODEL_TYPE_LAST: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + return -1; + }
default case is missing? Otherwise looking good.
Yeah, it could only happen due to memory corruption, but you're right we should stay both consistent and safe, consider it added. Thanks, Erik
On 11/12/18 1:14 PM, Erik Skultety wrote:
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; }
- if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("<hostdev> attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; }
- if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, }
+static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + Syntax-check does not like the above blank line... :-)
+ switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + break; + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + break; + case VIR_MDEV_MODEL_TYPE_LAST: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def,
Besides Marc question regard the default switch case Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Nov 12, 2018 at 04:28:09PM +0100, Boris Fiuczynski wrote:
On 11/12/18 1:14 PM, Erik Skultety wrote:
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } - if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("<hostdev> attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, } +static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + Syntax-check does not like the above blank line... :-)
Wow, good catch, quite interesting though, because I always build my patches on a bunch of different distros (a reasonable subset of those in our CI) and none of them reported this. What's your setup if may ask?
+ switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + break; + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + break; + case VIR_MDEV_MODEL_TYPE_LAST: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def,
Besides Marc question regard the default switch case Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Thanks, Erik
On 11/13/18 9:27 AM, Erik Skultety wrote:
On Mon, Nov 12, 2018 at 04:28:09PM +0100, Boris Fiuczynski wrote:
On 11/12/18 1:14 PM, Erik Skultety wrote:
Since we'll need to validate other models apart from VFIO PCI too, having a helper for each model should keep the code base cleaner.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/qemu/qemu_domain.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e25afdad6b..17d207513d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4564,11 +4564,11 @@ qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net) static int -qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, - const virDomainDef *def, - virQEMUCapsPtr qemuCaps) +qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) { - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ABSENT) + if (dev->display == VIR_TRISTATE_SWITCH_ABSENT) return 0; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_PCI_DISPLAY)) { @@ -4578,7 +4578,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } - if (mdevsrc->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { + if (dev->model != VIR_MDEV_MODEL_TYPE_VFIO_PCI) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("<hostdev> attribute 'display' is only supported" " with model='vfio-pci'")); @@ -4586,7 +4586,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, return -1; } - if (mdevsrc->display == VIR_TRISTATE_SWITCH_ON) { + if (dev->display == VIR_TRISTATE_SWITCH_ON) { if (def->ngraphics == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("graphics device is needed for attribute value " @@ -4599,6 +4599,29 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, } +static int +qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, + const virDomainDef *def, + virQEMUCapsPtr qemuCaps) +{ + Syntax-check does not like the above blank line... :-)
Wow, good catch, quite interesting though, because I always build my patches on a bunch of different distros (a reasonable subset of those in our CI) and none of them reported this. What's your setup if may ask? Fedora 28 on s390 running in a VPATH build make check followed by make syntax-check
+ switch ((virMediatedDeviceModelType) mdevsrc->model) { + case VIR_MDEV_MODEL_TYPE_VFIO_PCI: + return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); + case VIR_MDEV_MODEL_TYPE_VFIO_AP: + break; + case VIR_MDEV_MODEL_TYPE_VFIO_CCW: + break; + case VIR_MDEV_MODEL_TYPE_LAST: + virReportEnumRangeError(virMediatedDeviceModelType, + mdevsrc->model); + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefValidateHostdev(const virDomainHostdevDef *hostdev, const virDomainDef *def,
Besides Marc question regard the default switch case Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Thanks, Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Even though commit 11708641 claims that a domain is allowed have a single VFIO AP hostdev only, this is a limitation posed by the platform vendor on purely virtual devices. Generally, post parse should only be used to populate some default values if missing or at least fail gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL). This patch more of an attempt to follow the guidelines for adding new features rather than a precaution measure, since even if vfio-ap supported multiple devices, one would have to downgrade libvirt for a machine to vanish from the list or in terms of future device migration to slightly older libvirt, there would be most probably a driver mismatch that would render the migration impossible anyway. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 28 ---------------------------- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 237540bccc..e8e0adc819 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def) } -static int -virDomainDefPostParseHostdev(virDomainDefPtr def) -{ - size_t i; - bool vfioap_found = false; - - /* verify settings of hostdevs vfio-ap */ - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - - if (virHostdevIsMdevDevice(hostdev) && - hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { - if (vfioap_found) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only one hostdev of model vfio-ap is " - "supported")); - return -1; - } - vfioap_found = true; - } - } - return 0; -} - - /** * virDomainDriveAddressIsUsedByDisk: * @def: domain definition containing the disks to check @@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def, virDomainDefPostParseGraphics(def); - if (virDomainDefPostParseHostdev(def) < 0) - return -1; - if (virDomainDefPostParseCPU(def) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 17d207513d..90253ae867 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, } +static int +qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def) +{ + size_t i; + bool vfioap_found = false; + + /* currently, VFIO-AP is restricted to a single device only */ + for (i = 0; i < def->nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = def->hostdevs[i]; + + if (virHostdevIsMdevDevice(hostdev) && + hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { + if (vfioap_found) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only one hostdev of model vfio-ap is " + "supported")); + return -1; + } + vfioap_found = true; + } + } + + return 0; +} + + static int qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, const virDomainDef *def, @@ -4609,7 +4635,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, case VIR_MDEV_MODEL_TYPE_VFIO_PCI: return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); case VIR_MDEV_MODEL_TYPE_VFIO_AP: - break; + return qemuDomainMdevDefVFIOAPValidate(def); case VIR_MDEV_MODEL_TYPE_VFIO_CCW: break; case VIR_MDEV_MODEL_TYPE_LAST: -- 2.19.1
Even though commit 11708641 claims that a domain is allowed have a single VFIO AP hostdev only, this is a limitation posed by the platform vendor on purely virtual devices. Generally, post parse should only be I am little confused by the term "purely virtual devices". If you are talking about the mediated device itself "purely virtual" sounds okay but if you also consider what it represents within a guest
On 11/12/18 1:14 PM, Erik Skultety wrote: than that is no longer "purely virtual" since a vfio-ap hostdev represents a bunch of "real crypto hardware" that is passed through to the guest.
used to populate some default values if missing or at least fail gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL).
This patch more of an attempt to follow the guidelines for adding new features rather than a precaution measure, since even if vfio-ap supported multiple devices, one would have to downgrade libvirt for a machine to vanish from the list or in terms of future device migration to slightly older libvirt, there would be most probably a driver mismatch that would render the migration impossible anyway.
I agree that this is the correct place for the checking. Thanks for catching and fixing it. I successfully ran some tests with these changes with regard to vfio-ap. Looks good to me so far.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 28 ---------------------------- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 237540bccc..e8e0adc819 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def) }
-static int -virDomainDefPostParseHostdev(virDomainDefPtr def) -{ - size_t i; - bool vfioap_found = false; - - /* verify settings of hostdevs vfio-ap */ - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - - if (virHostdevIsMdevDevice(hostdev) && - hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { - if (vfioap_found) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only one hostdev of model vfio-ap is " - "supported")); - return -1; - } - vfioap_found = true; - } - } - return 0; -} - - /** * virDomainDriveAddressIsUsedByDisk: * @def: domain definition containing the disks to check @@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def,
virDomainDefPostParseGraphics(def);
- if (virDomainDefPostParseHostdev(def) < 0) - return -1; - if (virDomainDefPostParseCPU(def) < 0) return -1;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 17d207513d..90253ae867 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, }
+static int +qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def) +{ + size_t i; + bool vfioap_found = false; + + /* currently, VFIO-AP is restricted to a single device only */
Well, even so it is just on mdev device it defines the complete set of crypto devices consisting of adapters, domains and controldomains on the AP bus of the guest. The ap architecture allows only one such configuration. So I suggest to remove "currently," and instead of "single device" to write "single mediated device". Besides that Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
+ for (i = 0; i < def->nhostdevs; i++) { + virDomainHostdevDefPtr hostdev = def->hostdevs[i]; + + if (virHostdevIsMdevDevice(hostdev) && + hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { + if (vfioap_found) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Only one hostdev of model vfio-ap is " + "supported")); + return -1; + } + vfioap_found = true; + } + } + + return 0; +} + + static int qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, const virDomainDef *def, @@ -4609,7 +4635,7 @@ qemuDomainMdevDefValidate(const virDomainHostdevSubsysMediatedDev *mdevsrc, case VIR_MDEV_MODEL_TYPE_VFIO_PCI: return qemuDomainMdevDefVFIOPCIValidate(mdevsrc, def, qemuCaps); case VIR_MDEV_MODEL_TYPE_VFIO_AP: - break; + return qemuDomainMdevDefVFIOAPValidate(def); case VIR_MDEV_MODEL_TYPE_VFIO_CCW: break; case VIR_MDEV_MODEL_TYPE_LAST:
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Mon, Nov 12, 2018 at 05:39:38PM +0100, Boris Fiuczynski wrote:
On 11/12/18 1:14 PM, Erik Skultety wrote:
Even though commit 11708641 claims that a domain is allowed have a single VFIO AP hostdev only, this is a limitation posed by the platform vendor on purely virtual devices. Generally, post parse should only be I am little confused by the term "purely virtual devices". If you are talking about the mediated device itself "purely virtual" sounds okay but if you also consider what it represents within a guest than that is no longer "purely virtual" since a vfio-ap hostdev represents a bunch of "real crypto hardware" that is passed through to the guest.
Yes, I was talking in context of mediated devices themselves, otherwise it would not make sense as you pointed out (not just for AP). So, let's go simple, how about I rewrite the whole commit message in the following manner: "VFIO AP has a limitation on a single device per domain, however, when commit 11708641 added support for vfio-ap, this limitation was performed as part of post parse. Generally, checks like this should be performed within the driver's validation callback to eliminate any possible chance of failing in post parse, which in the worst case could lead to the XML config to vanish." Would you be okay with ^that?
used to populate some default values if missing or at least fail gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL).
This patch more of an attempt to follow the guidelines for adding new features rather than a precaution measure, since even if vfio-ap supported multiple devices, one would have to downgrade libvirt for a machine to vanish from the list or in terms of future device migration to slightly older libvirt, there would be most probably a driver mismatch that would render the migration impossible anyway.
I'd then just drop ^this paragraph, doesn't add much info anyway.
I agree that this is the correct place for the checking. Thanks for catching and fixing it. I successfully ran some tests with these changes with regard to vfio-ap. Looks good to me so far.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 28 ---------------------------- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 237540bccc..e8e0adc819 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def) } -static int -virDomainDefPostParseHostdev(virDomainDefPtr def) -{ - size_t i; - bool vfioap_found = false; - - /* verify settings of hostdevs vfio-ap */ - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - - if (virHostdevIsMdevDevice(hostdev) && - hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { - if (vfioap_found) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only one hostdev of model vfio-ap is " - "supported")); - return -1; - } - vfioap_found = true; - } - } - return 0; -} - - /** * virDomainDriveAddressIsUsedByDisk: * @def: domain definition containing the disks to check @@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def, virDomainDefPostParseGraphics(def); - if (virDomainDefPostParseHostdev(def) < 0) - return -1; - if (virDomainDefPostParseCPU(def) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 17d207513d..90253ae867 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, } +static int +qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def) +{ + size_t i; + bool vfioap_found = false; + + /* currently, VFIO-AP is restricted to a single device only */
Well, even so it is just on mdev device it defines the complete set of crypto devices consisting of adapters, domains and controldomains on the AP bus of the guest. The ap architecture allows only one such configuration. So I suggest to remove "currently," and instead of "single device" to write "single mediated device".
Okay, I should finally read the spec. Anyway, I always tend to look at this stuff from a larger perspective, in this case, mdev itself - it doesn't have such a limitation (it may exist within the vendor driver, like NVIDIA and we obviously don't check that because vfio-pci doesn't have that either). But AP is different, as I said, I need to look at the spec. I'll adjust according to your suggestion. Thanks for review and testing the patch, Erik
On 11/13/18 9:22 AM, Erik Skultety wrote:
On Mon, Nov 12, 2018 at 05:39:38PM +0100, Boris Fiuczynski wrote:
On 11/12/18 1:14 PM, Erik Skultety wrote:
Even though commit 11708641 claims that a domain is allowed have a single VFIO AP hostdev only, this is a limitation posed by the platform vendor on purely virtual devices. Generally, post parse should only be I am little confused by the term "purely virtual devices". If you are talking about the mediated device itself "purely virtual" sounds okay but if you also consider what it represents within a guest than that is no longer "purely virtual" since a vfio-ap hostdev represents a bunch of "real crypto hardware" that is passed through to the guest.
Yes, I was talking in context of mediated devices themselves, otherwise it would not make sense as you pointed out (not just for AP). So, let's go simple, how about I rewrite the whole commit message in the following manner:
"VFIO AP has a limitation on a single device per domain, however, when commit 11708641 added support for vfio-ap, this limitation was performed as part of post parse. Generally, checks like this should be performed within the driver's validation callback to eliminate any possible chance of failing in post parse, which in the worst case could lead to the XML config to vanish."
Would you be okay with ^that? Yes, it would! :)
used to populate some default values if missing or at least fail gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL).
This patch more of an attempt to follow the guidelines for adding new features rather than a precaution measure, since even if vfio-ap supported multiple devices, one would have to downgrade libvirt for a machine to vanish from the list or in terms of future device migration to slightly older libvirt, there would be most probably a driver mismatch that would render the migration impossible anyway.
I'd then just drop ^this paragraph, doesn't add much info anyway.
OK
I agree that this is the correct place for the checking. Thanks for catching and fixing it. I successfully ran some tests with these changes with regard to vfio-ap. Looks good to me so far.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 28 ---------------------------- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 237540bccc..e8e0adc819 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def) } -static int -virDomainDefPostParseHostdev(virDomainDefPtr def) -{ - size_t i; - bool vfioap_found = false; - - /* verify settings of hostdevs vfio-ap */ - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - - if (virHostdevIsMdevDevice(hostdev) && - hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { - if (vfioap_found) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only one hostdev of model vfio-ap is " - "supported")); - return -1; - } - vfioap_found = true; - } - } - return 0; -} - - /** * virDomainDriveAddressIsUsedByDisk: * @def: domain definition containing the disks to check @@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def, virDomainDefPostParseGraphics(def); - if (virDomainDefPostParseHostdev(def) < 0) - return -1; - if (virDomainDefPostParseCPU(def) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 17d207513d..90253ae867 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, } +static int +qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def) +{ + size_t i; + bool vfioap_found = false; + + /* currently, VFIO-AP is restricted to a single device only */
Well, even so it is just on mdev device it defines the complete set of crypto devices consisting of adapters, domains and controldomains on the AP bus of the guest. The ap architecture allows only one such configuration. So I suggest to remove "currently," and instead of "single device" to write "single mediated device".
Okay, I should finally read the spec. Anyway, I always tend to look at this stuff from a larger perspective, in this case, mdev itself - it doesn't have such a limitation (it may exist within the vendor driver, like NVIDIA and we obviously don't check that because vfio-pci doesn't have that either). But AP is different, as I said, I need to look at the spec. I'll adjust according to your suggestion.
You are right that it is a limit of vfio-ap in qemu and it also enforces it by throwing an error message. I did not have the check for the limit in the first version of my libvirt series and than while testing came across the error message that qemu generated... error: Failed to start domain ap01 error: internal error: No ap bus found for device /sys/bus/mdev/devices/c6391657-ae56-4a37-870e-4adc88fe8ae2 I decided that this generic qemu message is too misleading for the libvirt user to find out what is configured wrong in his domain...
Thanks for review and testing the patch, Erik
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Tue, Nov 13, 2018 at 10:02:43AM +0100, Boris Fiuczynski wrote:
On 11/13/18 9:22 AM, Erik Skultety wrote:
On Mon, Nov 12, 2018 at 05:39:38PM +0100, Boris Fiuczynski wrote:
On 11/12/18 1:14 PM, Erik Skultety wrote:
Even though commit 11708641 claims that a domain is allowed have a single VFIO AP hostdev only, this is a limitation posed by the platform vendor on purely virtual devices. Generally, post parse should only be I am little confused by the term "purely virtual devices". If you are talking about the mediated device itself "purely virtual" sounds okay but if you also consider what it represents within a guest than that is no longer "purely virtual" since a vfio-ap hostdev represents a bunch of "real crypto hardware" that is passed through to the guest.
Yes, I was talking in context of mediated devices themselves, otherwise it would not make sense as you pointed out (not just for AP). So, let's go simple, how about I rewrite the whole commit message in the following manner:
"VFIO AP has a limitation on a single device per domain, however, when commit 11708641 added support for vfio-ap, this limitation was performed as part of post parse. Generally, checks like this should be performed within the driver's validation callback to eliminate any possible chance of failing in post parse, which in the worst case could lead to the XML config to vanish."
Would you be okay with ^that? Yes, it would! :)
used to populate some default values if missing or at least fail gracefully with VIR_DOMAIN_DEF_PARSE_ALLOW_POST_PARSE_FAIL).
This patch more of an attempt to follow the guidelines for adding new features rather than a precaution measure, since even if vfio-ap supported multiple devices, one would have to downgrade libvirt for a machine to vanish from the list or in terms of future device migration to slightly older libvirt, there would be most probably a driver mismatch that would render the migration impossible anyway.
I'd then just drop ^this paragraph, doesn't add much info anyway.
OK
I agree that this is the correct place for the checking. Thanks for catching and fixing it. I successfully ran some tests with these changes with regard to vfio-ap. Looks good to me so far.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/conf/domain_conf.c | 28 ---------------------------- src/qemu/qemu_domain.c | 28 +++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 237540bccc..e8e0adc819 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4275,31 +4275,6 @@ virDomainDefPostParseGraphics(virDomainDef *def) } -static int -virDomainDefPostParseHostdev(virDomainDefPtr def) -{ - size_t i; - bool vfioap_found = false; - - /* verify settings of hostdevs vfio-ap */ - for (i = 0; i < def->nhostdevs; i++) { - virDomainHostdevDefPtr hostdev = def->hostdevs[i]; - - if (virHostdevIsMdevDevice(hostdev) && - hostdev->source.subsys.u.mdev.model == VIR_MDEV_MODEL_TYPE_VFIO_AP) { - if (vfioap_found) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Only one hostdev of model vfio-ap is " - "supported")); - return -1; - } - vfioap_found = true; - } - } - return 0; -} - - /** * virDomainDriveAddressIsUsedByDisk: * @def: domain definition containing the disks to check @@ -5210,9 +5185,6 @@ virDomainDefPostParseCommon(virDomainDefPtr def, virDomainDefPostParseGraphics(def); - if (virDomainDefPostParseHostdev(def) < 0) - return -1; - if (virDomainDefPostParseCPU(def) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 17d207513d..90253ae867 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4599,6 +4599,32 @@ qemuDomainMdevDefVFIOPCIValidate(const virDomainHostdevSubsysMediatedDev *dev, } +static int +qemuDomainMdevDefVFIOAPValidate(const virDomainDef *def) +{ + size_t i; + bool vfioap_found = false; + + /* currently, VFIO-AP is restricted to a single device only */
Well, even so it is just on mdev device it defines the complete set of crypto devices consisting of adapters, domains and controldomains on the AP bus of the guest. The ap architecture allows only one such configuration. So I suggest to remove "currently," and instead of "single device" to write "single mediated device".
Okay, I should finally read the spec. Anyway, I always tend to look at this stuff from a larger perspective, in this case, mdev itself - it doesn't have such a limitation (it may exist within the vendor driver, like NVIDIA and we obviously don't check that because vfio-pci doesn't have that either). But AP is different, as I said, I need to look at the spec. I'll adjust according to your suggestion.
You are right that it is a limit of vfio-ap in qemu and it also enforces it by throwing an error message. I did not have the check for the limit in the first version of my libvirt series and than while testing came across the error message that qemu generated... error: Failed to start domain ap01 error: internal error: No ap bus found for device /sys/bus/mdev/devices/c6391657-ae56-4a37-870e-4adc88fe8ae2
I decided that this generic qemu message is too misleading for the libvirt user to find out what is configured wrong in his domain...
Thanks for review and testing the patch, Erik
I incorporated your and Marc's suggestions into the changes and merged the series. Thanks, Erik
participants (3)
-
Boris Fiuczynski -
Erik Skultety -
Marc Hartmayer