[PATCH 00/13] Nested Virtualization: Overview

[PATCH 00/13] Nested Virtualization: Overview

[Christoph Egger]

Hi!

This patch series brings Nested Virtualization to Xen.
This is the third patch series. Improvements to the
previous patch submission:

- Incorporated feedback discussion with Keir, Tim and Eddie.

There might still be some nuances to fiddle with to make it
fit for VMX. Feedback from Intel is appreciated, therefore.

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


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

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

[Dong, Eddie]

The fundamental argument of whether we should convert vendor specific code into vendor neutral code is not solved yet.I guess I don;t need to review rest of the code due to this :)
I strongly suggest we remove those unnecessary wrapper for readibilty, flexibility and performance.

BTW, VMX has a policy to access VMCS field only when it is a must.

Thanks, Eddie



Christoph Egger wrote:
> Hi!
> 
> This patch series brings Nested Virtualization to Xen.
> This is the third patch series. Improvements to the
> previous patch submission:
> 
> - Incorporated feedback discussion with Keir, Tim and Eddie.
> 
> There might still be some nuances to fiddle with to make it
> fit for VMX. Feedback from Intel is appreciated, therefore.
> 
> 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.
> 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). 

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

[Christoph Egger]

On Friday 03 September 2010 10:12:08 Dong, Eddie wrote:
> The fundamental argument of whether we should convert vendor specific code
> into vendor neutral code is not solved yet.
> I guess I don;t need to review rest of the code due to this :) I strongly
> suggest we remove those unnecessary wrapper for readibilty, flexibility and
> performance. 

I am sort of surprised that you raise the same things again that have been
discussed in the last round. Please correct me if I am wrong: This sounds to
me that the statements/explanations from Keir, Tim and me are not clear
to you. Please feel free to ask whatever is unclear / questionable to you.

> BTW, VMX has a policy to access VMCS field only when it is a must.

SVM, too. AFAICS, the difference is the method *how* to access fields.

Christoph

> Thanks, Eddie
>
> Christoph Egger wrote:
> > Hi!
> >
> > This patch series brings Nested Virtualization to Xen.
> > This is the third patch series. Improvements to the
> > previous patch submission:
> >
> > - Incorporated feedback discussion with Keir, Tim and Eddie.
> >
> > There might still be some nuances to fiddle with to make it
> > fit for VMX. Feedback from Intel is appreciated, therefore.
> >
> > 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.
> > 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).



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

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

[Dong, Eddie]

Christoph Egger wrote:
> On Friday 03 September 2010 10:12:08 Dong, Eddie wrote:
>> The fundamental argument of whether we should convert vendor
>> specific code into vendor neutral code is not solved yet.
>> I guess I don;t need to review rest of the code due to this :) I
>> strongly suggest we remove those unnecessary wrapper for readibilty,
>> flexibility and performance.
> 
> I am sort of surprised that you raise the same things again that have
> been discussed in the last round. Please correct me if I am wrong:
> This sounds to me that the statements/explanations from Keir, Tim and
> me are not clear 
> to you. Please feel free to ask whatever is unclear / questionable to
> you. 

So you are actually not asking for my review comments, though you explicitly called it.

I am not convinced a wrapper for wrapping, which is just make it like C style as you said, should be taken just because somebody have called it out.

Freely (at any time) preload/post-store of VMCS fields is very hard because VMX only provide access to current VMCS (because it is CPU register), while SVM may be able to access all VMCBs given that it is in memory. I can't say it is impossible (one can solve it with further limitation/complexity), however enforcing those conversion simply put trickies to VMX code to limiting the time of pre-load/post-load, and the additional cost of VMCS access.



Another portion of so called common code are actually SVM code only. Here are part of them:



+
+static enum nestedhvm_vmexits
+nestedhvm_vmexit_msr(unsigned long *msr_bitmap, uint32_t msr, bool_t write)
+{
+	bool_t enabled;
+	unsigned long *msr_bit = NULL;
+
+	/*
+	 * See AMD64 Programmers Manual, Vol 2, Section 15.10
+	 * (MSR-Bitmap Address).
+	 */
+	if ( msr <= 0x1fff )
+		msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
+	else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
+		msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
+	else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
+		msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;



+/* Virtual GIF */
+int
+nestedsvm_vcpu_clgi(struct vcpu *v)
+{
+	if (!nestedhvm_enabled(v->domain)) {
+		hvm_inject_exception(TRAP_invalid_op, 0, 0);
+		return -1;
+	}
+
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return 0;
+
+	/* clear gif flag */
+	vcpu_nestedhvm(v).nh_gif = 0;
+	local_event_delivery_disable(); /* mask events for PV drivers */
+	return 0;
+}
+
+int
+nestedsvm_vcpu_stgi(struct vcpu *v)
+{
+	if (!nestedhvm_enabled(v->domain)) {
+		hvm_inject_exception(TRAP_invalid_op, 0, 0);
+		return -1;
+	}
+
+	/* Always set the GIF to make hvm_interrupt_blocked work. */
+	vcpu_nestedhvm(v).nh_gif = 1;
+
+	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+		return 0;
+
+	local_event_delivery_enable(); /* unmask events for PV drivers */
+	return 0;
+}
+


> 
>> BTW, VMX has a policy to access VMCS field only when it is a must.
> 
> SVM, too. AFAICS, the difference is the method *how* to access fields.
> 
> Christoph
> 

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

[Christoph Egger]

On Saturday 04 September 2010 03:30:56 Dong, Eddie wrote:
> Christoph Egger wrote:
> > On Friday 03 September 2010 10:12:08 Dong, Eddie wrote:
> >> The fundamental argument of whether we should convert vendor
> >> specific code into vendor neutral code is not solved yet.
> >> I guess I don;t need to review rest of the code due to this :) I
> >> strongly suggest we remove those unnecessary wrapper for readibilty,
> >> flexibility and performance.
> >
> > I am sort of surprised that you raise the same things again that have
> > been discussed in the last round. Please correct me if I am wrong:
> > This sounds to me that the statements/explanations from Keir, Tim and
> > me are not clear
> > to you. Please feel free to ask whatever is unclear / questionable to
> > you.
>
> So you are actually not asking for my review comments, though you
> explicitly called it.

Well, when you say so :)

> I am not convinced a wrapper for wrapping, which is just make it like C
> style as you said, should be taken just because somebody have called it
> out.

I wouldn't call it a wrapper. I call as the implementation of an abstraction.

And regarding to what Tim said:
http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01044.html

Is there a particular reason not to do so apart from the LOC as you brought
up in http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01255.html

(As Tim said in
http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01256.html
it's the duplication of logic that counts.)

> Freely (at any time) preload/post-store of VMCS fields is very hard because
> VMX only provide access to current VMCS (because it is CPU register), while
> SVM may be able to access all VMCBs given that it is in memory. I can't say
> it is impossible (one can solve it with further limitation/complexity),
> however enforcing those conversion simply put trickies to VMX code to
> limiting the time of pre-load/post-load, and the additional cost of VMCS
> access.
>

When the VCPU switches between host and guest mode then you need to
save the l1 guest state, restore the l2 guest state and vice versa.

This requires a lot of accesses from/to the vmcb/vmcs  unless you have
a lazy switching technique, do you ?


>
> Another portion of so called common code are actually SVM code only. Here
> are part of them:
>
>
>
> +
> +static enum nestedhvm_vmexits
> +nestedhvm_vmexit_msr(unsigned long *msr_bitmap, uint32_t msr, bool_t
> write) +{
> +	bool_t enabled;
> +	unsigned long *msr_bit = NULL;
> +
> +	/*
> +	 * See AMD64 Programmers Manual, Vol 2, Section 15.10
> +	 * (MSR-Bitmap Address).
> +	 */
> +	if ( msr <= 0x1fff )
> +		msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
> +	else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> +		msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
> +	else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
> +		msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;
>

Why does above code snippet not work on Intel CPUs ?


>
> +/* Virtual GIF */
> +int
> +nestedsvm_vcpu_clgi(struct vcpu *v)
> +{
> +	if (!nestedhvm_enabled(v->domain)) {
> +		hvm_inject_exception(TRAP_invalid_op, 0, 0);
> +		return -1;
> +	}
> +
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +		return 0;
> +
> +	/* clear gif flag */
> +	vcpu_nestedhvm(v).nh_gif = 0;
> +	local_event_delivery_disable(); /* mask events for PV drivers */
> +	return 0;
> +}
> +
> +int
> +nestedsvm_vcpu_stgi(struct vcpu *v)
> +{
> +	if (!nestedhvm_enabled(v->domain)) {
> +		hvm_inject_exception(TRAP_invalid_op, 0, 0);
> +		return -1;
> +	}
> +
> +	/* Always set the GIF to make hvm_interrupt_blocked work. */
> +	vcpu_nestedhvm(v).nh_gif = 1;
> +
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +		return 0;
> +
> +	local_event_delivery_enable(); /* unmask events for PV drivers */
> +	return 0;
> +}
> +

The reason to leave this in the generic code is what Keir stated out as
feedback to the second patch series:
http://lists.xensource.com/archives/html/xen-devel/2010-06/msg00280.html
(What was patch #10 there is  patch #8 in latest patch series).

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

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

[Dong, Eddie]

>> I am not convinced a wrapper for wrapping, which is just make it
>> like C style as you said, should be taken just because somebody have
>> called it out.
> 
> I wouldn't call it a wrapper. I call as the implementation of an
> abstraction. 


While, that is terminology difference, abstract for abstract is as same bad as wrapper for wrapper. One good example is SVM on VMX that you abstracted months ago.
Extending SVM knowledge to VMX and limiting the flexbility of VMX implementation (in case you won't implement VMX code) is not a practical solution IMO. Nested VMX will touch existing code more comprehensively.

>> Freely (at any time) preload/post-store of VMCS fields is very hard
>> because VMX only provide access to current VMCS (because it is CPU
>> register), while SVM may be able to access all VMCBs given that it
>> is in memory. I can't say it is impossible (one can solve it with
>> further limitation/complexity), however enforcing those conversion
>> simply put trickies to VMX code to limiting the time of
>> pre-load/post-load, and the additional cost of VMCS access. 
>> 
> 
> When the VCPU switches between host and guest mode then you need to
> save the l1 guest state, restore the l2 guest state and vice versa.

No. Switching from L1 to L2 guest, it is a simple restore of L2 guest state (VMCLEAR/VMPTRLD). VMX doesn't save L1 state, while SVM does require the save of L1 state per my understanding. Intel process can hold VMCS in processor for performance.

Switching from L2 to L1, we need to convert (or store) some VMCS information from physical to virtual VMCS. But it is limited and only covers the "guest state" and exit information. Load of L1 state may be as simple as VMPTRLD (of course it may modify some VMCS field upon different situation).

This is another good example how risky it is to extend SVM knowledge to VMX.

> 
> This requires a lot of accesses from/to the vmcb/vmcs  unless you have
> a lazy switching technique, do you ?

Yes, the Intel hardware already did lazy switching thru VMPTRLD.
And the access of VMCS is depending on the L1 guest modification, only dirty VMCS fields needs to be updated. 

In the majority case, the VM exit from L2 guest will be handled by root VMM directly. One example is external interrupt, which doesn't need to access rest of VMCS fields except the reason, but the wrapper makes the access a must, which I can't agree.

> 
> 
>> 
>> Another portion of so called common code are actually SVM code only.
>> Here are part of them: 
>> 
>> 
>> 
>> +
>> +static enum nestedhvm_vmexits
>> +nestedhvm_vmexit_msr(unsigned long *msr_bitmap, uint32_t msr,
>> bool_t write) +{ +	bool_t enabled;
>> +	unsigned long *msr_bit = NULL;
>> +
>> +	/*
>> +	 * See AMD64 Programmers Manual, Vol 2, Section 15.10 +	 *
>> (MSR-Bitmap Address). +	 */
>> +	if ( msr <= 0x1fff )
>> +		msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
>> +	else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> +		msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
>> +	else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
>> +		msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;
>> 
> 
> Why does above code snippet not work on Intel CPUs ?

It is said even "Intel processor" is following AMD64 manual, isn;t it?
msr_bitmap in Intel doesn't have a fixed bitmap position, rather it scan the entire table to decide which MSR to automatically save/restore for performance given that we only put single digital MSRs for that purpose.
BTW, Intel does implement MSRs large than 0x1fff such as 0x107D0. Preassuming their usage model for now is risky.

> 
> 
>> 
>> +/* Virtual GIF */
>> +int
>> +nestedsvm_vcpu_clgi(struct vcpu *v)
>> +{
>> +	if (!nestedhvm_enabled(v->domain)) {
>> +		hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +		return -1;
>> +	}
>> +
>> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>> +		return 0;
>> +
>> +	/* clear gif flag */
>> +	vcpu_nestedhvm(v).nh_gif = 0;
>> +	local_event_delivery_disable(); /* mask events for PV drivers */
>> +	return 0; +}
>> +
>> +int
>> +nestedsvm_vcpu_stgi(struct vcpu *v)
>> +{
>> +	if (!nestedhvm_enabled(v->domain)) {
>> +		hvm_inject_exception(TRAP_invalid_op, 0, 0);
>> +		return -1;
>> +	}
>> +
>> +	/* Always set the GIF to make hvm_interrupt_blocked work. */
>> +	vcpu_nestedhvm(v).nh_gif = 1;
>> +
>> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>> +		return 0;
>> +
>> +	local_event_delivery_enable(); /* unmask events for PV drivers */
>> +	return 0; +}
>> +
> 
> The reason to leave this in the generic code is what Keir stated out
> as feedback to the second patch series:
> http://lists.xensource.com/archives/html/xen-devel/2010-06/msg00280.html
> (What was patch #10 there is  patch #8 in latest patch series).
> 

While, I didn;t read in this way. Even the function itself is wrapped, they should go to SVM tree if not go together with the caller.

Anyway, to me given that nested SVM & VMX is still on the very beginning of development, I can only say yes to those wrappers that have clear understanding to both side. I would rather leave those uncertain wrappers to future, once the basic shape of nested virtualization is good and stable enough, i.e. lightweight wrapper.  We have plenty of performance work ahead such as virtual VTd support, enhanced PV driver for nested etc. Excessive wrapper is simple a burden to nested VMX developers for those future feaftures. 

Qing will post his patch today or tomorrow for your reference if you want.

C++ abstracts better than C, but C++ never replaces C in places like Linux. 

BTW, can you add SR-IOV test into your patch test system to avoid regression?

Thx, Eddie

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

[Christoph Egger]

Thank you very much for giving me the valuable information I am asking for, 
finally.

> >> Freely (at any time) preload/post-store of VMCS fields is very hard
> >> because VMX only provide access to current VMCS (because it is CPU
> >> register), while SVM may be able to access all VMCBs given that it
> >> is in memory. I can't say it is impossible (one can solve it with
> >> further limitation/complexity), however enforcing those conversion
> >> simply put trickies to VMX code to limiting the time of
> >> pre-load/post-load, and the additional cost of VMCS access.
> >
> > When the VCPU switches between host and guest mode then you need to
> > save the l1 guest state, restore the l2 guest state and vice versa.
>
> No. Switching from L1 to L2 guest, it is a simple restore of L2 guest state
> (VMCLEAR/VMPTRLD). VMX doesn't save L1 state,

So the function hooks prepare4vmentry is pretty cheap and the 'hostsave'
function hook is empty.

> while SVM does require the save of L1 state per my understanding.

SVM does not per architecture. It is the implementation I have taken over from
KVM. It will be replaced with an implementation that only requires a VMSAVE
to save the L1 state. But that won't change the patch series fundamentally as
this change touches SVM code only.

> Intel process can hold VMCS in processor for performance.
>
> Switching from L2 to L1, we need to convert (or store) some VMCS
> information from physical to virtual VMCS.

That is what can be put into the prepare4vmexit function hook.

> But it is limited and only covers the "guest state" and exit information.
> Load of L1 state may be as simple as VMPTRLD (of course it may modify some
> VMCS field upon different situation).

That is what can be put into the 'hostrestore' function hook.

> > This requires a lot of accesses from/to the vmcb/vmcs  unless you have
> > a lazy switching technique, do you ?
>
> Yes, the Intel hardware already did lazy switching thru VMPTRLD.
> And the access of VMCS is depending on the L1 guest modification, only
> dirty VMCS fields needs to be updated.

Sounds like you need a 'shadow vmcs' that holds the l2 guest state.

> In the majority case, the VM exit from L2 guest will be handled by root VMM
> directly.

Same on SVM. root VMM handles everything L1 guest does not intercept plus some
special intercepts such as interrupts, nmi's, page fault/nested page faults.

> One example is external interrupt, which doesn't need to access 
> rest of VMCS fields except the reason, but the wrapper makes the access a
> must, which I can't agree.

Which conversion function do you mean by 'wrapper' here ? Why does it require
additional VMCS field accesses ?

Can you explain this in detail, please ?


> >> Another portion of so called common code are actually SVM code only.
> >> Here are part of them:
> >>
> >>
> >>
> >> +
> >> +static enum nestedhvm_vmexits
> >> +nestedhvm_vmexit_msr(unsigned long *msr_bitmap, uint32_t msr,
> >> bool_t write) +{ +	bool_t enabled;
> >> +	unsigned long *msr_bit = NULL;
> >> +
> >> +	/*
> >> +	 * See AMD64 Programmers Manual, Vol 2, Section 15.10 +	 *
> >> (MSR-Bitmap Address). +	 */
> >> +	if ( msr <= 0x1fff )
> >> +		msr_bit = msr_bitmap + 0x0000 / BYTES_PER_LONG;
> >> +	else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
> >> +		msr_bit = msr_bitmap + 0x0800 / BYTES_PER_LONG;
> >> +	else if ( (msr >= 0xc0010000) && (msr <= 0xc0011fff) )
> >> +		msr_bit = msr_bitmap + 0x1000 / BYTES_PER_LONG;
> >
> > Why does above code snippet not work on Intel CPUs ?
>
> It is said even "Intel processor" is following AMD64 manual, isn;t it?
> msr_bitmap in Intel doesn't have a fixed bitmap position, rather it scan
> the entire table to decide which MSR to automatically save/restore for
> performance given that we only put single digital MSRs for that purpose.
> BTW, Intel does implement MSRs large than 0x1fff such as 0x107D0.
> Preassuming their usage model for now is risky.

Ok, I will think about this.

> >> +/* Virtual GIF */
> >> +int
> >> +nestedsvm_vcpu_clgi(struct vcpu *v)
> >> +{
> >> +	if (!nestedhvm_enabled(v->domain)) {
> >> +		hvm_inject_exception(TRAP_invalid_op, 0, 0);
> >> +		return -1;
> >> +	}
> >> +
> >> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> >> +		return 0;
> >> +
> >> +	/* clear gif flag */
> >> +	vcpu_nestedhvm(v).nh_gif = 0;
> >> +	local_event_delivery_disable(); /* mask events for PV drivers */
> >> +	return 0; +}
> >> +
> >> +int
> >> +nestedsvm_vcpu_stgi(struct vcpu *v)
> >> +{
> >> +	if (!nestedhvm_enabled(v->domain)) {
> >> +		hvm_inject_exception(TRAP_invalid_op, 0, 0);
> >> +		return -1;
> >> +	}
> >> +
> >> +	/* Always set the GIF to make hvm_interrupt_blocked work. */
> >> +	vcpu_nestedhvm(v).nh_gif = 1;
> >> +
> >> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> >> +		return 0;
> >> +
> >> +	local_event_delivery_enable(); /* unmask events for PV drivers */
> >> +	return 0; +}
> >> +
> >
> > The reason to leave this in the generic code is what Keir stated out
> > as feedback to the second patch series:
> > http://lists.xensource.com/archives/html/xen-devel/2010-06/msg00280.html
> > (What was patch #10 there is  patch #8 in latest patch series).
>
> While, I didn;t read in this way. Even the function itself is wrapped, they
> should go to SVM tree if not go together with the caller.
>
> Anyway, to me given that nested SVM & VMX is still on the very beginning of
> development, I can only say yes to those wrappers that have clear
> understanding to both side.

Good to know. See my offer below.

> I would rather leave those uncertain wrappers to future, once the basic
> shape of nested virtualization is good and stable enough, i.e. lightweight
> wrapper.  We have plenty of performance work ahead such as virtual VTd
> support, enhanced PV driver for nested etc. Excessive wrapper is simple a
> burden to nested VMX developers for those future features.
>
> Qing will post his patch today or tomorrow for your reference if you want.

Thanks. May I take code from there and add into my patch series ?

> C++ abstracts better than C, but C++ never replaces C in places like Linux.

It's not necessary. When you truely understand the concept behind interface
and implementation you can do that in any language you know.

> BTW, can you add SR-IOV test into your patch test system to avoid
> regression?

Yes.

> 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

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

[Dong, Eddie]

>>> When the VCPU switches between host and guest mode then you need to
>>> save the l1 guest state, restore the l2 guest state and vice versa.
>> 
>> No. Switching from L1 to L2 guest, it is a simple restore of L2
>> guest state (VMCLEAR/VMPTRLD). VMX doesn't save L1 state,
> 
> So the function hooks prepare4vmentry is pretty cheap and the
> 'hostsave' function hook is empty.

I don;t know how you conclude here, extending SVM knowledge to VMX again?
Prepare4vmentry has to load several VMCS fields, which is expansive in VMX.

>From SVM code, it loads 11 VMCB fields, which may be 11 VMCS field access in VMX.
nsvm_vmcb_prepare4vmexit does reverse thing which touches at least 6 VMCS field again.

Those 17 VMCS access already far exceeds the average VMCS access # in Xen for a typical VM exit, and even may be more expansive than an entire SW VM exit handler in some old platforms (The modern processor may be pretty fast though).
In many cases (even with nested virtualization), we can complete the VM exit handling in 2-3 VMCS field access.

> 
>> while SVM does require the save of L1 state per my understanding.
> 
> SVM does not per architecture. It is the implementation I have taken
> over from KVM. It will be replaced with an implementation that only
> requires a VMSAVE to save the L1 state. But that won't change the
> patch series fundamentally as this change touches SVM code only.
> 
>> Intel process can hold VMCS in processor for performance.
>> 
>> Switching from L2 to L1, we need to convert (or store) some VMCS
>> information from physical to virtual VMCS.
> 
> That is what can be put into the prepare4vmexit function hook.

No, all those are conditionally and accessible at current VMCS context only.
It is not guranteed at the time of prepare4vmexit. It is particular risky if we change the nested VMX code algorithm.

> 
>> But it is limited and only covers the "guest state" and exit
>> information. Load of L1 state may be as simple as VMPTRLD (of course
>> it may modify some VMCS field upon different situation).
> 
> That is what can be put into the 'hostrestore' function hook.
> 
>>> This requires a lot of accesses from/to the vmcb/vmcs  unless you
>>> have a lazy switching technique, do you ?
>> 
>> Yes, the Intel hardware already did lazy switching thru VMPTRLD.
>> And the access of VMCS is depending on the L1 guest modification,
>> only dirty VMCS fields needs to be updated.
> 
> Sounds like you need a 'shadow vmcs' that holds the l2 guest state.
> 
>> In the majority case, the VM exit from L2 guest will be handled by
>> root VMM directly.
> 
> Same on SVM. root VMM handles everything L1 guest does not intercept
> plus some special intercepts such as interrupts, nmi's, page
> fault/nested page faults. 

That is the key reason that I strongly against the wrapper of VM exit and VM entry. W/o the wrapper, we can complete the handler in 2-3 VMCS field access in most case, but w/ the wrapper, we have to spend 17+ VMCS field access.

> 
>> One example is external interrupt, which doesn't need to access
>> rest of VMCS fields except the reason, but the wrapper makes the
>> access a must, which I can't agree.
> 
> Which conversion function do you mean by 'wrapper' here ? Why does it
> require additional VMCS field accesses ?
> 
> Can you explain this in detail, please ?
> 

see before. nsvm_vmcb_prepare4vmexit and Prepare4vmentry.


>>> The reason to leave this in the generic code is what Keir stated out
>>> as feedback to the second patch series:
>>> http://lists.xensource.com/archives/html/xen-devel/2010-06/msg00280.html
>>> (What was patch #10 there is  patch #8 in latest patch series).
>> 
>> While, I didn;t read in this way. Even the function itself is
>> wrapped, they should go to SVM tree if not go together with the
>> caller. 
>> 
>> Anyway, to me given that nested SVM & VMX is still on the very
>> beginning of development, I can only say yes to those wrappers that
>> have clear understanding to both side.
> 
> Good to know. See my offer below.

I am not sure if you want to efficiently start with lighweight wrapper first (those wrapper with consense and individual solution for those not), or you want to keep spinning on the heavy wrapper.  For me, if you go with lighweight wrapper like single layer VMX/SVM virtualization does, I can ack soon. But for the heavy weight wrapper, it is not necessary to me and will block future development of nested VMX, so I can't ack. Even current code is a lot of hard to understand given that I need to understand the semantics of those new (vendor-neutral) fields, however the semantics of VMX fields are clear to all VMX developers. Sorry for that, although I know Tim is more considerative to the effort you have called it out. I appreciate your effort as well, but that reason is not strong enough yet to impact nested VMX development.

> 
>> I would rather leave those uncertain wrappers to future, once the
>> basic shape of nested virtualization is good and stable enough, i.e.
>> lightweight wrapper.  We have plenty of performance work ahead such
>> as virtual VTd support, enhanced PV driver for nested etc. Excessive
>> wrapper is simple a burden to nested VMX developers for those future
>> features. 
>> 
>> Qing will post his patch today or tomorrow for your reference if you
>> want. 
> 
> Thanks. May I take code from there and add into my patch series ?

Sure. But you won;t assume this is the logic we won't change. The more performance tunning happens, the more we may change, and it is possible some new hardware features may come some time later.

Thx, Eddie

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

[Christoph Egger]

On Wednesday 08 September 2010 10:58:52 Dong, Eddie wrote:
> >>> When the VCPU switches between host and guest mode then you need to
> >>> save the l1 guest state, restore the l2 guest state and vice versa.
> >>
> >> No. Switching from L1 to L2 guest, it is a simple restore of L2
> >> guest state (VMCLEAR/VMPTRLD). VMX doesn't save L1 state,
> >
> > So the function hooks prepare4vmentry is pretty cheap and the
> > 'hostsave' function hook is empty.
>
> I don;t know how you conclude here, extending SVM knowledge to VMX again?
> Prepare4vmentry has to load several VMCS fields, which is expansive in VMX.
>
> From SVM code, it loads 11 VMCB fields, which may be 11 VMCS field access
> in VMX. nsvm_vmcb_prepare4vmexit does reverse thing which touches at least
> 6 VMCS field again.

I better start with explaining the basic thing for software design: Interface
and Implementation.

Think on cars. All cars are different but they all have three pedals and a 
steering wheel. The three pedals and the steering wheel have the same
function in all cars. To drive a car you need to know what they do and how you
use them. But you do not have to know how they work.

Speaking in software design, this is called an interface.

How the pedals and the steering wheel work is up to the car manufacturers how
they make them work to function as intended. They build different motors
with different numbers of cylinders, one needs benzine an other needs diesel.
By concept the car manufacturer can even build in hamster wheels as a motor
(Getting the hamster running faster/slower is up to the car manufacturer
when using the pedals).

Speaking in software design, this is called an implementation.


Now translated to nested virtualization, the interface 'prepare4vmentry'
does prepare the l2 guest state ready to run. The way you think you have to
implement it is a possible way to make it work but the assumption you have
to follow the SVM implementation is wrong.

If I were you I would take the function 'virtual_vmentry' from
http://lists.xensource.com/archives/html/xen-devel/2010-04/msg01166.html
and implement 'prepare4vmrun' this way:

static void virtual_vmentry(struct vcpu *v, struct cpu_user_regs *regs)
{
    struct nestedhvm *hvm = &vcpu_nestedhvm(v);
    struct vmx_nest_struct *nest = hvm->nh_arch;
    struct vmcs_struct *svmcs = hvm->nh_vm;
    struct vmcs_struct *host_vmcs = v->arch.hvm_vmx.vmcs;

#ifdef __x86_64__
    unsigned long lm_l1, lm_l2;
#endif

    vmx_vmcs_switch_current(v, host_vmcs, svmcs);

    /* generic code switches vcpu into guest mode */

    nest->vmresume_pending = 0;

    /* generic code indicates that we are in vmentry emulation by
     * nh_hostflags.fields.vmentry flag.
     */

#ifdef __x86_64__
    /*
     * EFER handling:
     * hvm_set_efer won't work if CR0.PG = 1, so we change the value
     * directly to make hvm_long_mode_enabled(v) work in L2.
     * An additional update_paging_modes is also needed is
     * there is 32/64 switch. v->arch.hvm_vcpu.guest_efer doesn't
     * need to be saved, since its value on vmexit is determined by
     * L1 exit_controls
     */
    lm_l1 = !!hvm_long_mode_enabled(v);
    lm_l2 = !!(__get_vvmcs(nest->vvmcs, VM_ENTRY_CONTROLS) &
                           VM_ENTRY_IA32E_MODE);

    if ( lm_l2 )
        v->arch.hvm_vcpu.guest_efer |= EFER_LMA | EFER_LME;
    else
        v->arch.hvm_vcpu.guest_efer &= ~(EFER_LMA | EFER_LME);
#endif

    load_l2_control(nest);
    load_vvmcs_guest_state(nest);

#ifdef __x86_64__
    if ( lm_l1 != lm_l2 )
    {
        paging_update_paging_modes(v);
    }
#endif

    regs->rip = __get_vvmcs(nest->vvmcs, GUEST_RIP);
    regs->rsp = __get_vvmcs(nest->vvmcs, GUEST_RSP);
    regs->rflags = __get_vvmcs(nest->vvmcs, GUEST_RFLAGS);

    /* TODO: EPT_POINTER */
}


Analogous to this I would take 'virtual_vmexit' and implement it
for 'prepare4vmexit'.

Will this work for you ?
Do you still see utterly much VMCS field accesses when you go this way?


> >> Anyway, to me given that nested SVM & VMX is still on the very
> >> beginning of development, I can only say yes to those wrappers that
> >> have clear understanding to both side.
> >
> > Good to know. See my offer below.
>
> I am not sure if you want to efficiently start with lighweight wrapper
> first (those wrapper with consense and individual solution for those not),
> or you want to keep spinning on the heavy wrapper.  For me, if you go with
> lighweight wrapper like single layer VMX/SVM virtualization does, I can ack
> soon. But for the heavy weight wrapper, it is not necessary to me and will
> block future development of nested VMX, so I can't ack. Even current code
> is a lot of hard to understand given that I need to understand the
> semantics of those new (vendor-neutral) fields, however the semantics of
> VMX fields are clear to all VMX developers. Sorry for that, although I know
> Tim is more considerative to the effort you have called it out. I
> appreciate your effort as well, but that reason is not strong enough yet to
> impact nested VMX development.
>
> >> I would rather leave those uncertain wrappers to future, once the
> >> basic shape of nested virtualization is good and stable enough, i.e.
> >> lightweight wrapper.  We have plenty of performance work ahead such
> >> as virtual VTd support, enhanced PV driver for nested etc. Excessive
> >> wrapper is simple a burden to nested VMX developers for those future
> >> features.
> >>
> >> Qing will post his patch today or tomorrow for your reference if you
> >> want.
> >
> > Thanks. May I take code from there and add into my patch series ?
>
> Sure.

Thanks.

> But you won;t assume this is the logic we won't change. 

Of course not.

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

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

[Dong, Eddie]

Well, so you decide to cycle here, though it obviously not only hurt Intel but also AMD's progress.

Let me ask several questions first:
1: Does this heavy weight wrapper a must for nested virtualization?
2: What is the gain and lose?
3: How single layer virtualization does? Are they abstracting with heavy weight wrapper or light weight and why? Particularly do they abstract the 1st level VM exit/entry event or do they invent 3rd name space for VMCS/VMCB field?

The only reason I can see so far is that you called out the "abstract" in nested virtualization side, that is for some reason we have to appreciate but not enough for us to swallow the side effect of its impact to future nested VMX development. 


Taking your "car" abstract as example, 

> All cars are different but they all have three pedals and a steering wheel.
Well, automatic cars have 2 pedals only (but I know most German cars are mechanical car for green fuel reason which does have 3 pedals.
Steering wheel is current implementation, but there are researches inventing automatic controlled cars that is fully controlled by computer, and I am not sure if trolley car is a car? If it is, it can work without steering wheel though it may have today. 


Above comments doesn;t mean I against your abstract of "car with 3 pedals and a steering wheeling" even it may be not accurate enough. But it does reflect the trade off between innovation and industry standardization. W/o standaradization, there are many variants coming in to compete in different area, but once a standardization comes, the innovation is deprioritized, rather other competition comes such as price. Hardly we can see if we need innovation more (no standardization) or cost saving more. But at very beginning of a new product/feature, I think innovation is more important to allow different variants to compete. 

Taking software or nested virtualization as example, defining those wrappers or abstract to force each vendor to follow does deprioritize innovation, but they may be easier for followers to understand easily if they doesn't understand SVM/VMX itself very well. However, as I said, nested virtualization is still at its very beginning and we need innovation more to find what is the best solution especially from performance point of view. Otherwise re-abstract does require double effort for VMX developers to consider SVM support as well (and even more than double effort because normally Intel VMX developers doesn't know SVM architecture very well and we don't have test machine for SVM).

Anyway, the ball is in your side.

Thx, Eddie

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

[Christoph Egger]

On Thursday 09 September 2010 03:45:55 Dong, Eddie wrote:
> Well, so you decide to cycle here, though it obviously not only hurt Intel
> but also AMD's progress.
>
> Let me ask several questions first:
> 1: Does this heavy weight wrapper a must for nested virtualization?

It's not a question of a must. It's a question of design.

> 2: What is the gain and lose?

in what respect ?

> 3: How single layer virtualization does? Are they abstracting with heavy
> weight wrapper or light weight and why?

Maybe you should explain what you mean by 'heavy weight' and 'light weight'
wrapper.

By software concept, a wrapper is a function that implements one interface by
calling an other interface. The purpose is to provide/emulate yet another API.
Example: In NetBSD, the Linux syscalls are implemented by calling the
native sycalls. This allows to run Linux binaries on NetBSD.

In that sense, the answer to your question is "Neither nor".
It just puts common logic into one place that has been found over time.

> Particularly do they abstract the 1st level VM exit/entry event or do they
> invent 3rd name space for VMCS/VMCB field?

Yes, there is the guest_cr array for use in generic code, for example.


> The only reason I can see so far is that you called out the "abstract" in
> nested virtualization side, that is for some reason we have to appreciate
> but not enough for us to swallow the side effect of its impact to future
> nested VMX development.

You are worried about that the interface is fixed, right?

Note that this interface is an internal one. In opposition to an external
interface (xen/include/public/*) you do not need to care about backward
compatibility.

Common to an internal and an external interface are:
- care about not breaking what has worked before
- think first before doing a change what for, if it is really needed
   and that it makes really sense to avoid make it go wild

One example for a wild gone interface are the hypercalls
to get/set C- and P-States in Xen. When I want to start implementing
the feature in a Dom0 from scratch, I do not know which ones I should use.

Tim said in
http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01044.html
"I've no strong feelings about the details of the interface between the
common and the arch-specific code, but it seems like with a bit of
flexibility on both sides it could be made suitable for everyone."

Your point about standardization from below comes in here, too.


> Taking your "car" abstract as example,
>
> > All cars are different but they all have three pedals and a steering
> > wheel.
>
> Well, automatic cars have 2 pedals only (but I know most German cars are
> mechanical car for green fuel reason which does have 3 pedals.
> Steering wheel is current implementation, but there are researches inventing
> automatic controlled cars that is fully controlled by computer, and I am
> not sure if trolley car is a car? If it is, it can work without steering
> wheel though it may have today.

Having 2 or 3 pedals or a steering wheel or not, here by concept
the question is who *uses* the interface.

Switching the gears and the capability to turn left/right is still there, 
because the implementation of the same interface still exists.

The difference is that who can see and use which interface.
Who switches the gears and who turns left/right
- is it the human or a computer ?


You also mentioned research: You know that doing research means to develop
prototypes ?  Prototypes may be but must not be production stable.
The purpose for writing prototypes is figure out how things work and
what problems may come up.

In software engineering, prototypes are something that get thrown away to
implement the production version from scratch where you know the
problems ahead.

Below you say, nested virtualization is at the very beginning. That's right
for the code, the idea is old (Goldberg's paper).

If you want to call the patch series a prototype then that means that
all of our code needs to be thrown away and we both must start from
scratch. Do you really want that?

>
> Above comments doesn;t mean I against your abstract of "car with 3 pedals
> and a steering wheeling" even it may be not accurate enough. But it does
> reflect the trade off between innovation and industry standardization. W/o
> standaradization, there are many variants coming in to compete in different
> area, but once a standardization comes, the innovation is deprioritized,
> rather other competition comes such as price. Hardly we can see if we need
> innovation more (no standardization) or cost saving more. But at very
> beginning of a new product/feature, I think innovation is more important to
> allow different variants to compete.

I disagree in that standardization and innovation are exclusive on each other.
A community consisting full of software engineers show again and again
that there are synergy effects:

Example: In NetBSD, the access to  a bus are standardized through an interface 
(http://netbsd.gw.com/cgi-bin/man-cgi?bus_space++NetBSD-current).
The drivers have a frontend and a backend. The frontend attaches
the driver onto the pci, isa, whatever bus. The backend contains all the
code using the interface, there's no code specific to pci, isa, etc.

Now when you come up with a new and innovative bus for the PC,
then you can add support for it by implementing the interface.
Instead of writing/porting all drivers to the new bus you go the other
way: You use the existing drivers to test if your bus implementation
works. And if you find a bug in the driver then the fix is not only good
for the new innovative bus.

> Taking software or nested virtualization as example, defining those
> wrappers or abstract to force each vendor to follow does deprioritize
> innovation, but they may be easier for followers to understand easily if
> they doesn't understand SVM/VMX itself very well.

I do not say you are wrong here. The sad thing is that it doesn't
work in reality.

You said in
http://lists.xensource.com/archives/html/xen-devel/2010-08/msg01007.html

"Drawing a picture which doesn't have a real usage with massive common code 
change is a kind of too much load for us now. Xen hvm_function table is a good 
example. Intel enabled VMX at very beginning of Xen HVM support, and SVM comes 
later on with a lot of code reuse from VMX side. Then the community and both 
side work together to make an API wrapper to reuse common code better and 
accomodate both architecture. I think we have to go similar path to make it 
work first."

I looked through the changesets where xen/arch/x86/hvm/hvm.c
was involved (hg log -v xen/arch/x86/hvm/hvm.c). I did not find a
changeset from Intel where code from SVM and VMX has moved into
hvm.c. So "both side work together" does not apply here.
Factoring code out from VMX was a very hard time for AMD developers
and that time is still not over.

Tim said in
http://lists.xensource.com/archives/html/xen-devel/2010-04/msg00812.html

"I would like to see some reasonable discussion of exactly how much of
the nested virtualization logic can be shared.  With HVM it has turned
out to be quite a lot, but it's taken years of reshuffling to pull code
out into common HVM routines (and we're not there yet)."

I do not want to repeat that again with nested virtualization.

Later when the nested virtualization becomes major where
the 'standardization' matters according to you, I doubt that
Intel developers will take account for it for two reasons:

a) Almost every patch from Intel that touches a file under
xen/include/public/* breaks backward-compatibility.
b) The fact of rare if not missing Intel patches where code from SVM and VMX
moves into hvm.c w/o breaking anything.

That all speaks a lot for pulling as much logic as possible
into common code.

> However, as I said, nested virtualization is still at its very beginning and
> we need innovation more to find what is the best solution especially from
> performance point of view.

Well, that is the research aspect with prototypes I mentioned above.

> Otherwise re-abstract does require double effort for VMX developers 
> to consider SVM support as well

At least in thinking, but not necessarily in coding. The result is higher code
quality that makes this effort worth. Keir will more often apply patches
w/o asking you to fix/re-work things first.

> (and even more than double effort because normally Intel VMX developers
> doesn't know SVM architecture very well

Not knowing something does not mean being stupid. There are manuals
to learn from.

> and we don't have test machine for SVM). 

You surely can afford some AMD machines given the much higher revenue
Intel has over AMD.

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

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

[Tim Deegan]

Hi, 

Christoph, I've posted separate comments on patches 5, 12 and 13.
Otherwise this series is looking OK to me -- provided that you and Eddie
can agree on how the common code is going to work.

Eddie - I don't think having a common vmexit interface _requires_ you to
do 17 VMREADs on every VMEXIT (and if it does, that needs to be
changed).  The SVM implementation may pull all the information out at
once but the VMX one could do it lazily.

Christoph, I asked before about whether we could lose the extra
namespace for exit reasons.  Looking at the way they're used, couldn't
we just pass around the native numbers and maybe some flags for which of
the ancillary fields are valid?

Cheers,

Tim.


At 15:53 +0100 on 01 Sep (1283356406), Christoph Egger wrote:
> 
> Hi!
> 
> This patch series brings Nested Virtualization to Xen.
> This is the third patch series. Improvements to the
> previous patch submission:
> 
> - Incorporated feedback discussion with Keir, Tim and Eddie.
> 
> There might still be some nuances to fiddle with to make it
> fit for VMX. Feedback from Intel is appreciated, therefore.
> 
> 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.
> 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).
> 
> 
> -- 
> ---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
> 

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

[PATCH 00/13] Nested Virtualization: Overview

[Christoph Egger]

Hi!

This patch series brings Nested Virtualization to Xen.
This is the fifth patch series. Improvements to the
previous patch submission:

- Incorporated feedback from discussion with Tim and Eddie.
- Reworked common data structure and function hooks (patch #2 and #3)
   Adjusted all patches affected by this.
- Make use of hvm_map_guest_frame() introduced in c/s 22228:02e199c96ece
- Removed workaround in hap-on-hap patch thanks to c/s 22215:c8331262efe6
- Some bugfixes


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


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

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

[Dong, Eddie]

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

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

[Christoph Egger]

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

[PATCH 00/13] Nested Virtualization: Overview

[Christoph Egger]

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


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

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

[Tim Deegan]

Hi Christoph,

Thanks for these; I've commented on the individual patches, but mostly
the core nested hvm and nested svm looks like it's in good shape.  The
nested HAP code still needs work based on the last review, though. 

Ccing Eddie for his opinion on the common code (patches 1 to 8).

Cheers,

Tim.

At 18:39 +0000 on 12 Nov (1289587157), 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).
> 
> 
> -- 
> ---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@lists.xensource.com
> http://lists.xensource.com/xen-devel

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

[Dong, Eddie]

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?


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


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

Thx, Eddie

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

[Christoph Egger]

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

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

[Bruce Edge]

[-- Attachment #1.1: Type: text/plain, Size: 2980 bytes --]

On Fri, Nov 12, 2010 at 10:39 AM, Christoph Egger
<Christoph.Egger@amd.com>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).
>
>
> --
> ---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@lists.xensource.com
> http://lists.xensource.com/xen-devel
>


I'd like to ask a general question on nested virtualization. Can an L1 or
L>1 VM span multiple physical machines?
I'm guessing not from the pdf,
http://lists.xensource.com/archives/html/xen-devel/2010-04/pdfsVHwQIeZLB.pdf,
but I wanted to ask and confirm because if so, this would be a fantastic way
to segregate individual customer infrastructures in a cloud environment.

Thanks

-Bruce

[-- Attachment #1.2: Type: text/html, Size: 3731 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

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

[Christoph Egger]

On Tuesday 30 November 2010 18:20:40 Bruce Edge wrote:
> On Fri, Nov 12, 2010 at 10:39 AM, Christoph Egger
> <Christoph.Egger@amd.com<mailto:Christoph.Egger@amd.com>> 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).
>
>
> --
> ---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@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> http://lists.xensource.com/xen-devel
>
>
> I'd like to ask a general question on nested virtualization. Can an L1 or
> L>1 VM span multiple physical machines?

I guess you mean virtual machines here. The answer is yes.

> I'm guessing not from the pdf, 
> http://lists.xensource.com/archives/html/xen-devel/2010-04/pdfsVHwQIeZLB.pd
>f, but I wanted to ask and confirm because if so, this would be a fantastic
> way to segregate individual customer infrastructures in a cloud
> environment.

I do not consider the patch series production stable but I would be glad when 
you can help with testing.

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

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

[Shriram Rajagopalan]

[-- Attachment #1.1: Type: text/plain, Size: 4161 bytes --]

Following up on Bruce's question..
 What do you mean by VM spanning multiple physical machines?
 do you mean the ability to migrate VMs across hypervisor levels, across one
or more machines?

shriram
On Wed, Dec 1, 2010 at 1:08 AM, Christoph Egger <Christoph.Egger@amd.com>wrote:

> On Tuesday 30 November 2010 18:20:40 Bruce Edge wrote:
> > On Fri, Nov 12, 2010 at 10:39 AM, Christoph Egger
> > <Christoph.Egger@amd.com<mailto:Christoph.Egger@amd.com>> 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).
> >
> >
> > --
> > ---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@lists.xensource.com<mailto:Xen-devel@lists.xensource.com>
> > http://lists.xensource.com/xen-devel
> >
> >
> > I'd like to ask a general question on nested virtualization. Can an L1 or
> > L>1 VM span multiple physical machines?
>
> I guess you mean virtual machines here. The answer is yes.
>
> > I'm guessing not from the pdf,
> >
> http://lists.xensource.com/archives/html/xen-devel/2010-04/pdfsVHwQIeZLB.pd
> >f, but I wanted to ask and confirm because if so, this would be a
> fantastic
> > way to segregate individual customer infrastructures in a cloud
> > environment.
>
> I do not consider the patch series production stable but I would be glad
> when
> you can help with testing.
>
> 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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>



-- 
perception is but an offspring of its own self

[-- Attachment #1.2: Type: text/html, Size: 5643 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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