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

Re: [XEN v4] xen/arm64: io: Decode ldr/str post-indexing instructions



Hi,

On 26/01/2022 01:45, Stefano Stabellini wrote:
On Tue, 25 Jan 2022, Julien Grall wrote:
+
       /* TODO: Handle ARM instruction */
       gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
         return 1;
   }
   +#if CONFIG_ARM_64
+void post_increment_register(union ldr_str_instr_class *instr)

instr should not be modified, so please use const. Also, it would be
preferrable to pass the regs in parameter. So the none of the decoding code
relies on the current regs.

Furthermore, decode.c should only contain code to update the syndrome and in
theory Arm could decide to provide an valid syndrome in future revision. So I
would move this code in io.c (or maybe traps.c).

I was the one to suggest moving it to decode.c to keep it closer to the
decoding function it is related to, and also because it felt a bit out
of place in io.c.

How about traps.c? This is where we also take care of incrementing pc after we handle a MMIO trap.

+{
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+    register_t val;
+
+    /* handle when rn = SP */
+    if ( instr->code.rn == 31 )
+        val = regs->sp_el1;
+    else
+        val = get_user_reg(regs, instr->code.rn);
+
+    val += instr->code.imm9;
+
+    if ( instr->code.rn == 31 )
+        regs->sp_el1 = val;
+    else
+        set_user_reg(regs, instr->code.rn, val);
+}
+#endif
+
   /*
    * Local variables:
    * mode: C
diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
index 4613763bdb..511cd4a05f 100644
--- a/xen/arch/arm/decode.h
+++ b/xen/arch/arm/decode.h
@@ -23,6 +23,35 @@
   #include <asm/regs.h>
   #include <asm/processor.h>
   +/*
+ * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores
+ * Page 318 specifies the following bit pattern for
+ * "load/store register (immediate post-indexed)".
+ *
+ * 31 30 29  27 26 25  23   21 20              11   9         4       0
+ * ___________________________________________________________________
+ * |size|1 1 1 |V |0 0 |opc |0 |      imm9     |0 1 |  Rn     |  Rt   |
+ * |____|______|__|____|____|__|_______________|____|_________|_______|
+ */
+union ldr_str_instr_class {
+    uint32_t value;
+    struct ldr_str {

No need to name the struct here.

+        unsigned int rt:5;     /* Rt register */
+        unsigned int rn:5;     /* Rn register */
+        unsigned int fixed1:2; /* value == 01b */
+        signed int imm9:9;            /* imm9 */
+        unsigned int fixed2:1; /* value == 0b */
+        unsigned int opc:2;    /* opc */
+        unsigned int fixed3:2; /* value == 00b */
+        unsigned int v:1;      /* vector */
+        unsigned int fixed4:3; /* value == 111b */
+        unsigned int size:2;   /* size */
+    } code;

It would be best to name it ldr_str so this can be easily extended (e.g. no renaming) for other instructions in the future.

+};

Looking at the code, post_increment_register() only care about 'rn' and
'imm9'. So rather than exposing the full instruction, could we instead provide
the strict minimum? I.e something like:

struct
{
      enum instr_type; /* Unknown, ldr/str post increment */
      union
      {
          struct
          {
            register; /* Register to increment */
            imm;      /* Immediate to add */
          } ldr_str;
      }
      uint64_t register;
}
The full description helped a lot during review. I would prefer to keep
it if you don't feel strongly about it.

I haven't suggested to drop the union. Instead, I am suggesting to keep it internally to decode.c and expose something different to the external the user. The idea is the caller doesn't care about the full instruction, it only cares about what action to do.

Basically, what I am asking is an augmented dabt. So all the information are in one place rather than having to carry two structure (struct hsr_dabt and union ldr_str_instr_class) which contain mostly redundant information.

Cheers,

--
Julien Grall



 


Rackspace

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