| Message ID | 20220209152038.12303-1-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] KVM: SVM: Allow AVIC support on system w/ physical APIC ID > 255 | expand |
On Wed, Feb 09, 2022, Suravee Suthikulpanit wrote: > AVIC physical APIC ID table entry contains host physical > APIC ID field, which is used by the hardware to keep track of where > each vCPU is running. Originally, this field is 8-bit when AVIC was > introduced. > > The AMD64 Architecture Programmer’s Manual Volume 2 revision 3.38 > specify the physical APIC ID table entry bit [11:8] as reserved > for older generation of AVIC hardware. I'd prefer to explicitly call out that older versions of the APM were buggy, it's easy to overlook the importance of the "revision 3.38" blurb. > For newer hardware, this field is used to specify host physical APIC ID > larger than 255. For all intents and purposed the field was _architecturally_ always 12 bits, the APM just did a poor job of documenting that the number of bits that are actually consumed by the CPU is model specific behavior. > Unfortunately, there is no CPUID bit to help determine if AVIC hardware > can support 12-bit host physical APIC ID. For older system, since > the reserved bits [11:8] is documented as should be zero, it should be safe Please don't hedge, "it should be safe" implies we aren't confident about writing zeroes, but KVM already writes zeroes to the reserved bits. The changelog could instead call out that KVM trusts hardware to (a) not generate bogus x2APIC IDs and (b) to always support at least 8 bits. > to increase the host physical ID mask to 12 bits and clear the field > when programing new physical APIC ID. E.g. Expand KVM's mask for the AVIC host physical ID to the full 12 bits defined by the architecture. The number of bits consumed by hardware is model specific, e.g. early CPUs ignored bits 11:8, but there is no way for KVM to enumerate the "true" size. So, KVM must allow using all bits, else it risks rejecting completely legal x2APIC IDs on newer CPUs. This means KVM relies on hardware to not assign x2APIC IDs that exceed the "true" width of the field, but presmuably hardware is smart enough to tie the width to the max x2APIC ID. KVM also relies on hardware to support at least 8 bits, as the legacy xAPIC ID is writable by software. But, those assumptions are unavoidable due to the lack of any way to enumerate the "true" width. Note, older versions of the APM state that bits 11:8 are reserved for legacy xAPIC, but consumed for x2APIC. Revision 3.38 corrected this to state that bits 11:8 are "should be zero" on older hardware. > Suggested-by: Sean Christopherson <seanjc@google.com> > Cc: Maxim Levitsky <mlevitsk@redhat.com> Cc: stable@vger.kernel.org Fixes: 44a95dae1d22 ("KVM: x86: Detect and Initialize AVIC support") > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> > --- > arch/x86/kvm/svm/avic.c | 8 ++------ > arch/x86/kvm/svm/svm.h | 2 +- > 2 files changed, 3 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c > index 90364d02f22a..54ad98731181 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> Unnecessary new include. With the above addressed, Reviewed-by: Sean Christopherson <seanjc@google.com>
On 2/10/2022 12:54 AM, Sean Christopherson wrote: > E.g. > > Expand KVM's mask for the AVIC host physical ID to the full 12 bits defined > by the architecture. The number of bits consumed by hardware is model > specific, e.g. early CPUs ignored bits 11:8, but there is no way for KVM > to enumerate the "true" size. So, KVM must allow using all bits, else it > risks rejecting completely legal x2APIC IDs on newer CPUs. > > This means KVM relies on hardware to not assign x2APIC IDs that exceed the > "true" width of the field, but presmuably hardware is smart enough to tie > the width to the max x2APIC ID. KVM also relies on hardware to support at > least 8 bits, as the legacy xAPIC ID is writable by software. But, those > assumptions are unavoidable due to the lack of any way to enumerate the > "true" width. > > Note, older versions of the APM state that bits 11:8 are reserved for > legacy xAPIC, but consumed for x2APIC. Revision 3.38 corrected this to > state that bits 11:8 are "should be zero" on older hardware. This last paragraph is incorrect. The APM revision 3.38 and prior states Reserved/SBZ for legacy APIC; extension of Host Physical APIC ID when x2APIC is enabled. Here, "SBZ" means Should Be Zero. Therefore, I will remove the last paragraph in V6. Suravee
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c index 90364d02f22a..54ad98731181 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" @@ -974,17 +975,12 @@ 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); lockdep_assert_preemption_disabled(); - /* - * 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_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK)) return; /* diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 47ef8f4a9358..cede59cd8999 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -565,7 +565,7 @@ 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_HOST_PHYSICAL_ID_MASK GENMASK_ULL(11, 0) #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)
AVIC physical APIC ID table entry contains host physical APIC ID field, which is used by the hardware to keep track of where each vCPU is running. Originally, this field is 8-bit when AVIC was introduced. The AMD64 Architecture Programmer’s Manual Volume 2 revision 3.38 specify the physical APIC ID table entry bit [11:8] as reserved for older generation of AVIC hardware. For newer hardware, this field is used to specify host physical APIC ID larger than 255. Unfortunately, there is no CPUID bit to help determine if AVIC hardware can support 12-bit host physical APIC ID. For older system, since the reserved bits [11:8] is documented as should be zero, it should be safe to increase the host physical ID mask to 12 bits and clear the field when programing new physical APIC ID. Suggested-by: Sean Christopherson <seanjc@google.com> Cc: Maxim Levitsky <mlevitsk@redhat.com> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> --- arch/x86/kvm/svm/avic.c | 8 ++------ arch/x86/kvm/svm/svm.h | 2 +- 2 files changed, 3 insertions(+), 7 deletions(-)