Re: [RFC PATCH] Added the logic to decode 32 bit ldr/str post-indexing instructions

[Ayan Kumar Halder]

Hi Andre,

Many thanks for your inputs.
Apologies if I sound dumb, but I need a few clarifications.
On 26/11/2021 13:14, Andre Przywara wrote:
On Fri, 19 Nov 2021 16:52:02 +0000
Ayan Kumar Halder <ayan.kumar.hal...@xilinx.com> wrote:

Hi,

At present, post indexing instructions are not emulated by Xen.
When Xen gets the exception, EL2_ESR.ISV bit not set. Thus as a
result, data abort is triggered.

Added the logic to decode ldr/str post indexing instructions.
With this, Xen can decode instructions like these:-
ldr w2, [x1], #4
Thus, domU can read ioreg with post indexing instructions.

Where do those instructions come from? A (C) compiler? (Some mail in
another thread from Stefano suggests so)
If yes, I would argue that is broken:
IIUC C compilers assume normal memory attributes for every pointer they
handle, so they are free to use unaligned accesses, load/store exclusives,
split accesses (two halfword reads) and what not when generating code.
The GIC needs to be mapped as device memory, which explicitly forbids
unaligned accesses and exclusives (as in: always traps), so you cannot let
compiler-generated code access the GIC (or most other MMIO devices, for
that matter).
I know, this somewhat works(TM) in practise, because a uint32_t assignment
is very likely to end up in an ldr/str, but please let me know which car
this code ends up in, so that can I avoid this brand ;-)

You can tell the compiler to avoid unaligned accesses with -mstrict-align
(and should definitely do so when you are running C code with the MMU
off), but that still leaves exclusives and split accesses at the
compiler's discretion. A variation on the topic of split access is merged
writes, where the compiler uses NEON or SVE instructions, for instance, to
cover multiple words at once, possibly via some memset()/memcpy() routine.

I understand that we should be using inline assembly instructions to 
access any MMIO region. This is to prevent the compiler doing any tricks.

But is there a restriction that post indexing instructions can never be 
used to access MMIO region ?


On top there is this architectural restriction of the ARMv7/v8
virtualisation extension to not decode many "advanced" load/store
instructions in ESR_EL2.
Where do I find this restriction ?

Are you telling me that load/store with post indexing is an "advanced" 
instruction and ArmV8 does not allow decoding of these instructions in 
ESR_EL2 ? Isn't that a very strong limitation ?

Also what is your opinion on Xen decoding these instructions ?

- Ayan

Linux deliberately coded readl/writel using inline assembly, to only use
instructions that provide syndrome information, plus guarantee
device-memory compatible semantics.
Check out https://lwn.net/Articles/698014/ for a comprehensive discussion
of this whole MMIO topic.

So I think you should do the same in your guest/bare metal code: define
{read,write}{b,h,l,q} as inline assembly functions, using ldr?/str? only.
See xen/include/asm-arm/arm64/io.h for an example that uses static inline
functions in a header file, to generate most optimal code. Then always do
MMIO only via those accessors. That prevents any future compiler
surprises, plus makes you perfectly virtualisable.

Cheers,
Andre.

Signed-off-by: Ayan Kumar Halder <ayank...@xilinx.com>
---
Note to reviewer:-
This patch is based on an issue discussed in
https://lists.xenproject.org/archives/html/xen-devel/2021-11/msg00969.html
"Xen/ARM - Query about a data abort seen while reading GICD registers"


  xen/arch/arm/decode.c | 77 +++++++++++++++++++++++++++++++++++++++++++
  xen/arch/arm/io.c     | 14 ++++++--
  2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
index 792c2e92a7..7b60bedbc5 100644
--- a/xen/arch/arm/decode.c
+++ b/xen/arch/arm/decode.c
@@ -84,6 +84,80 @@ bad_thumb2:
      return 1;
  }
  
+static inline int32_t extract32(uint32_t value, int start, int length)
+{
+    int32_t ret;
+
+    if ( !(start >= 0 && length > 0 && length <= 32 - start) )
+        return -EINVAL;
+
+    ret = (value >> start) & (~0U >> (32 - length));
+
+    return ret;
+}
+
+static int decode_64bit_loadstore_postindexing(register_t pc, struct hsr_dabt 
*dabt)
+{
+    uint32_t instr;
+    int size;
+    int v;
+    int opc;
+    int rt;
+    int imm9;
+
+    /* For details on decoding, refer to Armv8 Architecture reference manual
+     * Section - "Load/store register (immediate post-indexed)", Pg 318
+    */
+    if ( raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)) )
+        return -EFAULT;
+
+    /* First, let's check for the fixed values */
+
+    /*  As per the "Encoding table for the Loads and Stores group", Pg 299
+     * op4 = 1 - Load/store register (immediate post-indexed)
+     */
+    if ( extract32(instr, 10, 2) != 1 )
+        goto bad_64bit_loadstore;
+
+    /* For the following, refer to "Load/store register (immediate 
post-indexed)"
+     * to get the fixed values at various bit positions.
+     */
+    if ( extract32(instr, 21, 1) != 0 )
+        goto bad_64bit_loadstore;
+
+    if ( extract32(instr, 24, 2) != 0 )
+        goto bad_64bit_loadstore;
+
+    if ( extract32(instr, 27, 3) != 7 )
+        goto bad_64bit_loadstore;
+
+    size = extract32(instr, 30, 2);
+    v = extract32(instr, 26, 1);
+    opc = extract32(instr, 22, 1);
+
+    /* At the moment, we support STR(immediate) - 32 bit variant and
+     * LDR(immediate) - 32 bit variant only.
+     */
+    if (!((size==2) && (v==0) && ((opc==0) || (opc==1))))
+        goto bad_64bit_loadstore;
+
+    rt = extract32(instr, 0, 5);
+    imm9 = extract32(instr, 12, 9);
+
+    if ( imm9 < 0 )
+        update_dabt(dabt, rt, size, true);
+    else
+        update_dabt(dabt, rt, size, false);
+
+    dabt->valid = 1;
+
+
+    return 0;
+bad_64bit_loadstore:
+    gprintk(XENLOG_ERR, "unhandled 64bit instruction 0x%x\n", instr);
+    return 1;
+}
+
  static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
  {
      uint16_t instr;
@@ -155,6 +229,9 @@ int decode_instruction(const struct cpu_user_regs *regs, 
struct hsr_dabt *dabt)
      if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
          return decode_thumb(regs->pc, dabt);
  
+    if ( is_64bit_domain(current->domain) )
+        return decode_64bit_loadstore_postindexing(regs->pc, dabt);
+
      /* TODO: Handle ARM instruction */
      gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
  
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 729287e37c..49e80358c0 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -106,14 +106,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
          .gpa = gpa,
          .dabt = dabt
      };
+    int rc;
  
      ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
  
      handler = find_mmio_handler(v->domain, info.gpa);
      if ( !handler )
      {
-        int rc;
-
          rc = try_fwd_ioserv(regs, v, &info);
          if ( rc == IO_HANDLED )
              return handle_ioserv(regs, v);
@@ -123,7 +122,16 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs,
  
      /* All the instructions used on emulated MMIO region should be valid */
      if ( !dabt.valid )
-        return IO_ABORT;
+    {
+        /*
+         * Post indexing ldr/str instructions are not emulated by Xen. So, the
+         * ISS is invalid. In such a scenario, we try to manually decode the
+         * instruction from the program counter.
+         */
+        rc = decode_instruction(regs, &info.dabt);
+        if ( rc )
+            return IO_ABORT;
+    }
  
      /*
       * Erratum 766422: Thumb store translation fault to Hypervisor may