[Xen-devel] [PATCH] tools/ocaml: Fix xenctrl ABI and introduce build-time checks

[Xen-devel] [PATCH] tools/ocaml: Fix xenctrl ABI and introduce build-time checks

[Andrew Cooper]

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.

c/s 82239182eb3 extended the XEN_SYSCTL_PHYSCAP_* ABI with a HAP bit.  Extend
the physinfo_cap_flag enumeration to match.

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>

A patch along these lines wants backporting to at least 4.12, but without the
HAP constant.  I'm happy to do the backport.
---
 tools/ocaml/libs/xc/xenctrl.ml      |  2 +
 tools/ocaml/libs/xc/xenctrl.mli     |  6 ++-
 tools/ocaml/libs/xc/xenctrl_stubs.c | 78 +++++++++++++++++++++++++++----------
 xen/include/public/sysctl.h         |  3 ++
 4 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 35958b94d5..5da7c96cee 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -99,7 +99,9 @@ type sched_control =
 
 type physinfo_cap_flag =
 	| CAP_HVM
+	| CAP_PV
 	| CAP_DirectIO
+	| CAP_HAP
 
 type physinfo =
 {
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 6c4268d453..b4e76b827e 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -82,7 +82,11 @@ 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
+  | CAP_HAP
 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..10b306f9f8 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,18 @@ 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_hap      != (1u <<  3));
+	BUILD_BUG_ON(XEN_SYSCTL_PHYSCAP_MAX      != XEN_SYSCTL_PHYSCAP_hap);
+	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 e02d7ce4c6..d4b455619c 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -94,6 +94,9 @@ struct xen_sysctl_tbuf_op {
 #define _XEN_SYSCTL_PHYSCAP_hap          3
 #define XEN_SYSCTL_PHYSCAP_hap           (1u<<_XEN_SYSCTL_PHYSCAP_hap)
 
+/* Max XEN_SYSCTL_PHYSCAP_* constant.  Used for ABI checking. */
+#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_hap
+
 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

Re: [Xen-devel] [PATCH] tools/ocaml: Fix xenctrl ABI and introduce build-time checks

[Andrew Cooper]

On 09/09/2019 14:32, Andrew Cooper wrote:
> 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.
>
> c/s 82239182eb3 extended the XEN_SYSCTL_PHYSCAP_* ABI with a HAP bit.  Extend
> the physinfo_cap_flag enumeration to match.
>
> 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>

Sorry - please ignore this.  It is based incorrectly WRT Roger's series.

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