Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall <julien@xxxxxxx>]

On 27/01/2022 13:20, Ayan Kumar Halder wrote:
> Hi,
>
> Asking here as well (might not have been clear on irc).
>
> On 27/01/2022 00:10, Julien Grall wrote:
> > Hi,
> >
> > On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, 
> > <ayan.kumar.halder@xxxxxxxxxx> 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.halder@xxxxxxxxxx> 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.
> Why should we care about MMIO when ISS is invalid ?
Because a translation fault doesn't mean this is emulated MMIO. This may 
be actual RAM but with the stage-2 entry marked as invalid for tracking 
purpose (or else).
> Should we not check 
> first if the ISS is valid or not as it will trigger IO_abort regardless 
> of the MMIO.
No. Imagine the guest decides to use a store exclusive on a RAM region 
that was temporally marked as invalid in the stage-2 page-table.
This will generate a data abort in Xen with ISV=0. We want to try to 
resolve the fault first and retry the instruction.
> I am assuming that once Xen resolves the MMIO 
> (p2m_resolve_translation_fault()), the guest will again try to run the 
> same instruction on MMIO region. This will be trapped in Xen which will 
> return IO abort as the post-indexing instruction could not be decoded.
The access will not trap again in Xen if the fault was resolved.

Cheers,

--
Julien Grall