From 546d574b11e02bfd5b15cdf1564842c14516dfab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frederik=20van=20H=C3=B6vell?= Date: Mon, 29 Jul 2024 13:34:18 +0100 Subject: [PATCH 01/21] hw/char/bcm2835_aux: Fix assert when receive FIFO fills up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a bare-metal application on the raspi3 board reads the AUX_MU_STAT_REG MMIO register while the device's buffer is at full receive FIFO capacity (i.e. `s->read_count == BCM2835_AUX_RX_FIFO_LEN`) the assertion `assert(s->read_count < BCM2835_AUX_RX_FIFO_LEN)` fails. Reported-by: Cryptjar Suggested-by: Cryptjar Resolves: https://gitlab.com/qemu-project/qemu/-/issues/459 Signed-off-by: Frederik van Hövell Reviewed-by: Philippe Mathieu-Daudé [PMM: commit message tweaks] Signed-off-by: Peter Maydell --- hw/char/bcm2835_aux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/char/bcm2835_aux.c b/hw/char/bcm2835_aux.c index 83990e20f7..fca2f27a55 100644 --- a/hw/char/bcm2835_aux.c +++ b/hw/char/bcm2835_aux.c @@ -138,7 +138,7 @@ static uint64_t bcm2835_aux_read(void *opaque, hwaddr offset, unsigned size) res = 0x30e; /* space in the output buffer, empty tx fifo, idle tx/rx */ if (s->read_count > 0) { res |= 0x1; /* data in input buffer */ - assert(s->read_count < BCM2835_AUX_RX_FIFO_LEN); + assert(s->read_count <= BCM2835_AUX_RX_FIFO_LEN); res |= ((uint32_t)s->read_count) << 16; /* rx fifo fill level */ } return res; From 8deba2f36e9f7e34bf1805b08e69699486481c8a Mon Sep 17 00:00:00 2001 From: Mostafa Saleh Date: Mon, 29 Jul 2024 13:34:18 +0100 Subject: [PATCH 02/21] hw/arm/smmuv3: Assert input to oas2bits() is valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity has spotted a possible problem with the OAS handling (CID 1558464), where the error return of oas2bits() -1 is not checked, which can cause an overflow in oas value. oas2bits() is only called with valid inputs, harden the function to assert that. Reported-By: Peter Maydell Signed-off-by: Mostafa Saleh Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Eric Auger Message-id: 20240722103531.2377348-1-smostafa@google.com Link: https://lore.kernel.org/qemu-devel/CAFEAcA-H=n-3mHC+eL6YjfL1m+x+b+Fk3mkgZbN74WNxifFVow@mail.gmail.com/ Signed-off-by: Mostafa Saleh Signed-off-by: Peter Maydell --- hw/arm/smmuv3-internal.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h index 0ebf2eebcf..b6b7399347 100644 --- a/hw/arm/smmuv3-internal.h +++ b/hw/arm/smmuv3-internal.h @@ -599,7 +599,8 @@ static inline int oas2bits(int oas_field) case 5: return 48; } - return -1; + + g_assert_not_reached(); } /* CD fields */ From 613d0b8ebbf1fa32f8037cb02b7c82f96011ef2d Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 20 Jul 2024 18:30:49 +0900 Subject: [PATCH 03/21] target/arm/kvm: Set PMU for host only when available target/arm/kvm.c checked PMU availability but unconditionally set the PMU feature flag for the host CPU model, which is confusing. Set the feature flag only when available. Signed-off-by: Akihiko Odaki Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/kvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/kvm.c b/target/arm/kvm.c index 70f79eda33..b20a35052f 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -280,6 +280,7 @@ static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) if (kvm_arm_pmu_supported()) { init.features[0] |= 1 << KVM_ARM_VCPU_PMU_V3; pmu_supported = true; + features |= 1ULL << ARM_FEATURE_PMU; } if (!kvm_arm_create_scratch_host_vcpu(cpus_to_try, fdarray, &init)) { @@ -448,7 +449,6 @@ static bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) features |= 1ULL << ARM_FEATURE_V8; features |= 1ULL << ARM_FEATURE_NEON; features |= 1ULL << ARM_FEATURE_AARCH64; - features |= 1ULL << ARM_FEATURE_PMU; features |= 1ULL << ARM_FEATURE_GENERIC_TIMER; ahcf->features = features; From 0d2aac4ab1777162c7a7ecd5a6d5d4de93842116 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 20 Jul 2024 18:30:50 +0900 Subject: [PATCH 04/21] target/arm/kvm: Do not silently remove PMU kvm_arch_init_vcpu() used to remove PMU when it is not available even if the CPU model needs one. It is semantically incorrect, and may continue execution on a misbehaving host that advertises a CPU model while lacking its PMU. Keep the PMU when the CPU model needs one, and let kvm_arm_vcpu_init() fail if the KVM implementation mismatches with our expectation. Signed-off-by: Akihiko Odaki Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/kvm.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/target/arm/kvm.c b/target/arm/kvm.c index b20a35052f..849e2e21b3 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -1888,13 +1888,8 @@ int kvm_arch_init_vcpu(CPUState *cs) if (!arm_feature(env, ARM_FEATURE_AARCH64)) { cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_EL1_32BIT; } - if (!kvm_check_extension(cs->kvm_state, KVM_CAP_ARM_PMU_V3)) { - cpu->has_pmu = false; - } if (cpu->has_pmu) { cpu->kvm_init_features[0] |= 1 << KVM_ARM_VCPU_PMU_V3; - } else { - env->features &= ~(1ULL << ARM_FEATURE_PMU); } if (cpu_isar_feature(aa64_sve, cpu)) { assert(kvm_arm_sve_supported()); From e9e640148c676ac2c087a3dd93e3917c8fca25ea Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 20 Jul 2024 18:30:52 +0900 Subject: [PATCH 05/21] hvf: arm: Raise an exception for sysreg by default Any sysreg access results in an exception unless defined otherwise so we should raise an exception by default. Signed-off-by: Akihiko Odaki Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/hvf/hvf.c | 174 +++++++++++++++++++++---------------------- 1 file changed, 85 insertions(+), 89 deletions(-) diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index eb090e67a2..1a749534fb 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -1199,57 +1199,56 @@ static bool hvf_sysreg_read_cp(CPUState *cpu, uint32_t reg, uint64_t *val) return false; } -static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt) +static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val) { ARMCPU *arm_cpu = ARM_CPU(cpu); CPUARMState *env = &arm_cpu->env; - uint64_t val = 0; switch (reg) { case SYSREG_CNTPCT_EL0: - val = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / + *val = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(arm_cpu); - break; + return 0; case SYSREG_PMCR_EL0: - val = env->cp15.c9_pmcr; - break; + *val = env->cp15.c9_pmcr; + return 0; case SYSREG_PMCCNTR_EL0: pmu_op_start(env); - val = env->cp15.c15_ccnt; + *val = env->cp15.c15_ccnt; pmu_op_finish(env); - break; + return 0; case SYSREG_PMCNTENCLR_EL0: - val = env->cp15.c9_pmcnten; - break; + *val = env->cp15.c9_pmcnten; + return 0; case SYSREG_PMOVSCLR_EL0: - val = env->cp15.c9_pmovsr; - break; + *val = env->cp15.c9_pmovsr; + return 0; case SYSREG_PMSELR_EL0: - val = env->cp15.c9_pmselr; - break; + *val = env->cp15.c9_pmselr; + return 0; case SYSREG_PMINTENCLR_EL1: - val = env->cp15.c9_pminten; - break; + *val = env->cp15.c9_pminten; + return 0; case SYSREG_PMCCFILTR_EL0: - val = env->cp15.pmccfiltr_el0; - break; + *val = env->cp15.pmccfiltr_el0; + return 0; case SYSREG_PMCNTENSET_EL0: - val = env->cp15.c9_pmcnten; - break; + *val = env->cp15.c9_pmcnten; + return 0; case SYSREG_PMUSERENR_EL0: - val = env->cp15.c9_pmuserenr; - break; + *val = env->cp15.c9_pmuserenr; + return 0; case SYSREG_PMCEID0_EL0: case SYSREG_PMCEID1_EL0: /* We can't really count anything yet, declare all events invalid */ - val = 0; - break; + *val = 0; + return 0; case SYSREG_OSLSR_EL1: - val = env->cp15.oslsr_el1; - break; + *val = env->cp15.oslsr_el1; + return 0; case SYSREG_OSDLR_EL1: /* Dummy register */ - break; + return 0; case SYSREG_ICC_AP0R0_EL1: case SYSREG_ICC_AP0R1_EL1: case SYSREG_ICC_AP0R2_EL1: @@ -1276,9 +1275,8 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt) case SYSREG_ICC_SRE_EL1: case SYSREG_ICC_CTLR_EL1: /* Call the TCG sysreg handler. This is only safe for GICv3 regs. */ - if (!hvf_sysreg_read_cp(cpu, reg, &val)) { - hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); - return 1; + if (hvf_sysreg_read_cp(cpu, reg, &val)) { + return 0; } break; case SYSREG_DBGBVR0_EL1: @@ -1297,8 +1295,8 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt) case SYSREG_DBGBVR13_EL1: case SYSREG_DBGBVR14_EL1: case SYSREG_DBGBVR15_EL1: - val = env->cp15.dbgbvr[SYSREG_CRM(reg)]; - break; + *val = env->cp15.dbgbvr[SYSREG_CRM(reg)]; + return 0; case SYSREG_DBGBCR0_EL1: case SYSREG_DBGBCR1_EL1: case SYSREG_DBGBCR2_EL1: @@ -1315,8 +1313,8 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt) case SYSREG_DBGBCR13_EL1: case SYSREG_DBGBCR14_EL1: case SYSREG_DBGBCR15_EL1: - val = env->cp15.dbgbcr[SYSREG_CRM(reg)]; - break; + *val = env->cp15.dbgbcr[SYSREG_CRM(reg)]; + return 0; case SYSREG_DBGWVR0_EL1: case SYSREG_DBGWVR1_EL1: case SYSREG_DBGWVR2_EL1: @@ -1333,8 +1331,8 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt) case SYSREG_DBGWVR13_EL1: case SYSREG_DBGWVR14_EL1: case SYSREG_DBGWVR15_EL1: - val = env->cp15.dbgwvr[SYSREG_CRM(reg)]; - break; + *val = env->cp15.dbgwvr[SYSREG_CRM(reg)]; + return 0; case SYSREG_DBGWCR0_EL1: case SYSREG_DBGWCR1_EL1: case SYSREG_DBGWCR2_EL1: @@ -1351,35 +1349,25 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt) case SYSREG_DBGWCR13_EL1: case SYSREG_DBGWCR14_EL1: case SYSREG_DBGWCR15_EL1: - val = env->cp15.dbgwcr[SYSREG_CRM(reg)]; - break; + *val = env->cp15.dbgwcr[SYSREG_CRM(reg)]; + return 0; default: if (is_id_sysreg(reg)) { /* ID system registers read as RES0 */ - val = 0; - break; + *val = 0; + return 0; } - cpu_synchronize_state(cpu); - trace_hvf_unhandled_sysreg_read(env->pc, reg, - SYSREG_OP0(reg), - SYSREG_OP1(reg), - SYSREG_CRN(reg), - SYSREG_CRM(reg), - SYSREG_OP2(reg)); - hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); - return 1; } - trace_hvf_sysreg_read(reg, - SYSREG_OP0(reg), - SYSREG_OP1(reg), - SYSREG_CRN(reg), - SYSREG_CRM(reg), - SYSREG_OP2(reg), - val); - hvf_set_reg(cpu, rt, val); - - return 0; + cpu_synchronize_state(cpu); + trace_hvf_unhandled_sysreg_read(env->pc, reg, + SYSREG_OP0(reg), + SYSREG_OP1(reg), + SYSREG_CRN(reg), + SYSREG_CRM(reg), + SYSREG_OP2(reg)); + hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); + return 1; } static void pmu_update_irq(CPUARMState *env) @@ -1503,7 +1491,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) pmu_op_start(env); env->cp15.c15_ccnt = val; pmu_op_finish(env); - break; + return 0; case SYSREG_PMCR_EL0: pmu_op_start(env); @@ -1523,45 +1511,45 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) env->cp15.c9_pmcr |= (val & PMCR_WRITABLE_MASK); pmu_op_finish(env); - break; + return 0; case SYSREG_PMUSERENR_EL0: env->cp15.c9_pmuserenr = val & 0xf; - break; + return 0; case SYSREG_PMCNTENSET_EL0: env->cp15.c9_pmcnten |= (val & pmu_counter_mask(env)); - break; + return 0; case SYSREG_PMCNTENCLR_EL0: env->cp15.c9_pmcnten &= ~(val & pmu_counter_mask(env)); - break; + return 0; case SYSREG_PMINTENCLR_EL1: pmu_op_start(env); env->cp15.c9_pminten |= val; pmu_op_finish(env); - break; + return 0; case SYSREG_PMOVSCLR_EL0: pmu_op_start(env); env->cp15.c9_pmovsr &= ~val; pmu_op_finish(env); - break; + return 0; case SYSREG_PMSWINC_EL0: pmu_op_start(env); pmswinc_write(env, val); pmu_op_finish(env); - break; + return 0; case SYSREG_PMSELR_EL0: env->cp15.c9_pmselr = val & 0x1f; - break; + return 0; case SYSREG_PMCCFILTR_EL0: pmu_op_start(env); env->cp15.pmccfiltr_el0 = val & PMCCFILTR_EL0; pmu_op_finish(env); - break; + return 0; case SYSREG_OSLAR_EL1: env->cp15.oslsr_el1 = val & 1; - break; + return 0; case SYSREG_OSDLR_EL1: /* Dummy register */ - break; + return 0; case SYSREG_ICC_AP0R0_EL1: case SYSREG_ICC_AP0R1_EL1: case SYSREG_ICC_AP0R2_EL1: @@ -1591,10 +1579,10 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) if (!hvf_sysreg_write_cp(cpu, reg, val)) { hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); } - break; + return 0; case SYSREG_MDSCR_EL1: env->cp15.mdscr_el1 = val; - break; + return 0; case SYSREG_DBGBVR0_EL1: case SYSREG_DBGBVR1_EL1: case SYSREG_DBGBVR2_EL1: @@ -1612,7 +1600,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) case SYSREG_DBGBVR14_EL1: case SYSREG_DBGBVR15_EL1: env->cp15.dbgbvr[SYSREG_CRM(reg)] = val; - break; + return 0; case SYSREG_DBGBCR0_EL1: case SYSREG_DBGBCR1_EL1: case SYSREG_DBGBCR2_EL1: @@ -1630,7 +1618,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) case SYSREG_DBGBCR14_EL1: case SYSREG_DBGBCR15_EL1: env->cp15.dbgbcr[SYSREG_CRM(reg)] = val; - break; + return 0; case SYSREG_DBGWVR0_EL1: case SYSREG_DBGWVR1_EL1: case SYSREG_DBGWVR2_EL1: @@ -1648,7 +1636,7 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) case SYSREG_DBGWVR14_EL1: case SYSREG_DBGWVR15_EL1: env->cp15.dbgwvr[SYSREG_CRM(reg)] = val; - break; + return 0; case SYSREG_DBGWCR0_EL1: case SYSREG_DBGWCR1_EL1: case SYSREG_DBGWCR2_EL1: @@ -1666,20 +1654,18 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) case SYSREG_DBGWCR14_EL1: case SYSREG_DBGWCR15_EL1: env->cp15.dbgwcr[SYSREG_CRM(reg)] = val; - break; - default: - cpu_synchronize_state(cpu); - trace_hvf_unhandled_sysreg_write(env->pc, reg, - SYSREG_OP0(reg), - SYSREG_OP1(reg), - SYSREG_CRN(reg), - SYSREG_CRM(reg), - SYSREG_OP2(reg)); - hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); - return 1; + return 0; } - return 0; + cpu_synchronize_state(cpu); + trace_hvf_unhandled_sysreg_write(env->pc, reg, + SYSREG_OP0(reg), + SYSREG_OP1(reg), + SYSREG_CRN(reg), + SYSREG_CRM(reg), + SYSREG_OP2(reg)); + hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); + return 1; } static int hvf_inject_interrupts(CPUState *cpu) @@ -1944,7 +1930,17 @@ int hvf_vcpu_exec(CPUState *cpu) int sysreg_ret = 0; if (isread) { - sysreg_ret = hvf_sysreg_read(cpu, reg, rt); + sysreg_ret = hvf_sysreg_read(cpu, reg, &val); + if (!sysreg_ret) { + trace_hvf_sysreg_read(reg, + SYSREG_OP0(reg), + SYSREG_OP1(reg), + SYSREG_CRN(reg), + SYSREG_CRM(reg), + SYSREG_OP2(reg), + val); + hvf_set_reg(cpu, rt, val); + } } else { val = hvf_get_reg(cpu, rt); sysreg_ret = hvf_sysreg_write(cpu, reg, val); From e6fd3192edb8178613e61f7f0242a774ad8ffa2c Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 20 Jul 2024 18:30:53 +0900 Subject: [PATCH 06/21] hvf: arm: Properly disable PMU Setting pmu property used to have no effect for hvf so fix it. Signed-off-by: Akihiko Odaki Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/hvf/hvf.c | 186 +++++++++++++++++++++++-------------------- 1 file changed, 98 insertions(+), 88 deletions(-) diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index 1a749534fb..adcdfae0b1 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -1204,45 +1204,50 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint64_t *val) ARMCPU *arm_cpu = ARM_CPU(cpu); CPUARMState *env = &arm_cpu->env; + if (arm_feature(env, ARM_FEATURE_PMU)) { + switch (reg) { + case SYSREG_PMCR_EL0: + *val = env->cp15.c9_pmcr; + return 0; + case SYSREG_PMCCNTR_EL0: + pmu_op_start(env); + *val = env->cp15.c15_ccnt; + pmu_op_finish(env); + return 0; + case SYSREG_PMCNTENCLR_EL0: + *val = env->cp15.c9_pmcnten; + return 0; + case SYSREG_PMOVSCLR_EL0: + *val = env->cp15.c9_pmovsr; + return 0; + case SYSREG_PMSELR_EL0: + *val = env->cp15.c9_pmselr; + return 0; + case SYSREG_PMINTENCLR_EL1: + *val = env->cp15.c9_pminten; + return 0; + case SYSREG_PMCCFILTR_EL0: + *val = env->cp15.pmccfiltr_el0; + return 0; + case SYSREG_PMCNTENSET_EL0: + *val = env->cp15.c9_pmcnten; + return 0; + case SYSREG_PMUSERENR_EL0: + *val = env->cp15.c9_pmuserenr; + return 0; + case SYSREG_PMCEID0_EL0: + case SYSREG_PMCEID1_EL0: + /* We can't really count anything yet, declare all events invalid */ + *val = 0; + return 0; + } + } + switch (reg) { case SYSREG_CNTPCT_EL0: *val = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(arm_cpu); return 0; - case SYSREG_PMCR_EL0: - *val = env->cp15.c9_pmcr; - return 0; - case SYSREG_PMCCNTR_EL0: - pmu_op_start(env); - *val = env->cp15.c15_ccnt; - pmu_op_finish(env); - return 0; - case SYSREG_PMCNTENCLR_EL0: - *val = env->cp15.c9_pmcnten; - return 0; - case SYSREG_PMOVSCLR_EL0: - *val = env->cp15.c9_pmovsr; - return 0; - case SYSREG_PMSELR_EL0: - *val = env->cp15.c9_pmselr; - return 0; - case SYSREG_PMINTENCLR_EL1: - *val = env->cp15.c9_pminten; - return 0; - case SYSREG_PMCCFILTR_EL0: - *val = env->cp15.pmccfiltr_el0; - return 0; - case SYSREG_PMCNTENSET_EL0: - *val = env->cp15.c9_pmcnten; - return 0; - case SYSREG_PMUSERENR_EL0: - *val = env->cp15.c9_pmuserenr; - return 0; - case SYSREG_PMCEID0_EL0: - case SYSREG_PMCEID1_EL0: - /* We can't really count anything yet, declare all events invalid */ - *val = 0; - return 0; case SYSREG_OSLSR_EL1: *val = env->cp15.oslsr_el1; return 0; @@ -1486,64 +1491,69 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) SYSREG_OP2(reg), val); - switch (reg) { - case SYSREG_PMCCNTR_EL0: - pmu_op_start(env); - env->cp15.c15_ccnt = val; - pmu_op_finish(env); - return 0; - case SYSREG_PMCR_EL0: - pmu_op_start(env); + if (arm_feature(env, ARM_FEATURE_PMU)) { + switch (reg) { + case SYSREG_PMCCNTR_EL0: + pmu_op_start(env); + env->cp15.c15_ccnt = val; + pmu_op_finish(env); + return 0; + case SYSREG_PMCR_EL0: + pmu_op_start(env); - if (val & PMCRC) { - /* The counter has been reset */ - env->cp15.c15_ccnt = 0; - } - - if (val & PMCRP) { - unsigned int i; - for (i = 0; i < pmu_num_counters(env); i++) { - env->cp15.c14_pmevcntr[i] = 0; + if (val & PMCRC) { + /* The counter has been reset */ + env->cp15.c15_ccnt = 0; } + + if (val & PMCRP) { + unsigned int i; + for (i = 0; i < pmu_num_counters(env); i++) { + env->cp15.c14_pmevcntr[i] = 0; + } + } + + env->cp15.c9_pmcr &= ~PMCR_WRITABLE_MASK; + env->cp15.c9_pmcr |= (val & PMCR_WRITABLE_MASK); + + pmu_op_finish(env); + return 0; + case SYSREG_PMUSERENR_EL0: + env->cp15.c9_pmuserenr = val & 0xf; + return 0; + case SYSREG_PMCNTENSET_EL0: + env->cp15.c9_pmcnten |= (val & pmu_counter_mask(env)); + return 0; + case SYSREG_PMCNTENCLR_EL0: + env->cp15.c9_pmcnten &= ~(val & pmu_counter_mask(env)); + return 0; + case SYSREG_PMINTENCLR_EL1: + pmu_op_start(env); + env->cp15.c9_pminten |= val; + pmu_op_finish(env); + return 0; + case SYSREG_PMOVSCLR_EL0: + pmu_op_start(env); + env->cp15.c9_pmovsr &= ~val; + pmu_op_finish(env); + return 0; + case SYSREG_PMSWINC_EL0: + pmu_op_start(env); + pmswinc_write(env, val); + pmu_op_finish(env); + return 0; + case SYSREG_PMSELR_EL0: + env->cp15.c9_pmselr = val & 0x1f; + return 0; + case SYSREG_PMCCFILTR_EL0: + pmu_op_start(env); + env->cp15.pmccfiltr_el0 = val & PMCCFILTR_EL0; + pmu_op_finish(env); + return 0; } + } - env->cp15.c9_pmcr &= ~PMCR_WRITABLE_MASK; - env->cp15.c9_pmcr |= (val & PMCR_WRITABLE_MASK); - - pmu_op_finish(env); - return 0; - case SYSREG_PMUSERENR_EL0: - env->cp15.c9_pmuserenr = val & 0xf; - return 0; - case SYSREG_PMCNTENSET_EL0: - env->cp15.c9_pmcnten |= (val & pmu_counter_mask(env)); - return 0; - case SYSREG_PMCNTENCLR_EL0: - env->cp15.c9_pmcnten &= ~(val & pmu_counter_mask(env)); - return 0; - case SYSREG_PMINTENCLR_EL1: - pmu_op_start(env); - env->cp15.c9_pminten |= val; - pmu_op_finish(env); - return 0; - case SYSREG_PMOVSCLR_EL0: - pmu_op_start(env); - env->cp15.c9_pmovsr &= ~val; - pmu_op_finish(env); - return 0; - case SYSREG_PMSWINC_EL0: - pmu_op_start(env); - pmswinc_write(env, val); - pmu_op_finish(env); - return 0; - case SYSREG_PMSELR_EL0: - env->cp15.c9_pmselr = val & 0x1f; - return 0; - case SYSREG_PMCCFILTR_EL0: - pmu_op_start(env); - env->cp15.pmccfiltr_el0 = val & PMCCFILTR_EL0; - pmu_op_finish(env); - return 0; + switch (reg) { case SYSREG_OSLAR_EL1: env->cp15.oslsr_el1 = val & 1; return 0; From 05b8d7249109c44a30352ac86d1f079ec0a8506e Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 20 Jul 2024 18:30:54 +0900 Subject: [PATCH 07/21] hvf: arm: Do not advance PC when raising an exception This is identical with commit 30a1690f2402 ("hvf: arm: Do not advance PC when raising an exception") but for writes instead of reads. Fixes: a2260983c655 ("hvf: arm: Add support for GICv3") Signed-off-by: Akihiko Odaki Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/hvf/hvf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index adcdfae0b1..c1496ad5be 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -1586,10 +1586,10 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val) case SYSREG_ICC_SGI1R_EL1: case SYSREG_ICC_SRE_EL1: /* Call the TCG sysreg handler. This is only safe for GICv3 regs. */ - if (!hvf_sysreg_write_cp(cpu, reg, val)) { - hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized()); + if (hvf_sysreg_write_cp(cpu, reg, val)) { + return 0; } - return 0; + break; case SYSREG_MDSCR_EL1: env->cp15.mdscr_el1 = val; return 0; From 0892fffc2abaadfb5d8b79bb0250ae1794862560 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 14:10:26 +0100 Subject: [PATCH 08/21] hw/misc/bcm2835_property: Fix handling of FRAMEBUFFER_SET_PALETTE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documentation of the "Set palette" mailbox property at https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface#set-palette says it has the form: Length: 24..1032 Value: u32: offset: first palette index to set (0-255) u32: length: number of palette entries to set (1-256) u32...: RGBA palette values (offset to offset+length-1) We get this wrong in a couple of ways: * we aren't checking the offset and length are in range, so the guest can make us spin for a long time by providing a large length * the bounds check on our loop is wrong: we should iterate through 'length' palette entries, not 'length - offset' entries Fix the loop to implement the bounds checks and get the loop condition right. In the process, make the variables local to this switch case, rather than function-global, so it's clearer what type they are when reading the code. Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240723131029.1159908-2-peter.maydell@linaro.org --- hw/misc/bcm2835_property.c | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 63de3db621..e28fdca984 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -31,7 +31,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) size_t resplen; uint32_t tmp; int n; - uint32_t offset, length, color; uint32_t start_num, number, otp_row; /* @@ -274,19 +273,25 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) resplen = 16; break; case RPI_FWREQ_FRAMEBUFFER_SET_PALETTE: - offset = ldl_le_phys(&s->dma_as, value + 12); - length = ldl_le_phys(&s->dma_as, value + 16); - n = 0; - while (n < length - offset) { - color = ldl_le_phys(&s->dma_as, value + 20 + (n << 2)); - stl_le_phys(&s->dma_as, - s->fbdev->vcram_base + ((offset + n) << 2), color); - n++; + { + uint32_t offset = ldl_le_phys(&s->dma_as, value + 12); + uint32_t length = ldl_le_phys(&s->dma_as, value + 16); + int resp; + + if (offset > 255 || length < 1 || length > 256) { + resp = 1; /* invalid request */ + } else { + for (uint32_t e = 0; e < length; e++) { + uint32_t color = ldl_le_phys(&s->dma_as, value + 20 + (e << 2)); + stl_le_phys(&s->dma_as, + s->fbdev->vcram_base + ((offset + e) << 2), color); + } + resp = 0; } - stl_le_phys(&s->dma_as, value + 12, 0); + stl_le_phys(&s->dma_as, value + 12, resp); resplen = 4; break; - + } case RPI_FWREQ_FRAMEBUFFER_GET_NUM_DISPLAYS: stl_le_phys(&s->dma_as, value + 12, 1); resplen = 4; From 32f1c201eedf992c2ac8033d8af95f1ff1758a4d Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 14:10:27 +0100 Subject: [PATCH 09/21] hw/misc/bcm2835_property: Avoid overflow in OTP access properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity points out that in our handling of the property RPI_FWREQ_SET_CUSTOMER_OTP we have a potential overflow. This happens because we read start_num and number from the guest as unsigned 32 bit integers, but then the variable 'n' we use as a loop counter as we iterate from start_num to start_num + number is only an "int". That means that if the guest passes us a very large start_num we will interpret it as negative. This will result in an assertion failure inside bcm2835_otp_set_row(), which checks that we didn't pass it an invalid row number. A similar issue applies to all the properties for accessing OTP rows where we are iterating through with a start and length read from the guest. Use uint32_t for the loop counter to avoid this problem. Because in all cases 'n' is only used as a loop counter, we can do this as part of the for(), restricting its scope to exactly where we need it. Resolves: Coverity CID 1549401 Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240723131029.1159908-3-peter.maydell@linaro.org --- hw/misc/bcm2835_property.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index e28fdca984..7eb623b4e9 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) uint32_t tot_len; size_t resplen; uint32_t tmp; - int n; uint32_t start_num, number, otp_row; /* @@ -337,7 +336,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) resplen = 8 + 4 * number; - for (n = start_num; n < start_num + number && + for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) { otp_row = bcm2835_otp_get_row(s->otp, BCM2835_OTP_CUSTOMER_OTP + n); @@ -366,7 +365,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) break; } - for (n = start_num; n < start_num + number && + for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) { otp_row = ldl_le_phys(&s->dma_as, value + 20 + ((n - start_num) << 2)); @@ -383,7 +382,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) resplen = 8 + 4 * number; - for (n = start_num; n < start_num + number && + for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) { otp_row = bcm2835_otp_get_row(s->otp, BCM2835_OTP_PRIVATE_KEY + n); @@ -403,7 +402,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) break; } - for (n = start_num; n < start_num + number && + for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) { otp_row = ldl_le_phys(&s->dma_as, value + 20 + ((n - start_num) << 2)); From 4b648238b8dd9870280b60ff32bf7fe2fe82607f Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 14:10:28 +0100 Subject: [PATCH 10/21] hw/misc/bcm2835_property: Restrict scope of start_num, number, otp_row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the long function bcm2835_property_mbox_push(), the variables start_num, number and otp_row are used only in the four cases which access OTP data, and their uses don't overlap with each other. Make these variables have scope restricted to the cases where they're used, so it's easier to read each individual case without having to cross-refer up to the variable declaration at the top of the function and check whether the variable is also used later in the loop. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240723131029.1159908-4-peter.maydell@linaro.org --- hw/misc/bcm2835_property.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 7eb623b4e9..443d42a182 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -30,7 +30,6 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) uint32_t tot_len; size_t resplen; uint32_t tmp; - uint32_t start_num, number, otp_row; /* * Copy the current state of the framebuffer config; we will update @@ -331,22 +330,25 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) /* Customer OTP */ case RPI_FWREQ_GET_CUSTOMER_OTP: - start_num = ldl_le_phys(&s->dma_as, value + 12); - number = ldl_le_phys(&s->dma_as, value + 16); + { + uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12); + uint32_t number = ldl_le_phys(&s->dma_as, value + 16); resplen = 8 + 4 * number; for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) { - otp_row = bcm2835_otp_get_row(s->otp, + uint32_t otp_row = bcm2835_otp_get_row(s->otp, BCM2835_OTP_CUSTOMER_OTP + n); stl_le_phys(&s->dma_as, value + 20 + ((n - start_num) << 2), otp_row); } break; + } case RPI_FWREQ_SET_CUSTOMER_OTP: - start_num = ldl_le_phys(&s->dma_as, value + 12); - number = ldl_le_phys(&s->dma_as, value + 16); + { + uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12); + uint32_t number = ldl_le_phys(&s->dma_as, value + 16); resplen = 4; @@ -367,32 +369,35 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_CUSTOMER_OTP_LEN; n++) { - otp_row = ldl_le_phys(&s->dma_as, + uint32_t otp_row = ldl_le_phys(&s->dma_as, value + 20 + ((n - start_num) << 2)); bcm2835_otp_set_row(s->otp, BCM2835_OTP_CUSTOMER_OTP + n, otp_row); } break; + } /* Device-specific private key */ - case RPI_FWREQ_GET_PRIVATE_KEY: - start_num = ldl_le_phys(&s->dma_as, value + 12); - number = ldl_le_phys(&s->dma_as, value + 16); + { + uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12); + uint32_t number = ldl_le_phys(&s->dma_as, value + 16); resplen = 8 + 4 * number; for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) { - otp_row = bcm2835_otp_get_row(s->otp, + uint32_t otp_row = bcm2835_otp_get_row(s->otp, BCM2835_OTP_PRIVATE_KEY + n); stl_le_phys(&s->dma_as, value + 20 + ((n - start_num) << 2), otp_row); } break; + } case RPI_FWREQ_SET_PRIVATE_KEY: - start_num = ldl_le_phys(&s->dma_as, value + 12); - number = ldl_le_phys(&s->dma_as, value + 16); + { + uint32_t start_num = ldl_le_phys(&s->dma_as, value + 12); + uint32_t number = ldl_le_phys(&s->dma_as, value + 16); resplen = 4; @@ -404,12 +409,13 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) for (uint32_t n = start_num; n < start_num + number && n < BCM2835_OTP_PRIVATE_KEY_LEN; n++) { - otp_row = ldl_le_phys(&s->dma_as, + uint32_t otp_row = ldl_le_phys(&s->dma_as, value + 20 + ((n - start_num) << 2)); bcm2835_otp_set_row(s->otp, BCM2835_OTP_PRIVATE_KEY + n, otp_row); } break; + } default: qemu_log_mask(LOG_UNIMP, "bcm2835_property: unhandled tag 0x%08x\n", tag); From 28fe81f052f1fcc651c4c4c098de68af73bbe4f6 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 14:10:29 +0100 Subject: [PATCH 11/21] hw/misc/bcm2835_property: Reduce scope of variables in mbox push function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In bcm2835_property_mbox_push(), some variables are defined at function scope but used only in a smaller scope of the function: * tag, bufsize, resplen are used only in the body of the while() loop * tmp is used only for RPI_FWREQ_SET_POWER_STATE (and is badly named) Declare these variables in the scope where they're needed, so the code is easier to read. Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240723131029.1159908-5-peter.maydell@linaro.org --- hw/misc/bcm2835_property.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 443d42a182..8ca3128f29 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -25,11 +25,7 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) { - uint32_t tag; - uint32_t bufsize; uint32_t tot_len; - size_t resplen; - uint32_t tmp; /* * Copy the current state of the framebuffer config; we will update @@ -48,10 +44,10 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) /* @(addr + 4) : Buffer response code */ value = s->addr + 8; while (value + 8 <= s->addr + tot_len) { - tag = ldl_le_phys(&s->dma_as, value); - bufsize = ldl_le_phys(&s->dma_as, value + 4); + uint32_t tag = ldl_le_phys(&s->dma_as, value); + uint32_t bufsize = ldl_le_phys(&s->dma_as, value + 4); /* @(value + 8) : Request/response indicator */ - resplen = 0; + size_t resplen = 0; switch (tag) { case RPI_FWREQ_PROPERTY_END: break; @@ -95,13 +91,16 @@ static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value) resplen = 8; break; case RPI_FWREQ_SET_POWER_STATE: - /* Assume that whatever device they asked for exists, - * and we'll just claim we set it to the desired state + { + /* + * Assume that whatever device they asked for exists, + * and we'll just claim we set it to the desired state. */ - tmp = ldl_le_phys(&s->dma_as, value + 16); - stl_le_phys(&s->dma_as, value + 16, (tmp & 1)); + uint32_t state = ldl_le_phys(&s->dma_as, value + 16); + stl_le_phys(&s->dma_as, value + 16, (state & 1)); resplen = 8; break; + } /* Clocks */ From 56f1c0db928aae0b83fd91c89ddb226b137e2b21 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 22 Jul 2024 18:29:54 +0100 Subject: [PATCH 12/21] target/arm: Don't assert for 128-bit tile accesses when SVL is 128 For an instruction which accesses a 128-bit element tile when the SVL is also 128 (for example MOV z0.Q, p0/M, ZA0H.Q[w0,0]), we will assert in get_tile_rowcol(): qemu-system-aarch64: ../../tcg/tcg-op.c:926: tcg_gen_deposit_z_i32: Assertion `len > 0' failed. This happens because we calculate len = ctz32(streaming_vec_reg_size(s)) - esz;$ but if the SVL and the element size are the same len is 0, and the deposit operation asserts. In this case the ZA storage contains exactly one 128 bit element ZA tile, and the horizontal or vertical slice is just that tile. This means that regardless of the index value in the Ws register, we always access that tile. (In pseudocode terms, we calculate (index + offset) MOD 1, which is 0.) Special case the len == 0 case to avoid hitting the assertion in tcg_gen_deposit_z_i32(). Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20240722172957.1041231-2-peter.maydell@linaro.org --- target/arm/tcg/translate-sme.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/target/arm/tcg/translate-sme.c b/target/arm/tcg/translate-sme.c index 185a8a917b..a50a419af2 100644 --- a/target/arm/tcg/translate-sme.c +++ b/target/arm/tcg/translate-sme.c @@ -49,7 +49,15 @@ static TCGv_ptr get_tile_rowcol(DisasContext *s, int esz, int rs, /* Prepare a power-of-two modulo via extraction of @len bits. */ len = ctz32(streaming_vec_reg_size(s)) - esz; - if (vertical) { + if (!len) { + /* + * SVL is 128 and the element size is 128. There is exactly + * one 128x128 tile in the ZA storage, and so we calculate + * (Rs + imm) MOD 1, which is always 0. We need to special case + * this because TCG doesn't allow deposit ops with len 0. + */ + tcg_gen_movi_i32(tmp, 0); + } else if (vertical) { /* * Compute the byte offset of the index within the tile: * (index % (svl / size)) * size From ea3f5a90f036734522e9af3bffd77e69e9f47355 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 22 Jul 2024 18:29:55 +0100 Subject: [PATCH 13/21] target/arm: Fix UMOPA/UMOPS of 16-bit values The UMOPA/UMOPS instructions are supposed to multiply unsigned 8 or 16 bit elements and accumulate the products into a 64-bit element. In the Arm ARM pseudocode, this is done with the usual infinite-precision signed arithmetic. However our implementation doesn't quite get it right, because in the DEF_IMOP_64() macro we do: sum += (NTYPE)(n >> 0) * (MTYPE)(m >> 0); where NTYPE and MTYPE are uint16_t or int16_t. In the uint16_t case, the C usual arithmetic conversions mean the values are converted to "int" type and the multiply is done as a 32-bit multiply. This means that if the inputs are, for example, 0xffff and 0xffff then the result is 0xFFFE0001 as an int, which is then promoted to uint64_t for the accumulation into sum; this promotion incorrectly sign extends the multiply. Avoid the incorrect sign extension by casting to int64_t before the multiply, so we do the multiply as 64-bit signed arithmetic, which is a type large enough that the multiply can never overflow into the sign bit. (The equivalent 8-bit operations in DEF_IMOP_32() are fine, because the 8-bit multiplies can never overflow into the sign bit of a 32-bit integer.) Cc: qemu-stable@nongnu.org Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2372 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20240722172957.1041231-3-peter.maydell@linaro.org --- target/arm/tcg/sme_helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/target/arm/tcg/sme_helper.c b/target/arm/tcg/sme_helper.c index 50bb088d04..3ba826a6ce 100644 --- a/target/arm/tcg/sme_helper.c +++ b/target/arm/tcg/sme_helper.c @@ -1162,10 +1162,10 @@ static uint64_t NAME(uint64_t n, uint64_t m, uint64_t a, uint8_t p, bool neg) \ uint64_t sum = 0; \ /* Apply P to N as a mask, making the inactive elements 0. */ \ n &= expand_pred_h(p); \ - sum += (NTYPE)(n >> 0) * (MTYPE)(m >> 0); \ - sum += (NTYPE)(n >> 16) * (MTYPE)(m >> 16); \ - sum += (NTYPE)(n >> 32) * (MTYPE)(m >> 32); \ - sum += (NTYPE)(n >> 48) * (MTYPE)(m >> 48); \ + sum += (int64_t)(NTYPE)(n >> 0) * (MTYPE)(m >> 0); \ + sum += (int64_t)(NTYPE)(n >> 16) * (MTYPE)(m >> 16); \ + sum += (int64_t)(NTYPE)(n >> 32) * (MTYPE)(m >> 32); \ + sum += (int64_t)(NTYPE)(n >> 48) * (MTYPE)(m >> 48); \ return neg ? a - sum : a + sum; \ } From 76916dfa89e8900639c1055c07a295c06628a0bc Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 22 Jul 2024 18:29:56 +0100 Subject: [PATCH 14/21] target/arm: Avoid shifts by -1 in tszimm_shr() and tszimm_shl() The function tszimm_esz() returns a shift amount, or possibly -1 in certain cases that correspond to unallocated encodings in the instruction set. We catch these later in the trans_ functions (generally with an "a-esz < 0" check), but before we do the decodetree-generated code will also call tszimm_shr() or tszimm_sl(), which will use the tszimm_esz() return value as a shift count without checking that it is not negative, which is undefined behaviour. Avoid the UB by checking the return value in tszimm_shr() and tszimm_shl(). Cc: qemu-stable@nongnu.org Resolves: Coverity CID 1547617, 1547694 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20240722172957.1041231-4-peter.maydell@linaro.org --- target/arm/tcg/translate-sve.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/target/arm/tcg/translate-sve.c b/target/arm/tcg/translate-sve.c index 798ab2bfb1..a72c262096 100644 --- a/target/arm/tcg/translate-sve.c +++ b/target/arm/tcg/translate-sve.c @@ -50,13 +50,27 @@ static int tszimm_esz(DisasContext *s, int x) static int tszimm_shr(DisasContext *s, int x) { - return (16 << tszimm_esz(s, x)) - x; + /* + * We won't use the tszimm_shr() value if tszimm_esz() returns -1 (the + * trans function will check for esz < 0), so we can return any + * value we like from here in that case as long as we avoid UB. + */ + int esz = tszimm_esz(s, x); + if (esz < 0) { + return esz; + } + return (16 << esz) - x; } /* See e.g. LSL (immediate, predicated). */ static int tszimm_shl(DisasContext *s, int x) { - return x - (8 << tszimm_esz(s, x)); + /* As with tszimm_shr(), value will be unused if esz < 0 */ + int esz = tszimm_esz(s, x); + if (esz < 0) { + return esz; + } + return x - (8 << esz); } /* The SH bit is in bit 8. Extract the low 8 and shift. */ From f573ac059ed060234fcef4299fae9e500d357c33 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Mon, 22 Jul 2024 18:29:57 +0100 Subject: [PATCH 15/21] target/arm: Ignore SMCR_EL2.LEN and SVCR_EL2.LEN if EL2 is not enabled When determining the current vector length, the SMCR_EL2.LEN and SVCR_EL2.LEN settings should only be considered if EL2 is enabled (compare the pseudocode CurrentSVL and CurrentNSVL which call EL2Enabled()). We were checking against ARM_FEATURE_EL2 rather than calling arm_is_el2_enabled(), which meant that we would look at SMCR_EL2/SVCR_EL2 when in Secure EL1 or Secure EL0 even if Secure EL2 was not enabled. Use the correct check in sve_vqm1_for_el_sm(). Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Message-id: 20240722172957.1041231-5-peter.maydell@linaro.org --- target/arm/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/helper.c b/target/arm/helper.c index ce31957235..8fb4b474e8 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -7232,7 +7232,7 @@ uint32_t sve_vqm1_for_el_sm(CPUARMState *env, int el, bool sm) if (el <= 1 && !el_is_in_host(env, el)) { len = MIN(len, 0xf & (uint32_t)cr[1]); } - if (el <= 2 && arm_feature(env, ARM_FEATURE_EL2)) { + if (el <= 2 && arm_is_el2_enabled(env)) { len = MIN(len, 0xf & (uint32_t)cr[2]); } if (arm_feature(env, ARM_FEATURE_EL3)) { From b69c95e718ba83fac6d5d725c4a79c4da808ab67 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 16:10:42 +0100 Subject: [PATCH 16/21] target/tricore: Use unsigned types for bitops in helper_eq_b() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity points out that in helper_eq_b() we have an int32_t 'msk' and we end up shifting into its sign bit. This is OK for QEMU because we use -fwrapv to give this well defined semantics, but when you look at what this function is doing it's doing bit operations, so we should be using an unsigned variable anyway. This also matches the return type of the function. Make 'ret' and 'msk' uint32_t. Resolves: Coverity CID 1547758 Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Message-id: 20240723151042.1396610-1-peter.maydell@linaro.org --- target/tricore/op_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/tricore/op_helper.c b/target/tricore/op_helper.c index ba9c4444b3..a0d5a0da1d 100644 --- a/target/tricore/op_helper.c +++ b/target/tricore/op_helper.c @@ -1505,8 +1505,8 @@ uint32_t helper_sub_h(CPUTriCoreState *env, target_ulong r1, target_ulong r2) uint32_t helper_eq_b(target_ulong r1, target_ulong r2) { - int32_t ret; - int32_t i, msk; + uint32_t ret, msk; + int32_t i; ret = 0; msk = 0xff; From b42ba4ea4322357fcbfcb0e6e613d79ede84de64 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 16:14:54 +0100 Subject: [PATCH 17/21] target/xtensa: Make use of 'segment' in pptlb helper less confusing Coverity gets confused about the use of the 'segment' variable in the pptlb helper function: it thinks that we can take a code path where we first initialize it: unsigned segment = XTENSA_MPU_PROBE_B; // 0x40000000 and then use that value as a shift count: } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) { In fact this isn't possible, beacuse xtensa_mpu_lookup() is passed '&segment', and it uses that as an output value, which it will always set if it returns nonzero. But the way the code is currently written is confusing to a human reader as well as to Coverity. Instead of initializing 'segment' at the top of the function with a value that's only used in the "nhits == 0" code path, use the constant value directly in that code path, and don't initialize segment. This matches the way we use xtensa_mpu_lookup() in its other callsites in get_physical_addr_mpu(). Resolves: Coverity CID 1547589 Signed-off-by: Peter Maydell Acked-by: Max Filippov Message-id: 20240723151454.1396826-1-peter.maydell@linaro.org --- target/xtensa/mmu_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c index 997b21d389..29b84d5dbf 100644 --- a/target/xtensa/mmu_helper.c +++ b/target/xtensa/mmu_helper.c @@ -991,7 +991,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s) uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v) { unsigned nhits; - unsigned segment = XTENSA_MPU_PROBE_B; + unsigned segment; unsigned bg_segment; nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments, @@ -1005,7 +1005,7 @@ uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v) xtensa_mpu_lookup(env->config->mpu_bg, env->config->n_mpu_bg_segments, v, &bg_segment); - return env->config->mpu_bg[bg_segment].attr | segment; + return env->config->mpu_bg[bg_segment].attr | XTENSA_MPU_PROBE_B; } } From 7d01623a1b0d282430a1e99fbe62068e27a4b4c4 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 16:42:07 +0100 Subject: [PATCH 18/21] target/m68k: avoid shift into sign bit in dump_address_map() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity complains (CID 1547592) that in dump_address_map() we take a value stored in a signed integer variable 'i' and shift it by enough to shift into the sign bit when we construct the value 'logical'. This isn't a bug for QEMU because we use -fwrapv semantics, but we can make Coverity happy by using an unsigned type for the loop variables i, j, k in this function. While we're changing the declaration of the variables, put them in the for() loops so their scope is the minimum required (a style now permitted by our coding style guide). Resolves: Coverity CID 1547592 Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240723154207.1483665-1-peter.maydell@linaro.org --- target/m68k/helper.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/target/m68k/helper.c b/target/m68k/helper.c index 7967ad13cb..4c85badd5d 100644 --- a/target/m68k/helper.c +++ b/target/m68k/helper.c @@ -479,7 +479,6 @@ static void print_address_zone(uint32_t logical, uint32_t physical, static void dump_address_map(CPUM68KState *env, uint32_t root_pointer) { - int i, j, k; int tic_size, tic_shift; uint32_t tib_mask; uint32_t tia, tib, tic; @@ -502,19 +501,19 @@ static void dump_address_map(CPUM68KState *env, uint32_t root_pointer) tic_shift = 12; tib_mask = M68K_4K_PAGE_MASK; } - for (i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) { + for (unsigned i = 0; i < M68K_ROOT_POINTER_ENTRIES; i++) { tia = address_space_ldl(cs->as, M68K_POINTER_BASE(root_pointer) + i * 4, MEMTXATTRS_UNSPECIFIED, &txres); if (txres != MEMTX_OK || !M68K_UDT_VALID(tia)) { continue; } - for (j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) { + for (unsigned j = 0; j < M68K_ROOT_POINTER_ENTRIES; j++) { tib = address_space_ldl(cs->as, M68K_POINTER_BASE(tia) + j * 4, MEMTXATTRS_UNSPECIFIED, &txres); if (txres != MEMTX_OK || !M68K_UDT_VALID(tib)) { continue; } - for (k = 0; k < tic_size; k++) { + for (unsigned k = 0; k < tic_size; k++) { tic = address_space_ldl(cs->as, (tib & tib_mask) + k * 4, MEMTXATTRS_UNSPECIFIED, &txres); if (txres != MEMTX_OK || !M68K_PDT_VALID(tic)) { From bde8adb808eeffb02fdcbd3425f09f1645bdcf4a Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 17:25:25 +0100 Subject: [PATCH 19/21] target/i386: Remove dead assignment to ss in do_interrupt64() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coverity points out that in do_interrupt64() in the "to inner privilege" codepath we set "ss = 0", but because we also set "new_stack = 1" there, later in the function we will always override that value of ss with "ss = 0 | dpl". Remove the unnecessary initialization of ss, which allows us to reduce the scope of the variable to only where it is used. Borrow a comment from helper_lcall_protected() that explains what "0 | dpl" means here. Resolves: Coverity CID 1527395 Signed-off-by: Peter Maydell Reviewed-by: Richard Henderson Reviewed-by: Philippe Mathieu-Daudé Message-id: 20240723162525.1585743-1-peter.maydell@linaro.org --- target/i386/tcg/seg_helper.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c index aac092a356..bab552cd53 100644 --- a/target/i386/tcg/seg_helper.c +++ b/target/i386/tcg/seg_helper.c @@ -926,7 +926,7 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int, target_ulong ptr; int type, dpl, selector, cpl, ist; int has_error_code, new_stack; - uint32_t e1, e2, e3, ss, eflags; + uint32_t e1, e2, e3, eflags; target_ulong old_eip, offset; bool set_rf; StackAccess sa; @@ -1007,7 +1007,6 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int, /* to inner privilege */ new_stack = 1; sa.sp = get_rsp_from_tss(env, ist != 0 ? ist + 3 : dpl); - ss = 0; } else { /* to same privilege */ if (env->eflags & VM_MASK) { @@ -1040,7 +1039,7 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int, env->eflags &= ~(TF_MASK | VM_MASK | RF_MASK | NT_MASK); if (new_stack) { - ss = 0 | dpl; + uint32_t ss = 0 | dpl; /* SS = NULL selector with RPL = new CPL */ cpu_x86_load_seg_cache(env, R_SS, ss, 0, 0, dpl << DESC_DPL_SHIFT); } env->regs[R_ESP] = sa.sp; From 525650cd71104c046e4260b5acaeb275f520d5c0 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 18:24:31 +0100 Subject: [PATCH 20/21] target/sh4: Avoid shift into sign bit in update_itlb_use() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In update_itlb_use() the variables or_mask and and_mask are uint8_t, which means that in expressions like "and_mask << 24" the usual C arithmetic conversions will result in the shift being done as a signed int type, and so we will shift into the sign bit. For QEMU this isn't undefined behaviour because we use -fwrapv; but we can avoid it anyway by using uint32_t types for or_mask and and_mask. Resolves: Coverity CID 1547628 Signed-off-by: Peter Maydell Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Richard Henderson Reviewed-by: Yoshinori Sato Message-id: 20240723172431.1757296-1-peter.maydell@linaro.org --- target/sh4/helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/sh4/helper.c b/target/sh4/helper.c index 6702910627..9659c69550 100644 --- a/target/sh4/helper.c +++ b/target/sh4/helper.c @@ -187,7 +187,7 @@ void superh_cpu_do_interrupt(CPUState *cs) static void update_itlb_use(CPUSH4State * env, int itlbnb) { - uint8_t or_mask = 0, and_mask = (uint8_t) - 1; + uint32_t or_mask = 0, and_mask = 0xff; switch (itlbnb) { case 0: From 73188068d7ba40c8a37b4763db38bb1ce24ca07d Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 23 Jul 2024 18:05:13 +0100 Subject: [PATCH 21/21] system/physmem: Where we assume we have a RAM MR, assert it In the functions invalidate_and_set_dirty() and cpu_physical_memory_snapshot_and_clear_dirty(), we assume that we are dealing with RAM memory regions. In this case we know that memory_region_get_ram_addr() will succeed. Assert this before we use the returned ram_addr_t in arithmetic. This makes Coverity happier about these functions: it otherwise complains that we might have an arithmetic overflow that stems from the possible -1 return from memory_region_get_ram_addr(). Resolves: Coverity CID 1547629, 1547715 Signed-off-by: Peter Maydell Reviewed-by: Peter Xu Reviewed-by: David Hildenbrand Message-id: 20240723170513.1676453-1-peter.maydell@linaro.org --- system/physmem.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/system/physmem.c b/system/physmem.c index 0e19186e1b..94600a33ec 100644 --- a/system/physmem.c +++ b/system/physmem.c @@ -923,13 +923,19 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client) { DirtyMemoryBlocks *blocks; - ram_addr_t start = memory_region_get_ram_addr(mr) + offset; + ram_addr_t start, first, last; unsigned long align = 1UL << (TARGET_PAGE_BITS + BITS_PER_LEVEL); - ram_addr_t first = QEMU_ALIGN_DOWN(start, align); - ram_addr_t last = QEMU_ALIGN_UP(start + length, align); DirtyBitmapSnapshot *snap; unsigned long page, end, dest; + start = memory_region_get_ram_addr(mr); + /* We know we're only called for RAM MemoryRegions */ + assert(start != RAM_ADDR_INVALID); + start += offset; + + first = QEMU_ALIGN_DOWN(start, align); + last = QEMU_ALIGN_UP(start + length, align); + snap = g_malloc0(sizeof(*snap) + ((last - first) >> (TARGET_PAGE_BITS + 3))); snap->start = first; @@ -2659,7 +2665,11 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr, hwaddr length) { uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr); - addr += memory_region_get_ram_addr(mr); + ram_addr_t ramaddr = memory_region_get_ram_addr(mr); + + /* We know we're only called for RAM MemoryRegions */ + assert(ramaddr != RAM_ADDR_INVALID); + addr += ramaddr; /* No early return if dirty_log_mask is or becomes 0, because * cpu_physical_memory_set_dirty_range will still call