We trap certain MMIO accesses and need to emulate their access. On x86, a 32-bit read will clear bits 32-63 of a register.
Inconsistently, on x86, 16-bit and 8-bit reads must not clear high bits. Jailhouse erroneously cleared those bits. Prevent this by applying a preserved mask that keeps bits alive. Add tests that check correct behaviour. Signed-off-by: Ralf Ramsauer <[email protected]> --- This patch is not related to "x86: mmio: add support for 0x66 operand prefix", it's just a bug that I stumbled over while working on a v2 of the other patch. The calculation of the preserve mask dynamically creates a bitmask. At the moment, only 8 bit support would be required, but it already supports 16 bit support, as that'll be introduced in the 0x66 operand prefix override support patch. hypervisor/arch/x86/include/asm/mmio.h | 5 +++++ hypervisor/arch/x86/mmio.c | 7 ++++++- hypervisor/arch/x86/vcpu.c | 7 +++++-- inmates/tests/x86/mmio-access-32.c | 10 +++++++--- inmates/tests/x86/mmio-access.c | 10 +++++++--- 5 files changed, 30 insertions(+), 9 deletions(-) diff --git a/hypervisor/arch/x86/include/asm/mmio.h b/hypervisor/arch/x86/include/asm/mmio.h index 756c84a8..e6c4e4dc 100644 --- a/hypervisor/arch/x86/include/asm/mmio.h +++ b/hypervisor/arch/x86/include/asm/mmio.h @@ -30,6 +30,11 @@ struct mmio_instruction { /** Output value, already copied either from a register or * from an immediate value */ unsigned long out_val; + /** A read must not clear the upper bits of registers, if the access + * width is smaller than 32 bit. This mask describes the bits that have + * to be preserved. + */ + unsigned long read_preserve_mask; }; /** diff --git a/hypervisor/arch/x86/mmio.c b/hypervisor/arch/x86/mmio.c index b234bd79..e8bf32bc 100644 --- a/hypervisor/arch/x86/mmio.c +++ b/hypervisor/arch/x86/mmio.c @@ -85,7 +85,8 @@ x86_mmio_parse(const struct guest_paging_structures *pg_structs, bool is_write) struct parse_context ctx = { .remaining = X86_MAX_INST_LEN, .count = 1 }; union registers *guest_regs = &this_cpu_data()->guest_regs; - struct mmio_instruction inst = { .inst_len = 0 }; + struct mmio_instruction inst = { .inst_len = 0, + .read_preserve_mask = 0, }; u64 pc = vcpu_vendor_get_rip(); unsigned int n, skip_len = 0; bool has_immediate = false; @@ -168,6 +169,10 @@ restart: op[2].raw = *ctx.inst; + if (!does_write && inst.access_size < 4) + inst.read_preserve_mask = + ~((1UL << (inst.access_size * 8)) - 1); + /* ensure that we are actually talking about mov imm,<mem> */ if (op[0].raw == X86_OP_MOV_IMMEDIATE_TO_MEM && op[2].modrm.reg != 0) goto error_unsupported; diff --git a/hypervisor/arch/x86/vcpu.c b/hypervisor/arch/x86/vcpu.c index 5a557d0b..3f6a13f1 100644 --- a/hypervisor/arch/x86/vcpu.c +++ b/hypervisor/arch/x86/vcpu.c @@ -231,6 +231,7 @@ bool vcpu_handle_mmio_access(void) struct mmio_access mmio = {.size = 0}; struct vcpu_mmio_intercept intercept; struct mmio_instruction inst; + unsigned long *value; vcpu_vendor_get_mmio_intercept(&intercept); @@ -249,8 +250,10 @@ bool vcpu_handle_mmio_access(void) result = mmio_handle_access(&mmio); if (result == MMIO_HANDLED) { - if (!mmio.is_write) - guest_regs->by_index[inst.in_reg_num] = mmio.value; + if (!mmio.is_write) { + value = &guest_regs->by_index[inst.in_reg_num]; + *value = (*value & inst.read_preserve_mask) | mmio.value; + } vcpu_skip_emulated_instruction(inst.inst_len); return true; } diff --git a/inmates/tests/x86/mmio-access-32.c b/inmates/tests/x86/mmio-access-32.c index 2f47f211..be1d470f 100644 --- a/inmates/tests/x86/mmio-access-32.c +++ b/inmates/tests/x86/mmio-access-32.c @@ -47,9 +47,13 @@ void inmate_main(void) EXPECT_EQUAL(reg32, pattern); /* MOV_FROM_MEM (8a), 8-bit data */ - asm volatile("movb (%%ebx), %%al" - : "=a" (reg32) : "a" (0), "b" (mmio_reg)); - EXPECT_EQUAL(reg32, (u8)pattern); + asm volatile("movb (%%eax), %%al" + : "=a" (reg32) : "a" (mmio_reg)); + EXPECT_EQUAL((u8)reg32, (u8)pattern); + /* %al should contain 0x44, while higher bits still hold the rest of + * mmio_reg. Test this. */ + EXPECT_EQUAL(reg32, + ((unsigned long)mmio_reg & ~0xffUL) | (pattern & 0xff)); /* MOVZXB (0f b6), 32-bit data, 32-bit address */ asm volatile("movzxb (%%ebx), %%eax" diff --git a/inmates/tests/x86/mmio-access.c b/inmates/tests/x86/mmio-access.c index 0e6a56b1..a9d2fcaf 100644 --- a/inmates/tests/x86/mmio-access.c +++ b/inmates/tests/x86/mmio-access.c @@ -67,9 +67,13 @@ void inmate_main(void) EXPECT_EQUAL(reg64, (u32)pattern); /* MOV_FROM_MEM (8a), 8-bit data */ - asm volatile("movb (%%rbx), %%al" - : "=a" (reg64) : "a" (0), "b" (mmio_reg)); - EXPECT_EQUAL(reg64, (u8)pattern); + asm volatile("movb (%%rax), %%al" + : "=a" (reg64) : "a" (mmio_reg)); + EXPECT_EQUAL((u8)reg64, (u8)pattern); + /* %al should contain 0x88, while high bits should still hold the rest + * of mmio_reg */ + 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" -- 2.21.0 -- 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/20190605163946.25316-1-ralf.ramsauer%40oth-regensburg.de. For more options, visit https://groups.google.com/d/optout.
