From 83ad95957c7e66f2685fb38c9675949d3bf478eb Mon Sep 17 00:00:00 2001 From: Alexander Graf Date: Tue, 19 Nov 2019 13:20:27 +0000 Subject: [PATCH 1/7] pl031: Expose RTCICR as proper WC register The current PL031 RTCICR register implementation always clears the IRQ pending status on a register write, regardless of the value the guest writes. To justify that behavior, it references the ARM926EJ-S Development Chip Reference Manual (DDI0287B) and indicates that said document states that any write clears the internal IRQ state. It is indeed true that in section 11.1 this document says: "The interrupt is cleared by writing any data value to the interrupt clear register RTCICR". However, later in section 11.2.2 it contradicts itself by saying: "Writing 1 to bit 0 of RTCICR clears the RTCINTR flag." The latter statement matches the PL031 TRM (DDI0224C), which says: "Writing 1 to bit position 0 clears the corresponding interrupt. Writing 0 has no effect." Let's assume that the self-contradictory DDI0287B is in error, and follow the reference manual for the device itself, by making the register write-one-to-clear. Reported-by: Hendrik Borghorst Signed-off-by: Alexander Graf Message-id: 20191104115228.30745-1-graf@amazon.com [PMM: updated commit message to note that DDI0287B says two conflicting things] Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- hw/rtc/pl031.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/hw/rtc/pl031.c b/hw/rtc/pl031.c index 3a982752a2..c57cf83165 100644 --- a/hw/rtc/pl031.c +++ b/hw/rtc/pl031.c @@ -149,11 +149,7 @@ static void pl031_write(void * opaque, hwaddr offset, pl031_update(s); break; case RTC_ICR: - /* The PL031 documentation (DDI0224B) states that the interrupt is - cleared when bit 0 of the written value is set. However the - arm926e documentation (DDI0287B) states that the interrupt is - cleared when any value is written. */ - s->is = 0; + s->is &= ~value; pl031_update(s); break; case RTC_CR: From 6e553f2a1b8450c9e9721fb60e3ef134492a4a39 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 19 Nov 2019 13:20:27 +0000 Subject: [PATCH 2/7] target/arm: Merge arm_cpu_vq_map_next_smaller into sole caller Coverity reports, in sve_zcr_get_valid_len, "Subtract operation overflows on operands arm_cpu_vq_map_next_smaller(cpu, start_vq + 1U) and 1U" First, the aarch32 stub version of arm_cpu_vq_map_next_smaller, returning 0, does exactly what Coverity reports. Remove it. Second, the aarch64 version of arm_cpu_vq_map_next_smaller has a set of asserts, but they don't cover the case in question. Further, there is a fair amount of extra arithmetic needed to convert from the 0-based zcr register, to the 1-base vq form, to the 0-based bitmap, and back again. This can be simplified by leaving the value in the 0-based form. Finally, use test_bit to simplify the common case, where the length in the zcr registers is in fact a supported length. Reported-by: Coverity (CID 1407217) Signed-off-by: Richard Henderson Reviewed-by: Andrew Jones Message-id: 20191118091414.19440-1-richard.henderson@linaro.org Signed-off-by: Peter Maydell --- target/arm/cpu.h | 3 --- target/arm/cpu64.c | 15 --------------- target/arm/helper.c | 9 +++++++-- 3 files changed, 7 insertions(+), 20 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index e1a66a2d1c..47d24a5375 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -185,12 +185,9 @@ typedef struct { #ifdef TARGET_AARCH64 # define ARM_MAX_VQ 16 void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp); -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq); #else # define ARM_MAX_VQ 1 static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { } -static inline uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq) -{ return 0; } #endif typedef struct ARMVectorReg { diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index 68baf0482f..a39d6fcea3 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -458,21 +458,6 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) cpu->sve_max_vq = max_vq; } -uint32_t arm_cpu_vq_map_next_smaller(ARMCPU *cpu, uint32_t vq) -{ - uint32_t bitnum; - - /* - * We allow vq == ARM_MAX_VQ + 1 to be input because the caller may want - * to find the maximum vq enabled, which may be ARM_MAX_VQ, but this - * function always returns the next smaller than the input. - */ - assert(vq && vq <= ARM_MAX_VQ + 1); - - bitnum = find_last_bit(cpu->sve_vq_map, vq - 1); - return bitnum == vq - 1 ? 0 : bitnum + 1; -} - static void cpu_max_get_sve_max_vq(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { diff --git a/target/arm/helper.c b/target/arm/helper.c index be67e2c66d..a089fb5a69 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5363,9 +5363,14 @@ int sve_exception_el(CPUARMState *env, int el) static uint32_t sve_zcr_get_valid_len(ARMCPU *cpu, uint32_t start_len) { - uint32_t start_vq = (start_len & 0xf) + 1; + uint32_t end_len; - return arm_cpu_vq_map_next_smaller(cpu, start_vq + 1) - 1; + end_len = start_len &= 0xf; + if (!test_bit(start_len, cpu->sve_vq_map)) { + end_len = find_last_bit(cpu->sve_vq_map, start_len); + assert(end_len < start_len); + } + return end_len; } /* From 3a6606c7aa1e6e687e435c1eae929a396bd866e3 Mon Sep 17 00:00:00 2001 From: Sai Pavan Boddu Date: Tue, 19 Nov 2019 13:20:27 +0000 Subject: [PATCH 3/7] ssi: xilinx_spips: Skip spi bus update for a few register writes A few configuration register writes need not update the spi bus state, so just return after the register write. Signed-off-by: Sai Pavan Boddu Reviewed-by: Alistair Francis Reviewed-by: Francisco Iglesias Tested-by: Francisco Iglesias Reviewed-by: Edgar E. Iglesias Message-id: 1573830705-14579-1-git-send-email-sai.pavan.boddu@xilinx.com Signed-off-by: Peter Maydell --- hw/ssi/xilinx_spips.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c index a309c712ca..0d6c2e1a61 100644 --- a/hw/ssi/xilinx_spips.c +++ b/hw/ssi/xilinx_spips.c @@ -109,6 +109,7 @@ #define R_GPIO (0x30 / 4) #define R_LPBK_DLY_ADJ (0x38 / 4) #define R_LPBK_DLY_ADJ_RESET (0x33) +#define R_IOU_TAPDLY_BYPASS (0x3C / 4) #define R_TXD1 (0x80 / 4) #define R_TXD2 (0x84 / 4) #define R_TXD3 (0x88 / 4) @@ -139,6 +140,8 @@ #define R_LQSPI_STS (0xA4 / 4) #define LQSPI_STS_WR_RECVD (1 << 1) +#define R_DUMMY_CYCLE_EN (0xC8 / 4) +#define R_ECO (0xF8 / 4) #define R_MOD_ID (0xFC / 4) #define R_GQSPI_SELECT (0x144 / 4) @@ -970,6 +973,7 @@ static void xilinx_spips_write(void *opaque, hwaddr addr, { int mask = ~0; XilinxSPIPS *s = opaque; + bool try_flush = true; DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr, (unsigned)value); addr >>= 2; @@ -1019,13 +1023,23 @@ static void xilinx_spips_write(void *opaque, hwaddr addr, tx_data_bytes(&s->tx_fifo, (uint32_t)value, 3, s->regs[R_CONFIG] & R_CONFIG_ENDIAN); goto no_reg_update; + /* Skip SPI bus update for below registers writes */ + case R_GPIO: + case R_LPBK_DLY_ADJ: + case R_IOU_TAPDLY_BYPASS: + case R_DUMMY_CYCLE_EN: + case R_ECO: + try_flush = false; + break; } s->regs[addr] = (s->regs[addr] & ~mask) | (value & mask); no_reg_update: - xilinx_spips_update_cs_lines(s); - xilinx_spips_check_flush(s); - xilinx_spips_update_cs_lines(s); - xilinx_spips_update_ixr(s); + if (try_flush) { + xilinx_spips_update_cs_lines(s); + xilinx_spips_check_flush(s); + xilinx_spips_update_cs_lines(s); + xilinx_spips_update_ixr(s); + } } static const MemoryRegionOps spips_ops = { From 6623d214451ec4f784d1c82c51c655a01fed1b06 Mon Sep 17 00:00:00 2001 From: Linus Ziegert Date: Tue, 19 Nov 2019 13:20:27 +0000 Subject: [PATCH 4/7] net/cadence_gem: Set PHY autonegotiation restart status The Linux kernel PHY driver sets AN_RESTART in the BMCR of the PHY when autonegotiation is started. Recently the kernel started to read back the PHY's AN_RESTART bit and now checks whether the autonegotiation is complete and the bit was cleared [1]. Otherwise the link status is down. The emulated PHY needs to clear AN_RESTART immediately to inform the kernel driver about the completion of autonegotiation phase. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c36757eb9dee Signed-off-by: Linus Ziegert Reviewed-by: Alistair Francis Message-id: 20191104181604.21943-1-linus.ziegert+qemu@holoplot.com Cc: qemu-stable@nongnu.org Signed-off-by: Peter Maydell --- hw/net/cadence_gem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index 7f9cb5ab95..b8be73dc55 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -271,9 +271,10 @@ #define PHY_REG_EXT_PHYSPCFC_ST 27 #define PHY_REG_CABLE_DIAG 28 -#define PHY_REG_CONTROL_RST 0x8000 -#define PHY_REG_CONTROL_LOOP 0x4000 -#define PHY_REG_CONTROL_ANEG 0x1000 +#define PHY_REG_CONTROL_RST 0x8000 +#define PHY_REG_CONTROL_LOOP 0x4000 +#define PHY_REG_CONTROL_ANEG 0x1000 +#define PHY_REG_CONTROL_ANRESTART 0x0200 #define PHY_REG_STATUS_LINK 0x0004 #define PHY_REG_STATUS_ANEGCMPL 0x0020 @@ -1345,7 +1346,7 @@ static void gem_phy_write(CadenceGEMState *s, unsigned reg_num, uint16_t val) } if (val & PHY_REG_CONTROL_ANEG) { /* Complete autonegotiation immediately */ - val &= ~PHY_REG_CONTROL_ANEG; + val &= ~(PHY_REG_CONTROL_ANEG | PHY_REG_CONTROL_ANRESTART); s->phy_regs[PHY_REG_STATUS] |= PHY_REG_STATUS_ANEGCMPL; } if (val & PHY_REG_CONTROL_LOOP) { From 655b02646dc175dc10666459b0a1e4346fc8d46a Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 19 Nov 2019 13:20:28 +0000 Subject: [PATCH 5/7] target/arm: Do not reject rt == rt2 for strexd There was too much cut and paste between ldrexd and strexd, as ldrexd does prohibit two output registers the same. Fixes: af288228995 Reported-by: Michael Goffioul Signed-off-by: Richard Henderson Message-id: 20191117090621.32425-2-richard.henderson@linaro.org Reviewed-by: Robert Foley Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/translate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/arm/translate.c b/target/arm/translate.c index 2ea9da7637..b285b23858 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -8934,7 +8934,7 @@ static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel) || (s->thumb && (a->rd == 13 || a->rt == 13)) || (mop == MO_64 && (a->rt2 == 15 - || a->rd == a->rt2 || a->rt == a->rt2 + || a->rd == a->rt2 || (s->thumb && a->rt2 == 13)))) { unallocated_encoding(s); return true; From d46ad79efac7aaf9f0eb9f5a96a576e9f39200e0 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 19 Nov 2019 13:20:28 +0000 Subject: [PATCH 6/7] target/arm: Relax r13 restriction for ldrex/strex for v8.0 Armv8-A removes UNPREDICTABLE for R13 for these cases. Signed-off-by: Richard Henderson Message-id: 20191117090621.32425-3-richard.henderson@linaro.org [PMM: changed ENABLE_ARCH_8 checks to check a new bool 'v8a', since these cases are still UNPREDICTABLE for v8M] Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/translate.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/target/arm/translate.c b/target/arm/translate.c index b285b23858..4d5d4bd888 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -8927,15 +8927,17 @@ static bool trans_SWPB(DisasContext *s, arg_SWP *a) static bool op_strex(DisasContext *s, arg_STREX *a, MemOp mop, bool rel) { TCGv_i32 addr; + /* Some cases stopped being UNPREDICTABLE in v8A (but not v8M) */ + bool v8a = ENABLE_ARCH_8 && !arm_dc_feature(s, ARM_FEATURE_M); /* We UNDEF for these UNPREDICTABLE cases. */ if (a->rd == 15 || a->rn == 15 || a->rt == 15 || a->rd == a->rn || a->rd == a->rt - || (s->thumb && (a->rd == 13 || a->rt == 13)) + || (!v8a && s->thumb && (a->rd == 13 || a->rt == 13)) || (mop == MO_64 && (a->rt2 == 15 || a->rd == a->rt2 - || (s->thumb && a->rt2 == 13)))) { + || (!v8a && s->thumb && a->rt2 == 13)))) { unallocated_encoding(s); return true; } @@ -9084,13 +9086,15 @@ static bool trans_STLH(DisasContext *s, arg_STL *a) static bool op_ldrex(DisasContext *s, arg_LDREX *a, MemOp mop, bool acq) { TCGv_i32 addr; + /* Some cases stopped being UNPREDICTABLE in v8A (but not v8M) */ + bool v8a = ENABLE_ARCH_8 && !arm_dc_feature(s, ARM_FEATURE_M); /* We UNDEF for these UNPREDICTABLE cases. */ if (a->rn == 15 || a->rt == 15 - || (s->thumb && a->rt == 13) + || (!v8a && s->thumb && a->rt == 13) || (mop == MO_64 && (a->rt2 == 15 || a->rt == a->rt2 - || (s->thumb && a->rt2 == 13)))) { + || (!v8a && s->thumb && a->rt2 == 13)))) { unallocated_encoding(s); return true; } From 04c9c81b8fa2ee33f59a26265700fae6fc646062 Mon Sep 17 00:00:00 2001 From: Richard Henderson Date: Tue, 19 Nov 2019 13:20:28 +0000 Subject: [PATCH 7/7] target/arm: Support EL0 v7m msr/mrs for CONFIG_USER_ONLY Simply moving the non-stub helper_v7m_mrs/msr outside of !CONFIG_USER_ONLY is not an option, because of all of the other system-mode helpers that are called. But we can split out a few subroutines to handle the few EL0 accessible registers without duplicating code. Reported-by: Christophe Lyon Signed-off-by: Richard Henderson Message-id: 20191118194916.3670-1-richard.henderson@linaro.org [PMM: deleted now-redundant comment; added a default case to switch in v7m_msr helper] Reviewed-by: Peter Maydell Signed-off-by: Peter Maydell --- target/arm/cpu.h | 2 + target/arm/m_helper.c | 114 ++++++++++++++++++++++++++---------------- 2 files changed, 73 insertions(+), 43 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 47d24a5375..83a809d4ba 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1314,6 +1314,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask) if (mask & XPSR_GE) { env->GE = (val & XPSR_GE) >> 16; } +#ifndef CONFIG_USER_ONLY if (mask & XPSR_T) { env->thumb = ((val & XPSR_T) != 0); } @@ -1329,6 +1330,7 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask) /* Note that this only happens on exception exit */ write_v7m_exception(env, val & XPSR_EXCP); } +#endif } #define HCR_VM (1ULL << 0) diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c index f2512e448e..4a48b79252 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -33,22 +33,82 @@ #include "exec/cpu_ldst.h" #endif +static void v7m_msr_xpsr(CPUARMState *env, uint32_t mask, + uint32_t reg, uint32_t val) +{ + /* Only APSR is actually writable */ + if (!(reg & 4)) { + uint32_t apsrmask = 0; + + if (mask & 8) { + apsrmask |= XPSR_NZCV | XPSR_Q; + } + if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) { + apsrmask |= XPSR_GE; + } + xpsr_write(env, val, apsrmask); + } +} + +static uint32_t v7m_mrs_xpsr(CPUARMState *env, uint32_t reg, unsigned el) +{ + uint32_t mask = 0; + + if ((reg & 1) && el) { + mask |= XPSR_EXCP; /* IPSR (unpriv. reads as zero) */ + } + if (!(reg & 4)) { + mask |= XPSR_NZCV | XPSR_Q; /* APSR */ + if (arm_feature(env, ARM_FEATURE_THUMB_DSP)) { + mask |= XPSR_GE; + } + } + /* EPSR reads as zero */ + return xpsr_read(env) & mask; +} + +static uint32_t v7m_mrs_control(CPUARMState *env, uint32_t secure) +{ + uint32_t value = env->v7m.control[secure]; + + if (!secure) { + /* SFPA is RAZ/WI from NS; FPCA is stored in the M_REG_S bank */ + value |= env->v7m.control[M_REG_S] & R_V7M_CONTROL_FPCA_MASK; + } + return value; +} + #ifdef CONFIG_USER_ONLY -/* These should probably raise undefined insn exceptions. */ -void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val) +void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val) { - ARMCPU *cpu = env_archcpu(env); + uint32_t mask = extract32(maskreg, 8, 4); + uint32_t reg = extract32(maskreg, 0, 8); - cpu_abort(CPU(cpu), "v7m_msr %d\n", reg); + switch (reg) { + case 0 ... 7: /* xPSR sub-fields */ + v7m_msr_xpsr(env, mask, reg, val); + break; + case 20: /* CONTROL */ + /* There are no sub-fields that are actually writable from EL0. */ + break; + default: + /* Unprivileged writes to other registers are ignored */ + break; + } } uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) { - ARMCPU *cpu = env_archcpu(env); - - cpu_abort(CPU(cpu), "v7m_mrs %d\n", reg); - return 0; + switch (reg) { + case 0 ... 7: /* xPSR sub-fields */ + return v7m_mrs_xpsr(env, reg, 0); + case 20: /* CONTROL */ + return v7m_mrs_control(env, 0); + default: + /* Unprivileged reads others as zero. */ + return 0; + } } void HELPER(v7m_bxns)(CPUARMState *env, uint32_t dest) @@ -2196,35 +2256,14 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs) uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg) { - uint32_t mask; unsigned el = arm_current_el(env); /* First handle registers which unprivileged can read */ - switch (reg) { case 0 ... 7: /* xPSR sub-fields */ - mask = 0; - if ((reg & 1) && el) { - mask |= XPSR_EXCP; /* IPSR (unpriv. reads as zero) */ - } - if (!(reg & 4)) { - mask |= XPSR_NZCV | XPSR_Q; /* APSR */ - if (arm_feature(env, ARM_FEATURE_THUMB_DSP)) { - mask |= XPSR_GE; - } - } - /* EPSR reads as zero */ - return xpsr_read(env) & mask; - break; + return v7m_mrs_xpsr(env, reg, el); case 20: /* CONTROL */ - { - uint32_t value = env->v7m.control[env->v7m.secure]; - if (!env->v7m.secure) { - /* SFPA is RAZ/WI from NS; FPCA is stored in the M_REG_S bank */ - value |= env->v7m.control[M_REG_S] & R_V7M_CONTROL_FPCA_MASK; - } - return value; - } + return v7m_mrs_control(env, env->v7m.secure); case 0x94: /* CONTROL_NS */ /* * We have to handle this here because unprivileged Secure code @@ -2454,18 +2493,7 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val) switch (reg) { case 0 ... 7: /* xPSR sub-fields */ - /* only APSR is actually writable */ - if (!(reg & 4)) { - uint32_t apsrmask = 0; - - if (mask & 8) { - apsrmask |= XPSR_NZCV | XPSR_Q; - } - if ((mask & 4) && arm_feature(env, ARM_FEATURE_THUMB_DSP)) { - apsrmask |= XPSR_GE; - } - xpsr_write(env, val, apsrmask); - } + v7m_msr_xpsr(env, mask, reg, val); break; case 8: /* MSP */ if (v7m_using_psp(env)) {