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,

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 ? Should we not check 
first if the ISS is valid or not as it will trigger IO_abort regardless 
of the MMIO.
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.
In this scenario, translation fault resolved by Xen was of no use.

Please help to clear my doubts.

- Ayan

> 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 <ayankuma@xxxxxxxxxx>
> >         ---
> >
> >         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 <http://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