[04/11] target/arm: Record the GP bit for a page in MemTxAttrs

Message ID 20190110121736.23448-5-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Implement ARMv8.5-BTI
Related show

Commit Message

Richard Henderson Jan. 10, 2019, 12:17 p.m.
This isn't really a transaction attribute, but that's the most
convenient place to hold a random bit of information within the
softmmu tlb.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 include/exec/memattrs.h | 2 ++
 target/arm/helper.c     | 6 ++++++
 2 files changed, 8 insertions(+)

-- 
2.17.2

Comments

Peter Maydell Jan. 22, 2019, 1:26 p.m. | #1
On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> This isn't really a transaction attribute, but that's the most

> convenient place to hold a random bit of information within the

> softmmu tlb.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  include/exec/memattrs.h | 2 ++

>  target/arm/helper.c     | 6 ++++++

>  2 files changed, 8 insertions(+)

>

> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h

> index d4a1642098..39d61188e1 100644

> --- a/include/exec/memattrs.h

> +++ b/include/exec/memattrs.h

> @@ -35,6 +35,8 @@ typedef struct MemTxAttrs {

>      unsigned int secure:1;

>      /* Memory access is usermode (unprivileged) */

>      unsigned int user:1;

> +    /* Page is marked as "guarded" */

> +    unsigned int guarded:1;


Given that this isn't a real transaction attribute in the traditional
sense, and it's pretty Arm-specific, I think we could do with a
more expansive comment than this...

>      /* Requester ID (for MSI for example) */

>      unsigned int requester_id:16;

>  } MemTxAttrs;

> diff --git a/target/arm/helper.c b/target/arm/helper.c

> index 138d9d5565..4e9ea2ed39 100644

> --- a/target/arm/helper.c

> +++ b/target/arm/helper.c

> @@ -9927,6 +9927,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,

>      bool ttbr1_valid;

>      uint64_t descaddrmask;

>      bool aarch64 = arm_el_is_aa64(env, el);

> +    bool guarded = false;

>

>      /* TODO:

>       * This code does not handle the different format TCR for VTCR_EL2.

> @@ -10098,6 +10099,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,

>          }

>          /* Merge in attributes from table descriptors */

>          attrs |= nstable << 3; /* NS */

> +        guarded |= extract64(descriptor, 50, 1);  /* GP */


Do we need to do the logical-OR here? Since this is a
block/page entry bit with no similar bit in the table
descriptors, there's no merging to be done (ie we
only execute this code once and 'guarded' will always
be 'false' before execution of the |=.)

>          if (param.hpd) {

>              /* HPD disables all the table attributes except NSTable.  */

>              break;

> @@ -10143,6 +10145,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,

>           */

>          txattrs->secure = false;

>      }

> +    /* When in aarch64 mode, and BTI is enabled, remember GP in the IOTLB.  */

> +    if (aarch64 && guarded && cpu_isar_feature(aa64_bti, cpu)) {

> +        txattrs->guarded = true;

> +    }

>

>      if (cacheattrs != NULL) {

>          if (mmu_idx == ARMMMUIdx_S2NS) {

> --

> 2.17.2

>


thanks
-- PMM
Richard Henderson Jan. 28, 2019, 9:08 p.m. | #2
On 1/22/19 5:26 AM, Peter Maydell wrote:
> On Thu, 10 Jan 2019 at 12:17, Richard Henderson

> <richard.henderson@linaro.org> wrote:

>>

>> This isn't really a transaction attribute, but that's the most

>> convenient place to hold a random bit of information within the

>> softmmu tlb.

>>

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>  include/exec/memattrs.h | 2 ++

>>  target/arm/helper.c     | 6 ++++++

>>  2 files changed, 8 insertions(+)

>>

>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h

>> index d4a1642098..39d61188e1 100644

>> --- a/include/exec/memattrs.h

>> +++ b/include/exec/memattrs.h

>> @@ -35,6 +35,8 @@ typedef struct MemTxAttrs {

>>      unsigned int secure:1;

>>      /* Memory access is usermode (unprivileged) */

>>      unsigned int user:1;

>> +    /* Page is marked as "guarded" */

>> +    unsigned int guarded:1;

> 

> Given that this isn't a real transaction attribute in the traditional

> sense, and it's pretty Arm-specific, I think we could do with a

> more expansive comment than this...


I have split this out to a separate patch, rearranged this to
target_tlb_bit[0-2], with a large block comment.  We will need some more of
these bits for for system mode v8.5-MemTag anyway.


>> +        guarded |= extract64(descriptor, 50, 1);  /* GP */

> 

> Do we need to do the logical-OR here? Since this is a

> block/page entry bit with no similar bit in the table

> descriptors, there's no merging to be done (ie we

> only execute this code once and 'guarded' will always

> be 'false' before execution of the |=.)


The document that I have has exactly one sentence about this, and does not
specify whether the bit is akin to the page table attributes (which appear at
every table level) or not.

As written above, this will execute more than once.


r~
Peter Maydell Jan. 29, 2019, 9:55 a.m. | #3
On Mon, 28 Jan 2019 at 21:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 1/22/19 5:26 AM, Peter Maydell wrote:

> >> +        guarded |= extract64(descriptor, 50, 1);  /* GP */

> >

> > Do we need to do the logical-OR here? Since this is a

> > block/page entry bit with no similar bit in the table

> > descriptors, there's no merging to be done (ie we

> > only execute this code once and 'guarded' will always

> > be 'false' before execution of the |=.)

>

> The document that I have has exactly one sentence about this, and does not

> specify whether the bit is akin to the page table attributes (which appear at

> every table level) or not.


Translation table descriptor formats come in four flavours:
 * Invalid
 * Table (which gives the address of the next level table)
 * Block (which gives the address of a large lump of memory)
 * Page (which gives the address of a page)

The GP bit documented to be in Block and Page entries, not
Table (which is how you've coded it).

> As written above, this will execute more than once.


I don't see how -- all the code paths forward from
"guarded |= extract64(descriptor, 50, 1);" reach a
"break" statement that terminates the loop, don't they?

thanks
-- PMM
Richard Henderson Jan. 29, 2019, 2:38 p.m. | #4
On 1/29/19 1:55 AM, Peter Maydell wrote:
>> As written above, this will execute more than once.

> 

> I don't see how -- all the code paths forward from

> "guarded |= extract64(descriptor, 50, 1);" reach a

> "break" statement that terminates the loop, don't they?


You're right.  I've misread the surrounding code.


r~

Patch

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d4a1642098..39d61188e1 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -35,6 +35,8 @@  typedef struct MemTxAttrs {
     unsigned int secure:1;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
+    /* Page is marked as "guarded" */
+    unsigned int guarded:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
 } MemTxAttrs;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 138d9d5565..4e9ea2ed39 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9927,6 +9927,7 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     bool ttbr1_valid;
     uint64_t descaddrmask;
     bool aarch64 = arm_el_is_aa64(env, el);
+    bool guarded = false;
 
     /* TODO:
      * This code does not handle the different format TCR for VTCR_EL2.
@@ -10098,6 +10099,7 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         }
         /* Merge in attributes from table descriptors */
         attrs |= nstable << 3; /* NS */
+        guarded |= extract64(descriptor, 50, 1);  /* GP */
         if (param.hpd) {
             /* HPD disables all the table attributes except NSTable.  */
             break;
@@ -10143,6 +10145,10 @@  static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
          */
         txattrs->secure = false;
     }
+    /* When in aarch64 mode, and BTI is enabled, remember GP in the IOTLB.  */
+    if (aarch64 && guarded && cpu_isar_feature(aa64_bti, cpu)) {
+        txattrs->guarded = true;
+    }
 
     if (cacheattrs != NULL) {
         if (mmu_idx == ARMMMUIdx_S2NS) {