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