[PATCH for-4.22 0/2] tools/ocaml: Fixes to Xenctrl.domain_getinfo{,list}()

[PATCH for-4.22 0/2] tools/ocaml: Fixes to Xenctrl.domain_getinfo{,list}()

[Andrew Cooper]

For 4.22(?). This fixes a segfault in the Ocaml runtime on ARM.

It's not a regression vs 4.21, but it is deterministic and easy to fix.
Technically patch 1 is sufficient to fix the segfault, but producing an
unconditional exception is almost as useless.

Andrew Cooper (1):
  tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM

Julian Vetter (1):
  tools/ocaml: Fill arch_config for ARM in domain_getinfo{,list}()

 tools/ocaml/libs/xc/xenctrl_stubs.c | 37 +++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 10 deletions(-)


base-commit: 75f920bd47a4f59eaaa4596aa3f4e12a447d26d2
-- 
2.39.5

[PATCH 1/2] tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM

[Andrew Cooper]

The Store_field(result, 16, arch_config) sits inside an ifdef x86, meaning
that on other archtiectures the pointer is not filled in.  The Ocaml runtime
then falls over a NULL pointer (really the Val_unit used to initialise
'result') when the layout in the heap doesn't match the type system.

Rearrange alloc_domaininfo() to avoid this.  Similarly to
physinfo_arch_caps(), raise an exception if the architecture code hasn't
filled in an appropriate tag.  Move the setup of arch_domainconfig to be
common logic.

In order to simplify the addition of other architectures, remove the
arch_config variable (resuing tmp as it's touched exactly once), and rename
x86_arch_config to be arch_config so each architecture can fill in a suitable
one without needing more local variables.

Reported-by: Julian Vetter <julian.vetter@vates.tech>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Andrii Sultanov <andriy.sultanov@vates.tech>
CC: Guillaume Thouvenin <guillaume.thouvenin@vates.tech>
CC: Julian Vetter <julian.vetter@vates.tech>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 7f6381cdd2fe..441e1d83cfec 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -414,7 +414,8 @@ CAMLprim value stub_xc_domain_shutdown(value xch_val, value domid, value reason)
 static value alloc_domaininfo(xc_domaininfo_t * info)
 {
 	CAMLparam0();
-	CAMLlocal5(result, tmp, arch_config, x86_arch_config, emul_list);
+	CAMLlocal4(result, tmp, arch_config, emul_list);
+	int tag = -1;
 	int i;
 
 	result = caml_alloc_tuple(17);
@@ -444,6 +445,9 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
 	Store_field(result, 15, tmp);
 
 #if defined(__i386__) || defined(__x86_64__)
+
+	tag = 1; /* tag x86 */
+
 	/*
 	 * emulation_flags: x86_arch_emulation_flags list;
 	 */
@@ -452,16 +456,17 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
 		(info->arch_config.emulation_flags);
 
 	/* xen_x86_arch_domainconfig */
-	x86_arch_config = caml_alloc_tuple(1);
-	Store_field(x86_arch_config, 0, emul_list);
+	arch_config = caml_alloc_tuple(1);
+	Field(arch_config, 0) = emul_list;
 
-	/* arch_config: arch_domainconfig */
-	arch_config = caml_alloc_small(1, 1);
-
-	Store_field(arch_config, 0, x86_arch_config);
-
-	Store_field(result, 16, arch_config);
 #endif
+	if (tag < 0)
+		caml_failwith("Unimplemented architecutre in alloc_domaininfo()");
+
+	/* arch_config: arch_domainconfig */
+	tmp = caml_alloc_small(1, tag);
+	Field(tmp, 0) = arch_config;
+	Field(result, 16) = tmp;
 
 	CAMLreturn(result);
 }
-- 
2.39.5

[PATCH 2/2] tools/ocaml: Fill arch_config for ARM in domain_getinfo{,list}()

[Andrew Cooper]

From: Julian Vetter <julian.vetter@vates.tech>

Add the missing ARM logic, populating xen_arm_arch_domainconfig
from the raw xc_domaininfo_t the same way the x86 branch does for
xen_x86_arch_domainconfig.

Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Andrii Sultanov <andriy.sultanov@vates.tech>
CC: Guillaume Thouvenin <guillaume.thouvenin@vates.tech>
CC: Julian Vetter <julian.vetter@vates.tech>
CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 441e1d83cfec..fb983709066f 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -444,9 +444,21 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
 
 	Store_field(result, 15, tmp);
 
-#if defined(__i386__) || defined(__x86_64__)
+#if defined(__arm__) || defined(__aarch64__)
 
-	tag = 1; /* tag x86 */
+	tag = 0; /* tag ARM */
+
+        /* xen_arm_arch_domainconfig */
+        arch_config = caml_alloc_tuple(3);
+        Field(arch_config, 0) = Val_int(info->arch_config.gic_version);
+        Field(arch_config, 1) = Val_int(info->arch_config.nr_spis);
+
+	tmp = caml_copy_int32(info->arch_config.clock_frequency);
+        Field(arch_config, 2) = tmp;
+
+#elif defined(__i386__) || defined(__x86_64__)
+
+        tag = 1; /* tag x86 */
 
 	/*
 	 * emulation_flags: x86_arch_emulation_flags list;
-- 
2.39.5

Re: [PATCH for-4.22 0/2] tools/ocaml: Fixes to Xenctrl.domain_getinfo{,list}()

[Oleksii Kurochko]

On 7/28/26 5:48 PM, Andrew Cooper wrote:
> For 4.22(?). This fixes a segfault in the Ocaml runtime on ARM.
> 
> It's not a regression vs 4.21, but it is deterministic and easy to fix.
> Technically patch 1 is sufficient to fix the segfault, but producing an
> unconditional exception is almost as useless.

For both patches:

Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

Re: [PATCH 2/2] tools/ocaml: Fill arch_config for ARM in domain_getinfo{,list}()

[Jan Beulich]

On 28.07.2026 17:48, Andrew Cooper wrote:
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -444,9 +444,21 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
>  
>  	Store_field(result, 15, tmp);
>  
> -#if defined(__i386__) || defined(__x86_64__)
> +#if defined(__arm__) || defined(__aarch64__)
>  
> -	tag = 1; /* tag x86 */
> +	tag = 0; /* tag ARM */
> +
> +        /* xen_arm_arch_domainconfig */
> +        arch_config = caml_alloc_tuple(3);
> +        Field(arch_config, 0) = Val_int(info->arch_config.gic_version);
> +        Field(arch_config, 1) = Val_int(info->arch_config.nr_spis);
> +
> +	tmp = caml_copy_int32(info->arch_config.clock_frequency);
> +        Field(arch_config, 2) = tmp;
> +
> +#elif defined(__i386__) || defined(__x86_64__)
> +
> +        tag = 1; /* tag x86 */
>  
>  	/*
>  	 * emulation_flags: x86_arch_emulation_flags list;

I know nothing about style rules or conventions here, but simply from
looking at the diff I came to wonder: Is this mix of tab vs space
indentation deliberate?

Jan

Re: [PATCH 2/2] tools/ocaml: Fill arch_config for ARM in domain_getinfo{,list}()

[Andrew Cooper]

On 28/07/2026 4:52 pm, Jan Beulich wrote:
> On 28.07.2026 17:48, Andrew Cooper wrote:
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -444,9 +444,21 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
>>  
>>  	Store_field(result, 15, tmp);
>>  
>> -#if defined(__i386__) || defined(__x86_64__)
>> +#if defined(__arm__) || defined(__aarch64__)
>>  
>> -	tag = 1; /* tag x86 */
>> +	tag = 0; /* tag ARM */
>> +
>> +        /* xen_arm_arch_domainconfig */
>> +        arch_config = caml_alloc_tuple(3);
>> +        Field(arch_config, 0) = Val_int(info->arch_config.gic_version);
>> +        Field(arch_config, 1) = Val_int(info->arch_config.nr_spis);
>> +
>> +	tmp = caml_copy_int32(info->arch_config.clock_frequency);
>> +        Field(arch_config, 2) = tmp;
>> +
>> +#elif defined(__i386__) || defined(__x86_64__)
>> +
>> +        tag = 1; /* tag x86 */
>>  
>>  	/*
>>  	 * emulation_flags: x86_arch_emulation_flags list;
> I know nothing about style rules or conventions here, but simply from
> looking at the diff I came to wonder: Is this mix of tab vs space
> indentation deliberate?

No, unintentional.  I'll fix up locally.

~Andrew

Re: [PATCH for-4.22 0/2] tools/ocaml: Fixes to Xenctrl.domain_getinfo{,list}()

[Andrew Cooper]

On 28/07/2026 4:50 pm, Oleksii Kurochko wrote:
>
>
> On 7/28/26 5:48 PM, Andrew Cooper wrote:
>> For 4.22(?). This fixes a segfault in the Ocaml runtime on ARM.
>>
>> It's not a regression vs 4.21, but it is deterministic and easy to fix.
>> Technically patch 1 is sufficient to fix the segfault, but producing an
>> unconditional exception is almost as useless.
>
> For both patches:
>
> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Thanks.  I've managed to get some testing in GitlabCI showing the segfault:

  https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/15583141354

and things working properly with this series in place:

  https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/15583735066

~Andrew

Re: [PATCH 1/2] tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM

[Jan Beulich]

On 28.07.2026 17:48, Andrew Cooper wrote:
> @@ -452,16 +456,17 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
>  		(info->arch_config.emulation_flags);
>  
>  	/* xen_x86_arch_domainconfig */
> -	x86_arch_config = caml_alloc_tuple(1);
> -	Store_field(x86_arch_config, 0, emul_list);
> +	arch_config = caml_alloc_tuple(1);
> +	Field(arch_config, 0) = emul_list;
>  
> -	/* arch_config: arch_domainconfig */
> -	arch_config = caml_alloc_small(1, 1);
> -
> -	Store_field(arch_config, 0, x86_arch_config);
> -
> -	Store_field(result, 16, arch_config);
>  #endif
> +	if (tag < 0)
> +		caml_failwith("Unimplemented architecutre in alloc_domaininfo()");

As I now ended up looking here as well (to determine whether this series will
want backporting): s/architecutre/architecture/ .

As to backporting: Both patches may want to have Fixes: tags?

Jan

Re: [PATCH 1/2] tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM

[Andrew Cooper]

On 29/07/2026 7:03 am, Jan Beulich wrote:
> On 28.07.2026 17:48, Andrew Cooper wrote:
>> @@ -452,16 +456,17 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
>>  		(info->arch_config.emulation_flags);
>>  
>>  	/* xen_x86_arch_domainconfig */
>> -	x86_arch_config = caml_alloc_tuple(1);
>> -	Store_field(x86_arch_config, 0, emul_list);
>> +	arch_config = caml_alloc_tuple(1);
>> +	Field(arch_config, 0) = emul_list;
>>  
>> -	/* arch_config: arch_domainconfig */
>> -	arch_config = caml_alloc_small(1, 1);
>> -
>> -	Store_field(arch_config, 0, x86_arch_config);
>> -
>> -	Store_field(result, 16, arch_config);
>>  #endif
>> +	if (tag < 0)
>> +		caml_failwith("Unimplemented architecutre in alloc_domaininfo()");
> As I now ended up looking here as well (to determine whether this series will
> want backporting): s/architecutre/architecture/ .

Fixed.  There was also a typo in the commit message.

> As to backporting: Both patches may want to have Fixes: tags?

Hmm, yes.

Technically, Fixes: 81838c9067ab ("ocaml: fix arm build") was the one
which caused the pointer not to be filled in, but it was a build fix for
Fixes: 9d683b5e375d ("tools/ocaml: Expose arch_config in domaininfo")
which was the main one intending to produce an arch_config

I guess I should go with both tags.

Patch 2 is interesting as well, because really it's both tags too.  In
my main first patch (if it had compiled), we would have ended up handing
an xen_x86_arch_domainconfig back when an ARM toolstack asked.

Fixed up locally.

~Andrew

Re: [PATCH 1/2] tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM

[Teddy Astie]

[-- Attachment #1.1.1: Type: text/plain, Size: 1165 bytes --]

Le 28/07/2026 à 17:49, Andrew Cooper a écrit :
> The Store_field(result, 16, arch_config) sits inside an ifdef x86, meaning
> that on other archtiectures the pointer is not filled in.  The Ocaml runtime
> then falls over a NULL pointer (really the Val_unit used to initialise
> 'result') when the layout in the heap doesn't match the type system.
> 
> Rearrange alloc_domaininfo() to avoid this.  Similarly to
> physinfo_arch_caps(), raise an exception if the architecture code hasn't
> filled in an appropriate tag.  Move the setup of arch_domainconfig to be
> common logic.
> 
> In order to simplify the addition of other architectures, remove the
> arch_config variable (resuing tmp as it's touched exactly once), and rename
> x86_arch_config to be arch_config so each architecture can fill in a suitable
> one without needing more local variables.
> 
> Reported-by: Julian Vetter <julian.vetter@vates.tech>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

I guess that requires the OCaml user side to be appropriately patched to 
not get data in the wrong order ?

Teddy

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [PATCH 2/2] tools/ocaml: Fill arch_config for ARM in domain_getinfo{,list}()

[Teddy Astie]

[-- Attachment #1.1.1: Type: text/plain, Size: 1885 bytes --]

Le 28/07/2026 à 17:50, Andrew Cooper a écrit :
> From: Julian Vetter <julian.vetter@vates.tech>
> 
> Add the missing ARM logic, populating xen_arm_arch_domainconfig
> from the raw xc_domaininfo_t the same way the x86 branch does for
> xen_x86_arch_domainconfig.
> 
> Signed-off-by: Julian Vetter <julian.vetter@vates.tech>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

> ---
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Andrii Sultanov <andriy.sultanov@vates.tech>
> CC: Guillaume Thouvenin <guillaume.thouvenin@vates.tech>
> CC: Julian Vetter <julian.vetter@vates.tech>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   tools/ocaml/libs/xc/xenctrl_stubs.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 441e1d83cfec..fb983709066f 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -444,9 +444,21 @@ static value alloc_domaininfo(xc_domaininfo_t * info)
>   
>   	Store_field(result, 15, tmp);
>   
> -#if defined(__i386__) || defined(__x86_64__)
> +#if defined(__arm__) || defined(__aarch64__)
>   
> -	tag = 1; /* tag x86 */
> +	tag = 0; /* tag ARM */
> +
> +        /* xen_arm_arch_domainconfig */
> +        arch_config = caml_alloc_tuple(3);
> +        Field(arch_config, 0) = Val_int(info->arch_config.gic_version);
> +        Field(arch_config, 1) = Val_int(info->arch_config.nr_spis);
> +
> +	tmp = caml_copy_int32(info->arch_config.clock_frequency);
> +        Field(arch_config, 2) = tmp;
> +
> +#elif defined(__i386__) || defined(__x86_64__)
> +
> +        tag = 1; /* tag x86 */
>   
>   	/*
>   	 * emulation_flags: x86_arch_emulation_flags list;

Teddy

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2489 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

Re: [PATCH 1/2] tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM

[Andrew Cooper]

On 29/07/2026 12:46 pm, Teddy Astie wrote:
> Le 28/07/2026 à 17:49, Andrew Cooper a écrit :
>> The Store_field(result, 16, arch_config) sits inside an ifdef x86,
>> meaning
>> that on other archtiectures the pointer is not filled in.  The Ocaml
>> runtime
>> then falls over a NULL pointer (really the Val_unit used to initialise
>> 'result') when the layout in the heap doesn't match the type system.
>>
>> Rearrange alloc_domaininfo() to avoid this.  Similarly to
>> physinfo_arch_caps(), raise an exception if the architecture code hasn't
>> filled in an appropriate tag.  Move the setup of arch_domainconfig to be
>> common logic.
>>
>> In order to simplify the addition of other architectures, remove the
>> arch_config variable (resuing tmp as it's touched exactly once), and
>> rename
>> x86_arch_config to be arch_config so each architecture can fill in a
>> suitable
>> one without needing more local variables.
>>
>> Reported-by: Julian Vetter <julian.vetter@vates.tech>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Teddy Astie <teddy.astie@vates.tech> 

Thanks.

> I guess that requires the OCaml user side to be appropriately patched
> to not get data in the wrong order ?

I don't quite understand the question.  There are no changes needed to
the OCaml side.  The problem is that this piece of C is not producing an
object that the type system describes.

Prior to this patch, Xenctrl.domain_getinfo on ARM produces a malformed
object with a missing (NULL-ish) interior pointer.  Attempts to
interpret this object in Ocaml code segfault.

With this patch, Xenctrl.domain_getinfo on ARM will unconditionally
raise Failure.  Ocaml code won't segfault, but the overall behaviour
isn't helpful.

With patch 2, Xenctrl.domain_getinfo on ARM gets you back a good object.

~Andrew