[PATCH v2 0/3] KVM: SVM: Fix GATag bug for >256 vCPUs

[PATCH v2 0/3] KVM: SVM: Fix GATag bug for >256 vCPUs

[Sean Christopherson]

Fix a bug in KVM's use of the GATag where it unintentionally drops a bit
from vCPU IDs greater than 255 and as a result wakes the wrong vCPU.

Suravee and/or Alejandro, can you give this proper testing?  It's compile
tested only at this point.  I'll do basic testing before officially
applying, but AFAIK I don't have access to x2AVIC hardware, nor do I have
a ready-to-go configuration to properly exercise this code.

Thanks!

Sean Christopherson (2):
  KVM: SVM: Fix a benign off-by-one bug in AVIC physical table mask
  KVM: SVM: WARN if GATag generation drops VM or vCPU ID information

Suravee Suthikulpanit (1):
  KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

 arch/x86/include/asm/svm.h | 12 +++++++-----
 arch/x86/kvm/svm/avic.c    | 37 ++++++++++++++++++++++++++++---------
 2 files changed, 35 insertions(+), 14 deletions(-)


base-commit: 32e69f232db4ca11f26e5961daeff93906ce232f
-- 
2.39.1.519.gcb327c4b5f-goog

[PATCH v2 1/3] KVM: SVM: Fix a benign off-by-one bug in AVIC physical table mask

[Sean Christopherson]

Define the "physical table max index mask" as bits 8:0, not 9:0.  x2AVIC
currently supports a max of 512 entries, i.e. the max index is 511, and
the inputs to GENMASK_ULL() are inclusive.  The bug is benign as bit 9 is
reserved and never set by KVM, i.e. KVM is just clearing bits that are
guaranteed to be zero.

Note, as of this writing, APM "Rev. 3.39-October 2022" incorrectly states
that bits 11:8 are reserved in Table B-1. VMCB Layout, Control Area.  I.e.
that table wasn't updated when x2AVIC support was added.

Opportunistically fix the comment for the max AVIC ID to align with the
code, and clean up comment formatting too.

Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
Cc: stable@vger.kernel.org
Cc: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/svm.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index cb1ee53ad3b1..770dcf75eaa9 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -261,20 +261,22 @@ enum avic_ipi_failure_cause {
 	AVIC_IPI_FAILURE_INVALID_BACKING_PAGE,
 };
 
-#define AVIC_PHYSICAL_MAX_INDEX_MASK	GENMASK_ULL(9, 0)
+#define AVIC_PHYSICAL_MAX_INDEX_MASK	GENMASK_ULL(8, 0)
 
 /*
- * For AVIC, the max index allowed for physical APIC ID
- * table is 0xff (255).
+ * For AVIC, the max index allowed for physical APIC ID table is 0xfe (254), as
+ * 0xff is a broadcast to all CPUs, i.e. can't be targeted individually.
  */
 #define AVIC_MAX_PHYSICAL_ID		0XFEULL
 
 /*
- * For x2AVIC, the max index allowed for physical APIC ID
- * table is 0x1ff (511).
+ * For x2AVIC, the max index allowed for physical APIC ID table is 0x1ff (511).
  */
 #define X2AVIC_MAX_PHYSICAL_ID		0x1FFUL
 
+static_assert((AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == AVIC_MAX_PHYSICAL_ID);
+static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_MAX_PHYSICAL_ID);
+
 #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
-- 
2.39.1.519.gcb327c4b5f-goog

[PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

[Sean Christopherson]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
that effectively controls the largest guest physical APIC ID supported by
x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
VM bits to 24).

The AVIC GATag is programmed into the AMD IOMMU IRTE to provide a
reference back to KVM in case the IOMMU cannot inject an interrupt into a
non-running vCPU.  In such a case, the IOMMU notifies software by creating
a GALog entry with the corresponded GATag, and KVM then uses the GATag to
find the correct VM+vCPU to kick.  Dropping bit 8 from the GATag results
in kicking the wrong vCPU when targeting vCPUs with x2APIC ID > 255.

Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
Cc: stable@vger.kernel.org
Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index ca684979e90d..326341a22153 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -27,19 +27,29 @@
 #include "irq.h"
 #include "svm.h"
 
-/* AVIC GATAG is encoded using VM and VCPU IDs */
-#define AVIC_VCPU_ID_BITS		8
-#define AVIC_VCPU_ID_MASK		((1 << AVIC_VCPU_ID_BITS) - 1)
+/*
+ * Encode the arbitrary VM ID and the vCPU's default APIC ID, i.e the vCPU ID,
+ * into the GATag so that KVM can retrieve the correct vCPU from a GALog entry
+ * if an interrupt can't be delivered, e.g. because the vCPU isn't running.
+ *
+ * For the vCPU ID, use however many bits are currently allowed for the max
+ * guest physical APIC ID (limited by the size of the physical ID table), and
+ * use whatever bits remain to assign arbitrary AVIC IDs to VMs.  Note, the
+ * size of the GATag is defined by hardware (32 bits), but is an opaque value
+ * as far as hardware is concerned.
+ */
+#define AVIC_VCPU_ID_MASK		AVIC_PHYSICAL_MAX_INDEX_MASK
 
-#define AVIC_VM_ID_BITS			24
-#define AVIC_VM_ID_NR			(1 << AVIC_VM_ID_BITS)
-#define AVIC_VM_ID_MASK			((1 << AVIC_VM_ID_BITS) - 1)
+#define AVIC_VM_ID_SHIFT		HWEIGHT32(AVIC_PHYSICAL_MAX_INDEX_MASK)
+#define AVIC_VM_ID_MASK			(GENMASK(31, AVIC_VM_ID_SHIFT) >> AVIC_VM_ID_SHIFT)
 
-#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VCPU_ID_BITS) | \
+#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
 						(y & AVIC_VCPU_ID_MASK))
-#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
+#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VM_ID_SHIFT) & AVIC_VM_ID_MASK)
 #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
 
+static_assert(AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
+
 static bool force_avic;
 module_param_unsafe(force_avic, bool, 0444);
 
-- 
2.39.1.519.gcb327c4b5f-goog

[PATCH v2 3/3] KVM: SVM: WARN if GATag generation drops VM or vCPU ID information

[Sean Christopherson]

WARN if generating a GATag given a VM ID and vCPU ID doesn't yield the
same IDs when pulling the IDs back out of the tag.  Don't bother adding
error handling to callers, this is very much a paranoid sanity check as
KVM fully controls the VM ID and is supposed to reject too-big vCPU IDs.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 326341a22153..cfc8ab773025 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -43,12 +43,21 @@
 #define AVIC_VM_ID_SHIFT		HWEIGHT32(AVIC_PHYSICAL_MAX_INDEX_MASK)
 #define AVIC_VM_ID_MASK			(GENMASK(31, AVIC_VM_ID_SHIFT) >> AVIC_VM_ID_SHIFT)
 
-#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
-						(y & AVIC_VCPU_ID_MASK))
 #define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VM_ID_SHIFT) & AVIC_VM_ID_MASK)
 #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
 
-static_assert(AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
+#define __AVIC_GATAG(vm_id, vcpu_id)	((((vm_id) & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
+					 ((vcpu_id) & AVIC_VCPU_ID_MASK))
+#define AVIC_GATAG(vm_id, vcpu_id)					\
+({									\
+	u32 ga_tag = __AVIC_GATAG(vm_id, vcpu_id);			\
+									\
+	WARN_ON_ONCE(AVIC_GATAG_TO_VCPUID(ga_tag) != (vcpu_id));	\
+	WARN_ON_ONCE(AVIC_GATAG_TO_VMID(ga_tag) != (vm_id));		\
+	ga_tag;								\
+})
+
+static_assert(__AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
 
 static bool force_avic;
 module_param_unsafe(force_avic, bool, 0444);
-- 
2.39.1.519.gcb327c4b5f-goog

Re: [PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

[Igor Mammedov]

On Tue,  7 Feb 2023 00:21:55 +0000
Sean Christopherson <seanjc@google.com> wrote:

> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
> that effectively controls the largest guest physical APIC ID supported by
> x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
> VM bits to 24).

Is there any particular reason not to tie it to max supported by KVM
KVM_MAX_VCPU_IDS?

Another question:
 will guest fail to start when configured with more than 512 vCPUs
 or it will start broken?

> 
> The AVIC GATag is programmed into the AMD IOMMU IRTE to provide a
> reference back to KVM in case the IOMMU cannot inject an interrupt into a
> non-running vCPU.  In such a case, the IOMMU notifies software by creating
> a GALog entry with the corresponded GATag, and KVM then uses the GATag to
> find the correct VM+vCPU to kick.  Dropping bit 8 from the GATag results
> in kicking the wrong vCPU when targeting vCPUs with x2APIC ID > 255.
> 
> Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
> Cc: stable@vger.kernel.org
> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/avic.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index ca684979e90d..326341a22153 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -27,19 +27,29 @@
>  #include "irq.h"
>  #include "svm.h"
>  
> -/* AVIC GATAG is encoded using VM and VCPU IDs */
> -#define AVIC_VCPU_ID_BITS		8
> -#define AVIC_VCPU_ID_MASK		((1 << AVIC_VCPU_ID_BITS) - 1)
> +/*
> + * Encode the arbitrary VM ID and the vCPU's default APIC ID, i.e the vCPU ID,
> + * into the GATag so that KVM can retrieve the correct vCPU from a GALog entry
> + * if an interrupt can't be delivered, e.g. because the vCPU isn't running.
> + *
> + * For the vCPU ID, use however many bits are currently allowed for the max
> + * guest physical APIC ID (limited by the size of the physical ID table), and
> + * use whatever bits remain to assign arbitrary AVIC IDs to VMs.  Note, the
> + * size of the GATag is defined by hardware (32 bits), but is an opaque value
> + * as far as hardware is concerned.
> + */
> +#define AVIC_VCPU_ID_MASK		AVIC_PHYSICAL_MAX_INDEX_MASK
>  
> -#define AVIC_VM_ID_BITS			24
> -#define AVIC_VM_ID_NR			(1 << AVIC_VM_ID_BITS)
> -#define AVIC_VM_ID_MASK			((1 << AVIC_VM_ID_BITS) - 1)
> +#define AVIC_VM_ID_SHIFT		HWEIGHT32(AVIC_PHYSICAL_MAX_INDEX_MASK)
> +#define AVIC_VM_ID_MASK			(GENMASK(31, AVIC_VM_ID_SHIFT) >> AVIC_VM_ID_SHIFT)
>  
> -#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VCPU_ID_BITS) | \
> +#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
>  						(y & AVIC_VCPU_ID_MASK))
> -#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
> +#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VM_ID_SHIFT) & AVIC_VM_ID_MASK)
>  #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
>  
> +static_assert(AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
> +
>  static bool force_avic;
>  module_param_unsafe(force_avic, bool, 0444);
>  

Re: [PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

[Joao Martins]

On 07/02/2023 08:33, Igor Mammedov wrote:
> On Tue,  7 Feb 2023 00:21:55 +0000
> Sean Christopherson <seanjc@google.com> wrote:
> 
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
>> that effectively controls the largest guest physical APIC ID supported by
>> x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
>> VM bits to 24).
> 
> Is there any particular reason not to tie it to max supported by KVM
> KVM_MAX_VCPU_IDS?
> 
> Another question:
>  will guest fail to start when configured with more than 512 vCPUs
>  or it will start broken?
> 

I think the problem is not so much the GATag (which can really be anything at
the resolution you want). It's more of an SVM limit AIUI. Provided you can't
have GATAgs if you don't have guest-mode/AVIC active, then makes sense have the
same limit on both.

SVM seems to be limited to 256 vcpus in xAPIC mode or 512 vcpus in x2APIC
mode[0]. IIUC You actually won't be able to create guests with more than
512vcpus as KVM bound checks those max limits very early in the vCPU init (see
avic_init_vcpu()). I guess the alternative would an AVIC inhibit if vCPU count
goes beyond those limits -- probably a must have once avic flips to 1 by default
like Intel.

[0] in APM Volume 2 15.29.4.3 Physical Address Pointer Restrictions,

* All the addresses point to 4-Kbyte aligned data structures. Bits 11:0 are
reserved (except for offset 0F8h) and should be set to zero. The lower 8 bits of
offset 0F8h are used for the field AVIC_PHYSICAL_MAX_INDEX. VMRUN fails with
#VMEXIT(VMEXIT_INVALID) if AVIC_PHYSICAL_MAX_INDEX is greater than 255 in xAVIC
mode or greater than 511 in x2AVIC mode.

Re: [PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

[Sean Christopherson]

On Tue, Feb 07, 2023, Joao Martins wrote:
> On 07/02/2023 08:33, Igor Mammedov wrote:
> > On Tue,  7 Feb 2023 00:21:55 +0000
> > Sean Christopherson <seanjc@google.com> wrote:
> > 
> >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> >>
> >> Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
> >> that effectively controls the largest guest physical APIC ID supported by
> >> x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
> >> VM bits to 24).
> > 
> > Is there any particular reason not to tie it to max supported by KVM
> > KVM_MAX_VCPU_IDS?
> > 
> > Another question:
> >  will guest fail to start when configured with more than 512 vCPUs
> >  or it will start broken?
> > 
> 
> I think the problem is not so much the GATag (which can really be anything at
> the resolution you want). It's more of an SVM limit AIUI. Provided you can't
> have GATAgs if you don't have guest-mode/AVIC active, then makes sense have the
> same limit on both.

Yep.  The physical ID table, which is needed to achieve full AVIC benefits for a
vCPU, is a single 4KiB page that holds 512 64-bit entries.  AIUI, the GATag is
used if and only if the interrupt target is in the physical ID table, so using
more GATag bits for vCPU ID is pointless.

> SVM seems to be limited to 256 vcpus in xAPIC mode or 512 vcpus in x2APIC
> mode[0]. IIUC You actually won't be able to create guests with more than
> 512vcpus as KVM bound checks those max limits very early in the vCPU init (see
> avic_init_vcpu()). I guess the alternative would an AVIC inhibit if vCPU count
> goes beyond those limits -- probably a must have once avic flips to 1 by default
> like Intel.

I don't _think_ KVM would have to explicitly inhibit AVIC.  I believe the fallout
would be that vCPUs >= 512 would simply not be eligible for virtual interrupt
delivery, e.g. KVM would get a "Invalid Target in IPI" exit.  I haven't dug into
the IOMMU side of things though, so it's possible something in that world would
necessitate disabling (x2)AVIC.

> [0] in APM Volume 2 15.29.4.3 Physical Address Pointer Restrictions,
> 
> * All the addresses point to 4-Kbyte aligned data structures. Bits 11:0 are
> reserved (except for offset 0F8h) and should be set to zero. The lower 8 bits of
> offset 0F8h are used for the field AVIC_PHYSICAL_MAX_INDEX. VMRUN fails with
> #VMEXIT(VMEXIT_INVALID) if AVIC_PHYSICAL_MAX_INDEX is greater than 255 in xAVIC
> mode or greater than 511 in x2AVIC mode.

Re: [PATCH v2 1/3] KVM: SVM: Fix a benign off-by-one bug in AVIC physical table mask

[Suthikulpanit, Suravee]

On 2/7/2023 7:21 AM, Sean Christopherson wrote:
> Define the "physical table max index mask" as bits 8:0, not 9:0.  x2AVIC
> currently supports a max of 512 entries, i.e. the max index is 511, and
> the inputs to GENMASK_ULL() are inclusive.  The bug is benign as bit 9 is
> reserved and never set by KVM, i.e. KVM is just clearing bits that are
> guaranteed to be zero.
> 
> Note, as of this writing, APM "Rev. 3.39-October 2022" incorrectly states
> that bits 11:8 are reserved in Table B-1. VMCB Layout, Control Area.  I.e.
> that table wasn't updated when x2AVIC support was added.
> 
> Opportunistically fix the comment for the max AVIC ID to align with the
> code, and clean up comment formatting too.
> 
> Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
> Cc: stable@vger.kernel.org
> Cc: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/svm.h | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index cb1ee53ad3b1..770dcf75eaa9 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -261,20 +261,22 @@ enum avic_ipi_failure_cause {
>   	AVIC_IPI_FAILURE_INVALID_BACKING_PAGE,
>   };
>   
> -#define AVIC_PHYSICAL_MAX_INDEX_MASK	GENMASK_ULL(9, 0)
> +#define AVIC_PHYSICAL_MAX_INDEX_MASK	GENMASK_ULL(8, 0)
>   
>   /*
> - * For AVIC, the max index allowed for physical APIC ID
> - * table is 0xff (255).
> + * For AVIC, the max index allowed for physical APIC ID table is 0xfe (254), as
> + * 0xff is a broadcast to all CPUs, i.e. can't be targeted individually.
>    */
>   #define AVIC_MAX_PHYSICAL_ID		0XFEULL
>   
>   /*
> - * For x2AVIC, the max index allowed for physical APIC ID
> - * table is 0x1ff (511).
> + * For x2AVIC, the max index allowed for physical APIC ID table is 0x1ff (511).
>    */
>   #define X2AVIC_MAX_PHYSICAL_ID		0x1FFUL
>   
> +static_assert((AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == AVIC_MAX_PHYSICAL_ID);
> +static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_MAX_PHYSICAL_ID);
> +
>   #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
>   #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
>   

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,
Suravee

Re: [PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

[Suthikulpanit, Suravee]

On 2/7/2023 7:21 AM, Sean Christopherson wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
> that effectively controls the largest guest physical APIC ID supported by
> x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
> VM bits to 24).
> 
> The AVIC GATag is programmed into the AMD IOMMU IRTE to provide a
> reference back to KVM in case the IOMMU cannot inject an interrupt into a
> non-running vCPU.  In such a case, the IOMMU notifies software by creating
> a GALog entry with the corresponded GATag, and KVM then uses the GATag to
> find the correct VM+vCPU to kick.  Dropping bit 8 from the GATag results
> in kicking the wrong vCPU when targeting vCPUs with x2APIC ID > 255.
> 
> Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
> Cc: stable@vger.kernel.org
> Reported-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/avic.c | 26 ++++++++++++++++++--------
>   1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index ca684979e90d..326341a22153 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -27,19 +27,29 @@
>   #include "irq.h"
>   #include "svm.h"
>   
> -/* AVIC GATAG is encoded using VM and VCPU IDs */
> -#define AVIC_VCPU_ID_BITS		8
> -#define AVIC_VCPU_ID_MASK		((1 << AVIC_VCPU_ID_BITS) - 1)
> +/*
> + * Encode the arbitrary VM ID and the vCPU's default APIC ID, i.e the vCPU ID,
> + * into the GATag so that KVM can retrieve the correct vCPU from a GALog entry
> + * if an interrupt can't be delivered, e.g. because the vCPU isn't running.
> + *
> + * For the vCPU ID, use however many bits are currently allowed for the max
> + * guest physical APIC ID (limited by the size of the physical ID table), and
> + * use whatever bits remain to assign arbitrary AVIC IDs to VMs.  Note, the
> + * size of the GATag is defined by hardware (32 bits), but is an opaque value
> + * as far as hardware is concerned.
> + */
> +#define AVIC_VCPU_ID_MASK		AVIC_PHYSICAL_MAX_INDEX_MASK
>   
> -#define AVIC_VM_ID_BITS			24
> -#define AVIC_VM_ID_NR			(1 << AVIC_VM_ID_BITS)
> -#define AVIC_VM_ID_MASK			((1 << AVIC_VM_ID_BITS) - 1)
> +#define AVIC_VM_ID_SHIFT		HWEIGHT32(AVIC_PHYSICAL_MAX_INDEX_MASK)
> +#define AVIC_VM_ID_MASK			(GENMASK(31, AVIC_VM_ID_SHIFT) >> AVIC_VM_ID_SHIFT)
>   
> -#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VCPU_ID_BITS) | \
> +#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
>   						(y & AVIC_VCPU_ID_MASK))
> -#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
> +#define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VM_ID_SHIFT) & AVIC_VM_ID_MASK)
>   #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
>   
> +static_assert(AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
> +
>   static bool force_avic;
>   module_param_unsafe(force_avic, bool, 0444);
>   

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Thanks,
Suravee

Re: [PATCH v2 2/3] KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs

[Suthikulpanit, Suravee]

On 2/7/2023 11:38 PM, Sean Christopherson wrote:
> On Tue, Feb 07, 2023, Joao Martins wrote:
>> On 07/02/2023 08:33, Igor Mammedov wrote:
>>> On Tue,  7 Feb 2023 00:21:55 +0000
>>> Sean Christopherson <seanjc@google.com> wrote:
>>>
>>>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>>>
>>>> Define AVIC_VCPU_ID_MASK based on AVIC_PHYSICAL_MAX_INDEX, i.e. the mask
>>>> that effectively controls the largest guest physical APIC ID supported by
>>>> x2AVIC, instead of hardcoding the number of bits to 8 (and the number of
>>>> VM bits to 24).
>>>
>>> Is there any particular reason not to tie it to max supported by KVM
>>> KVM_MAX_VCPU_IDS?
>>>
>>> Another question:
>>>   will guest fail to start when configured with more than 512 vCPUs
>>>   or it will start broken?
>>>
>>
>> I think the problem is not so much the GATag (which can really be anything at
>> the resolution you want). It's more of an SVM limit AIUI. Provided you can't
>> have GATAgs if you don't have guest-mode/AVIC active, then makes sense have the
>> same limit on both.

Correct.

> Yep.  The physical ID table, which is needed to achieve full AVIC benefits for a
> vCPU, is a single 4KiB page that holds 512 64-bit entries.  AIUI, the GATag is
> used if and only if the interrupt target is in the physical ID table, so using
> more GATag bits for vCPU ID is pointless.

Correct.

>> SVM seems to be limited to 256 vcpus in xAPIC mode or 512 vcpus in x2APIC
>> mode[0]. IIUC You actually won't be able to create guests with more than
>> 512vcpus as KVM bound checks those max limits very early in the vCPU init (see
>> avic_init_vcpu()). I guess the alternative would an AVIC inhibit if vCPU count
>> goes beyond those limits -- probably a must have once avic flips to 1 by default
>> like Intel.
> 
> I don't _think_ KVM would have to explicitly inhibit AVIC.  I believe the fallout
> would be that vCPUs >= 512 would simply not be eligible for virtual interrupt
> delivery, e.g. KVM would get a "Invalid Target in IPI" exit.  I haven't dug into
> the IOMMU side of things though, so it's possible something in that world would
> necessitate disabling (x2)AVIC.

SVM-AVIC is independent of the IOMMU-AVIC. We can enable SVM-AVIC, and 
use the legacy IOMMU interrupt remapping mode IRTE[GuestMode]=0.
However, I have not explored the case of combining of the two modes. I 
can look into it and experiment with this case.

Thanks,
Suravee

>> [0] in APM Volume 2 15.29.4.3 Physical Address Pointer Restrictions,
>>
>> * All the addresses point to 4-Kbyte aligned data structures. Bits 11:0 are
>> reserved (except for offset 0F8h) and should be set to zero. The lower 8 bits of
>> offset 0F8h are used for the field AVIC_PHYSICAL_MAX_INDEX. VMRUN fails with
>> #VMEXIT(VMEXIT_INVALID) if AVIC_PHYSICAL_MAX_INDEX is greater than 255 in xAVIC
>> mode or greater than 511 in x2AVIC mode.

Re: [PATCH v2 3/3] KVM: SVM: WARN if GATag generation drops VM or vCPU ID information

[Suthikulpanit, Suravee]

On 2/7/2023 7:21 AM, Sean Christopherson wrote:
> WARN if generating a GATag given a VM ID and vCPU ID doesn't yield the
> same IDs when pulling the IDs back out of the tag.  Don't bother adding
> error handling to callers, this is very much a paranoid sanity check as
> KVM fully controls the VM ID and is supposed to reject too-big vCPU IDs.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/avic.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 326341a22153..cfc8ab773025 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -43,12 +43,21 @@
>   #define AVIC_VM_ID_SHIFT		HWEIGHT32(AVIC_PHYSICAL_MAX_INDEX_MASK)
>   #define AVIC_VM_ID_MASK			(GENMASK(31, AVIC_VM_ID_SHIFT) >> AVIC_VM_ID_SHIFT)
>   
> -#define AVIC_GATAG(x, y)		(((x & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
> -						(y & AVIC_VCPU_ID_MASK))
>   #define AVIC_GATAG_TO_VMID(x)		((x >> AVIC_VM_ID_SHIFT) & AVIC_VM_ID_MASK)
>   #define AVIC_GATAG_TO_VCPUID(x)		(x & AVIC_VCPU_ID_MASK)
>   
> -static_assert(AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
> +#define __AVIC_GATAG(vm_id, vcpu_id)	((((vm_id) & AVIC_VM_ID_MASK) << AVIC_VM_ID_SHIFT) | \
> +					 ((vcpu_id) & AVIC_VCPU_ID_MASK))
> +#define AVIC_GATAG(vm_id, vcpu_id)					\
> +({									\
> +	u32 ga_tag = __AVIC_GATAG(vm_id, vcpu_id);			\
> +									\
> +	WARN_ON_ONCE(AVIC_GATAG_TO_VCPUID(ga_tag) != (vcpu_id));	\
> +	WARN_ON_ONCE(AVIC_GATAG_TO_VMID(ga_tag) != (vm_id));		\
> +	ga_tag;								\
> +})
> +
> +static_assert(__AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_ID_MASK) == -1u);
>   
>   static bool force_avic;
>   module_param_unsafe(force_avic, bool, 0444);

Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,
Suravee

Re: [PATCH v2 0/3] KVM: SVM: Fix GATag bug for >256 vCPUs

[Suthikulpanit, Suravee]

On 2/7/2023 7:21 AM, Sean Christopherson wrote:
> Fix a bug in KVM's use of the GATag where it unintentionally drops a bit
> from vCPU IDs greater than 255 and as a result wakes the wrong vCPU.
> 
> Suravee and/or Alejandro, can you give this proper testing?  It's compile
> tested only at this point.  I'll do basic testing before officially
> applying, but AFAIK I don't have access to x2AVIC hardware, nor do I have
> a ready-to-go configuration to properly exercise this code.
> 
> Thanks!
> 
> Sean Christopherson (2):
>    KVM: SVM: Fix a benign off-by-one bug in AVIC physical table mask
>    KVM: SVM: WARN if GATag generation drops VM or vCPU ID information
> 
> Suravee Suthikulpanit (1):
>    KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs
> 
>   arch/x86/include/asm/svm.h | 12 +++++++-----
>   arch/x86/kvm/svm/avic.c    | 37 ++++++++++++++++++++++++++++---------
>   2 files changed, 35 insertions(+), 14 deletions(-)
> 
> 
> base-commit: 32e69f232db4ca11f26e5961daeff93906ce232f

For the series:

Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks,
Suravee

Re: [PATCH v2 0/3] KVM: SVM: Fix GATag bug for >256 vCPUs

[Sean Christopherson]

On Thu, Feb 16, 2023, Suthikulpanit, Suravee wrote:
> 
> 
> On 2/7/2023 7:21 AM, Sean Christopherson wrote:
> > Fix a bug in KVM's use of the GATag where it unintentionally drops a bit
> > from vCPU IDs greater than 255 and as a result wakes the wrong vCPU.
> > 
> > Suravee and/or Alejandro, can you give this proper testing?  It's compile
> > tested only at this point.  I'll do basic testing before officially
> > applying, but AFAIK I don't have access to x2AVIC hardware, nor do I have
> > a ready-to-go configuration to properly exercise this code.
> > 
> > Thanks!
> > 
> > Sean Christopherson (2):
> >    KVM: SVM: Fix a benign off-by-one bug in AVIC physical table mask
> >    KVM: SVM: WARN if GATag generation drops VM or vCPU ID information
> > 
> > Suravee Suthikulpanit (1):
> >    KVM: SVM: Modify AVIC GATag to support max number of 512 vCPUs
> > 
> >   arch/x86/include/asm/svm.h | 12 +++++++-----
> >   arch/x86/kvm/svm/avic.c    | 37 ++++++++++++++++++++++++++++---------
> >   2 files changed, 35 insertions(+), 14 deletions(-)
> > 
> > 
> > base-commit: 32e69f232db4ca11f26e5961daeff93906ce232f
> 
> For the series:
> 
> Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Thanks much!

Paolo, do you want to grab this directly, or should I throw this in next for a
few days and send a separate pull request?  Or do something else entirely?

Re: [PATCH v2 0/3] KVM: SVM: Fix GATag bug for >256 vCPUs

[Paolo Bonzini]

Queued, thanks.

Paolo