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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 27 Jan 2022, Julien Grall wrote:
> On 27/01/2022 15:48, Ayan Kumar Halder wrote:
> > On 27/01/2022 13:57, Julien Grall wrote:
> > > 
> > > 
> > > 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.
> > 
> > I think your words makes sense.
> > 
> > However, I am still concerned that we might not be doing the correct thing
> > in try_fwd_ioserv().
> > 
> > See this :-
> > 
> >      ioreq_t p = {
> >          .type = IOREQ_TYPE_COPY,
> >          .addr = info->gpa,
> >          .size = 1 << info->dabt.size,
> >          .count = 1,
> >          .dir = !info->dabt.write,
> >          /*
> >           * On x86, df is used by 'rep' instruction to tell the direction
> >           * to iterate (forward or backward).
> >           * On Arm, all the accesses to MMIO region will do a single
> >           * memory access. So for now, we can safely always set to 0.
> >           */
> >          .df = 0,
> >          .data = get_user_reg(regs, info->dabt.reg),
> >          .state = STATE_IOREQ_READY,
> >      };
> > If info->dabt.valid = 0, then '.size', '.dir' and '.data' fields are
> > invalid.
> 
> Sort of. ISV=0 means that bits [23:14] are RES0. So this doesn't cover the
> field WnR. The validity of WnR will depend on DFSC. In our case, we will
> always reach this code with a translation fault. So WnR should always be
> valid.
> 
> That said, the rest will not be valid.
> 
> > 
> > '.size' gets used in ioreq_server_select() to obtain the end address. It
> > seems incorrect to me.
> > 
> > I suppose "if ( !info->dabt.valid )" needs to be checked before "s =
> > ioreq_server_select(v->domain, &p);".
> 
> The trouble is we would need to return IO_UNHANDLED (so the rest of the code
> can pick it up) which I think is incorrect here.
> 
> The approach I can think of is to call ioreq_server_select() with size = 0
> (i.e. byte access). Then decode the size (if needed) and then check the access
> is correct.
> 
> This is not very nice. But that at the same time, I would like to avoid
> dealing emulated I/O in Xen or outside differently. Stefano?

I am with you on both points.

One thing I noticed is that the code today is not able to deal with
IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
not called a second time (or am I mistaken?)

Another thing I noticed is that currently find_mmio_handler and
try_fwd_ioserv expect dabt to be already populated and valid so it would
be better if we could get there only when dabt.valid.

With these two things in mind, I think maybe the best thing to do is to
change the code in do_trap_stage2_abort_guest slightly so that
p2m_resolve_translation_fault and try_map_mmio are called first when
!dabt.valid.

Patch below only for explaning; it doesn't build as is and I am not sure
it is 100% correct. For instance, if try_map_mmio succeeds, should we
return or goto again?


diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9339d12f58..d09f901a50 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1965,7 +1965,8 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
          *
          * Note that emulated region cannot be executed
          */
-        if ( is_data )
+again:
+        if ( is_data && hsr.dabt.valid )
         {
             enum io_state state = try_handle_mmio(regs, hsr, gpa);
 
@@ -1996,6 +1997,12 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
         if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
             return;
 
+        if ( !hsr.dabt.valid )
+        {
+            if ( !decode_instruction(regs, &hsr.dabt) )
+                goto again;
+        }
+
         break;
     default:
         gprintk(XENLOG_WARNING,