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

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

> @@ -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