Re: [Xen-devel] [PATCH 3/8] x86emul: support BMI1 insns

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/01/17 11:19, Jan Beulich wrote:
>>>> On 13.01.17 at 18:40, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 13/01/17 15:31, Jan Beulich wrote:
>>> @@ -5866,6 +5879,67 @@ x86_emulate(
>>>          break;
>>>  #endif
>>>  
>>> +    case X86EMUL_OPC_VEX(0x0f38, 0xf2):    /* andn r/m,r,r */
>>> +    case X86EMUL_OPC_VEX(0x0f38, 0xf7):    /* bextr r,r/m,r */
>>> +    {
>>> +        uint8_t *buf = get_stub(stub);
>>> +        typeof(vex) *pvex = container_of(buf + 1, typeof(vex), raw[0]);
>>> +
>>> +        host_and_vcpu_must_have(bmi1);
>>> +        generate_exception_if(vex.l, EXC_UD);
>> The manual also states #UD if VEX.W is set.
> This is very clearly a doc error: For one, is doesn't _also_ state this,
> but says nothing about VEX.L. And the instruction encodings list .W1
> variants (as expected) to encode 64-bit operations.

VEX.L != 0 is called out, but only in the text, not the exception list.

The exact text is:

"This instruction is not supported in real mode and virtual-8086 mode.
The operand size is always 32 bits if not in 64-bit mode. In 64-bit mode
operand size 64 requires VEX.W1. VEX.W1 is ignored in non-64-bit modes.
An attempt to execute this instruction with VEX.L not equal to 0 will
cause #UD."

with:

"#UD If VEX.W = 1"

in the exception list.

I am confused about the references to VEX.W1 in the text, because it
doesn't match any described VEX fields.  At a guess, I'd say it should
be referring to VEX.B which control operand size, while VEX.W is an
opcode extention.

>
>>> --- a/xen/include/asm-x86/cpufeature.h
>>> +++ b/xen/include/asm-x86/cpufeature.h
>>> @@ -57,6 +57,7 @@
>>>  #define cpu_has_xsave           boot_cpu_has(X86_FEATURE_XSAVE)
>>>  #define cpu_has_avx             boot_cpu_has(X86_FEATURE_AVX)
>>>  #define cpu_has_lwp             boot_cpu_has(X86_FEATURE_LWP)
>>> +#define cpu_has_bmi1            boot_cpu_has(X86_FEATURE_BMI1)
>>>  #define cpu_has_mpx             boot_cpu_has(X86_FEATURE_MPX)
>>>  #define cpu_has_arch_perfmon    boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
>>>  #define cpu_has_rdtscp          boot_cpu_has(X86_FEATURE_RDTSCP)
>> After trying this out, we clearly need to alter the position on VEX
>> prefixes.  VEX encoded GPR instructions don't fall within the previous
>> assumptions made about the dependences of VEX instructions.
> Should I fold this in, or do you want to submit it as a separate
> patch?

I will submit a separate patch.  I don't think it changes any of the
currently-dependent content.

~Andrew

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