[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[xen staging] Revert "xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler"



commit 07aebcd55fd2f7997e9fe50a6c849c8a12ec2e68
Author:     Julien Grall <jgrall@xxxxxxxxxx>
AuthorDate: Mon Mar 14 22:58:33 2022 +0000
Commit:     Julien Grall <jgrall@xxxxxxxxxx>
CommitDate: Mon Mar 14 23:01:02 2022 +0000

    Revert "xen/arm64: io: Support instructions (for which ISS is not valid) on 
emulated MMIO region using MMIO/ioreq handler"
    
    This reverts commit 9e5a68a6652cc54ce3cb3b0ce208eeed79d5aeb5.
    
    This breaks boot on arm32:
    
    https://lore.kernel.org/xen-devel/osstest-168589-mainreport@xxxxxxx/T/#u
---
 xen/arch/arm/arm32/traps.c        | 11 -----
 xen/arch/arm/arm64/traps.c        | 52 ----------------------
 xen/arch/arm/decode.c             |  2 -
 xen/arch/arm/include/asm/domain.h |  4 --
 xen/arch/arm/include/asm/mmio.h   | 17 +-------
 xen/arch/arm/include/asm/traps.h  |  2 -
 xen/arch/arm/io.c                 | 90 +++++++++++++++------------------------
 xen/arch/arm/ioreq.c              |  8 +---
 xen/arch/arm/traps.c              | 77 +++++++--------------------------
 xen/arch/x86/include/asm/ioreq.h  |  3 --
 xen/include/xen/sched.h           |  2 -
 11 files changed, 54 insertions(+), 214 deletions(-)

diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index 159e3cef8b..9c9790a6d1 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -18,11 +18,9 @@
 
 #include <xen/lib.h>
 #include <xen/kernel.h>
-#include <xen/sched.h>
 
 #include <public/xen.h>
 
-#include <asm/mmio.h>
 #include <asm/processor.h>
 #include <asm/traps.h>
 
@@ -84,15 +82,6 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
         do_unexpected_trap("Data Abort", regs);
 }
 
-void post_increment_register(const struct instr_details *instr)
-{
-    /*
-     * We have not implemented decoding of post indexing instructions for 32 
bit.
-     * Thus, this should be unreachable.
-     */
-    domain_crash(current->domain);
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
index 6ce4a1fa8c..9113a15c7a 100644
--- a/xen/arch/arm/arm64/traps.c
+++ b/xen/arch/arm/arm64/traps.c
@@ -17,7 +17,6 @@
  */
 
 #include <xen/lib.h>
-#include <xen/sched.h>
 
 #include <asm/hsr.h>
 #include <asm/system.h>
@@ -45,57 +44,6 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
     panic("bad mode\n");
 }
 
-void post_increment_register(const struct instr_details *instr)
-{
-    struct cpu_user_regs *regs = guest_cpu_user_regs();
-    register_t val = 0;
-    uint8_t psr_mode = (regs->cpsr & PSR_MODE_MASK);
-
-    /* Currently, we handle only ldr/str post indexing instructions */
-    if ( instr->state != INSTR_LDR_STR_POSTINDEXING )
-        return;
-
-    /*
-     * Handle when rn = SP
-     * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register
-     * selection"
-     * t = SP_EL0
-     * h = SP_ELx
-     * and M[3:0] (Page - C5-474 "When exception taken from AArch64 state:")
-     */
-    if ( instr->rn == 31 )
-    {
-        switch ( psr_mode )
-        {
-        case PSR_MODE_EL1h:
-            val = regs->sp_el1;
-            break;
-        case PSR_MODE_EL1t:
-        case PSR_MODE_EL0t:
-            val = regs->sp_el0;
-            break;
-
-        default:
-            domain_crash(current->domain);
-            return;
-        }
-    }
-    else
-        val = get_user_reg(regs, instr->rn);
-
-    val += instr->imm9;
-
-    if ( instr->rn == 31 )
-    {
-        if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
-            regs->sp_el1 = val;
-        else
-            regs->sp_el0 = val;
-    }
-    else
-        set_user_reg(regs, instr->rn, val);
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index f5f6562600..3add87e83a 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -146,10 +146,8 @@ static int decode_arm64(register_t pc, mmio_info_t *info)
 
     update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false);
 
-    dabt_instr->state = INSTR_LDR_STR_POSTINDEXING;
     dabt_instr->rn = opcode.ldr_str.rn;
     dabt_instr->imm9 = opcode.ldr_str.imm9;
-    dabt->valid = 1;
 
     return 0;
 
diff --git a/xen/arch/arm/include/asm/domain.h 
b/xen/arch/arm/include/asm/domain.h
index ed63c2b6f9..c56f6e4398 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -281,10 +281,6 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
 /* vPCI is not available on Arm */
 #define has_vpci(d)    ({ (void)(d); false; })
 
-struct arch_vcpu_io {
-    struct instr_details dabt_instr; /* when the instruction is decoded */
-};
-
 #endif /* __ASM_DOMAIN_H__ */
 
 /*
diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
index ca259a79c2..3354d9c635 100644
--- a/xen/arch/arm/include/asm/mmio.h
+++ b/xen/arch/arm/include/asm/mmio.h
@@ -26,24 +26,12 @@
 
 #define MAX_IO_HANDLER  16
 
-enum instr_decode_state
-{
-    INSTR_ERROR,                    /* Error encountered while decoding instr 
*/
-    INSTR_VALID,                    /* ISS is valid, so no need to decode */
-    /*
-     * Instruction is decoded successfully. It is a ldr/str post indexing
-     * instruction.
-     */
-    INSTR_LDR_STR_POSTINDEXING,
-};
-
 typedef struct
 {
     struct hsr_dabt dabt;
     struct instr_details {
         unsigned long rn:5;
         signed int imm9:9;
-        enum instr_decode_state state;
     } dabt_instr;
     paddr_t gpa;
 } mmio_info_t;
@@ -81,15 +69,14 @@ struct vmmio {
 };
 
 enum io_state try_handle_mmio(struct cpu_user_regs *regs,
-                              mmio_info_t *info);
+                              const union hsr hsr,
+                              paddr_t gpa);
 void register_mmio_handler(struct domain *d,
                            const struct mmio_handler_ops *ops,
                            paddr_t addr, paddr_t size, void *priv);
 int domain_io_init(struct domain *d, int max_count);
 void domain_io_free(struct domain *d);
 
-void try_decode_instruction(const struct cpu_user_regs *regs,
-                            mmio_info_t *info);
 
 #endif  /* __ASM_ARM_MMIO_H__ */
 
diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h
index 95c46ad391..2ed2b85c6f 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -109,8 +109,6 @@ static inline register_t sign_extend(const struct hsr_dabt 
dabt, register_t r)
     return r;
 }
 
-void post_increment_register(const struct instr_details *instr);
-
 #endif /* __ASM_ARM_TRAPS__ */
 /*
  * Local variables:
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index fd903b7b03..fad103bdbd 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -102,79 +102,57 @@ static const struct mmio_handler 
*find_mmio_handler(struct domain *d,
     return handler;
 }
 
-void try_decode_instruction(const struct cpu_user_regs *regs,
-                            mmio_info_t *info)
-{
-    int rc;
-
-    if ( info->dabt.valid )
-    {
-        info->dabt_instr.state = INSTR_VALID;
-
-        /*
-         * Erratum 766422: Thumb store translation fault to Hypervisor may
-         * not have correct HSR Rt value.
-         */
-        if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
-             info->dabt.write )
-        {
-            rc = decode_instruction(regs, info);
-            if ( rc )
-            {
-                gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
-                info->dabt_instr.state = INSTR_ERROR;
-            }
-        }
-        return;
-    }
-
-    /*
-     * Armv8 processor does not provide a valid syndrome for decoding some
-     * instructions. So in order to process these instructions, Xen must
-     * decode them.
-     */
-    rc = decode_instruction(regs, info);
-    if ( rc )
-    {
-        gprintk(XENLOG_ERR, "Unable to decode instruction\n");
-        info->dabt_instr.state = INSTR_ERROR;
-    }
-}
-
 enum io_state try_handle_mmio(struct cpu_user_regs *regs,
-                              mmio_info_t *info)
+                              const union hsr hsr,
+                              paddr_t gpa)
 {
     struct vcpu *v = current;
     const struct mmio_handler *handler = NULL;
-    int rc;
+    const struct hsr_dabt dabt = hsr.dabt;
+    mmio_info_t info = {
+        .gpa = gpa,
+        .dabt = dabt
+    };
 
-    ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
+    ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
 
-    if ( !info->dabt.valid )
-    {
-        ASSERT_UNREACHABLE();
-        return IO_ABORT;
-    }
-
-    handler = find_mmio_handler(v->domain, info->gpa);
+    handler = find_mmio_handler(v->domain, info.gpa);
     if ( !handler )
     {
-        rc = try_fwd_ioserv(regs, v, info);
+        int rc;
+
+        rc = try_fwd_ioserv(regs, v, &info);
         if ( rc == IO_HANDLED )
             return handle_ioserv(regs, v);
 
         return rc;
     }
 
+    /* All the instructions used on emulated MMIO region should be valid */
+    if ( !dabt.valid )
+        return IO_ABORT;
+
     /*
-     * At this point, we know that the instruction is either valid or has been
-     * decoded successfully. Thus, Xen should be allowed to execute the
-     * instruction on the emulated MMIO region.
+     * Erratum 766422: Thumb store translation fault to Hypervisor may
+     * not have correct HSR Rt value.
      */
-    if ( info->dabt.write )
-        return handle_write(handler, v, info);
+    if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
+         dabt.write )
+    {
+        int rc;
+
+        rc = decode_instruction(regs, &info);
+        if ( rc )
+        {
+            gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
+            return IO_ABORT;
+        }
+    }
+
+    if ( info.dabt.write )
+        return handle_write(handler, v, &info);
     else
-        return handle_read(handler, v, info);
+        return handle_read(handler, v, &info);
 }
 
 void register_mmio_handler(struct domain *d,
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index cc9bf23213..308650b400 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -47,8 +47,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
                              struct vcpu *v, mmio_info_t *info)
 {
     struct vcpu_io *vio = &v->io;
-    struct instr_details instr = info->dabt_instr;
-    struct hsr_dabt dabt = info->dabt;
     ioreq_t p = {
         .type = IOREQ_TYPE_COPY,
         .addr = info->gpa,
@@ -78,10 +76,10 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
     if ( !s )
         return IO_UNHANDLED;
 
-    ASSERT(dabt.valid);
+    if ( !info->dabt.valid )
+        return IO_ABORT;
 
     vio->req = p;
-    vio->info.dabt_instr = instr;
 
     rc = ioreq_send(s, &p, 0);
     if ( rc != IO_RETRY || v->domain->is_shutting_down )
@@ -97,7 +95,6 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
 bool arch_ioreq_complete_mmio(void)
 {
     struct vcpu *v = current;
-    struct instr_details dabt_instr = v->io.info.dabt_instr;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     const union hsr hsr = { .bits = regs->hsr };
 
@@ -109,7 +106,6 @@ bool arch_ioreq_complete_mmio(void)
 
     if ( handle_ioserv(regs, v) == IO_HANDLED )
     {
-        post_increment_register(&dabt_instr);
         advance_pc(regs, hsr);
         return true;
     }
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 53652d7781..7a1b679b8c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1893,21 +1893,6 @@ static bool try_map_mmio(gfn_t gfn)
     return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
 }
 
-static inline bool check_p2m(bool is_data, paddr_t gpa)
-{
-    /*
-     * First check if the translation fault can be resolved by the P2M 
subsystem.
-     * If that's the case nothing else to do.
-     */
-    if ( p2m_resolve_translation_fault(current->domain , gaddr_to_gfn(gpa)) )
-        return true;
-
-    if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
-        return true;
-
-    return false;
-}
-
 static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
                                        const union hsr hsr)
 {
@@ -1921,8 +1906,6 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
     paddr_t gpa;
     uint8_t fsc = xabt.fsc & ~FSC_LL_MASK;
     bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
-    mmio_info_t info;
-    enum io_state state;
 
     /*
      * If this bit has been set, it means that this stage-2 abort is caused
@@ -1976,52 +1959,21 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
         return;
     }
     case FSC_FLT_TRANS:
-    {
-        info.gpa = gpa;
-        info.dabt = hsr.dabt;
-
         /*
-         * Assumption :- Most of the times when we get a data abort and the ISS
-         * is invalid or an instruction abort, the underlying cause is that the
-         * page tables have not been set up correctly.
+         * Attempt first to emulate the MMIO as the data abort will
+         * likely happen in an emulated region.
+         *
+         * Note that emulated region cannot be executed
          */
-        if ( !is_data || !info.dabt.valid )
+        if ( is_data )
         {
-            if ( check_p2m(is_data, gpa) )
-                return;
-
-            /*
-             * If the instruction abort could not be resolved by setting the
-             * appropriate bits in the translation table, then Xen should
-             * forward the abort to the guest.
-             */
-            if ( !is_data )
-                goto inject_abt;
-        }
-
-        try_decode_instruction(regs, &info);
-
-        /*
-         * If Xen could not decode the instruction or encountered an error
-         * while decoding, then it should forward the abort to the guest.
-         */
-        if ( info.dabt_instr.state == INSTR_ERROR )
-            goto inject_abt;
-
-        state = try_handle_mmio(regs, &info);
+            enum io_state state = try_handle_mmio(regs, hsr, gpa);
 
-        switch ( state )
-        {
+            switch ( state )
+            {
             case IO_ABORT:
                 goto inject_abt;
             case IO_HANDLED:
-                /*
-                 * If the instruction was decoded and has executed successfully
-                 * on the MMIO region, then Xen should execute the next part of
-                 * the instruction. (for eg increment the rn if it is a
-                 * post-indexing instruction.
-                 */
-                post_increment_register(&info.dabt_instr);
                 advance_pc(regs, hsr);
                 return;
             case IO_RETRY:
@@ -2030,18 +1982,21 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
             case IO_UNHANDLED:
                 /* IO unhandled, try another way to handle it. */
                 break;
+            }
         }
 
         /*
-         * If the instruction syndrome was invalid, then we already checked if
-         * this was due to a P2M fault. So no point to check again as the 
result
-         * will be the same.
+         * First check if the translation fault can be resolved by the
+         * P2M subsystem. If that's the case nothing else to do.
          */
-        if ( (info.dabt_instr.state == INSTR_VALID) && check_p2m(is_data, gpa) 
)
+        if ( p2m_resolve_translation_fault(current->domain,
+                                           gaddr_to_gfn(gpa)) )
+            return;
+
+        if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
             return;
 
         break;
-    }
     default:
         gprintk(XENLOG_WARNING,
                 "Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
diff --git a/xen/arch/x86/include/asm/ioreq.h b/xen/arch/x86/include/asm/ioreq.h
index ecfe7f9fdb..d06ce9a6ea 100644
--- a/xen/arch/x86/include/asm/ioreq.h
+++ b/xen/arch/x86/include/asm/ioreq.h
@@ -26,9 +26,6 @@
 #include <asm/hvm/ioreq.h>
 #endif
 
-struct arch_vcpu_io {
-};
-
 #endif /* __ASM_X86_IOREQ_H__ */
 
 /*
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 406d9bc610..10ea969c7a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -160,8 +160,6 @@ struct vcpu_io {
     /* I/O request in flight to device model. */
     enum vio_completion  completion;
     ioreq_t              req;
-    /* Arch specific info pertaining to the io request */
-    struct arch_vcpu_io  info;
 };
 
 struct vcpu
--
generated by git-patchbot for /home/xen/git/xen.git#staging



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.