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

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 arch_clock_frequency_hz field in
XEN_SYSCTL_physinfo, populated via arch_do_physinfo(), and mirroring how
the GIC capability bits were already moved there. The field is only set
when booting via Device Tree. Systems booting via ACPI get the frequency
from firmware/CNTFRQ directly and leave it 0, same as 'property not
present'.

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 bindins define as a single 32-bit cell. The
XEN_DOMCTL_INTERFACE_VERSION doesn't need to be bumped, because only a
previously zero'ed / ignored field is now used.

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 <[email protected]>
---
Changes in v3:
- Replaced arch_clock_frequency by arch_clock_frequency_hz to clearly
  specify which granularity the value has
- Updated commit message and comments in the code to...
  - ...mention why a 32bit value is "enough" for now
  - ...mention behaviour when a guest boots via ACPI
---
 tools/libs/light/libxl.c          |  1 +
 tools/libs/light/libxl_arm.c      | 13 ++++++++++++-
 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/vtimer.h |  3 +--
 xen/arch/arm/sysctl.c             |  3 +++
 xen/arch/arm/vtimer.c             |  4 +---
 xen/include/public/arch-arm.h     | 16 +---------------
 xen/include/public/sysctl.h       | 21 ++++++++++++++++++++-
 11 files changed, 41 insertions(+), 25 deletions(-)

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;
 
     GC_FREE;
     return 0;
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 926d651857..cf441f0737 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,15 @@ 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);
+    if (rc) {
+        LOG(ERROR, "failed to get physinfo");
+        libxl_physinfo_dispose(&info);
+        return ERROR_FAIL;
+    }
+    state->clock_frequency = info.arch_clock_frequency_hz;
+    libxl_physinfo_dispose(&info);
 
     return 0;
 }
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index a7893460f0..e24a3253f8 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", [
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;
 }
 
 type x86_arch_emulation_flags =
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index b396d5e615..d6d80ac55d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -711,7 +711,7 @@ int arch_domain_create(struct domain *d,
     if ( (rc = domain_vgic_init(d, config->arch.nr_spis)) != 0 )
         goto fail;
 
-    if ( (rc = domain_vtimer_init(d, &config->arch)) != 0 )
+    if ( (rc = domain_vtimer_init(d)) != 0 )
         goto fail;
 
     if ( (rc = tee_domain_init(d, config->arch.tee_type)) != 0 )
diff --git a/xen/arch/arm/include/asm/vtimer.h b/xen/arch/arm/include/asm/vtimer.h
index 9d4fb4c6e8..6bbfcf4e69 100644
--- a/xen/arch/arm/include/asm/vtimer.h
+++ b/xen/arch/arm/include/asm/vtimer.h
@@ -20,8 +20,7 @@
 #ifndef __ARCH_ARM_VTIMER_H__
 #define __ARCH_ARM_VTIMER_H__
 
-extern int domain_vtimer_init(struct domain *d,
-                              struct xen_arch_domainconfig *config);
+extern int domain_vtimer_init(struct domain *d);
 extern int vcpu_vtimer_init(struct vcpu *v);
 extern bool vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr);
 extern void virt_timer_save(struct vcpu *v);
diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
index 3b0edf4cec..060132dabb 100644
--- a/xen/arch/arm/sysctl.c
+++ b/xen/arch/arm/sysctl.c
@@ -15,6 +15,7 @@
 
 #include <asm/arm64/sve.h>
 #include <asm/gic.h>
+#include <asm/time.h>
 
 #include <public/sysctl.h>
 
@@ -25,6 +26,8 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
     pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
                                        XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
 
+    pi->arch_clock_frequency_hz = timer_dt_clock_frequency;
+
     /*
      * The GIC version(s) we're happy creating guests with.  Right now for
      * simplicity it is tied to the active hardware version, but this will
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 2e85ff2b6e..18f5676158 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -52,7 +52,7 @@ static void virt_timer_expired(void *data)
     perfc_incr(vtimer_virt_inject);
 }
 
-int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
+int domain_vtimer_init(struct domain *d)
 {
     d->arch.virt_timer_base.offset = get_cycles();
     d->arch.virt_timer_base.nanoseconds =
@@ -60,8 +60,6 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
     d->time_offset.seconds = d->arch.virt_timer_base.nanoseconds;
     do_div(d->time_offset.seconds, 1000000000);
 
-    config->clock_frequency = timer_dt_clock_frequency;
-
     /*
      * Per the ACPI specification, providing a secure EL1 timer
      * interrupt is optional and will be ignored by non-secure OS.
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 6987f5bdf4..b88c61c8ff 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -334,7 +334,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 #define XEN_DOMCTL_CONFIG_ARM_V8R_EL1_MSA_VMSA    2
 
 struct xen_arch_domainconfig {
-    /* IN/OUT */
+    /* IN */
     uint8_t gic_version;
     /* IN - Contains SVE vector length divided by 128 */
     uint8_t sve_vl;
@@ -342,20 +342,6 @@ struct xen_arch_domainconfig {
     uint16_t tee_type;
     /* IN */
     uint32_t nr_spis;
-    /*
-     * OUT
-     * Based on the property clock-frequency in the DT timer node.
-     * The property may be present when the bootloader/firmware doesn't
-     * set correctly CNTFRQ which hold the timer frequency.
-     *
-     * As it's not possible to trap this register, we have to replicate
-     * the value in the guest DT.
-     *
-     * = 0 => property not present
-     * > 0 => Value of the property
-     *
-     */
-    uint32_t clock_frequency;
     /* IN */
     uint8_t arm_sci_type;
     /* IN */
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index d20ebf3644..7d051bf14f 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -120,7 +120,26 @@ struct xen_sysctl_physinfo {
     uint32_t cpu_khz;
     uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */
     uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */
-    uint32_t pad;
+    /*
+     * ARM only, and only set when booting via Device Tree. Value, in Hz, of
+     * the "clock-frequency" property in the DT timer node. The property may
+     * be present when the bootloader/firmware doesn't correctly set CNTFRQ
+     * to hold the timer frequency.
+     *
+     * As it's not possible to trap this register, we have to replicate the
+     * value in the guest DT.
+     *
+     * This field mirrors the DT property (a single <u32> cell), independent of
+     * CNTFRQ_EL0's own width or any given implementation's actual frequency.
+     * Should the DT binding ever grow a wider encoding, this field would need
+     * to grow (or gain a companion) to match, same as any other ABI change.
+     *
+     * = 0 => property not present, non-ARM, or booting via ACPI (ACPI
+     *        guests get the frequency from firmware/CNTFRQ instead, so
+     *        there is nothing to replicate).
+     * > 0 => Value of the property, in Hz.
+     */
+    uint32_t arch_clock_frequency_hz;
     uint64_aligned_t total_pages;
     uint64_aligned_t free_pages;
     uint64_aligned_t scrub_pages;
-- 
2.53.0



-- 
Julian Vetter | Vates Hypervisor & Kernel Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech