Re: [Xen-devel] [PATCH] x86/vvmx: Improvements to INVEPT instruction handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.17 at 11:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/02/17 10:19, Jan Beulich wrote:
>>  >>> On 06.02.17 at 17:55, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel