diff mbox series

[v6,15/17] KVM: SVM: Use target APIC ID to complete x2AVIC IRQs when possible

Message ID 20220519102709.24125-16-suravee.suthikulpanit@amd.com (mailing list archive)
State New, archived
Headers show
Series Introducing AMD x2AVIC and hybrid-AVIC modes | expand

Commit Message

Suravee Suthikulpanit May 19, 2022, 10:27 a.m. UTC 
For x2AVIC, the index from incomplete IPI #vmexit info is invalid
for logical cluster mode. Only ICRH/ICRL values can be used
to determine the IPI destination APIC ID.

Since QEMU defines guest physical APIC ID to be the same as
vCPU ID, it can be used to quickly identify the target vCPU to deliver IPI,
and avoid the overhead from searching through all vCPUs to match the target
vCPU.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/kvm/svm/avic.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini June 24, 2022, 4:41 p.m. UTC | #1 
On 5/19/22 12:27, Suravee Suthikulpanit wrote:
> +			 * If the x2APIC logical ID sub-field (i.e. icrh[15:0]) contains zero
> +			 * or more than 1 bits, we cannot match just one vcpu to kick for
> +			 * fast path.
> +			 */
> +			if (!first || (first != last))
> +				return -EINVAL;
> +
> +			apic = first - 1;
> +			if ((apic < 0) || (apic > 15) || (cluster >= 0xfffff))
> +				return -EINVAL;

Neither of these is possible: first == 0 has been cheked above, and
ffs(icrh & 0xffff) cannot exceed 15.  Likewise, cluster is actually
limited to 16 bits, not 20.

Plus, C is not Pascal so no parentheses. :)

Putting everything together, it can be simplified to this:

+                       int cluster = (icrh & 0xffff0000) >> 16;
+                       int apic = ffs(icrh & 0xffff) - 1;
+
+                       /*
+                        * If the x2APIC logical ID sub-field (i.e. icrh[15:0])
+                        * contains anything but a single bit, we cannot use the
+                        * fast path, because it is limited to a single vCPU.
+                        */
+                       if (apic < 0 || icrh != (1 << apic))
+                               return -EINVAL;
+
+                       l1_physical_id = (cluster << 4) + apic;


> +			apic_id = (cluster << 4) + apic;
Maxim Levitsky June 27, 2022, 10:55 p.m. UTC | #2 
On Fri, 2022-06-24 at 18:41 +0200, Paolo Bonzini wrote:
> On 5/19/22 12:27, Suravee Suthikulpanit wrote:
> > +			 * If the x2APIC logical ID sub-field (i.e. icrh[15:0]) contains zero
> > +			 * or more than 1 bits, we cannot match just one vcpu to kick for
> > +			 * fast path.
> > +			 */
> > +			if (!first || (first != last))
> > +				return -EINVAL;
> > +
> > +			apic = first - 1;
> > +			if ((apic < 0) || (apic > 15) || (cluster >= 0xfffff))
> > +				return -EINVAL;
> 
> Neither of these is possible: first == 0 has been cheked above, and
> ffs(icrh & 0xffff) cannot exceed 15.  Likewise, cluster is actually
> limited to 16 bits, not 20.
> 
> Plus, C is not Pascal so no parentheses. :)
> 
> Putting everything together, it can be simplified to this:
> 
> +                       int cluster = (icrh & 0xffff0000) >> 16;
> +                       int apic = ffs(icrh & 0xffff) - 1;
> +
> +                       /*
> +                        * If the x2APIC logical ID sub-field (i.e. icrh[15:0])
> +                        * contains anything but a single bit, we cannot use the
> +                        * fast path, because it is limited to a single vCPU.
> +                        */
> +                       if (apic < 0 || icrh != (1 << apic))
> +                               return -EINVAL;
> +
> +                       l1_physical_id = (cluster << 4) + apic;
> 
> 
> > +			apic_id = (cluster << 4) + apic;

Hi Paolo and Suravee Suthikulpanit!

Note that this patch is not needed anymore, I fixed the avic_kick_target_vcpus_fast function,
and added the support for x2apic because it was very easy to do
(I already needed to parse logical id for flat and cluser modes)

Best regards,
	Maxim Levitsky
Suravee Suthikulpanit June 28, 2022, 2:35 a.m. UTC | #3 
On 6/28/2022 5:55 AM, Maxim Levitsky wrote:
> On Fri, 2022-06-24 at 18:41 +0200, Paolo Bonzini wrote:
>> On 5/19/22 12:27, Suravee Suthikulpanit wrote:
>>> +			 * If the x2APIC logical ID sub-field (i.e. icrh[15:0]) contains zero
>>> +			 * or more than 1 bits, we cannot match just one vcpu to kick for
>>> +			 * fast path.
>>> +			 */
>>> +			if (!first || (first != last))
>>> +				return -EINVAL;
>>> +
>>> +			apic = first - 1;
>>> +			if ((apic < 0) || (apic > 15) || (cluster >= 0xfffff))
>>> +				return -EINVAL;
>>
>> Neither of these is possible: first == 0 has been cheked above, and
>> ffs(icrh & 0xffff) cannot exceed 15.  Likewise, cluster is actually
>> limited to 16 bits, not 20.
>>
>> Plus, C is not Pascal so no parentheses. :)
>>
>> Putting everything together, it can be simplified to this:
>>
>> +                       int cluster = (icrh & 0xffff0000) >> 16;
>> +                       int apic = ffs(icrh & 0xffff) - 1;
>> +
>> +                       /*
>> +                        * If the x2APIC logical ID sub-field (i.e. icrh[15:0])
>> +                        * contains anything but a single bit, we cannot use the
>> +                        * fast path, because it is limited to a single vCPU.
>> +                        */
>> +                       if (apic < 0 || icrh != (1 << apic))
>> +                               return -EINVAL;
>> +
>> +                       l1_physical_id = (cluster << 4) + apic;
>>
>>
>>> +			apic_id = (cluster << 4) + apic;
> 
> Hi Paolo and Suravee Suthikulpanit!
> 
> Note that this patch is not needed anymore, I fixed the avic_kick_target_vcpus_fast function,
> and added the support for x2apic because it was very easy to do
> (I already needed to parse logical id for flat and cluser modes)
> 
> Best regards,
> 	Maxim Levitsky
> 

Understood. I was about to send v7 to remove this patch from the series, but too late. I'll test the current queue branch and provide update.

Best Regards,
Suravee
Maxim Levitsky June 28, 2022, 8:59 a.m. UTC | #4 
On Tue, 2022-06-28 at 09:35 +0700, Suthikulpanit, Suravee wrote:
> 
> On 6/28/2022 5:55 AM, Maxim Levitsky wrote:
> > On Fri, 2022-06-24 at 18:41 +0200, Paolo Bonzini wrote:
> > > On 5/19/22 12:27, Suravee Suthikulpanit wrote:
> > > > +			 * If the x2APIC logical ID sub-field (i.e. icrh[15:0]) contains zero
> > > > +			 * or more than 1 bits, we cannot match just one vcpu to kick for
> > > > +			 * fast path.
> > > > +			 */
> > > > +			if (!first || (first != last))
> > > > +				return -EINVAL;
> > > > +
> > > > +			apic = first - 1;
> > > > +			if ((apic < 0) || (apic > 15) || (cluster >= 0xfffff))
> > > > +				return -EINVAL;
> > > 
> > > Neither of these is possible: first == 0 has been cheked above, and
> > > ffs(icrh & 0xffff) cannot exceed 15.  Likewise, cluster is actually
> > > limited to 16 bits, not 20.
> > > 
> > > Plus, C is not Pascal so no parentheses. :)
> > > 
> > > Putting everything together, it can be simplified to this:
> > > 
> > > +                       int cluster = (icrh & 0xffff0000) >> 16;
> > > +                       int apic = ffs(icrh & 0xffff) - 1;
> > > +
> > > +                       /*
> > > +                        * If the x2APIC logical ID sub-field (i.e. icrh[15:0])
> > > +                        * contains anything but a single bit, we cannot use the
> > > +                        * fast path, because it is limited to a single vCPU.
> > > +                        */
> > > +                       if (apic < 0 || icrh != (1 << apic))
> > > +                               return -EINVAL;
> > > +
> > > +                       l1_physical_id = (cluster << 4) + apic;
> > > 
> > > 
> > > > +			apic_id = (cluster << 4) + apic;
> > 
> > Hi Paolo and Suravee Suthikulpanit!
> > 
> > Note that this patch is not needed anymore, I fixed the avic_kick_target_vcpus_fast function,
> > and added the support for x2apic because it was very easy to do
> > (I already needed to parse logical id for flat and cluser modes)
> > 
> > Best regards,
> > 	Maxim Levitsky
> > 
> 
> Understood. I was about to send v7 to remove this patch from the series, but too late. I'll test the current queue branch and provide update.

Also this really needs a KVM unit test, to avoid breaking corner cases like
sending IPI to 0xFF address, which was the reason I had to fix the 
avic_kick_target_vcpus_fast.

We do have 'apic' test in kvm unit tests,
and I was already looking to extend it to cover more cases and to run it with AVIC's
compatible settings. I hope I will be able to do this this week.

Best regards,
	Maxim Levitsky


> 
> Best Regards,
> Suravee
>
Suravee Suthikulpanit June 28, 2022, 12:36 p.m. UTC | #5 
Paolo / Maxim

On 6/28/2022 3:59 PM, Maxim Levitsky wrote:
>>> Hi Paolo and Suravee Suthikulpanit!
>>>
>>> Note that this patch is not needed anymore, I fixed the avic_kick_target_vcpus_fast function,
>>> and added the support for x2apic because it was very easy to do
>>> (I already needed to parse logical id for flat and cluser modes)
>>>
>>> Best regards,
>>> 	Maxim Levitsky
>>>
>> Understood. I was about to send v7 to remove this patch from the series, but too late. I'll test the current queue branch and provide update.
> Also this really needs a KVM unit test, to avoid breaking corner cases like
> sending IPI to 0xFF address, which was the reason I had to fix the
> avic_kick_target_vcpus_fast.
> 
> We do have 'apic' test in kvm unit tests,
> and I was already looking to extend it to cover more cases and to run it with AVIC's
> compatible settings. I hope I will be able to do this this week.

Thanks. Would you please CC me as well once ready?

> 
> Best regards,
> 	Maxim Levitsky

I have also submitted a patch to fix the 603ccef42ce9 ("KVM: x86: SVM: fix avic_kick_target_vcpus_fast"),
which was queued previously.

Best Regards,
Suravee Suthikulpanit
Maxim Levitsky June 28, 2022, 1:14 p.m. UTC | #6 
On Tue, 2022-06-28 at 19:36 +0700, Suthikulpanit, Suravee wrote:
> Paolo / Maxim
> 
> On 6/28/2022 3:59 PM, Maxim Levitsky wrote:
> > > > Hi Paolo and Suravee Suthikulpanit!
> > > > 
> > > > Note that this patch is not needed anymore, I fixed the avic_kick_target_vcpus_fast function,
> > > > and added the support for x2apic because it was very easy to do
> > > > (I already needed to parse logical id for flat and cluser modes)
> > > > 
> > > > Best regards,
> > > > 	Maxim Levitsky
> > > > 
> > > Understood. I was about to send v7 to remove this patch from the series, but too late. I'll test the current queue branch and provide update.
> > Also this really needs a KVM unit test, to avoid breaking corner cases like
> > sending IPI to 0xFF address, which was the reason I had to fix the
> > avic_kick_target_vcpus_fast.
> > 
> > We do have 'apic' test in kvm unit tests,
> > and I was already looking to extend it to cover more cases and to run it with AVIC's
> > compatible settings. I hope I will be able to do this this week.
> 
> Thanks. Would you please CC me as well once ready?

Of course!
> 
> > Best regards,
> > 	Maxim Levitsky
> 
> I have also submitted a patch to fix the 603ccef42ce9 ("KVM: x86: SVM: fix avic_kick_target_vcpus_fast"),
> which was queued previously.

Thank you very much!

Best regards,
	Maxim Levitsky


> 
> Best Regards,
> Suravee Suthikulpanit
>
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index bac876bb1cf1..9c439a32c343 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -358,7 +358,26 @@  static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
 			/* For xAPIC logical mode, the index is for logical APIC table. */
 			apic_id = avic_logical_id_table[index] & 0x1ff;
 		} else {
-			return -EINVAL;
+			/* For x2APIC logical mode, cannot leverage the index.
+			 * Instead, calculate physical ID from logical ID in ICRH.
+			 */
+			int apic;
+			int first = ffs(icrh & 0xffff);
+			int last = fls(icrh & 0xffff);
+			int cluster = (icrh & 0xffff0000) >> 16;
+
+			/*
+			 * If the x2APIC logical ID sub-field (i.e. icrh[15:0]) contains zero
+			 * or more than 1 bits, we cannot match just one vcpu to kick for
+			 * fast path.
+			 */
+			if (!first || (first != last))
+				return -EINVAL;
+
+			apic = first - 1;
+			if ((apic < 0) || (apic > 15) || (cluster >= 0xfffff))
+				return -EINVAL;
+			apic_id = (cluster << 4) + apic;
 		}
 	}