Originally, AMD SVM AVIC supports 8-bit host physical APIC ID.
However, newer AMD systems can have physical APIC ID larger than 255,
and AVIC hardware has been extended to support upto the maximum physical
APIC ID available in the system.
This series introduces a helper function in the APIC subsystem to get
the maximum physical APIC ID allowing the SVM AVIC driver to calculate
the number of bits to program the host physical APIC ID in the AVIC
physical APIC ID table entry.
Regards,
Suravee Suthikulpanit
Changes from V1 (https://lkml.org/lkml/2021/11/10/243) :
* Refactor AVIC hardware setup code into a helper function to simplify
logic for enabling AVIC. (patch 1/3)
* Simplify the logic in avic_init_host_physical_apicid_mask() by
by removing the CPUID[0xb,0x1] check for processor level APIC ID
mask. (patch 3/3)
Suravee Suthikulpanit (3):
KVM: SVM: Refactor AVIC hardware setup logic into helper function
x86/apic: Add helper function to get maximum physical APIC ID
KVM: SVM: Extend host physical APIC ID field to support more than
8-bit
arch/x86/include/asm/apic.h | 1 +
arch/x86/kernel/apic/apic.c | 6 ++++++
arch/x86/kvm/svm/avic.c | 38 +++++++++++++++++++++++++++++--------
arch/x86/kvm/svm/svm.c | 8 +-------
arch/x86/kvm/svm/svm.h | 2 +-
5 files changed, 39 insertions(+), 16 deletions(-)
--
2.25.1
The AVIC physical APIC ID table entry contains the host physical
APIC ID field, which the hardware uses to keep track of where each
vCPU is running. Originally, the field is an 8-bit value, which can
only support physical APIC ID up to 255.
To support system with larger APIC ID, the AVIC hardware extends
this field to support up to the largest possible physical APIC ID
available on the system.
Therefore, replace the hard-coded mask value with the value
calculated from the maximum possible physical APIC ID in the system.
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/kvm/svm/avic.c | 28 ++++++++++++++++++++--------
arch/x86/kvm/svm/svm.h | 1 -
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 6aca1682f4b7..2a0f58e6edf5 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -19,6 +19,7 @@
#include <linux/amd-iommu.h>
#include <linux/kvm_host.h>
+#include <asm/apic.h>
#include <asm/irq_remapping.h>
#include "trace.h"
@@ -63,6 +64,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 u64 avic_host_physical_id_mask;
static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
/*
@@ -133,6 +135,20 @@ void avic_vm_destroy(struct kvm *kvm)
spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
}
+static void avic_init_host_physical_apicid_mask(void)
+{
+ if (!x2apic_mode) {
+ /* If host is in xAPIC mode, default to only 8-bit mask. */
+ avic_host_physical_id_mask = 0xffULL;
+ } else {
+ u32 count = get_count_order(apic_get_max_phys_apicid());
+
+ avic_host_physical_id_mask = BIT(count) - 1;
+ }
+ pr_debug("Using AVIC host physical APIC ID mask %#0llx\n",
+ avic_host_physical_id_mask);
+}
+
int avic_vm_init(struct kvm *kvm)
{
unsigned long flags;
@@ -943,22 +959,17 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
{
u64 entry;
- /* ID = 0xff (broadcast), ID > 0xff (reserved) */
int h_physical_id = kvm_cpu_get_apicid(cpu);
struct vcpu_svm *svm = to_svm(vcpu);
- /*
- * Since the host physical APIC id is 8 bits,
- * we can support host APIC ID upto 255.
- */
- if (WARN_ON(h_physical_id > AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
+ if (WARN_ON(h_physical_id > avic_host_physical_id_mask))
return;
entry = READ_ONCE(*(svm->avic_physical_id_cache));
WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
- entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
- entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
+ entry &= ~avic_host_physical_id_mask;
+ entry |= h_physical_id;
entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
if (svm->avic_is_running)
@@ -1018,6 +1029,7 @@ bool avic_hardware_setup(bool avic, bool npt)
return false;
pr_info("AVIC enabled\n");
+ avic_init_host_physical_apicid_mask();
amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
return true;
}
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 1d2d72e56dd1..b4cb71c538b3 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -497,7 +497,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
#define AVIC_LOGICAL_ID_ENTRY_VALID_BIT 31
#define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1 << 31)
-#define AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK (0xFFULL)
#define AVIC_PHYSICAL_ID_ENTRY_BACKING_PAGE_MASK (0xFFFFFFFFFFULL << 12)
#define AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK (1ULL << 62)
#define AVIC_PHYSICAL_ID_ENTRY_VALID_MASK (1ULL << 63)
--
2.25.1
To prepare for upcoming AVIC changes. There is no functional change.
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/kvm/svm/avic.c | 10 ++++++++++
arch/x86/kvm/svm/svm.c | 8 +-------
arch/x86/kvm/svm/svm.h | 1 +
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 8052d92069e0..6aca1682f4b7 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1011,3 +1011,13 @@ void svm_vcpu_unblocking(struct kvm_vcpu *vcpu)
kvm_vcpu_update_apicv(vcpu);
avic_set_running(vcpu, true);
}
+
+bool avic_hardware_setup(bool avic, bool npt)
+{
+ if (!avic || !npt || !boot_cpu_has(X86_FEATURE_AVIC))
+ return false;
+
+ pr_info("AVIC enabled\n");
+ amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
+ return true;
+}
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 989685098b3e..d23bc7a7c48e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1031,13 +1031,7 @@ static __init int svm_hardware_setup(void)
nrips = false;
}
- enable_apicv = avic = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
-
- if (enable_apicv) {
- pr_info("AVIC enabled\n");
-
- amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
- }
+ enable_apicv = avic = avic_hardware_setup(avic, npt_enabled);
if (vls) {
if (!npt_enabled ||
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5d30db599e10..1d2d72e56dd1 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -515,6 +515,7 @@ static inline bool avic_vcpu_is_running(struct kvm_vcpu *vcpu)
return (READ_ONCE(*entry) & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
}
+bool avic_hardware_setup(bool avic, bool npt);
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
Export the max_physical_apicid via a helper function since this information
is required by AMD SVM AVIC support.
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/include/asm/apic.h | 1 +
arch/x86/kernel/apic/apic.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 48067af94678..77d9cb2a7e28 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -435,6 +435,7 @@ static inline void apic_set_eoi_write(void (*eoi_write)(u32 reg, u32 v)) {}
#endif /* CONFIG_X86_LOCAL_APIC */
extern void apic_ack_irq(struct irq_data *data);
+extern u32 apic_get_max_phys_apicid(void);
static inline void ack_APIC_irq(void)
{
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..47653d8c05f2 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2361,6 +2361,12 @@ bool apic_id_is_primary_thread(unsigned int apicid)
}
#endif
+u32 apic_get_max_phys_apicid(void)
+{
+ return max_physical_apicid;
+}
+EXPORT_SYMBOL_GPL(apic_get_max_phys_apicid);
+
/*
* Should use this API to allocate logical CPU IDs to keep nr_logical_cpuids
* and cpuid_to_apicid[] synchronized.
--
2.25.1
On Thu, 2021-12-02 at 17:58 -0600, Suravee Suthikulpanit wrote:
> To prepare for upcoming AVIC changes. There is no functional change.
>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> arch/x86/kvm/svm/avic.c | 10 ++++++++++
> arch/x86/kvm/svm/svm.c | 8 +-------
> arch/x86/kvm/svm/svm.h | 1 +
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 8052d92069e0..6aca1682f4b7 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1011,3 +1011,13 @@ void svm_vcpu_unblocking(struct kvm_vcpu *vcpu)
> kvm_vcpu_update_apicv(vcpu);
> avic_set_running(vcpu, true);
> }
> +
> +bool avic_hardware_setup(bool avic, bool npt)
> +{
> + if (!avic || !npt || !boot_cpu_has(X86_FEATURE_AVIC))
> + return false;
Nitpick: Why to pass these as local variables? npt_enabled for example is
used in many places directly.
> +
> + pr_info("AVIC enabled\n");
> + amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> + return true;
> +}
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 989685098b3e..d23bc7a7c48e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1031,13 +1031,7 @@ static __init int svm_hardware_setup(void)
> nrips = false;
> }
>
> - enable_apicv = avic = avic && npt_enabled && boot_cpu_has(X86_FEATURE_AVIC);
> -
> - if (enable_apicv) {
> - pr_info("AVIC enabled\n");
> -
> - amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> - }
> + enable_apicv = avic = avic_hardware_setup(avic, npt_enabled);
>
> if (vls) {
> if (!npt_enabled ||
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5d30db599e10..1d2d72e56dd1 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -515,6 +515,7 @@ static inline bool avic_vcpu_is_running(struct kvm_vcpu *vcpu)
> return (READ_ONCE(*entry) & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
> }
>
> +bool avic_hardware_setup(bool avic, bool npt);
> int avic_ga_log_notifier(u32 ga_tag);
> void avic_vm_destroy(struct kvm *kvm);
> int avic_vm_init(struct kvm *kvm);
Best regards,
Maxim Levitsky
On Thu, 2021-12-02 at 17:58 -0600, Suravee Suthikulpanit wrote:
> The AVIC physical APIC ID table entry contains the host physical
> APIC ID field, which the hardware uses to keep track of where each
> vCPU is running. Originally, the field is an 8-bit value, which can
> only support physical APIC ID up to 255.
>
> To support system with larger APIC ID, the AVIC hardware extends
> this field to support up to the largest possible physical APIC ID
> available on the system.
>
> Therefore, replace the hard-coded mask value with the value
> calculated from the maximum possible physical APIC ID in the system.
>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> arch/x86/kvm/svm/avic.c | 28 ++++++++++++++++++++--------
> arch/x86/kvm/svm/svm.h | 1 -
> 2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 6aca1682f4b7..2a0f58e6edf5 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -19,6 +19,7 @@
> #include <linux/amd-iommu.h>
> #include <linux/kvm_host.h>
>
> +#include <asm/apic.h>
> #include <asm/irq_remapping.h>
>
> #include "trace.h"
> @@ -63,6 +64,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 u64 avic_host_physical_id_mask;
> static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
>
> /*
> @@ -133,6 +135,20 @@ void avic_vm_destroy(struct kvm *kvm)
> spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
> }
>
> +static void avic_init_host_physical_apicid_mask(void)
> +{
> + if (!x2apic_mode) {
Wonder why this is a exported global variable and not function.
Not the patch fault though.
> + /* If host is in xAPIC mode, default to only 8-bit mask. */
> + avic_host_physical_id_mask = 0xffULL;
> + } else {
> + u32 count = get_count_order(apic_get_max_phys_apicid());
> +
> + avic_host_physical_id_mask = BIT(count) - 1;
I think that there were some complains about using this macro and instead encouraged
to use 1 << x directly, but I see it used already in other places in avic.c so I don't know.
> + }
> + pr_debug("Using AVIC host physical APIC ID mask %#0llx\n",
> + avic_host_physical_id_mask);
> +}
> +
> int avic_vm_init(struct kvm *kvm)
> {
> unsigned long flags;
> @@ -943,22 +959,17 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
> void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> u64 entry;
> - /* ID = 0xff (broadcast), ID > 0xff (reserved) */
> int h_physical_id = kvm_cpu_get_apicid(cpu);
> struct vcpu_svm *svm = to_svm(vcpu);
>
> - /*
> - * Since the host physical APIC id is 8 bits,
> - * we can support host APIC ID upto 255.
> - */
> - if (WARN_ON(h_physical_id > AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK))
> + if (WARN_ON(h_physical_id > avic_host_physical_id_mask))
> return;
>
> entry = READ_ONCE(*(svm->avic_physical_id_cache));
> WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
>
> - entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK;
> - entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK);
> + entry &= ~avic_host_physical_id_mask;
> + entry |= h_physical_id;
>
> entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
> if (svm->avic_is_running)
> @@ -1018,6 +1029,7 @@ bool avic_hardware_setup(bool avic, bool npt)
> return false;
>
> pr_info("AVIC enabled\n");
> + avic_init_host_physical_apicid_mask();
> amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
> return true;
> }
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 1d2d72e56dd1..b4cb71c538b3 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -497,7 +497,6 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
> #define AVIC_LOGICAL_ID_ENTRY_VALID_BIT 31
> #define AVIC_LOGICAL_ID_ENTRY_VALID_MASK (1 << 31)
>
> -#define AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK (0xFFULL)
> #define AVIC_PHYSICAL_ID_ENTRY_BACKING_PAGE_MASK (0xFFFFFFFFFFULL << 12)
> #define AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK (1ULL << 62)
> #define AVIC_PHYSICAL_ID_ENTRY_VALID_MASK (1ULL << 63)
Reviewed-by: Maxim Levitsky <[email protected]>
Best regards,
Maxim Levitsky
On 12/3/21 1:46 AM, Maxim Levitsky wrote:
> On Thu, 2021-12-02 at 17:58 -0600, Suravee Suthikulpanit wrote:
>> @@ -63,6 +64,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 u64 avic_host_physical_id_mask;
>> static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
>>
>> /*
>> @@ -133,6 +135,20 @@ void avic_vm_destroy(struct kvm *kvm)
>> spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>> }
>>
>> +static void avic_init_host_physical_apicid_mask(void)
>> +{
>> + if (!x2apic_mode) {
> Wonder why this is a exported global variable and not function.
> Not the patch fault though.
>> + /* If host is in xAPIC mode, default to only 8-bit mask. */
>> + avic_host_physical_id_mask = 0xffULL;
>> + } else {
>> + u32 count = get_count_order(apic_get_max_phys_apicid());
>> +
>> + avic_host_physical_id_mask = BIT(count) - 1;
> I think that there were some complains about using this macro and instead encouraged
> to use 1 << x directly, but I see it used already in other places in avic.c so I don't know.
And I think it should be BIT_ULL() since avic_host_physical_id_mask is a u64.
Thanks,
Tom
>
Hi Maxim,
On 12/3/2021 2:39 PM, Maxim Levitsky wrote:
> On Thu, 2021-12-02 at 17:58 -0600, Suravee Suthikulpanit wrote:
>> To prepare for upcoming AVIC changes. There is no functional change.
>>
>> Signed-off-by: Suravee Suthikulpanit<[email protected]>
>> ---
>> arch/x86/kvm/svm/avic.c | 10 ++++++++++
>> arch/x86/kvm/svm/svm.c | 8 +-------
>> arch/x86/kvm/svm/svm.h | 1 +
>> 3 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 8052d92069e0..6aca1682f4b7 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -1011,3 +1011,13 @@ void svm_vcpu_unblocking(struct kvm_vcpu *vcpu)
>> kvm_vcpu_update_apicv(vcpu);
>> avic_set_running(vcpu, true);
>> }
>> +
>> +bool avic_hardware_setup(bool avic, bool npt)
>> +{
>> + if (!avic || !npt || !boot_cpu_has(X86_FEATURE_AVIC))
>> + return false;
>
> Nitpick: Why to pass these as local variables? npt_enabled for example is
> used in many places directly.
Good point. I didn't see that it's already declared as extern in the svm.h. I'll update this in V3.
Thanks,
Suravee
On 12/3/2021 2:46 PM, Maxim Levitsky wrote:
>
>
> Reviewed-by: Maxim Levitsky <[email protected]>
>
> Best regards,
> Maxim Levitsky
>
Thanks for the review.
Regards,
Suravee
On 12/3/2021 11:34 PM, Tom Lendacky wrote:
> On 12/3/21 1:46 AM, Maxim Levitsky wrote:
>> On Thu, 2021-12-02 at 17:58 -0600, Suravee Suthikulpanit wrote:
>
>>> @@ -63,6 +64,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 u64 avic_host_physical_id_mask;
>>> static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
>>> /*
>>> @@ -133,6 +135,20 @@ void avic_vm_destroy(struct kvm *kvm)
>>> spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>>> }
>>> +static void avic_init_host_physical_apicid_mask(void)
>>> +{
>>> + if (!x2apic_mode) {
>> Wonder why this is a exported global variable and not function.
>> Not the patch fault though.
>>> + /* If host is in xAPIC mode, default to only 8-bit mask. */
>>> + avic_host_physical_id_mask = 0xffULL;
>>> + } else {
>>> + u32 count = get_count_order(apic_get_max_phys_apicid());
>>> +
>>> + avic_host_physical_id_mask = BIT(count) - 1;
>> I think that there were some complains about using this macro and instead encouraged
>> to use 1 << x directly, but I see it used already in other places in avic.c so I don't know.
>
> And I think it should be BIT_ULL() since avic_host_physical_id_mask is a u64.
>
> Thanks,
> Tom
I am not sure about complains on the use of the BIT macros. However, we can just use BIT_ULL() for now
and clean up the whole file at once later if needed.
Regards,
Suravee