1. Patchew
  2. Libvirt
  3. [PATCH v2 0/9] qemu: Move <hostdev> preparation into qemuDomainPrepareHostdev()
  4. View patch

[PATCH v2 5/9] qemu: Deny all but VFIO PCI backends in hostdev prepare phase

Michal Privoznik posted 9 patches 2 years, 9 months ago
[PATCH v2 5/9] qemu: Deny all but VFIO PCI backends in hostdev prepare phase
Posted by Michal Privoznik 2 years, 9 months ago 
We used to support KVM and VFIO style of PCI assignment. The
former was dropped in v5.7.0-rc1~103 and thus we only support
VFIO. All other backends lead to an error (see
qemuBuildPCIHostdevDevProps(), or qemuBuildPCIHostdevDevStr() as
it used to be called in the era of aforementioned commit).

Might as well report the error in prepare phase and save hassle
of proceeding with device preparation (e.g. in case of hotplug
overriding the device's driver, setting seclabels, etc.).

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 58cd3dd710..72f36c807b 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11326,12 +11326,11 @@ qemuDomainPrepareHostdevPCI(virDomainHostdevDef *hostdev,
         break;
 
     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                       _("host doesn't support legacy PCI passthrough"));
-        return false;
-
     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("invalid PCI passthrough type '%1$s'"),
+                       virDomainHostdevSubsysPCIBackendTypeToString(*backend));
         break;
     }
 
-- 
2.39.2
Re: [PATCH v2 5/9] qemu: Deny all but VFIO PCI backends in hostdev prepare phase
Posted by Martin Kletzander 2 years, 9 months ago 
On Mon, Apr 24, 2023 at 12:41:44PM +0200, Michal Privoznik wrote:
>We used to support KVM and VFIO style of PCI assignment. The
>former was dropped in v5.7.0-rc1~103 and thus we only support
>VFIO. All other backends lead to an error (see
>qemuBuildPCIHostdevDevProps(), or qemuBuildPCIHostdevDevStr() as
>it used to be called in the era of aforementioned commit).
>
>Might as well report the error in prepare phase and save hassle
>of proceeding with device preparation (e.g. in case of hotplug
>overriding the device's driver, setting seclabels, etc.).
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/qemu/qemu_domain.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
>diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>index 58cd3dd710..72f36c807b 100644
>--- a/src/qemu/qemu_domain.c
>+++ b/src/qemu/qemu_domain.c
>@@ -11326,12 +11326,11 @@ qemuDomainPrepareHostdevPCI(virDomainHostdevDef *hostdev,
>         break;
>
>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
>-        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>-                       _("host doesn't support legacy PCI passthrough"));
>-        return false;
>-
>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>+        virReportError(VIR_ERR_INTERNAL_ERROR,
>+                       _("invalid PCI passthrough type '%1$s'"),
>+                       virDomainHostdevSubsysPCIBackendTypeToString(*backend));

This is a bit misleading as it is not "invalid", it's just unsupported,
and on top of that you're changing it to internal error and I think it
should still be CONFIG_UNSUPPORTED, at least for KVM and XEN.  _LAST
(and possibly default:) is still an internal error of course.

>         break;
>     }
>
>-- 
>2.39.2
>
Re: [PATCH v2 5/9] qemu: Deny all but VFIO PCI backends in hostdev prepare phase
Posted by Michal Prívozník 2 years, 9 months ago 
On 4/24/23 13:11, Martin Kletzander wrote:
> On Mon, Apr 24, 2023 at 12:41:44PM +0200, Michal Privoznik wrote:
>> We used to support KVM and VFIO style of PCI assignment. The
>> former was dropped in v5.7.0-rc1~103 and thus we only support
>> VFIO. All other backends lead to an error (see
>> qemuBuildPCIHostdevDevProps(), or qemuBuildPCIHostdevDevStr() as
>> it used to be called in the era of aforementioned commit).
>>
>> Might as well report the error in prepare phase and save hassle
>> of proceeding with device preparation (e.g. in case of hotplug
>> overriding the device's driver, setting seclabels, etc.).
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>> src/qemu/qemu_domain.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 58cd3dd710..72f36c807b 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -11326,12 +11326,11 @@
>> qemuDomainPrepareHostdevPCI(virDomainHostdevDef *hostdev,
>>         break;
>>
>>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                       _("host doesn't support legacy PCI
>> passthrough"));
>> -        return false;
>> -
>>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
>>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("invalid PCI passthrough type '%1$s'"),
>> +                      
>> virDomainHostdevSubsysPCIBackendTypeToString(*backend));
> 
> This is a bit misleading as it is not "invalid", it's just unsupported,
> and on top of that you're changing it to internal error and I think it
> should still be CONFIG_UNSUPPORTED, at least for KVM and XEN.  _LAST
> (and possibly default:) is still an internal error of course.

Fair enough. How about this then?

    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                       _("host doesn't support legacy PCI passthrough"));
        return false;

    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                       _("QEMU does not support device assignment mode '%1$s'"),
                       virDomainHostdevSubsysPCIBackendTypeToString(*backend));
        return false;

    default:
    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
        virReportEnumRangeError(virDomainHostdevSubsysPCIBackendType, *backend);
        break;
    }

I have this as a fixup patch on the top of this one (well, this is how
it looks after the patch), locally.

Michal
Re: [PATCH v2 5/9] qemu: Deny all but VFIO PCI backends in hostdev prepare phase
Posted by Martin Kletzander 2 years, 9 months ago 
On Tue, Apr 25, 2023 at 10:56:43AM +0200, Michal Prívozník wrote:
>On 4/24/23 13:11, Martin Kletzander wrote:
>> On Mon, Apr 24, 2023 at 12:41:44PM +0200, Michal Privoznik wrote:
>>> We used to support KVM and VFIO style of PCI assignment. The
>>> former was dropped in v5.7.0-rc1~103 and thus we only support
>>> VFIO. All other backends lead to an error (see
>>> qemuBuildPCIHostdevDevProps(), or qemuBuildPCIHostdevDevStr() as
>>> it used to be called in the era of aforementioned commit).
>>>
>>> Might as well report the error in prepare phase and save hassle
>>> of proceeding with device preparation (e.g. in case of hotplug
>>> overriding the device's driver, setting seclabels, etc.).
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>> src/qemu/qemu_domain.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 58cd3dd710..72f36c807b 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -11326,12 +11326,11 @@
>>> qemuDomainPrepareHostdevPCI(virDomainHostdevDef *hostdev,
>>>         break;
>>>
>>>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
>>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> -                       _("host doesn't support legacy PCI
>>> passthrough"));
>>> -        return false;
>>> -
>>>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
>>>     case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                       _("invalid PCI passthrough type '%1$s'"),
>>> +                      
>>> virDomainHostdevSubsysPCIBackendTypeToString(*backend));
>>
>> This is a bit misleading as it is not "invalid", it's just unsupported,
>> and on top of that you're changing it to internal error and I think it
>> should still be CONFIG_UNSUPPORTED, at least for KVM and XEN.  _LAST
>> (and possibly default:) is still an internal error of course.
>
>Fair enough. How about this then?
>
>    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
>        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                       _("host doesn't support legacy PCI passthrough"));
>        return false;
>
>    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
>        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                       _("QEMU does not support device assignment mode '%1$s'"),
>                       virDomainHostdevSubsysPCIBackendTypeToString(*backend));
>        return false;
>
>    default:
>    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>        virReportEnumRangeError(virDomainHostdevSubsysPCIBackendType, *backend);
>        break;
>    }
>
>I have this as a fixup patch on the top of this one (well, this is how
>it looks after the patch), locally.
>

Yeah, that looks fine, thanks

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.