Re: [PATCH v4 04/16] xen: Add is_vmware_port_enabled

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/16/2014 08:08 AM, Slutz, Donald Christopher wrote:
> On 09/12/14 09:08, Boris Ostrovsky wrote:
>> On 09/11/2014 02:36 PM, Don Slutz wrote:
>>>    int __get_instruction_length_from_list(struct vcpu *v,
>>> -        const enum instruction_index *list, unsigned int list_count)
>>> +                                       const enum instruction_index
>>> *list,
>>> +                                       unsigned int list_count,
>>> +                                       bool_t err_rpt)
>>>    {
>>>        struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>>        unsigned int i, j, inst_len = 0;
>>> @@ -211,10 +222,13 @@ int __get_instruction_length_from_list(struct
>>> vcpu *v,
>>>        mismatch: ;
>>>        }
>>>    -    gdprintk(XENLOG_WARNING,
>>> -             "%s: Mismatch between expected and actual instruction
>>> bytes: "
>>> -             "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
>>> -    hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> +    if ( err_rpt )
>>> +    {
>>> +        gdprintk(XENLOG_WARNING,
>>> +                 "%s: Mismatch between expected and actual
>>> instruction bytes: "
>>> +                 "eip = %lx\n",  __func__, (unsigned long)vmcb->rip);
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
>>> +    }
>>>        return 0;
>>>       done:
>>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>>> index b5188e6..9e14d2a 100644
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -59,6 +59,7 @@
>>>    #include <public/sched.h>
>>>    #include <asm/hvm/vpt.h>
>>>    #include <asm/hvm/trace.h>
>>> +#include <asm/hvm/vmport.h>
>>>    #include <asm/hap.h>
>>>    #include <asm/apic.h>
>>>    #include <asm/debugger.h>
>>> @@ -2065,6 +2066,38 @@ 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;
>>> +    unsigned long inst_len;
>>> +    unsigned long inst_addr = svm_rip2pointer(v);
>>> +    int rc;
>>> +    static const enum instruction_index list[] = {
>>> +        INSTR_INL_DX, INSTR_INB_DX, INSTR_OUTL_DX, INSTR_OUTB_DX
>>> +    };
>>> +
>>> +    inst_len = __get_instruction_length_from_list(
>>> +        v, list, ARRAY_SIZE(list), 0);
>> I should have asked earlier but I don't think I understand why the
>> last argument here is 0 (and therefore why you have this last argument
>> at all).
>>
>> Because whether or not you are warning in
>> __get_instruction_length_from_list() it will still return 0. And that,
>> in turn, will cause vmport_gp_check() to return an error. And then you
>> will print another warning in VMPORT_LOG. So there is a warning anyway.
>>
> A key part that you appear to have missed is that
> __get_instruction_length_from_list() uses gdprintk(XENLOG_WARNING,...
> but VMPORT_DBG_LOG is only available in debug=y builds.  So the new
> last argument is used to control this.  Since this change includes
> enabling #GP vmexits, it is now possible for ring 3 users to generate at
> large volume of these which with gdprintk() can flood the console.

Would it be possible to decide where and whether to print the warning 
inside __get_instruction_length_from_list() as opposed to passing a new 
parameter? E.g. if vmware_port_enabled is set and list includes IN/OUT 
and possibly something else?

-boris

>
>
>> Second, since this handler appears to be handling #GP only for VMware
>> guest we should make sure that it is not executed for any other guest.
>> You do now condition intercept got #GP for such guests only but I
>> still think having a check here is worth doing. Maybe a BUG() or
>> ASSERT()?
>>
>> The same comments are applicable to VMX code, I suspect.
>>
> I will change the check in vmport_gp_check on is_vmware_port_enabled
> into an ASSERT() so both SVM and VMX will be covered.
>
>>> +
>>> +    rc = vmport_gp_check(regs, v, inst_len, inst_addr,
>>> +                         vmcb->exitinfo1, vmcb->exitinfo2);
>>> +    if ( !rc )
>>> +        __update_guest_eip(regs, inst_len);
>>> +    else
>>> +    {
>>> +        VMPORT_DBG_LOG(VMPORT_LOG_GP_UNKNOWN,
>>> +                       "gp: rc=%d ei1=0x%lx ei2=0x%lx ip=%"PRIx64
>>> +                       " (0x%lx,%ld) ax=%"PRIx64" bx=%"PRIx64"
>>> cx=%"PRIx64
>>> +                       " dx=%"PRIx64" si=%"PRIx64" di=%"PRIx64, rc,
>>> +                       (unsigned long)vmcb->exitinfo1,
>>> +                       (unsigned long)vmcb->exitinfo2, regs->rip,
>>> inst_addr,
>>> +                       inst_len, regs->rax, regs->rbx, regs->rcx,
>>> regs->rdx,
>>> +                       regs->rsi, regs->rdi);
>>> +        hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code);
>>> +    }
>>> +}
>>> +
>> .