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

Rahul Singh posted 11 patches 4 years, 4 months ago
There is a newer version of this series
[PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Rahul Singh 4 years, 4 months ago
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>
---
Change in v5:
- Added Acked-by: Christian Lindig <christian.lindig@citrix.com>
- Added Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Change in v4: Added in this version
---
---
 tools/ocaml/libs/xc/xenctrl.ml  | 1 +
 tools/ocaml/libs/xc/xenctrl.mli | 1 +
 xen/arch/arm/domain.c           | 4 ++--
 xen/arch/x86/domain.c           | 6 ++++++
 xen/common/domain.c             | 2 +-
 xen/include/public/domctl.h     | 4 +++-
 6 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index a5588c643f..7ed1c00e47 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -69,6 +69,7 @@ type domain_create_flag =
 	| CDF_XS_DOMAIN
 	| CDF_IOMMU
 	| CDF_NESTED_VIRT
+	| CDF_VPCI
 
 type domain_create_iommu_opts =
 	| IOMMU_NO_SHAREPT
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 6e94940a8a..391d4abdf8 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -62,6 +62,7 @@ type domain_create_flag =
   | CDF_XS_DOMAIN
   | CDF_IOMMU
   | CDF_NESTED_VIRT
+  | CDF_VPCI
 
 type domain_create_iommu_opts =
   | IOMMU_NO_SHAREPT
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 19c756ac3d..36138c1b2e 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -622,8 +622,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     unsigned int max_vcpus;
 
-    /* HVM and HAP must be set. IOMMU may or may not be */
-    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
+    /* HVM and HAP must be set. IOMMU and VPCI may or may not be */
+    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu & ~XEN_DOMCTL_CDF_vpci) !=
          (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
     {
         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..79c2aa4636 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -662,6 +662,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( config->flags & XEN_DOMCTL_CDF_vpci )
+    {
+        dprintk(XENLOG_INFO, "vPCI cannot be enabled yet\n");
+        return -EINVAL;
+    }
+
     if ( config->vmtrace_size )
     {
         unsigned int size = config->vmtrace_size;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6ee5d033b0..40d67ec342 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -483,7 +483,7 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
          ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
            XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
            XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
-           XEN_DOMCTL_CDF_nested_virt) )
+           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci) )
     {
         dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
         return -EINVAL;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 96696e3842..4245da3f45 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -70,9 +70,11 @@ struct xen_domctl_createdomain {
 #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
 #define _XEN_DOMCTL_CDF_nested_virt   6
 #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
+#define _XEN_DOMCTL_CDF_vpci          7
+#define XEN_DOMCTL_CDF_vpci           (1U << _XEN_DOMCTL_CDF_vpci)
 
 /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
-#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_nested_virt
+#define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpci
 
     uint32_t flags;
 
-- 
2.25.1


Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Jan Beulich 4 years, 4 months ago
On 06.10.2021 19:40, Rahul Singh wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -70,9 +70,11 @@ struct xen_domctl_createdomain {
>  #define XEN_DOMCTL_CDF_iommu          (1U<<_XEN_DOMCTL_CDF_iommu)
>  #define _XEN_DOMCTL_CDF_nested_virt   6
>  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
> +#define _XEN_DOMCTL_CDF_vpci          7
> +#define XEN_DOMCTL_CDF_vpci           (1U << _XEN_DOMCTL_CDF_vpci)

Like said in [1] here I similarly don't see the need for two constants.
Preferably with the former of the two dropped
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[1] https://lists.xen.org/archives/html/xen-devel/2021-10/msg00266.html


Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Andrew Cooper 4 years, 4 months ago
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.

This flag needs using in Patch 10 to reject attempts to create a VM with
devices when passthrough support is unavailable.

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.

~Andrew


Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Julien Grall 4 years, 4 months ago
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?

Cheers,


> ~Andrew
>
>
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Stefano Stabellini 4 years, 4 months ago
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.
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Julien Grall 4 years, 4 months ago
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

Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Michal Orzel 4 years, 4 months ago
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@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.
> 
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

Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Jan Beulich 4 years, 4 months ago
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@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.
>>
> 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?

Jan


Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Roger Pau Monné 4 years, 4 months ago
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@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.
> >>
> > 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.

Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Posted by Ian Jackson 4 years, 4 months ago
Andrew Cooper writes ("Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag"):
> 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 have caught up on this thread.  I think (hope?) it's converging.
If not please let me know and maybe I can help.

Can I ask to please be CC'd on the whole series for the patch(es) to
sort this out.  Please also make sure that those who commented are
CC'd.  I want the fixes that ultimately get committed to be the final
fixes (probably that means they should have consensus).

FTAOD, from a formal release management point of view: I regard those
putative fixes as bugfixes so they can go in after the feature freeze
(which is this Friday).  But if suitable fixes don't make it in within
the first few weeks of the freeze (and, as I expect, the maintainers
or I still regard this as an RC bug) then a revert of the new feature
will be the only option.

Ian.