Re: [Qemu-devel] [PATCH 04/11] target/arm: Record the GP bit for a page in MemTxAttrs

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<address@hidden> 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 <address@hidden>
> ---
>  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