[PATCH 0/5] target/arm: Fixes for RME

[PATCH 0/5] target/arm: Fixes for RME

[Jean-Philippe Brucker]

With these patches I'm able to boot a Realm guest under
"-cpu max,x-rme=on". They are based on Peter's series which fixes
handling of NSTable:
https://lore.kernel.org/qemu-devel/20230714154648.327466-1-peter.maydell@linaro.org/


Running a Realm guest requires components at EL3 and R-EL2. Some rough
support for TF-A and RMM is available here:
https://jpbrucker.net/git/tf-a/log/?h=qemu-rme
https://jpbrucker.net/git/rmm/log/?h=qemu-rme
I'll clean this up before sending it out.

I also need to manually disable FEAT_SME in QEMU in order to boot this,
otherwise the Linux host fails to boot because hyp-stub accesses to SME
regs are trapped to EL3, which doesn't support RME+SME at the moment.
The right fix is probably in TF-A but I haven't investigated yet.

Jean-Philippe Brucker (5):
  target/arm/ptw: Load stage-2 tables from realm physical space
  target/arm/helper: Fix vae2_tlbmask()
  target/arm: Skip granule protection checks for AT instructions
  target/arm: Pass security space rather than flag for AT instructions
  target/arm/helper: Implement CNTHCTL_EL2.CNT[VP]MASK

 target/arm/internals.h | 25 ++++++++------
 target/arm/helper.c    | 78 ++++++++++++++++++++++++++++--------------
 target/arm/ptw.c       | 19 ++++++----
 3 files changed, 79 insertions(+), 43 deletions(-)

-- 
2.41.0

[PATCH 1/5] target/arm/ptw: Load stage-2 tables from realm physical space

[Jean-Philippe Brucker]

In realm state, stage-2 translation tables are fetched from the realm
physical address space (R_PGRQD).

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 target/arm/ptw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index d1de934702..6318e13b98 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -164,7 +164,11 @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, ARMMMUIdx stage2idx)
      * an NS stage 1+2 lookup while the NS bit is 0.)
      */
     if (!arm_is_secure_below_el3(env) || !arm_el_is_aa64(env, 3)) {
-        return ARMMMUIdx_Phys_NS;
+        if (arm_security_space_below_el3(env) == ARMSS_Realm) {
+            return ARMMMUIdx_Phys_Realm;
+        } else {
+            return ARMMMUIdx_Phys_NS;
+        }
     }
     if (stage2idx == ARMMMUIdx_Stage2_S) {
         s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);
-- 
2.41.0

Re: [PATCH 1/5] target/arm/ptw: Load stage-2 tables from realm physical space

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> In realm state, stage-2 translation tables are fetched from the realm
> physical address space (R_PGRQD).
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  target/arm/ptw.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/ptw.c b/target/arm/ptw.c
> index d1de934702..6318e13b98 100644
> --- a/target/arm/ptw.c
> +++ b/target/arm/ptw.c
> @@ -164,7 +164,11 @@ static ARMMMUIdx ptw_idx_for_stage_2(CPUARMState *env, ARMMMUIdx stage2idx)
>       * an NS stage 1+2 lookup while the NS bit is 0.)
>       */
>      if (!arm_is_secure_below_el3(env) || !arm_el_is_aa64(env, 3)) {
> -        return ARMMMUIdx_Phys_NS;
> +        if (arm_security_space_below_el3(env) == ARMSS_Realm) {
> +            return ARMMMUIdx_Phys_Realm;
> +        } else {
> +            return ARMMMUIdx_Phys_NS;
> +        }
>      }
>      if (stage2idx == ARMMMUIdx_Stage2_S) {
>          s2walk_secure = !(env->cp15.vstcr_el2 & VSTCR_SW);

This isn't wrong, but arm_is_secure_below_el3()
calls arm_security_space_below_el3(), so we kinda
duplicate work there. I think we should instead have:

    if (!arm_el_is_aa64(env, 3)) {
        return ARMMMUIdx_Phys_NS;
    }

    switch (arm_security_space_below_el3(env)) {
    case ARMSS_NonSecure:
        return ARMMUIdx_Phys_NS;
    case ARMSS_Realm:
        return ARMMMUIdx_Phys_Realm;
    case ARMSS_Secure:
        [existing code to look at the SW/NSW bits]
        return s2walk_secure ? ...;
    default:
        g_assert_not_reached();
    }

The comment above the function also needs tweaking
to say "SCR_EL3.NS or SCR_EL3.NSE bits" (we do already
do the TLB flush in scr_write).

thanks
-- PMM

[PATCH 2/5] target/arm/helper: Fix vae2_tlbmask()

[Jean-Philippe Brucker]

When HCR_EL2.E2H is enabled, TLB entries are formed using the EL2&0
translation regime, instead of the EL2 translation regime. The TLB VAE2*
instructions invalidate the regime that corresponds to the current value
of HCR_EL2.E2H.

At the moment we only invalidate the EL2 translation regime. This causes
problems with RMM, which issues TLBI VAE2IS instructions with
HCR_EL2.E2H enabled. Update vae2_tlbmask() to take HCR_EL2.E2H into
account.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 target/arm/helper.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index e1b3db6f5f..07a9ac70f5 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -4663,6 +4663,21 @@ static int vae1_tlbmask(CPUARMState *env)
     return mask;
 }
 
+static int vae2_tlbmask(CPUARMState *env)
+{
+    uint64_t hcr = arm_hcr_el2_eff(env);
+    uint16_t mask;
+
+    if (hcr & HCR_E2H) {
+        mask = ARMMMUIdxBit_E20_2 |
+               ARMMMUIdxBit_E20_2_PAN |
+               ARMMMUIdxBit_E20_0;
+    } else {
+        mask = ARMMMUIdxBit_E2;
+    }
+    return mask;
+}
+
 /* Return 56 if TBI is enabled, 64 otherwise. */
 static int tlbbits_for_regime(CPUARMState *env, ARMMMUIdx mmu_idx,
                               uint64_t addr)
@@ -4781,7 +4796,7 @@ static void tlbi_aa64_vae2_write(CPUARMState *env, const ARMCPRegInfo *ri,
      * flush-last-level-only.
      */
     CPUState *cs = env_cpu(env);
-    int mask = e2_tlbmask(env);
+    int mask = vae2_tlbmask(env);
     uint64_t pageaddr = sextract64(value << 12, 0, 56);
 
     tlb_flush_page_by_mmuidx(cs, pageaddr, mask);
@@ -4838,11 +4853,11 @@ static void tlbi_aa64_vae2is_write(CPUARMState *env, const ARMCPRegInfo *ri,
                                    uint64_t value)
 {
     CPUState *cs = env_cpu(env);
+    int mask = vae2_tlbmask(env);
     uint64_t pageaddr = sextract64(value << 12, 0, 56);
     int bits = tlbbits_for_regime(env, ARMMMUIdx_E2, pageaddr);
 
-    tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr,
-                                                  ARMMMUIdxBit_E2, bits);
+    tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr, mask, bits);
 }
 
 static void tlbi_aa64_vae3is_write(CPUARMState *env, const ARMCPRegInfo *ri,
@@ -5014,11 +5029,6 @@ static void tlbi_aa64_rvae1is_write(CPUARMState *env,
     do_rvae_write(env, value, vae1_tlbmask(env), true);
 }
 
-static int vae2_tlbmask(CPUARMState *env)
-{
-    return ARMMMUIdxBit_E2;
-}
-
 static void tlbi_aa64_rvae2_write(CPUARMState *env,
                                   const ARMCPRegInfo *ri,
                                   uint64_t value)
-- 
2.41.0

Re: [PATCH 2/5] target/arm/helper: Fix vae2_tlbmask()

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> When HCR_EL2.E2H is enabled, TLB entries are formed using the EL2&0
> translation regime, instead of the EL2 translation regime. The TLB VAE2*
> instructions invalidate the regime that corresponds to the current value
> of HCR_EL2.E2H.
>
> At the moment we only invalidate the EL2 translation regime. This causes
> problems with RMM, which issues TLBI VAE2IS instructions with
> HCR_EL2.E2H enabled. Update vae2_tlbmask() to take HCR_EL2.E2H into
> account.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  target/arm/helper.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index e1b3db6f5f..07a9ac70f5 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -4663,6 +4663,21 @@ static int vae1_tlbmask(CPUARMState *env)
>      return mask;
>  }
>
> +static int vae2_tlbmask(CPUARMState *env)
> +{
> +    uint64_t hcr = arm_hcr_el2_eff(env);
> +    uint16_t mask;
> +
> +    if (hcr & HCR_E2H) {
> +        mask = ARMMMUIdxBit_E20_2 |
> +               ARMMMUIdxBit_E20_2_PAN |
> +               ARMMMUIdxBit_E20_0;
> +    } else {
> +        mask = ARMMMUIdxBit_E2;
> +    }
> +    return mask;
> +}
> +
>  /* Return 56 if TBI is enabled, 64 otherwise. */
>  static int tlbbits_for_regime(CPUARMState *env, ARMMMUIdx mmu_idx,
>                                uint64_t addr)

> @@ -4838,11 +4853,11 @@ static void tlbi_aa64_vae2is_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                                     uint64_t value)
>  {
>      CPUState *cs = env_cpu(env);
> +    int mask = vae2_tlbmask(env);
>      uint64_t pageaddr = sextract64(value << 12, 0, 56);
>      int bits = tlbbits_for_regime(env, ARMMMUIdx_E2, pageaddr);

Shouldn't the argument to tlbbits_for_regime() also change
if we're dealing with the EL2&0 regime rather than EL2 ?

>
> -    tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr,
> -                                                  ARMMMUIdxBit_E2, bits);
> +    tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr, mask, bits);
>  }

thanks
-- PMM

Re: [PATCH 2/5] target/arm/helper: Fix vae2_tlbmask()

[Jean-Philippe Brucker]

On Thu, Jul 20, 2023 at 05:35:49PM +0100, Peter Maydell wrote:
> On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > When HCR_EL2.E2H is enabled, TLB entries are formed using the EL2&0
> > translation regime, instead of the EL2 translation regime. The TLB VAE2*
> > instructions invalidate the regime that corresponds to the current value
> > of HCR_EL2.E2H.
> >
> > At the moment we only invalidate the EL2 translation regime. This causes
> > problems with RMM, which issues TLBI VAE2IS instructions with
> > HCR_EL2.E2H enabled. Update vae2_tlbmask() to take HCR_EL2.E2H into
> > account.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> >  target/arm/helper.c | 26 ++++++++++++++++++--------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index e1b3db6f5f..07a9ac70f5 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -4663,6 +4663,21 @@ static int vae1_tlbmask(CPUARMState *env)
> >      return mask;
> >  }
> >
> > +static int vae2_tlbmask(CPUARMState *env)
> > +{
> > +    uint64_t hcr = arm_hcr_el2_eff(env);
> > +    uint16_t mask;
> > +
> > +    if (hcr & HCR_E2H) {
> > +        mask = ARMMMUIdxBit_E20_2 |
> > +               ARMMMUIdxBit_E20_2_PAN |
> > +               ARMMMUIdxBit_E20_0;
> > +    } else {
> > +        mask = ARMMMUIdxBit_E2;
> > +    }
> > +    return mask;
> > +}
> > +
> >  /* Return 56 if TBI is enabled, 64 otherwise. */
> >  static int tlbbits_for_regime(CPUARMState *env, ARMMMUIdx mmu_idx,
> >                                uint64_t addr)
> 
> > @@ -4838,11 +4853,11 @@ static void tlbi_aa64_vae2is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> >                                     uint64_t value)
> >  {
> >      CPUState *cs = env_cpu(env);
> > +    int mask = vae2_tlbmask(env);
> >      uint64_t pageaddr = sextract64(value << 12, 0, 56);
> >      int bits = tlbbits_for_regime(env, ARMMMUIdx_E2, pageaddr);
> 
> Shouldn't the argument to tlbbits_for_regime() also change
> if we're dealing with the EL2&0 regime rather than EL2 ?

Yes, it affects the result since EL2&0 has two ranges

Thanks,
Jean

> 
> >
> > -    tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr,
> > -                                                  ARMMMUIdxBit_E2, bits);
> > +    tlb_flush_page_bits_by_mmuidx_all_cpus_synced(cs, pageaddr, mask, bits);
> >  }
> 
> thanks
> -- PMM

[PATCH 3/5] target/arm: Skip granule protection checks for AT instructions

[Jean-Philippe Brucker]

GPC checks are not performed on the output address for AT instructions,
as stated by ARM DDI 0487J in D8.12.2:

  When populating PAR_EL1 with the result of an address translation
  instruction, granule protection checks are not performed on the final
  output address of a successful translation.

Rename get_phys_addr_with_secure(), since it's only used to handle AT
instructions.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
This incidentally fixes a problem with AT S1E1 instructions which can
output an IPA and should definitely not cause a GPC.
---
 target/arm/internals.h | 25 ++++++++++++++-----------
 target/arm/helper.c    |  8 ++++++--
 target/arm/ptw.c       | 11 ++++++-----
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 0f01bc32a8..fc90c364f7 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult {
 } GetPhysAddrResult;
 
 /**
- * get_phys_addr_with_secure: get the physical address for a virtual address
+ * get_phys_addr: get the physical address for a virtual address
  * @env: CPUARMState
  * @address: virtual address to get physical address for
  * @access_type: 0 for read, 1 for write, 2 for execute
  * @mmu_idx: MMU index indicating required translation regime
- * @is_secure: security state for the access
  * @result: set on translation success.
  * @fi: set to fault info if the translation fails
  *
@@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult {
  *  * for PSMAv5 based systems we don't bother to return a full FSR format
  *    value.
  */
-bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
-                               MMUAccessType access_type,
-                               ARMMMUIdx mmu_idx, bool is_secure,
-                               GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
+bool get_phys_addr(CPUARMState *env, target_ulong address,
+                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
+                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
     __attribute__((nonnull));
 
 /**
- * get_phys_addr: get the physical address for a virtual address
+ * get_phys_addr_with_secure_nogpc: get the physical address for a virtual
+ *                                  address
  * @env: CPUARMState
  * @address: virtual address to get physical address for
  * @access_type: 0 for read, 1 for write, 2 for execute
  * @mmu_idx: MMU index indicating required translation regime
+ * @is_secure: security state for the access
  * @result: set on translation success.
  * @fi: set to fault info if the translation fails
  *
- * Similarly, but use the security regime of @mmu_idx.
+ * Similar to get_phys_addr, but use the given security regime and don't perform
+ * a Granule Protection Check on the resulting address.
  */
-bool get_phys_addr(CPUARMState *env, target_ulong address,
-                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
+bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address,
+                                     MMUAccessType access_type,
+                                     ARMMMUIdx mmu_idx, bool is_secure,
+                                     GetPhysAddrResult *result,
+                                     ARMMMUFaultInfo *fi)
     __attribute__((nonnull));
 
 bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 07a9ac70f5..3ee2bb5fe1 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3365,8 +3365,12 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
     ARMMMUFaultInfo fi = {};
     GetPhysAddrResult res = {};
 
-    ret = get_phys_addr_with_secure(env, value, access_type, mmu_idx,
-                                    is_secure, &res, &fi);
+    /*
+     * I_MXTJT: Granule protection checks are not performed on the final address
+     * of a successful translation.
+     */
+    ret = get_phys_addr_with_secure_nogpc(env, value, access_type, mmu_idx,
+                                          is_secure, &res, &fi);
 
     /*
      * ATS operations only do S1 or S1+S2 translations, so we never
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 6318e13b98..1aef2b8cef 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3412,16 +3412,17 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
     return false;
 }
 
-bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
-                               MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                               bool is_secure, GetPhysAddrResult *result,
-                               ARMMMUFaultInfo *fi)
+bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address,
+                                     MMUAccessType access_type,
+                                     ARMMMUIdx mmu_idx, bool is_secure,
+                                     GetPhysAddrResult *result,
+                                     ARMMMUFaultInfo *fi)
 {
     S1Translate ptw = {
         .in_mmu_idx = mmu_idx,
         .in_space = arm_secure_to_space(is_secure),
     };
-    return get_phys_addr_gpc(env, &ptw, address, access_type, result, fi);
+    return get_phys_addr_nogpc(env, &ptw, address, access_type, result, fi);
 }
 
 bool get_phys_addr(CPUARMState *env, target_ulong address,
-- 
2.41.0

Re: [PATCH 3/5] target/arm: Skip granule protection checks for AT instructions

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> GPC checks are not performed on the output address for AT instructions,
> as stated by ARM DDI 0487J in D8.12.2:
>
>   When populating PAR_EL1 with the result of an address translation
>   instruction, granule protection checks are not performed on the final
>   output address of a successful translation.
>
> Rename get_phys_addr_with_secure(), since it's only used to handle AT
> instructions.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> This incidentally fixes a problem with AT S1E1 instructions which can
> output an IPA and should definitely not cause a GPC.
> ---
>  target/arm/internals.h | 25 ++++++++++++++-----------
>  target/arm/helper.c    |  8 ++++++--
>  target/arm/ptw.c       | 11 ++++++-----
>  3 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 0f01bc32a8..fc90c364f7 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult {
>  } GetPhysAddrResult;
>
>  /**
> - * get_phys_addr_with_secure: get the physical address for a virtual address
> + * get_phys_addr: get the physical address for a virtual address
>   * @env: CPUARMState
>   * @address: virtual address to get physical address for
>   * @access_type: 0 for read, 1 for write, 2 for execute
>   * @mmu_idx: MMU index indicating required translation regime
> - * @is_secure: security state for the access
>   * @result: set on translation success.
>   * @fi: set to fault info if the translation fails
>   *
> @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult {
>   *  * for PSMAv5 based systems we don't bother to return a full FSR format
>   *    value.
>   */
> -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
> -                               MMUAccessType access_type,
> -                               ARMMMUIdx mmu_idx, bool is_secure,
> -                               GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> +bool get_phys_addr(CPUARMState *env, target_ulong address,
> +                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
> +                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
>      __attribute__((nonnull));


What is going on in this bit of the patch ? We already
have a prototype for get_phys_addr() with a doc comment.
Is this git's diff-output producing something silly
for a change that is not logically touching get_phys_addr()'s
prototype and comment at all?

>  /**
> - * get_phys_addr: get the physical address for a virtual address
> + * get_phys_addr_with_secure_nogpc: get the physical address for a virtual
> + *                                  address
>   * @env: CPUARMState
>   * @address: virtual address to get physical address for
>   * @access_type: 0 for read, 1 for write, 2 for execute
>   * @mmu_idx: MMU index indicating required translation regime
> + * @is_secure: security state for the access
>   * @result: set on translation success.
>   * @fi: set to fault info if the translation fails
>   *
> - * Similarly, but use the security regime of @mmu_idx.
> + * Similar to get_phys_addr, but use the given security regime and don't perform
> + * a Granule Protection Check on the resulting address.
>   */
> -bool get_phys_addr(CPUARMState *env, target_ulong address,
> -                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
> -                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address,
> +                                     MMUAccessType access_type,
> +                                     ARMMMUIdx mmu_idx, bool is_secure,
> +                                     GetPhysAddrResult *result,
> +                                     ARMMMUFaultInfo *fi)
>      __attribute__((nonnull));

thanks
-- PMM

Re: [PATCH 3/5] target/arm: Skip granule protection checks for AT instructions

[Peter Maydell]

On Thu, 20 Jul 2023 at 17:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > GPC checks are not performed on the output address for AT instructions,
> > as stated by ARM DDI 0487J in D8.12.2:
> >
> >   When populating PAR_EL1 with the result of an address translation
> >   instruction, granule protection checks are not performed on the final
> >   output address of a successful translation.
> >
> > Rename get_phys_addr_with_secure(), since it's only used to handle AT
> > instructions.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > This incidentally fixes a problem with AT S1E1 instructions which can
> > output an IPA and should definitely not cause a GPC.
> > ---
> >  target/arm/internals.h | 25 ++++++++++++++-----------
> >  target/arm/helper.c    |  8 ++++++--
> >  target/arm/ptw.c       | 11 ++++++-----
> >  3 files changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > index 0f01bc32a8..fc90c364f7 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult {
> >  } GetPhysAddrResult;
> >
> >  /**
> > - * get_phys_addr_with_secure: get the physical address for a virtual address
> > + * get_phys_addr: get the physical address for a virtual address
> >   * @env: CPUARMState
> >   * @address: virtual address to get physical address for
> >   * @access_type: 0 for read, 1 for write, 2 for execute
> >   * @mmu_idx: MMU index indicating required translation regime
> > - * @is_secure: security state for the access
> >   * @result: set on translation success.
> >   * @fi: set to fault info if the translation fails
> >   *
> > @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult {
> >   *  * for PSMAv5 based systems we don't bother to return a full FSR format
> >   *    value.
> >   */
> > -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
> > -                               MMUAccessType access_type,
> > -                               ARMMMUIdx mmu_idx, bool is_secure,
> > -                               GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> > +bool get_phys_addr(CPUARMState *env, target_ulong address,
> > +                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
> > +                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> >      __attribute__((nonnull));
>
>
> What is going on in this bit of the patch ? We already
> have a prototype for get_phys_addr() with a doc comment.
> Is this git's diff-output producing something silly
> for a change that is not logically touching get_phys_addr()'s
> prototype and comment at all?

Looking more closely, that does seem to be what's happened, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/5] target/arm: Skip granule protection checks for AT instructions

[Jean-Philippe Brucker]

On Thu, Jul 20, 2023 at 05:39:56PM +0100, Peter Maydell wrote:
> On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > GPC checks are not performed on the output address for AT instructions,
> > as stated by ARM DDI 0487J in D8.12.2:
> >
> >   When populating PAR_EL1 with the result of an address translation
> >   instruction, granule protection checks are not performed on the final
> >   output address of a successful translation.
> >
> > Rename get_phys_addr_with_secure(), since it's only used to handle AT
> > instructions.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > ---
> > This incidentally fixes a problem with AT S1E1 instructions which can
> > output an IPA and should definitely not cause a GPC.
> > ---
> >  target/arm/internals.h | 25 ++++++++++++++-----------
> >  target/arm/helper.c    |  8 ++++++--
> >  target/arm/ptw.c       | 11 ++++++-----
> >  3 files changed, 26 insertions(+), 18 deletions(-)
> >
> > diff --git a/target/arm/internals.h b/target/arm/internals.h
> > index 0f01bc32a8..fc90c364f7 100644
> > --- a/target/arm/internals.h
> > +++ b/target/arm/internals.h
> > @@ -1190,12 +1190,11 @@ typedef struct GetPhysAddrResult {
> >  } GetPhysAddrResult;
> >
> >  /**
> > - * get_phys_addr_with_secure: get the physical address for a virtual address
> > + * get_phys_addr: get the physical address for a virtual address
> >   * @env: CPUARMState
> >   * @address: virtual address to get physical address for
> >   * @access_type: 0 for read, 1 for write, 2 for execute
> >   * @mmu_idx: MMU index indicating required translation regime
> > - * @is_secure: security state for the access
> >   * @result: set on translation success.
> >   * @fi: set to fault info if the translation fails
> >   *
> > @@ -1212,26 +1211,30 @@ typedef struct GetPhysAddrResult {
> >   *  * for PSMAv5 based systems we don't bother to return a full FSR format
> >   *    value.
> >   */
> > -bool get_phys_addr_with_secure(CPUARMState *env, target_ulong address,
> > -                               MMUAccessType access_type,
> > -                               ARMMMUIdx mmu_idx, bool is_secure,
> > -                               GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> > +bool get_phys_addr(CPUARMState *env, target_ulong address,
> > +                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
> > +                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> >      __attribute__((nonnull));
> 
> 
> What is going on in this bit of the patch ? We already
> have a prototype for get_phys_addr() with a doc comment.
> Is this git's diff-output producing something silly
> for a change that is not logically touching get_phys_addr()'s
> prototype and comment at all?

I swapped the two prototypes in order to make the new comment for
get_phys_addr_with_secure_nogpc() more clear. Tried to convey that
get_phys_addr() is the normal function and
get_phys_addr_with_secure_nogpc() is special. So git is working as
expected and this is a style change, I can switch them back if you prefer.

Thanks,
Jean

> 
> >  /**
> > - * get_phys_addr: get the physical address for a virtual address
> > + * get_phys_addr_with_secure_nogpc: get the physical address for a virtual
> > + *                                  address
> >   * @env: CPUARMState
> >   * @address: virtual address to get physical address for
> >   * @access_type: 0 for read, 1 for write, 2 for execute
> >   * @mmu_idx: MMU index indicating required translation regime
> > + * @is_secure: security state for the access
> >   * @result: set on translation success.
> >   * @fi: set to fault info if the translation fails
> >   *
> > - * Similarly, but use the security regime of @mmu_idx.
> > + * Similar to get_phys_addr, but use the given security regime and don't perform
> > + * a Granule Protection Check on the resulting address.
> >   */
> > -bool get_phys_addr(CPUARMState *env, target_ulong address,
> > -                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
> > -                   GetPhysAddrResult *result, ARMMMUFaultInfo *fi)
> > +bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address,
> > +                                     MMUAccessType access_type,
> > +                                     ARMMMUIdx mmu_idx, bool is_secure,
> > +                                     GetPhysAddrResult *result,
> > +                                     ARMMMUFaultInfo *fi)
> >      __attribute__((nonnull));
> 
> thanks
> -- PMM

[PATCH 4/5] target/arm: Pass security space rather than flag for AT instructions

[Jean-Philippe Brucker]

At the moment we only handle Secure and Nonsecure security spaces for
the AT instructions. Add support for Realm and Root.

For AArch64, arm_security_space() gives the desired space. ARM DDI0487J
says (R_NYXTL):

  If EL3 is implemented, then when an address translation instruction
  that applies to an Exception level lower than EL3 is executed, the
  Effective value of SCR_EL3.{NSE, NS} determines the target Security
  state that the instruction applies to.

For AArch32, some instructions can access NonSecure space from Secure,
so we still need to pass the state explicitly to do_ats_write().

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
I haven't tested AT instructions in Realm/Root space yet, but it looks
like the patch is needed. RMM doesn't issue AT instructions like KVM
does in non-secure state (which triggered the bug in the previous
patch).
---
 target/arm/internals.h | 18 +++++++++---------
 target/arm/helper.c    | 27 ++++++++++++---------------
 target/arm/ptw.c       | 12 ++++++------
 3 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index fc90c364f7..cf13bb94f5 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1217,24 +1217,24 @@ bool get_phys_addr(CPUARMState *env, target_ulong address,
     __attribute__((nonnull));
 
 /**
- * get_phys_addr_with_secure_nogpc: get the physical address for a virtual
- *                                  address
+ * get_phys_addr_with_space_nogpc: get the physical address for a virtual
+ *                                 address
  * @env: CPUARMState
  * @address: virtual address to get physical address for
  * @access_type: 0 for read, 1 for write, 2 for execute
  * @mmu_idx: MMU index indicating required translation regime
- * @is_secure: security state for the access
+ * @space: security space for the access
  * @result: set on translation success.
  * @fi: set to fault info if the translation fails
  *
- * Similar to get_phys_addr, but use the given security regime and don't perform
+ * Similar to get_phys_addr, but use the given security space and don't perform
  * a Granule Protection Check on the resulting address.
  */
-bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address,
-                                     MMUAccessType access_type,
-                                     ARMMMUIdx mmu_idx, bool is_secure,
-                                     GetPhysAddrResult *result,
-                                     ARMMMUFaultInfo *fi)
+bool get_phys_addr_with_space_nogpc(CPUARMState *env, target_ulong address,
+                                    MMUAccessType access_type,
+                                    ARMMMUIdx mmu_idx, ARMSecuritySpace space,
+                                    GetPhysAddrResult *result,
+                                    ARMMMUFaultInfo *fi)
     __attribute__((nonnull));
 
 bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3ee2bb5fe1..2017b11795 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -3357,7 +3357,7 @@ static int par_el1_shareability(GetPhysAddrResult *res)
 
 static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
                              MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                             bool is_secure)
+                             ARMSecuritySpace ss)
 {
     bool ret;
     uint64_t par64;
@@ -3369,8 +3369,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value,
      * I_MXTJT: Granule protection checks are not performed on the final address
      * of a successful translation.
      */
-    ret = get_phys_addr_with_secure_nogpc(env, value, access_type, mmu_idx,
-                                          is_secure, &res, &fi);
+    ret = get_phys_addr_with_space_nogpc(env, value, access_type, mmu_idx, ss,
+                                         &res, &fi);
 
     /*
      * ATS operations only do S1 or S1+S2 translations, so we never
@@ -3535,7 +3535,7 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
     uint64_t par64;
     ARMMMUIdx mmu_idx;
     int el = arm_current_el(env);
-    bool secure = arm_is_secure_below_el3(env);
+    ARMSecuritySpace ss = arm_security_space(env);
 
     switch (ri->opc2 & 6) {
     case 0:
@@ -3543,10 +3543,9 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
         switch (el) {
         case 3:
             mmu_idx = ARMMMUIdx_E3;
-            secure = true;
             break;
         case 2:
-            g_assert(!secure);  /* ARMv8.4-SecEL2 is 64-bit only */
+            g_assert(ss != ARMSS_Secure);  /* ARMv8.4-SecEL2 is 64-bit only */
             /* fall through */
         case 1:
             if (ri->crm == 9 && (env->uncached_cpsr & CPSR_PAN)) {
@@ -3564,10 +3563,9 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
         switch (el) {
         case 3:
             mmu_idx = ARMMMUIdx_E10_0;
-            secure = true;
             break;
         case 2:
-            g_assert(!secure);  /* ARMv8.4-SecEL2 is 64-bit only */
+            g_assert(ss != ARMSS_Secure);  /* ARMv8.4-SecEL2 is 64-bit only */
             mmu_idx = ARMMMUIdx_Stage1_E0;
             break;
         case 1:
@@ -3580,18 +3578,18 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
     case 4:
         /* stage 1+2 NonSecure PL1: ATS12NSOPR, ATS12NSOPW */
         mmu_idx = ARMMMUIdx_E10_1;
-        secure = false;
+        ss = ARMSS_NonSecure;
         break;
     case 6:
         /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */
         mmu_idx = ARMMMUIdx_E10_0;
-        secure = false;
+        ss = ARMSS_NonSecure;
         break;
     default:
         g_assert_not_reached();
     }
 
-    par64 = do_ats_write(env, value, access_type, mmu_idx, secure);
+    par64 = do_ats_write(env, value, access_type, mmu_idx, ss);
 
     A32_BANKED_CURRENT_REG_SET(env, par, par64);
 #else
@@ -3608,7 +3606,8 @@ static void ats1h_write(CPUARMState *env, const ARMCPRegInfo *ri,
     uint64_t par64;
 
     /* There is no SecureEL2 for AArch32. */
-    par64 = do_ats_write(env, value, access_type, ARMMMUIdx_E2, false);
+    par64 = do_ats_write(env, value, access_type, ARMMMUIdx_E2,
+                         ARMSS_NonSecure);
 
     A32_BANKED_CURRENT_REG_SET(env, par, par64);
 #else
@@ -3633,7 +3632,6 @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
 #ifdef CONFIG_TCG
     MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : MMU_DATA_LOAD;
     ARMMMUIdx mmu_idx;
-    int secure = arm_is_secure_below_el3(env);
     uint64_t hcr_el2 = arm_hcr_el2_eff(env);
     bool regime_e20 = (hcr_el2 & (HCR_E2H | HCR_TGE)) == (HCR_E2H | HCR_TGE);
 
@@ -3653,7 +3651,6 @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
             break;
         case 6: /* AT S1E3R, AT S1E3W */
             mmu_idx = ARMMMUIdx_E3;
-            secure = true;
             break;
         default:
             g_assert_not_reached();
@@ -3673,7 +3670,7 @@ static void ats_write64(CPUARMState *env, const ARMCPRegInfo *ri,
     }
 
     env->cp15.par_el[1] = do_ats_write(env, value, access_type,
-                                       mmu_idx, secure);
+                                       mmu_idx, arm_security_space(env));
 #else
     /* Handled by hardware accelerator. */
     g_assert_not_reached();
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 1aef2b8cef..d0270776be 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -3412,15 +3412,15 @@ static bool get_phys_addr_gpc(CPUARMState *env, S1Translate *ptw,
     return false;
 }
 
-bool get_phys_addr_with_secure_nogpc(CPUARMState *env, target_ulong address,
-                                     MMUAccessType access_type,
-                                     ARMMMUIdx mmu_idx, bool is_secure,
-                                     GetPhysAddrResult *result,
-                                     ARMMMUFaultInfo *fi)
+bool get_phys_addr_with_space_nogpc(CPUARMState *env, target_ulong address,
+                                    MMUAccessType access_type,
+                                    ARMMMUIdx mmu_idx, ARMSecuritySpace space,
+                                    GetPhysAddrResult *result,
+                                    ARMMMUFaultInfo *fi)
 {
     S1Translate ptw = {
         .in_mmu_idx = mmu_idx,
-        .in_space = arm_secure_to_space(is_secure),
+        .in_space = space,
     };
     return get_phys_addr_nogpc(env, &ptw, address, access_type, result, fi);
 }
-- 
2.41.0

Re: [PATCH 4/5] target/arm: Pass security space rather than flag for AT instructions

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> At the moment we only handle Secure and Nonsecure security spaces for
> the AT instructions. Add support for Realm and Root.
>
> For AArch64, arm_security_space() gives the desired space. ARM DDI0487J
> says (R_NYXTL):
>
>   If EL3 is implemented, then when an address translation instruction
>   that applies to an Exception level lower than EL3 is executed, the
>   Effective value of SCR_EL3.{NSE, NS} determines the target Security
>   state that the instruction applies to.
>
> For AArch32, some instructions can access NonSecure space from Secure,
> so we still need to pass the state explicitly to do_ats_write().
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
> I haven't tested AT instructions in Realm/Root space yet, but it looks
> like the patch is needed. RMM doesn't issue AT instructions like KVM
> does in non-secure state (which triggered the bug in the previous
> patch).
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

We should also implement the check that the AT S1E[012] ops have
that if FEAT_RME is implemented and SCR_EL3.{NS,NSE} are a reserved
value then the AT insn should UNDEF. But that's a separate patch.

thanks
-- PMM

[PATCH 5/5] target/arm/helper: Implement CNTHCTL_EL2.CNT[VP]MASK

[Jean-Philippe Brucker]

When FEAT_RME is implemented, these bits override the value of
CNT[VP]_CTL_EL0.IMASK in Realm and Root state.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 target/arm/helper.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2017b11795..5b173a827f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2608,6 +2608,23 @@ static uint64_t gt_get_countervalue(CPUARMState *env)
     return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(cpu);
 }
 
+static bool gt_is_masked(CPUARMState *env, int timeridx)
+{
+    ARMSecuritySpace ss = arm_security_space(env);
+
+    /*
+     * If bits CNTHCTL_EL2.CNT[VP]MASK are set, they override
+     * CNT[VP]_CTL_EL0.IMASK. They are RES0 in Secure and NonSecure state.
+     */
+    if ((ss == ARMSS_Root || ss == ARMSS_Realm) &&
+        ((timeridx == GTIMER_VIRT && extract64(env->cp15.cnthctl_el2, 18, 1)) ||
+         (timeridx == GTIMER_PHYS && extract64(env->cp15.cnthctl_el2, 19, 1)))) {
+        return true;
+    }
+
+    return env->cp15.c14_timer[timeridx].ctl & 2;
+}
+
 static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
 {
     ARMGenericTimer *gt = &cpu->env.cp15.c14_timer[timeridx];
@@ -2627,7 +2644,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
 
         gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
 
-        irqstate = (istatus && !(gt->ctl & 2));
+        irqstate = (istatus && !gt_is_masked(&cpu->env, timeridx));
         qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
 
         if (istatus) {
@@ -2759,7 +2776,7 @@ static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
          * IMASK toggled: don't need to recalculate,
          * just set the interrupt line based on ISTATUS
          */
-        int irqstate = (oldval & 4) && !(value & 2);
+        int irqstate = (oldval & 4) && !gt_is_masked(env, timeridx);
 
         trace_arm_gt_imask_toggle(timeridx, irqstate);
         qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
-- 
2.41.0

Re: [PATCH 5/5] target/arm/helper: Implement CNTHCTL_EL2.CNT[VP]MASK

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> When FEAT_RME is implemented, these bits override the value of
> CNT[VP]_CTL_EL0.IMASK in Realm and Root state.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  target/arm/helper.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2017b11795..5b173a827f 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2608,6 +2608,23 @@ static uint64_t gt_get_countervalue(CPUARMState *env)
>      return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / gt_cntfrq_period_ns(cpu);
>  }
>
> +static bool gt_is_masked(CPUARMState *env, int timeridx)
> +{
> +    ARMSecuritySpace ss = arm_security_space(env);
> +
> +    /*
> +     * If bits CNTHCTL_EL2.CNT[VP]MASK are set, they override
> +     * CNT[VP]_CTL_EL0.IMASK. They are RES0 in Secure and NonSecure state.
> +     */
> +    if ((ss == ARMSS_Root || ss == ARMSS_Realm) &&
> +        ((timeridx == GTIMER_VIRT && extract64(env->cp15.cnthctl_el2, 18, 1)) ||
> +         (timeridx == GTIMER_PHYS && extract64(env->cp15.cnthctl_el2, 19, 1)))) {
> +        return true;
> +    }
> +
> +    return env->cp15.c14_timer[timeridx].ctl & 2;
> +}
> +
>  static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>  {
>      ARMGenericTimer *gt = &cpu->env.cp15.c14_timer[timeridx];
> @@ -2627,7 +2644,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
>
>          gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
>
> -        irqstate = (istatus && !(gt->ctl & 2));
> +        irqstate = (istatus && !gt_is_masked(&cpu->env, timeridx));
>          qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);
>
>          if (istatus) {
> @@ -2759,7 +2776,7 @@ static void gt_ctl_write(CPUARMState *env, const ARMCPRegInfo *ri,
>           * IMASK toggled: don't need to recalculate,
>           * just set the interrupt line based on ISTATUS
>           */
> -        int irqstate = (oldval & 4) && !(value & 2);
> +        int irqstate = (oldval & 4) && !gt_is_masked(env, timeridx);
>
>          trace_arm_gt_imask_toggle(timeridx, irqstate);
>          qemu_set_irq(cpu->gt_timer_outputs[timeridx], irqstate);

If these CNTHCTL bits now affect whether the timer interrupts
are masked, then we need to update the timer irq state
on writes to CNTHCTL that change the bits.

thanks
-- PMM

Re: [PATCH 0/5] target/arm: Fixes for RME

[Peter Maydell]

On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> With these patches I'm able to boot a Realm guest under
> "-cpu max,x-rme=on". They are based on Peter's series which fixes
> handling of NSTable:
> https://lore.kernel.org/qemu-devel/20230714154648.327466-1-peter.maydell@linaro.org/

Thanks for testing this -- this is a lot closer to
working out of the box than I thought we might be :-)
I'm tempted to try to put these fixes (and my ptw patchset)
into 8.1, but OTOH I suspect work on Realm guests will probably
still want to use a bleeding-edge QEMU for other bugs we're
going to discover over the next few months, so IDK. We'll
see how code review goes on those, I guess.

> Running a Realm guest requires components at EL3 and R-EL2. Some rough
> support for TF-A and RMM is available here:
> https://jpbrucker.net/git/tf-a/log/?h=qemu-rme
> https://jpbrucker.net/git/rmm/log/?h=qemu-rme
> I'll clean this up before sending it out.
>
> I also need to manually disable FEAT_SME in QEMU in order to boot this,

Do you mean you needed to do something more invasive than
'-cpu max,x-rme=on,sme=off' ?

> otherwise the Linux host fails to boot because hyp-stub accesses to SME
> regs are trapped to EL3, which doesn't support RME+SME at the moment.
> The right fix is probably in TF-A but I haven't investigated yet.

thanks
-- PMM

Re: [PATCH 0/5] target/arm: Fixes for RME

[Jean-Philippe Brucker]

On Thu, Jul 20, 2023 at 01:05:58PM +0100, Peter Maydell wrote:
> On Wed, 19 Jul 2023 at 16:56, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> >
> > With these patches I'm able to boot a Realm guest under
> > "-cpu max,x-rme=on". They are based on Peter's series which fixes
> > handling of NSTable:
> > https://lore.kernel.org/qemu-devel/20230714154648.327466-1-peter.maydell@linaro.org/
> 
> Thanks for testing this -- this is a lot closer to
> working out of the box than I thought we might be :-)
> I'm tempted to try to put these fixes (and my ptw patchset)
> into 8.1, but OTOH I suspect work on Realm guests will probably
> still want to use a bleeding-edge QEMU for other bugs we're
> going to discover over the next few months, so IDK. We'll
> see how code review goes on those, I guess.
> 
> > Running a Realm guest requires components at EL3 and R-EL2. Some rough
> > support for TF-A and RMM is available here:
> > https://jpbrucker.net/git/tf-a/log/?h=qemu-rme
> > https://jpbrucker.net/git/rmm/log/?h=qemu-rme
> > I'll clean this up before sending it out.
> >
> > I also need to manually disable FEAT_SME in QEMU in order to boot this,
> 
> Do you mean you needed to do something more invasive than
> '-cpu max,x-rme=on,sme=off' ?

Ah no, I hadn't noticed there was a sme=off switch, that's much better

Thanks,
Jean

> 
> > otherwise the Linux host fails to boot because hyp-stub accesses to SME
> > regs are trapped to EL3, which doesn't support RME+SME at the moment.
> > The right fix is probably in TF-A but I haven't investigated yet.
> 
> thanks
> -- PMM