Re: [Qemu-arm] [PATCH 1/6] armv7m: MRS/MSR: handle unprivileged access

[Peter Maydell]

On 24 January 2017 at 16:25, Alex Bennée <address@hidden> wrote:
>
> 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.. */
>     ...
>   }
>
> ?

I think a simple switch on the register number is rather
less confusing than the pseudocode's nested switches on
the two halves of the register number -- this is a case where
we don't have to follow the pseudocode if it phrases something
weirdly.

>> +        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) */

B5.2.2 Notes says "If unprivileged code attempts to read [...] the IPSR,
the read returns zero". In the pseudocode this is handled by an
architectural guarantee that you can't get here in unprivileged
mode with an IPSR value that isn't zero; however given the current
state of the QEMU code I'm not really prepared to make that assertion,
so it seems better to just report 0 here.

>> +        }
>> +        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.

No, it's decimal 20, binary 0b10100. CONTROL is the case
of 00010 in the outer switch and 100 in the inner, and
it isn't privilege-checked. (See how confusing the
pseudocode phrasing is ? :-))

>> +    }
>> +
>> +    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....

insn<10> is mask<0> -- the behaviour is UNPREDICTABLE unless
the DSP extension is implemented. This code all predates the
attempt to support that in our cortex-m4 model (and indeed
predates the existence of M profile DSP extns), so it doesn't
care about the mask bits. I think the translate.c code is
probably just going to get that decode wrong at the moment.
We should tighten it up but it will likely be a different
helper and definitely a separate patch.

>>          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;
>>      }
>>  }

thanks
-- PMM