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

[Don Slutz <dslutz@verizon.com>]

On 12/12/13 19:51, Andrew Cooper wrote:
> On 12/12/2013 19:15, 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());
>> +
> Use register_portio_handler(), which is the already-existing
> infrastructure for intercepting ports.

Will try it out.

>
>>       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;
>> +    bytes_len = 2 /* inst_len < 15 ? inst_len > 1 ? inst_len : 2 : 15 */;
>> +    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);
> This appears to be intercepting an L2 guest doing vmport magic IO to the
> L1 hypervisor.
>
> Is this sane/sensible/wise?

If I am reading this right; my answer is yes.  This is how VMware 
defined it's backdoor port.  What might be worse is that many "commands" 
work in ring 3.

L1 hypervisor is Xen.
L2 guest is both dom0 and domU.

I.E. you are not talking about nested (xen on xen) case.

>> +            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;
> This doesn't look like it is logically part of "connecting vmport up"

You are right.  This code should not be part of this patch set. Will 
drop it.


> ~Andrew
>

[snip]

>> diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
>> index e2f60a6..32489f0 100644
>> --- a/xen/include/public/trace.h
>> +++ b/xen/include/public/trace.h
>> @@ -223,6 +223,7 @@
>>   #define TRC_HVM_NPF             (TRC_HVM_HANDLER + 0x21)
>>   #define TRC_HVM_REALMODE_EMULATE (TRC_HVM_HANDLER + 0x22)
>>   #define TRC_HVM_TRAP             (TRC_HVM_HANDLER + 0x23)
>> +#define TRC_HVM_TRAP64           (TRC_HVM_HANDLER + TRC_64_FLAG + 0x23)
> Haven't you already defined this in a previous patch?

Nope, just a related one.  Will factor both out into their own patch.

    -Don Slutz

>>   #define TRC_HVM_TRAP_DEBUG       (TRC_HVM_HANDLER + 0x24)
>>   #define TRC_HVM_VLAPIC           (TRC_HVM_HANDLER + 0x25)
>>