Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall.oss@gmail.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Rahul Singh" <rahul.singh@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	"Bertrand Marquis" <bertrand.marquis@arm.com>,
	"Andre Przywara" <Andre.Przywara@arm.com>,
	"Christian Lindig" <christian.lindig@citrix.com>,
	"David Scott" <dave@recoil.org>,
	"Ian Jackson" <iwj@xenproject.org>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Date: Fri, 8 Oct 2021 14:46:14 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2110081433110.25528@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <CAJ=z9a2kYdYB9fSXX8NhLk90px03z8+D-YE5AQKW9uABx9kvrQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2662 bytes --]

On Fri, 8 Oct 2021, Julien Grall wrote:
> Hi Andrew,
> 
> On Fri, 8 Oct 2021, 20:07 Andrew Cooper, <andrew.cooper3@citrix.com> wrote:
>       On 06/10/2021 18:40, Rahul Singh wrote:
>       > Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN.
>       > Reject the use of this new flag for x86 as VPCI is not supported for
>       > DOMU guests for x86.
>       >
>       > Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>       > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>       > Acked-by: Christian Lindig <christian.lindig@citrix.com>
> 
>       I'm afraid this logic is broken.
> 
>       There's no matching feature to indicate to the toolstack whether
>       XEN_DOMCTL_CDF_vpci will be accepted or not.  The usual way of doing
>       this is with a physinfo_cap field.
> 
> 
> I am slightly puzzled by this. I am assuming you are referring to XENVER_get_features which AFAICT is a stable interface. So why should we
> use it to describe the presence of an unstable interface?
> 
> 
>       This flag needs using in Patch 10 to reject attempts to create a VM with
>       devices when passthrough support is unavailable.
> 
> 
> May I ask why we can't rely on the hypervisor to reject the flag when suitable?
> 
> 
>       Ian, for the 4.16 release, this series either needs completing with the
>       additional flag implemented, or this patch needs reverting to avoid us
>       shipping a broken interface.
> 
> 
> I fail to see how the interface would be broken... Would you mind to describe a bit more what could go wrong with this interface?

After chatting with Andrew on IRC, this is my understanding.

Today if pci=[] is specified in the VM config file then
XEN_DOMCTL_CDF_vpci is added. If Xen doesn't support it, Xen returns
an error but libxl/xl won't be able to tell exactly why it fails. So xl
will end up printing a generic VM creation failure. Andrew would like to
see something like the following in libxl:

if ( PCI_devices && !cap.vcpi )
    error("Sorry - PCI not supported")

So that the user gets a clear informative error back rather than a
generic VM creation failure. Also, this is a requirement for the stable
hypercall interface.

I think that's fine and we can implement this request easily by adding
XEN_SYSCTL_PHYSCAP_vpci. Rahul or Bertrand, are you guys happy with
doing that? Otherwise I could take it on.

As a side note, given that PCI passthrough support is actually not yet
complete on ARM, we could even just do the following in xl/libxl:

if ( PCI_devices )
    error("Sorry - PCI not supported")

or temporarily remove XEN_DOMCTL_CDF_vpci until PCI passthrough gets
finalized.