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,

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