Re: [PATCH] tools/ocaml/xc: Fix xc_physinfo() bindings

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 8 Jun 2023, at 20:33, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
> CC: Edwin Török <edwin.torok@xxxxxxxxx>
> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx>
> CC: Luca Fancellu <luca.fancellu@xxxxxxx>
> 
> 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
>