Re: [PATCH v5 5/6] xen/arm: report clock_frequency via sysctl physinfo, not createdomain

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Sep 11, 2026 at 02:47:35PM +0200, Julian Vetter wrote:
> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
> index a1fe16274d..ec7e6d3f65 100644
> --- a/tools/libs/light/libxl.c
> +++ b/tools/libs/light/libxl.c
> @@ -410,6 +410,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo 
> *physinfo)
>      physinfo->cap_gnttab_v2 =
>          !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_gnttab_v2);
>      physinfo->arch_capabilities = xcphysinfo.arch_capabilities;
> +    physinfo->arch_clock_frequency_hz = xcphysinfo.arch_clock_frequency_hz;

That new field name doesn't really make sense. How a clock can be arch
specific? Which clock, I'm sure they can be many?
(also a comment about that new field in `libxl_physinfo` which is said
to be ARM only, which doesn't make sense because every CPU architectures
needs a clock, or many).

Anyway, if the name is acceptable in the hypervisor public interfaces,
so be it.

> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 283cfb749b..3d232040c9 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -252,6 +252,9 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
>                                     libxl__domain_build_state *state,
>                                     const struct xen_domctl_createdomain 
> *config)
>  {
> +    libxl_physinfo info;
> +    int rc;
> +
>      switch (config->arch.gic_version) {
>      case XEN_DOMCTL_CONFIG_GIC_V2:
>          d_config->b_info.arch_arm.gic_version = LIBXL_GIC_VERSION_V2;
> @@ -264,7 +267,23 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
>          return ERROR_FAIL;
>      }
>  
> -    state->clock_frequency = config->arch.clock_frequency;
> +    libxl_physinfo_init(&info);
> +    rc = libxl_get_physinfo(CTX, &info);

Could you move both call to the beginning of the function? It might be
useful to more than just the new code.

> +    if (rc) {
> +        LOG(ERROR, "failed to get physinfo");
> +        libxl_physinfo_dispose(&info);

It would be better if there's only a single call of this function
throughout the function, and so only a single error path.

Could you an "out" label at the end a of function, and ..

> +        return ERROR_FAIL;

.. this and the `_dispose()` call can be replace by `goto out`;

No need to change the value of `rc`, it's already set by a libxl
function so it can be returned.

> +    }
> +    /*
> +     * Pass the timer frequency on to the guest DT only when Xen took it from
> +     * the host DT (XEN_SYSCTL_PHYSCAP_ARM_TIMER_DT_FREQ). Otherwise the 
> guest
> +     * gets the right value from CNTFRQ_EL0.
> +     */
> +    if (arch_capabilities_arm_timer_dt_freq(info.arch_capabilities))
> +        state->clock_frequency = info.arch_clock_frequency_hz;
> +    else
> +        state->clock_frequency = 0;

Add here:

    rc = 0;
out:

> +    libxl_physinfo_dispose(&info);
>  
>      return 0;

Then replace the return by `return rc`;

Then, replace every `return X` in the function by
    rc = X;
    goto out;

That error handling style is used in many places in libxl.

>  }
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index 8699ab3013..e2e4be7323 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -1201,6 +1201,7 @@ libxl_physinfo = Struct("physinfo", [
>      ("cap_gnttab_v1", bool),
>      ("cap_gnttab_v2", bool),
>      ("arch_capabilities", uint32),
> +    ("arch_clock_frequency_hz", uint32), # ARM only
>      ], dir=DIR_OUT)
>  
>  libxl_connectorinfo = Struct("connectorinfo", [

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech