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@xxxxxxxxxxxxxxxxxxx/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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home