| Message ID | 20200812171946.2044791-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | target/arm: Complete ISS for MTE tag check fail | expand |
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 >
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~
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~
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?
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. <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Aug 12, 2020 at 11:03 AM Andrey Konovalov <<a href="mailto:andreyknvl@google.com">andreyknvl@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Aug 12, 2020 at 7:52 PM Richard Henderson<br> <<a href="mailto:richard.henderson@linaro.org" target="_blank">richard.henderson@linaro.org</a>> wrote:<br> ><br> > On 8/12/20 10:38 AM, Andrey Konovalov wrote:<br> > > On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson<br> > > <<a href="mailto:richard.henderson@linaro.org" target="_blank">richard.henderson@linaro.org</a>> wrote:<br> > >><br> > >> As reported by Andrey, I was missing the complete ISS info for<br> > >> the Data Abort raised upon a synchronous tag check fail.<br> > >><br> > >> The following should fix that. All the twisty little rules for<br> > >> the ISS.ISV bit are already handled by merge_syn_data_abort.<br> > >> Probably the most important bit that was missing was ISS.WnR,<br> > >> as that is independent of ISS.ISV.<br> > >><br> > >> Andrey, will you please test?<br> > ><br> > > Looks like WnR is now being set properly, but SAS is still always 0.<br> ><br> > Are you looking at ESR_EL1?<br> ><br> > On page D13-2992 of revision F.a:<br> ><br> > # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.<br> ><br> > which means that ISS[23:14] are RES0, which includes SAS.<br> <br> +more Arm and Google people<br> <br> Is this known? Do we not get access size when MTE fault happens?<br></blockquote><div><br></div><div>It sounds like this applies to all data abort exceptions, no matter MTE or not.</div><div> </div></div></div>
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 <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> On 12/08/2020 20:06, Evgenii Stepanov wrote:<br> <blockquote type="cite" cite="mid:CAFKCwrjSU89jiUbzd8Ys8nV6NDCJer=FbUnGWv8m0p0E+9MdVg@mail.gmail.com"> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> <div dir="ltr">On Wed, Aug 12, 2020 at 11:03 AM Andrey Konovalov <<a href="mailto:andreyknvl@google.com" moz-do-not-send="true">andreyknvl@google.com</a>> wrote:<br> <div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Aug 12, 2020 at 7:52 PM Richard Henderson<br> <<a href="mailto:richard.henderson@linaro.org" target="_blank" moz-do-not-send="true">richard.henderson@linaro.org</a>> wrote:<br> ><br> > On 8/12/20 10:38 AM, Andrey Konovalov wrote:<br> > > On Wed, Aug 12, 2020 at 7:19 PM Richard Henderson<br> > > <<a href="mailto:richard.henderson@linaro.org" target="_blank" moz-do-not-send="true">richard.henderson@linaro.org</a>> wrote:<br> > >><br> > >> As reported by Andrey, I was missing the complete ISS info for<br> > >> the Data Abort raised upon a synchronous tag check fail.<br> > >><br> > >> The following should fix that. All the twisty little rules for<br> > >> the ISS.ISV bit are already handled by merge_syn_data_abort.<br> > >> Probably the most important bit that was missing was ISS.WnR,<br> > >> as that is independent of ISS.ISV.<br> > >><br> > >> Andrey, will you please test?<br> > ><br> > > Looks like WnR is now being set properly, but SAS is still always 0.<br> ><br> > Are you looking at ESR_EL1?<br> ><br> > On page D13-2992 of revision F.a:<br> ><br> > # ISV is 0 for all faults reported in ESR_EL1 or ESR_EL3.<br> ><br> > which means that ISS[23:14] are RES0, which includes SAS.<br> <br> +more Arm and Google people<br> <br> Is this known? Do we not get access size when MTE fault happens?<br> </blockquote> <div><br> </div> <div>It sounds like this applies to all data abort exceptions, no matter MTE or not.</div> </div> </div> </blockquote> <br> 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?<br> <br> Kevin<br> </body> </html>
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