| 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 |
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
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.
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
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]
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
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.