Re: [Xen-devel] [PATCH] x86/emul: Corrections to cmpxchg{8, 16}b emulation (to fix 32bit PV guests)

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.01.17 at 09:52, <andrew.cooper3@xxxxxxxxxx> wrote:

Commenting on just the parts not replaced by the other patch.

> @@ -461,6 +484,53 @@ int main(int argc, char **argv)
>          goto fail;
>      printf("okay\n");
>  
> +#ifdef __x86_64__
> +    memset(&state, 0, sizeof(state));
> +    printf("%-40s", "Testing lock cmpxchg16b (%rdi) [succeeding]...");
> +    instr[0] = 0xf0; instr[1] = 0x48; instr[2] = 0x0f; instr[3] = 0xc7; 
> instr[4] = 0x0f;
> +    res[0]      = 0x9abcdef0;
> +    res[1]      = 0x12345678;
> +    res[2]      = 0x87654321;
> +    res[3]      = 0x12345678;
> +    regs.rax    = 0x123456789abcdef0ull;
> +    regs.rdx    = 0x1234567887654321ull;
> +    regs.rflags = 0x200;
> +    regs.rcx    = 0xCCCCCCCCAAAAAAAAull;
> +    regs.rbx    = 0x99999999FFFFFFFFull;
> +    regs.rip    = (unsigned long)&instr[0];
> +    regs.rdi    = (unsigned long)res;
> +    rc = x86_emulate(&ctxt, &emulops);
> +    if ( (rc != X86EMUL_OKAY) ||
> +         (res[0] != 0xFFFFFFFF) ||
> +         (res[1] != 0x99999999) ||
> +         (res[2] != 0xAAAAAAAA) ||
> +         (res[3] != 0xCCCCCCCC) ||
> +         (state.reads != 1) ||
> +         (state.writes != 0) ||
> +         (state.cmpxchgs != 1) ||
> +         ((regs.rflags&0x240) != 0x240) ||

Please widen the mask - the other arithmetic flags should remain
untouched.

> +         (regs.rip != (unsigned long)&instr[5]) )
> +        goto fail;
> +    printf("okay\n");
> +
> +    memset(&state, 0, sizeof(state));
> +    printf("%-40s", "Testing lock cmpxchg16b (%rdi) [misaligned]...");
> +    instr[0] = 0xf0; instr[1] = 0x48; instr[2] = 0x0f; instr[3] = 0xc7; 
> instr[4] = 0x0f;

I'd rather omit the lock prefix at least here. Even for the other case
it's probably better to omit it, because (other than for other opcodes)
there should be no call to ->write() regardless of lock prefix.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1045,6 +1045,7 @@ static int read_ulong(
>          const struct x86_emulate_ops *ops)
>  {
>      *val = 0;
> +    ASSERT(bytes <= sizeof(*val));
>      return ops->read(seg, offset, val, bytes, ctxt);
>  }

This should also fine to be kept.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel