[PATCH for-4.17 0/2] Ocaml stub fixes

[PATCH for-4.17 0/2] Ocaml stub fixes

[Andrew Cooper]

Patch 1 is purely some a tweak to improve legibility.  It's not necessary for
4.17 but it's also 0 risk to take as well.

Patch 2 is a change to an ABI which was newly introduced in 4.17.  It is
suboptimal for two reasons and specifically does want changing before 4.17
ships.  See patch for details.

Andrew Cooper (2):
  tools/ocaml/xc: Fix code legibility in stub_xc_domain_create()
  tools/ocaml/xc: Address ABI issues with physinfo arch flags

 tools/ocaml/libs/xc/xenctrl.ml      | 10 ++++++----
 tools/ocaml/libs/xc/xenctrl.mli     | 11 +++++++----
 tools/ocaml/libs/xc/xenctrl_stubs.c | 28 ++++++++++++++--------------
 3 files changed, 27 insertions(+), 22 deletions(-)

-- 
2.11.0

[PATCH 1/2] tools/ocaml/xc: Fix code legibility in stub_xc_domain_create()

[Andrew Cooper]

Reposition the defines to match the outer style and to make the logic
half-legible.

No functional change.

Fixes: 0570d7f276dd ("x86/msr: introduce an option for compatible MSR behavior selection")
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_stubs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 19335bdf4572..fe9c00ce008a 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -232,22 +232,20 @@ CAMLprim value stub_xc_domain_create(value xch, value wanted_domid, value config
 
         /* Mnemonics for the named fields inside xen_x86_arch_domainconfig */
 #define VAL_EMUL_FLAGS          Field(arch_domconfig, 0)
+#define VAL_MISC_FLAGS          Field(arch_domconfig, 1)
 
 		cfg.arch.emulation_flags = ocaml_list_to_c_bitmap
 			/* ! x86_arch_emulation_flags X86_EMU_ none */
 			/* ! XEN_X86_EMU_ XEN_X86_EMU_ALL all */
 			(VAL_EMUL_FLAGS);
 
-#undef VAL_EMUL_FLAGS
-
-#define VAL_MISC_FLAGS          Field(arch_domconfig, 1)
-
 		cfg.arch.misc_flags = ocaml_list_to_c_bitmap
 			/* ! x86_arch_misc_flags X86_ none */
 			/* ! XEN_X86_ XEN_X86_MISC_FLAGS_MAX max */
 			(VAL_MISC_FLAGS);
 
 #undef VAL_MISC_FLAGS
+#undef VAL_EMUL_FLAGS
 
 #else
 		caml_failwith("Unhandled: x86");
-- 
2.11.0

[PATCH 2/2] tools/ocaml/xc: Address ABI issues with physinfo arch flags

[Andrew Cooper]

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

RE: [PATCH 1/2] tools/ocaml/xc: Fix code legibility in stub_xc_domain_create()

[Henry Wang]

Hi Andrew,

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Subject: [PATCH 1/2] tools/ocaml/xc: Fix code legibility in
> stub_xc_domain_create()
> 
> Reposition the defines to match the outer style and to make the logic
> half-legible.
> 
> No functional change.
> 
> Fixes: 0570d7f276dd ("x86/msr: introduce an option for compatible MSR
> behavior selection")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Kind regards,
Henry

RE: [PATCH 2/2] tools/ocaml/xc: Address ABI issues with physinfo arch flags

[Henry Wang]

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

Re: [PATCH for-4.17 0/2] Ocaml stub fixes

[Christian Lindig]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

This is a good use of the OCaml type system to ensure only compatible flags are shared in a list and not mixed between architectures. The macro changes are good housekeeping.

— C

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>


On 12 Oct 2022, at 19:25, Andrew Cooper <Andrew.Cooper3@citrix.com<mailto:Andrew.Cooper3@citrix.com>> wrote:

Patch 1 is purely some a tweak to improve legibility.  It's not necessary for
4.17 but it's also 0 risk to take as well.

Patch 2 is a change to an ABI which was newly introduced in 4.17.  It is
suboptimal for two reasons and specifically does want changing before 4.17
ships.  See patch for details.

Andrew Cooper (2):
 tools/ocaml/xc: Fix code legibility in stub_xc_domain_create()
 tools/ocaml/xc: Address ABI issues with physinfo arch flags

tools/ocaml/libs/xc/xenctrl.ml      | 10 ++++++----
tools/ocaml/libs/xc/xenctrl.mli     | 11 +++++++----
tools/ocaml/libs/xc/xenctrl_stubs.c | 28 ++++++++++++++--------------
3 files changed, 27 insertions(+), 22 deletions(-)

--
2.11.0



[-- Attachment #2: Type: text/html, Size: 3011 bytes --]