Re: [Xen-devel] [PATCH for-4.5 v8 4/7] xen: Add vmware_port support

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 01/22/15 03:32, Jan Beulich wrote:
>>>> On 21.01.15 at 18:52, <dslutz@xxxxxxxxxxx> wrote:
>> On 01/16/15 05:09, Jan Beulich wrote:
>>>>>> On 03.10.14 at 00:40, <dslutz@xxxxxxxxxxx> wrote:
>>>> This is a new domain_create() flag, DOMCRF_vmware_port.  It is
>>>> passed to domctl as XEN_DOMCTL_CDF_vmware_port.
>>> Can you explain why a HVM param isn't suitable here?
>>>
>> The issue is that you need this flag during construct_vmcb() and
>> construct_vmcs().  While Intel has vmx_update_exception_bitmap()
>> AMD does not.  So when HVM param's are setup and/or changed there
>> currently is no way to adjust AMD's exception bitmap.
>>
>> So this is the simpler way.
> But the less desirable one from a design/consistency perspective.
> Unless other maintainers disagree, I'd like to see this changed.

Ok, but will wait some time to see if "Unless other maintainers disagree"

>>>> This is both a more complete support then in currently provided by
>>>> QEMU and/or KVM and less.  The missing part requires QEMU changes
>>>> and has been left out until the QEMU patches are accepted upstream.
>>> I vaguely recall the question having been asked before, but I can't
>>> find it to the answer to it: If qemu has support for this, why can't
>>> you build on that rather than adding everything in the hypervisor?
>>>
>> The v10 version of this patch set (which is waiting for an adjusted
>> QEMU (the released 2.2.0 is one) does use QEMU for more VMware port
>> support.  The issues are:
> Was there a newer version of these posted than the v8 I looked at?
> If so, I must have overlooked the posting (as otherwise I would of
> course have commented on the newer version).
>

The newer version was being worked on, but had not been posted (and had no
changes to this patch).  Since it was never posted, I will just continue
getting v9
(instead of v10) into shape to post.


>> 1) QEMU needs access to parts of CPU registers to handle VMware port.
>> 2) You need to allow ring 3 access to this 1 I/O port.
>> 3) There is more state in xen that would need to also be sent to
>>    QEMU if all support is moved to QEMU.
> Understood.
>
>>>> @@ -2111,6 +2112,31 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
>>>>       return;
>>>>   }
>>>>
>>>> +static void svm_vmexit_gp_intercept(struct cpu_user_regs *regs,
>>>> +                                    struct vcpu *v)
>>>> +{
>>>> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>>> +    /*
>>>> +     * Just use 15 for the instruction length; vmport_gp_check will
>>>> +     * adjust it.  This is because
>>>> +     * __get_instruction_length_from_list() has issues, and may
>>>> +     * require a double read of the instruction bytes.  At some
>>>> +     * point a new routine could be added that is based on the code
>>>> +     * in vmport_gp_check with extensions to make it more general.
>>>> +     * Since that routine is the only user of this code this can be
>>>> +     * done later.
>>>> +     */
>>>> +    unsigned long inst_len = 15;
>>> Surely this can be unsigned int?
>> The code is smaller this way.  In vmx_vmexit_gp_intercept():
>>
>>     unsigned long inst_len;
>> ...
>>     __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
>> ...
>>     rc = vmport_gp_check(regs, v, &inst_len, inst_addr,
>> ...
>>
>> So changing the argument to vmport_gp_check() to "unsigned int" would
>> add code there.
> So be it then. Generic code shouldn't use odd types just because of
> vendor specific code needs it, unless this makes things _a lot_ more
> complicated.
>

Ok.  Since It looks like I will not be using get_instruction_length() I will
change this to "unsigned int" (or should I use "unsigned short" or
"unsigned byte"?).

>>>> +    int rc = X86EMUL_OKAY;
>>>> +
>>>> +    if ( regs->_eax == BDOOR_MAGIC )
>>> With this, is handling other than 32-bit in/out really meaningful/
>>> correct?
>>>
>> Yes. Harder to use, but since VMware allows it, I allow it also.
> But then a comment explaining the non-architectural (from an
> instruction set perspective) behavior is the minimum that's
> needed for future readers (and reviewers) to understand this.

Ok will add.


>>>> +        case BDOOR_CMD_GETHWVERSION:
>>>> +            /* vmware_hw */
>>>> +            regs->_eax = 0;get_instruction_length
>>>> +            if ( is_hvm_vcpu(curr) )
>>> Since you can't get here for PV, I can't see what you need this
>>> conditional for.
>>>
>> Since I was not 100% sure, I was being safe.  Would converting
>> this to be a "debug=y" check be ok?
> ASSERT() would indeed be the right vehicle.
>

Will do.

>>>> +            {
>>>> +                struct hvm_domain *hd = &d->arch.hvm_domain;
>>>> +
>>>> +                regs->_eax = hd->params[HVM_PARAM_VMWARE_HW];
>>>> +            }
>>>> +            if ( !regs->_eax )
>>>> +                regs->_eax = 4;  /* Act like version 4 */
>>> Why version 4?
>> That is the 1st version that VMware was more consistent in the handling
>> of the "VMware hardware version".  Any value between 1 and 6 would be
>> ok.  This should only happen in strange configs.
> Please make the comment say so then.
>

Will do.

>>>> +            /* hostUsecs */
>>>> +            regs->_ebx =value / 1000000ULL;
>>>> +            /* maxTimeLag */
>>>> +            regs->_ecx = 1000000;
>>>> +            break;
>>> Perhaps this should share code with BDOOR_CMD_GETTIME; I have
>>> to admit though that I can't make any sense of why the latter one
>>> has a FULL suffix when it returns _less_ information.
>>>
>> Sharing of code is not simple.
>> Since I did not pick the names, VMware did.
>>
>> Bug found.  The full returns data in si & dx.
>> will fix. And also makes sharing more complex then not.
> Of course if the current code is incomplete, sharing makes less sense
> once completed.
>
>>>> +        unsigned char bytes[MAX_INST_LEN];
>>>> +        unsigned int fetch_len;
>>>> +        int frc;
>>>> +
>>>> +        /* in or out are limited to 32bits */
>>>> +        if ( byte_cnt > 4 )
>>>> +            byte_cnt = 4;
>>>> +
>>>> +        /*
>>>> +         * Fetch up to the next page break; we'll fetch from the
>>>> +         * next page later if we have to.
>>>> +         */
>>>> +        fetch_len = min_t(unsigned int, *inst_len,
>>>> +                          PAGE_SIZE - (inst_addr  & ~PAGE_MASK));
>>>> +        frc = hvm_fetch_from_guest_virt_nofault(bytes, inst_addr, 
>>>> fetch_len,
>>>> +                                                PFEC_page_present);
>>>> +        if ( frc != HVMCOPY_okay )
>>>> +        {
>>>> +            gdprintk(XENLOG_WARNING,
>>>> +                     "Bad instruction fetch at %#lx (frc=%d il=%lu 
>>>> fl=%u)\n",
>>>> +                     (unsigned long) inst_addr, frc, *inst_len, 
>>>> fetch_len);
>>> Pointless cast. But the value of log messages like this one is
>>> questionable anyway.
>>>
>> Will drop cast.  I am not sure it is possible to get here. The best I
>> have come up with is to change the GDT entry for CS to fault, then do
>> this instruction.  Not sure it would fault, and clearly is an attempt
>> to break in.
>>
>> I do know that if Xen is running under VMware (Why anyone would do
>> this?) this is possible.
>>
>> With all this, should I just drop this message (or make it debug=y
>> only)?
> Yes - dropping would be preferred by me, but I'd accept a debug=y
> only one too.

Ok, will drop.

>>>> +
>>>> +        /* Only adjust byte_cnt 1 time */
>>>> +        if ( bytes[0] == 0x66 )     /* operand size prefix */
>>>> +        {
>>>> +            if ( byte_cnt == 4 )
>>>> +                byte_cnt = 2;
>>>> +            else
>>>> +                byte_cnt = 4;
>>>> +        }
>>> Iirc REX.W set following 0x66 cancels the effect of the latter. Another
>>> thing x86emul would be taking care of for you if you used it.
>> I did not know this.  Most of my testing was done without any check
>> for prefix(s).  I.E. (Open) VMware Tools only uses the inl.  I do
>> not know of anybody using 16bit segments and VMware tools.
> But this isn't the perspective to take when adding code to the
> hypervisor - you should always consider what a (perhaps
> malicious) guest could do.

Ok, but my read of this statement does not help decide which way
to go.  I see several options:

1) Only allow #GP to work for 0xed (inl (%dx),%eax).
    Pros: No attack surface for malicious guest.
             No need for get_instruction_length().
             No need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    Cons: There may exist user apps. that work on VMware and
               not on xen (16bit segments, realmode, vm86, etc).

2) Only allow #GP to work for all 4 I/O instructions without prefix.
    Pros: No attack surface for malicious guest.
             No need for get_instruction_length().
             No need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    Cons: There may exist user apps. that work on VMware and
                 not on xen (16bit segments, realmode, vm86, etc).

3) Only allow zero or one 0x66 prefix and 0xed (inl (%dx),%eax).
    Pros: No attack surface for malicious guest.
             No need for get_instruction_length().
             No need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    Cons: There may exist user apps. that work on VMware and
                 not on xen (using too many prefixes, using other
                 opcodes).

4) Only allow zero or one 0x66 prefix and all 4 I/O instructions.
    Pros: No attack surface for malicious guest.
             No need for get_instruction_length().
             No need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    Cons: There may exist user apps. that work on VMware and
                 not on xen (using too many prefixes).

5) Only allow zero to 14 0x66 prefix and 0xed (inl (%dx),%eax).
    Pros: No attack surface for malicious guest.
    Cons: There may exist user apps. that work on VMware and
                not on xen (using unneeded prefixes, using other
                opcodes).
    5a:    Would be cleaner with get_instruction_length() on Intel,
                but would need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    5b:     Always pass in MAX_INST_LEN.
   

6) Only allow zero to 14 0x66 prefix and all 4 I/O instructions.
    Pros: No attack surface for malicious guest.
    Cons: There may exist user apps. that work on VMware and
               not on xen (using unneeded  prefixes).
    6a:    Would be cleaner with get_instruction_length() on Intel,
                but would need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    6b:     Always pass in MAX_INST_LEN.

7) Add complete prefix handling, and all 4 I/O instructions
    Pros: Limited attack surface for malicious guest (the handling
              of all prefixes greatly increases the complexity of the
              code).
    Cons: Lots of added code.
    7a:    Would be cleaner with get_instruction_length() on Intel,
                but would need for Intel to confirm the necessary hardware
                behaviour as being architectural.
    7b:     Always pass in MAX_INST_LEN.

8) Use hvm_emulate_one().
    Pros: shares code, reduces new code.
    Cons: Adds a lot of attack surface for malicious guest.


I had picked #6, you asked for #8, but I read your answer as do not
do #8.

I would be happy to go with any of the 8 ways (or a way I did not list
above),
just need to know which one to focus on.


>>>> +static void vmx_vmexit_gp_intercept(struct cpu_user_regs *regs,
>>>> +                                    struct vcpu *v)
>>>> +{
>>>> +    unsigned long exit_qualification;
>>>> +    unsigned long inst_len;
>>>> +    unsigned long inst_addr = vmx_rip2pointer(regs, v);
>>>> +    unsigned long ecode;
>>>> +    int rc;
>>>> +#ifndef NDEBUG
>>>> +    unsigned long orig_inst_len;
>>>> +    unsigned long vector;
>>>> +
>>>> +    __vmread(VM_EXIT_INTR_INFO, &vector);
>>>> +    BUG_ON(!(vector & INTR_INFO_VALID_MASK));
>>>> +    BUG_ON(!(vector & INTR_INFO_DELIVER_CODE_MASK));
>>>> +#endif
>>> If you use ASSERT() instead of BUG_ON(), I think you can avoid most
>>> of this preprocessor conditional.
>>>
>> I do not see how.  vector only exists in "debug=y".  So yes if using
>> ASSERT() I could move the #endif up 2 lines, but that does not
>> look better to me.
> I don't follow - ASSERT() is intentionally coded in a way such that
> variables used only by it don't cause compiler warnings. And the
> optimizer ought to be able to eliminate the then unnecessary
> __vmread().
>

I am more use to explicit conditional code and to not depend on the
compilers
to correctly optimize the code.  Will change.

Since the most likely case is that I will stop using
get_instruction_length()
(__vmread(VM_EXIT_INSTRUCTION_LEN,...)).  This drops the need for
orig_inst_len
also.

>>>> +    __vmread(EXIT_QUALIFICATION, &exit_qualification);
>>>> +    __vmread(VM_EXIT_INSTRUCTION_LEN, &inst_len);
>>> get_instruction_length(). But is it architecturally defined that
>>> #GP intercept vmexits actually set this field?
>>>
>> I could not find a clear statement.
> That's the point of my comment.
>
>>  My reading of (directly out of
>> "Intel 64 and IA-32 Architectures
>> Software Developerâs Manual
>> Volume 3 (3A, 3B & 3C):
>> [...]
>> to me says that yes, this field is set on a #GP exit on an IN.  But the
>> #GP case is not called out by name.
> And it is not any of the cases mentioned.
>
>> My read is that a #GP fault is a "VM Exits Unconditionally" based on the
>> setting of the exception bit mask.
> Right, but it's not exactly an instruction based exit. Unless Intel
> confirms that your extending of the manual says is correct, I'd
> rather recommend against relying on unspecified behavior. If
> any CPU model ends up behaving differently, this might be
> rather hard to diagnose I'm afraid.
>
>> So not using get_instruction_length() does avoid a possible BUG_ON()
>> if I am wrong.
>>
>> So there are 3 options here:
>> 1) Add an ASSERT() like the BUG_ON() in get_instruction_length()
>> 2) Switch to using get_instruction_length()
>> 3) Switch to using MAX_INST_LEN.
>>
>> Let me know which way to go.
> As said above - use get_instruction_length() if Intel confirms the
> necessary hardware behavior as being architectural. If they
> don't, 3) looks like the only viable option.


So what is the procedure to getting "Intel confirms the necessary hardware
behaviour as being architectural"?



>>>> @@ -2182,6 +2183,8 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs 
>>>> *regs,
>>>>               if ( v->fpu_dirtied )
>>>>                   nvcpu->nv_vmexit_pending = 1;
>>>>           }
>>>> +        else if ( vector == TRAP_gp_fault )
>>>> +            nvcpu->nv_vmexit_pending = 1;
>>> Doesn't that mean an unconditional vmexit even if the L1 hypervisor
>>> didn't ask for such?
>> I might.  I have not done any testing here for the nested VMX case.
>> I could just drop this for now and deside what to do for this code later.
> If dropping the code is safe without also forbidding the combination
> of nested and VMware emulation.

Will have to do a lot more testing to know.  At the time I started the
coding
it was still considered experimental.  Looks like for 4.6 I will need it
to be
fully unit tested.

    -Don Slutz


> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel