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

On 30/07/2026 9:43 am, Julian Vetter wrote:
>
> On 7/28/26 17:48, Andrew Cooper wrote:
>> From: Julian Vetter <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Andrew Cooper <[email protected]>
>> ---
>> CC: Andrew Cooper <[email protected]>
>> CC: Andrii Sultanov <[email protected]>
>> CC: Guillaume Thouvenin <[email protected]>
>> CC: Julian Vetter <[email protected]>
>> CC: Oleksii Kurochko <[email protected]>
>> ---
>>   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;
> Shouldn't the Field() be a `Store_field(arch_config, 2, tmp);`, because 
> the `caml_copy_int32` makes an allocation on the minor heap to allow 
> OCAML's GC to keep track of this pointer if ever the arch_config is 
> promoted?

No (ish).  The Ocaml/C manual is subtly wrong in it's advice about this.

Using Store_field() would be safe, but it has an obscene overhead for a
scalar store, and caml_modify() does a whole bunch of barriers/atomics
which are not necessary on anything which isn't allocated from
caml_alloc_shr() (which we don't use at all).

The buggy construct is:

    Field(foo, bar) = caml_{copy,alloc,etc}(baz);

because C does not provide an ordering between evaluation of the Rvalue
vs the Lvalue.  Specifically, if the Lvalue were evaluated prior to the
function call, then indeed we could store into a stale pointer.

But, using tmp in the way we do:

    tmp = caml_copy_*(baz);
    Field(foo, bar) = tmp;

puts a sequence point after calling caml_copy_*(), and prior to
evaluating Field(), so guarantees to do them in the correct order.

In the Store_field() macro, this occurs by taking (val) into a local
variable, and not because of how it calls caml_modify().


In a perfect world we'd have a Store_field() that arranged for the
intermediate safety without calling caml_modify().

It is also telling that the Ocaml runtime has a total of 15 uses of
Store_field, and we've got more than that in this function alone, let
alone the rest of our bindings.

I do have a cleanup patch for those as well, once we got this bug sorted.

~Andrew