[libvirt] [PATCH 2/2] conf: Move VFIO AP validation from post parse to QEMU validation code
Boris Fiuczynski
fiuczy at linux.ibm.com
Tue Nov 13 09:02:43 UTC 2018
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
>
> --
> libvir-list mailing list
> libvir-list at 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
More information about the libvir-list
mailing list