Re: [libvirt] [PATCH 2/2] conf: Move VFIO AP validation from post parse to QEMU validation code

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(a)redhat.com&gt; > > > --- > > > 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