On 27.07.2017 15:20, Paolo Bonzini wrote:
> Expose the "Enable INVPCID" secondary execution control to the guest
> and properly reflect the exit reason.
>
> In addition, before this patch the guest was always running with
> INVPCID enabled, causing pcid.flat's "Test on INVPCID when disabled"
> test to fail.
Did you wanted to send "KVM: nVMX: do not fill vm_exit_intr_error_code
in prepare_vmcs12" I can spot on kvm/queue? (but sent this patch twice
instead?)
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 34 +++++++++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ed43fd824264..9c3c6c524430 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8189,6 +8189,10 @@ static bool nested_vmx_exit_reflected(struct kvm_vcpu *vcpu, u32 exit_reason)
> * table is L0's fault.
> */
> return false;
> + case EXIT_REASON_INVPCID:
> + return
> + nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_INVPCID) &&
> + nested_cpu_has(vmcs12, CPU_BASED_INVLPG_EXITING);
(why the extra line after the return ?)
> case EXIT_REASON_WBINVD:
> return nested_cpu_has2(vmcs12, SECONDARY_EXEC_WBINVD_EXITING);
> case EXIT_REASON_XSETBV:
> @@ -9440,7 +9444,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
>
> static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> {
> - struct kvm_cpuid_entry2 *best;
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
>
> @@ -9459,15 +9462,27 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> }
> }
>
> - /* Exposing INVPCID only when PCID is exposed */
> - best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> - if (vmx_invpcid_supported() &&
> - (!best || !(best->ebx & bit(X86_FEATURE_INVPCID)) ||
> - !guest_cpuid_has_pcid(vcpu))) {
> - secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> + if (vmx_invpcid_supported()) {
> + /* Exposing INVPCID only when PCID is exposed */
> + struct kvm_cpuid_entry2 *best = kvm_find_cpuid_entry(vcpu, 0x7, 0);
> + bool invpcid_enabled =
> + best && best->ebx & bit(X86_FEATURE_INVPCID) &&
I thought parentheses are recommended around &, but I am usually wrong
about these things :)
> + guest_cpuid_has_pcid(vcpu);
>
> - if (best)
> - best->ebx &= ~bit(X86_FEATURE_INVPCID);
> + if (!invpcid_enabled) {
> + secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
> + if (best)
(thinking loud: only relevant if !guest_cpuid_has_pcid(vcpu))
> + best->ebx &= ~bit(X86_FEATURE_INVPCID);
> + }
Can't we rewrite that a little bit, avoiding that "best" handling
(introducing guest_cpuid_disable_invpcid() and guest_cpuid_has_invpcid())
bool invpcid_enabled = guest_cpuid_has_pcid(vcpu) &&
guest_cpuid_has_invpcid();
if (!invpcid_enabled) {
secondary_exec_ctl &= ~SECONDARY_EXEC_ENABLE_INVPCID;
/* make sure there is no no INVPCID without PCID */
guest_cpuid_disable_invpcid(vcpu);
}
if (nested) {
...
> +
> + if (nested) {
> + if (invpcid_enabled)
> + vmx->nested.nested_vmx_secondary_ctls_high |=
> + SECONDARY_EXEC_ENABLE_INVPCID;
> + else
> + vmx->nested.nested_vmx_secondary_ctls_high &=
> + ~SECONDARY_EXEC_ENABLE_INVPCID;
> + }
> }
>
> if (cpu_has_secondary_exec_ctrls())
> @@ -10175,6 +10190,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
>
> /* Take the following fields only from vmcs12 */
> exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> + SECONDARY_EXEC_ENABLE_INVPCID |
> SECONDARY_EXEC_RDTSCP |
> SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
> SECONDARY_EXEC_APIC_REGISTER_VIRT);
>
Makes sense to me!
--
Thanks,
David
