Re: [Xen-devel] [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands
>>> On 26.10.17 at 19:03, 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?
> @@ -1801,7 +1803,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
> gpa = nvcpu->nv_vvmcxaddr;
>
> rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
> - decode.op[0].len, 0, &pfinfo);
> + decode.op[0].bytes, 0, &pfinfo);
Just like for vmptrld the operand size is uniformly 64 bits here.
> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
> {
> case INVEPT_SINGLE_CONTEXT:
> {
> -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.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands
On 28/11/17 08:32, Jan Beulich wrote:
On 26.10.17 at 19:03, 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.
>
>> @@ -1801,7 +1803,7 @@ int nvmx_handle_vmptrst(struct cpu_user_regs *regs)
>> gpa = nvcpu->nv_vvmcxaddr;
>>
>> rc = hvm_copy_to_guest_linear(decode.op[0].mem, &gpa,
>> - decode.op[0].len, 0, &pfinfo);
>> + decode.op[0].bytes, 0, &pfinfo);
> Just like for vmptrld the operand size is uniformly 64 bits here.
>
>> @@ -1987,9 +1989,9 @@ int nvmx_handle_invept(struct cpu_user_regs *regs)
>> {
>> case INVEPT_SINGLE_CONTEXT:
>> {
>> -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.
It makes sense from a performance point of view. No point having
microcode make unnecessary memory accesses.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 7/9] x86/vvmx: Use correct sizes when reading operands
>>> On 01.12.17 at 19:45, wrote:
> On 28/11/17 08:32, Jan Beulich wrote:
> On 26.10.17 at 19:03, 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)
THEN #UD;
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)
>>> {
>>> case INVEPT_SINGLE_CONTEXT:
>>> {
>>> -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).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
