On 6/7/19 12:58 PM, Jan Kiszka wrote:
> On 07.06.19 12:24, Ralf Ramsauer wrote:
>> On 6/7/19 10:35 AM, Jan Kiszka wrote:
>>> On 07.06.19 00:44, Ralf Ramsauer wrote:
>>>> 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]>
>>>> ---
>>>> hypervisor/arch/x86/include/asm/mmio.h | 5 +++++
>>>> hypervisor/arch/x86/mmio.c | 5 ++++-
>>>> hypervisor/arch/x86/vcpu.c | 7 +++++--
>>>> inmates/tests/x86/mmio-access-32.c | 10 +++++++---
>>>> inmates/tests/x86/mmio-access.c | 10 +++++++---
>>>> 5 files changed, 28 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hypervisor/arch/x86/include/asm/mmio.h
>>>> b/hypervisor/arch/x86/include/asm/mmio.h
>>>> index 756c84a8..4b3b2ea8 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 reg_preserve_mask;
>>>> };
>>>> /**
>>>> diff --git a/hypervisor/arch/x86/mmio.c b/hypervisor/arch/x86/mmio.c
>>>> index b234bd79..c04cf449 100644
>>>> --- a/hypervisor/arch/x86/mmio.c
>>>> +++ b/hypervisor/arch/x86/mmio.c
>>>> @@ -85,7 +85,7 @@ 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 = { 0 };
>>>> u64 pc = vcpu_vendor_get_rip();
>>>> unsigned int n, skip_len = 0;
>>>> bool has_immediate = false;
>>>> @@ -168,6 +168,9 @@ restart:
>>>> op[2].raw = *ctx.inst;
>>>> + if (!does_write && inst.access_size < 4)
>>>> + inst.reg_preserve_mask = ~BYTE_MASK(inst.access_size);
>>>> +
>>>> /* 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..a1fb8660 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 *reg;
>>>> 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) {
>>>> + reg= &guest_regs->by_index[inst.in_reg_num];
>>>> + *reg = (*reg & inst.reg_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);
>>>
>>> Hmm, that test looks redundant to the following one. Same for other
>>> double-checks in this patch and patch 3. Or am I missing something?
>>
>> If the implementation of the simulator is correct, then those tests are
>> redundant.
>>
>> Think of cases where the first check passes, and the second check fails.
>> Was helpful during development.
>
> Well, don't we print both values when the test fails?
Yes, we do, and yes, it's redundant. If this is the question, let's
simply remove those redundant checks. The second check covers both
cases. I just wanted to argue why they are still here -- it was a
guidance for debugging.
Ralf
>
> 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/a99d8c19-d19f-80b5-3faa-40d548960460%40oth-regensburg.de.
For more options, visit https://groups.google.com/d/optout.
