Peter Maydell <peter.mayd...@linaro.org> writes:
> From: Michael Davidsaver <mdavidsa...@gmail.com>
>
> The MRS and MSR instruction handling has a number of flaws:
> * unprivileged accesses should only be able to read
> CONTROL and the xPSR subfields, and only write APSR
> (others RAZ/WI)
> * privileged access should not be able to write xPSR
> subfields other than APSR
> * accesses to unimplemented registers should log as
> guest errors, not abort QEMU
>
> Signed-off-by: Michael Davidsaver <mdavidsa...@gmail.com>
> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
> [PMM: rewrote commit message]
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> ---
> target/arm/helper.c | 79
> +++++++++++++++++++++++++----------------------------
> 1 file changed, 37 insertions(+), 42 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 7111c8c..ad23de3 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8243,23 +8243,32 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState
> *cs, vaddr addr,
>
> uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
> {
> - ARMCPU *cpu = arm_env_get_cpu(env);
> + uint32_t mask;
> + unsigned el = arm_current_el(env);
> +
> + /* First handle registers which unprivileged can read */
> +
> + switch (reg) {
> + case 0 ... 7: /* xPSR sub-fields */
This reads a little confusingly compared to the pseudo-code in the ARM
ARM. Would it be clearer if we just went:
switch(extract32(reg, 3, 5)) {
case 0: /* xPSR */
...
case 1: /* SP */
...
case 2: /* Priority Mask or CONTROL.. */
...
}
?
> + mask = 0;
> + if ((reg & 1) && el) {
> + mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */
As B5.2.2 doesn't imply any particular access limit perhaps the comment
should read /* ISPR (reads as zero when not in exception) */
> + }
> + if (!(reg & 4)) {
> + mask |= 0xf8000000; /* APSR */
> + }
> + /* EPSR reads as zero */
> + return xpsr_read(env) & mask;
> + break;
> + case 20: /* CONTROL */
> + return env->v7m.control;
I'm fairly sure this was meant to be 0x20 and either way the result is
gated by current privilege.
> + }
> +
> + if (el == 0) {
> + return 0; /* unprivileged reads others as zero */
> + }
>
> switch (reg) {
> - case 0: /* APSR */
> - return xpsr_read(env) & 0xf8000000;
> - case 1: /* IAPSR */
> - return xpsr_read(env) & 0xf80001ff;
> - case 2: /* EAPSR */
> - return xpsr_read(env) & 0xff00fc00;
> - case 3: /* xPSR */
> - return xpsr_read(env) & 0xff00fdff;
> - case 5: /* IPSR */
> - return xpsr_read(env) & 0x000001ff;
> - case 6: /* EPSR */
> - return xpsr_read(env) & 0x0700fc00;
> - case 7: /* IEPSR */
> - return xpsr_read(env) & 0x0700edff;
> case 8: /* MSP */
> return env->v7m.current_sp ? env->v7m.other_sp : env->regs[13];
> case 9: /* PSP */
> @@ -8271,40 +8280,26 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t
> reg)
> return env->v7m.basepri;
> case 19: /* FAULTMASK */
> return (env->daif & PSTATE_F) != 0;
> - case 20: /* CONTROL */
> - return env->v7m.control;
> default:
> - /* ??? For debugging only. */
> - cpu_abort(CPU(cpu), "Unimplemented system register read (%d)\n",
> reg);
> + qemu_log_mask(LOG_GUEST_ERROR, "Attempt to read unknown special"
> + " register %d\n", reg);
> return 0;
> }
> }
>
> void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg, uint32_t val)
> {
> - ARMCPU *cpu = arm_env_get_cpu(env);
> + if (arm_current_el(env) == 0 && reg > 7) {
> + /* only xPSR sub-fields may be written by unprivileged */
> + return;
> + }
>
> switch (reg) {
> - case 0: /* APSR */
> - xpsr_write(env, val, 0xf8000000);
> - break;
> - case 1: /* IAPSR */
> - xpsr_write(env, val, 0xf8000000);
> - break;
> - case 2: /* EAPSR */
> - xpsr_write(env, val, 0xfe00fc00);
> - break;
> - case 3: /* xPSR */
> - xpsr_write(env, val, 0xfe00fc00);
> - break;
> - case 5: /* IPSR */
> - /* IPSR bits are readonly. */
> - break;
> - case 6: /* EPSR */
> - xpsr_write(env, val, 0x0600fc00);
> - break;
> - case 7: /* IEPSR */
> - xpsr_write(env, val, 0x0600fc00);
> + case 0 ... 7: /* xPSR sub-fields */
> + /* only APSR is actually writable */
> + if (reg & 4) {
> + xpsr_write(env, val, 0xf8000000); /* APSR */
> + }
I assuming insn<10> selects a different helper....
> break;
> case 8: /* MSP */
> if (env->v7m.current_sp)
> @@ -8345,8 +8340,8 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t reg,
> uint32_t val)
> switch_v7m_sp(env, (val & 2) != 0);
> break;
> default:
> - /* ??? For debugging only. */
> - cpu_abort(CPU(cpu), "Unimplemented system register write (%d)\n",
> reg);
> + qemu_log_mask(LOG_GUEST_ERROR, "Attempt to write unknown special"
> + " register %d\n", reg);
> return;
> }
> }
--
Alex Bennée
