On Wed, Jan 28, 2015 at 4:00 PM, Zhang, Yang Z <yang.z.zhang@xxxxxxxxx> wrote: >> @@ -5812,13 +5813,18 @@ static __init int hardware_setup(void) >> (unsigned long >> *)__get_free_page(GFP_KERNEL); >> if (!vmx_msr_bitmap_longmode_x2apic) >> goto out4; >> + >> + vmx_msr_bitmap_nested = (unsigned long >> *)__get_free_page(GFP_KERNEL); >> + if (!vmx_msr_bitmap_nested) >> + goto out5; >> + > > Since the nested virtualization is off by default. It's better to allocate the page > only when nested is true. Maybe adding the following check is better: > > if (nested) { > vmx_msr_bitmap_nested = (unsigned long *)__get_free_page(GFP_KERNEL); > if (!vmx_msr_bitmap_nested) > goto out5; > } Agreed. Will do. > > ...snip... > >> + >> +/* >> + * Merge L0's and L1's MSR bitmap, return false to indicate that >> + * we do not use the hardware. >> + */ >> +static inline bool nested_vmx_merge_msr_bitmap(struct kvm_vcpu *vcpu, >> + struct vmcs12 >> *vmcs12) { >> + return false; >> +} >> + > > The following patches have nothing to do with the MSR control. Why leave the function empty here? > No. In patch "Enable nested virtualize x2apic mode", we will return false if L1 disabled virt_x2apic_mode, then the hardware MSR-bitmap control is disabled. This is faster than rebuilding the vmx_msr_bitmap_nested. This function returns false here to indicate that we do not use the hardware. Since It is not only virtualize x2apic mode using this, other features may use this either. I think it isn't suitable to introduce this function in other patches. > Best regards, > Yang > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html