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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

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.

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 == 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