[prev in list] [next in list] [prev in thread] [next in thread] 

List:       libvir-list
Subject:    Re: [PATCH v2 5/9] qemu: Deny all but VFIO PCI backends in hostdev prepare phase
From:       Martin Kletzander <mkletzan () redhat ! com>
Date:       2023-04-25 10:30:53
Message-ID: ZEer3feCgVD7M4z6 () wheatley ! k8r ! cz
[Download RAW message or body]


On Tue, Apr 25, 2023 at 10:56:43AM +0200, Michal Pr=EDvozn=EDk 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,
>>> =A0=A0=A0=A0=A0=A0=A0 break;
>>>
>>> =A0=A0=A0 case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_KVM:
>>> -=A0=A0=A0=A0=A0=A0=A0 virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 _("=
host doesn't support legacy PCI
>>> passthrough"));
>>> -=A0=A0=A0=A0=A0=A0=A0 return false;
>>> -
>>> =A0=A0=A0 case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN:
>>> =A0=A0=A0 case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>>> +=A0=A0=A0=A0=A0=A0=A0 virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 _("=
invalid PCI passthrough type '%1$s'"),
>>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0
>>> 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.=A0 _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(*backe=
nd));
>        return false;
>
>    default:
>    case VIR_DOMAIN_HOSTDEV_PCI_BACKEND_TYPE_LAST:
>        virReportEnumRangeError(virDomainHostdevSubsysPCIBackendType, *bac=
kend);
>        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>

["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]