| Message ID | 1501161639-9707-1-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@redhat.com> > --- > 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!
----- Original Message ----- > From: "David Hildenbrand" <david@redhat.com> > To: "Paolo Bonzini" <pbonzini@redhat.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org > 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@redhat.com> > > --- > > 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 >
On 27/07/2017 20:29, David Hildenbrand wrote: > 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@redhat.com> >> --- >> 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 ?) I'm used to do return first_long_condition && second_long_condition; but I admit it looks a bit weird with 8-space indent. >> 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 :) In general the compiler complains about those things, so I didn't add the parentheses here. In can see why it doesn't, because & ^ | are consistently above && ||. The problems in general stem from the precedence between & ^ | and && ||. >> + 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) >> + 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); > } I don't know... if you need a comment, it means the different structure of the code doesn't spare many doubts from the reader. And the code doesn't become much simpler since you have to handle "nested" anyway. What I tried to do was to mimic as much as possible the rdtscp case, but it cannot be exactly the same due to the interaction between PCID and INVPCID. Paolo
>> 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); >> } > > I don't know... if you need a comment, it means the different structure > of the code doesn't spare many doubts from the reader. And the code > doesn't become much simpler since you have to handle "nested" anyway. > What I tried to do was to mimic as much as possible the rdtscp case, but > it cannot be exactly the same due to the interaction between PCID and > INVPCID. It's more about the handling of best here, which can be avoided quite easily as I showed (by encapsulating how cpuids are looked up/modified). But you are the maintainer, so feel free to stick to what you have. :) > > Paolo >
On 01/08/2017 13:18, David Hildenbrand wrote: > >>> 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); >>> } >> >> I don't know... if you need a comment, it means the different structure >> of the code doesn't spare many doubts from the reader. And the code >> doesn't become much simpler since you have to handle "nested" anyway. >> What I tried to do was to mimic as much as possible the rdtscp case, but >> it cannot be exactly the same due to the interaction between PCID and >> INVPCID. > > It's more about the handling of best here, which can be avoided quite > easily as I showed (by encapsulating how cpuids are looked up/modified). Yeah, I don't like either option. :) Luckily there is a second maintainer! Paolo
2017-08-01 13:35+0200, Paolo Bonzini: > On 01/08/2017 13:18, David Hildenbrand wrote: > > > >>> 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); > >>> } > >> > >> I don't know... if you need a comment, it means the different structure > >> of the code doesn't spare many doubts from the reader. And the code > >> doesn't become much simpler since you have to handle "nested" anyway. > >> What I tried to do was to mimic as much as possible the rdtscp case, but > >> it cannot be exactly the same due to the interaction between PCID and > >> INVPCID. > > > > It's more about the handling of best here, which can be avoided quite > > easily as I showed (by encapsulating how cpuids are looked up/modified). > > Yeah, I don't like either option. :) Luckily there is a second maintainer! With three people, we'll just have three options! :) I'd go with Paolo's version, because it is efficient and also follows patterns the bit clearing in kvm_update_cpuid(). --- I would like the wrappers if they were more generic, e.g.: guest_cpuid_has(vcpu, X86_FEATURE_INVPCID); guest_cpuid_clear(vcpu, X86_FEATURE_INVPCID); To do that, we need a mapping from X86_FEATURE to (leaf, subleaf, register) triple, which can be done with cpuid_leafs. I haven't tried to compile the idea below, but if the optimizer is good, then we should have only slightly worse byte code as we do now thanks to x86_feature being a compile-time constant. Do you it's less ugly than the other two options? static inline bool x86_feature_cpuid(int x86_feature, int *id, int *count, int *register) { static struct {int x86_leaf; int id; int count; int register;} cpuid[] = { {CPUID_1_EDX, 1, 0, 3}, {CPUID_8000_0001_EDX, 0x80000001, 0, 3}, {CPUID_8086_0001_EDX, 0x80860001, 0, 3}, {CPUID_1_ECX, 1, 0, 2}, ... // you get the idea, it's tedious :) }; for (size_t i = 0; i < ARRAY_SIZE(cpuid); i++) if (cpuid[i].x86_leaf == x86_feature / 32) { *id = cpuid[i].id; *count = cpuid[i].count; *register = cpuid[i].register; return true; } return false; } static inline bool guest_cpuid_has(struct kvm_vcpu *vcpu, int x86_feature) { int id, count, register; u32 *reg; struct kvm_cpuid_entry2 *best; if (!x86_feature_cpuid(x86_feature, &id, &count, ®ister)) return false; if (!(best = kvm_find_cpuid_entry(id, count))) return false; reg = &best->eax + register; // rewrite to be safer return *reg & bit(x86_feature); } Similar for guest_cpuid_clear and we can also factor out the common part for getting the register (all up to the last line, with NULL as failure)
On 01/08/2017 19:37, Radim Krčmář wrote: > > Do you it's less ugly than the other two options? It's awesome, but it's a non-trivial project of its own. :) Paolo
On 02.08.2017 09:38, Paolo Bonzini wrote: > On 01/08/2017 19:37, Radim Krčmář wrote: >> >> Do you it's less ugly than the other two options? > > It's awesome, but it's a non-trivial project of its own. :) > > Paolo > That would be a perfect cleanup and I'll be happy to review it ;) ... until then, Paolo's patch in the current form is fine from my side.
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); 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) && + 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) + best->ebx &= ~bit(X86_FEATURE_INVPCID); + } + + 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);
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. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/vmx.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)