On 6/13/19 9:47 AM, Jan Kiszka wrote:
> On 12.06.19 21:00, Ralf Ramsauer wrote:
>> movzx is a move with zero-extend. This means, it will either move 1 or
>> 2 byte,
>> and zeroes the rest of the register. The definition of the rest of the
>> register depends on the operand size override prefix:
>>
>> - If OP SZ is not set, always zero the whole register, independent
>> of rex_w.
>> This mean all bits can be zeroed if the destination is eax or
>> rax. No need
>> to set the preserve mask
>>
>> - OP SZ is only set if ax is used. This is the only remaining case.
>>
>> The preserve mask then depends on the width of the access. In
>> case of B,
>> zero bits 8-15, and preserve 16-63. In case of W, zero nothing, but
>> preserve 16-63.
>
> I'm not sure how this explanation correlates with the test cases, nor to
> speak of the implementation. I feel like some bit is missing in comment
> or commit log.
Does the following make it clear?
--8<--
movzx is a move with zero-extend. It will either move 1 byte (0f b6) or
2 bytes (0f b7). The destination are lower 8 or 16 bits. Zero-extend
means that upper bits need to be cleared. The definition of upper bits
depends on the destination register.
We already have a preserve mask that allows us for to clear or preserve
bits when emulating the instruction. In case of movzx, the preserve mask
only depends on the width of the destination register. For the
destination register, we have the following cases:
- rax -- instruction has REX prefix 0x48 (rex_w set)
- eax -- default case, no prefix, nothing
- ax -- instruction has OP SZ override prefix 0x66
- al -- not possible, and doesn't make sense for movzx
Now, rax and eax have the same effect: Always clear all upper bits (IOW,
bits 8-63 if access_size is 1 or bits 16-63 if access_size is 2).
Solution for rax and eax is easy: Simply don't set the preserve mask at all.
The fun part is if we have the 0x66 operand override size prefix. This
means that the 'visibility' of the destination register is narrowed to
16 bit.
In case of a 1 byte move (0f b6), copy the source to bits 0-7, clear
bits 8-15 and preserve bits 16-63. access_width ensures that we only
copy 8 bit to bits 0-7, and the preserve_mask 0xffff.ffff.ffff.0000 does
the rest: preserve bits 16-63 and implicitely clear bits 8-15.
In case of a 2 byte move (0f b7), copy the source to bits 0-15, clear
nothing and preserve bits 16-63. access_width ensures that we only copy
16 bit to bits 0-15, and the preserve_mask 0xffff.ffff.ffff.0000 does
the rest: preserve bits 16-63.
(Ok, I already see my mistake from below)
>
>>
>> Once this pattern is clear, the fix in the hypervisor is straight
>> forward.
>>
>> Amend existing and add new test cases that test correct behaviour.
>>
>> Signed-off-by: Ralf Ramsauer <[email protected]>
>> ---
>>
>> I'm starting to get frustrated with x86. I thought I catched all
>> relevant cases, but x86 provides enough complexity for a bunch of corner
>> cases...
>>
>> hypervisor/arch/x86/mmio.c | 11 ++++-
>> inmates/tests/x86/mmio-access-32.c | 24 +++++++----
>> inmates/tests/x86/mmio-access.c | 67 ++++++++++++++++++++++++------
>> 3 files changed, 80 insertions(+), 22 deletions(-)
>>
>> diff --git a/hypervisor/arch/x86/mmio.c b/hypervisor/arch/x86/mmio.c
>> index 124f9966..76d70053 100644
>> --- a/hypervisor/arch/x86/mmio.c
>> +++ b/hypervisor/arch/x86/mmio.c
>> @@ -55,6 +55,7 @@ struct parse_context {
>> bool has_rex_r;
>> bool has_addrsz_prefix;
>> bool has_opsz_prefix;
>> + bool zero_extend;
>> };
>> static bool ctx_update(struct parse_context *ctx, u64 *pc,
>> unsigned int advance,
>> @@ -144,6 +145,7 @@ restart:
>> ctx.has_opsz_prefix = true;
>> goto restart;
>> case X86_OP_MOVZX_OPC1:
>> + ctx.zero_extend = true;
>> if (!ctx_update(&ctx, &pc, 1, pg_structs))
>> goto error_noinst;
>> op[1].raw = *ctx.inst;
>> @@ -191,8 +193,13 @@ restart:
>> op[2].raw = *ctx.inst;
>> - if (!ctx.does_write && inst.access_size < 4)
>> - inst.reg_preserve_mask = ~BYTE_MASK(inst.access_size);
/* The reg_preserve_mask only needs to be set in case of reads */
>> + if (!ctx.does_write) {
/* reg_preserve mask needs to be adjusted if the operand override prefix
* is set (IOW, if access_size < 4). And if we don't have an instruction
* with zero-extend.
*/
>> + if(!ctx.zero_extend && inst.access_size < 4)
>> + inst.reg_preserve_mask = ~BYTE_MASK(inst.access_size);
/* In case of zero-extend, we only need to preserve bits, if we have the
* operand size override prefix set. In this case, preserve bits 16-63.
*
* No need to do anything if the operand override prefix is not set: we
* have to clear all unused bits, which we do automatically if
* reg_preserve_mask is zero (which is the default case)
*/
>> + else if (ctx.zero_extend && ctx.has_opsz_prefix)
>> + inst.reg_preserve_mask =
>> + ~BYTE_MASK(inst.access_size ^ 0x3);
Heck, yesterday this made perfectly sense to me. I used a handwritten
table where I tried to find the pattern.
>From my explanation above, would you agree this needs to be ~BYTE_MASK(2)?
All tests - without modifications - still pass. And guess why I didn't
see that bug: Well, we lack a test case. :-)
We lack the test case
66 0f b7 00 movzww (%rax),%ax
that would have shown the bug. Sigh...
>
> This logic deserves some comment, and I'm not sure if it is correct
> already:
>
> Access size = 1 -> BYTE_MASK(2) -> preserve bits 16-63
> Access size = 2 -> BYTE_MASK(1) -> preserve bits 8-63???
>
> Access sizes 4 or 8 are not possible?
>
>> + }
>> /* 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)
>> diff --git a/inmates/tests/x86/mmio-access-32.c
>> b/inmates/tests/x86/mmio-access-32.c
>> index db743410..756d3947 100644
>> --- a/inmates/tests/x86/mmio-access-32.c
>> +++ b/inmates/tests/x86/mmio-access-32.c
>> @@ -64,15 +64,23 @@ void inmate_main(void)
>> 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));
>> - EXPECT_EQUAL(reg32, (u8)pattern);
>> + /* MOVZXB (0f b6), 8-bit data, 32-bit address, zero extend bits
>> 8-31 */
>> + asm volatile("movzxb (%%eax), %%eax"
>> + : "=a" (reg32) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg32, pattern & 0xff);
>> - /* MOVZXW (0f b7) */
>> - asm volatile("movzxw (%%ebx), %%eax"
>> - : "=a" (reg32) : "a" (0), "b" (mmio_reg));
>
> Where does this case go? No longer differentiating?
Differentiating between what? Registers?
I need to trigger special cases. I do this by using the same register
for input and output. I can also leave some tests that work on different
registers.
>
>> - EXPECT_EQUAL(reg32, (u16)pattern);
>> + /* MOVZXB (0f b6), 8-bit data, 32-bit address, zero extend bits
>> 8-16,
>
> 8-15
Will fix all of your remarks, thanks.
>
>> + * operand size prefix */
>> + asm volatile("movzxb (%%eax), %%ax"
>> + : "=a" (reg32) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg32,
>> + ((unsigned long)mmio_reg & ~0xffff) | (pattern & 0xff));
>> +
>> + /* MOVZXW (67 0f b7), 16-bit data, 32-bit address, zero extend
>> bits 16-31,
>> + * AD SZ prefix */
>> + asm volatile("movzxw (%%eax), %%eax"
>> + : "=a" (reg32) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg32, pattern & 0xffff);
>> /* MEM_TO_AX (a1), 32-bit data, 32-bit address */
>> asm volatile("mov (0x101ff8), %%eax"
>> diff --git a/inmates/tests/x86/mmio-access.c
>> b/inmates/tests/x86/mmio-access.c
>> index a17455b0..18eab3a5 100644
>> --- a/inmates/tests/x86/mmio-access.c
>> +++ b/inmates/tests/x86/mmio-access.c
>> @@ -84,20 +84,63 @@ void inmate_main(void)
>> EXPECT_EQUAL(reg64,
>> ((unsigned long)mmio_reg & ~0xffUL) | (pattern & 0xff));
>> - /* MOVZXB (0f b6), to 64-bit, mod=0, reg=0, rm=3 */
>
> Not relevant anymore?
>
>> - asm volatile("movzxb (%%rbx), %%rax"
>> - : "=a" (reg64) : "a" (0), "b" (mmio_reg));
>> - EXPECT_EQUAL(reg64, (u8)pattern);
>> + /* MOVZXB (48 0f b6), to 64-bit, mod=0, reg=0, rm=3 */
>> + asm volatile("movzxb (%%rax), %%rax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xff);
>> - /* MOVZXB (0f b6), 32-bit data, 32-bit address */
>> - asm volatile("movzxb (%%ebx), %%eax"
>> - : "=a" (reg64) : "a" (0), "b" (mmio_reg));
>> - EXPECT_EQUAL(reg64, (u8)pattern);
>> + /* MOVZXB (0f b6), to 32-bit, clear bits 31-63 */
>
> 32-63
>
>> + asm volatile("movzxb (%%rax), %%eax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xff);
>> - /* MOVZXW (0f b7) */
>> - asm volatile("movzxw (%%rbx), %%rax"
>> - : "=a" (reg64) : "a" (0), "b" (mmio_reg));
>> - EXPECT_EQUAL(reg64, (u16)pattern);
>> + /* MOVZXB (66 0f b6), to 32-bit, clear bits 8-16, keep 17-73,
>
> 8-15, 16-63
>
>> + * operand size prefix */
>> + asm volatile("movzxb (%%rax), %%ax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64,
>> + ((unsigned long)mmio_reg & ~0xffffUL) | (pattern & 0xff));
>> +
>> + /* MOVZXB (67 0f b6), 8-bit data, clear bits 8-63, 32-bit address,
>> + * AD SZ override prefix */
>> + asm volatile("movzxb (%%eax), %%rax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xff);
>> +
>> + /* MOVZXB (67 0f b6), 8-bit data, clear bits 8-63, 32-bit address,
>> + * AD SZ override prefix */
>
> Same as above? Surely not.
>
>> + asm volatile("movzxb (%%eax), %%eax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xff);
>> +
>> + /* MOVZXB (67 0f b6), 8-bit data, clear bits 8-16, keep 17-73,
>
> 8-15, 16-63
>
>> + * 32-bit address, AD SZ override prefix, OP SZ override prefix */
>> + asm volatile("movzxb (%%eax), %%ax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64,
>> + ((unsigned long)mmio_reg & ~0xffffUL) | (pattern & 0xff));
>> +
>> + /* MOVZXW (48 0f b7), 16-bit data, clear bits 16-63, 64-bit
>> address */
>> + asm volatile("movzxw (%%rax), %%rax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xffff);
>> +
>> + /* MOVZXW (0f b7), 16-bit data, clear bits 16-63, 64-bit address */
>
> Variation in the target register size missing in description.
>
>> + asm volatile("movzxw (%%rax), %%eax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xffff);
>> +
>> + /* MOVZXW (67 48 0f b7), 16-bit data, clear bits 16-63, 32-bit
>> address,
>> + * AD SZ prefix */
>> + asm volatile("movzxw (%%eax), %%rax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xffff);
>> +
>> + /* MOVZXW (67 0f b7), 16-bit data, clear bits 16-63, 32-bit address,
>> + * AD SZ prefix */
>
> Also here.
>
>> + asm volatile("movzxw (%%eax), %%eax"
>> + : "=a" (reg64) : "a" (mmio_reg));
>> + EXPECT_EQUAL(reg64, pattern & 0xffff);
>> /* MEM_TO_AX (a1), 64-bit data, 64-bit address */
>> asm volatile("movabs (0x101ff8), %%rax"
>>
>
>
> Horrible, this huge amount of cases.
I'm sorry -- still not enough.
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/630de120-5105-75ef-17a7-67fb45500bb0%40oth-regensburg.de.
For more options, visit https://groups.google.com/d/optout.
