diff mbox series

tools/ocaml/xc: Fix xc_physinfo() bindings

Message ID 20230608193312.2520222-1-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series tools/ocaml/xc: Fix xc_physinfo() bindings | expand

Commit Message

Andrew Cooper June 8, 2023, 7:33 p.m. UTC 
The original change doesn't compile on ARM:

  xenctrl_stubs.c: In function 'stub_xc_physinfo':
  xenctrl_stubs.c:821:16: error: unused variable 'arch_cap_flags_tag' [-Werror=unused-variable]
    821 |         int r, arch_cap_flags_tag;
        |                ^~~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors

but it was buggy too.

First, it tried storing an int in a pointer slot, causing heap corruption.

Next, it is not legitimate to exclude arm32 in the toolstack as it explicitly
can operate an arm64 toolstack and build arm64 domains.  That in turn means
that you can't stash a C uint32_t in an OCaml int.

Rewrite the arch_capabilities handling from scratch.  Break it out into a
separate function, and make the construction of arch_physinfo_cap_flags common
to prevent other indirection bugs.

Reintroduce arm_physinfo_caps with the fields broken out.

Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: Edwin Török <edwin.torok@cloud.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>
CC: Luca Fancellu <luca.fancellu@arm.com>

RFC - untested because I'm failing at cross-compiling ARM Ocaml, but staging
has been broken too long...
---
 tools/ocaml/libs/xc/xenctrl.ml      |  7 +++-
 tools/ocaml/libs/xc/xenctrl.mli     |  7 +++-
 tools/ocaml/libs/xc/xenctrl_stubs.c | 52 ++++++++++++++++++++---------
 3 files changed, 48 insertions(+), 18 deletions(-)


base-commit: 6915a120641b5d232762af7882266048cf039ca0
prerequisite-patch-id: 85ffb6cbe1ddfdec0afb2ac5c2cfd8910ddfd783

Comments

Luca Fancellu June 8, 2023, 7:42 p.m. UTC | #1 
> On 8 Jun 2023, at 20:33, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> The original change doesn't compile on ARM:
> 
>  xenctrl_stubs.c: In function 'stub_xc_physinfo':
>  xenctrl_stubs.c:821:16: error: unused variable 'arch_cap_flags_tag' [-Werror=unused-variable]
>    821 |         int r, arch_cap_flags_tag;
>        |                ^~~~~~~~~~~~~~~~~~
>  cc1: all warnings being treated as errors
> 
> but it was buggy too.
> 
> First, it tried storing an int in a pointer slot, causing heap corruption.
> 
> Next, it is not legitimate to exclude arm32 in the toolstack as it explicitly
> can operate an arm64 toolstack and build arm64 domains.  That in turn means
> that you can't stash a C uint32_t in an OCaml int.
> 
> Rewrite the arch_capabilities handling from scratch.  Break it out into a
> separate function, and make the construction of arch_physinfo_cap_flags common
> to prevent other indirection bugs.
> 
> Reintroduce arm_physinfo_caps with the fields broken out.
> 
> Fixes: 56a7aaa16bfe ("tools: add physinfo arch_capabilities handling for Arm")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Christian Lindig <christian.lindig@citrix.com>
> CC: Edwin Török <edwin.torok@cloud.com>
> CC: Rob Hoes <Rob.Hoes@citrix.com>
> CC: Luca Fancellu <luca.fancellu@arm.com>
> 
> RFC - untested because I'm failing at cross-compiling ARM Ocaml, but staging
> has been broken too long...

Thank you for this patch, I’ll try in the next days to build it for Arm

> ---
> tools/ocaml/libs/xc/xenctrl.ml      |  7 +++-
> tools/ocaml/libs/xc/xenctrl.mli     |  7 +++-
> tools/ocaml/libs/xc/xenctrl_stubs.c | 52 ++++++++++++++++++++---------
> 3 files changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index bf23ca50bb15..d6c6eb73db44 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -128,10 +128,15 @@ type physinfo_cap_flag =
>   | CAP_Gnttab_v1
>   | CAP_Gnttab_v2
> 
> +type arm_physinfo_caps =
> +  {
> +    sve_vl: int;
> +  }
> +
> type x86_physinfo_cap_flag
> 
> type arch_physinfo_cap_flags =
> -  | ARM of int
> +  | ARM of arm_physinfo_caps
>   | X86 of x86_physinfo_cap_flag list
> 
> type physinfo =
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index ed1e28ea30a0..3bfc16edba96 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -113,10 +113,15 @@ type physinfo_cap_flag =
>   | CAP_Gnttab_v1
>   | CAP_Gnttab_v2
> 
> +type arm_physinfo_caps =
> +  {
> +    sve_vl: int;
> +  }
> +
> type x86_physinfo_cap_flag
> 
> type arch_physinfo_cap_flags =
> -  | ARM of int
> +  | ARM of arm_physinfo_caps
>   | X86 of x86_physinfo_cap_flag list
> 
> type physinfo = {
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index a03da31f6f2c..6b2869af04ef 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -812,13 +812,46 @@ CAMLprim value stub_xc_send_debug_keys(value xch_val, value keys)
> CAMLreturn(Val_unit);
> }
> 
> +CAMLprim value physinfo_arch_caps(const xc_physinfo_t *info)
> +{
> + CAMLparam0();
> + CAMLlocal2(arch_cap_flags, arch_obj);
> + int tag = -1;
> +
> +#if defined(__arm__) || defined(__aarch64__)
> +
> + tag = 0; /* tag ARM */
> +
> + arch_obj = caml_alloc_tuple(1);
> +
> + Store_field(arch_obj, 0,
> +    Val_int(MASK_EXTR(info->arch_capabilities,
> +      XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK) * 128));
> +
> +#elif defined(__i386__) || defined(__x86_64__)
> +
> + tag = 1; /* tag x86 */
> +
> + arch_obj = Tag_cons;
> +
> +#endif
> +
> + if ( tag < 0 )
> + caml_failwith("Unhandled architecture");
> +
> + arch_cap_flags = caml_alloc_small(1, tag);
> + Store_field(arch_cap_flags, 0, arch_obj);
> +
> + return arch_cap_flags;
> +}
> +
> CAMLprim value stub_xc_physinfo(value xch_val)
> {
> CAMLparam1(xch_val);
> - CAMLlocal4(physinfo, cap_list, arch_cap_flags, arch_cap_list);
> + CAMLlocal2(physinfo, cap_list);
> xc_interface *xch = xch_of_val(xch_val);
> xc_physinfo_t c_physinfo;
> - int r, arch_cap_flags_tag;
> + int r;
> 
> caml_enter_blocking_section();
> r = xc_physinfo(xch, &c_physinfo);
> @@ -846,20 +879,7 @@ CAMLprim value stub_xc_physinfo(value xch_val)
> Store_field(physinfo, 7, caml_copy_nativeint(c_physinfo.scrub_pages));
> Store_field(physinfo, 8, cap_list);
> Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
> -
> -#if defined(__i386__) || defined(__x86_64__)
> - arch_cap_list = Tag_cons;
> -
> - arch_cap_flags_tag = 1; /* tag x86 */
> -
> - 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);
> -#elif defined(__aarch64__)
> - Store_field(physinfo, 10, Val_int(c_physinfo.arch_capabilities));
> -#else
> - caml_failwith("Unhandled architecture");
> -#endif
> + Store_field(physinfo, 10, physinfo_arch_caps(&c_physinfo));
> 
> CAMLreturn(physinfo);
> }
> 
> base-commit: 6915a120641b5d232762af7882266048cf039ca0
> prerequisite-patch-id: 85ffb6cbe1ddfdec0afb2ac5c2cfd8910ddfd783
> -- 
> 2.30.2
>
Christian Lindig June 9, 2023, 8:17 a.m. UTC | #2 
> On 8 Jun 2023, at 20:33, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> +type arm_physinfo_caps =
> +  {
> +    sve_vl: int;
> +  }
> +


Does the OCaml side need to know about the structure of this value or is it enough to pass it around as an abstract value because all logic is on the C side? I assume the OCaml side needs at least a way to persist the value and hence needs to know some representation. 

> 	Store_field(arch_obj, 0,
> +		    Val_int(MASK_EXTR(info->arch_capabilities,
> +				      XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK) * 128));
> +

What is the “* 128” achieving as part of this encoding?

— C
Andrew Cooper June 9, 2023, 9:37 a.m. UTC | #3 
On 09/06/2023 9:17 am, Christian Lindig wrote:
>> On 8 Jun 2023, at 20:33, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>
>> +type arm_physinfo_caps =
>> +  {
>> +    sve_vl: int;
>> +  }
>> +
>
> Does the OCaml side need to know about the structure of this value or is it enough to pass it around as an abstract value because all logic is on the C side? I assume the OCaml side needs at least a way to persist the value and hence needs to know some representation.

Yes, Ocaml does need to know about the structure.

info->arch_capabilities is a collection of misc info.  It was intended
to be just a bitmap of things, hence why it's a list on the x86 side (We
added fields, then had to revert and I haven't got back around to
reworking that yet).

But now ARM have put a non-bit field field into it, where a toolstack
needs to select a domain_create sve_vl setting between min(0,
physinfo.sve_vl)

The Ocaml code doesn't have an #include <public/sysctl.h> so doesn't
have the masks/constants required to decompose the integer into it's
constitute parts.

>
>> 	Store_field(arch_obj, 0,
>> +		    Val_int(MASK_EXTR(info->arch_capabilities,
>> +				      XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK) * 128));
>> +
> What is the “* 128” achieving as part of this encoding?

No functional change from before...  except it was previously hidden in
a different header file which I'm in the process of deleting.

The vector length is a multiple of 128.  This value is more amenable to
being understood in a log file by a human.

~Andrew
Christian Lindig June 9, 2023, 10 a.m. UTC | #4 
> On 9 Jun 2023, at 10:37, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 09/06/2023 9:17 am, Christian Lindig wrote:
>>> On 8 Jun 2023, at 20:33, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>> 
>>> +type arm_physinfo_caps =
>>> +  {
>>> +    sve_vl: int;
>>> +  }
>>> +
>> 
>> Does the OCaml side need to know about the structure of this value or is it enough to pass it around as an abstract value because all logic is on the C side? I assume the OCaml side needs at least a way to persist the value and hence needs to know some representation.
> 
> Yes, Ocaml does need to know about the structure.
> 
> [..]
> 
> The vector length is a multiple of 128.  This value is more amenable to
> being understood in a log file by a human.


Acked-by: Christian Lindig <christian.lindig@cloud.com>
diff mbox series

Patch

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index bf23ca50bb15..d6c6eb73db44 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -128,10 +128,15 @@  type physinfo_cap_flag =
   | CAP_Gnttab_v1
   | CAP_Gnttab_v2
 
+type arm_physinfo_caps =
+  {
+    sve_vl: int;
+  }
+
 type x86_physinfo_cap_flag
 
 type arch_physinfo_cap_flags =
-  | ARM of int
+  | ARM of arm_physinfo_caps
   | X86 of x86_physinfo_cap_flag list
 
 type physinfo =
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index ed1e28ea30a0..3bfc16edba96 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -113,10 +113,15 @@  type physinfo_cap_flag =
   | CAP_Gnttab_v1
   | CAP_Gnttab_v2
 
+type arm_physinfo_caps =
+  {
+    sve_vl: int;
+  }
+
 type x86_physinfo_cap_flag
 
 type arch_physinfo_cap_flags =
-  | ARM of int
+  | ARM of arm_physinfo_caps
   | X86 of x86_physinfo_cap_flag list
 
 type physinfo = {
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index a03da31f6f2c..6b2869af04ef 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -812,13 +812,46 @@  CAMLprim value stub_xc_send_debug_keys(value xch_val, value keys)
 	CAMLreturn(Val_unit);
 }
 
+CAMLprim value physinfo_arch_caps(const xc_physinfo_t *info)
+{
+	CAMLparam0();
+	CAMLlocal2(arch_cap_flags, arch_obj);
+	int tag = -1;
+
+#if defined(__arm__) || defined(__aarch64__)
+
+	tag = 0; /* tag ARM */
+
+	arch_obj = caml_alloc_tuple(1);
+
+	Store_field(arch_obj, 0,
+		    Val_int(MASK_EXTR(info->arch_capabilities,
+				      XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK) * 128));
+
+#elif defined(__i386__) || defined(__x86_64__)
+
+	tag = 1; /* tag x86 */
+
+	arch_obj = Tag_cons;
+
+#endif
+
+	if ( tag < 0 )
+		caml_failwith("Unhandled architecture");
+
+	arch_cap_flags = caml_alloc_small(1, tag);
+	Store_field(arch_cap_flags, 0, arch_obj);
+
+	return arch_cap_flags;
+}
+
 CAMLprim value stub_xc_physinfo(value xch_val)
 {
 	CAMLparam1(xch_val);
-	CAMLlocal4(physinfo, cap_list, arch_cap_flags, arch_cap_list);
+	CAMLlocal2(physinfo, cap_list);
 	xc_interface *xch = xch_of_val(xch_val);
 	xc_physinfo_t c_physinfo;
-	int r, arch_cap_flags_tag;
+	int r;
 
 	caml_enter_blocking_section();
 	r = xc_physinfo(xch, &c_physinfo);
@@ -846,20 +879,7 @@  CAMLprim value stub_xc_physinfo(value xch_val)
 	Store_field(physinfo, 7, caml_copy_nativeint(c_physinfo.scrub_pages));
 	Store_field(physinfo, 8, cap_list);
 	Store_field(physinfo, 9, Val_int(c_physinfo.max_cpu_id + 1));
-
-#if defined(__i386__) || defined(__x86_64__)
-	arch_cap_list = Tag_cons;
-
-	arch_cap_flags_tag = 1; /* tag x86 */
-
-	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);
-#elif defined(__aarch64__)
-	Store_field(physinfo, 10, Val_int(c_physinfo.arch_capabilities));
-#else
-	caml_failwith("Unhandled architecture");
-#endif
+	Store_field(physinfo, 10, physinfo_arch_caps(&c_physinfo));
 
 	CAMLreturn(physinfo);
 }