Re: [Qemu-devel] [PATCH 6/6] ARM: enable PMSAv7-style MPU on Cortex-M3/M4

[Alex Züpke]

Am 14.07.2015 um 19:49 schrieb Peter Maydell:
> On 7 July 2015 at 19:25, Alex Zuepke <address@hidden> wrote:
> 
> A commit message that wasn't just the one-line summary would
> be nice. Sometimes a patch really is trivial enough that it's
> not worth describing in more than just a single line, but
> those situations are the exception rather than the rule.

OK.

> [snip]
>> @@ -406,6 +436,57 @@ static void nvic_writel(nvic_state *s, uint32_t offset, 
>> uint32_t value)
>>          qemu_log_mask(LOG_UNIMP,
>>                        "NVIC: AUX fault status registers unimplemented\n");
>>          break;
>> +    case 0xd94: /* MPU control register.  */
>> +        cpu = ARM_CPU(current_cpu);
>> +        if (cpu->pmsav7_dregion == 0)
>> +            break;
>> +        cpu->env.v7m.mpu_ctrl = value & 0x7;
>> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_ENABLE)
>> +            cpu->env.cp15.sctlr_ns |= SCTLR_M;
>> +        else
>> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_M;
>> +        /* TODO: mimic MPU_CTRL_HFNMIENA */
> 
> That will be interesting to implement.
> 
>> +        if (cpu->env.v7m.mpu_ctrl & MPU_CTRL_PRIVDEFENA)
>> +            cpu->env.cp15.sctlr_ns |= SCTLR_BR;
>> +        else
>> +            cpu->env.cp15.sctlr_ns &= ~SCTLR_BR;
> 
> This is kind of ugly. Wouldn't it be nicer to make the code
> which tests for MMU-enabled and privdefena be M-profile aware?

Yes, but I wanted to keep the impact as small as possible ...
As a general question: is it OK to (mis-)use the existing cp15 register 
infrastructure for that,
or should I put the MPU enable bits, region number, etc into dedicated 
MPU-related sub structures in CPUARMState?

> [snip]
>> @@ -445,6 +526,19 @@ static uint64_t nvic_sysreg_read(void *opaque, hwaddr 
>> addr,
>>              return val & 0xffff;
>>          }
>>          break;
>> +    case 0xda0 ... 0xdb7: /* MPU_RSAR and aliases.  */
>> +        cpu = ARM_CPU(current_cpu);
>> +        if (cpu->pmsav7_dregion == 0)
>> +            break;
>> +        if ((size == 2) && (offset & 7) == 0) {
>> +            val = cpu->env.pmsav7.drsr[cpu->env.cp15.c6_rgnr];
>> +            return val & 0xffff;
>> +        }
>> +        if ((size == 2) && (offset & 7) == 2) {
>> +            val = cpu->env.pmsav7.dracr[cpu->env.cp15.c6_rgnr];
>> +            return val & 0xffff;
>> +        }
> 
> RASR is UNPREDICTABLE for non-word-size access, so we don't need
> this at all.

It's from ARM recommended sample code:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0553a/BIHHHDDJ.html


Thanks a lot for the reviews!
I'll send a new set of patches next week.



Best regards
Alex