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

[Julien Grall <julien@xen.org>]

Hi Stefano,

On 08/10/2021 22:46, Stefano Stabellini wrote:
> 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. 

I can understand this request. However...

Also, this is a requirement for the stable
> hypercall interface.

... leaving aside the fact that domctl is clearly not stable today, the 
flag would be rejected on hypervisor not supporting vPCI. So I don't 
really see how this is a requirement for the stable hypercall interface.

Would you mind providing more details?

> 
> 
> 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.

Cheers,


> 

-- 
Julien Grall