----- Original Message -----
> From: "David Hildenbrand" <david@xxxxxxxxxx>
> To: "Paolo Bonzini" <pbonzini@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, kvm@xxxxxxxxxxxxxxx
> Sent: Thursday, July 27, 2017 8:29:21 PM
> Subject: Re: [PATCH] KVM: nVMX: INVPCID support
>
> 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?)
Yes, but it was two separate mistakes not one. :)
I forgot I had sent this one already *and* I also forgot to send the other.
Paolo
> >
> > 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
>
