RE: [PATCH v4 1/3] domctl: introduce a new domain create flag, XEN_DOMCTL_CDF_disable_fifo, ...

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 25 November 2020 09:36
> To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; Elnikety, Eslam 
> <elnikety@xxxxxxxxxx>; Christian Lindig
> <christian.lindig@xxxxxxxxxx>; David Scott <dave@xxxxxxxxxx>; Ian Jackson 
> <iwj@xxxxxxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Julien Grall 
> <julien@xxxxxxx>; Stefano
> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul 
> Durrant <paul@xxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH v4 1/3] domctl: introduce a new domain create 
> flag,
> XEN_DOMCTL_CDF_disable_fifo, ...
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 25.11.2020 10:20, Jan Beulich wrote:
> > On 24.11.2020 20:17, Paul Durrant 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_disable_fifo  7
> >> +#define XEN_DOMCTL_CDF_disable_fifo   (1U << _XEN_DOMCTL_CDF_disable_fifo)
> >
> > Despite getting longish, I think this needs "evtchn" somewhere in
> > the name. To keep size bounded, maybe XEN_DOMCTL_CDF_no_fifo_evtchn?
> >

I'm ok with that name; I'll send a v5.

> >>  /* 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_disable_fifo
> >
> > While not directly related to this patch, I'm puzzled by the
> > presence of this constant: I've not been able to find any use of
> > it. In particular you did have a need to modify
> > sanitise_domain_config().
> 
> So it was you to introduce this, right away without any user, in
> 7fb0e134f8c6 ("tools/ocaml: abi: Use formal conversion and check
> in more places"). The only reference is from what I regard as a
> comment (I don't speak any ocaml, so I may be wrong). Could you
> clarify why we need to maintain this constant?
> 

I can't remember the exact sequence of events but it became apparent at some 
point that the ocaml bindings were out of sync and they rely on a list of 
domain create flags where the number has to match the bit-shift value in 
domctl.h (among other things). Thus there is an auto-generated header called 
"xenctrl_abi_check.h" which is included by xenctrl_stubs.c. This header is 
generated from xenctrl.ml by the perl script "abi-check" and it relies the 
XEN_DOMCTL_CDF_MAX constant to form part of the checks it generates.

As an example, here is the generated header with this patch applied:

// found ocaml type x86_arch_emulation_flags at xenctrl.ml:38
BUILD_BUG_ON( XEN_X86_EMU_LAPIC              != (1u << 0)  );
BUILD_BUG_ON( XEN_X86_EMU_HPET               != (1u << 1)  );
BUILD_BUG_ON( XEN_X86_EMU_PM                 != (1u << 2)  );
BUILD_BUG_ON( XEN_X86_EMU_RTC                != (1u << 3)  );
BUILD_BUG_ON( XEN_X86_EMU_IOAPIC             != (1u << 4)  );
BUILD_BUG_ON( XEN_X86_EMU_PIC                != (1u << 5)  );
BUILD_BUG_ON( XEN_X86_EMU_VGA                != (1u << 6)  );
BUILD_BUG_ON( XEN_X86_EMU_IOMMU              != (1u << 7)  );
BUILD_BUG_ON( XEN_X86_EMU_PIT                != (1u << 8)  );
BUILD_BUG_ON( XEN_X86_EMU_USE_PIRQ           != (1u << 9)  );
BUILD_BUG_ON( XEN_X86_EMU_VPCI               != (1u << 10) );
BUILD_BUG_ON( XEN_X86_EMU_ALL                != (1u << 11)-1u );
// found ocaml type domain_create_flag at xenctrl.ml:60
BUILD_BUG_ON( XEN_DOMCTL_CDF_hvm             != (1u << 0)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_hap             != (1u << 1)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_s3_integrity    != (1u << 2)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_oos_off         != (1u << 3)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_xs_domain       != (1u << 4)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_iommu           != (1u << 5)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_nested_virt     != (1u << 6)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_disable_fifo    != (1u << 7)  );
BUILD_BUG_ON( XEN_DOMCTL_CDF_MAX             != (1u << 7)  );
// found ocaml type domain_create_iommu_opts at xenctrl.ml:70
BUILD_BUG_ON( XEN_DOMCTL_IOMMU_no_sharept    != (1u << 0)  );
BUILD_BUG_ON( XEN_DOMCTL_IOMMU_MAX           != (1u << 0)  );
// found ocaml type physinfo_cap_flag at xenctrl.ml:113
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_hvm         != (1u << 0)  );
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_pv          != (1u << 1)  );
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_directio    != (1u << 2)  );
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_hap         != (1u << 3)  );
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_shadow      != (1u << 4)  );
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share != (1u << 5)  );
BUILD_BUG_ON( XEN_SYSCTL_PHYSCAP_MAX         != (1u << 5)  );

  Paul