>>> On 07.02.17 at 11:39, <andrew.coop...@citrix.com> wrote: > On 07/02/17 10:19, Jan Beulich wrote: >> >>> On 06.02.17 at 17:55, <andrew.coop...@citrix.com> wrote: >>> * Latch current once at the start. >>> * Avoid the memory operand read for INVEPT_ALL_CONTEXT. Experimentally, >>> this >>> is how hardware behaves, and avoids an unnecessary pagewalk. >> Hmm, so you say #GP is being raised for all possible reasons, but >> #PF can't result here? That would be pretty unusual instruction >> semantics. But if it's that way (to be confirmed by Intel), and ... > > No. The memory operand is entirely ignored. No #PF, or #GP or #SS for > bad segment or canonical setups.
But then the #GP related checks in decode_vmx_inst() would also need skipping. >>> * Reject Reg/Reg encodings of the instruction. >> ... if this is possible to occur at all (I'd expect #UD to result instead >> of a VM exit), then ... > > I'd hope so as well. This addition is partly from an entirely emulation > point of view, as well as a proper sanity check of the decode.mem union > before use. The "entirely emulation point of view" is not realy applicable here, as we don't decode the instruction a 2nd time (after the hardware had done so). Sanity checking hardware produced values of course is reasonable in places like this; I'm just not sure whether reporting such issues as #UD to the guest is appropriate - I'd rather see the guest killed if hardware doesn't behave itself. >>> --- a/xen/arch/x86/hvm/vmx/vvmx.c >>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c >>> @@ -1770,33 +1770,64 @@ int nvmx_handle_vmwrite(struct cpu_user_regs *regs) >>> >>> int nvmx_handle_invept(struct cpu_user_regs *regs) >>> { >>> + struct vcpu *curr = current; >>> + struct domain *currd = curr->domain; >>> struct vmx_inst_decoded decode; >>> - unsigned long eptp; >>> int ret; >>> >>> - if ( (ret = decode_vmx_inst(regs, &decode, &eptp, 0)) != X86EMUL_OKAY ) >>> + if ( (ret = decode_vmx_inst(regs, &decode, NULL, 0)) != X86EMUL_OKAY ) >>> return ret; >>> >>> + /* TODO - reject instruction completely if not configured. */ >>> + >>> + /* Instruction must have a memory operand. */ >>> + if ( decode.type != VMX_INST_MEMREG_TYPE_MEMORY ) >>> + { >>> + hvm_inject_hw_exception(TRAP_invalid_op, 0); >>> + return X86EMUL_EXCEPTION; >>> + } >>> + >>> switch ( reg_read(regs, decode.reg2) ) >>> { >>> case INVEPT_SINGLE_CONTEXT: >>> { >>> - struct p2m_domain *p2m = p2m_get_nestedp2m(current, eptp); >>> + struct p2m_domain *p2m; >>> + pagefault_info_t pfinfo; >>> + unsigned long eptp; >>> + >>> + /* TODO - reject SINGLE_CONTEXT if not configured. */ >>> + >>> + ret = hvm_copy_from_guest_linear(&eptp, decode.mem, 8, 0, &pfinfo); >> Please use sizeof(eptp) here. > > Ok, but in that case eptp needs to become uint64_t to match the ABI. In > fact, I probably need to read all 128 bytes and perform the MBZ check on > the 2nd word. Oh, indeed, the more that this may also effect eventual exceptions needing raising. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel