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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
>> @@ -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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel