Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ
On Tue, 3 Dec 2019, at 16:49, Philippe Mathieu-Daudé wrote:
> On 12/3/19 5:14 AM, Andrew Jeffery wrote:
> > The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On
> > recent firmwares this is at 1125MHz, which is considerably quicker than
> > the assumed 62.5MHz of the current generic timer implementation. The
> > delta between the value as read from CNTFRQ and the true rate of the
> > underlying QEMUTimer leads to sticky behaviour in AST2600 guests.
> >
> > Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the
> > generic timer. This allows platforms to configure CNTFRQ (and the
> > associated QEMUTimer) to the appropriate frequency prior to starting the
> > guest.
> >
> > As the platform can now determine the rate of CNTFRQ we're exposed to
> > limitations of QEMUTimer that didn't previously materialise: In the
> > course of emulation we need to arbitrarily and accurately convert
> > between guest ticks and time, but we're constrained by QEMUTimer's use
> > of an integer scaling factor. The effect is QEMUTimer cannot exactly
> > capture the period of frequencies that do not cleanly divide
> > NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an
> > equally inaccurate scaling factor for scaling time to ticks so at least
> > a self-consistent inverse relationship holds.
> >
> > Signed-off-by: Andrew Jeffery
> > Reviewed-by: Richard Henderson
> > ---
> > target/arm/cpu.c| 43 +--
> > target/arm/cpu.h| 18 ++
> > target/arm/helper.c | 9 -
> > 3 files changed, 59 insertions(+), 11 deletions(-)
> >
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 5698a74061bb..f186019a77fd 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -974,10 +974,12 @@ static void arm_cpu_initfn(Object *obj)
> > if (tcg_enabled()) {
> > cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
> > }
> > -
> > -cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE;
> > }
> >
> > +static Property arm_cpu_gt_cntfrq_property =
> > +DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq,
> > + NANOSECONDS_PER_SECOND / GTIMER_SCALE);
> > +
> > static Property arm_cpu_reset_cbar_property =
> > DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
> >
> > @@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj)
> >
> > qdev_property_add_static(DEVICE(obj), _cpu_cfgend_property,
> >_abort);
> > +
> > +if (arm_feature(>env, ARM_FEATURE_GENERIC_TIMER)) {
> > +qdev_property_add_static(DEVICE(cpu), _cpu_gt_cntfrq_property,
> > + _abort);
> > +}
> > }
> >
> > static void arm_cpu_finalizefn(Object *obj)
> > @@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev,
> > Error **errp)
> > }
> > }
> >
> > -cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL,
> > GTIMER_SCALE,
> > - arm_gt_ptimer_cb, cpu);
> > -cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL,
> > GTIMER_SCALE,
> > - arm_gt_vtimer_cb, cpu);
> > -cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> > - arm_gt_htimer_cb, cpu);
> > -cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
> > - arm_gt_stimer_cb, cpu);
> > +
> > +{
> > +uint64_t scale;
>
> Apparently you have to use this odd indent due to the '#ifndef
> CONFIG_USER_ONLY'. Well, acceptable.
It's the indent associated with the block scope for the scale variable to limit
its lifetime
to where I needed it.
>
> > +
> > +if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
> > +if (!cpu->gt_cntfrq) {
> > +error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
> > + cpu->gt_cntfrq);
> > +return;
> > +}
> > +scale = gt_cntfrq_period_ns(cpu);
> > +} else {
> > +scale = GTIMER_SCALE;
> > +}
> > +
> > +cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > + arm_gt_ptimer_cb, cpu);
> > +cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > + arm_gt_vtimer_cb, cpu);
> > +cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > + arm_gt_htimer_cb, cpu);
> > +cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
> > + arm_gt_stimer_cb, cpu);
> > +}
> > #endif
> >
> > cpu_exec_realizefn(cs, _err);
> > diff --git
Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ
On 12/3/19 5:14 AM, Andrew Jeffery wrote:
The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On
recent firmwares this is at 1125MHz, which is considerably quicker than
the assumed 62.5MHz of the current generic timer implementation. The
delta between the value as read from CNTFRQ and the true rate of the
underlying QEMUTimer leads to sticky behaviour in AST2600 guests.
Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the
generic timer. This allows platforms to configure CNTFRQ (and the
associated QEMUTimer) to the appropriate frequency prior to starting the
guest.
As the platform can now determine the rate of CNTFRQ we're exposed to
limitations of QEMUTimer that didn't previously materialise: In the
course of emulation we need to arbitrarily and accurately convert
between guest ticks and time, but we're constrained by QEMUTimer's use
of an integer scaling factor. The effect is QEMUTimer cannot exactly
capture the period of frequencies that do not cleanly divide
NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an
equally inaccurate scaling factor for scaling time to ticks so at least
a self-consistent inverse relationship holds.
Signed-off-by: Andrew Jeffery
Reviewed-by: Richard Henderson
---
target/arm/cpu.c| 43 +--
target/arm/cpu.h| 18 ++
target/arm/helper.c | 9 -
3 files changed, 59 insertions(+), 11 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5698a74061bb..f186019a77fd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -974,10 +974,12 @@ static void arm_cpu_initfn(Object *obj)
if (tcg_enabled()) {
cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
}
-
-cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE;
}
+static Property arm_cpu_gt_cntfrq_property =
+DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq,
+ NANOSECONDS_PER_SECOND / GTIMER_SCALE);
+
static Property arm_cpu_reset_cbar_property =
DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
@@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj)
qdev_property_add_static(DEVICE(obj), _cpu_cfgend_property,
_abort);
+
+if (arm_feature(>env, ARM_FEATURE_GENERIC_TIMER)) {
+qdev_property_add_static(DEVICE(cpu), _cpu_gt_cntfrq_property,
+ _abort);
+}
}
static void arm_cpu_finalizefn(Object *obj)
@@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
**errp)
}
}
-cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_ptimer_cb, cpu);
-cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_vtimer_cb, cpu);
-cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_htimer_cb, cpu);
-cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_stimer_cb, cpu);
+
+{
+uint64_t scale;
Apparently you have to use this odd indent due to the '#ifndef
CONFIG_USER_ONLY'. Well, acceptable.
+
+if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
+if (!cpu->gt_cntfrq) {
+error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
+ cpu->gt_cntfrq);
+return;
+}
+scale = gt_cntfrq_period_ns(cpu);
+} else {
+scale = GTIMER_SCALE;
+}
+
+cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_ptimer_cb, cpu);
+cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_vtimer_cb, cpu);
+cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_htimer_cb, cpu);
+cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_stimer_cb, cpu);
+}
#endif
cpu_exec_realizefn(cs, _err);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 666c03871fdf..0bcd13dcac81 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -939,6 +939,24 @@ struct ARMCPU {
static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
{
+/*
+ * The exact approach to calculating guest ticks is:
+ *
+ * muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), cpu->gt_cntfrq,
+ * NANOSECONDS_PER_SECOND);
+ *
+ * We don't do that. Rather we intentionally use integer division
+ * truncation below and in the caller for the conversion of host monotonic
+ * time to
[PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ
The ASPEED AST2600 clocks the generic timer at the rate of HPLL. On
recent firmwares this is at 1125MHz, which is considerably quicker than
the assumed 62.5MHz of the current generic timer implementation. The
delta between the value as read from CNTFRQ and the true rate of the
underlying QEMUTimer leads to sticky behaviour in AST2600 guests.
Add a feature-gated property exposing CNTFRQ for ARM CPUs providing the
generic timer. This allows platforms to configure CNTFRQ (and the
associated QEMUTimer) to the appropriate frequency prior to starting the
guest.
As the platform can now determine the rate of CNTFRQ we're exposed to
limitations of QEMUTimer that didn't previously materialise: In the
course of emulation we need to arbitrarily and accurately convert
between guest ticks and time, but we're constrained by QEMUTimer's use
of an integer scaling factor. The effect is QEMUTimer cannot exactly
capture the period of frequencies that do not cleanly divide
NANOSECONDS_PER_SECOND for scaling ticks to time. As such, provide an
equally inaccurate scaling factor for scaling time to ticks so at least
a self-consistent inverse relationship holds.
Signed-off-by: Andrew Jeffery
Reviewed-by: Richard Henderson
---
target/arm/cpu.c| 43 +--
target/arm/cpu.h| 18 ++
target/arm/helper.c | 9 -
3 files changed, 59 insertions(+), 11 deletions(-)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 5698a74061bb..f186019a77fd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -974,10 +974,12 @@ static void arm_cpu_initfn(Object *obj)
if (tcg_enabled()) {
cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
}
-
-cpu->gt_cntfrq = NANOSECONDS_PER_SECOND / GTIMER_SCALE;
}
+static Property arm_cpu_gt_cntfrq_property =
+DEFINE_PROP_UINT64("cntfrq", ARMCPU, gt_cntfrq,
+ NANOSECONDS_PER_SECOND / GTIMER_SCALE);
+
static Property arm_cpu_reset_cbar_property =
DEFINE_PROP_UINT64("reset-cbar", ARMCPU, reset_cbar, 0);
@@ -1174,6 +1176,11 @@ void arm_cpu_post_init(Object *obj)
qdev_property_add_static(DEVICE(obj), _cpu_cfgend_property,
_abort);
+
+if (arm_feature(>env, ARM_FEATURE_GENERIC_TIMER)) {
+qdev_property_add_static(DEVICE(cpu), _cpu_gt_cntfrq_property,
+ _abort);
+}
}
static void arm_cpu_finalizefn(Object *obj)
@@ -1253,14 +1260,30 @@ static void arm_cpu_realizefn(DeviceState *dev, Error
**errp)
}
}
-cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_ptimer_cb, cpu);
-cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_vtimer_cb, cpu);
-cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_htimer_cb, cpu);
-cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, GTIMER_SCALE,
- arm_gt_stimer_cb, cpu);
+
+{
+uint64_t scale;
+
+if (arm_feature(env, ARM_FEATURE_GENERIC_TIMER)) {
+if (!cpu->gt_cntfrq) {
+error_setg(errp, "Invalid CNTFRQ: %"PRId64"Hz",
+ cpu->gt_cntfrq);
+return;
+}
+scale = gt_cntfrq_period_ns(cpu);
+} else {
+scale = GTIMER_SCALE;
+}
+
+cpu->gt_timer[GTIMER_PHYS] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_ptimer_cb, cpu);
+cpu->gt_timer[GTIMER_VIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_vtimer_cb, cpu);
+cpu->gt_timer[GTIMER_HYP] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_htimer_cb, cpu);
+cpu->gt_timer[GTIMER_SEC] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
+ arm_gt_stimer_cb, cpu);
+}
#endif
cpu_exec_realizefn(cs, _err);
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 666c03871fdf..0bcd13dcac81 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -939,6 +939,24 @@ struct ARMCPU {
static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
{
+/*
+ * The exact approach to calculating guest ticks is:
+ *
+ * muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), cpu->gt_cntfrq,
+ * NANOSECONDS_PER_SECOND);
+ *
+ * We don't do that. Rather we intentionally use integer division
+ * truncation below and in the caller for the conversion of host monotonic
+ * time to guest ticks to provide the exact inverse for the semantics of
+ * the QEMUTimer scale factor. QEMUTimer's scale facter is an integer, so
+ * it loses precision when representing
