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

[Bandan Das <bsd@redhat.com>]

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 ?


> Paolo
>
>>  	}
>>  
>>