| Message ID | 1447031505-12477-2-git-send-email-mdavidsaver@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote: > The MRS and MSR instruction handling isn't checking > the current permission level. > > Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com> > --- > target-arm/helper.c | 79 +++++++++++++++++++++++++---------------------------- > 1 file changed, 37 insertions(+), 42 deletions(-) This patch looks good overall, but there's one style nit: > + case 0 ... 7: /* xPSR sub-fields */ > + mask = 0; > + if ((reg&1) && el) { you want spaces around operators, so "reg & 1" here and elsewhere. It would also be good to mention in the commit message the other things this patch is fixing: * privileged attempts to write EPSR should do nothing * accessing an unknown special register now triggers a guest-error warning rather than aborting QEMU thanks -- PMM
On 11/17/2015 12:09 PM, Peter Maydell wrote: > On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote: >> The MRS and MSR instruction handling isn't checking >> the current permission level. >> >> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com> >> --- >> target-arm/helper.c | 79 +++++++++++++++++++++++++---------------------------- >> 1 file changed, 37 insertions(+), 42 deletions(-) > > This patch looks good overall, but there's one style nit: > >> + case 0 ... 7: /* xPSR sub-fields */ >> + mask = 0; >> + if ((reg&1) && el) { > > you want spaces around operators, so "reg & 1" here and elsewhere. Would be nice if checkpatch.pl caught these, but I understand that this would be quite difficult to do well. I've tried to catch this with grep and sort through the false positives. I think I got them all. > It would also be good to mention in the commit message the > other things this patch is fixing: > * privileged attempts to write EPSR should do nothing > * accessing an unknown special register now triggers a > guest-error warning rather than aborting QEMU Will do.
On 2 December 2015 at 22:51, Michael Davidsaver <mdavidsaver@gmail.com> wrote: > > > On 11/17/2015 12:09 PM, Peter Maydell wrote: >> On 9 November 2015 at 01:11, Michael Davidsaver <mdavidsaver@gmail.com> wrote: >>> The MRS and MSR instruction handling isn't checking >>> the current permission level. >>> >>> Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com> >>> --- >>> target-arm/helper.c | 79 +++++++++++++++++++++++++---------------------------- >>> 1 file changed, 37 insertions(+), 42 deletions(-) >> >> This patch looks good overall, but there's one style nit: >> >>> + case 0 ... 7: /* xPSR sub-fields */ >>> + mask = 0; >>> + if ((reg&1) && el) { >> >> you want spaces around operators, so "reg & 1" here and elsewhere. > > Would be nice if checkpatch.pl caught these, but I understand that > this would be quite difficult to do well. I've tried to catch this > with grep and sort through the false positives. I think I got them > all. It looks like this is a case where checkpatch.pl is applying Linux kernel style (since that's where we borrowed it from) which is looser than ours, or at least looser than what I think our style is. There's a specific check: # << and >> may either have or not have spaces both sides } elsif ($op eq '<<' or $op eq '>>' or $op eq '&' or $op eq '^' or $op eq '|' or $op eq '+' or $op eq '-' or $op eq '*' or $op eq '/' or $op eq '%') and those operators are only checked for "must have consistent spacing both sides" rather than "must have spaces on both sides". thanks -- PMM
diff --git a/target-arm/helper.c b/target-arm/helper.c index 4ecae61..4408100 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -7365,23 +7365,32 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode) 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 */ + mask = 0; + if ((reg&1) && el) { + mask |= 0x000001ff; /* IPSR (unpriv. reads as zero) */ + } + if (!(reg&4)) { + mask |= 0xf8000000; /* APSR */ + } + /* EPSR reads as zero */ + return xpsr_read(env) & mask; + break; + case 20: /* CONTROL */ + return env->v7m.control; + } + + 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 */ @@ -7393,40 +7402,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 */ + } break; case 8: /* MSP */ if (env->v7m.current_sp) @@ -7467,8 +7462,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; } }
The MRS and MSR instruction handling isn't checking the current permission level. Signed-off-by: Michael Davidsaver <mdavidsaver@gmail.com> --- target-arm/helper.c | 79 +++++++++++++++++++++++++---------------------------- 1 file changed, 37 insertions(+), 42 deletions(-)