2022-10-01 01:15:30

by Sean Christopherson

Subject: [PATCH v4 27/32] KVM: SVM: Update svm->ldr_reg cache even if LDR is "bad"

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 <[email protected]>
---
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;
+
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)
--
2.38.0.rc1.362.ged0d419d3c-goog

2022-12-08 22:40:31

by Maxim Levitsky

Subject: Re: [PATCH v4 27/32] KVM: SVM: Update svm->ldr_reg cache even if LDR is "bad"

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 <[email protected]>
> ---
> 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 <[email protected]>


Best regards,
Maxim Levitsky <[email protected]>

2022-12-09 01:15:58

by Sean Christopherson

Subject: Re: [PATCH v4 27/32] KVM: SVM: Update svm->ldr_reg cache even if LDR is "bad"

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 <[email protected]>
> > ---
> > 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.