Re: [PATCH v2 3/3] x86: mmio: add support for 0x66 operand prefix

Fri, 07 Jun 2019 04:04:48 -0700 

On 07.06.19 00:44, Ralf Ramsauer wrote:

mov (%rax), %ax is a 16-bit data MOV_FROM_MEM that will emit
0x66 0x8b 0x00.

0x66 is the operand-size override prefix which we currently do not support.

We should support it, as we can find this opcode, for example, for some
mmconfig space access from Linux (e.g., pci_generic_config_read).

This also adds appropriate mmio-access tests.

Tested in QEMU virtual target.

Signed-off-by: Ralf Ramsauer <[email protected]>
---
  hypervisor/arch/x86/include/asm/processor.h |  1 +
  hypervisor/arch/x86/mmio.c                  | 47 +++++++++++++++------
  inmates/tests/x86/mmio-access-32.c          | 21 ++++++++-
  inmates/tests/x86/mmio-access.c             | 21 ++++++++-
  4 files changed, 76 insertions(+), 14 deletions(-)

diff --git a/hypervisor/arch/x86/include/asm/processor.h 
b/hypervisor/arch/x86/include/asm/processor.h
index 70a6c3ff..d8111690 100644
--- a/hypervisor/arch/x86/include/asm/processor.h
+++ b/hypervisor/arch/x86/include/asm/processor.h
@@ -145,6 +145,7 @@
#define X86_REX_CODE 4 +#define X86_PREFIX_OP_SZ 0x66 
  #define X86_PREFIX_ADDR_SZ                            0x67
#define X86_OP_MOVZX_OPC1 0x0f 
diff --git a/hypervisor/arch/x86/mmio.c b/hypervisor/arch/x86/mmio.c
index df8c97a1..b836f43c 100644
--- a/hypervisor/arch/x86/mmio.c
+++ b/hypervisor/arch/x86/mmio.c
@@ -54,6 +54,7 @@ struct parse_context {
        bool has_rex_w;
        bool has_rex_r;
        bool has_addrsz_prefix;
+       bool has_opsz_prefix;
  };
static bool ctx_update(struct parse_context *ctx, u64 *pc, unsigned int advance, 
@@ -74,14 +75,33 @@ static bool ctx_update(struct parse_context *ctx, u64 *pc, 
unsigned int advance,
        return true;
  }
-static unsigned int get_address_width(bool has_addrsz_prefix) 
+static void parse_widths(struct parse_context *ctx,
+                        struct mmio_instruction *inst, bool parse_addr_width)
  {
        u16 cs_attr = vcpu_vendor_get_cs_attr();
-       bool long_mode = (vcpu_vendor_get_efer() & EFER_LMA) &&
-               (cs_attr & VCPU_CS_L);
+       bool cs_db = !!(cs_attr & VCPU_CS_DB);
+       bool long_mode =
+               (vcpu_vendor_get_efer() & EFER_LMA) && (cs_attr & VCPU_CS_L);
- return long_mode ? (has_addrsz_prefix ? 4 : 8) : 
-               (!!(cs_attr & VCPU_CS_DB) ^ has_addrsz_prefix) ? 4 : 2;
+       /* Op size prefix is ignored if rex.w = 1 */
+       if (ctx->has_rex_w) {
+               inst->access_size = 8;
+       } else {
+               if (long_mode)
+               /* CS.d is ignored in long mode */
+                       inst->access_size = ctx->has_opsz_prefix ? 2 : 4;
+               else
+                       inst->access_size =
+                               (cs_db ^ ctx->has_opsz_prefix) ? 4 : 2;
+       }
+
+       if (parse_addr_width) {
+               if (long_mode)
+                       inst->inst_len += ctx->has_addrsz_prefix ? 4 : 8;
+               else
+                       inst->inst_len +=
+                               (cs_db ^ ctx->has_addrsz_prefix) ? 4 : 2;
+       }
  }
struct mmio_instruction 
@@ -118,6 +138,11 @@ restart:
                        goto error_noinst;
                ctx.has_addrsz_prefix = true;
                goto restart;
+       case X86_PREFIX_OP_SZ:
+               if (!ctx_update(&ctx, &pc, 1, pg_structs))
+                       goto error_noinst;
+               ctx.has_opsz_prefix = true;
+               goto restart;
        case X86_OP_MOVZX_OPC1:
                if (!ctx_update(&ctx, &pc, 1, pg_structs))
                        goto error_noinst;
@@ -134,28 +159,26 @@ restart:
                ctx.does_write = true;
                break;
        case X86_OP_MOV_TO_MEM:
-               inst.access_size = ctx.has_rex_w ? 8 : 4;
+               parse_widths(&ctx, &inst, false);
                ctx.does_write = true;
                break;
        case X86_OP_MOVB_FROM_MEM:
                inst.access_size = 1;
                break;
        case X86_OP_MOV_FROM_MEM:
-               inst.access_size = ctx.has_rex_w ? 8 : 4;
+               parse_widths(&ctx, &inst, false);
                break;
        case X86_OP_MOV_IMMEDIATE_TO_MEM:
-               inst.access_size = ctx.has_rex_w ? 8 : 4;
+               parse_widths(&ctx, &inst, false);
                ctx.has_immediate = true;
                ctx.does_write = true;
                break;
        case X86_OP_MOV_MEM_TO_AX:
-               inst.inst_len += get_address_width(ctx.has_addrsz_prefix);
-               inst.access_size = ctx.has_rex_w ? 8 : 4;
+               parse_widths(&ctx, &inst, true);
                inst.in_reg_num = 15;
                goto final;
        case X86_OP_MOV_AX_TO_MEM:
-               inst.inst_len += get_address_width(ctx.has_addrsz_prefix);
-               inst.access_size = ctx.has_rex_w ? 8 : 4;
+               parse_widths(&ctx, &inst, true);
                inst.out_val = guest_regs->by_index[15];
                ctx.does_write = true;
                goto final;
diff --git a/inmates/tests/x86/mmio-access-32.c 
b/inmates/tests/x86/mmio-access-32.c
index be1d470f..9c1db1d8 100644
--- a/inmates/tests/x86/mmio-access-32.c
+++ b/inmates/tests/x86/mmio-access-32.c
@@ -41,6 +41,10 @@ void inmate_main(void)
        mmio_write32(mmio_reg, pattern);
        EXPECT_EQUAL(*comm_page_reg, pattern);
+ /* MOV_FROM_MEM (8b), 16-bit data, 32-bit address, OP size prefix */ 
+       asm volatile("mov (%%eax), %%ax" : "=a" (reg32) : "a" (mmio_reg));
+       EXPECT_EQUAL((u16)reg32, (u16)pattern);
+
        /* MOV_FROM_MEM (8b), 32-bit data, 32-bit address */
        asm volatile("movl (%%ebx), %%eax"
                : "=a" (reg32) : "a" (0), "b" (mmio_reg));
@@ -55,6 +59,13 @@ void inmate_main(void)
        EXPECT_EQUAL(reg32,
                     ((unsigned long)mmio_reg & ~0xffUL) | (pattern & 0xff));
+ /* MOV_FROM_MEM (8a), 8-bit data, 32-bit address, OP size prefix */ 
+       asm volatile("data16 mov (%%eax), %%al"
+               : "=a" (reg32) : "a" (mmio_reg));
+       EXPECT_EQUAL((u8)reg32, (u8)pattern);
+       EXPECT_EQUAL(reg32,
+                    ((unsigned long)mmio_reg & ~0xffUL) | (pattern & 0xff));
+
        /* MOVZXB (0f b6), 32-bit data, 32-bit address */
        asm volatile("movzxb (%%ebx), %%eax"
                : "=a" (reg32) : "a" (0), "b" (mmio_reg));
@@ -87,7 +98,15 @@ void inmate_main(void)
        /* MOV_TO_MEM (88), 8-bit data */
        asm volatile("movb %%al, (%%ebx)"
                : : "a" (0x42), "b" (mmio_reg));
-       EXPECT_EQUAL(*comm_page_reg, (pattern & 0xffffff00) | 0x42);
+       EXPECT_EQUAL(*comm_page_reg, (pattern & ~0xffUL) | 0x42);
+
+       /* MOV_TO_MEM (88), 8-bit data, OP size prefix */
+       asm volatile("data16 mov %%al, (%%ebx)" : : "a" (0x23), "b" (mmio_reg));
+       EXPECT_EQUAL(*comm_page_reg, (pattern & ~0xffUL) | 0x23);
+
+       /* MOV_TO_MEM (89), 16-bit data, OP size prefix */
+       asm volatile("mov %%ax, (%%ebx)" : : "a" (0x2342), "b" (mmio_reg));
+       EXPECT_EQUAL(*comm_page_reg, (pattern & ~0xffffUL) | 0x2342);
/* IMMEDIATE_TO_MEM (c7), 32-bit data, mod=0, reg=0, rm=3 */ 
        asm volatile("movl %0, (%%ebx)"
diff --git a/inmates/tests/x86/mmio-access.c b/inmates/tests/x86/mmio-access.c
index a9d2fcaf..3794555f 100644
--- a/inmates/tests/x86/mmio-access.c
+++ b/inmates/tests/x86/mmio-access.c
@@ -51,6 +51,10 @@ void inmate_main(void)
        mmio_write64(mmio_reg, pattern);
        EXPECT_EQUAL(*comm_page_reg, pattern);
+ /* MOV_FROM_MEM (8b), 16-bit data, Ox66 OP size prefix */ 
+       asm volatile("mov (%%rax), %%ax" : "=a" (reg64) : "a" (mmio_reg));
+       EXPECT_EQUAL((u16)reg64, (u16)pattern);
+
        /* MOV_FROM_MEM (8b), 64-bit data, mod=0, reg=0, rm=3 */
        asm volatile("movq (%%rbx), %%rax"
                : "=a" (reg64) : "a" (0), "b" (mmio_reg));
@@ -75,6 +79,13 @@ void inmate_main(void)
        EXPECT_EQUAL(reg64,
                     ((unsigned long)mmio_reg & ~0xffUL) | (pattern & 0xff));
+ /* MOV_FROM_MEM (8a), 8-bit data */ 
+       asm volatile("data16 mov (%%rax), %%al"
The comment should probably clarify that data16 has to be ignored in 64-bit mode, right? 


+               : "=a" (reg64) : "a" (mmio_reg));
+       EXPECT_EQUAL((u8)reg64, (u8)pattern);
+       EXPECT_EQUAL(reg64,
+                    ((unsigned long)mmio_reg & ~0xffUL) | (pattern & 0xff));
+
        /* MOVZXB (0f b6), to 64-bit, mod=0, reg=0, rm=3 */
        asm volatile("movzxb (%%rbx), %%rax"
                : "=a" (reg64) : "a" (0), "b" (mmio_reg));
@@ -129,7 +140,15 @@ void inmate_main(void)
        /* MOV_TO_MEM (88), 8-bit data */
        asm volatile("movb %%al, (%%rbx)"
                : : "a" (0x42), "b" (mmio_reg));
-       EXPECT_EQUAL(*comm_page_reg, (pattern & 0xffffffffffffff00) | 0x42);
+       EXPECT_EQUAL(*comm_page_reg, (pattern & ~0xffUL) | 0x42);
+
+       /* MOV_TO_MEM (88), 8-bit data, OP size prefix */
+       asm volatile("data16 mov %%al, (%%ebx)" : : "a" (0x23), "b" (mmio_reg));
+       EXPECT_EQUAL(*comm_page_reg, (pattern & ~0xffUL) | 0x23);
+
+       /* MOV_TO_MEM (89), 16-bit data, OP size prefix */
+       asm volatile("mov %%ax, (%%ebx)" : : "a" (0x2342), "b" (mmio_reg));
+       EXPECT_EQUAL(*comm_page_reg, (pattern & ~0xffffUL) | 0x2342);
/* IMMEDIATE_TO_MEM (c7), 64-bit data, mod=0, reg=0, rm=3 */ 
        asm volatile("movq %0, (%%rbx)"



Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

--
You received this message because you are subscribed to the Google Groups 
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To view this discussion on the web visit 
https://groups.google.com/d/msgid/jailhouse-dev/6294be2a-0ce1-1021-1691-c801456b36a3%40siemens.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to