c/s f089fddd941 broke the Ocaml ABI by renumering XEN_SYSCTL_PHYSCAP_directio
without adjusting the Ocaml physinfo_cap_flag enumeration.  Fix this by
inserting CAP_PV between CAP_HVM and CAP_DirectIO.

Factor out the bitmap-to-list conversion logic into a helper, to avoid an
opencoded truncation of the bitmap.  To cover this, add BUILD_BUG_ON()'s at
the caller for each constant in the C-to-Ocaml conversion, and for the the
applicable max/all constant.

This will result in a compile time failure whenever constants get
renumbered/added without a compatible adjustment to the Ocaml ABI.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>
CC: Edwin Török <edvin.torok@citrix.com>

This needs backporting to Xen 4.12

v2:
 * Base this patch correctly.
---
 tools/ocaml/libs/xc/xenctrl.ml      |  1 +
 tools/ocaml/libs/xc/xenctrl.mli     |  5 ++-
 tools/ocaml/libs/xc/xenctrl_stubs.c | 77 +++++++++++++++++++++++++++----------
 xen/include/public/sysctl.h         |  4 ++
 4 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 35958b94d5..cd7e95a6fa 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -99,6 +99,7 @@ type sched_control =
 
 type physinfo_cap_flag =
 	| CAP_HVM
+	| CAP_PV
 	| CAP_DirectIO
 
 type physinfo =
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 6c4268d453..0bd06ed920 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -82,7 +82,10 @@ type domaininfo = {
   arch_config : arch_domainconfig;
 }
 type sched_control = { weight : int; cap : int; }
-type physinfo_cap_flag = CAP_HVM | CAP_DirectIO
+type physinfo_cap_flag =
+  | CAP_HVM
+  | CAP_PV
+  | CAP_DirectIO
 type physinfo = {
   threads_per_core : int;
   cores_per_socket : int;
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 2e1b29ce33..ce8dbba437 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -32,6 +32,7 @@
 
 #define XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenctrl.h>
+#include <xen-tools/libs.h>
 
 #include "mmap_stubs.h"
 
@@ -119,6 +120,31 @@ static void domain_handle_of_uuid_string(xen_domain_handle_t h,
 #undef X
 }
 
+/*
+ * Various fields which are a bitmap in the C ABI are converted to lists of
+ * integers in the Ocaml ABI for more idiomatic handling.
+ */
+static value c_bitmap_to_ocaml_list(unsigned int bitmap)
+{
+	CAMLparam0();
+	CAMLlocal2(list, tmp);
+
+	list = tmp = Val_emptylist;
+
+	for ( unsigned int i = 0; bitmap; i++, bitmap >>= 1 )
+	{
+		if ( !(bitmap & 1) )
+			continue;
+
+		tmp = caml_alloc_small(2, Tag_cons);
+		Field(tmp, 0) = Val_int(i);
+		Field(tmp, 1) = list;
+		list = tmp;
+	}
+
+	CAMLreturn(list);
+}
+
 CAMLprim value stub_xc_domain_create(value xch, value config)
 {
 	CAMLparam2(xch, config);
@@ -315,16 +341,25 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
 	Store_field(result, 15, tmp);
 
 #if defined(__i386__) || defined(__x86_64__)
-	/* emulation_flags: x86_arch_emulation_flags list; */
-	tmp = emul_list = Val_emptylist;
-	for (i = 0; i < 10; i++) {
-		if ((info->arch_config.emulation_flags >> i) & 1) {
-			tmp = caml_alloc_small(2, Tag_cons);
-			Field(tmp, 0) = Val_int(i);
-			Field(tmp, 1) = emul_list;
-			emul_list = tmp;
-		}
-	}
+	/*
+	 * emulation_flags: x86_arch_emulation_flags list;
+	 *
+	 * These BUILD_BUG_ON()'s map the C ABI to the Ocaml ABI.  If they
+	 * trip, xenctrl.ml{,i} need updating to match.
+	 */
+	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      != 0x7ff);
+	emul_list = c_bitmap_to_ocaml_list(info->arch_config.emulation_flags);
 
 	/* xen_x86_arch_domainconfig */
 	x86_arch_config = caml_alloc_tuple(1);
@@ -635,7 +670,7 @@ CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
 CAMLprim value stub_xc_physinfo(value xch)
 {
 	CAMLparam1(xch);
-	CAMLlocal3(physinfo, cap_list, tmp);
+	CAMLlocal2(physinfo, cap_list);
 	xc_physinfo_t c_physinfo;
 	int r;
 
@@ -646,15 +681,17 @@ CAMLprim value stub_xc_physinfo(value xch)
 	if (r)
 		failwith_xc(_H(xch));
 
-	tmp = cap_list = Val_emptylist;
-	for (r = 0; r < 2; r++) {
-		if ((c_physinfo.capabilities >> r) & 1) {
-			tmp = caml_alloc_small(2, Tag_cons);
-			Field(tmp, 0) = Val_int(r);
-			Field(tmp, 1) = cap_list;
-			cap_list = tmp;
-		}
-	}
+	/*
+	 * capabilities: physinfo_cap_flag list;
+	 *
+	 * These BUILD_BUG_ON()'s map the C ABI to the Ocaml ABI.  If they
+	 * trip, xenctrl.ml{,i} need updating to match.
+	 */
+	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_MAX      != XEN_SYSCTL_PHYSCAP_directio);
+	cap_list = c_bitmap_to_ocaml_list(c_physinfo.capabilities);
 
 	physinfo = caml_alloc_tuple(10);
 	Store_field(physinfo, 0, Val_int(c_physinfo.threads_per_core));
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 36b3f8c429..5401f9c2fe 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -90,6 +90,10 @@ struct xen_sysctl_tbuf_op {
  /* The platform supports direct access to I/O devices with IOMMU. */
 #define _XEN_SYSCTL_PHYSCAP_directio     2
 #define XEN_SYSCTL_PHYSCAP_directio  (1u<<_XEN_SYSCTL_PHYSCAP_directio)
+
+/* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
+#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_directio
+
 struct xen_sysctl_physinfo {
     uint32_t threads_per_core;
     uint32_t cores_per_socket;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Andrew Cooper writes ("[PATCH v2] tools/ocaml: Fix xenctrl ABI and introduce build-time checks"):
> c/s f089fddd941 broke the Ocaml ABI by renumering XEN_SYSCTL_PHYSCAP_directio
> without adjusting the Ocaml physinfo_cap_flag enumeration.  Fix this by
> inserting CAP_PV between CAP_HVM and CAP_DirectIO.
...
>  type physinfo_cap_flag =
>  	| CAP_HVM
> +	| CAP_PV
>  	| CAP_DirectIO

It is surely scandalous that we had this open-coding here of a
duplication of a Xen ABI list.  Thanks for trying to fix it.

> +/*
> + * Various fields which are a bitmap in the C ABI are converted to lists of
> + * integers in the Ocaml ABI for more idiomatic handling.

Err, I don't think you mean lists of integers.  I think you mean
lists enums, which happen to be enums.

> +	 * emulation_flags: x86_arch_emulation_flags list;
> +	 *
> +	 * These BUILD_BUG_ON()'s map the C ABI to the Ocaml ABI.  If they
> +	 * trip, xenctrl.ml{,i} need updating to match.
> +	 */
> +	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));

I really don't like this approach.  Instead of automatically deriving
the ocaml enum from the Xen ABI, or automatically checking that the
ocaml ABI agrees with the Xen one, you are instead adding a new list
which duplicates the ocaml ABI.

I suggest we do something in the build system - a new script or shell
rune, which is given the strings `x86_arch_emulation_flags' and
`X86_EMU' (and correspondingly for the other enums).

The new thing would search xenctrl.ml[i] for the type and read the
enum list there with an ad-hoc shoddy ocaml parser and then do one or
more of:

(a) synthesise the enum conversion function to map the flag
    numbers back and forth (ie the numbers in ocaml would no
    longer need to match)

(b) synthesise the BUILD_BUG_ON list you have above

(c) search the Xen headers itself and check the value correspondences

Ideally it would have been better to automatically generate
xenctrl.ml[i] from the Xen headers but I rejected that as being too
much annoying interaction with the ocaml build.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 09/09/2019 15:25, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v2] tools/ocaml: Fix xenctrl ABI and introduce build-time checks"):
>> c/s f089fddd941 broke the Ocaml ABI by renumering XEN_SYSCTL_PHYSCAP_directio
>> without adjusting the Ocaml physinfo_cap_flag enumeration.  Fix this by
>> inserting CAP_PV between CAP_HVM and CAP_DirectIO.
> ...
>>  type physinfo_cap_flag =
>>  	| CAP_HVM
>> +	| CAP_PV
>>  	| CAP_DirectIO
> It is surely scandalous that we had this open-coding here of a
> duplication of a Xen ABI list.  Thanks for trying to fix it.

Well - worse is having two of them (xenctrl.ml and .mli)

I think it is a side effect of trying to express it in more idiomatic Ocaml.

>
>> +/*
>> + * Various fields which are a bitmap in the C ABI are converted to lists of
>> + * integers in the Ocaml ABI for more idiomatic handling.
> Err, I don't think you mean lists of integers.  I think you mean
> lists enums, which happen to be enums.

The distinction between enums and integers in Ocaml is fuzzy, because
enums are defined in terms of unboxed integers, hence their construction
with "Val_int(i);".

The return value from this function *is* a list of integers, which will
either be interpreted by Ocaml as "x86_arch_emulation_flags list" or
"physinfo_cap_flag list" as appropriate.

>
>> +	 * emulation_flags: x86_arch_emulation_flags list;
>> +	 *
>> +	 * These BUILD_BUG_ON()'s map the C ABI to the Ocaml ABI.  If they
>> +	 * trip, xenctrl.ml{,i} need updating to match.
>> +	 */
>> +	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));
> I really don't like this approach.

Nor me, but it was the only thing which came to mind which didn't
involve rewriting part of the build.

> Instead of automatically deriving
> the ocaml enum from the Xen ABI, or automatically checking that the
> ocaml ABI agrees with the Xen one, you are instead adding a new list
> which duplicates the ocaml ABI.
>
> I suggest we do something in the build system - a new script or shell
> rune, which is given the strings `x86_arch_emulation_flags' and
> `X86_EMU' (and correspondingly for the other enums).
>
> The new thing would search xenctrl.ml[i] for the type and read the
> enum list there with an ad-hoc shoddy ocaml parser and then do one or
> more of:
>
> (a) synthesise the enum conversion function to map the flag
>     numbers back and forth (ie the numbers in ocaml would no
>     longer need to match)

The current code depends on the Ocaml constants being the logarithm of
the C constants so that the conversion is easy.

While this isn't a hard requirement, anything more complicated should
have a compelling reason to use.

> (b) synthesise the BUILD_BUG_ON list you have above
>
> (c) search the Xen headers itself and check the value correspondences

This would then result in two ad-hoc shoddy parsers, which I don't think
is a direction we want to go.

Option (b) seems to be the least bad alternative.

The common case needing catching is someone adding a constant to the C
ABI.  This is why the BUILD_BUG_ON()'s against XEN_X86_EMU_ALL and
XEN_SYSCTL_PHYSCAP_MAX exist.  The list of individual constants was to
catch the renumbering, which ought to be a much rarer issue.

>
> Ideally it would have been better to automatically generate
> xenctrl.ml[i] from the Xen headers but I rejected that as being too
> much annoying interaction with the ocaml build.

The interface here is a subset of some headers, and superset of others. 
Automatically generating it isn't feasible IMO, especially as we have
inconsistent ways of doing similar things.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel