+Tom
Can someone from AMD confirm that this is indeed the behavior, and that for AMD
CPUs, it's the architectural behavior?
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
>
