mbox series

[0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

Message ID 20240419184608.2675213-1-peter.maydell@linaro.org
Headers show
Series target/arm: Make the counter frequency default 1GHz for new CPUs, machines | expand

Message

Peter Maydell April 19, 2024, 6:46 p.m. UTC
In previous versions of the Arm architecture, the frequency of the
generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
In Armv8.6, the architecture standardized this frequency to 1GHz.

Because there is no ID register feature field that indicates whether a
CPU is v8.6 or that it ought to have this counter frequency, we
implement this by changing our default CNTFRQ value for all CPUs, with
exceptions for backwards compatibility:

 * CPU types which we already implement will retain the old
   default value. None of these are v8.6 CPUs, so this is
   architecturally OK.
 * CPUs used in versioned machine types with a version of 9.0
   or earlier will retain the old default value.

The upshot is that the only CPU type that changes is 'max'; but any
new type we add in future (whether v8.6 or not) will also get the new
1GHz default (assuming we spot in code review any attempts to set
the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
of cut-n-paste from an older CPU initfn ;-)).

It remains the case that the machine model can override the default
value via the 'cntfrq' QOM property (regardless of the CPU type).

Patch 1 is Paolo's "add the new versioned machine types" patch that
he sent out last month; patch 2 is some preliminary cleanup so that
we set the default cntfrq value in exactly one place, and patch 3
is the mechanics to set the default appropriately for the two
back-compat scenarios.

thanks
-- PMM

Paolo Bonzini (1):
  hw: Add compat machines for 9.1

Peter Maydell (2):
  target/arm: Refactor default generic timer frequency handling
  target/arm: Default to 1GHz cntfrq for 'max' and new CPUs

 include/hw/boards.h        |  3 +++
 include/hw/i386/pc.h       |  3 +++
 target/arm/cpu.h           | 11 +++++++++
 target/arm/internals.h     | 15 +++++++++---
 hw/arm/virt.c              | 11 +++++++--
 hw/core/machine.c          |  5 ++++
 hw/i386/pc.c               |  3 +++
 hw/i386/pc_piix.c          | 17 +++++++++++---
 hw/i386/pc_q35.c           | 14 ++++++++++--
 hw/m68k/virt.c             | 11 +++++++--
 hw/ppc/spapr.c             | 17 +++++++++++---
 hw/s390x/s390-virtio-ccw.c | 14 +++++++++++-
 target/arm/cpu.c           | 47 ++++++++++++++++++++++++++------------
 target/arm/cpu64.c         |  2 ++
 target/arm/helper.c        | 16 ++++++-------
 target/arm/tcg/cpu32.c     |  4 ++++
 target/arm/tcg/cpu64.c     | 18 +++++++++++++++
 17 files changed, 173 insertions(+), 38 deletions(-)

Comments

Peter Maydell April 22, 2024, 12:56 p.m. UTC | #1
On Fri, 19 Apr 2024 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> In previous versions of the Arm architecture, the frequency of the
> generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
> and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
> In Armv8.6, the architecture standardized this frequency to 1GHz.
>
> Because there is no ID register feature field that indicates whether a
> CPU is v8.6 or that it ought to have this counter frequency, we
> implement this by changing our default CNTFRQ value for all CPUs, with
> exceptions for backwards compatibility:
>
>  * CPU types which we already implement will retain the old
>    default value. None of these are v8.6 CPUs, so this is
>    architecturally OK.
>  * CPUs used in versioned machine types with a version of 9.0
>    or earlier will retain the old default value.
>
> The upshot is that the only CPU type that changes is 'max'; but any
> new type we add in future (whether v8.6 or not) will also get the new
> 1GHz default (assuming we spot in code review any attempts to set
> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
> of cut-n-paste from an older CPU initfn ;-)).
>
> It remains the case that the machine model can override the default
> value via the 'cntfrq' QOM property (regardless of the CPU type).

This patchset turns out to break the sbsa-ref board: the
Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
avocado test both (a) takes rather longer to boot and (b) when
running thinks that time is advancing very fast.

I suspect this may be because the firmware hard-codes the
generic timer clock frequency it is expecting. I've cc'd the
sbsa-ref maintainers: is that correct?

If so, we can deal with it by making the sbsa-ref board set the
cntfrq QOM property on its CPUs to force them to the old
frequency. However this will produce a technically-out-of-spec
CPU when used with a v8.6-compliant CPU type, so probably we
should do something to be able to tell the firmware the clock
frequency (if it doesn't want to just read CNTFRQ_EL0 itself).

thanks
-- PMM
Marcin Juszkiewicz April 22, 2024, 1:38 p.m. UTC | #2
W dniu 22.04.2024 o 14:56, Peter Maydell pisze:
> On Fri, 19 Apr 2024 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:

>> The upshot is that the only CPU type that changes is 'max'; but any
>> new type we add in future (whether v8.6 or not) will also get the new
>> 1GHz default (assuming we spot in code review any attempts to set
>> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
>> of cut-n-paste from an older CPU initfn ;-)).
>>
>> It remains the case that the machine model can override the default
>> value via the 'cntfrq' QOM property (regardless of the CPU type).
> 
> This patchset turns out to break the sbsa-ref board: the
> Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
> avocado test both (a) takes rather longer to boot and (b) when
> running thinks that time is advancing very fast.
> 
> I suspect this may be because the firmware hard-codes the
> generic timer clock frequency it is expecting. I've cc'd the
> sbsa-ref maintainers: is that correct?
> 
> If so, we can deal with it by making the sbsa-ref board set the
> cntfrq QOM property on its CPUs to force them to the old
> frequency. However this will produce a technically-out-of-spec
> CPU when used with a v8.6-compliant CPU type, so probably we
> should do something to be able to tell the firmware the clock
> frequency (if it doesn't want to just read CNTFRQ_EL0 itself).

 From what I see in EDK2 code we read CNTFREQ_EL0:

GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
ArmReadCntFrq() -> "mrs x0, cntfrq_el0"

I added debug output to firmware and it shows me:

HRW: GetPlatformTimerFreq TimerFreq = 62500000

Local tree:
ed0604e99c (HEAD -> test-counter) target/arm: Default to 1GHz cntfrq for 'max' and new CPUs
c0a8c341f5 target/arm: Refactor default generic timer frequency handling
592c01312b hw: Add compat machines for 9.1
62dbe54c24 (tag: v9.0.0-rc4, origin/master, origin/HEAD) Update version for v9.0.0-rc4 release
a12214d1c4 (origin/staging) usb-storage: Fix BlockConf defaults

sbsa-ref with "-cpu max" used. All cpu cores give me same value.
Peter Maydell April 22, 2024, 2:18 p.m. UTC | #3
On Mon, 22 Apr 2024 at 14:38, Marcin Juszkiewicz
<marcin.juszkiewicz@linaro.org> wrote:
>
> W dniu 22.04.2024 o 14:56, Peter Maydell pisze:
> > On Fri, 19 Apr 2024 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> >> The upshot is that the only CPU type that changes is 'max'; but any
> >> new type we add in future (whether v8.6 or not) will also get the new
> >> 1GHz default (assuming we spot in code review any attempts to set
> >> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
> >> of cut-n-paste from an older CPU initfn ;-)).
> >>
> >> It remains the case that the machine model can override the default
> >> value via the 'cntfrq' QOM property (regardless of the CPU type).
> >
> > This patchset turns out to break the sbsa-ref board: the
> > Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
> > avocado test both (a) takes rather longer to boot and (b) when
> > running thinks that time is advancing very fast.
> >
> > I suspect this may be because the firmware hard-codes the
> > generic timer clock frequency it is expecting. I've cc'd the
> > sbsa-ref maintainers: is that correct?
> >
> > If so, we can deal with it by making the sbsa-ref board set the
> > cntfrq QOM property on its CPUs to force them to the old
> > frequency. However this will produce a technically-out-of-spec
> > CPU when used with a v8.6-compliant CPU type, so probably we
> > should do something to be able to tell the firmware the clock
> > frequency (if it doesn't want to just read CNTFRQ_EL0 itself).
>
>  From what I see in EDK2 code we read CNTFREQ_EL0:
>
> GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
> sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
> ArmReadCntFrq() -> "mrs x0, cntfrq_el0"

Yeah, it looks like it's TF-A that hardcodes the frequency:
https://github.com/ARM-software/arm-trusted-firmware/blob/c8be7c08c3b3a2330d54b58651faa9438ff34f6e/plat/qemu/qemu_sbsa/include/platform_def.h#L269

I imagine that value gets written into CNTFRQ by TF-A somewhere
along the line (and then read by EDK2 later), though I haven't
quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
assumes that the generic-timer frequency and the watchdog
timer frequency are the same, which is a bit dubious IMHO.

It also seems to be hardcoded in TF-A's support for the virt
board too, annoyingly.

thanks
-- PMM
Marcin Juszkiewicz April 22, 2024, 3:37 p.m. UTC | #4
W dniu 22.04.2024 o 16:18, Peter Maydell pisze:
> On Mon, 22 Apr 2024 at 14:38, Marcin Juszkiewicz

>>   From what I see in EDK2 code we read CNTFREQ_EL0:
>>
>> GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
>> sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
>> ArmReadCntFrq() -> "mrs x0, cntfrq_el0"
> 
> Yeah, it looks like it's TF-A that hardcodes the frequency:
> https://github.com/ARM-software/arm-trusted-firmware/blob/c8be7c08c3b3a2330d54b58651faa9438ff34f6e/plat/qemu/qemu_sbsa/include/platform_def.h#L269
> 
> I imagine that value gets written into CNTFRQ by TF-A somewhere
> along the line (and then read by EDK2 later), though I haven't
> quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
> assumes that the generic-timer frequency and the watchdog
> timer frequency are the same, which is a bit dubious IMHO.
> 
> It also seems to be hardcoded in TF-A's support for the virt
> board too, annoyingly.

I looked at it and it seems that TF-A can just read what is in 
CNTFRQ_EL0 instead of hardcoding the value.

Submitted patch:

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/28313
refactor(qemu): do not hardcode counter frequency [NEW]
Peter Maydell April 22, 2024, 3:57 p.m. UTC | #5
On Mon, 22 Apr 2024 at 15:18, Peter Maydell <peter.maydell@linaro.org> wrote:
> I imagine that value gets written into CNTFRQ by TF-A somewhere
> along the line (and then read by EDK2 later), though I haven't
> quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
> assumes that the generic-timer frequency and the watchdog
> timer frequency are the same, which is a bit dubious IMHO.

Checking the BSA spec, this is actually correct -- the system
watchdog is supposed to run at the generic counter frequency,
which will be the same as the one the CPU generic timers use.
So we need on the QEMU side to make the sbsa-watchdog device
be runtime configurable for frequency and arrange for it to
be set the same as the CPU.

(We could also arrange this by modelling the memory mapped
system counter properly; I have some slightly half-baked
patches to do that floating around somewhere. But I'm still
not quite sure it's worth the effort needed to try to get
them into a fully baked state :-))

thanks
-- PMM
Marcin Juszkiewicz April 23, 2024, 7:26 a.m. UTC | #6
W dniu 22.04.2024 o 17:37, Marcin Juszkiewicz pisze:
>> It also seems to be hardcoded in TF-A's support for the virt
>> board too, annoyingly.
> 
> I looked at it and it seems that TF-A can just read what is in 
> CNTFRQ_EL0 instead of hardcoding the value.
> 
> Submitted patch:
> 
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/28313
> refactor(qemu): do not hardcode counter frequency [NEW]

Change is now merged.