Hi, just a short side note:
x86 opcode parsing is really a hell, by the way... All existing tests in mmio-access tests (of course) still pass. [1] This doesn't necessarily mean that my patch is absolutely correct. Please review carefully. get_op_width() returns the width according to this (really helpful) table: https://wiki.osdev.org/X86-64_Instruction_Encoding#Operand-size_and_address-size_override_prefix I make the assumption that if rex.w is set, we can always return 8 for performance reasons (we can save other useless checks in this case). rex.w must only be set in long mode, and then we always have 8b width, so there's no need to check for non-long mode if rex.w is set -- or should we? Could this maybe be exploited by faulty opcodes by guests running in non-long mode that inject 0x66 8b 00 e.g.? (e.g., by targeted access to page boundaries) Ralf [1] Without my SSE series, 32-bit requires -mno-sse to be tested at the moment On 6/4/19 11:02 PM, 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 | 37 +++++++++++++++++---- > inmates/tests/x86/mmio-access-32.c | 4 +++ > inmates/tests/x86/mmio-access.c | 4 +++ > 4 files changed, 40 insertions(+), 6 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 b234bd79..6d9ad1c5 100644 > --- a/hypervisor/arch/x86/mmio.c > +++ b/hypervisor/arch/x86/mmio.c > @@ -79,6 +79,26 @@ static unsigned int get_address_width(bool > has_addrsz_prefix) > (!!(cs_attr & VCPU_CS_DB) ^ has_addrsz_prefix) ? 4 : 2; > } > > +static unsigned int get_op_width(bool has_rex_w, bool has_opsz_prefix) > +{ > + u16 cs_attr; > + bool long_mode; > + > + /* Op size prefix is ignored if rex.w = 1 */ > + if (has_rex_w) > + return 8; > + > + cs_attr = vcpu_vendor_get_cs_attr(); > + long_mode = (vcpu_vendor_get_efer() & EFER_LMA) && > + (cs_attr & VCPU_CS_L); > + > + if (long_mode) > + /* CS.d is ignored in long mode */ > + return has_opsz_prefix ? 2 : 4; > + > + return (!!(cs_attr & VCPU_CS_DB) ^ has_opsz_prefix) ? 4 : 2; > +} > + > struct mmio_instruction > x86_mmio_parse(const struct guest_paging_structures *pg_structs, bool > is_write) > { > @@ -94,6 +114,7 @@ x86_mmio_parse(const struct guest_paging_structures > *pg_structs, bool is_write) > bool has_rex_w = false; > bool has_rex_r = false; > bool has_addrsz_prefix = false; > + bool has_opsz_prefix = false; > > if (!ctx_update(&ctx, &pc, 0, pg_structs)) > goto error_noinst; > @@ -114,9 +135,13 @@ restart: > } > switch (op[0].raw) { > case X86_PREFIX_ADDR_SZ: > + case X86_PREFIX_OP_SZ: > if (!ctx_update(&ctx, &pc, 1, pg_structs)) > goto error_noinst; > - has_addrsz_prefix = true; > + if (op[0].raw == X86_PREFIX_ADDR_SZ) > + has_addrsz_prefix = true; > + else > + has_opsz_prefix = true; > goto restart; > case X86_OP_MOVZX_OPC1: > if (!ctx_update(&ctx, &pc, 1, pg_structs)) > @@ -134,28 +159,28 @@ restart: > does_write = true; > break; > case X86_OP_MOV_TO_MEM: > - inst.access_size = has_rex_w ? 8 : 4; > + inst.access_size = get_op_width(has_rex_w, has_opsz_prefix); > does_write = true; > break; > case X86_OP_MOVB_FROM_MEM: > inst.access_size = 1; > break; > case X86_OP_MOV_FROM_MEM: > - inst.access_size = has_rex_w ? 8 : 4; > + inst.access_size = get_op_width(has_rex_w, has_opsz_prefix); > break; > case X86_OP_MOV_IMMEDIATE_TO_MEM: > - inst.access_size = has_rex_w ? 8 : 4; > + inst.access_size = get_op_width(has_rex_w, has_opsz_prefix); > has_immediate = true; > does_write = true; > break; > case X86_OP_MOV_MEM_TO_AX: > inst.inst_len += get_address_width(has_addrsz_prefix); > - inst.access_size = has_rex_w ? 8 : 4; > + inst.access_size = get_op_width(has_rex_w, has_opsz_prefix); > inst.in_reg_num = 15; > goto final; > case X86_OP_MOV_AX_TO_MEM: > inst.inst_len += get_address_width(has_addrsz_prefix); > - inst.access_size = has_rex_w ? 8 : 4; > + inst.access_size = get_op_width(has_rex_w, has_opsz_prefix); > inst.out_val = guest_regs->by_index[15]; > does_write = true; > goto final; > diff --git a/inmates/tests/x86/mmio-access-32.c > b/inmates/tests/x86/mmio-access-32.c > index 2f47f211..b4f83850 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(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)); > diff --git a/inmates/tests/x86/mmio-access.c b/inmates/tests/x86/mmio-access.c > index 0e6a56b1..86694353 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(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)); > -- 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/813d52b7-789f-acea-98a3-ed71e36a6022%40oth-regensburg.de. For more options, visit https://groups.google.com/d/optout.
