From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Andrew Jeffery <andrew@aj.id.au>, 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 2/4] target/arm: Abstract the generic timer frequency
Date: Tue, 3 Dec 2019 18:27:41 +0100 [thread overview]
Message-ID: <87bf3c1d-9e77-f4a1-1163-548a3cfee3bc@redhat.com> (raw)
In-Reply-To: <4e90d36d-aa13-441f-9298-56f83a5bff6a@www.fastmail.com>
On 12/3/19 1:48 PM, Andrew Jeffery wrote:
> On Tue, 3 Dec 2019, at 16:39, Philippe Mathieu-Daudé wrote:
>> On 12/3/19 5:14 AM, Andrew Jeffery wrote:
>>> Prepare for SoCs such as the ASPEED AST2600 whose firmware configures
>>> CNTFRQ to values significantly larger than the static 62.5MHz value
>>> currently derived from GTIMER_SCALE. As the OS potentially derives its
>>> timer periods from the CNTFRQ value the lack of support for running
>>> QEMUTimers at the appropriate rate leads to sticky behaviour in the
>>> guest.
>>>
>>> Substitute the GTIMER_SCALE constant with use of a helper to derive the
>>> period from gt_cntfrq stored in struct ARMCPU. Initially set gt_cntfrq
>>> to the frequency associated with GTIMER_SCALE so current behaviour is
>>> maintained.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>> target/arm/cpu.c | 2 ++
>>> target/arm/cpu.h | 10 ++++++++++
>>> target/arm/helper.c | 10 +++++++---
>>> 3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 7a4ac9339bf9..5698a74061bb 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -974,6 +974,8 @@ 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_reset_cbar_property =
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 83a809d4bac4..666c03871fdf 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -932,8 +932,18 @@ struct ARMCPU {
>>> */
>>> DECLARE_BITMAP(sve_vq_map, ARM_MAX_VQ);
>>> DECLARE_BITMAP(sve_vq_init, ARM_MAX_VQ);
>>> +
>>> + /* Generic timer counter frequency, in Hz */
>>> + uint64_t gt_cntfrq;
>>
>> You can also explicit the unit by calling it 'gt_cntfrq_hz'.
>
> Fair call, I'll fix that.
>
>>
>>> };
>>>
>>> +static inline unsigned int gt_cntfrq_period_ns(ARMCPU *cpu)
>>> +{
>>> + /* XXX: Could include qemu/timer.h to get NANOSECONDS_PER_SECOND? */
>>
>> Why inline this call? I doubt there is a significant performance gain.
>
> It wasn't so much performance. It started out as a macro for a simple calculation
> because I didn't want to duplicate it across a number of places, then I wanted type
> safety for the pointer so I switched the macro in the header to an inline function. So
> it is an evolution of the patch rather than something that came from an explicit goal
> of e.g. performance.
OK. Eventually NANOSECONDS_PER_SECOND will move to "qemu/units.h".
Should the XXX comment stay? I'm not sure, it is confusing.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
next prev parent reply other threads:[~2019-12-03 18:38 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é [this message]
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
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=87bf3c1d-9e77-f4a1-1163-548a3cfee3bc@redhat.com \
--to=philmd@redhat.com \
--cc=andrew@aj.id.au \
--cc=clg@kaod.org \
--cc=joel@jms.id.au \
--cc=peter.maydell@linaro.org \
--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).