Re: [PATCH v5 5/8] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit
From: Zeng Guang
Date: Mon Jan 17 2022 - 22:27:27 EST
On 1/18/2022 8:44 AM, Yuan Yao wrote:
On Sat, Jan 15, 2022 at 10:08:10AM +0800, Zeng Guang wrote:
On 1/15/2022 1:34 AM, Sean Christopherson wrote:
On Fri, Jan 14, 2022, Zeng Guang wrote:
kvm_lapic_reg_read() is limited to read up to 4 bytes. It needs extension to
support 64bit read.
Ah, right.
And another concern is here getting reg value only specific from vICR(no
other regs need take care), going through whole path on kvm_lapic_reg_read()
could be time-consuming unnecessarily. Is it proper that calling
kvm_lapic_get_reg64() to retrieve vICR value directly?
Hmm, no, I don't think that's proper. Retrieving a 64-bit value really is unique
to vICR. Yes, the code does WARN on that, but if future architectural extensions
even generate APIC-write exits on other registers, then using kvm_lapic_get_reg64()
would be wrong and this code would need to be updated again.
Split on x2apic and WARN on (offset != APIC_ICR) already limit register read
to vICR only. Actually
we just need consider to deal with 64bit data specific to vICR in APIC-write
exits. From this point of
view, previous design can be compatible on handling other registers even if
future architectural
extensions changes. :)
What about tweaking my prep patch from before to the below? That would yield:
if (apic_x2apic_mode(apic)) {
if (WARN_ON_ONCE(offset != APIC_ICR))
return 1;
kvm_lapic_msr_read(apic, offset, &val);
I think it's problematic to use kvm_lapic_msr_read() in this case. It
premises the high 32bit value
already valid at APIC_ICR2, while in handling "nodecode" x2APIC writes we
need get continuous 64bit
data from offset 300H first and prepare emulation of APIC_ICR2 write. At
this time, APIC_ICR2 is not
ready yet.
How about combine them, then you can handle the ICR write vmexit for
IPI virtualization and Sean's patch can still work with code reusing,
like below:
if (apic_x2apic_mode(apic)) {
if (WARN_ON_ONCE(offset != APIC_ICR))
kvm_lapic_msr_read(apic, offset, &val);
else
kvm_lapic_get_reg64(apic, offset, &val);
kvm_lapic_msr_write(apic, offset, val);
} else {
kvm_lapic_reg_read(apic, offset, 4, &val);
kvm_lapic_reg_write(apic, offset, val);
}
Alternatively we can merge as this if Sean think it ok to call
kvm_lapic_get_reg64() retrieving 64bit data from vICR directly.
kvm_lapic_msr_write(apic, offset, val);
} else {
kvm_lapic_reg_read(apic, offset, 4, &val);
kvm_lapic_reg_write(apic, offset, val);
}
I like that the above has "msr" in the low level x2apic helpers, and it maximizes
code reuse. Compile tested only...
From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Fri, 14 Jan 2022 09:29:34 -0800
Subject: [PATCH] KVM: x86: Add helpers to handle 64-bit APIC MSR read/writes
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@xxxxxxxxxx>
---
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 f206fc35deff..cc4531eb448f 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2787,6 +2787,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;
@@ -2798,16 +2822,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;
@@ -2815,45 +2836,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)
--