[v4] linux-user: Streamline handling of SIGSEGV

[PATCH v4 17/41] linux-user/host/riscv: Improve host_signal_write

Posted by Richard Henderson 4 years, 4 months ago

Do not read 4 bytes before we determine the size of the insn.
Simplify triple switches in favor of checking major opcodes.
Include the missing cases of compact fsd and fsdsp.

Cc: qemu-riscv@nongnu.org
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/host/riscv/host-signal.h | 83 ++++++++++-------------------
 1 file changed, 28 insertions(+), 55 deletions(-)

diff --git a/linux-user/host/riscv/host-signal.h b/linux-user/host/riscv/host-signal.h
index 5860dce7d7..ab06d70964 100644
--- a/linux-user/host/riscv/host-signal.h
+++ b/linux-user/host/riscv/host-signal.h
@@ -17,65 +17,38 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
 
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
-    uint32_t insn = *(uint32_t *)host_signal_pc(uc);
-
     /*
-     * Detect store by reading the instruction at the program
-     * counter. Note: we currently only generate 32-bit
-     * instructions so we thus only detect 32-bit stores
+     * Detect store by reading the instruction at the program counter.
+     * Do not read more than 16 bits, because we have not yet determined
+     * the size of the instruction.
      */
-    switch (((insn >> 0) & 0b11)) {
-    case 3:
-        switch (((insn >> 2) & 0b11111)) {
-        case 8:
-            switch (((insn >> 12) & 0b111)) {
-            case 0: /* sb */
-            case 1: /* sh */
-            case 2: /* sw */
-            case 3: /* sd */
-            case 4: /* sq */
-                return true;
-            default:
-                break;
-            }
-            break;
-        case 9:
-            switch (((insn >> 12) & 0b111)) {
-            case 2: /* fsw */
-            case 3: /* fsd */
-            case 4: /* fsq */
-                return true;
-            default:
-                break;
-            }
-            break;
-        default:
-            break;
-        }
+    const uint16_t *pinsn = (const uint16_t *)host_signal_pc(uc);
+    uint16_t insn = pinsn[0];
+
+    /* 16-bit instructions */
+    switch (insn & 0xe003) {
+    case 0xa000: /* c.fsd */
+    case 0xc000: /* c.sw */
+    case 0xe000: /* c.sd (rv64) / c.fsw (rv32) */
+    case 0xa002: /* c.fsdsp */
+    case 0xc002: /* c.swsp */
+    case 0xe002: /* c.sdsp (rv64) / c.fswsp (rv32) */
+        return true;
     }
 
-    /* Check for compressed instructions */
-    switch (((insn >> 13) & 0b111)) {
-    case 7:
-        switch (insn & 0b11) {
-        case 0: /*c.sd */
-        case 2: /* c.sdsp */
-            return true;
-        default:
-            break;
-        }
-        break;
-    case 6:
-        switch (insn & 0b11) {
-        case 0: /* c.sw */
-        case 3: /* c.swsp */
-            return true;
-        default:
-            break;
-        }
-        break;
-    default:
-        break;
+    /* 32-bit instructions, major opcodes */
+    switch (insn & 0x7f) {
+    case 0x23: /* store */
+    case 0x27: /* store-fp */
+        return true;
+    case 0x2f: /* amo */
+        /*
+         * The AMO function code is in bits 25-31, unread as yet.
+         * The AMO functions are LR (read), SC (write), and the
+         * rest are all read-modify-write.
+         */
+        insn = pinsn[1];
+        return (insn >> 11) != 2; /* LR */
     }
 
     return false;
-- 
2.25.1

Re: [PATCH v4 17/41] linux-user/host/riscv: Improve host_signal_write

Posted by Alistair Francis 4 years, 4 months ago

On Thu, Oct 7, 2021 at 3:37 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Do not read 4 bytes before we determine the size of the insn.
> Simplify triple switches in favor of checking major opcodes.
> Include the missing cases of compact fsd and fsdsp.
>
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  linux-user/host/riscv/host-signal.h | 83 ++++++++++-------------------
>  1 file changed, 28 insertions(+), 55 deletions(-)
>
> diff --git a/linux-user/host/riscv/host-signal.h b/linux-user/host/riscv/host-signal.h
> index 5860dce7d7..ab06d70964 100644
> --- a/linux-user/host/riscv/host-signal.h
> +++ b/linux-user/host/riscv/host-signal.h
> @@ -17,65 +17,38 @@ static inline uintptr_t host_signal_pc(ucontext_t *uc)
>
>  static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
>  {
> -    uint32_t insn = *(uint32_t *)host_signal_pc(uc);
> -
>      /*
> -     * Detect store by reading the instruction at the program
> -     * counter. Note: we currently only generate 32-bit
> -     * instructions so we thus only detect 32-bit stores
> +     * Detect store by reading the instruction at the program counter.
> +     * Do not read more than 16 bits, because we have not yet determined
> +     * the size of the instruction.
>       */
> -    switch (((insn >> 0) & 0b11)) {
> -    case 3:
> -        switch (((insn >> 2) & 0b11111)) {
> -        case 8:
> -            switch (((insn >> 12) & 0b111)) {
> -            case 0: /* sb */
> -            case 1: /* sh */
> -            case 2: /* sw */
> -            case 3: /* sd */
> -            case 4: /* sq */
> -                return true;
> -            default:
> -                break;
> -            }
> -            break;
> -        case 9:
> -            switch (((insn >> 12) & 0b111)) {
> -            case 2: /* fsw */
> -            case 3: /* fsd */
> -            case 4: /* fsq */
> -                return true;
> -            default:
> -                break;
> -            }
> -            break;
> -        default:
> -            break;
> -        }
> +    const uint16_t *pinsn = (const uint16_t *)host_signal_pc(uc);
> +    uint16_t insn = pinsn[0];
> +
> +    /* 16-bit instructions */
> +    switch (insn & 0xe003) {
> +    case 0xa000: /* c.fsd */
> +    case 0xc000: /* c.sw */
> +    case 0xe000: /* c.sd (rv64) / c.fsw (rv32) */
> +    case 0xa002: /* c.fsdsp */
> +    case 0xc002: /* c.swsp */
> +    case 0xe002: /* c.sdsp (rv64) / c.fswsp (rv32) */
> +        return true;
>      }
>
> -    /* Check for compressed instructions */
> -    switch (((insn >> 13) & 0b111)) {
> -    case 7:
> -        switch (insn & 0b11) {
> -        case 0: /*c.sd */
> -        case 2: /* c.sdsp */
> -            return true;
> -        default:
> -            break;
> -        }
> -        break;
> -    case 6:
> -        switch (insn & 0b11) {
> -        case 0: /* c.sw */
> -        case 3: /* c.swsp */
> -            return true;
> -        default:
> -            break;
> -        }
> -        break;
> -    default:
> -        break;
> +    /* 32-bit instructions, major opcodes */
> +    switch (insn & 0x7f) {
> +    case 0x23: /* store */
> +    case 0x27: /* store-fp */
> +        return true;
> +    case 0x2f: /* amo */
> +        /*
> +         * The AMO function code is in bits 25-31, unread as yet.
> +         * The AMO functions are LR (read), SC (write), and the
> +         * rest are all read-modify-write.
> +         */
> +        insn = pinsn[1];
> +        return (insn >> 11) != 2; /* LR */
>      }
>
>      return false;
> --
> 2.25.1
>
>