Hi, On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, <ayan.kumar.hal...@xilinx.com> wrote:
> Hi Julien, > On 26/01/2022 17:22, Julien Grall wrote: > > Hi, > > On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder, < > ayan.kumar.hal...@xilinx.com> wrote: > >> Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding for an >> exception >> from a Data Abort" :- >> ISV - ISS[23:14] holds a valid instruction syndrome >> >> When the ISV is 0, the instruction could not be decoded by the hardware >> (ie ISS >> is invalid). One should immediately return an error to the caller with an >> appropriate error message. > > > That's going to be very spammy because we have use-case where it could > trap without a valid ISV (e.g. when break-before-make happens). So please > don't had a message. > > There is already a logging statement in traps.c :- > > inject_abt: > gdprintk(XENLOG_DEBUG, > "HSR=%#"PRIregister" pc=%#"PRIregister" gva=%#"PRIvaddr" > gpa=%#"PRIpaddr"\n", > hsr.bits, regs->pc, gva, gpa); > > The reason for the error is to give the user some hint that an IO abort is > triggered by Xen. > The main difference here is inject_dabt should only be reached when we exhausted all the possibility in I/O handling. In your case, if we can't handle as an MMIO then we should fallthrough the other method (see do_trap_stage2_abort_guest()). In fact, moving the check early and doing the decoding before find_mmio() or try_fwd_io() is actually wrong. Sorry I should realized that earlier. So we want to provide an helper that will do the dabt.vid check and do the decoding. The helper will be called in 2 places. With that in place, then I agree the gprintk could be kept in place. Can we keep the error message ? > > Also, I think this is a duplicate check > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/ioreq.c;h=308650b40051825fdea78bb33bfbcc87ef5deaff;hb=HEAD#l79 > , I will remove this in v2 if it makes sense. > > - Ayan > > > That makes me think that the same will be valid for your other patch. > > There is no use of the MMIO handler. This is the >> reason why one should check for ISV before attempting to find a MMIO >> handler. >> >> Signed-off-by: Ayan Kumar Halder <ayank...@xilinx.com> >> --- >> >> Suggested by Julien Grall in >> https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html >> >> xen/arch/arm/io.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c >> index 729287e37c..14d39222f2 100644 >> --- a/xen/arch/arm/io.c >> +++ b/xen/arch/arm/io.c >> @@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs >> *regs, >> >> ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); >> >> + /* All the instructions used on emulated MMIO region should be valid >> */ >> + if ( !dabt.valid ) >> + { >> + gprintk(XENLOG_DEBUG, "No valid instruction syndrome for data >> abort\n"); >> + return IO_ABORT; >> + } >> + >> handler = find_mmio_handler(v->domain, info.gpa); >> if ( !handler ) >> { >> @@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct cpu_user_regs >> *regs, >> return rc; >> } >> >> - /* All the instructions used on emulated MMIO region should be valid >> */ >> - if ( !dabt.valid ) >> - return IO_ABORT; >> - >> /* >> * Erratum 766422: Thumb store translation fault to Hypervisor may >> * not have correct HSR Rt value. >> -- >> 2.17 >> >
