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

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

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.

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.

> +
> +    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);
> +    }
> +}
> +

...

> +
> +int vmport_gp_check(struct cpu_user_regs *regs, struct vcpu *v,
> +                    unsigned long inst_len, unsigned long inst_addr,
> +                    unsigned long ei1, unsigned long ei2)
> +{
> +    if ( !v->domain->arch.hvm_domain.is_vmware_port_enabled )
> +        return 10;
> +
> +    if ( inst_len && inst_len <= 2 && get_low_bits(regs->rdx) == BDOOR_PORT &&
> +         ei1 == 0 && ei2 == 0 && regs->error_code == 0 &&
> +         (uint32_t)regs->rax == BDOOR_MAGIC )
> +    {
> +        int i = 0;
> +        uint32_t val;
> +        uint32_t byte_cnt = 4;
> +        unsigned char bytes[2];
> +        unsigned int fetch_len;
> +        int frc;
> +        int rc;
> +
> +        /*
> +         * 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);
> +            return 11;
> +        }
> +        if ( bytes[0] == 0x66 )     /* operand size prefix */
> +        {
> +            byte_cnt = 2;
> +            i = 1;
> +            if ( fetch_len != inst_len )
> +            {
> +                frc = hvm_fetch_from_guest_virt_nofault(&bytes[1],
> +                                                        inst_addr + 1, 1,
> +                                                        PFEC_page_present);
> +                if ( frc != HVMCOPY_okay )
> +                {
> +                    gdprintk(XENLOG_WARNING,
> +                             "Bad instruction fetch at %#lx + 1 (frc=%d)\n",
> +                             (unsigned long) inst_addr, frc);
> +                    return 12;
> +                }
> +            }
> +        }
> +        if ( bytes[i] == 0xed )     /* in (%dx),%eax or in (%dx),%ax */
> +        {
> +            rc = vmport_ioport(IOREQ_READ, BDOOR_PORT, byte_cnt, &val);
> +            VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER,
> +                           "gp: VMwareIn  rc=%d ip=%"PRIx64" byte_cnt=%d ax=%"
> +                           PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64
> +                           " si=%"PRIx64" di=%"PRIx64, rc,
> +                           inst_addr, byte_cnt, regs->rax, regs->rbx,
> +                           regs->rcx, regs->rdx, regs->rsi, regs->rdi);
> +            return rc;
> +        }
> +        else if ( bytes[i] == 0xec )     /* in (%dx),%al */
> +        {
> +            rc = vmport_ioport(IOREQ_READ, BDOOR_PORT, 1, &val);
> +            VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER,
> +                           "gp: VMwareIn  rc=%d ip=%"PRIx64" byte_cnt=1 ax=%"
> +                           PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64
> +                           " si=%"PRIx64" di=%"PRIx64, rc,
> +                           inst_addr, regs->rax, regs->rbx, regs->rcx,
> +                           regs->rdx, regs->rsi, regs->rdi);
> +            return rc;
> +        }
> +        else if ( bytes[i] == 0xef )     /* out %eax,(%dx) or out %ax,(%dx) */
> +        {
> +            rc = vmport_ioport(IOREQ_WRITE, BDOOR_PORT, byte_cnt, &val);
> +            VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER,
> +                           "gp: VMwareOut rc=%d ip=%"PRIx64" byte_cnt=%d ax=%"
> +                           PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64
> +                           " si=%"PRIx64" di=%"PRIx64, rc,
> +                           inst_addr, byte_cnt, regs->rax, regs->rbx,
> +                           regs->rcx, regs->rdx, regs->rsi, regs->rdi);
> +            return rc;
> +        }
> +        else if ( bytes[i] == 0xee )     /* out %al,(%dx) */
> +        {
> +            rc = vmport_ioport(IOREQ_WRITE, BDOOR_PORT, 1, &val);
> +            VMPORT_DBG_LOG(VMPORT_LOG_GP_VMWARE_AFTER,
> +                           "gp: VMwareOut rc=%d ip=%"PRIx64" byte_cnt=1 ax=%"
> +                           PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64
> +                           " si=%"PRIx64" di=%"PRIx64, rc,
> +                           inst_addr, regs->rax, regs->rbx, regs->rcx,
> +                           regs->rdx, regs->rsi, regs->rdi);
> +            return rc;
> +        }
> +        else
> +        {
> +            VMPORT_DBG_LOG(VMPORT_LOG_GP_FAIL_RD_INST,
> +                           "gp: VMware? lip=%"PRIx64"[%d]=>0x%x(%ld) ax=%"
> +                           PRIx64" bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64
> +                           " si=%"PRIx64" di=%"PRIx64,
> +                           inst_addr, i, bytes[i], inst_len, regs->rax,
> +                           regs->rbx, regs->rcx, regs->rdx, regs->rsi,
> +                           regs->rdi);
> +            return 13;
> +        }
> +    }
> +    return 14;

The return values should be defined as macros --- otherwise they look 
like some magic integers.

-boris