Re: [RFC PATCH 08/10] connect vmport up

[Don Slutz <dslutz@verizon.com>]

On 12/13/13 10:46, Boris Ostrovsky wrote:
> On 12/12/2013 02:15 PM, Don Slutz wrote:
>> From: Don Slutz <dslutz@verizon.com>
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>   xen/arch/x86/hvm/io.c       |   4 ++
>>   xen/arch/x86/hvm/svm/svm.c  | 104 ++++++++++++++++++++++++++++++++++++
>>   xen/arch/x86/hvm/svm/vmcb.c |   1 +
>>   xen/arch/x86/hvm/vmx/vmcs.c |   1 +
>>   xen/arch/x86/hvm/vmx/vmx.c  | 125 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   xen/arch/x86/hvm/vmx/vvmx.c |  13 +++++
>>   xen/include/public/trace.h  |   1 +
>>   7 files changed, 249 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
>> index bf6309d..4bc4716 100644
>> --- a/xen/arch/x86/hvm/io.c
>> +++ b/xen/arch/x86/hvm/io.c
>> @@ -42,6 +42,7 @@
>>   #include <asm/hvm/vlapic.h>
>>   #include <asm/hvm/trace.h>
>>   #include <asm/hvm/emulate.h>
>> +#include <asm/hvm/vmport.h>
>>   #include <public/sched.h>
>>   #include <xen/iocap.h>
>>   #include <public/hvm/ioreq.h>
>> @@ -236,6 +237,9 @@ int handle_pio(uint16_t port, unsigned int size, 
>> int dir)
>>       if ( dir == IOREQ_WRITE )
>>           data = guest_cpu_user_regs()->eax;
>>   +    if ( port == VMPORT_PORT )
>> +        return vmport_ioport(dir, size, data, guest_cpu_user_regs());
>> +
>>       rc = hvmemul_do_pio(port, &reps, size, 0, dir, 0, &data);
>>         switch ( rc )
>> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
>> index 406d394..80cf2bf 100644
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -56,6 +56,7 @@
>>   #include <asm/hvm/svm/nestedsvm.h>
>>   #include <asm/hvm/nestedhvm.h>
>>   #include <asm/x86_emulate.h>
>> +#include <asm/hvm/vmport.h>
>>   #include <public/sched.h>
>>   #include <asm/hvm/vpt.h>
>>   #include <asm/hvm/trace.h>
>> @@ -1904,6 +1905,105 @@ svm_vmexit_do_vmsave(struct vmcb_struct *vmcb,
>>       return;
>>   }
>>   +static void svm_vmexit_gp_intercept(struct cpu_user_regs *regs, 
>> struct vcpu *v)
>> +{
>> +    struct hvm_domain *hd = &v->domain->arch.hvm_domain;
>> +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>> +    unsigned long inst_len, bytes_len;
>> +    int frc;
>> +    unsigned char bytes[15];
>> +
>> +    regs->error_code = vmcb->exitinfo1;
>> +    if ( !cpu_has_svm_nrips || (vmcb->nextrip <= vmcb->rip) )
>> +        inst_len = 0;
>> +    else
>> +        inst_len = vmcb->nextrip - vmcb->rip;
>
> You can use svm_nextrip_insn_length(), with some adjustments to NDEBUG 
> case there.

Will check it out.

>
>> +    bytes_len = 2 /* inst_len < 15 ? inst_len > 1 ? inst_len : 2 : 
>> 15 */;
>
> Saying this in words would be preferable --- I am not sure I 
> understand why it's 2. Do you only expect specific instructions here? 
> Or are you only interested in the first two bytes of the opcode?
>

I only expect specific instructions here that are all 1 or 2 bytes. It 
turns out the 2 byte ones should have been a later patch.   Will fix.

The 1 byte instruction "0xed" is either "in %ax,%dx" or "in %eax,%dx" 
depending on mode guest is in.  I just found a web page that states that 
"out %eax,%dx" should also work.  Need to test out that VMware does 
support this.

>> +    frc = hvm_fetch_from_guest_virt_nofault(bytes, regs->eip,
>> +                                            bytes_len,
>> +                                            PFEC_page_present);
>> +
>> +    if ( hvm_long_mode_enabled(v) )
>> +        HVMTRACE_LONG_4D(TRAP, TRAP_gp_fault, inst_len,
>> +                         regs->error_code,
>> +                         TRC_PAR_LONG(vmcb->exitinfo2) );
>> +    else
>> +        HVMTRACE_4D(TRAP, TRAP_gp_fault, inst_len,
>> +                    regs->error_code, vmcb->exitinfo2 );
>> +
>> +    if (hd->params[HVM_PARAM_VMPORT_LOGMASK] & 0x400000 /* 
>> LOG_GP_FAIL_RD_INST */)
>> +        printk("[HVM:%d.%d] <%s> "
>> +               "gp: e2=%lx ec=%lx ip=%lx=>0x%x 0x%x(%ld,%ld,%d) 
>> nip(%d)=%lx(%d,%d(0x%x) 0x%x 0x%x)"
>> +               "\n",
>> +               current->domain->domain_id, current->vcpu_id, __func__,
>> +               (unsigned long)vmcb->exitinfo2,
>> +               (unsigned long)regs->error_code,
>> +               (unsigned long)regs->eip, (unsigned int)bytes[0],
>> +               (unsigned int)bytes[1], bytes_len, inst_len, frc,
>> +               cpu_has_svm_nrips, (unsigned long)vmcb->nextrip,
>> +               cpu_has_svm_decode, vmcb->guest_ins_len & 0xf, 
>> vmcb->guest_ins_len,
>> +               vmcb->guest_ins[0], vmcb->guest_ins[1]);
>> +
>> +    if ( !frc && bytes[0] == 0xed && (regs->edx & 0xffff) == 
>> VMPORT_PORT &&
>> +         vmcb->exitinfo2 == 0 && regs->error_code == 0 )
>> +    {
>> +        /*  in (%dx),%eax */
>> +        uint32_t magic = regs->eax;
>> +
>> +        if ( magic == VMPORT_MAGIC ) {
>> +            __update_guest_eip(regs, 1);
>> +            vmport_ioport(IOREQ_READ, 4, 0, regs);
>> +            if (hd->params[HVM_PARAM_VMPORT_LOGMASK] & 0x800000 /* 
>> LOG_GP_VMWARE_AFTER */)
>> +                printk("[HVM:%d.%d] <%s> "
>> +                       "gp: VMware ip=%lx ax=%lx bx=%lx cx=%lx 
>> dx=%lx si=%lx di=%lx"
>> +                       "\n",
>> +                       current->domain->domain_id, current->vcpu_id, 
>> __func__,
>> +                       (unsigned long)regs->eip,
>> +                       (unsigned long)regs->eax, (unsigned 
>> long)regs->ebx,
>> +                       (unsigned long)regs->ecx, (unsigned 
>> long)regs->edx,
>> +                       (unsigned long)regs->esi, (unsigned 
>> long)regs->edi);
>> +            return;
>> +        } else {
>> +            if (hd->params[HVM_PARAM_VMPORT_LOGMASK] & 0x200000 /* 
>> LOG_GP_NOT_VMWARE */)
>> +                printk("[HVM:%d.%d] <%s> "
>> +                       "gp: ip=%lx ax=%lx bx=%lx cx=%lx dx=%lx 
>> si=%lx di=%lx"
>> +                       "\n",
>> +                       current->domain->domain_id, current->vcpu_id, 
>> __func__,
>> +                       (unsigned long)regs->eip,
>> +                       (unsigned long)regs->eax, (unsigned 
>> long)regs->ebx,
>> +                       (unsigned long)regs->ecx, (unsigned 
>> long)regs->edx,
>> +                       (unsigned long)regs->esi, (unsigned 
>> long)regs->edi);
>> +            hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code);
>> +        }
>> +    } else if (!frc && regs->error_code == 0
>> +               && bytes[0] == 0x0f && bytes[1] == 0x33 && regs->ecx 
>> == 0x10000)
>> +    {
>> +        /* "rdpmc 0x10000" */
>> +        /* Not a very good emulation!  But just not faulting is good 
>> enough
>> +         * to get NetApp booting. */
>> +        regs->edx = regs->eax = 0;
>> +
>> +        __update_guest_eip(regs, inst_len);
>
> What if inst_len is zero? (e.g. if NRIP is not supported?)

Needs to be handled (but this line is part of the lines that are planned 
to be dropped from this patch).


>
>
>
> -boris
>

    -Don Slutz

>> +
>> +        /* Doing the log in this case was too noisy for NetApp, so I 
>> moved
>> +         * it to 'else' */
>> +    } else {
>> +        if (hd->params[HVM_PARAM_VMPORT_LOGMASK] & 0x100000 /* 
>> LOG_GP_UNKNOWN */) {
>> +            printk("[HVM:%d.%d] <%s> "
>> +                   "gp: e2=%lx ec=%lx ip=%lx=>0x%x 0x%x(%ld,%d) 
>> ax=%lx bx=%lx cx=%lx dx=%lx si=%lx di=%lx"
>> +                   "\n",
>> +                   current->domain->domain_id, current->vcpu_id, 
>> __func__,
>> +                   (unsigned long)vmcb->exitinfo2, (unsigned 
>> long)regs->error_code,
>> +                   (unsigned long)regs->eip, (unsigned int)bytes[0],
>> +                   (unsigned int)bytes[1], inst_len, frc,
>> +                   (unsigned long)regs->eax, (unsigned long)regs->ebx,
>> +                   (unsigned long)regs->ecx, (unsigned long)regs->edx,
>> +                   (unsigned long)regs->esi, (unsigned long)regs->edi);
>> +        }
>> +        hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code);
>> +    }
>> +}
>> +
>>
>