Re: [Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Michael Davidsaver]

On 11/17/2015 12:09 PM, Peter Maydell wrote:
> On 9 November 2015 at 01:11, Michael Davidsaver  wrote:
>> The MRS and MSR instruction handling isn't checking
>> the current permission level.
>>
>> Signed-off-by: Michael Davidsaver 
>> ---
>>  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.

Re: [Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Peter Maydell]

On 2 December 2015 at 22:51, Michael Davidsaver  wrote:
>
>
> On 11/17/2015 12:09 PM, Peter Maydell wrote:
>> On 9 November 2015 at 01:11, Michael Davidsaver  
>> wrote:
>>> The MRS and MSR instruction handling isn't checking
>>> the current permission level.
>>>
>>> Signed-off-by: Michael Davidsaver 
>>> ---
>>>  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

Re: [Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Peter Maydell]

On 9 November 2015 at 01:11, Michael Davidsaver  wrote:
> The MRS and MSR instruction handling isn't checking
> the current permission level.
>
> Signed-off-by: Michael Davidsaver 
> ---
>  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

[Qemu-devel] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Michael Davidsaver]

The MRS and MSR instruction handling isn't checking
the current permission level.

Signed-off-by: Michael Davidsaver 
---
 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 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 |= 0x01ff; /* IPSR (unpriv. reads as zero) */
+}
+if (!(reg&4)) {
+mask |= 0xf800; /* 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) & 0xf800;
-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) & 0x01ff;
-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, 0xf800);
-break;
-case 1: /* IAPSR */
-xpsr_write(env, val, 0xf800);
-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, 0xf800); /* 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;
 }
 }
-- 
2.1.4