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

[Erik Skultety]

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 at 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