Re: [PATCH 3/3] nVMX: Advertise PML to L1 hypervisor

[Paolo Bonzini <pbonzini@redhat.com>]

On 04/05/2017 20:22, Bandan Das wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 04/05/2017 00:14, Bandan Das wrote:
>>> Advertise the PML bit in vmcs12 but clear it out
>>> before running L2 since we don't depend on hardware support
>>> for PML emulation.
>>>
>>> Signed-off-by: Bandan Das <bsd@redhat.com>
>>> ---
>>>  arch/x86/kvm/vmx.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 5e5abb7..df71116 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -2763,8 +2763,11 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>>>  		vmx->nested.nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT |
>>>  			VMX_EPT_EXTENT_CONTEXT_BIT | VMX_EPT_2MB_PAGE_BIT |
>>>  			VMX_EPT_1GB_PAGE_BIT;
>>> -	       if (enable_ept_ad_bits)
>>> +		if (enable_ept_ad_bits) {
>>> +			vmx->nested.nested_vmx_secondary_ctls_high |=
>>> +				SECONDARY_EXEC_ENABLE_PML;
>>>  		       vmx->nested.nested_vmx_ept_caps |= VMX_EPT_AD_BIT;
>>> +		}
>>>  	} else
>>>  		vmx->nested.nested_vmx_ept_caps = 0;
>>>  
>>> @@ -10080,6 +10083,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>>>  		if (exec_control & SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)
>>>  			vmcs_write64(APIC_ACCESS_ADDR, -1ull);
>>>  
>>> +		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>>>  		vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
>>
>> L0 is still using its own page modification log when running L2, so you
>> have to clear the bit here instead:
>>
>>             exec_control |= vmcs12->secondary_vm_exec_control;
>>
> 
> Oops, good catch, thank you!
> 
>> and set up PML_ADDRESS and GUEST_PML_INDEX.  Though, the lack of
>> PML_ADDRESS and GUEST_PML_INDEX initialization is a pre-existing bug.
> 
> A little further down I see that these fields are being reset as part of
> commit 1fb883bb827:
> ...
> 	if (enable_pml) {
> 		/*
> 		 * Conceptually we want to copy the PML address and index from
> 		 * vmcs01 here, and then back to vmcs01 on nested vmexit. But,
> 		 * since we always flush the log on each vmexit, this happens
> 		 * to be equivalent to simply resetting the fields in vmcs02.
> 		 */
> 		ASSERT(vmx->pml_pg);
> 		vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
> 		vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
> 	}
> 
> Or are you referring to a different place, these fields need to be set ?

Oh, I missed that, I was looking at an old tree.  That's better, thanks. :)

Paolo

> 
> 
>> Paolo
>>
>>>  	}
>>>  
>>>