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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 08.10.2021 23: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@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.

Cheers,
Michal