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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Oct 11, 2021 at 01:35:18PM +0200, Jan Beulich wrote:
> On 11.10.2021 13:29, Michal Orzel wrote:
> > On 08.10.2021 23:46, Stefano Stabellini wrote:
> >> On Fri, 8 Oct 2021, Julien Grall wrote:
> >>> On Fri, 8 Oct 2021, 20:07 Andrew Cooper, <andrew.cooper3@xxxxxxxxxx> 
> >>> 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@xxxxxxx>
> >>>       > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >>>       > Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>
> >>>
> >>>       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.
> >>
> > As Rahul is on leave:
> > I'm ok to introduce XEN_SYSCTL_PHYSCAP_vpci. I did the same for vpmu so 
> > it's ok.
> > However the problem I have is about setting this cap.
> > On arm it is easy as we are not supporting vpci at the moment so the cap 
> > can be set to false.
> > But how to deal with this cap in x86 code? I'm not familiar with x86 so I'm 
> > asking for advice.
> 
> As the sysctl is mainly from tool stacks to drive guests (DomU-s), I'd
> set it to false for x86 as well. Roger - do you see any reason this
> could be needed to express anything Dom0-related?

I agree, I don't think we should set it to true unless we also support
vPCI for domUs on x86, or else it's just going to confuse the
toolstack.

Thanks, Roger.