Re: [PATCH v4 5/6] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit

[Sean Christopherson <seanjc@google.com>]

On Mon, Aug 09, 2021, Zeng Guang wrote:
> Since IA x86 platform introduce features of IPI virtualization and
> User Interrupts, new behavior applies to the execution of WRMSR ICR

What do User Interrupts have to do with anything?

> register that causes APIC-write VM exit instead of MSR-write VM exit
> in x2APIC mode.

Please lead with what support is actually being added, and more directly state
what the new behavior actually is, e.g. when should KVM expect these types of
traps.  The shortlog helps a bit, but APIC-write is somewhat ambiguous without
the context that it refers to the trap-like exits, not exception-like exits on
the WRMSR itself.

Peeking ahead, this probably should be squashed with the next patch that adds
IPI virtualizatio support.  Without that patch's code that disables ICR MSR
intercepts for IPIv, this patch makes zero sense.

I'm not totally opposed to splitting IPIv support into two patches, I just don't
like splitting out this tiny subset that makes zero sense without the IPIv
code/context.  I assume you took this approach so that the shortlog could be
"KVM: VMX:" for the IPIv code.  IMO it's perfectly ok to keep that shortlog even
though there are minor changes outside of vmx/.  VMX is the only user of
kvm_apic_write_nodecode(), so it's not wrong to say it affects only VMX.

> This requires KVM to emulate writing 64-bit value to offset 300H on
> the virtual-APIC page(VICR) for guest running in x2APIC mode when

Maybe stylize that as vICR to make it stand out as virtual ICR?

> APIC-wrtie VM exit occurs. Prevoisely KVM doesn't consider this
       ^^^^^                 ^^^^^^^^^^
       write                 Previously

> situation as CPU never produce APIC-write VM exit in x2APIC mode before.
> 
> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
> ---
>  arch/x86/kvm/lapic.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index ba5a27879f1d..0b0f0ce96679 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2188,7 +2188,14 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>  	/* hw has done the conditional check and inst decode */
>  	offset &= 0xff0;
>  
> -	kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);

Probably worth snapshotting vcpu->arch.apic.

> +	if (apic_x2apic_mode(vcpu->arch.apic) && (offset == APIC_ICR)) {


A comment here would be _extremely_ helpful.  IIUC, this path is reached when IPIv
is enabled for all ICR writes that can't be virtualized, e.g. broadcast IPIs.

And I'm tempted to say this should WARN and do nothing if KVM gets an exit on
anything except ICR writes.

> +		u64 icr_val = *((u64 *)(vcpu->arch.apic->regs + offset));

Maybe just bump "val" to a u64?

Rather than open code this, can't this be:

		kvm_lapic_reg_read(apic, offset, 8, &val);
> +
> +		kvm_lapic_reg_write(vcpu->arch.apic, APIC_ICR2, (u32)(icr_val>>32));
> +		val = (u32)icr_val;

Hmm, this is the third path that open codes the ICR2:ICR split.  I think it's
probably worth adding a helper (patch below), and this can become:

void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
{
	struct kvm_lapic *apic = vcpu->arch.apic;
	u64 val = 0;

	/* hw has done the conditional check and inst decode */
	offset &= 0xff0;

	/* TODO: optimize to just emulate side effect w/o one more write */
	if (apic_x2apic_mode(apic)) {
		if (WARN_ON_ONCE(offset != APIC_ICR))
			return 1;

		kvm_lapic_reg_read(apic, offset, 8, &val);
		kvm_lapic_reg_write64(apic, offset, val);
	} else {
		kvm_lapic_reg_read(apic, offset, 4, &val);
		kvm_lapic_reg_write(apic, offset, val);
	}
}

There is some risk my idea will backfire if the CPU traps other WRMSRs, but even
then the pedant in me thinks the code for that should be:


	if (apic_x2apic_mode(apic)) {
		int size = offset == APIC_ICR ? 8 : 4;

		kvm_lapic_reg_read(apic, offset, size, &val);
		kvm_lapic_reg_write64(apic, offset, val);
	} else {
		...
	}

or worst case scenario, move the APIC_ICR check back so that the non-ICR path
back to "if (apic_x2apic_mode(vcpu->arch.apic) && (offset == APIC_ICR))" so that
it naturally falls into the 4-byte read+write.

> +	} else {
> +		kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
> +	}
>  
>  	/* TODO: optimize to just emulate side effect w/o one more write */
>  	kvm_lapic_reg_write(vcpu->arch.apic, offset, val);
> -- 
> 2.25.1


From c7641cf0c2ea2a1c5e6dda4007f8d285595ff82d Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 10 Sep 2021 15:07:57 -0700
Subject: [PATCH] KVM: x86: Add a helper to handle 64-bit APIC writes to ICR

Add a helper to handle 64-bit APIC writes, e.g. for x2APIC WRMSR, to
deduplicate the handling of ICR writes, which KVM needs to emulate as
back-to-back writes to ICR2 and then ICR.  Future support for IPI
virtualization will add yet another path where KVM must handle a 64-bit
APIC write.

Opportunistically fix the comment; ICR2 holds the destination (if there's
no shorthand), not the vector.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 76fb00921203..5f526ee10301 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2183,6 +2183,14 @@ void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);

+static int kvm_lapic_reg_write64(struct kvm_lapic *apic, u32 reg, u64 data)
+{
+	/* For 64-bit ICR writes, set ICR2 (dest) before ICR (command). */
+	if (reg == APIC_ICR)
+		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
+	return kvm_lapic_reg_write(apic, reg, (u32)data);
+}
+
 /* emulate APIC access in a trap manner */
 void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 {
@@ -2794,10 +2802,7 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	if (reg == APIC_ICR2)
 		return 1;

-	/* if this is ICR write vector before command */
-	if (reg == APIC_ICR)
-		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
-	return kvm_lapic_reg_write(apic, reg, (u32)data);
+	return kvm_lapic_reg_write64(apic, reg, data);
 }

 int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
@@ -2828,10 +2833,7 @@ int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
 	if (!lapic_in_kernel(vcpu))
 		return 1;

-	/* if this is ICR write vector before command */
-	if (reg == APIC_ICR)
-		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
-	return kvm_lapic_reg_write(apic, reg, (u32)data);
+	return kvm_lapic_reg_write64(apic, reg, data);
 }

 int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
--