On 8/22/24 13:44, Sean Christopherson wrote:
> +Tom
>
> Can someone from AMD confirm that this is indeed the behavior, and that for AMD
> CPUs, it's the architectural behavior?
In section "16.11 Accessing x2APIC Register" of APM Vol 2, there is this
statement:
"For 64-bit x2APIC registers, the high-order bits (bits 63:32) are
mapped to EDX[31:0]"
and in section "16.11.1 x2APIC Register Address Space" of APM Vol 2,
there is this statement:
"The two 32-bit Interrupt Command Registers in APIC mode (MMIO offsets
300h and 310h) are merged into a single 64-bit x2APIC register at MSR
address 830h."
So I believe this isn't necessary. @Suravee, agree?
Are you seeing a bug related to this?
Thanks,
Tom
>
> A sanity check on the code would be appreciated too, it'd be nice to get this
> into v6.12.
>
> Thanks!
>
> On Fri, Jul 19, 2024, Sean Christopherson wrote:
>> Re-introduce the "split" x2APIC ICR storage that KVM used prior to Intel's
>> IPI virtualization support, but only for AMD. While not stated anywhere
>> in the APM, despite stating the ICR is a single 64-bit register, AMD CPUs
>> store the 64-bit ICR as two separate 32-bit values in ICR and ICR2. When
>> IPI virtualization (IPIv on Intel, all AVIC flavors on AMD) is enabled,
>> KVM needs to match CPU behavior as some ICR ICR writes will be handled by
>> the CPU, not by KVM.
>>
>> Add a kvm_x86_ops knob to control the underlying format used by the CPU to
>> store the x2APIC ICR, and tune it to AMD vs. Intel regardless of whether
>> or not x2AVIC is enabled. If KVM is handling all ICR writes, the storage
>> format for x2APIC mode doesn't matter, and having the behavior follow AMD
>> versus Intel will provide better test coverage and ease debugging.
>>
>> Fixes: 4d1d7942e36a ("KVM: SVM: Introduce logic to (de)activate x2AVIC mode")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Cc: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
>> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> ---
>> arch/x86/include/asm/kvm_host.h | 2 ++
>> arch/x86/kvm/lapic.c | 42 +++++++++++++++++++++++----------
>> arch/x86/kvm/svm/svm.c | 2 ++
>> arch/x86/kvm/vmx/main.c | 2 ++
>> 4 files changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 950a03e0181e..edc235521434 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1726,6 +1726,8 @@ struct kvm_x86_ops {
>> void (*enable_nmi_window)(struct kvm_vcpu *vcpu);
>> void (*enable_irq_window)(struct kvm_vcpu *vcpu);
>> void (*update_cr8_intercept)(struct kvm_vcpu *vcpu, int tpr, int irr);
>> +
>> + const bool x2apic_icr_is_split;
>> const unsigned long required_apicv_inhibits;
>> bool allow_apicv_in_x2apic_without_x2apic_virtualization;
>> void (*refresh_apicv_exec_ctrl)(struct kvm_vcpu *vcpu);
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index d14ef485b0bd..cc0a1008fae4 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2473,11 +2473,25 @@ int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data)
>> data &= ~APIC_ICR_BUSY;
>>
>> kvm_apic_send_ipi(apic, (u32)data, (u32)(data >> 32));
>> - kvm_lapic_set_reg64(apic, APIC_ICR, data);
>> + if (kvm_x86_ops.x2apic_icr_is_split) {
>> + kvm_lapic_set_reg(apic, APIC_ICR, data);
>> + kvm_lapic_set_reg(apic, APIC_ICR2, data >> 32);
>> + } else {
>> + kvm_lapic_set_reg64(apic, APIC_ICR, data);
>> + }
>> trace_kvm_apic_write(APIC_ICR, data);
>> return 0;
>> }
>>
>> +static u64 kvm_x2apic_icr_read(struct kvm_lapic *apic)
>> +{
>> + if (kvm_x86_ops.x2apic_icr_is_split)
>> + return (u64)kvm_lapic_get_reg(apic, APIC_ICR) |
>> + (u64)kvm_lapic_get_reg(apic, APIC_ICR2) << 32;
>> +
>> + return kvm_lapic_get_reg64(apic, APIC_ICR);
>> +}
>> +
>> /* emulate APIC access in a trap manner */
>> void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>> {
>> @@ -2495,7 +2509,7 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>> * maybe-unecessary write, and both are in the noise anyways.
>> */
>> if (apic_x2apic_mode(apic) && offset == APIC_ICR)
>> - WARN_ON_ONCE(kvm_x2apic_icr_write(apic, kvm_lapic_get_reg64(apic, APIC_ICR)));
>> + WARN_ON_ONCE(kvm_x2apic_icr_write(apic, kvm_x2apic_icr_read(apic)));
>> else
>> kvm_lapic_reg_write(apic, offset, kvm_lapic_get_reg(apic, offset));
>> }
>> @@ -3005,18 +3019,22 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
>>
>> /*
>> * In x2APIC mode, the LDR is fixed and based on the id. And
>> - * ICR is internally a single 64-bit register, but needs to be
>> - * split to ICR+ICR2 in userspace for backwards compatibility.
>> + * if the ICR is _not_ split, ICR is internally a single 64-bit
>> + * register, but needs to be split to ICR+ICR2 in userspace for
>> + * backwards compatibility.
>> */
>> - if (set) {
>> + if (set)
>> *ldr = kvm_apic_calc_x2apic_ldr(*id);
>>
>> - icr = __kvm_lapic_get_reg(s->regs, APIC_ICR) |
>> - (u64)__kvm_lapic_get_reg(s->regs, APIC_ICR2) << 32;
>> - __kvm_lapic_set_reg64(s->regs, APIC_ICR, icr);
>> - } else {
>> - icr = __kvm_lapic_get_reg64(s->regs, APIC_ICR);
>> - __kvm_lapic_set_reg(s->regs, APIC_ICR2, icr >> 32);
>> + if (!kvm_x86_ops.x2apic_icr_is_split) {
>> + if (set) {
>> + icr = __kvm_lapic_get_reg(s->regs, APIC_ICR) |
>> + (u64)__kvm_lapic_get_reg(s->regs, APIC_ICR2) << 32;
>> + __kvm_lapic_set_reg64(s->regs, APIC_ICR, icr);
>> + } else {
>> + icr = __kvm_lapic_get_reg64(s->regs, APIC_ICR);
>> + __kvm_lapic_set_reg(s->regs, APIC_ICR2, icr >> 32);
>> + }
>> }
>> }
>>
>> @@ -3214,7 +3232,7 @@ static int kvm_lapic_msr_read(struct kvm_lapic *apic, u32 reg, u64 *data)
>> u32 low;
>>
>> if (reg == APIC_ICR) {
>> - *data = kvm_lapic_get_reg64(apic, APIC_ICR);
>> + *data = kvm_x2apic_icr_read(apic);
>> return 0;
>> }
>>
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index c115d26844f7..04c113386de6 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -5049,6 +5049,8 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
>> .enable_nmi_window = svm_enable_nmi_window,
>> .enable_irq_window = svm_enable_irq_window,
>> .update_cr8_intercept = svm_update_cr8_intercept,
>> +
>> + .x2apic_icr_is_split = true,
>> .set_virtual_apic_mode = avic_refresh_virtual_apic_mode,
>> .refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
>> .apicv_post_state_restore = avic_apicv_post_state_restore,
>> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
>> index 0bf35ebe8a1b..a70699665e11 100644
>> --- a/arch/x86/kvm/vmx/main.c
>> +++ b/arch/x86/kvm/vmx/main.c
>> @@ -89,6 +89,8 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>> .enable_nmi_window = vmx_enable_nmi_window,
>> .enable_irq_window = vmx_enable_irq_window,
>> .update_cr8_intercept = vmx_update_cr8_intercept,
>> +
>> + .x2apic_icr_is_split = false,
>> .set_virtual_apic_mode = vmx_set_virtual_apic_mode,
>> .set_apic_access_page_addr = vmx_set_apic_access_page_addr,
>> .refresh_apicv_exec_ctrl = vmx_refresh_apicv_exec_ctrl,
>> --
>> 2.45.2.1089.g2a221341d9-goog
>>
