| Message ID | 20230512170223.3801643-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: Fix two minor coverity issues | expand |
On 5/12/23 10:02, Peter Maydell wrote: > The IMPDEF sysreg L2CTLR_EL1 found on the Cortex-A35, A53, A57, A72 > and which we (arguably dubiously) also provide in '-cpu max' has a > 2 bit field for the number of processors in the cluster. On real > hardware this must be sufficient because it can only be configured > with up to 4 CPUs in the cluster. However on QEMU if the board code > does not explicitly configure the code into clusters with the right > CPU count we default to "give the value assuming that all CPUs in > the system are in a single cluster", which might be too big to fit > in the field. > > Instead of just overflowing this 2-bit field, saturate to 3 (meaning > "4 CPUs", so at least we don't overwrite other fields in the register. > It's unlikely that any guest code really cares about the value in > this field; at least, if it does it probably also wants the system > to be more closely matching real hardware, i.e. not to have more > than 4 CPUs. > > This issue has been present since the L2CTLR was first added in > commit 377a44ec8f2fac5b back in 2014. It was only noticed because > Coverity complains (CID 1509227) that the shift might overflow 32 bits > and inadvertently sign extend into the top half of the 64 bit value. > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > target/arm/cortex-regs.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/arm/cortex-regs.c b/target/arm/cortex-regs.c index 17708480e75..ae817b08ddf 100644 --- a/target/arm/cortex-regs.c +++ b/target/arm/cortex-regs.c @@ -15,8 +15,15 @@ static uint64_t l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri) { ARMCPU *cpu = env_archcpu(env); - /* Number of cores is in [25:24]; otherwise we RAZ */ - return (cpu->core_count - 1) << 24; + /* + * Number of cores is in [25:24]; otherwise we RAZ. + * If the board didn't configure the CPUs into clusters, + * we default to "all CPUs in one cluster", which might be + * more than the 4 that the hardware permits and which is + * all you can report in this two-bit field. Saturate to + * 0b11 (== 4 CPUs) rather than overflowing the field. + */ + return MIN(cpu->core_count - 1, 3) << 24; } static const ARMCPRegInfo cortex_a72_a57_a53_cp_reginfo[] = {
The IMPDEF sysreg L2CTLR_EL1 found on the Cortex-A35, A53, A57, A72 and which we (arguably dubiously) also provide in '-cpu max' has a 2 bit field for the number of processors in the cluster. On real hardware this must be sufficient because it can only be configured with up to 4 CPUs in the cluster. However on QEMU if the board code does not explicitly configure the code into clusters with the right CPU count we default to "give the value assuming that all CPUs in the system are in a single cluster", which might be too big to fit in the field. Instead of just overflowing this 2-bit field, saturate to 3 (meaning "4 CPUs", so at least we don't overwrite other fields in the register. It's unlikely that any guest code really cares about the value in this field; at least, if it does it probably also wants the system to be more closely matching real hardware, i.e. not to have more than 4 CPUs. This issue has been present since the L2CTLR was first added in commit 377a44ec8f2fac5b back in 2014. It was only noticed because Coverity complains (CID 1509227) that the shift might overflow 32 bits and inadvertently sign extend into the top half of the 64 bit value. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/cortex-regs.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)