diff mbox series

[v2,07/21] include/exec/memattrs: Add two bits of space to MemTxAttrs

Message ID 20230220232626.429947-8-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: Implement FEAT_RME | expand

Commit Message

Richard Henderson Feb. 20, 2023, 11:26 p.m. UTC
We will need 2 bits to represent ARMSecurityState.

Do not attempt to replace or widen secure, even though it
logically overlaps the new field -- there are uses within
e.g. hw/block/pflash_cfi01.c, which don't know anything
specific about ARM.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/memattrs.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Feb. 21, 2023, 7:56 a.m. UTC | #1
On 21/2/23 00:26, Richard Henderson wrote:
> We will need 2 bits to represent ARMSecurityState.
> 
> Do not attempt to replace or widen secure, even though it
> logically overlaps the new field -- there are uses within
> e.g. hw/block/pflash_cfi01.c, which don't know anything
> specific about ARM.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/exec/memattrs.h | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index 9fb98bc1ef..d04170aa27 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -29,10 +29,17 @@ typedef struct MemTxAttrs {
>        * "didn't specify" if necessary.
>        */
>       unsigned int unspecified:1;
> -    /* ARM/AMBA: TrustZone Secure access
> +    /*
> +     * ARM/AMBA: TrustZone Secure access
>        * x86: System Management Mode access
>        */
>       unsigned int secure:1;
> +    /*
> +     * ARM: ArmSecuritySpace.  This partially overlaps secure, but it is
> +     * easier to have both fields to assist code that does not understand
> +     * ARMv9 RME, or no specific knowledge of ARM at all (e.g. pflash).
> +     */
> +    unsigned int space:2;
>       /* Memory access is usermode (unprivileged) */
>       unsigned int user:1;

'secure' & 'user' seem mutually exclusive. If we get short in bits,
they could be shared.
Richard Henderson Feb. 21, 2023, 10:01 a.m. UTC | #2
On 2/20/23 21:56, Philippe Mathieu-Daudé wrote:
> 'secure' & 'user' seem mutually exclusive. If we get short in bits,
> they could be shared.

They are not.  ARM has Secure EL0, or secure user mode.


r~
Philippe Mathieu-Daudé Feb. 21, 2023, 10:42 a.m. UTC | #3
On 21/2/23 11:01, Richard Henderson wrote:
> On 2/20/23 21:56, Philippe Mathieu-Daudé wrote:
>> 'secure' & 'user' seem mutually exclusive. If we get short in bits,
>> they could be shared.
> 
> They are not.  ARM has Secure EL0, or secure user mode.

Oh, I misunderstood this field with user-emulation then (I tried
commenting it and my TCG/HVF build succeeded).
Richard Henderson Feb. 21, 2023, 10:50 a.m. UTC | #4
On 2/21/23 00:42, Philippe Mathieu-Daudé wrote:
> On 21/2/23 11:01, Richard Henderson wrote:
>> On 2/20/23 21:56, Philippe Mathieu-Daudé wrote:
>>> 'secure' & 'user' seem mutually exclusive. If we get short in bits,
>>> they could be shared.
>>
>> They are not.  ARM has Secure EL0, or secure user mode.
> 
> Oh, I misunderstood this field with user-emulation then (I tried
> commenting it and my TCG/HVF build succeeded).
> 

target/arm/ptw.c:2853:    result->f.attrs.user = regime_is_user(env, mmu_idx);

So... it shouldn't have built?

r~
Philippe Mathieu-Daudé Feb. 21, 2023, 11:38 a.m. UTC | #5
On 21/2/23 11:50, Richard Henderson wrote:
> On 2/21/23 00:42, Philippe Mathieu-Daudé wrote:
>> On 21/2/23 11:01, Richard Henderson wrote:
>>> On 2/20/23 21:56, Philippe Mathieu-Daudé wrote:
>>>> 'secure' & 'user' seem mutually exclusive. If we get short in bits,
>>>> they could be shared.
>>>
>>> They are not.  ARM has Secure EL0, or secure user mode.
>>
>> Oh, I misunderstood this field with user-emulation then (I tried
>> commenting it and my TCG/HVF build succeeded).
>>
> 
> target/arm/ptw.c:2853:    result->f.attrs.user = regime_is_user(env, 
> mmu_idx);
> 
> So... it shouldn't have built?

Eh correct... I guess I wasn't sitting in a directory with ARM target
selected when I tried that 🤦

../../hw/misc/armv7m_ras.c:19:15: error: no member named 'user' in 
'struct MemTxAttrs'
     if (attrs.user) {
         ~~~~~ ^
diff mbox series

Patch

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index 9fb98bc1ef..d04170aa27 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -29,10 +29,17 @@  typedef struct MemTxAttrs {
      * "didn't specify" if necessary.
      */
     unsigned int unspecified:1;
-    /* ARM/AMBA: TrustZone Secure access
+    /*
+     * ARM/AMBA: TrustZone Secure access
      * x86: System Management Mode access
      */
     unsigned int secure:1;
+    /*
+     * ARM: ArmSecuritySpace.  This partially overlaps secure, but it is
+     * easier to have both fields to assist code that does not understand
+     * ARMv9 RME, or no specific knowledge of ARM at all (e.g. pflash).
+     */
+    unsigned int space:2;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
     /*