| Message ID | 20170503221457.18869-4-bsd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; 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. Paolo > } > >
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 > >> } >> >>
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 >> >>> } >>> >>>
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); }
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(-)