Add CPUID check for the x2APIC virtualization (x2AVIC) feature.
If available, the SVM driver can support both AVIC and x2AVIC modes
when load the kvm_amd driver with avic=1. The operating mode will be
determined at runtime depending on the guest APIC mode.
Reviewed-by: Maxim Levitsky <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/include/asm/svm.h | 3 +++
arch/x86/kvm/svm/avic.c | 40 ++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/svm/svm.c | 15 ++------------
arch/x86/kvm/svm/svm.h | 1 +
4 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index f70a5108d464..2c2a104b777e 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -195,6 +195,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
#define AVIC_ENABLE_SHIFT 31
#define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_SHIFT)
+#define X2APIC_MODE_SHIFT 30
+#define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
+
#define LBR_CTL_ENABLE_MASK BIT_ULL(0)
#define VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK BIT_ULL(1)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index a8f514212b87..fc3ba6071482 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -40,6 +40,15 @@
#define AVIC_GATAG_TO_VMID(x) ((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
#define AVIC_GATAG_TO_VCPUID(x) (x & AVIC_VCPU_ID_MASK)
+enum avic_modes {
+ AVIC_MODE_NONE = 0,
+ AVIC_MODE_X1,
+ AVIC_MODE_X2,
+};
+
+static bool force_avic;
+module_param_unsafe(force_avic, bool, 0444);
+
/* Note:
* This hash table is used to map VM_ID to a struct kvm_svm,
* when handling AMD IOMMU GALOG notification to schedule in
@@ -50,6 +59,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
static u32 next_vm_id = 0;
static bool next_vm_id_wrapped = 0;
static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
+static enum avic_modes avic_mode;
/*
* This is a wrapper of struct amd_iommu_ir_data.
@@ -1077,3 +1087,33 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
avic_vcpu_load(vcpu);
}
+
+/*
+ * Note:
+ * - The module param avic enable both xAPIC and x2APIC mode.
+ * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
+ * - The mode can be switched at run-time.
+ */
+bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
+{
+ if (!npt_enabled)
+ return false;
+
+ if (boot_cpu_has(X86_FEATURE_AVIC)) {
+ avic_mode = AVIC_MODE_X1;
+ pr_info("AVIC enabled\n");
+ } else if (force_avic) {
+ pr_warn("AVIC is not supported in CPUID but force enabled");
+ pr_warn("Your system might crash and burn");
+ }
+
+ if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
+ avic_mode = AVIC_MODE_X2;
+ pr_info("x2AVIC enabled\n");
+ }
+
+ if (avic_mode != AVIC_MODE_NONE)
+ amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
+
+ return !!avic_mode;
+}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 3b49337998ec..74e6f86f5dc3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -188,9 +188,6 @@ module_param(tsc_scaling, int, 0444);
static bool avic;
module_param(avic, bool, 0444);
-static bool force_avic;
-module_param_unsafe(force_avic, bool, 0444);
-
bool __read_mostly dump_invalid_vmcb;
module_param(dump_invalid_vmcb, bool, 0644);
@@ -4913,17 +4910,9 @@ static __init int svm_hardware_setup(void)
nrips = false;
}
- enable_apicv = avic = avic && npt_enabled && (boot_cpu_has(X86_FEATURE_AVIC) || force_avic);
+ enable_apicv = avic = avic && avic_hardware_setup(&svm_x86_ops);
- if (enable_apicv) {
- if (!boot_cpu_has(X86_FEATURE_AVIC)) {
- pr_warn("AVIC is not supported in CPUID but force enabled");
- pr_warn("Your system might crash and burn");
- } else
- pr_info("AVIC enabled\n");
-
- amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
- } else {
+ if (!enable_apicv) {
svm_x86_ops.vcpu_blocking = NULL;
svm_x86_ops.vcpu_unblocking = NULL;
svm_x86_ops.vcpu_get_apicv_inhibit_reasons = NULL;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 32220a1b0ea2..678fc7757fe4 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -603,6 +603,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
/* avic.c */
+bool avic_hardware_setup(struct kvm_x86_ops *ops);
int avic_ga_log_notifier(u32 ga_tag);
void avic_vm_destroy(struct kvm *kvm);
int avic_vm_init(struct kvm *kvm);
--
2.25.1
On Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
> Add CPUID check for the x2APIC virtualization (x2AVIC) feature.
> If available, the SVM driver can support both AVIC and x2AVIC modes
> when load the kvm_amd driver with avic=1. The operating mode will be
> determined at runtime depending on the guest APIC mode.
>
> Reviewed-by: Maxim Levitsky <[email protected]>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> arch/x86/include/asm/svm.h | 3 +++
> arch/x86/kvm/svm/avic.c | 40 ++++++++++++++++++++++++++++++++++++++
> arch/x86/kvm/svm/svm.c | 15 ++------------
> arch/x86/kvm/svm/svm.h | 1 +
> 4 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index f70a5108d464..2c2a104b777e 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -195,6 +195,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
> #define AVIC_ENABLE_SHIFT 31
> #define AVIC_ENABLE_MASK (1 << AVIC_ENABLE_SHIFT)
>
> +#define X2APIC_MODE_SHIFT 30
> +#define X2APIC_MODE_MASK (1 << X2APIC_MODE_SHIFT)
> +
> #define LBR_CTL_ENABLE_MASK BIT_ULL(0)
> #define VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK BIT_ULL(1)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index a8f514212b87..fc3ba6071482 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -40,6 +40,15 @@
> #define AVIC_GATAG_TO_VMID(x) ((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
> #define AVIC_GATAG_TO_VCPUID(x) (x & AVIC_VCPU_ID_MASK)
>
> +enum avic_modes {
> + AVIC_MODE_NONE = 0,
> + AVIC_MODE_X1,
> + AVIC_MODE_X2,
> +};
> +
> +static bool force_avic;
> +module_param_unsafe(force_avic, bool, 0444);
> +
> /* Note:
> * This hash table is used to map VM_ID to a struct kvm_svm,
> * when handling AMD IOMMU GALOG notification to schedule in
> @@ -50,6 +59,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
> static u32 next_vm_id = 0;
> static bool next_vm_id_wrapped = 0;
> static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
> +static enum avic_modes avic_mode;
>
> /*
> * This is a wrapper of struct amd_iommu_ir_data.
> @@ -1077,3 +1087,33 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
>
> avic_vcpu_load(vcpu);
> }
> +
> +/*
> + * Note:
> + * - The module param avic enable both xAPIC and x2APIC mode.
> + * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
> + * - The mode can be switched at run-time.
> + */
> +bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
> +{
> + if (!npt_enabled)
> + return false;
> +
> + if (boot_cpu_has(X86_FEATURE_AVIC)) {
> + avic_mode = AVIC_MODE_X1;
> + pr_info("AVIC enabled\n");
> + } else if (force_avic) {
> + pr_warn("AVIC is not supported in CPUID but force enabled");
> + pr_warn("Your system might crash and burn");
I think in this case avic_mode should also be set to AVIC_MODE_X1
(Hopefully this won't be needed for systems that have x2avic enabled)
Best regards,
Maxim Levitsky
> + }
> +
> + if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
> + avic_mode = AVIC_MODE_X2;
> + pr_info("x2AVIC enabled\n");
> + }
> +
> + if (avic_mode != AVIC_MODE_NONE)
> + amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> +
> + return !!avic_mode;
> +}
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 3b49337998ec..74e6f86f5dc3 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -188,9 +188,6 @@ module_param(tsc_scaling, int, 0444);
> static bool avic;
> module_param(avic, bool, 0444);
>
> -static bool force_avic;
> -module_param_unsafe(force_avic, bool, 0444);
> -
> bool __read_mostly dump_invalid_vmcb;
> module_param(dump_invalid_vmcb, bool, 0644);
>
> @@ -4913,17 +4910,9 @@ static __init int svm_hardware_setup(void)
> nrips = false;
> }
>
> - enable_apicv = avic = avic && npt_enabled && (boot_cpu_has(X86_FEATURE_AVIC) || force_avic);
> + enable_apicv = avic = avic && avic_hardware_setup(&svm_x86_ops);
>
> - if (enable_apicv) {
> - if (!boot_cpu_has(X86_FEATURE_AVIC)) {
> - pr_warn("AVIC is not supported in CPUID but force enabled");
> - pr_warn("Your system might crash and burn");
> - } else
> - pr_info("AVIC enabled\n");
> -
> - amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> - } else {
> + if (!enable_apicv) {
> svm_x86_ops.vcpu_blocking = NULL;
> svm_x86_ops.vcpu_unblocking = NULL;
> svm_x86_ops.vcpu_get_apicv_inhibit_reasons = NULL;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 32220a1b0ea2..678fc7757fe4 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -603,6 +603,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
>
> /* avic.c */
>
> +bool avic_hardware_setup(struct kvm_x86_ops *ops);
> int avic_ga_log_notifier(u32 ga_tag);
> void avic_vm_destroy(struct kvm *kvm);
> int avic_vm_init(struct kvm *kvm);
On Thu, 2022-05-05 at 11:07 +0700, Suravee Suthikulpanit wrote:
> Maxim,
>
> On 5/4/22 7:12 PM, Maxim Levitsky wrote:
> > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > index a8f514212b87..fc3ba6071482 100644
> > > --- a/arch/x86/kvm/svm/avic.c
> > > +++ b/arch/x86/kvm/svm/avic.c
> > > @@ -40,6 +40,15 @@
> > > #define AVIC_GATAG_TO_VMID(x) ((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
> > > #define AVIC_GATAG_TO_VCPUID(x) (x & AVIC_VCPU_ID_MASK)
> > >
> > > +enum avic_modes {
> > > + AVIC_MODE_NONE = 0,
> > > + AVIC_MODE_X1,
> > > + AVIC_MODE_X2,
> > > +};
> > > +
> > > +static bool force_avic;
> > > +module_param_unsafe(force_avic, bool, 0444);
> > > +
> > > /* Note:
> > > * This hash table is used to map VM_ID to a struct kvm_svm,
> > > * when handling AMD IOMMU GALOG notification to schedule in
> > > @@ -50,6 +59,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
> > > static u32 next_vm_id = 0;
> > > static bool next_vm_id_wrapped = 0;
> > > static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
> > > +static enum avic_modes avic_mode;
> > >
> > > /*
> > > * This is a wrapper of struct amd_iommu_ir_data.
> > > @@ -1077,3 +1087,33 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
> > >
> > > avic_vcpu_load(vcpu);
> > > }
> > > +
> > > +/*
> > > + * Note:
> > > + * - The module param avic enable both xAPIC and x2APIC mode.
> > > + * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
> > > + * - The mode can be switched at run-time.
> > > + */
> > > +bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
> > > +{
> > > + if (!npt_enabled)
> > > + return false;
> > > +
> > > + if (boot_cpu_has(X86_FEATURE_AVIC)) {
> > > + avic_mode = AVIC_MODE_X1;
> > > + pr_info("AVIC enabled\n");
> > > + } else if (force_avic) {
> > > + pr_warn("AVIC is not supported in CPUID but force enabled");
> > > + pr_warn("Your system might crash and burn");
> > I think in this case avic_mode should also be set to AVIC_MODE_X1
> > (Hopefully this won't be needed for systems that have x2avic enabled)
>
> You are right. My appology :(
>
> Actually, x2AVIC depends on both CPUID bits (i.e. X86_FEATURE_AVIC and X86_FEATURE_X2AVIC).
> If the force_avic option is only applicable to only the X86_FEATURE_AVIC bit, we would
> need to check for the following condition before enabling x2AVIC support in the driver:
>
> if ((X86_FEATURE_AVIC | avic_force) & X86_FEATURE_X2AVIC)
> avic_mode = AVIC_MODE_X2
Let it be just for AVIC_MODE_X1, that is
else if (force_avic) {
pr_warn("AVIC is not supported in CPUID but force enabled");
pr_warn("Your system might crash and burn");
avic_mode = AVIC_MODE_X1;
Best regards,
Maxim Levitsky
>
> Regards,
> Suravee
>
>
Maxim,
On 5/4/22 7:12 PM, Maxim Levitsky wrote:
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index a8f514212b87..fc3ba6071482 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -40,6 +40,15 @@
>> #define AVIC_GATAG_TO_VMID(x) ((x >> AVIC_VCPU_ID_BITS) & AVIC_VM_ID_MASK)
>> #define AVIC_GATAG_TO_VCPUID(x) (x & AVIC_VCPU_ID_MASK)
>>
>> +enum avic_modes {
>> + AVIC_MODE_NONE = 0,
>> + AVIC_MODE_X1,
>> + AVIC_MODE_X2,
>> +};
>> +
>> +static bool force_avic;
>> +module_param_unsafe(force_avic, bool, 0444);
>> +
>> /* Note:
>> * This hash table is used to map VM_ID to a struct kvm_svm,
>> * when handling AMD IOMMU GALOG notification to schedule in
>> @@ -50,6 +59,7 @@ static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
>> static u32 next_vm_id = 0;
>> static bool next_vm_id_wrapped = 0;
>> static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
>> +static enum avic_modes avic_mode;
>>
>> /*
>> * This is a wrapper of struct amd_iommu_ir_data.
>> @@ -1077,3 +1087,33 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
>>
>> avic_vcpu_load(vcpu);
>> }
>> +
>> +/*
>> + * Note:
>> + * - The module param avic enable both xAPIC and x2APIC mode.
>> + * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
>> + * - The mode can be switched at run-time.
>> + */
>> +bool avic_hardware_setup(struct kvm_x86_ops *x86_ops)
>> +{
>> + if (!npt_enabled)
>> + return false;
>> +
>> + if (boot_cpu_has(X86_FEATURE_AVIC)) {
>> + avic_mode = AVIC_MODE_X1;
>> + pr_info("AVIC enabled\n");
>> + } else if (force_avic) {
>> + pr_warn("AVIC is not supported in CPUID but force enabled");
>> + pr_warn("Your system might crash and burn");
> I think in this case avic_mode should also be set to AVIC_MODE_X1
> (Hopefully this won't be needed for systems that have x2avic enabled)
You are right. My appology :(
Actually, x2AVIC depends on both CPUID bits (i.e. X86_FEATURE_AVIC and X86_FEATURE_X2AVIC).
If the force_avic option is only applicable to only the X86_FEATURE_AVIC bit, we would
need to check for the following condition before enabling x2AVIC support in the driver:
if ((X86_FEATURE_AVIC | avic_force) & X86_FEATURE_X2AVIC)
avic_mode = AVIC_MODE_X2
Regards,
Suravee