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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/07/2026 9:43 am, Julian Vetter wrote:
>
> On 7/28/26 17:48, Andrew Cooper wrote:
>> From: Julian Vetter <julian.vetter@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> CC: Andrii Sultanov <andriy.sultanov@xxxxxxxxxx>
>> CC: Guillaume Thouvenin <guillaume.thouvenin@xxxxxxxxxx>
>> CC: Julian Vetter <julian.vetter@xxxxxxxxxx>
>> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>> ---
>>   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