Re: [PATCH 06/19] KVM: SVM: Get x2APIC logical dest bitmap from ICRH[15:0], not ICHR[31:16]

[Maxim Levitsky]

On Wed, 2022-08-31 at 16:35 +0000, Sean Christopherson wrote:
> On Wed, Aug 31, 2022, Maxim Levitsky wrote:
> > On Wed, 2022-08-31 at 09:09 +0300, Maxim Levitsky wrote:
> > > On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> > > > When attempting a fast kick for x2AVIC, get the destination bitmap from
> > > > ICR[15:0], not ICHR[31:16].  The upper 16 bits contain the cluster, the
> > > > lower 16 bits hold the bitmap.
> > > > 
> > > > Fixes: 603ccef42ce9 ("KVM: x86: SVM: fix avic_kick_target_vcpus_fast")
> > > > Cc: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> > > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > > ---
> > > >  arch/x86/kvm/svm/avic.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > > index 3ace0f2f52f0..3c333cd2e752 100644
> > > > --- a/arch/x86/kvm/svm/avic.c
> > > > +++ b/arch/x86/kvm/svm/avic.c
> > > > @@ -368,7 +368,7 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
> > > >  
> > > >  		if (apic_x2apic_mode(source)) {
> > > >  			/* 16 bit dest mask, 16 bit cluster id */
> > > > -			bitmap = dest & 0xFFFF0000;
> > > > +			bitmap = dest & 0xFFFF;
> > > >  			cluster = (dest >> 16) << 4;
> > > >  		} else if (kvm_lapic_get_reg(source, APIC_DFR) == APIC_DFR_FLAT) {
> > > >  			/* 8 bit dest mask*/
> > > 
> > > I swear I have seen a patch from Suravee Suthikulpanit fixing this my mistake, I don't know why it was not
> > > accepted upstream.
> > 
> > This is the patch, which I guess got forgotten.
> > 
> > https://www.spinics.net/lists/kernel/msg4417427.html
> 
> Ah, we just missed it, doubt there's anything more than that to the story.

100% sure about it.

BTW there another minor bug I need to fix which was pointed to me by Suravee Suthikulpanit,
just so that I don't forget about it:

My code which inhibits APIC when APIC_ID != vcpu_id has a bug in regard that
when we have more that 255 vCPUs, this condition becames always true, but it is
to some extent wrong to inhibit the AVIC always in this case - there is a use case
when the guest uses only 255 vCPUs, then AVIC will work.

The relaxed condition for inhibit should be that APIC_ID == (vcpu_id & 0xFF), and it AVIC
is actually used on vCPU > 255, then it will be inhibited ( I need to check if the code
for this exists).

Best regards,
	Maxim Levitsky



> 
> > Since it is literaly the same patch, you can just add credit to Suravee Suthikulpanit.
> > 
> > So with the credit added:
> > 
> > Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> 
> I'll grab Suravee's patch and added your review.  Thanks!
>