* [PATCH 1/3] target/arm: Export merge_syn_data_abort from tlb_helper.c
2020-08-12 17:19 [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail Richard Henderson
@ 2020-08-12 17:19 ` Richard Henderson
2020-08-12 17:19 ` [PATCH 2/3] target/arm: Pass the entire mte descriptor to mte_check_fail Richard Henderson
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2020-08-12 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Andrey Konovalov, Vincenzo Frascino, alex.bennee, peter.maydell
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] target/arm: Pass the entire mte descriptor to mte_check_fail
2020-08-12 17:19 [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail Richard Henderson
2020-08-12 17:19 ` [PATCH 1/3] target/arm: Export merge_syn_data_abort from tlb_helper.c Richard Henderson
@ 2020-08-12 17:19 ` Richard Henderson
2020-08-12 17:19 ` [PATCH 3/3] target/arm: Merge ISS for data abort from tag check fail Richard Henderson
2020-08-12 17:38 ` [PATCH 0/3] target/arm: Complete ISS for MTE " Andrey Konovalov
3 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2020-08-12 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Andrey Konovalov, Vincenzo Frascino, alex.bennee, peter.maydell
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] target/arm: Merge ISS for data abort from tag check fail
2020-08-12 17:19 [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail Richard Henderson
2020-08-12 17:19 ` [PATCH 1/3] target/arm: Export merge_syn_data_abort from tlb_helper.c Richard Henderson
2020-08-12 17:19 ` [PATCH 2/3] target/arm: Pass the entire mte descriptor to mte_check_fail Richard Henderson
@ 2020-08-12 17:19 ` Richard Henderson
2020-08-12 17:38 ` [PATCH 0/3] target/arm: Complete ISS for MTE " Andrey Konovalov
3 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2020-08-12 17:19 UTC (permalink / raw)
To: qemu-devel
Cc: Andrey Konovalov, Vincenzo Frascino, alex.bennee, peter.maydell
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-12 17:19 [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail Richard Henderson
` (2 preceding siblings ...)
2020-08-12 17:19 ` [PATCH 3/3] target/arm: Merge ISS for data abort from tag check fail Richard Henderson
@ 2020-08-12 17:38 ` Andrey Konovalov
2020-08-12 17:52 ` Richard Henderson
3 siblings, 1 reply; 11+ messages in thread
From: Andrey Konovalov @ 2020-08-12 17:38 UTC (permalink / raw)
To: Richard Henderson
Cc: qemu-devel, Vincenzo Frascino, alex.bennee, peter.maydell
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-12 17:38 ` [PATCH 0/3] target/arm: Complete ISS for MTE " Andrey Konovalov
@ 2020-08-12 17:52 ` Richard Henderson
2020-08-12 18:00 ` Richard Henderson
2020-08-12 18:02 ` Andrey Konovalov
0 siblings, 2 replies; 11+ messages in thread
From: Richard Henderson @ 2020-08-12 17:52 UTC (permalink / raw)
To: Andrey Konovalov
Cc: peter.maydell, Vincenzo Frascino, alex.bennee, qemu-devel
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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-12 17:52 ` Richard Henderson
@ 2020-08-12 18:00 ` Richard Henderson
2020-08-12 18:02 ` Andrey Konovalov
1 sibling, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2020-08-12 18:00 UTC (permalink / raw)
To: Andrey Konovalov
Cc: peter.maydell, Vincenzo Frascino, alex.bennee, qemu-devel
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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-12 17:52 ` Richard Henderson
2020-08-12 18:00 ` Richard Henderson
@ 2020-08-12 18:02 ` Andrey Konovalov
2020-08-12 19:06 ` Evgenii Stepanov
1 sibling, 1 reply; 11+ messages in thread
From: Andrey Konovalov @ 2020-08-12 18:02 UTC (permalink / raw)
To: Vincenzo Frascino, Catalin Marinas, Kevin Brodsky, Branislav Rankov
Cc: qemu-devel, alex.bennee, peter.maydell, Evgenii Stepanov,
Elena Petrova, Kostya Serebryany, Dmitry Vyukov,
Richard Henderson
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?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-12 18:02 ` Andrey Konovalov
@ 2020-08-12 19:06 ` Evgenii Stepanov
2020-08-13 10:01 ` Kevin Brodsky
0 siblings, 1 reply; 11+ messages in thread
From: Evgenii Stepanov @ 2020-08-12 19:06 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Vincenzo Frascino, Catalin Marinas, Kevin Brodsky,
Branislav Rankov, qemu-devel, alex.bennee, peter.maydell,
Elena Petrova, Kostya Serebryany, Dmitry Vyukov,
Richard Henderson, Peter Collingbourne
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-12 19:06 ` Evgenii Stepanov
@ 2020-08-13 10:01 ` Kevin Brodsky
2020-08-13 12:26 ` Andrey Konovalov
0 siblings, 1 reply; 11+ messages in thread
From: Kevin Brodsky @ 2020-08-13 10:01 UTC (permalink / raw)
To: Evgenii Stepanov, Andrey Konovalov
Cc: peter.maydell, Branislav Rankov, Elena Petrova,
Peter Collingbourne, Catalin Marinas, Richard Henderson,
qemu-devel, Kostya Serebryany, Vincenzo Frascino, alex.bennee,
Dmitry Vyukov
[-- 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 --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] target/arm: Complete ISS for MTE tag check fail
2020-08-13 10:01 ` Kevin Brodsky
@ 2020-08-13 12:26 ` Andrey Konovalov
0 siblings, 0 replies; 11+ messages in thread
From: Andrey Konovalov @ 2020-08-13 12:26 UTC (permalink / raw)
To: Kevin Brodsky, Richard Henderson
Cc: Evgenii Stepanov, Vincenzo Frascino, Catalin Marinas,
Branislav Rankov, qemu-devel, alex.bennee, peter.maydell,
Elena Petrova, Kostya Serebryany, Dmitry Vyukov,
Peter Collingbourne
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
^ permalink raw reply [flat|nested] 11+ messages in thread