Re: [PATCH v2 03/10] KVM: x86: Re-split x2APIC ICR into ICR+ICR2 for AMD (x2AVIC)

[Sean Christopherson <seanjc@google.com>]

+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@vger.kernel.org
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  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
>