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

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

Hi Julien,

Thanks a lot for your clarification. Appreciate your help. I have a 
concern as below:-
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.
'.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);".
What do you think ?

- Ayan

> Cheers,