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

Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler



Hi Ayan,

On 01/03/2022 12:40, Ayan Kumar Halder wrote:
+void post_increment_register(const struct instr_details *instr)
+{
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+    register_t val = 0;
+
+    /* 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 )
+    {
+        if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
+            val = regs->sp_el1;
+        else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
+                    ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )

You are using 3 times regs->cpsr & PSR_MODE_MASK. Can you introduce a temporary variable?

Alternatively, a switch could be used here.

[...]

diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h
index 3354d9c635..ef2c57a2d5 100644
--- a/xen/arch/arm/include/asm/mmio.h
+++ b/xen/arch/arm/include/asm/mmio.h
@@ -26,12 +26,24 @@
#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

NIT: Please add ',' even for the last item. This would reduce the diff if we add new one.

+};
+
  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;
@@ -69,14 +81,15 @@ struct vmmio {
  };
enum io_state try_handle_mmio(struct cpu_user_regs *regs,
-                              const union hsr hsr,
-                              paddr_t gpa);
+                              mmio_info_t *info);
  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 2ed2b85c6f..95c46ad391 100644
--- a/xen/arch/arm/include/asm/traps.h
+++ b/xen/arch/arm/include/asm/traps.h
@@ -109,6 +109,8 @@ 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 fad103bdbd..bea69ffb08 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -102,57 +102,79 @@ 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 )

This change is not explained in the commit message. TBH, I think it should be separate but I am not going to request that at v9.

+        {
+            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,
-                              const union hsr hsr,
-                              paddr_t gpa)
+                              mmio_info_t *info)
  {
      struct vcpu *v = current;
      const struct mmio_handler *handler = NULL;
-    const struct hsr_dabt dabt = hsr.dabt;
-    mmio_info_t info = {
-        .gpa = gpa,
-        .dabt = dabt
-    };
+    int rc;
- ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
+    ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);
- handler = find_mmio_handler(v->domain, info.gpa);
-    if ( !handler )
+    if ( !((info->dabt_instr.state == INSTR_VALID) || (info->dabt_instr.state 
== INSTR_LDR_STR_POSTINDEXING)) )

This check will become quite large if we decode more class. I would instead set the dabt.valid bit whenever we successfully decoded the instruction and check that if dabt.valid here.

      {
-        int rc;
+        ASSERT_UNREACHABLE();
+        return IO_ABORT;
+    }

[...]

@@ -1982,21 +2030,18 @@ static void do_trap_stage2_abort_guest(struct 
cpu_user_regs *regs,
              case IO_UNHANDLED:
                  /* IO unhandled, try another way to handle it. */
                  break;
-            }
          }
/*
-         * First check if the translation fault can be resolved by the
-         * P2M subsystem. If that's the case nothing else to do.
+         * 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.
           */
-        if ( p2m_resolve_translation_fault(current->domain,
-                                           gaddr_to_gfn(gpa)) )
-            return;
-
-        if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
+        if ( info.dabt.valid && check_p2m(is_data, gpa) )

This check would need to be adjusted to check the instruction state is INSTR_VALID.

Cheers,

--
Julien Grall



 


Rackspace

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