Re: [PATCH 00/13] Nested Virtualization: Overview

[Christoph Egger <Christoph.Egger@amd.com>]

Eddie,

I think I have found a way to address most if not all of your concerns.

As a first step I have:
- made SVM's VMRUN emulation SVM-specific
- turned nh_arch into a union
- moved msr permission maps into SVM
- made nested pagefault vmexit injection SVM-specific
- removed nh_arch_size
- renamed nh_vmaddr to nh_vmcxaddr
- moved hvm_intblk_svm_gif into SVM

Next I will:
- make fields like nh_vm_guestcr3, nh_vm_hostcr3, and nh_guest_asid 
SVM-specific (we need to cache those values for architectural reasons) and 
provide a wrapper for where these values may be needed by generic code
- move nestedhvm_vcpu_interrupt() into SVM. This will remove the generic 
exitcode field from 'struct nestedhvm' and will allow me to move 
the 'exitinfo1' and 'exitinfo2' fields into SVM.
- make SVM's VMEXIT emulation SVM-specific. That should do away with state 
validation and mapping issues and end the confusion around nestedhvm_vmexit, 
nestedhvm_vcpu_vmexit, nhvm_vcpu_vmexit, too.
- address other outstanding terminology issues such as nhvm, struct nvcpu

It will probably take a few days to get this straightened out, so stay tuned.

Christoph


On Monday 18 October 2010 03:30:10 Dong, Eddie wrote:
> Christoph Egger wrote:
> > Hi!
> >
> > This patch series brings Nested Virtualization to Xen.
> > This is the fifth patch series. Improvements to the
> > previous patch submission:
>
> It is a step forward progress. At least those obvious SVM specific stuff is
> moved back to SVM tree, although there are still something left which is
> not good but maybe not that critical (like -    hvm_intblk_nmi_iret   /*
> NMI blocked until IRET */
> +    hvm_intblk_nmi_iret,  /* NMI blocked until IRET */
> +    hvm_intblk_svm_gif,   /* GIF cleared */
> ).
>
> However the majority of my comments are still not addressed
> (http://www.mailinglistarchive.com/html/xen-devel@lists.xensource.com/2010-
>09/msg01078.html).
>
> I can re-comments here:
>
> +struct nestedhvm {
> +    bool_t nh_guestmode; /* vcpu in guestmode? */
> +    void *nh_vmcx; /* l1 guest virtual VMCB/VMCS */
> +
> +    /* address of l1 guest virtual VMCB/VMCS, needed for VMEXIT */
> +    uint64_t nh_vmaddr;
> +
> +    void *nh_arch; /* SVM/VMX specific data */
> +    size_t nh_arch_size;
>
> I hate the pointer+size style. Rather I prefer union for SVM/VMX data
> structure.
>
> Once this is followed, the API
> nestedhvm_vcpu_initialise/nestedhvm_vcpu_reset/nestedhvm_vcpu_destroy can
> be back to wrapper only if not completely removed.
>
>
> +    /* Cache guest cr3/host cr3 the guest sets up for the nested guest.
> +     * Used by Shadow-on-Shadow and Nested-on-Nested.
> +     * nh_vm_guestcr3: in l2 guest physical address space and points to
> +     *     the l2 guest page table
> +     * nh_vm_hostcr3: in l1 guest physical address space and points to
> +     *     the l1 guest nested page table
> +     */
> +    uint64_t nh_vm_guestcr3, nh_vm_hostcr3;
> +    uint32_t nh_guest_asid;
>
> Duplicating data instance (Caching) is really not a good solution to me. I
> prefer using a wrapper API for that as my proposal sent to you in private
> mail. Caching really doesn't bring additional value here.
>
>
> +    /* Only meaningful when forcevmexit flag is set */
> +    struct {
> +        uint64_t exitcode;  /* generic exitcode */
> +        uint64_t exitinfo1; /* additional information to the exitcode */
> +        uint64_t exitinfo2; /* additional information to the exitcode */
> +    } nh_forcevmexit;
> +    union {
> +        uint32_t bytes;
> +        struct {
> +            uint32_t forcevmexit : 1;
> +            uint32_t use_native_exitcode : 1;
> +            uint32_t vmentry : 1;   /* true during vmentry/vmexit
> emulation */ +            uint32_t reserved : 29;
> +        } fields;
> +    } nh_hostflags;
>
> New namespace for VM exit info and entry control.
>
> I am not convinced by the value of this new namespace comparing with
> soultion that demux in each arch while call 1st level helper API in common
> code. The only different result is that we will pay with one API:
> nestedhvm_vcpu_vmexit, but we gain with removed conversion to/from between
> new & old namespace APIs. I strongly prefer the demux + 1st level helper
> solution.
>
> +int
> +nestedhvm_vcpu_vmentry(struct vcpu *v, struct cpu_user_regs *regs,
> +	uint64_t vmaddr, unsigned int inst_len)
> +{
> +	int ret;
> +	struct nestedhvm *hvm = &vcpu_nestedhvm(v);
> +
> +	hvm->nh_hostflags.fields.vmentry = 1;
> +
> +	ret = nestedhvm_vcpu_state_validate(v, vmaddr);
> +	if (ret) {
> +		gdprintk(XENLOG_ERR,
> +			"nestedhvm_vcpu_state_validate failed, injecting 0x%x\n",
> +			ret);
> +		hvm_inject_exception(ret, HVM_DELIVER_NO_ERROR_CODE, 0);
> +		return ret;
> +	}
> +
> +	/* Save vmaddr. Needed for VMEXIT */
> +	hvm->nh_vmaddr = vmaddr;
> +
> +	/* get nested vm */
> +	ASSERT(hvm->nh_vmcx == NULL);
> +	hvm->nh_vmcx = hvm_map_guest_frame_ro(vmaddr >> PAGE_SHIFT);
> +	if (hvm->nh_vmcx == NULL) {
> +		gdprintk(XENLOG_ERR,
> +			"hvm_map_guest_frame_ro failed, injecting #GP\n");
> +		hvm_inject_exception(TRAP_gp_fault,
> +			HVM_DELIVER_NO_ERROR_CODE, 0);
> +		hvm->nh_hostflags.fields.vmentry = 0;
> +		return TRAP_gp_fault;
> +	}
> +
> +	/* save host state */
> +	ret = nhvm_vcpu_vmentry(v, regs, inst_len);
> +	if (ret) {
> +		gdprintk(XENLOG_ERR,
> +			"nhvm_vcpu_vmentry failed, injecting #UD\n");
> +		hvm_inject_exception(TRAP_invalid_op,
> +			HVM_DELIVER_NO_ERROR_CODE, 0);
> +		hvm->nh_hostflags.fields.vmentry = 0;
> +		hvm_unmap_guest_frame(hvm->nh_vmcx);
> +		hvm->nh_vmcx = NULL;
> +		return ret;
> +	}
> +
> +	hvm_unmap_guest_frame(hvm->nh_vmcx);
> +	hvm->nh_vmcx = NULL;
> +
> +	/* Switch vcpu to guest mode.
> +	 */
> +	nestedhvm_vcpu_enter_guestmode(v);
> +
> +	hvm->nh_hostflags.fields.vmentry = 0;
> +	return 0;
> +}
>
> You must not read VMX code yet or didn't understand the reason why VMX
> can;t do this. Appearantly this common code doesn't apply to VMX and have
> to be moved to arch specific stuff. VMX can only switch the context (L1 or
> L2 guest) at certain point (now defered to last step before resume) because
> only one VMCS can be accessed in one context.
>
> Thx, Eddie



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632