WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implem

To: Christoph Egger <Christoph.Egger@xxxxxxx>
Subject: Re: [Xen-devel] [PATCH 10/14] Nested Virtualization: svm specific implementation
From: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Date: Mon, 9 Aug 2010 13:57:31 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 09 Aug 2010 06:06:00 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <201008051704.03074.Christoph.Egger@xxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <201008051704.03074.Christoph.Egger@xxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.18 (2008-05-17)
> @@ -692,8 +732,10 @@ static void svm_ctxt_switch_to(struct vc
>  static void svm_do_resume(struct vcpu *v) 
>  {
>      bool_t debug_state = v->domain->debugger_attached;
> -
> -    if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
> +    bool_t guestmode = nestedhvm_vcpu_in_guestmode(v);
> +
> +    if ( !guestmode &&
> +        unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) )
>      {
>          uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3);
>          v->arch.hvm_vcpu.debug_state_latch = debug_state;
> @@ -712,11 +754,14 @@ static void svm_do_resume(struct vcpu *v
>          hvm_asid_flush_vcpu(v);
>      }
>  
> -    /* Reflect the vlapic's TPR in the hardware vtpr */
> -    v->arch.hvm_svm.vmcb->vintr.fields.tpr = 
> -        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
> -
> -    hvm_do_resume(v);
> +    if ( !guestmode )
> +    {
> +        /* Reflect the vlapic's TPR in the hardware vtpr */
> +        v->arch.hvm_svm.vmcb->vintr.fields.tpr = 
> +            (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
> +
> +        hvm_do_resume(v);
> +    }

Can you explain why we shouldn't sync the vTPR and the vlapic state when
the guest is in nested mode?

[...]

> +    } else {
> +        /* host shadow paging + guest shadow paging. */
> +        host_vmcb->np_enable = 0;
> +        host_vmcb->h_cr3 = 0x0;
> +
> +#if 0
> +        host_vmcb->cr3 = v->shadow_shadow_table;
> +
> +        /* hvm_set_cr3() below sets v->arch.hvm_vcpu.guest_cr[3] for us. */
> +        rc = hvm_set_cr3(ns_vmcb->cr3);
> +        if (rc != X86EMUL_OKAY)
> +            gdprintk(XENLOG_ERR, "hvm_set_cr3 failed, rc: %u\n", rc);
> +#endif
> +    }
> +

Please remove this. 

[...]

> +static void
> +svm_vmexit_do_vmload(struct vmcb_struct *vmcb,
> +                     struct cpu_user_regs *regs,
> +                     struct vcpu *v, uint64_t vmcbaddr)
> +{
> +    int ret;
> +    unsigned int inst_len;
> +    struct vmcb_struct *tmp_vmcb;
> +
> +    if ( (inst_len = __get_instruction_length(v, INSTR_VMLOAD)) == 0 )
> +        return;
> +
> +    /* tmp_vmcb can't be a local variable on the stack because
> +     * the machine stops with a sudden freeze.
> +     */

A VMCB structure is large, and Xen stacks are small. :)  xmalloc()ing it
on every call seems a bit fragile though - and silently returning if the
xmalloc() fails is wrong.  Is this something that could be allocated
once at VCPU creation time? 

> +    tmp_vmcb = xmalloc(struct vmcb_struct);
> +    if (tmp_vmcb == NULL)
> +        return;
> +

[...]

> @@ -1623,11 +2895,25 @@ asmlinkage void svm_vmexit_handler(struc
>  
>      case VMEXIT_MONITOR:
>      case VMEXIT_MWAIT:
> +        hvm_inject_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE, 0);
> +        break;
> +
>      case VMEXIT_VMRUN:
> +        svm_vmexit_do_vmrun(regs, v,
> +                            regs->eax);
> +        break;

svm_vmexit_do_vmrun() and the other routines called below don't seem to
check that nested HVM is enabled; that seems like it would be bad if a
non-nesting guest tries to execute VMRUN &c.  Or have you changed the
intercept bits somehow so that never happens?

Cheers,

Tim.

>      case VMEXIT_VMLOAD:
> +        svm_vmexit_do_vmload(vmcb, regs, v, regs->eax);
> +        break;
>      case VMEXIT_VMSAVE:
> +        svm_vmexit_do_vmsave(vmcb, regs, v, regs->eax);
> +        break;
>      case VMEXIT_STGI:
> +        svm_vmexit_do_stgi(regs, v);
> +        break;
>      case VMEXIT_CLGI:
> +        svm_vmexit_do_clgi(regs, v);
> +        break;
>      case VMEXIT_SKINIT:
>          hvm_inject_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE, 0);
>          break;

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel