On 6/7/19 1:45 PM, Jan Kiszka wrote:
> On 07.06.19 13:22, Ralf Ramsauer wrote:
>>
>>
>> On 6/7/19 1:04 PM, Jan Kiszka wrote:
>>> 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?
>>
>> Right. In 32-bit mode it remains a 8-bit mov as well.
>>
>> /* MOV_FROM_MEM(8a), 8-bit data, 0x66 OP size prefix (ignored) */
>>
>>
>> (second comment below)
>>
>>
>>>
>>>> + : "=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"
>>
>> Just noticed: ebx should be rbx, and…
>>
>>>> (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));
>>
>> … same here. Doesn't make a difference, but should be aligned.
>>
>
> OK, integrated all - and decided to drop the duplicate tests. Merged to
> next.
Great, thanks for fixing up things.
Ralf
>
> Thanks,
> Jan
>
--
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/bad98443-d907-352e-f7d1-ab42281bbc9e%40oth-regensburg.de.
For more options, visit https://groups.google.com/d/optout.
