[PATCH 00/11] KVM: x86: Prep work for VMX IPI virtualization

[PATCH 00/11] KVM: x86: Prep work for VMX IPI virtualization

[Sean Christopherson]

Prepare for VMX's IPI virtualization, in which hardware treats ICR as a
single 64-bit register in x2APIC mode.  The SDM wasn't clear on how ICR
should be modeled, KVM just took the easier path and guessed wrong.

Hardware's implementation of ICR as a 64-bit register requires explicit
handling to maintain backwards compatibility in KVM_{G,S}ET_REG, as
migrating a VM between hosts with different IPI virtualization support
would lead to ICR "corruption" for writes that aren't intercepted by
KVM (hardware doesn't fill ICR2 in vAPIC page).

This series includes AVIC cleanups for things I encountered along the way.
AVIC still has multiple issues, this only fixes the easy bugs.

Sean Christopherson (11):
  Revert "svm: Add warning message for AVIC IPI invalid target"
  KVM: VMX: Handle APIC-write offset wrangling in VMX code
  KVM: x86: Use "raw" APIC register read for handling APIC-write VM-Exit
  KVM: SVM: Use common kvm_apic_write_nodecode() for AVIC write traps
  KVM: SVM: Don't rewrite guest ICR on AVIC IPI virtualization failure
  KVM: x86: WARN if KVM emulates an IPI without clearing the BUSY flag
  KVM: x86: Make kvm_lapic_reg_{read,write}() static
  KVM: x86: Add helpers to handle 64-bit APIC MSR read/writes
  KVM: x86: Treat x2APIC's ICR as a 64-bit register, not two 32-bit regs
  KVM: x86: Make kvm_lapic_set_reg() a "private" xAPIC helper
  KVM: selftests: Add test to verify KVM handles x2APIC ICR=>ICR2 dance

 arch/x86/kvm/lapic.c                          | 193 ++++++++++++------
 arch/x86/kvm/lapic.h                          |  21 +-
 arch/x86/kvm/svm/avic.c                       |  38 ++--
 arch/x86/kvm/trace.h                          |   6 +-
 arch/x86/kvm/vmx/vmx.c                        |  11 +-
 arch/x86/kvm/x86.c                            |  15 +-
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/apic.h       |   1 +
 .../selftests/kvm/x86_64/xapic_state_test.c   | 150 ++++++++++++++
 10 files changed, 325 insertions(+), 112 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xapic_state_test.c


base-commit: 17179d0068b20413de2355f84c75a93740257e20
-- 
2.35.0.263.gb82422642f-goog

[PATCH 01/11] Revert "svm: Add warning message for AVIC IPI invalid target"

[Sean Christopherson]

Remove a WARN on an "AVIC IPI invalid target" exit, the WARN is trivial
to trigger from guest as it will fail on any destination APIC ID that
doesn't exist from the guest's perspective.

Don't bother recording anything in the kernel log, the common tracepoint
for kvm_avic_incomplete_ipi() is sufficient for debugging.

This reverts commit 37ef0c4414c9743ba7f1af4392f0a27a99649f2a.

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/avic.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 90364d02f22a..ecc81c48c0ca 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -345,8 +345,6 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
 		avic_kick_target_vcpus(vcpu->kvm, apic, icrl, icrh);
 		break;
 	case AVIC_IPI_FAILURE_INVALID_TARGET:
-		WARN_ONCE(1, "Invalid IPI target: index=%u, vcpu=%d, icr=%#0x:%#0x\n",
-			  index, vcpu->vcpu_id, icrh, icrl);
 		break;
 	case AVIC_IPI_FAILURE_INVALID_BACKING_PAGE:
 		WARN_ONCE(1, "Invalid backing page\n");
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 01/11] Revert "svm: Add warning message for AVIC IPI invalid target"

[Paolo Bonzini]

On 2/4/22 22:41, Sean Christopherson wrote:
> Remove a WARN on an "AVIC IPI invalid target" exit, the WARN is trivial
> to trigger from guest as it will fail on any destination APIC ID that
> doesn't exist from the guest's perspective.
> 
> Don't bother recording anything in the kernel log, the common tracepoint
> for kvm_avic_incomplete_ipi() is sufficient for debugging.
> 
> This reverts commit 37ef0c4414c9743ba7f1af4392f0a27a99649f2a.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/avic.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 90364d02f22a..ecc81c48c0ca 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -345,8 +345,6 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
>   		avic_kick_target_vcpus(vcpu->kvm, apic, icrl, icrh);
>   		break;
>   	case AVIC_IPI_FAILURE_INVALID_TARGET:
> -		WARN_ONCE(1, "Invalid IPI target: index=%u, vcpu=%d, icr=%#0x:%#0x\n",
> -			  index, vcpu->vcpu_id, icrh, icrl);
>   		break;
>   	case AVIC_IPI_FAILURE_INVALID_BACKING_PAGE:
>   		WARN_ONCE(1, "Invalid backing page\n");

Queued for 5.17, thanks.

Paolo

[PATCH 02/11] KVM: VMX: Handle APIC-write offset wrangling in VMX code

[Sean Christopherson]

Move the vAPIC offset adjustments done in the APIC-write trap path from
common x86 to VMX in anticipation of using the nodecode path for SVM's
AVIC.  The adjustment reflects hardware behavior, i.e. it's technically a
property of VMX, no common x86.  SVM's AVIC behavior is identical, so
it's a bit of a moot point, the goal is purely to make it easier to
understand why the adjustment is ok.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c   |  3 ---
 arch/x86/kvm/vmx/vmx.c | 11 +++++++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4662469240bc..fbce455a9d17 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2188,9 +2188,6 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 {
 	u32 val = 0;
 
-	/* hw has done the conditional check and inst decode */
-	offset &= 0xff0;
-
 	kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
 
 	/* TODO: optimize to just emulate side effect w/o one more write */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b1165bb13a5a..1b135473677b 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5302,9 +5302,16 @@ static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
 static int handle_apic_write(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification = vmx_get_exit_qual(vcpu);
-	u32 offset = exit_qualification & 0xfff;
 
-	/* APIC-write VM exit is trap-like and thus no need to adjust IP */
+	/*
+	 * APIC-write VM-Exit is trap-like, KVM doesn't need to advance RIP and
+	 * hardware has done any necessary aliasing, offset adjustments, etc...
+	 * for the access.  I.e. the correct value has already been  written to
+	 * the vAPIC page for the correct 16-byte chunk.  KVM needs only to
+	 * retrieve the register value and emulate the access.
+	 */
+	u32 offset = exit_qualification & 0xff0;
+
 	kvm_apic_write_nodecode(vcpu, offset);
 	return 1;
 }
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 02/11] KVM: VMX: Handle APIC-write offset wrangling in VMX code

[Chao Gao]

>--- a/arch/x86/kvm/vmx/vmx.c
>+++ b/arch/x86/kvm/vmx/vmx.c
>@@ -5302,9 +5302,16 @@ static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
> static int handle_apic_write(struct kvm_vcpu *vcpu)
> {
> 	unsigned long exit_qualification = vmx_get_exit_qual(vcpu);
>-	u32 offset = exit_qualification & 0xfff;
> 
>-	/* APIC-write VM exit is trap-like and thus no need to adjust IP */
>+	/*
>+	 * APIC-write VM-Exit is trap-like, KVM doesn't need to advance RIP and
>+	 * hardware has done any necessary aliasing, offset adjustments, etc...
>+	 * for the access.  I.e. the correct value has already been  written to
>+	 * the vAPIC page for the correct 16-byte chunk.  KVM needs only to
>+	 * retrieve the register value and emulate the access.
>+	 */
>+	u32 offset = exit_qualification & 0xff0;

Can we take this opportunity to remove offset/exit_qualification?
They are used just once.

>+
> 	kvm_apic_write_nodecode(vcpu, offset);
> 	return 1;
> }
>-- 
>2.35.0.263.gb82422642f-goog
>

Re: [PATCH 02/11] KVM: VMX: Handle APIC-write offset wrangling in VMX code

[Sean Christopherson]

On Tue, Feb 15, 2022, Chao Gao wrote:
> >--- a/arch/x86/kvm/vmx/vmx.c
> >+++ b/arch/x86/kvm/vmx/vmx.c
> >@@ -5302,9 +5302,16 @@ static int handle_apic_eoi_induced(struct kvm_vcpu *vcpu)
> > static int handle_apic_write(struct kvm_vcpu *vcpu)
> > {
> > 	unsigned long exit_qualification = vmx_get_exit_qual(vcpu);
> >-	u32 offset = exit_qualification & 0xfff;
> > 
> >-	/* APIC-write VM exit is trap-like and thus no need to adjust IP */
> >+	/*
> >+	 * APIC-write VM-Exit is trap-like, KVM doesn't need to advance RIP and
> >+	 * hardware has done any necessary aliasing, offset adjustments, etc...
> >+	 * for the access.  I.e. the correct value has already been  written to
> >+	 * the vAPIC page for the correct 16-byte chunk.  KVM needs only to
> >+	 * retrieve the register value and emulate the access.
> >+	 */
> >+	u32 offset = exit_qualification & 0xff0;
> 
> Can we take this opportunity to remove offset/exit_qualification?
> They are used just once.

Definitely should have dropped exit_qualification, not sure why I didn't.

I'd prefer to keep offset to document what is held in vmcs.EXIT_QUALIFICATION
without having to add an explicit comment.

[PATCH 03/11] KVM: x86: Use "raw" APIC register read for handling APIC-write VM-Exit

[Sean Christopherson]

Use the "raw" helper to read the vAPIC register after an APIC-write trap
VM-Exit.  Hardware is responsible for vetting the write, and the caller
is responsible for sanitizing the offset.  This is a functional change,
as it means KVM will consume whatever happens to be in the vAPIC page if
the write was dropped by hardware.  But, unless userspace deliberately
wrote garbage into the vAPIC page via KVM_SET_LAPIC, the value should be
zero since it's not writable by the guest.

This aligns common x86 with SVM's AVIC logic, i.e. paves the way for
using the nodecode path to handle APIC-write traps when AVIC is enabled.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index fbce455a9d17..2c88815657a9 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2186,9 +2186,7 @@ EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
 /* emulate APIC access in a trap manner */
 void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 {
-	u32 val = 0;
-
-	kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
+	u32 val = kvm_lapic_get_reg(vcpu->arch.apic, offset);
 
 	/* TODO: optimize to just emulate side effect w/o one more write */
 	kvm_lapic_reg_write(vcpu->arch.apic, offset, val);
-- 
2.35.0.263.gb82422642f-goog

[PATCH 04/11] KVM: SVM: Use common kvm_apic_write_nodecode() for AVIC write traps

[Sean Christopherson]

Use the common kvm_apic_write_nodecode() to handle AVIC/APIC-write traps
instead of open coding the same exact code.  This will allow making the
low level lapic helpers inaccessible outside of lapic.c code.

Opportunistically clean up the params to eliminate a bunch of svm=>vcpu
reflection.

No functional change intended.

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

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index ecc81c48c0ca..462ab073db38 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -476,10 +476,9 @@ static void avic_handle_dfr_update(struct kvm_vcpu *vcpu)
 	svm->dfr_reg = dfr;
 }
 
-static int avic_unaccel_trap_write(struct vcpu_svm *svm)
+static int avic_unaccel_trap_write(struct kvm_vcpu *vcpu)
 {
-	struct kvm_lapic *apic = svm->vcpu.arch.apic;
-	u32 offset = svm->vmcb->control.exit_info_1 &
+	u32 offset = to_svm(vcpu)->vmcb->control.exit_info_1 &
 				AVIC_UNACCEL_ACCESS_OFFSET_MASK;
 
 	switch (offset) {
@@ -488,18 +487,17 @@ static int avic_unaccel_trap_write(struct vcpu_svm *svm)
 			return 0;
 		break;
 	case APIC_LDR:
-		if (avic_handle_ldr_update(&svm->vcpu))
+		if (avic_handle_ldr_update(vcpu))
 			return 0;
 		break;
 	case APIC_DFR:
-		avic_handle_dfr_update(&svm->vcpu);
+		avic_handle_dfr_update(vcpu);
 		break;
 	default:
 		break;
 	}
 
-	kvm_lapic_reg_write(apic, offset, kvm_lapic_get_reg(apic, offset));
-
+	kvm_apic_write_nodecode(vcpu, offset);
 	return 1;
 }
 
@@ -549,7 +547,7 @@ int avic_unaccelerated_access_interception(struct kvm_vcpu *vcpu)
 	if (trap) {
 		/* Handling Trap */
 		WARN_ONCE(!write, "svm: Handling trap read.\n");
-		ret = avic_unaccel_trap_write(svm);
+		ret = avic_unaccel_trap_write(vcpu);
 	} else {
 		/* Handling Fault */
 		ret = kvm_emulate_instruction(vcpu, 0);
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 04/11] KVM: SVM: Use common kvm_apic_write_nodecode() for AVIC write traps

[kernel test robot]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on 17179d0068b20413de2355f84c75a93740257e20]

url:    https://github.com/0day-ci/linux/commits/Sean-Christopherson/KVM-x86-Prep-work-for-VMX-IPI-virtualization/20220205-054418
base:   17179d0068b20413de2355f84c75a93740257e20
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20220205/202202050720.YPm113nN-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/528172fca9c0e8fac06680430bf69a55e4559974
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sean-Christopherson/KVM-x86-Prep-work-for-VMX-IPI-virtualization/20220205-054418
        git checkout 528172fca9c0e8fac06680430bf69a55e4559974
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash arch/x86/kvm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/x86/kvm/svm/avic.c: In function 'avic_unaccel_trap_write':
>> arch/x86/kvm/svm/avic.c:486:35: error: 'svm' undeclared (first use in this function); did you mean 'sem'?
     486 |   if (avic_handle_apic_id_update(&svm->vcpu))
         |                                   ^~~
         |                                   sem
   arch/x86/kvm/svm/avic.c:486:35: note: each undeclared identifier is reported only once for each function it appears in


vim +486 arch/x86/kvm/svm/avic.c

ef0f64960d012cb Joerg Roedel        2020-03-31  478  
528172fca9c0e8f Sean Christopherson 2022-02-04  479  static int avic_unaccel_trap_write(struct kvm_vcpu *vcpu)
ef0f64960d012cb Joerg Roedel        2020-03-31  480  {
528172fca9c0e8f Sean Christopherson 2022-02-04  481  	u32 offset = to_svm(vcpu)->vmcb->control.exit_info_1 &
ef0f64960d012cb Joerg Roedel        2020-03-31  482  				AVIC_UNACCEL_ACCESS_OFFSET_MASK;
ef0f64960d012cb Joerg Roedel        2020-03-31  483  
ef0f64960d012cb Joerg Roedel        2020-03-31  484  	switch (offset) {
ef0f64960d012cb Joerg Roedel        2020-03-31  485  	case APIC_ID:
ef0f64960d012cb Joerg Roedel        2020-03-31 @486  		if (avic_handle_apic_id_update(&svm->vcpu))
ef0f64960d012cb Joerg Roedel        2020-03-31  487  			return 0;
ef0f64960d012cb Joerg Roedel        2020-03-31  488  		break;
ef0f64960d012cb Joerg Roedel        2020-03-31  489  	case APIC_LDR:
528172fca9c0e8f Sean Christopherson 2022-02-04  490  		if (avic_handle_ldr_update(vcpu))
ef0f64960d012cb Joerg Roedel        2020-03-31  491  			return 0;
ef0f64960d012cb Joerg Roedel        2020-03-31  492  		break;
ef0f64960d012cb Joerg Roedel        2020-03-31  493  	case APIC_DFR:
528172fca9c0e8f Sean Christopherson 2022-02-04  494  		avic_handle_dfr_update(vcpu);
ef0f64960d012cb Joerg Roedel        2020-03-31  495  		break;
ef0f64960d012cb Joerg Roedel        2020-03-31  496  	default:
ef0f64960d012cb Joerg Roedel        2020-03-31  497  		break;
ef0f64960d012cb Joerg Roedel        2020-03-31  498  	}
ef0f64960d012cb Joerg Roedel        2020-03-31  499  
528172fca9c0e8f Sean Christopherson 2022-02-04  500  	kvm_apic_write_nodecode(vcpu, offset);
ef0f64960d012cb Joerg Roedel        2020-03-31  501  	return 1;
ef0f64960d012cb Joerg Roedel        2020-03-31  502  }
ef0f64960d012cb Joerg Roedel        2020-03-31  503  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 04/11] KVM: SVM: Use common kvm_apic_write_nodecode() for AVIC write traps

[Sean Christopherson]

On Sat, Feb 05, 2022, kernel test robot wrote:
> All errors (new ones prefixed by >>):
> 
>    arch/x86/kvm/svm/avic.c: In function 'avic_unaccel_trap_write':
> >> arch/x86/kvm/svm/avic.c:486:35: error: 'svm' undeclared (first use in this function); did you mean 'sem'?
>      486 |   if (avic_handle_apic_id_update(&svm->vcpu))
>          |                                   ^~~
>          |                                   sem
>    arch/x86/kvm/svm/avic.c:486:35: note: each undeclared identifier is reported only once for each function it appears in
> 
> 
> vim +486 arch/x86/kvm/svm/avic.c
> 
> ef0f64960d012cb Joerg Roedel        2020-03-31  478  
> 528172fca9c0e8f Sean Christopherson 2022-02-04  479  static int avic_unaccel_trap_write(struct kvm_vcpu *vcpu)
> ef0f64960d012cb Joerg Roedel        2020-03-31  480  {
> 528172fca9c0e8f Sean Christopherson 2022-02-04  481  	u32 offset = to_svm(vcpu)->vmcb->control.exit_info_1 &
> ef0f64960d012cb Joerg Roedel        2020-03-31  482  				AVIC_UNACCEL_ACCESS_OFFSET_MASK;
> ef0f64960d012cb Joerg Roedel        2020-03-31  483  
> ef0f64960d012cb Joerg Roedel        2020-03-31  484  	switch (offset) {
> ef0f64960d012cb Joerg Roedel        2020-03-31  485  	case APIC_ID:
> ef0f64960d012cb Joerg Roedel        2020-03-31 @486  		if (avic_handle_apic_id_update(&svm->vcpu))
> ef0f64960d012cb Joerg Roedel        2020-03-31  487  			return 0;

Doh, I did all my testing with avic_handle_apic_id_update() completely removed
(because it's broken), but obviously forgot to rebuild without that patch when
posting.

[PATCH 05/11] KVM: SVM: Don't rewrite guest ICR on AVIC IPI virtualization failure

[Sean Christopherson]

Don't bother rewriting the ICR value into the vAPIC page on an AVIC IPI
virtualization failure, the access is a trap, i.e. the value has already
been written to the vAPIC page.  The one caveat is if hardware left the
BUSY flag set (which appears to happen somewhat arbitrarily), in which
case go through the "nodecode" APIC-write path in order to clear the BUSY
flag.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c    |  1 +
 arch/x86/kvm/svm/avic.c | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 2c88815657a9..6e1f9e83eb68 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1298,6 +1298,7 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 
 	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL);
 }
+EXPORT_SYMBOL_GPL(kvm_apic_send_ipi);
 
 static u32 apic_get_tmcct(struct kvm_lapic *apic)
 {
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 462ab073db38..82d56f8055de 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -323,18 +323,18 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
 	switch (id) {
 	case AVIC_IPI_FAILURE_INVALID_INT_TYPE:
 		/*
-		 * AVIC hardware handles the generation of
-		 * IPIs when the specified Message Type is Fixed
-		 * (also known as fixed delivery mode) and
-		 * the Trigger Mode is edge-triggered. The hardware
-		 * also supports self and broadcast delivery modes
-		 * specified via the Destination Shorthand(DSH)
-		 * field of the ICRL. Logical and physical APIC ID
-		 * formats are supported. All other IPI types cause
-		 * a #VMEXIT, which needs to emulated.
+		 * Emulate IPIs that are not handled by AVIC hardware, which
+		 * only virtualizes Fixed, Edge-Triggered INTRs.  The exit is
+		 * a trap, e.g. ICR holds the correct value and RIP has been
+		 * advanced, KVM is responsible only for emulating the IPI.
+		 * Sadly, hardware may sometimes leave the BUSY flag set, in
+		 * which case KVM needs to emulate the ICR write as well in
+		 * order to clear the BUSY flag.
 		 */
-		kvm_lapic_reg_write(apic, APIC_ICR2, icrh);
-		kvm_lapic_reg_write(apic, APIC_ICR, icrl);
+		if (icrl & APIC_ICR_BUSY)
+			kvm_apic_write_nodecode(vcpu, APIC_ICR);
+		else
+			kvm_apic_send_ipi(apic, icrl, icrh);
 		break;
 	case AVIC_IPI_FAILURE_TARGET_NOT_RUNNING:
 		/*
-- 
2.35.0.263.gb82422642f-goog

[PATCH 06/11] KVM: x86: WARN if KVM emulates an IPI without clearing the BUSY flag

[Sean Christopherson]

WARN if KVM emulates an IPI without clearing the BUSY flag, failure to do
so could hang the guest if it waits for the IPI be sent.

Opportunistically use APIC_ICR_BUSY macro instead of open coding the
magic number, and add a comment to clarify why kvm_recalculate_apic_map()
is unconditionally invoked (it's really, really confusing for IPIs due to
the existence of fast paths that don't trigger a potential recalc).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 10 +++++++++-
 arch/x86/kvm/x86.c   |  9 ++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6e1f9e83eb68..4f57b6f5ebd4 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1282,6 +1282,9 @@ void kvm_apic_send_ipi(struct kvm_lapic *apic, u32 icr_low, u32 icr_high)
 {
 	struct kvm_lapic_irq irq;
 
+	/* KVM has no delay and should always clear the BUSY/PENDING flag. */
+	WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
+
 	irq.vector = icr_low & APIC_VECTOR_MASK;
 	irq.delivery_mode = icr_low & APIC_MODE_MASK;
 	irq.dest_mode = icr_low & APIC_DEST_MASK;
@@ -2060,7 +2063,7 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 	}
 	case APIC_ICR:
 		/* No delay here, so we always clear the pending bit */
-		val &= ~(1 << 12);
+		val &= ~APIC_ICR_BUSY;
 		kvm_apic_send_ipi(apic, val, kvm_lapic_get_reg(apic, APIC_ICR2));
 		kvm_lapic_set_reg(apic, APIC_ICR, val);
 		break;
@@ -2139,6 +2142,11 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 	}
 
+	/*
+	 * Recalculate APIC maps if necessary, e.g. if the software enable bit
+	 * was toggled, the APIC ID changed, etc...   The maps are marked dirty
+	 * on relevant changes, i.e. this is a nop for most writes.
+	 */
 	kvm_recalculate_apic_map(apic->vcpu->kvm);
 
 	return ret;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c25a6ef0ff06..9024e33c2add 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2012,11 +2012,10 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
 		return 1;
 
 	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
-		((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
-		((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
-		((u32)(data >> 32) != X2APIC_BROADCAST)) {
-
-		data &= ~(1 << 12);
+	    ((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
+	    ((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
+	    ((u32)(data >> 32) != X2APIC_BROADCAST)) {
+		data &= ~APIC_ICR_BUSY;
 		kvm_apic_send_ipi(vcpu->arch.apic, (u32)data, (u32)(data >> 32));
 		kvm_lapic_set_reg(vcpu->arch.apic, APIC_ICR2, (u32)(data >> 32));
 		kvm_lapic_set_reg(vcpu->arch.apic, APIC_ICR, (u32)data);
-- 
2.35.0.263.gb82422642f-goog

[PATCH 07/11] KVM: x86: Make kvm_lapic_reg_{read,write}() static

[Sean Christopherson]

Make the low level read/write lapic helpers static, any accesses to the
local APIC from vendor code or non-APIC code should be routed through
proper helpers.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 8 +++-----
 arch/x86/kvm/lapic.h | 3 ---
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 4f57b6f5ebd4..deac73ce2de5 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1385,8 +1385,8 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
 #define APIC_REGS_MASK(first, count) \
 	(APIC_REG_MASK(first) * ((1ull << (count)) - 1))
 
-int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
-		void *data)
+static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
+			      void *data)
 {
 	unsigned char alignment = offset & 0xf;
 	u32 result;
@@ -1442,7 +1442,6 @@ int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(kvm_lapic_reg_read);
 
 static int apic_mmio_in_range(struct kvm_lapic *apic, gpa_t addr)
 {
@@ -2003,7 +2002,7 @@ static void apic_manage_nmi_watchdog(struct kvm_lapic *apic, u32 lvt0_val)
 	}
 }
 
-int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
+static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 {
 	int ret = 0;
 
@@ -2151,7 +2150,6 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(kvm_lapic_reg_write);
 
 static int apic_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
 			    gpa_t address, int len, const void *data)
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index 2b44e533fc8d..ab76896a8c3f 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -85,9 +85,6 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value);
 u64 kvm_lapic_get_base(struct kvm_vcpu *vcpu);
 void kvm_recalculate_apic_map(struct kvm *kvm);
 void kvm_apic_set_version(struct kvm_vcpu *vcpu);
-int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val);
-int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
-		       void *data);
 bool kvm_apic_match_dest(struct kvm_vcpu *vcpu, struct kvm_lapic *source,
 			   int shorthand, unsigned int dest, int dest_mode);
 int kvm_apic_compare_prio(struct kvm_vcpu *vcpu1, struct kvm_vcpu *vcpu2);
-- 
2.35.0.263.gb82422642f-goog

[PATCH 08/11] KVM: x86: Add helpers to handle 64-bit APIC MSR read/writes

[Sean Christopherson]

Add helpers to handle 64-bit APIC read/writes via MSRs to deduplicate the
x2APIC and Hyper-V code needed to service reads/writes to ICR.  Future
support for IPI virtualization will add yet another path where KVM must
handle 64-bit APIC MSR reads/write (to ICR).

Opportunistically fix the comment in the write path; 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 | 59 ++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index deac73ce2de5..f72f3043134e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2782,6 +2782,30 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 	return 0;
 }
 
+static int kvm_lapic_msr_read(struct kvm_lapic *apic, u32 reg, u64 *data)
+{
+	u32 low, high = 0;
+
+	if (kvm_lapic_reg_read(apic, reg, 4, &low))
+		return 1;
+
+	if (reg == APIC_ICR &&
+	    WARN_ON_ONCE(kvm_lapic_reg_read(apic, APIC_ICR2, 4, &high)))
+		return 1;
+
+	*data = (((u64)high) << 32) | low;
+
+	return 0;
+}
+
+static int kvm_lapic_msr_write(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);
+}
+
 int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
@@ -2793,16 +2817,13 @@ 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_msr_write(apic, reg, data);
 }
 
 int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
-	u32 reg = (msr - APIC_BASE_MSR) << 4, low, high = 0;
+	u32 reg = (msr - APIC_BASE_MSR) << 4;
 
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
 		return 1;
@@ -2810,45 +2831,23 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 	if (reg == APIC_DFR || reg == APIC_ICR2)
 		return 1;
 
-	if (kvm_lapic_reg_read(apic, reg, 4, &low))
-		return 1;
-	if (reg == APIC_ICR)
-		kvm_lapic_reg_read(apic, APIC_ICR2, 4, &high);
-
-	*data = (((u64)high) << 32) | low;
-
-	return 0;
+	return kvm_lapic_msr_read(apic, reg, data);
 }
 
 int kvm_hv_vapic_msr_write(struct kvm_vcpu *vcpu, u32 reg, u64 data)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
-
 	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_msr_write(vcpu->arch.apic, reg, data);
 }
 
 int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
 {
-	struct kvm_lapic *apic = vcpu->arch.apic;
-	u32 low, high = 0;
-
 	if (!lapic_in_kernel(vcpu))
 		return 1;
 
-	if (kvm_lapic_reg_read(apic, reg, 4, &low))
-		return 1;
-	if (reg == APIC_ICR)
-		kvm_lapic_reg_read(apic, APIC_ICR2, 4, &high);
-
-	*data = (((u64)high) << 32) | low;
-
-	return 0;
+	return kvm_lapic_msr_read(vcpu->arch.apic, reg, data);
 }
 
 int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len)
-- 
2.35.0.263.gb82422642f-goog

[PATCH 09/11] KVM: x86: Treat x2APIC's ICR as a 64-bit register, not two 32-bit regs

[Sean Christopherson]

Emulate the x2APIC ICR as a single 64-bit register, as opposed to forking
it across ICR and ICR2 as two 32-bit registers.  This mirrors hardware
behavior for Intel's upcoming IPI virtualization support, which does not
split the access.

Previous versions of Intel's SDM and AMD's APM don't explicitly state
exactly how ICR is reflected in the vAPIC page for x2APIC, KVM just
happened to speculate incorrectly.

Handling the upcoming behavior is necessary in order to maintain
backwards compatibility with KVM_{G,S}ET_LAPIC, e.g. failure to shuffle
the 64-bit ICR to ICR+ICR2 and vice versa would break live migration if
IPI virtualization support isn't symmetrical across the source and dest.

Cc: Zeng Guang <guang.zeng@intel.com>
Cc: Chao Gao <chao.gao@intel.com>
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 114 +++++++++++++++++++++++++++++++++----------
 arch/x86/kvm/lapic.h |   8 ++-
 arch/x86/kvm/trace.h |   6 +--
 arch/x86/kvm/x86.c   |  10 +---
 4 files changed, 99 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f72f3043134e..dd185367a62c 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -68,6 +68,29 @@ static bool lapic_timer_advance_dynamic __read_mostly;
 /* step-by-step approximation to mitigate fluctuation */
 #define LAPIC_TIMER_ADVANCE_ADJUST_STEP 8
 
+static __always_inline u64 __kvm_lapic_get_reg64(char *regs, int reg)
+{
+	BUILD_BUG_ON(reg != APIC_ICR);
+	return *((u64 *) (regs + reg));
+}
+
+static __always_inline u64 kvm_lapic_get_reg64(struct kvm_lapic *apic, int reg)
+{
+	return __kvm_lapic_get_reg64(apic->regs, reg);
+}
+
+static __always_inline void __kvm_lapic_set_reg64(char *regs, int reg, u64 val)
+{
+	BUILD_BUG_ON(reg != APIC_ICR);
+	*((u64 *) (regs + reg)) = val;
+}
+
+static __always_inline void kvm_lapic_set_reg64(struct kvm_lapic *apic,
+						int reg, u64 val)
+{
+	__kvm_lapic_set_reg64(apic->regs, reg, val);
+}
+
 static inline int apic_test_vector(int vec, void *bitmap)
 {
 	return test_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
@@ -1404,7 +1427,6 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 		APIC_REGS_MASK(APIC_IRR, APIC_ISR_NR) |
 		APIC_REG_MASK(APIC_ESR) |
 		APIC_REG_MASK(APIC_ICR) |
-		APIC_REG_MASK(APIC_ICR2) |
 		APIC_REG_MASK(APIC_LVTT) |
 		APIC_REG_MASK(APIC_LVTTHMR) |
 		APIC_REG_MASK(APIC_LVTPC) |
@@ -1415,9 +1437,16 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 		APIC_REG_MASK(APIC_TMCCT) |
 		APIC_REG_MASK(APIC_TDCR);
 
-	/* ARBPRI is not valid on x2APIC */
+	/*
+	 * ARBPRI and ICR2 are not valid in x2APIC mode.  WARN if KVM reads ICR
+	 * in x2APIC mode as it's an 8-byte register in x2APIC and needs to be
+	 * manually handled by the caller.
+	 */
 	if (!apic_x2apic_mode(apic))
-		valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI);
+		valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI) |
+				  APIC_REG_MASK(APIC_ICR2);
+	else
+		WARN_ON_ONCE(offset == APIC_ICR);
 
 	if (alignment + len > 4)
 		return 1;
@@ -2061,16 +2090,18 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 	}
 	case APIC_ICR:
+		WARN_ON_ONCE(apic_x2apic_mode(apic));
+
 		/* No delay here, so we always clear the pending bit */
 		val &= ~APIC_ICR_BUSY;
 		kvm_apic_send_ipi(apic, val, kvm_lapic_get_reg(apic, APIC_ICR2));
 		kvm_lapic_set_reg(apic, APIC_ICR, val);
 		break;
-
 	case APIC_ICR2:
-		if (!apic_x2apic_mode(apic))
-			val &= 0xff000000;
-		kvm_lapic_set_reg(apic, APIC_ICR2, val);
+		if (apic_x2apic_mode(apic))
+			ret = 1;
+		else
+			kvm_lapic_set_reg(apic, APIC_ICR2, val & 0xff000000);
 		break;
 
 	case APIC_LVT0:
@@ -2130,10 +2161,9 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 
 	case APIC_SELF_IPI:
-		if (apic_x2apic_mode(apic)) {
-			kvm_lapic_reg_write(apic, APIC_ICR,
-					    APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
-		} else
+		if (apic_x2apic_mode(apic))
+			kvm_x2apic_icr_write(apic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
+		else
 			ret = 1;
 		break;
 	default:
@@ -2359,8 +2389,12 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
 	if (!apic_x2apic_mode(apic))
 		kvm_apic_set_ldr(apic, 0);
 	kvm_lapic_set_reg(apic, APIC_ESR, 0);
-	kvm_lapic_set_reg(apic, APIC_ICR, 0);
-	kvm_lapic_set_reg(apic, APIC_ICR2, 0);
+	if (!apic_x2apic_mode(apic)) {
+		kvm_lapic_set_reg(apic, APIC_ICR, 0);
+		kvm_lapic_set_reg(apic, APIC_ICR2, 0);
+	} else {
+		kvm_lapic_set_reg64(apic, APIC_ICR, 0);
+	}
 	kvm_lapic_set_reg(apic, APIC_TDCR, 0);
 	kvm_lapic_set_reg(apic, APIC_TMICT, 0);
 	for (i = 0; i < 8; i++) {
@@ -2577,6 +2611,7 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
 	if (apic_x2apic_mode(vcpu->arch.apic)) {
 		u32 *id = (u32 *)(s->regs + APIC_ID);
 		u32 *ldr = (u32 *)(s->regs + APIC_LDR);
+		u64 icr;
 
 		if (vcpu->kvm->arch.x2apic_format) {
 			if (*id != vcpu->vcpu_id)
@@ -2588,9 +2623,21 @@ static int kvm_apic_state_fixup(struct kvm_vcpu *vcpu,
 				*id <<= 24;
 		}
 
-		/* In x2APIC mode, the LDR is fixed and based on the id */
-		if (set)
+		/*
+		 * 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 (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);
+		}
 	}
 
 	return 0;
@@ -2782,27 +2829,43 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
 	return 0;
 }
 
+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);
+	trace_kvm_apic_write(APIC_ICR, data);
+	return 0;
+}
+
 static int kvm_lapic_msr_read(struct kvm_lapic *apic, u32 reg, u64 *data)
 {
-	u32 low, high = 0;
+	u32 low;
+
+	if (reg == APIC_ICR) {
+		*data = kvm_lapic_get_reg64(apic, APIC_ICR);
+		return 0;
+	}
 
 	if (kvm_lapic_reg_read(apic, reg, 4, &low))
 		return 1;
 
-	if (reg == APIC_ICR &&
-	    WARN_ON_ONCE(kvm_lapic_reg_read(apic, APIC_ICR2, 4, &high)))
-		return 1;
-
-	*data = (((u64)high) << 32) | low;
+	*data = low;
 
 	return 0;
 }
 
 static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
 {
-	/* For 64-bit ICR writes, set ICR2 (dest) before ICR (command). */
+	/*
+	 * ICR is a 64-bit register in x2APIC mode (and Hyper'v PV vAPIC) and
+	 * can be written as such, all other registers remain accessible only
+	 * through 32-bit reads/writes.
+	 */
 	if (reg == APIC_ICR)
-		kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32));
+		return kvm_x2apic_icr_write(apic, data);
+
 	return kvm_lapic_reg_write(apic, reg, (u32)data);
 }
 
@@ -2814,9 +2877,6 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
 		return 1;
 
-	if (reg == APIC_ICR2)
-		return 1;
-
 	return kvm_lapic_msr_write(apic, reg, data);
 }
 
@@ -2828,7 +2888,7 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
 		return 1;
 
-	if (reg == APIC_DFR || reg == APIC_ICR2)
+	if (reg == APIC_DFR)
 		return 1;
 
 	return kvm_lapic_msr_read(apic, reg, data);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index ab76896a8c3f..e39e7ec5c2b4 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -118,6 +118,7 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr);
 void kvm_lapic_sync_from_vapic(struct kvm_vcpu *vcpu);
 void kvm_lapic_sync_to_vapic(struct kvm_vcpu *vcpu);
 
+int kvm_x2apic_icr_write(struct kvm_lapic *apic, u64 data);
 int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data);
 int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
 
@@ -150,9 +151,14 @@ static inline void kvm_lapic_set_irr(int vec, struct kvm_lapic *apic)
 	apic->irr_pending = true;
 }
 
+static inline u32 __kvm_lapic_get_reg(char *regs, int reg_off)
+{
+	return *((u32 *) (regs + reg_off));
+}
+
 static inline u32 kvm_lapic_get_reg(struct kvm_lapic *apic, int reg_off)
 {
-	return *((u32 *) (apic->regs + reg_off));
+	return __kvm_lapic_get_reg(apic->regs, reg_off);
 }
 
 static inline void __kvm_lapic_set_reg(char *regs, int reg_off, u32 val)
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 92e6f6702f00..340394a8ce7a 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -251,13 +251,13 @@ TRACE_EVENT(kvm_cpuid,
  * Tracepoint for apic access.
  */
 TRACE_EVENT(kvm_apic,
-	TP_PROTO(unsigned int rw, unsigned int reg, unsigned int val),
+	TP_PROTO(unsigned int rw, unsigned int reg, u64 val),
 	TP_ARGS(rw, reg, val),
 
 	TP_STRUCT__entry(
 		__field(	unsigned int,	rw		)
 		__field(	unsigned int,	reg		)
-		__field(	unsigned int,	val		)
+		__field(	u64,		val		)
 	),
 
 	TP_fast_assign(
@@ -266,7 +266,7 @@ TRACE_EVENT(kvm_apic,
 		__entry->val		= val;
 	),
 
-	TP_printk("apic_%s %s = 0x%x",
+	TP_printk("apic_%s %s = 0x%llx",
 		  __entry->rw ? "write" : "read",
 		  __print_symbolic(__entry->reg, kvm_trace_symbol_apic),
 		  __entry->val)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9024e33c2add..eaad2f485b64 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2014,14 +2014,8 @@ static int handle_fastpath_set_x2apic_icr_irqoff(struct kvm_vcpu *vcpu, u64 data
 	if (((data & APIC_SHORT_MASK) == APIC_DEST_NOSHORT) &&
 	    ((data & APIC_DEST_MASK) == APIC_DEST_PHYSICAL) &&
 	    ((data & APIC_MODE_MASK) == APIC_DM_FIXED) &&
-	    ((u32)(data >> 32) != X2APIC_BROADCAST)) {
-		data &= ~APIC_ICR_BUSY;
-		kvm_apic_send_ipi(vcpu->arch.apic, (u32)data, (u32)(data >> 32));
-		kvm_lapic_set_reg(vcpu->arch.apic, APIC_ICR2, (u32)(data >> 32));
-		kvm_lapic_set_reg(vcpu->arch.apic, APIC_ICR, (u32)data);
-		trace_kvm_apic_write(APIC_ICR, (u32)data);
-		return 0;
-	}
+	    ((u32)(data >> 32) != X2APIC_BROADCAST))
+		return kvm_x2apic_icr_write(vcpu->arch.apic, data);
 
 	return 1;
 }
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 09/11] KVM: x86: Treat x2APIC's ICR as a 64-bit register, not two 32-bit regs

[Chao Gao]

> 	case APIC_SELF_IPI:
>-		if (apic_x2apic_mode(apic)) {
>-			kvm_lapic_reg_write(apic, APIC_ICR,
>-					    APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
>-		} else
>+		if (apic_x2apic_mode(apic))
>+			kvm_x2apic_icr_write(apic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
>+		else

The original code looks incorrect. Emulating writes to SELF_IPI by writes to
ICR has an unwanted side-effect: the value of ICR in vAPIC page gets changed.

It is better to use kvm_apic_send_ipi() directly.

Re: [PATCH 09/11] KVM: x86: Treat x2APIC's ICR as a 64-bit register, not two 32-bit regs

[Sean Christopherson]

On Tue, Feb 15, 2022, Chao Gao wrote:
> > 	case APIC_SELF_IPI:
> >-		if (apic_x2apic_mode(apic)) {
> >-			kvm_lapic_reg_write(apic, APIC_ICR,
> >-					    APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
> >-		} else
> >+		if (apic_x2apic_mode(apic))
> >+			kvm_x2apic_icr_write(apic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
> >+		else
> 
> The original code looks incorrect. Emulating writes to SELF_IPI by writes to
> ICR has an unwanted side-effect: the value of ICR in vAPIC page gets changed.
> 
> It is better to use kvm_apic_send_ipi() directly.

Agreed, the SDM lists SELF_IPI as write-only, with no associated MMIO offset, so
it should have no visible side effect in the vAPIC.  I'll add a patch to fix this.

Thanks!

[PATCH 10/11] KVM: x86: Make kvm_lapic_set_reg() a "private" xAPIC helper

[Sean Christopherson]

Hide the lapic's "raw" write helper inside lapic.c to force non-APIC code
to go through proper helpers when modification the vAPIC state.  Keep the
read helper visible to outsiders for now, refactoring KVM to hide it too
is possible, it will just take more work to do so.

No functional change intended.

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

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index dd185367a62c..d60eb6251bed 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -68,6 +68,16 @@ static bool lapic_timer_advance_dynamic __read_mostly;
 /* step-by-step approximation to mitigate fluctuation */
 #define LAPIC_TIMER_ADVANCE_ADJUST_STEP 8
 
+static inline void __kvm_lapic_set_reg(char *regs, int reg_off, u32 val)
+{
+	*((u32 *) (regs + reg_off)) = val;
+}
+
+static inline void kvm_lapic_set_reg(struct kvm_lapic *apic, int reg_off, u32 val)
+{
+	__kvm_lapic_set_reg(apic->regs, reg_off, val);
+}
+
 static __always_inline u64 __kvm_lapic_get_reg64(char *regs, int reg)
 {
 	BUILD_BUG_ON(reg != APIC_ICR);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index e39e7ec5c2b4..4e4f8a22754f 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -161,16 +161,6 @@ static inline u32 kvm_lapic_get_reg(struct kvm_lapic *apic, int reg_off)
 	return __kvm_lapic_get_reg(apic->regs, reg_off);
 }
 
-static inline void __kvm_lapic_set_reg(char *regs, int reg_off, u32 val)
-{
-	*((u32 *) (regs + reg_off)) = val;
-}
-
-static inline void kvm_lapic_set_reg(struct kvm_lapic *apic, int reg_off, u32 val)
-{
-	__kvm_lapic_set_reg(apic->regs, reg_off, val);
-}
-
 DECLARE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
 
 static inline bool lapic_in_kernel(struct kvm_vcpu *vcpu)
-- 
2.35.0.263.gb82422642f-goog

[PATCH 11/11] KVM: selftests: Add test to verify KVM handles x2APIC ICR=>ICR2 dance

[Sean Christopherson]

Add a selftest to verify that KVM copies x2APIC's ICR[63:32] to/from
ICR2 when userspace accesses the vAPIC page via KVM_{G,S}ET_LAPIC.  KVM
previously split x2APIC ICR to ICR+ICR2 at the time of write (from the
guest), and so KVM must preserve that behavior for backwards
compatibility between different versions of KVM.

Opportunsitically test other invariants, e.g. that KVM clears the BUSY
flag on ICR writes, that the reserved bits in ICR2 are dropped on writes
from the guest, etc...

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/apic.h       |   1 +
 .../selftests/kvm/x86_64/xapic_state_test.c   | 150 ++++++++++++++++++
 4 files changed, 153 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/xapic_state_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index dce7de7755e6..2b0e47f420b3 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -45,6 +45,7 @@
 /x86_64/vmx_tsc_adjust_test
 /x86_64/vmx_nested_tsc_scaling_test
 /x86_64/xapic_ipi_test
+/x86_64/xapic_state_test
 /x86_64/xen_shinfo_test
 /x86_64/xen_vmcall_test
 /x86_64/xss_msr_test
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 0e4926bc9a58..4b5211afc6dc 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -76,6 +76,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
 TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test
+TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test
 TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
 TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
 TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
diff --git a/tools/testing/selftests/kvm/include/x86_64/apic.h b/tools/testing/selftests/kvm/include/x86_64/apic.h
index 0be4757f1f20..ac88557dcc9a 100644
--- a/tools/testing/selftests/kvm/include/x86_64/apic.h
+++ b/tools/testing/selftests/kvm/include/x86_64/apic.h
@@ -33,6 +33,7 @@
 #define	APIC_SPIV	0xF0
 #define		APIC_SPIV_FOCUS_DISABLED	(1 << 9)
 #define		APIC_SPIV_APIC_ENABLED		(1 << 8)
+#define APIC_IRR	0x200
 #define	APIC_ICR	0x300
 #define		APIC_DEST_SELF		0x40000
 #define		APIC_DEST_ALLINC	0x80000
diff --git a/tools/testing/selftests/kvm/x86_64/xapic_state_test.c b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
new file mode 100644
index 000000000000..0792334ba243
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/xapic_state_test.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define _GNU_SOURCE /* for program_invocation_short_name */
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "apic.h"
+#include "kvm_util.h"
+#include "processor.h"
+#include "test_util.h"
+
+struct kvm_vcpu {
+	uint32_t id;
+	bool is_x2apic;
+};
+
+static void xapic_guest_code(void)
+{
+	asm volatile("cli");
+
+	xapic_enable();
+
+	while (1) {
+		uint64_t val = (u64)xapic_read_reg(APIC_IRR) |
+			       (u64)xapic_read_reg(APIC_IRR + 0x10) << 32;
+
+		xapic_write_reg(APIC_ICR2, val >> 32);
+		xapic_write_reg(APIC_ICR, val);
+		GUEST_SYNC(val);
+	}
+}
+
+static void x2apic_guest_code(void)
+{
+	asm volatile("cli");
+
+	x2apic_enable();
+
+	do {
+		uint64_t val = x2apic_read_reg(APIC_IRR) |
+			       x2apic_read_reg(APIC_IRR + 0x10) << 32;
+
+		x2apic_write_reg(APIC_ICR, val);
+		GUEST_SYNC(val);
+	} while (1);
+}
+
+static void ____test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu, uint64_t val)
+{
+	struct kvm_lapic_state xapic;
+	struct ucall uc;
+	uint64_t icr;
+
+	/*
+	 * Tell the guest what ICR value to write.  Use the IRR to pass info,
+	 * all bits are valid and should not be modified by KVM (ignoring the
+	 * fact that vectors 0-15 are technically illegal).
+	 */
+	vcpu_ioctl(vm, vcpu->id, KVM_GET_LAPIC, &xapic);
+	*((u32 *)&xapic.regs[APIC_IRR]) = val;
+	*((u32 *)&xapic.regs[APIC_IRR + 0x10]) = val >> 32;
+	vcpu_ioctl(vm, vcpu->id, KVM_SET_LAPIC, &xapic);
+
+	vcpu_run(vm, vcpu->id);
+	ASSERT_EQ(get_ucall(vm, vcpu->id, &uc), UCALL_SYNC);
+	ASSERT_EQ(uc.args[1], val);
+
+	vcpu_ioctl(vm, vcpu->id, KVM_GET_LAPIC, &xapic);
+	icr = (u64)(*((u32 *)&xapic.regs[APIC_ICR])) |
+	      (u64)(*((u32 *)&xapic.regs[APIC_ICR2])) << 32;
+	if (!vcpu->is_x2apic)
+		val &= (-1u | (0xffull << (32 + 24)));
+	ASSERT_EQ(icr, val & ~APIC_ICR_BUSY);
+}
+
+static void __test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu, uint64_t val)
+{
+	____test_icr(vm, vcpu, val | APIC_ICR_BUSY);
+	____test_icr(vm, vcpu, val & ~(u64)APIC_ICR_BUSY);
+}
+
+static void test_icr(struct kvm_vm *vm, struct kvm_vcpu *vcpu)
+{
+	uint64_t icr, i, j;
+
+	icr = APIC_DEST_SELF | APIC_INT_ASSERT | APIC_DM_FIXED;
+	for (i = 0; i <= 0xff; i++)
+		__test_icr(vm, vcpu, icr | i);
+
+	icr = APIC_INT_ASSERT | APIC_DM_FIXED;
+	for (i = 0; i <= 0xff; i++)
+		__test_icr(vm, vcpu, icr | i);
+
+	/*
+	 * Send all flavors of IPIs to non-existent vCPUs.  TODO: use number of
+	 * vCPUs, not vcpu.id + 1.  Arbitrarily use vector 0xff.
+	 */
+	icr = APIC_INT_ASSERT | 0xff;
+	for (i = vcpu->id + 1; i < 0xff; i++) {
+		for (j = 0; j < 8; j++)
+			__test_icr(vm, vcpu, i << (32 + 24) | APIC_INT_ASSERT | (j << 8));
+	}
+
+	/* And again with a shorthand destination for all types of IPIs. */
+	icr = APIC_DEST_ALLBUT | APIC_INT_ASSERT;
+	for (i = 0; i < 8; i++)
+		__test_icr(vm, vcpu, icr | (i << 8));
+
+	/* And a few garbage value, just make sure it's an IRQ (blocked). */
+	__test_icr(vm, vcpu, 0xa5a5a5a5a5a5a5a5 & ~APIC_DM_FIXED_MASK);
+	__test_icr(vm, vcpu, 0x5a5a5a5a5a5a5a5a & ~APIC_DM_FIXED_MASK);
+	__test_icr(vm, vcpu, -1ull & ~APIC_DM_FIXED_MASK);
+}
+
+int main(int argc, char *argv[])
+{
+	struct kvm_vcpu vcpu = {
+		.id = 0,
+		.is_x2apic = true,
+	};
+	struct kvm_cpuid2 *cpuid;
+	struct kvm_vm *vm;
+	int i;
+
+	vm = vm_create_default(vcpu.id, 0, x2apic_guest_code);
+	test_icr(vm, &vcpu);
+	kvm_vm_free(vm);
+
+	/*
+	 * Use a second VM for the xAPIC test so that x2APIC can be hidden from
+	 * the guest in order to test AVIC.  KVM disallows changing CPUID after
+	 * KVM_RUN and AVIC is disabled if _any_ vCPU is allowed to use x2APIC.
+	 */
+	vm = vm_create_default(vcpu.id, 0, xapic_guest_code);
+	vcpu.is_x2apic = false;
+
+	cpuid = vcpu_get_cpuid(vm, vcpu.id);
+	for (i = 0; i < cpuid->nent; i++) {
+		if (cpuid->entries[i].function == 1)
+			break;
+	}
+	cpuid->entries[i].ecx &= ~BIT(21);
+	vcpu_set_cpuid(vm, vcpu.id, cpuid);
+
+	virt_pg_map(vm, APIC_DEFAULT_GPA, APIC_DEFAULT_GPA);
+	test_icr(vm, &vcpu);
+	kvm_vm_free(vm);
+}
-- 
2.35.0.263.gb82422642f-goog

Re: [PATCH 00/11] KVM: x86: Prep work for VMX IPI virtualization

[Paolo Bonzini]

On 2/4/22 22:41, Sean Christopherson wrote:
> Prepare for VMX's IPI virtualization, in which hardware treats ICR as a
> single 64-bit register in x2APIC mode.  The SDM wasn't clear on how ICR
> should be modeled, KVM just took the easier path and guessed wrong.
> 
> Hardware's implementation of ICR as a 64-bit register requires explicit
> handling to maintain backwards compatibility in KVM_{G,S}ET_REG, as
> migrating a VM between hosts with different IPI virtualization support
> would lead to ICR "corruption" for writes that aren't intercepted by
> KVM (hardware doesn't fill ICR2 in vAPIC page).
> 
> This series includes AVIC cleanups for things I encountered along the way.
> AVIC still has multiple issues, this only fixes the easy bugs.
> 
> Sean Christopherson (11):
>    Revert "svm: Add warning message for AVIC IPI invalid target"
>    KVM: VMX: Handle APIC-write offset wrangling in VMX code
>    KVM: x86: Use "raw" APIC register read for handling APIC-write VM-Exit
>    KVM: SVM: Use common kvm_apic_write_nodecode() for AVIC write traps
>    KVM: SVM: Don't rewrite guest ICR on AVIC IPI virtualization failure
>    KVM: x86: WARN if KVM emulates an IPI without clearing the BUSY flag
>    KVM: x86: Make kvm_lapic_reg_{read,write}() static
>    KVM: x86: Add helpers to handle 64-bit APIC MSR read/writes
>    KVM: x86: Treat x2APIC's ICR as a 64-bit register, not two 32-bit regs
>    KVM: x86: Make kvm_lapic_set_reg() a "private" xAPIC helper
>    KVM: selftests: Add test to verify KVM handles x2APIC ICR=>ICR2 dance
> 
>   arch/x86/kvm/lapic.c                          | 193 ++++++++++++------
>   arch/x86/kvm/lapic.h                          |  21 +-
>   arch/x86/kvm/svm/avic.c                       |  38 ++--
>   arch/x86/kvm/trace.h                          |   6 +-
>   arch/x86/kvm/vmx/vmx.c                        |  11 +-
>   arch/x86/kvm/x86.c                            |  15 +-
>   tools/testing/selftests/kvm/.gitignore        |   1 +
>   tools/testing/selftests/kvm/Makefile          |   1 +
>   .../selftests/kvm/include/x86_64/apic.h       |   1 +
>   .../selftests/kvm/x86_64/xapic_state_test.c   | 150 ++++++++++++++
>   10 files changed, 325 insertions(+), 112 deletions(-)
>   create mode 100644 tools/testing/selftests/kvm/x86_64/xapic_state_test.c
> 
> 
> base-commit: 17179d0068b20413de2355f84c75a93740257e20

Queued, with patch 4 adjusted.  Thanks,

Paolo