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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 28-Sep-26 14:20, Julian Vetter wrote:
> The xen_arch_domainconfig.clock_frequency value is populated in
> domain_vtimer_init() during XEN_DOMCTL_createdomain from the global
> timer_dt_clock_frequency, which comes from the host's DT timer node and
> has nothing to do with the domain being created. Like now removed
> GIC_NATIVE resolution, this is a host-wide system property being
> smuggled out through a domain-creation IN struct.
> 
> Expose it instead as a new arm_clock_frequency_hz field in
> XEN_SYSCTL_physinfo, populated via arch_do_physinfo(), and mirroring how
> the GIC capability bits were already moved there.
> 
> Rather than making the field dependant on DT boot, make Xen always
> report the timer frequency, either via the DT "clock-frequency" node, or
> directly via CNTFRQ_EL0. preinit_xen_time() already computes cpu_khz for
> every boot path. The renamed timer_clock_frequency_hz now captures
> whichever of the two produced that value, in full Hz precision, instead
> of only recording the DT case. So, ACPI guests get a real value too,
> allowing to drop the special case.
> 
> The DT "clock-frequency" property exists because firmware might leave
> CNTFRQ_EL0 wrong, and since CNTFRQ_EL0 cannot be trapped the only fix is
> to replicate the correct value into the guest DT. To keep that signal, a
> new XEN_SYSCTL_PHYSCAP_ARM_TIMER_DT_FREQ capability bit records whether
> arm_clock_frequency_hz came from the DT property. Then libxl only emits
> a "clock-frequency" property into the guest timer node when that bit is
> set. A guest whose CNTFRQ_EL0 is already correct keeps an unmodified
> timer node, exactly as before.
> 
> Although the CNTFRQ_EL0 register is 64 bits wide, and some current timer
> implementations run at 1GHz, a 32bit value is sufficient to store the
> timer value, because it only mirrors the DT 'clock-frequency' property,
> which the bindings define as a single 32-bit cell.
> 
> In struct xen_sysctl_physinfo the new field just reuses the former pad
> word, so sysctl consumers are unaffected. struct xen_arch_domainconfig
> however loses clock_frequency from its middle, which shrinks the struct
> and shifts every field after it, so bump XEN_DOMCTL_INTERFACE_VERSION.
> 
> The xen_arch_domainconfig parameter passed to domain_vtimer_init() is no
> longer needed, so drop that parameter entirely. libxl now fetches the
> frequency via libxl_get_physinfo() in libxl__arch_domain_save_config()
> instead of reading it back out of the createdomain reply. The OCaml
> xen_arch_domainconfig mirror drops the field too.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
This can stay (you still need here and in patch 2 acks from others) provided you
fix the below remarks.

> ---
> Changes in v6:
> - Rename arch_clock_frequency_hz to arm_clock_frequency_hz
> - Drop now-redundant "# ARM only" comment on the libxl IDL entry
> - Rework libxl__arch_domain_save_config() to a single exit point: fetch
>   physinfo up front and use "goto out" on every error path
> - Move the gic_version IN/OUT -> IN comment update to the GIC_NATIVE
>   removal patch
> - Added Reviewed-by
> ---
>  .../include/xen-tools/arm-arch-capabilities.h | 10 +++++++
>  tools/libs/light/libxl.c                      |  1 +
>  tools/libs/light/libxl_arm.c                  | 29 +++++++++++++++++--
>  tools/libs/light/libxl_types.idl              |  1 +
>  tools/ocaml/libs/xc/xenctrl.ml                |  1 -
>  tools/ocaml/libs/xc/xenctrl.mli               |  1 -
>  xen/arch/arm/domain.c                         |  2 +-
>  xen/arch/arm/include/asm/time.h               |  7 +++--
>  xen/arch/arm/include/asm/vtimer.h             |  3 +-
>  xen/arch/arm/sysctl.c                         |  5 ++++
>  xen/arch/arm/time.c                           |  9 ++++--
>  xen/arch/arm/vtimer.c                         |  4 +--
>  xen/include/public/arch-arm.h                 | 14 ---------
>  xen/include/public/domctl.h                   |  4 +--
>  xen/include/public/sysctl.h                   | 20 ++++++++++++-
>  15 files changed, 77 insertions(+), 34 deletions(-)
> 
> diff --git a/tools/include/xen-tools/arm-arch-capabilities.h 
> b/tools/include/xen-tools/arm-arch-capabilities.h
> index 21e3c73bd1..a927bc4703 100644
> --- a/tools/include/xen-tools/arm-arch-capabilities.h
> +++ b/tools/include/xen-tools/arm-arch-capabilities.h
> @@ -46,4 +46,14 @@ bool arch_capabilities_arm_gic_v3(unsigned int 
> arch_capabilities)
>  #endif
>  }
>  
> +static inline
> +bool arch_capabilities_arm_timer_dt_freq(unsigned int arch_capabilities)
> +{
> +#if defined(__arm__) || defined(__aarch64__)
> +    return MASK_EXTR(arch_capabilities, 
> XEN_SYSCTL_PHYSCAP_ARM_TIMER_DT_FREQ);
> +#else
> +    return false;
> +#endif
> +}
> +
>  #endif /* ARM_ARCH_CAPABILITIES_H */
> diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
> index a1fe16274d..98c7f0fdb3 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->arm_clock_frequency_hz = xcphysinfo.arm_clock_frequency_hz;
>  
>      GC_FREE;
>      return 0;
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index b9c0c820d6..47b662ea5c 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -252,6 +252,16 @@ 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;
> +
> +    libxl_physinfo_init(&info);
> +    rc = libxl_get_physinfo(CTX, &info);
> +    if (rc) {
> +        LOG(ERROR, "failed to get physinfo");
> +        goto out;
> +    }
> +
>      switch (config->arch.gic_version) {
>      case XEN_DOMCTL_CONFIG_GIC_V2:
>          d_config->b_info.arch_arm.gic_version = LIBXL_GIC_VERSION_V2;
> @@ -261,12 +271,25 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
>          break;
>      default:
>          LOG(ERROR, "Unexpected gic version %u", config->arch.gic_version);
> -        return ERROR_FAIL;
> +        rc = ERROR_FAIL;
> +        goto out;
>      }
>  
> -    state->clock_frequency = config->arch.clock_frequency;
> +    /*
> +     * 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.arm_clock_frequency_hz;
> +    else
> +        state->clock_frequency = 0;
>  
> -    return 0;
> +    rc = 0;
> +out:
> +    libxl_physinfo_dispose(&info);
> +
> +    return rc;
>  }
>  
>  int libxl__arch_domain_create(libxl__gc *gc,
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index a7893460f0..4284a4f577 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),
> +    ("arm_clock_frequency_hz", uint32),
The golang bindings need regenerating I think.

>      ], dir=DIR_OUT)
>  
>  libxl_connectorinfo = Struct("connectorinfo", [
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 147afa62c2..582897af6d 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -32,7 +32,6 @@ type xen_arm_arch_domainconfig =
>    {
>      gic_version: int;
>      nr_spis: int;
> -    clock_frequency: int32;
>    }
>  
>  type x86_arch_emulation_flags =
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index 9fccb2c2c2..9414b87164 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -26,7 +26,6 @@ type vcpuinfo = {
>  type xen_arm_arch_domainconfig = {
>    gic_version: int;
>    nr_spis: int;
> -  clock_frequency: int32;
>  }
OCaml stubs fail to build on Arm because you haven't updated the C side
(alloc_domaininfo() in xenctlr_stubs.c).

~Michal