For PMSAv7, the v7A/R Arm ARM defines that setting AP to 0b111
is an UNPREDICTABLE reserved combination. However, for v7M
this value is documented as having the same behaviour as 0b110:
read-only for both privileged and unprivileged. Accept this
value on an M profile core rather than treating it as a guest
error and a no-access page.

Reported-by: Andy Gross <andy.gross@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 91a9300..2f53dd8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9229,6 +9229,13 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                 case 6:
                     *prot |= PAGE_READ | PAGE_EXEC;
                     break;
+                case 7:
+                    /* for v7M, same as 6; for R profile a reserved value */
+                    if (arm_feature(env, ARM_FEATURE_M)) {
+                        *prot |= PAGE_READ | PAGE_EXEC;
+                        break;
+                    }
+                    /* fall through */
                 default:
                     qemu_log_mask(LOG_GUEST_ERROR,
                                   "DRACR[%d]: Bad value for AP bits: 0x%"
@@ -9247,6 +9254,13 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
                 case 6:
                     *prot |= PAGE_READ | PAGE_EXEC;
                     break;
+                case 7:
+                    /* for v7M, same as 6; for R profile a reserved value */
+                    if (arm_feature(env, ARM_FEATURE_M)) {
+                        *prot |= PAGE_READ | PAGE_EXEC;
+                        break;
+                    }
+                    /* fall through */
                 default:
                     qemu_log_mask(LOG_GUEST_ERROR,
                                   "DRACR[%d]: Bad value for AP bits: 0x%"
-- 
2.7.4

Ping for code review?

thanks
-- PMM

On 8 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> For PMSAv7, the v7A/R Arm ARM defines that setting AP to 0b111
> is an UNPREDICTABLE reserved combination. However, for v7M
> this value is documented as having the same behaviour as 0b110:
> read-only for both privileged and unprivileged. Accept this
> value on an M profile core rather than treating it as a guest
> error and a no-access page.
>
> Reported-by: Andy Gross <andy.gross@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 91a9300..2f53dd8 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9229,6 +9229,13 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                  case 6:
>                      *prot |= PAGE_READ | PAGE_EXEC;
>                      break;
> +                case 7:
> +                    /* for v7M, same as 6; for R profile a reserved value */
> +                    if (arm_feature(env, ARM_FEATURE_M)) {
> +                        *prot |= PAGE_READ | PAGE_EXEC;
> +                        break;
> +                    }
> +                    /* fall through */
>                  default:
>                      qemu_log_mask(LOG_GUEST_ERROR,
>                                    "DRACR[%d]: Bad value for AP bits: 0x%"
> @@ -9247,6 +9254,13 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>                  case 6:
>                      *prot |= PAGE_READ | PAGE_EXEC;
>                      break;
> +                case 7:
> +                    /* for v7M, same as 6; for R profile a reserved value */
> +                    if (arm_feature(env, ARM_FEATURE_M)) {
> +                        *prot |= PAGE_READ | PAGE_EXEC;
> +                        break;
> +                    }
> +                    /* fall through */
>                  default:
>                      qemu_log_mask(LOG_GUEST_ERROR,
>                                    "DRACR[%d]: Bad value for AP bits: 0x%"
> --
> 2.7.4

Hi Peter, Andy,

> On 8 December 2017 at 14:13, Peter Maydell <peter.maydell@linaro.org> wrote:
>> For PMSAv7, the v7A/R Arm ARM defines that setting AP to 0b111
>> is an UNPREDICTABLE reserved combination. However, for v7M
>> this value is documented as having the same behaviour as 0b110:
>> read-only for both privileged and unprivileged. Accept this
>> value on an M profile core rather than treating it as a guest
>> error and a no-access page.

So, checking "Access permission checking":

v7-AR:
 case perms.ap of
   when ‘000’ abort = TRUE;
   when ‘001’ abort = !ispriv;
   when ‘010’ abort = !ispriv && iswrite;
   when ‘011’ abort = FALSE;
   when ‘100’ UNPREDICTABLE;
   when ‘101’ abort = !ispriv || iswrite;
   when ‘110’ abort = iswrite;
   when ‘111’
     if MemorySystemArchitecture() == MemArch_VMSA then
       abort = iswrite
     else
       UNPREDICTABLE;

v7-M:
 case perms.ap of
   when ‘000’ fault = TRUE;
   when ‘001’ fault = !ispriv;
   when ‘010’ fault = !ispriv && iswrite;
   when ‘011’ fault = FALSE;
   when ‘100’ UNPREDICTABLE;
   when ‘101’ fault = !ispriv || iswrite;
   when ‘110’ fault = iswrite;
   when ‘111’ fault = iswrite;
   otherwise UNPREDICTABLE;

You are indeed correct.

Having a 3bits perms.ap, I wonder how you can reach the 'otherwise'
case... :)

// Access permissions descriptor
type Permissions is (
 bits(3) ap, // Access Permission bits
 bit xn // Execute Never bit
)

This appears in the "Access permissions field encoding" table:

```
The AP bits, AP[2:0], are used for access permissions.
...
110 Read-only Read-only Privileged and unprivileged read-only
111 Read-only Read-only Privileged and unprivileged read-only
```

However I think this v7-M PMSAv7 behaviour is not obviously
differentiated in the  Architecture Reference Manual:

```
ARMv7-M supports the standard PMSAv7 of the ARMv7-R architecture
profile, with the following extensions:

• An optimized two register update model, where software can select the
region to update by writing to the
MPU Region Base Address Register. This optimization applies to the first
sixteen memory regions (0 ≤
RegionNumber ≤ 0xF) only.

• The MPU Region Base Address Register and the MPU Region Attribute and
Size Register pairs are aliased
in three consecutive dual-word locations. Using the two register update
model, software can modify up to
four regions by writing the appropriate even number of words using a
single STM multi-word store instruction.
```

They might add smth such:

"and with the following differences/restrictions: ..."

>>
>> Reported-by: Andy Gross <andy.gross@linaro.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>> ---
>>  target/arm/helper.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 91a9300..2f53dd8 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -9229,6 +9229,13 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>                  case 6:
>>                      *prot |= PAGE_READ | PAGE_EXEC;
>>                      break;
>> +                case 7:
>> +                    /* for v7M, same as 6; for R profile a reserved value */
>> +                    if (arm_feature(env, ARM_FEATURE_M)) {
>> +                        *prot |= PAGE_READ | PAGE_EXEC;
>> +                        break;
>> +                    }
>> +                    /* fall through */
>>                  default:
>>                      qemu_log_mask(LOG_GUEST_ERROR,
>>                                    "DRACR[%d]: Bad value for AP bits: 0x%"
>> @@ -9247,6 +9254,13 @@ static bool get_phys_addr_pmsav7(CPUARMState *env, uint32_t address,
>>                  case 6:
>>                      *prot |= PAGE_READ | PAGE_EXEC;
>>                      break;
>> +                case 7:
>> +                    /* for v7M, same as 6; for R profile a reserved value */
>> +                    if (arm_feature(env, ARM_FEATURE_M)) {
>> +                        *prot |= PAGE_READ | PAGE_EXEC;
>> +                        break;
>> +                    }
>> +                    /* fall through */
>>                  default:
>>                      qemu_log_mask(LOG_GUEST_ERROR,
>>                                    "DRACR[%d]: Bad value for AP bits: 0x%"
>> --
>> 2.7.4
>