The current bindings function, but the preexisting

  type physinfo_arch_cap_flag =
         | X86 of x86_physinfo_arch_cap_flag

is a special case in the Ocaml type system with an unusual indirection, and
will break when a second option, e.g. `| ARM of ...` is added.

Also, the position the list is logically wrong.  Currently, the types express
a list of elements which might be an x86 flag or an arm flag (and can
intermix), whereas what we actually want is either a list of x86 flags, or a
list of ARM flags (that cannot intermix).

Rework the Ocaml types to avoid the ABI special case and move the list
primitive, and adjust the C bindings to match.

Fixes: 2ce11ce249a3 ("x86/HVM: allow per-domain usage of hardware virtualized APIC")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: David Scott <dave@recoil.org>
CC: Edwin Torok <edvin.torok@citrix.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>
CC: Henry Wang <Henry.Wang@arm.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      | 10 ++++++----
 tools/ocaml/libs/xc/xenctrl.mli     | 11 +++++++----
 tools/ocaml/libs/xc/xenctrl_stubs.c | 22 ++++++++++++----------
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 0c71e5eef3c7..28ed6422317c 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -130,13 +130,15 @@ type physinfo_cap_flag =
 	| CAP_Gnttab_v1
 	| CAP_Gnttab_v2
 
+type arm_physinfo_cap_flag
 
-type x86_physinfo_arch_cap_flag =
+type x86_physinfo_cap_flag =
 	| CAP_X86_ASSISTED_XAPIC
 	| CAP_X86_ASSISTED_X2APIC
 
-type physinfo_arch_cap_flag =
-	| X86 of x86_physinfo_arch_cap_flag
+type arch_physinfo_cap_flags =
+	| ARM of arm_physinfo_cap_flag list
+	| X86 of x86_physinfo_cap_flag list
 
 type physinfo =
 {
@@ -151,7 +153,7 @@ type physinfo =
 	(* XXX hw_cap *)
 	capabilities     : physinfo_cap_flag list;
 	max_nr_cpus      : int;
-	arch_capabilities : physinfo_arch_cap_flag list;
+	arch_capabilities : arch_physinfo_cap_flags;
 }
 
 type version =
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index a8458e19ca4b..c2076d60c970 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -115,12 +115,15 @@ type physinfo_cap_flag =
   | CAP_Gnttab_v1
   | CAP_Gnttab_v2
 
-type x86_physinfo_arch_cap_flag =
+type arm_physinfo_cap_flag
+
+type x86_physinfo_cap_flag =
   | CAP_X86_ASSISTED_XAPIC
   | CAP_X86_ASSISTED_X2APIC
 
-type physinfo_arch_cap_flag =
-  | X86 of x86_physinfo_arch_cap_flag
+type arch_physinfo_cap_flags =
+  | ARM of arm_physinfo_cap_flag list
+  | X86 of x86_physinfo_cap_flag list
 
 type physinfo = {
   threads_per_core : int;
@@ -133,7 +136,7 @@ type physinfo = {
   scrub_pages      : nativeint;
   capabilities     : physinfo_cap_flag list;
   max_nr_cpus      : int; (** compile-time max possible number of nr_cpus *)
-  arch_capabilities : physinfo_arch_cap_flag list;
+  arch_capabilities : arch_physinfo_cap_flags;
 }
 type version = { major : int; minor : int; extra : string; }
 type compile_info = {
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index fe9c00ce008a..03f4cbf93cd3 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -716,9 +716,10 @@ CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
 CAMLprim value stub_xc_physinfo(value xch)
 {
 	CAMLparam1(xch);
-	CAMLlocal4(physinfo, cap_list, x86_arch_cap_list, arch_cap_list);
+	CAMLlocal2(physinfo, cap_list);
+	CAMLlocal2(arch_cap_flags, arch_cap_list);
 	xc_physinfo_t c_physinfo;
-	int r;
+	int r, arch_cap_flags_tag;
 
 	caml_enter_blocking_section();
 	r = xc_physinfo(_H(xch), &c_physinfo);
@@ -748,18 +749,19 @@ CAMLprim value stub_xc_physinfo(value xch)
 	Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
 
 #if defined(__i386__) || defined(__x86_64__)
-	x86_arch_cap_list = c_bitmap_to_ocaml_list
-		/* ! x86_physinfo_arch_cap_flag CAP_X86_ none */
+	arch_cap_list = c_bitmap_to_ocaml_list
+		/* ! x86_physinfo_cap_flag CAP_X86_ none */
 		/* ! XEN_SYSCTL_PHYSCAP_X86_ XEN_SYSCTL_PHYSCAP_X86_MAX max */
 		(c_physinfo.arch_capabilities);
-	/*
-	 * arch_capabilities: physinfo_arch_cap_flag list;
-	 */
-	arch_cap_list = x86_arch_cap_list;
+
+	arch_cap_flags_tag = 1; /* tag x86 */
 #else
-	arch_cap_list = Val_emptylist;
+	caml_failwith("Unhandled architecture");
 #endif
-	Store_field(physinfo, 10, arch_cap_list);
+
+	arch_cap_flags = caml_alloc_small(1, arch_cap_flags_tag);
+	Store_field(arch_cap_flags, 0, arch_cap_list);
+	Store_field(physinfo, 10, arch_cap_flags);
 
 	CAMLreturn(physinfo);
 }
-- 
2.11.0

Hi Andrew,

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Subject: [PATCH 2/2] tools/ocaml/xc: Address ABI issues with physinfo arch
> flags
> 
> The current bindings function, but the preexisting
> 
>   type physinfo_arch_cap_flag =
>          | X86 of x86_physinfo_arch_cap_flag
> 
> is a special case in the Ocaml type system with an unusual indirection, and
> will break when a second option, e.g. `| ARM of ...` is added.
> 
> Also, the position the list is logically wrong.  Currently, the types express
> a list of elements which might be an x86 flag or an arm flag (and can
> intermix), whereas what we actually want is either a list of x86 flags, or a
> list of ARM flags (that cannot intermix).
> 
> Rework the Ocaml types to avoid the ABI special case and move the list
> primitive, and adjust the C bindings to match.
> 
> Fixes: 2ce11ce249a3 ("x86/HVM: allow per-domain usage of hardware
> virtualized APIC")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry