| Message ID | 20221001005915.2041642-28-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: AVIC and local APIC fixes+cleanups | expand |
On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote: > Update SVM's cache of the LDR even if the new value is "bad". Leaving > stale information in the cache can result in KVM missing updates and/or > invalidating the wrong entry, e.g. if avic_invalidate_logical_id_entry() > is triggered after a different vCPU has "claimed" the old LDR. > > Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/avic.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index 2b640c73f447..4b6fc9d64f4d 100644 > --- a/arch/x86/kvm/svm/avic.c > +++ b/arch/x86/kvm/svm/avic.c > @@ -539,23 +539,24 @@ static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat) > return &logical_apic_id_table[index]; > } > > -static int avic_ldr_write(struct kvm_vcpu *vcpu, u8 g_physical_id, u32 ldr) > +static void avic_ldr_write(struct kvm_vcpu *vcpu, u8 g_physical_id, u32 ldr) > { > bool flat; > u32 *entry, new_entry; > > + if (!ldr) > + return; > + The avic_get_logical_id_entry already returns NULL in this case, so I don't think that this is needed. > flat = kvm_lapic_get_reg(vcpu->arch.apic, APIC_DFR) == APIC_DFR_FLAT; > entry = avic_get_logical_id_entry(vcpu, ldr, flat); > if (!entry) > - return -EINVAL; > + return; > > new_entry = READ_ONCE(*entry); > new_entry &= ~AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK; > new_entry |= (g_physical_id & AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK); > new_entry |= AVIC_LOGICAL_ID_ENTRY_VALID_MASK; > WRITE_ONCE(*entry, new_entry); > - > - return 0; > } > > static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu) > @@ -575,7 +576,6 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu) > > static void avic_handle_ldr_update(struct kvm_vcpu *vcpu) > { > - int ret = 0; > struct vcpu_svm *svm = to_svm(vcpu); > u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR); > u32 id = kvm_xapic_id(vcpu->arch.apic); > @@ -589,11 +589,8 @@ static void avic_handle_ldr_update(struct kvm_vcpu *vcpu) > > avic_invalidate_logical_id_entry(vcpu); > > - if (ldr) > - ret = avic_ldr_write(vcpu, id, ldr); > - > - if (!ret) > - svm->ldr_reg = ldr; > + svm->ldr_reg = ldr; > + avic_ldr_write(vcpu, id, ldr); > } > > static void avic_handle_dfr_update(struct kvm_vcpu *vcpu) With the nitpick fixed: Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky <mlevitsk@redhat.com>
On Thu, Dec 08, 2022, Maxim Levitsky wrote: > On Sat, 2022-10-01 at 00:59 +0000, Sean Christopherson wrote: > > Update SVM's cache of the LDR even if the new value is "bad". Leaving > > stale information in the cache can result in KVM missing updates and/or > > invalidating the wrong entry, e.g. if avic_invalidate_logical_id_entry() > > is triggered after a different vCPU has "claimed" the old LDR. > > > > Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC") > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/svm/avic.c | 17 +++++++---------- > > 1 file changed, 7 insertions(+), 10 deletions(-) > > > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > > index 2b640c73f447..4b6fc9d64f4d 100644 > > --- a/arch/x86/kvm/svm/avic.c > > +++ b/arch/x86/kvm/svm/avic.c > > @@ -539,23 +539,24 @@ static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat) > > return &logical_apic_id_table[index]; > > } > > > > -static int avic_ldr_write(struct kvm_vcpu *vcpu, u8 g_physical_id, u32 ldr) > > +static void avic_ldr_write(struct kvm_vcpu *vcpu, u8 g_physical_id, u32 ldr) > > { > > bool flat; > > u32 *entry, new_entry; > > > > + if (!ldr) > > + return; > > + > The avic_get_logical_id_entry already returns NULL in this case, so I don't think > that this is needed. Hmm, yeah, it's not strictly needed. I think I kept it because of the explicit check that previously existed in avic_handle_ldr_update(). Part of me likes the explicit check, but avic_invalidate_logical_id_entry() doesn't manually guard against ldr==0, to I agree it's better to drop this for consistency.
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 2b640c73f447..4b6fc9d64f4d 100644 --- a/arch/x86/kvm/svm/avic.c +++ b/arch/x86/kvm/svm/avic.c @@ -539,23 +539,24 @@ static u32 *avic_get_logical_id_entry(struct kvm_vcpu *vcpu, u32 ldr, bool flat) return &logical_apic_id_table[index]; } -static int avic_ldr_write(struct kvm_vcpu *vcpu, u8 g_physical_id, u32 ldr) +static void avic_ldr_write(struct kvm_vcpu *vcpu, u8 g_physical_id, u32 ldr) { bool flat; u32 *entry, new_entry; + if (!ldr) + return; + flat = kvm_lapic_get_reg(vcpu->arch.apic, APIC_DFR) == APIC_DFR_FLAT; entry = avic_get_logical_id_entry(vcpu, ldr, flat); if (!entry) - return -EINVAL; + return; new_entry = READ_ONCE(*entry); new_entry &= ~AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK; new_entry |= (g_physical_id & AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK); new_entry |= AVIC_LOGICAL_ID_ENTRY_VALID_MASK; WRITE_ONCE(*entry, new_entry); - - return 0; } static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu) @@ -575,7 +576,6 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu) static void avic_handle_ldr_update(struct kvm_vcpu *vcpu) { - int ret = 0; struct vcpu_svm *svm = to_svm(vcpu); u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR); u32 id = kvm_xapic_id(vcpu->arch.apic); @@ -589,11 +589,8 @@ static void avic_handle_ldr_update(struct kvm_vcpu *vcpu) avic_invalidate_logical_id_entry(vcpu); - if (ldr) - ret = avic_ldr_write(vcpu, id, ldr); - - if (!ret) - svm->ldr_reg = ldr; + svm->ldr_reg = ldr; + avic_ldr_write(vcpu, id, ldr); } static void avic_handle_dfr_update(struct kvm_vcpu *vcpu)
Update SVM's cache of the LDR even if the new value is "bad". Leaving stale information in the cache can result in KVM missing updates and/or invalidating the wrong entry, e.g. if avic_invalidate_logical_id_entry() is triggered after a different vCPU has "claimed" the old LDR. Fixes: 18f40c53e10f ("svm: Add VMEXIT handlers for AVIC") Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/avic.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)