[PATCH v2 0/2] target/arm: Reduce aa64_va_parameter overhead

[PATCH v2 0/2] target/arm: Reduce aa64_va_parameter overhead

[Richard Henderson]

Something I noticed while developing and testing VHE.
For v2, fix select as a separate patch.


r~


Richard Henderson (2):
  target/arm: Fix select for aa64_va_parameters_both
  target/arm: Split out aa64_va_parameter_tbi, aa64_va_parameter_tbid

 target/arm/internals.h |   3 -
 target/arm/helper.c    | 138 ++++++++++++++++++++++-------------------
 2 files changed, 73 insertions(+), 68 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both

[Richard Henderson]

Select should always be 0 for a regime with one range.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 46 +++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7d15d5c933..a008c70c87 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10074,13 +10074,8 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
     bool tbi, tbid, epd, hpd, using16k, using64k;
     int select, tsz;
 
-    /*
-     * Bit 55 is always between the two regions, and is canonical for
-     * determining if address tagging is enabled.
-     */
-    select = extract64(va, 55, 1);
-
     if (!regime_has_2_ranges(mmu_idx)) {
+        select = 0;
         tsz = extract32(tcr, 0, 6);
         using64k = extract32(tcr, 14, 1);
         using16k = extract32(tcr, 15, 1);
@@ -10093,23 +10088,30 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
             tbid = extract32(tcr, 29, 1);
         }
         epd = false;
-    } else if (!select) {
-        tsz = extract32(tcr, 0, 6);
-        epd = extract32(tcr, 7, 1);
-        using64k = extract32(tcr, 14, 1);
-        using16k = extract32(tcr, 15, 1);
-        tbi = extract64(tcr, 37, 1);
-        hpd = extract64(tcr, 41, 1);
-        tbid = extract64(tcr, 51, 1);
     } else {
-        int tg = extract32(tcr, 30, 2);
-        using16k = tg == 1;
-        using64k = tg == 3;
-        tsz = extract32(tcr, 16, 6);
-        epd = extract32(tcr, 23, 1);
-        tbi = extract64(tcr, 38, 1);
-        hpd = extract64(tcr, 42, 1);
-        tbid = extract64(tcr, 52, 1);
+        /*
+         * Bit 55 is always between the two regions, and is canonical for
+         * determining if address tagging is enabled.
+         */
+        select = extract64(va, 55, 1);
+        if (!select) {
+            tsz = extract32(tcr, 0, 6);
+            epd = extract32(tcr, 7, 1);
+            using64k = extract32(tcr, 14, 1);
+            using16k = extract32(tcr, 15, 1);
+            tbi = extract64(tcr, 37, 1);
+            hpd = extract64(tcr, 41, 1);
+            tbid = extract64(tcr, 51, 1);
+        } else {
+            int tg = extract32(tcr, 30, 2);
+            using16k = tg == 1;
+            using64k = tg == 3;
+            tsz = extract32(tcr, 16, 6);
+            epd = extract32(tcr, 23, 1);
+            tbi = extract64(tcr, 38, 1);
+            hpd = extract64(tcr, 42, 1);
+            tbid = extract64(tcr, 52, 1);
+        }
     }
     tsz = MIN(tsz, 39);  /* TODO: ARMv8.4-TTST */
     tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
-- 
2.20.1

[PATCH v2 2/2] target/arm: Split out aa64_va_parameter_tbi, aa64_va_parameter_tbid

[Richard Henderson]

For the purpose of rebuild_hflags_a64, we do not need to compute
all of the va parameters, only tbi.  Moreover, we can compute them
in a form that is more useful to storing in hflags.

This eliminates the need for aa64_va_parameter_both, so fold that
in to aa64_va_parameter.  The remaining calls to aa64_va_parameter
are in get_phys_addr_lpae and in pauth_helper.c.

This reduces the total cpu consumption of aa64_va_parameter in a
kernel boot plus a kvm guest kernel boot from 3% to 0.5%.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h |  3 --
 target/arm/helper.c    | 68 +++++++++++++++++++++++-------------------
 2 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 6d4a942bde..6ac84bbca7 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1042,15 +1042,12 @@ typedef struct ARMVAParameters {
     unsigned tsz    : 8;
     unsigned select : 1;
     bool tbi        : 1;
-    bool tbid       : 1;
     bool epd        : 1;
     bool hpd        : 1;
     bool using16k   : 1;
     bool using64k   : 1;
 } ARMVAParameters;
 
-ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
-                                        ARMMMUIdx mmu_idx);
 ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
                                    ARMMMUIdx mmu_idx, bool data);
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index a008c70c87..9ebd7e9459 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10067,12 +10067,34 @@ static uint8_t convert_stage2_attrs(CPUARMState *env, uint8_t s2attrs)
 }
 #endif /* !CONFIG_USER_ONLY */
 
-ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
-                                        ARMMMUIdx mmu_idx)
+static int aa64_va_parameter_tbi(uint64_t tcr, ARMMMUIdx mmu_idx)
+{
+    if (regime_has_2_ranges(mmu_idx)) {
+        return extract64(tcr, 37, 2);
+    } else if (mmu_idx == ARMMMUIdx_Stage2) {
+        return 0; /* VTCR_EL2 */
+    } else {
+        return extract32(tcr, 20, 1);
+    }
+}
+
+static int aa64_va_parameter_tbid(uint64_t tcr, ARMMMUIdx mmu_idx)
+{
+    if (regime_has_2_ranges(mmu_idx)) {
+        return extract64(tcr, 51, 2);
+    } else if (mmu_idx == ARMMMUIdx_Stage2) {
+        return 0; /* VTCR_EL2 */
+    } else {
+        return extract32(tcr, 29, 1);
+    }
+}
+
+ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
+                                   ARMMMUIdx mmu_idx, bool data)
 {
     uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
-    bool tbi, tbid, epd, hpd, using16k, using64k;
-    int select, tsz;
+    bool epd, hpd, using16k, using64k;
+    int select, tsz, tbi;
 
     if (!regime_has_2_ranges(mmu_idx)) {
         select = 0;
@@ -10081,11 +10103,9 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
         using16k = extract32(tcr, 15, 1);
         if (mmu_idx == ARMMMUIdx_Stage2) {
             /* VTCR_EL2 */
-            tbi = tbid = hpd = false;
+            hpd = false;
         } else {
-            tbi = extract32(tcr, 20, 1);
             hpd = extract32(tcr, 24, 1);
-            tbid = extract32(tcr, 29, 1);
         }
         epd = false;
     } else {
@@ -10099,28 +10119,30 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
             epd = extract32(tcr, 7, 1);
             using64k = extract32(tcr, 14, 1);
             using16k = extract32(tcr, 15, 1);
-            tbi = extract64(tcr, 37, 1);
             hpd = extract64(tcr, 41, 1);
-            tbid = extract64(tcr, 51, 1);
         } else {
             int tg = extract32(tcr, 30, 2);
             using16k = tg == 1;
             using64k = tg == 3;
             tsz = extract32(tcr, 16, 6);
             epd = extract32(tcr, 23, 1);
-            tbi = extract64(tcr, 38, 1);
             hpd = extract64(tcr, 42, 1);
-            tbid = extract64(tcr, 52, 1);
         }
     }
     tsz = MIN(tsz, 39);  /* TODO: ARMv8.4-TTST */
     tsz = MAX(tsz, 16);  /* TODO: ARMv8.2-LVA  */
 
+    /* Present TBI as a composite with TBID.  */
+    tbi = aa64_va_parameter_tbi(tcr, mmu_idx);
+    if (!data) {
+        tbi &= ~aa64_va_parameter_tbid(tcr, mmu_idx);
+    }
+    tbi = (tbi >> select) & 1;
+
     return (ARMVAParameters) {
         .tsz = tsz,
         .select = select,
         .tbi = tbi,
-        .tbid = tbid,
         .epd = epd,
         .hpd = hpd,
         .using16k = using16k,
@@ -10128,16 +10150,6 @@ ARMVAParameters aa64_va_parameters_both(CPUARMState *env, uint64_t va,
     };
 }
 
-ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
-                                   ARMMMUIdx mmu_idx, bool data)
-{
-    ARMVAParameters ret = aa64_va_parameters_both(env, va, mmu_idx);
-
-    /* Present TBI as a composite with TBID.  */
-    ret.tbi &= (data || !ret.tbid);
-    return ret;
-}
-
 #ifndef CONFIG_USER_ONLY
 static ARMVAParameters aa32_va_parameters(CPUARMState *env, uint32_t va,
                                           ARMMMUIdx mmu_idx)
@@ -11957,21 +11969,15 @@ static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
 {
     uint32_t flags = rebuild_hflags_aprofile(env);
     ARMMMUIdx stage1 = stage_1_mmu_idx(mmu_idx);
-    ARMVAParameters p0 = aa64_va_parameters_both(env, 0, stage1);
+    uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr;
     uint64_t sctlr;
     int tbii, tbid;
 
     flags = FIELD_DP32(flags, TBFLAG_ANY, AARCH64_STATE, 1);
 
     /* Get control bits for tagged addresses.  */
-    if (regime_has_2_ranges(mmu_idx)) {
-        ARMVAParameters p1 = aa64_va_parameters_both(env, -1, stage1);
-        tbid = (p1.tbi << 1) | p0.tbi;
-        tbii = tbid & ~((p1.tbid << 1) | p0.tbid);
-    } else {
-        tbid = p0.tbi;
-        tbii = tbid & !p0.tbid;
-    }
+    tbid = aa64_va_parameter_tbi(tcr, mmu_idx);
+    tbii = tbid & ~aa64_va_parameter_tbid(tcr, mmu_idx);
 
     flags = FIELD_DP32(flags, TBFLAG_A64, TBII, tbii);
     flags = FIELD_DP32(flags, TBFLAG_A64, TBID, tbid);
-- 
2.20.1

Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both

[Peter Maydell]

On Tue, 11 Feb 2020 at 19:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Select should always be 0 for a regime with one range.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

This change makes sense, and matches what aa32_va_parameters() does,
but I think we need to update some of the callsites.

(1) In get_phys_addr_lpae() we have the check:

        if (-top_bits != param.select || (param.select && !ttbr1_valid)) {

where ttbr1_valid is the return value of (effectively)
 aarch64 ? regime_has_2_ranges() : (el != 2);
but I think it's no longer possible to get here with param.select == 1
and !ttbr1_valid, so this becomes a dead check.
(Side note, could we pull "ttbr1_valid = regime_has_2_ranges(mmu_idx);"
out of the "if (aarch64) {...} else {...}"  ? -- I think it works for
aarch32 too, right?)

(2) in pauth_original_ptr() we do
    uint64_t extfield = -param.select;
but in the pseudocode Auth() function the extfield is unconditionally
calculated based on bit 55 of the address, regardless of whether
the regime has 1 range or 2. So I think this code can't use
param.select any more but should simply pull out and replicate
bit 55 of its 'ptr' argument, now that param.select is not simply
the value of bit 55.


Change 1 would need to be done after this patch and change 2 before it.

thanks
-- PMM

Re: [PATCH v2 2/2] target/arm: Split out aa64_va_parameter_tbi, aa64_va_parameter_tbid

[Peter Maydell]

On Tue, 11 Feb 2020 at 19:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> For the purpose of rebuild_hflags_a64, we do not need to compute
> all of the va parameters, only tbi.  Moreover, we can compute them
> in a form that is more useful to storing in hflags.
>
> This eliminates the need for aa64_va_parameter_both, so fold that
> in to aa64_va_parameter.  The remaining calls to aa64_va_parameter
> are in get_phys_addr_lpae and in pauth_helper.c.
>
> This reduces the total cpu consumption of aa64_va_parameter in a
> kernel boot plus a kvm guest kernel boot from 3% to 0.5%.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

thanks
-- PMM

Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both

[Peter Maydell]

On Thu, 13 Feb 2020 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 11 Feb 2020 at 19:42, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Select should always be 0 for a regime with one range.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> This change makes sense, and matches what aa32_va_parameters() does,
> but I think we need to update some of the callsites.

Assuming those changes are done in separate patches, for
this patch itself:

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

thanks
-- PMM

Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both

[Peter Maydell]

On Thu, 13 Feb 2020 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 11 Feb 2020 at 19:42, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > Select should always be 0 for a regime with one range.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> This change makes sense, and matches what aa32_va_parameters() does,
> but I think we need to update some of the callsites.
>
> (1) In get_phys_addr_lpae() we have the check:
>
>         if (-top_bits != param.select || (param.select && !ttbr1_valid)) {
>
> where ttbr1_valid is the return value of (effectively)
>  aarch64 ? regime_has_2_ranges() : (el != 2);
> but I think it's no longer possible to get here with param.select == 1
> and !ttbr1_valid, so this becomes a dead check.

...or should the code instead be checking literal pointer bit 55
against ttbr1_valid now ?

thanks
-- PMM

Re: [PATCH v2 1/2] target/arm: Fix select for aa64_va_parameters_both

[Richard Henderson]

On 2/13/20 5:25 AM, Peter Maydell wrote:
> On Thu, 13 Feb 2020 at 13:12, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Tue, 11 Feb 2020 at 19:42, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> Select should always be 0 for a regime with one range.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> This change makes sense, and matches what aa32_va_parameters() does,
>> but I think we need to update some of the callsites.
>>
>> (1) In get_phys_addr_lpae() we have the check:
>>
>>         if (-top_bits != param.select || (param.select && !ttbr1_valid)) {
>>
>> where ttbr1_valid is the return value of (effectively)
>>  aarch64 ? regime_has_2_ranges() : (el != 2);
>> but I think it's no longer possible to get here with param.select == 1
>> and !ttbr1_valid, so this becomes a dead check.
> 
> ...or should the code instead be checking literal pointer bit 55
> against ttbr1_valid now ?

No, I think the first expression now covers everything, as you suggested in the
first reply.


r~