From: "Andrew Jeffery" <andrew@aj.id.au>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>, qemu-arm@nongnu.org
Cc: "Joel Stanley" <joel@jms.id.au>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Richard Henderson" <richard.henderson@linaro.org>,
qemu-devel@nongnu.org, "Cédric Le Goater" <clg@kaod.org>
Subject: Re: [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ
Date: Tue, 03 Dec 2019 23:29:54 +1030 [thread overview]
Message-ID: <915ab8c8-5cd9-43fa-8abf-e1ea8c91c807@www.fastmail.com> (raw)
In-Reply-To: <fc320f0d-b2e2-c603-e5db-170f24df1149@redhat.com>
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 <andrew@aj.id.au>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> > 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), &arm_cpu_cfgend_property,
> > &error_abort);
> > +
> > + if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) {
> > + qdev_property_add_static(DEVICE(cpu), &arm_cpu_gt_cntfrq_property,
> > + &error_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, &local_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 frequencies where
> > + * `(NANOSECONDS_PER_SECOND % cpu->gt_cntfrq) > 0` holds. Failing to
> > + * provide an exact inverse leads to scheduling timers with negative
> > + * periods, which in turn leads to sticky behaviour in the guest.
> > + *
> > + * Finally, CNTFRQ is effectively capped at 1GHz to ensure our scale factor
> > + * cannot become zero.
> > + */
>
> This comment belong to the previous patch.
Sort of, but also sort of not. We don't expose the limitation until this patch as
NANOSECONDS_PER_SECOND is an integer multiple of GTIMER_SCALE, which
is what gt_cntfrq is set to until we add the property to configure it to arbitrary
values in this patch. So I added the comment in this patch rather than the
previous one which adds the code.
Andrew
next prev parent reply other threads:[~2019-12-03 13:06 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-03 4:14 [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Andrew Jeffery
2019-12-03 4:14 ` [PATCH v2 1/4] target/arm: Remove redundant scaling of nexttick Andrew Jeffery
2019-12-03 4:14 ` [PATCH v2 2/4] target/arm: Abstract the generic timer frequency Andrew Jeffery
2019-12-03 6:09 ` Philippe Mathieu-Daudé
2019-12-03 12:48 ` Andrew Jeffery
2019-12-03 17:27 ` Philippe Mathieu-Daudé
2019-12-05 5:04 ` Andrew Jeffery
2019-12-03 4:14 ` [PATCH v2 3/4] target/arm: Prepare generic timer for per-platform CNTFRQ Andrew Jeffery
2019-12-03 6:19 ` Philippe Mathieu-Daudé
2019-12-03 12:59 ` Andrew Jeffery [this message]
2019-12-03 4:14 ` [PATCH v2 4/4] ast2600: Configure CNTFRQ at 1125MHz Andrew Jeffery
2019-12-03 6:20 ` Philippe Mathieu-Daudé
2019-12-03 6:05 ` [PATCH v2 0/4] Expose GT CNTFRQ as a CPU property to support AST2600 Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=915ab8c8-5cd9-43fa-8abf-e1ea8c91c807@www.fastmail.com \
--to=andrew@aj.id.au \
--cc=clg@kaod.org \
--cc=joel@jms.id.au \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=richard.henderson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).