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

[Julian Vetter <julian.vetter@xxxxxxxxxx>]

On 7/30/26 11:38, Andrew Cooper wrote:
> 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.
> 

Aha, thank you for the clarification. Indeed the Ocaml/C manual 
suggested that this might be triggered by any caml_copy_* and not just a 
caml_alloc_shr().

I've tested this series (both patches applied together) on real Ampere 
ARM64 hardware (XCP-ng 9.0'ish, based on Xen 4.21.1). The crash in 
Xenctrl.domain_getinfo{,list} on ARM is gone, and VMs create and boot 
correctly. So,

Patch 1 ("tools/ocaml: Fix crash in Xenctrl.domain_getinfo{,list} on ARM"):
Reviewed-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
Tested-by: Julian Vetter <julian.vetter@xxxxxxxxxx>

Patch 2 ("tools/ocaml: Fill arch_config for ARM in 
domain_getinfo{,list}()"):
Tested-by: Julian Vetter <julian.vetter@xxxxxxxxxx>


> 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
> 



--
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech