From mboxrd@z Thu Jan  1 00:00:00 1970
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v3 04/16] hypervisor part of add vmware_port
	to xl.cfg
Date: Mon, 08 Sep 2014 11:01:10 -0400
Message-ID: <540DC4B6.1000707@oracle.com>
References: <1410182158-8542-1-git-send-email-dslutz@verizon.com>
	<1410182158-8542-5-git-send-email-dslutz@verizon.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1410182158-8542-5-git-send-email-dslutz@verizon.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Don Slutz <dslutz@verizon.com>, xen-devel@lists.xen.org
Cc: Kevin Tian <kevin.tian@intel.com>, Keir Fraser <keir@xen.org>, Ian Campbell <ian.campbell@citrix.com>, Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Jun Nakajima <jun.nakajima@intel.com>, Eddie Dong <eddie.dong@intel.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>, Jan Beulich <jbeulich@suse.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
List-Id: xen-devel@lists.xenproject.org

On 09/08/2014 09:15 AM, Don Slutz wrote:
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index b5188e6..12079be 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
> +    };
> +
> +    regs->error_code = vmcb->exitinfo1;

I am not sure this is a good idea. I have a feeling this may mess up 
fault reporting in case of double faults (e.g. see AMD volume 2 section 
15.2, Example paragraph).

Do you really need to save it into regs or can out pass exitinfo1 as 
error_code argument directly to vmport_gp_check()?

> +    inst_len = __get_instruction_length_from_list(
> +        v, list, ARRAY_SIZE(list), 0);
> +
> +    rc = vmport_gp_check(regs, v, inst_len, inst_addr, vmcb->exitinfo2);

You probably want to call this only when 
__get_instruction_length_from_list() succeeded (i.e. instr_len >0)

What happens when you have a non-VMware-aware guest that performs this 
access? Prior to this patch it would get a #GP but now it will continue 
happily running, right? This changes user-visible behavior.

I wonder whether we should enable #GP intercepts only when we know that 
the guest is VMware-aware (which we do as far as I can tell since we 
have a config option).

-boris


> +    if ( !rc )
> +        __update_guest_eip(regs, inst_len);
> +    else
> +    {
> +        VMPORT_DBG_LOG(VMPORT_LOG_GP_UNKNOWN,
> +                       "gp: rc=%d e2=%lx ec=%lx ip=%"PRIx64" (%ld) ax=%"PRIx64
> +                       " bx=%"PRIx64" cx=%"PRIx64" dx=%"PRIx64" si=%"PRIx64
> +                       " di=%"PRIx64, rc,
> +                       (unsigned long)vmcb->exitinfo2,
> +                       (unsigned long)regs->error_code,
> +                       regs->rip, inst_len, regs->rax, regs->rbx, regs->rcx,
> +                       regs->rdx, regs->rsi, regs->rdi);
> +        hvm_inject_hw_exception(TRAP_gp_fault, regs->error_code);
> +    }
> +}
> +
>