[PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Richard Henderson]

As reported by Andrey, I was missing the complete ISS info for
the Data Abort raised upon a synchronous tag check fail.

The following should fix that.  All the twisty little rules for
the ISS.ISV bit are already handled by merge_syn_data_abort.
Probably the most important bit that was missing was ISS.WnR,
as that is independent of ISS.ISV.

Andrey, will you please test?


r~


Richard Henderson (3):
  target/arm: Export merge_syn_data_abort from tlb_helper.c
  target/arm: Pass the entire mte descriptor to mte_check_fail
  target/arm: Merge ISS for data abort from tag check fail

 target/arm/internals.h  |  4 ++++
 target/arm/mte_helper.c | 24 ++++++++++++++----------
 target/arm/tlb_helper.c |  8 +++-----
 3 files changed, 21 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH 1/3] target/arm: Export merge_syn_data_abort from tlb_helper.c

[Richard Henderson]

We will shortly need this function in mte_helper.c.
It's also a bit too large for inlining, for something
that is on an exceptional path.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h  | 4 ++++
 target/arm/tlb_helper.c | 8 +++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index ae99725d2b..967d1319de 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -500,6 +500,10 @@ static inline uint32_t syn_wfx(int cv, int cond, int ti, bool is_16bit)
            (cv << 24) | (cond << 20) | ti;
 }
 
+uint32_t merge_syn_data_abort(uint32_t template_syn, unsigned int target_el,
+                              bool same_el, bool ea, bool s1ptw,
+                              bool is_write, int fsc);
+
 /* Update a QEMU watchpoint based on the information the guest has set in the
  * DBGWCR<n>_EL1 and DBGWVR<n>_EL1 registers.
  */
diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
index b35dc8a011..e8cf39f7d9 100644
--- a/target/arm/tlb_helper.c
+++ b/target/arm/tlb_helper.c
@@ -10,11 +10,9 @@
 #include "internals.h"
 #include "exec/exec-all.h"
 
-static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
-                                            unsigned int target_el,
-                                            bool same_el, bool ea,
-                                            bool s1ptw, bool is_write,
-                                            int fsc)
+uint32_t merge_syn_data_abort(uint32_t template_syn, unsigned int target_el,
+                              bool same_el, bool ea, bool s1ptw,
+                              bool is_write, int fsc)
 {
     uint32_t syn;
 
-- 
2.25.1

[PATCH 2/3] target/arm: Pass the entire mte descriptor to mte_check_fail

[Richard Henderson]

We need more information than just the mmu_idx in order
to create the proper exception syndrome.  Only change the
function signature so far.

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

diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index 104752041f..a40454588d 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -514,9 +514,10 @@ void HELPER(stzgm_tags)(CPUARMState *env, uint64_t ptr, uint64_t val)
 }
 
 /* Record a tag check failure.  */
-static void mte_check_fail(CPUARMState *env, int mmu_idx,
+static void mte_check_fail(CPUARMState *env, uint32_t desc,
                            uint64_t dirty_ptr, uintptr_t ra)
 {
+    int mmu_idx = FIELD_EX32(desc, MTEDESC, MIDX);
     ARMMMUIdx arm_mmu_idx = core_to_aa64_mmu_idx(mmu_idx);
     int el, reg_el, tcf, select;
     uint64_t sctlr;
@@ -639,8 +640,7 @@ uint64_t mte_check1(CPUARMState *env, uint32_t desc,
     }
 
     if (unlikely(!mte_probe1_int(env, desc, ptr, ra, bit55))) {
-        int mmu_idx = FIELD_EX32(desc, MTEDESC, MIDX);
-        mte_check_fail(env, mmu_idx, ptr, ra);
+        mte_check_fail(env, desc, ptr, ra);
     }
 
     return useronly_clean_ptr(ptr);
@@ -810,7 +810,7 @@ uint64_t mte_checkN(CPUARMState *env, uint32_t desc,
 
         fail_ofs = tag_first + n * TAG_GRANULE - ptr;
         fail_ofs = ROUND_UP(fail_ofs, esize);
-        mte_check_fail(env, mmu_idx, ptr + fail_ofs, ra);
+        mte_check_fail(env, desc, ptr + fail_ofs, ra);
     }
 
  done:
@@ -922,7 +922,7 @@ uint64_t HELPER(mte_check_zva)(CPUARMState *env, uint32_t desc, uint64_t ptr)
  fail:
     /* Locate the first nibble that differs. */
     i = ctz64(mem_tag ^ ptr_tag) >> 4;
-    mte_check_fail(env, mmu_idx, align_ptr + i * TAG_GRANULE, ra);
+    mte_check_fail(env, desc, align_ptr + i * TAG_GRANULE, ra);
 
  done:
     return useronly_clean_ptr(ptr);
-- 
2.25.1

[PATCH 3/3] target/arm: Merge ISS for data abort from tag check fail

[Richard Henderson]

Merge the ISS data saved by the translator for the originating insn.
Use merge_syn_data_abort so that we properly take the target EL into
account.  Set the WnR bit properly in all cases.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/mte_helper.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index a40454588d..2dff4c548d 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -519,7 +519,7 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
 {
     int mmu_idx = FIELD_EX32(desc, MTEDESC, MIDX);
     ARMMMUIdx arm_mmu_idx = core_to_aa64_mmu_idx(mmu_idx);
-    int el, reg_el, tcf, select;
+    int el, target_el, reg_el, tcf, select, is_write, syn;
     uint64_t sctlr;
 
     reg_el = regime_el(env, arm_mmu_idx);
@@ -543,13 +543,17 @@ static void mte_check_fail(CPUARMState *env, uint32_t desc,
          *
          * In restore_state_to_opc, we set the exception syndrome
          * for the load or store operation.  Unwind first so we
-         * may overwrite that with the syndrome for the tag check.
+         * may merge that with the syndrome for the tag check.
          */
         cpu_restore_state(env_cpu(env), ra, true);
         env->exception.vaddress = dirty_ptr;
-        raise_exception(env, EXCP_DATA_ABORT,
-                        syn_data_abort_no_iss(el != 0, 0, 0, 0, 0, 0, 0x11),
-                        exception_target_el(env));
+
+        target_el = exception_target_el(env);
+        is_write = FIELD_EX32(desc, MTEDESC, WRITE);
+        syn = merge_syn_data_abort(env->exception.syndrome, target_el,
+                                   target_el == el, 0, 0, is_write, 0x11);
+
+        raise_exception(env, EXCP_DATA_ABORT, syn, target_el);
         /* noreturn, but fall through to the assert anyway */
 
     case 0:
-- 
2.25.1

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Andrey Konovalov]

On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> As reported by Andrey, I was missing the complete ISS info for
> the Data Abort raised upon a synchronous tag check fail.
>
> The following should fix that.  All the twisty little rules for
> the ISS.ISV bit are already handled by merge_syn_data_abort.
> Probably the most important bit that was missing was ISS.WnR,
> as that is independent of ISS.ISV.
>
> Andrey, will you please test?

Looks like WnR is now being set properly, but SAS is still always 0.


>
>
> r~
>
>
> Richard Henderson (3):
>   target/arm: Export merge_syn_data_abort from tlb_helper.c
>   target/arm: Pass the entire mte descriptor to mte_check_fail
>   target/arm: Merge ISS for data abort from tag check fail
>
>  target/arm/internals.h  |  4 ++++
>  target/arm/mte_helper.c | 24 ++++++++++++++----------
>  target/arm/tlb_helper.c |  8 +++-----
>  3 files changed, 21 insertions(+), 15 deletions(-)
>
> --
> 2.25.1
>

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Richard Henderson]

On 8/12/20 10:38 AM, Andrey Konovalov wrote:
> On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> As reported by Andrey, I was missing the complete ISS info for
>> the Data Abort raised upon a synchronous tag check fail.
>>
>> The following should fix that.  All the twisty little rules for
>> the ISS.ISV bit are already handled by merge_syn_data_abort.
>> Probably the most important bit that was missing was ISS.WnR,
>> as that is independent of ISS.ISV.
>>
>> Andrey, will you please test?
> 
> Looks like WnR is now being set properly, but SAS is still always 0.

Are you looking at ESR_EL1?

On page D13-2992 of revision F.a:

# ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.

which means that ISS[23:14] are RES0, which includes SAS.


r~

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Richard Henderson]

On 8/12/20 10:52 AM, Richard Henderson wrote:
> On 8/12/20 10:38 AM, Andrey Konovalov wrote:
>> On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> As reported by Andrey, I was missing the complete ISS info for
>>> the Data Abort raised upon a synchronous tag check fail.
>>>
>>> The following should fix that.  All the twisty little rules for
>>> the ISS.ISV bit are already handled by merge_syn_data_abort.
>>> Probably the most important bit that was missing was ISS.WnR,
>>> as that is independent of ISS.ISV.
>>>
>>> Andrey, will you please test?
>>
>> Looks like WnR is now being set properly, but SAS is still always 0.
> 
> Are you looking at ESR_EL1?
> 
> On page D13-2992 of revision F.a:
> 
> # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.
> 
> which means that ISS[23:14] are RES0, which includes SAS.

Actually, note that AArch64.TagCheckFault never fills in anything except WnR.
So the final patch here merging the recorded syndrome information is wrong.  I
was only missing WnR in the original implementation.


r~

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Andrey Konovalov]

On Wed, Aug 12, 2020 at 7:52 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/12/20 10:38 AM, Andrey Konovalov wrote:
> > On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> As reported by Andrey, I was missing the complete ISS info for
> >> the Data Abort raised upon a synchronous tag check fail.
> >>
> >> The following should fix that.  All the twisty little rules for
> >> the ISS.ISV bit are already handled by merge_syn_data_abort.
> >> Probably the most important bit that was missing was ISS.WnR,
> >> as that is independent of ISS.ISV.
> >>
> >> Andrey, will you please test?
> >
> > Looks like WnR is now being set properly, but SAS is still always 0.
>
> Are you looking at ESR_EL1?
>
> On page D13-2992 of revision F.a:
>
> # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.
>
> which means that ISS[23:14] are RES0, which includes SAS.

+more Arm and Google people

Is this known? Do we not get access size when MTE fault happens?

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Evgenii Stepanov]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On Wed, Aug 12, 2020 at 11:03 AM Andrey Konovalov <andreyknvl@google.com>
wrote:

> On Wed, Aug 12, 2020 at 7:52 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 8/12/20 10:38 AM, Andrey Konovalov wrote:
> > > On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
> > > <richard.henderson@linaro.org> wrote:
> > >>
> > >> As reported by Andrey, I was missing the complete ISS info for
> > >> the Data Abort raised upon a synchronous tag check fail.
> > >>
> > >> The following should fix that.  All the twisty little rules for
> > >> the ISS.ISV bit are already handled by merge_syn_data_abort.
> > >> Probably the most important bit that was missing was ISS.WnR,
> > >> as that is independent of ISS.ISV.
> > >>
> > >> Andrey, will you please test?
> > >
> > > Looks like WnR is now being set properly, but SAS is still always 0.
> >
> > Are you looking at ESR_EL1?
> >
> > On page D13-2992 of revision F.a:
> >
> > # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.
> >
> > which means that ISS[23:14] are RES0, which includes SAS.
>
> +more Arm and Google people
>
> Is this known? Do we not get access size when MTE fault happens?
>

It sounds like this applies to all data abort exceptions, no matter MTE or
not.

[-- Attachment #2: Type: text/html, Size: 1966 bytes --]

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Kevin Brodsky]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On 12/08/2020 20:06, Evgenii Stepanov wrote:
> On Wed, Aug 12, 2020 at 11:03 AM Andrey Konovalov <andreyknvl@google.com 
> <mailto:andreyknvl@google.com>> wrote:
>
>     On Wed, Aug 12, 2020 at 7:52 PM Richard Henderson
>     <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> wrote:
>     >
>     > On 8/12/20 10:38 AM, Andrey Konovalov wrote:
>     > > On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
>     > > <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> wrote:
>     > >>
>     > >> As reported by Andrey, I was missing the complete ISS info for
>     > >> the Data Abort raised upon a synchronous tag check fail.
>     > >>
>     > >> The following should fix that.  All the twisty little rules for
>     > >> the ISS.ISV bit are already handled by merge_syn_data_abort.
>     > >> Probably the most important bit that was missing was ISS.WnR,
>     > >> as that is independent of ISS.ISV.
>     > >>
>     > >> Andrey, will you please test?
>     > >
>     > > Looks like WnR is now being set properly, but SAS is still always 0.
>     >
>     > Are you looking at ESR_EL1?
>     >
>     > On page D13-2992 of revision F.a:
>     >
>     > # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.
>     >
>     > which means that ISS[23:14] are RES0, which includes SAS.
>
>     +more Arm and Google people
>
>     Is this known? Do we not get access size when MTE fault happens?
>
>
> It sounds like this applies to all data abort exceptions, no matter MTE or not.

Correct. For data aborts in general, the extra syndrome information in ISS[23:14] is 
only provided at EL2, in order to help hypervisors emulate simple loads/stores (that 
access device memory) by looking at ESR_EL2 without having to decode the trapped 
instruction. Did you have any particular use-case in mind for SAS being set even in 
ESR_EL1?

Kevin

[-- Attachment #2: Type: text/html, Size: 3397 bytes --]

Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail

[Andrey Konovalov]

On Thu, Aug 13, 2020 at 12:01 PM Kevin Brodsky <kevin.brodsky@arm.com> wrote:
>
> On 12/08/2020 20:06, Evgenii Stepanov wrote:
>
> On Wed, Aug 12, 2020 at 11:03 AM Andrey Konovalov <andreyknvl@google.com> wrote:
>>
>> On Wed, Aug 12, 2020 at 7:52 PM Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>> >
>> > On 8/12/20 10:38 AM, Andrey Konovalov wrote:
>> > > On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson
>> > > <richard.henderson@linaro.org> wrote:
>> > >>
>> > >> As reported by Andrey, I was missing the complete ISS info for
>> > >> the Data Abort raised upon a synchronous tag check fail.
>> > >>
>> > >> The following should fix that.  All the twisty little rules for
>> > >> the ISS.ISV bit are already handled by merge_syn_data_abort.
>> > >> Probably the most important bit that was missing was ISS.WnR,
>> > >> as that is independent of ISS.ISV.
>> > >>
>> > >> Andrey, will you please test?
>> > >
>> > > Looks like WnR is now being set properly, but SAS is still always 0.
>> >
>> > Are you looking at ESR_EL1?
>> >
>> > On page D13-2992 of revision F.a:
>> >
>> > # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.
>> >
>> > which means that ISS[23:14] are RES0, which includes SAS.
>>
>> +more Arm and Google people
>>
>> Is this known? Do we not get access size when MTE fault happens?
>
>
> It sounds like this applies to all data abort exceptions, no matter MTE or not.
>
>
> Correct. For data aborts in general, the extra syndrome information in ISS[23:14] is only provided at EL2, in order to help hypervisors emulate simple loads/stores (that access device memory) by looking at ESR_EL2 without having to decode the trapped instruction.

OK, got it.

The WnR bit works properly though, so

Tested-by: Andrey Konovalov <andreyknvl@google.com>

for the series.

> Did you have any particular use-case in mind for SAS being set even in ESR_EL1?

Yes, we could use that to extract the size of the access that caused
the fault to print more informative KASAN reports. We usually have a
header like:

BUG: KASAN: slab-out-of-bounds in usb_destroy_configuration+0x636/0x6d0
Read of size 8 at addr ffff8881c7e3c758 by task kworker/1:7/3434