Re: [PATCH] MSR_EFER acceleration for lighweight VM Exit

Avi Kivity
2007-05-20T07:28:12Z

Dong, Eddie wrote:
> Avi:
>
> This patch is to avoid saving and restoring of msr_efer on
> lightweight vmexit.
>
> With this patch, the Kernel build get 10% increasement for
> 64bits on 64 bits,
> and 5-8% increasement for 32bits on 64 bits.
> Vmexit.flat can see ~1185 cycles save for 64 bits on 64 bits,
> which is 29%
> save of total VM Exit. It can save ~1206 cycles for 32bits on
> 64bits, which is
> 27.5% save of total.
> BTW, I use 4 core clovertown processor, the total VM Exit time
> reported by
> vmexit.flat is about 2860 cycles now.
>
>

Very impressive speedup!

Comments below.

>
> +void cpuid_fix_nx_cap(struct kvm_vcpu *vcpu)
>

static function, please.

> +{
> + __u64 efer;
> +
> + rdmsrl(MSR_EFER, efer);
> + if ((vcpu->cpuid_nent >= 1) && !(efer & EFER_NX)) {
> + vcpu->cpuid_entries[1].edx &= ~(1<<20);
>

Nothing guarantees that function 1 is in entry 1. You need to iterate
over the loop.

> + printk(KERN_INFO ": guest NX capibility removed\n");
>

The warning should be printed only if the capability is actually present
in the cpuid_entry. Otherwise non-nx capable processors will always
show the warning.

> @@ -41,6 +41,7 @@ static struct page *vmx_io_bitmap_b;
> #else
> #define HOST_IS_64 0
> #endif
> +#define EFER_SR_BITS EFER_SCE
>

I'm guessing "SR" is save/restore? Please use the full name.

>
> static struct vmcs_descriptor {
> int size;
> @@ -56,6 +57,10 @@ static struct vmcs_descriptor {
> .ar_bytes = GUEST_##seg##_AR_BYTES, \
> }
>
> +#define efer_msr_srbits_changed(vcpu)
> \
> + ((vcpu->host_msrs[vcpu->msr_offset_efer].data & EFER_SR_BITS)!=
> \
> + (vcpu->guest_msrs[vcpu->msr_offset_efer].data & EFER_SR_BITS))
>

static function instead of macro.

> +
> static struct kvm_vmx_segment_field {
> unsigned selector;
> unsigned base;
> @@ -264,6 +269,18 @@ static void reload_tss(void)
> #endif
> }
>
> +void load_transition_efer(struct kvm_vcpu *vcpu)
>

static function

> +{
> + __u64 msr_efer_trans;
>

kernel code should use u64, not __u64. __u64 is for kernel code that is
shared with userspace.

>
> static void vmx_load_host_state(struct kvm_vcpu *vcpu)
> @@ -335,6 +354,8 @@ static void vmx_load_host_state(struct kvm_vcpu
> *vcpu)
> }
> save_msrs(vcpu->guest_msrs, vcpu->save_nmsrs);
> load_msrs(vcpu->host_msrs, vcpu->save_nmsrs);
> + if (efer_msr_srbits_changed(vcpu))
> + load_msrs(vcpu->host_msrs+vcpu->msr_offset_efer, 1);
>

space around '+'.

--
error compiling committee.c: too many arguments to function