[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] armv7m: MRS/MSR: handle unprivil
From: |
Alex Bennée |
Subject: |
Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access |
Date: |
Tue, 24 Jan 2017 16:25:47 +0000 |
User-agent: |
mu4e 0.9.19; emacs 25.1.91.4 |
Peter Maydell <address@hidden> writes:
> From: Michael Davidsaver <address@hidden>
>
> 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 <address@hidden>
> Reviewed-by: Peter Maydell <address@hidden>
> [PMM: rewrote commit message]
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> 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
- [Qemu-devel] [PATCH 0/6] ARMv7M: some simple bugfixes and cleanups, Peter Maydell, 2017/01/20
- [Qemu-devel] [PATCH 6/6] armv7m: Clear FAULTMASK on return from non-NMI exceptions, Peter Maydell, 2017/01/20
- [Qemu-devel] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access, Peter Maydell, 2017/01/20
- Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access,
Alex Bennée <=
- [Qemu-devel] [PATCH 3/6] armv7m: Explicit error for bad vector table, Peter Maydell, 2017/01/20
- [Qemu-devel] [PATCH 2/6] armv7m: Replace armv7m.hack with unassigned_access handler, Peter Maydell, 2017/01/20
- [Qemu-devel] [PATCH 5/6] armv7m: Fix reads of CONTROL register bit 1, Peter Maydell, 2017/01/20
- [Qemu-devel] [PATCH 4/6] hw/registerfields.h: Pull FIELD etc macros out of hw/register.h, Peter Maydell, 2017/01/20