[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands

>>> On 01.12.17 at 19:45, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 28/11/17 08:32, Jan Beulich wrote:
>>>>> On 26.10.17 at 19:03, <euan.harris@xxxxxxxxxx> wrote:
>>> * invvpid has a 128-bit memory operand but we only require the VPID value
>>>   which lies in the lower 64 bits.
>> The memory operand (wrongly) isn't being read at all - I don't
>> understand the above bullet point for that reason.
>>> @@ -464,6 +462,8 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
>>>      unsigned long base, index, seg_base, disp, offset;
>>>      int scale, size;
>>> +    unsigned int bytes = vmx_guest_x86_mode(v);
>>> +
>> Except in extraordinary circumstances please don't add blank lines in
>> the middle of declarations.
>> Also - don't you get 2 here for 16-bit protected mode, which you'd
>> need to convert to 4?
> We can never reach this point from a 16 bit segment.  All VMX
> instructions #UD if executed outside of a 64bit (in long mode) or 32bit
> (outside of long mode) segment.

That's certainly not what the insn pages say: The common conditional
used is

IF (not in VMX operation) or (CR0.PE = 0) or (RFLAGS.VM = 1) or (IA32_EFER.LMA 
= 1 and CS.L = 0)

which excludes real, VM86, and compat modes, but not 16-bit
protected mode. Of course there's the option of hardware
behaving like you say and the manual just being wrong.

>>> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
>>>      {
>>>      {
>>> -        unsigned long eptp;
>>> +        uint64_t eptp;
>>> -        ret = operand_read(&eptp, &decode.op[0], regs, decode.op[0].len);
>>> +        ret = operand_read(&eptp, &decode.op[0], regs, sizeof(eptp));
>> I think you should read the full 128 bits for correct faulting behavior
>> (also for invvpid). I also can't derive from the SDM that this reading
>> won't occur for the INVEPT_ALL_CONTEXT case. Sadly the SDM
>> doesn't say whether the reserved upper half is enforced to be zero
>> (which we would then need to emulate), or whether it is being
>> ignored. For invvpid however it does state that bits 16:63 are being
>> checked.
> Observations on real hardware to the contrary.  Each of the generations
> I've tried never read the operand, or the part of the operand that they
> don't need.

Correct faulting behavior implies more than whether the actual read
occurs: A quick test confirms that the ept_sync_all() used during
VMX enabling ends in #GP when the memory address of INVEPT is
being changed to a non-canonical one, and in #PF if changed to a
suitable canonical one (on a Westmere system).


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.