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

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

On Monday 22 November 2010 07:55:04 Dong, Eddie wrote:
> Christoph Egger wrote:
> > Hi!
> >
> > This patch series brings Nested Virtualization to Xen.
> > This is the sixth patch series. Improvements to the
> > previous patch submission:
> >
> > - Move GIF definition into SVM
> > - Move VMEXIT emulation into SVM
> > - Introduce hooks for getting host/guest cr3 for use with hap-on-hap
> >    per proposal from Eddie Dong
> > - Moved fields from struct nestedhvm into SVM
> > - Renamed struct nestedhvm to struct nestedvcpu
> > - Reworked VMRUN and VMEXIT emulation. It uses a defered emulation
> >    mechanism that makes interrupt handling more efficient and is
> >    closer to what VMX is doing
> > - VMCB is peristent mapped. Only remap the VMCB when l1 guest
> >    changes the address.
> >
> >
> > The patch series:
> >
> > patch 01: add nestedhvm guest config option to the tools.
> >                   This is the only one patch touching the tools
> > patch 02: Add data structures for nested virtualization.
> > patch 03: add nestedhvm function hooks.
> > patch 04: The heart of nested virtualization.
> > patch 05: Allow switch to paged real mode during vmrun emulation.
> >                   Emulate cr0 and cr4 when guest does not intercept
> >                   them (i.e. Hyper-V/Windows7, KVM)
> > patch 06: When injecting an exception into nested guest, inject
> >                   #VMEXIT into the guest if intercepted.
> > patch 07: Allow guest to enable SVM in EFER only on AMD.
> > patch 08: Handle interrupts (generic part).
> > patch 09: SVM specific implementation for nested virtualization.
> > patch 10: Handle interrupts (SVM specific).
> > patch 11: The piece of code that effectively turns on nested
> > virtualization. patch 12: Move dirty_vram from struct hvm_domain to
> >                   struct p2m_domain. This change is the first part
> >                   from a larger not-yet-ready change where the vram
> >                   and log_dirty tracking is teached to work on per
> > p2m.
> > patch 13: Handle nested pagefault to enable hap-on-hap and handle
> >                   nested guest page-table-walks to emulate
> >                   instructions the guest does not intercept (i.e.
> > WBINVD with Windows 7).
>
> Thanks for the progress!
> I am fine with the patch series in general .
>
> Some minor comments here:
>
> +enum nestedhvm_intercepts {
> +	/* exitinfo1 and exitinfo2 undefined */
> +	NESTEDHVM_INTERCEPT_INVALID = 0, /* INVALID vmcb/vmcs */
> +	NESTEDHVM_INTERCEPT_SHUTDOWN = 1, /* kill guest */
> +	NESTEDHVM_INTERCEPT_MCE = 2, /* machine check exception */
> +	NESTEDHVM_INTERCEPT_VMMCALL = 3, /* VMMCALL/VMCALL */
> +
> +	/* exitinfo1 is hvm_intsrc_*, exitinfo2 is the vector */
> +	NESTEDHVM_INTERCEPT_INTR = 4, /* interrupt exit code */
> +	NESTEDHVM_INTERCEPT_NMI = 5, /* NMI exit code */
> +
> +	/* exitinfo1 is PF error code, exitinfo2 is PF fault address */
> +	NESTEDHVM_INTERCEPT_NPF = 6, /* nested page fault */
> +	NESTEDHVM_INTERCEPT_PF = 7, /* page fault */
> +
> +	/* exceptions: exitinfo1 and exitinfo2 are undefined */
> +	NESTEDHVM_INTERCEPT_NM = 8, /* device-not-available */
> +
> +	/* end mark */
> +	NESTEDHVM_INTERCEPT_LAST,
> +};
>
> I didn't see where it is used. Did I miss something?

I forgot to remove them. Thanks for pointing this out. Fixed.

>
>
> +    union {
> +        uint32_t bytes;
> +        struct {
> +            uint32_t vmentry_pending: 1;
> +            uint32_t vmexit_pending: 1;
> +            uint32_t vmswitch_in_progress: 1; /* true during
> vmentry/vmexit emulation */ +            uint32_t reserved : 29;
> +        } fields;
> +    } nv_hostflags;
>
> I feel it is unnecessary to use tricky bit definition (slow and readers may
> think it has some relationship w/ hardware field). Using a simple byte
> variable for each field seems more simple to me.

hmm... hw bit field in common code?
Anyway, I don't mind. I will change it as you suggest.

>
> Both nhvm & nestedhvm is used in API names, what is the policy?

nhvm just call the function hooks.
Likewise within SVM: nsvm implements the interface.

Christoph



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