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

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

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