On Fri, Sep 08, 2023 at 06:12:12PM +0800, Chao Gao wrote: > On Fri, Sep 08, 2023 at 12:11:15PM +0800, Tao Su wrote: > >When IPI virtualization is enabled, a WARN is triggered if bit12 of ICR > >MSR is set after APIC-write VM-exit. The reason is kvm_apic_send_ipi() > >thinks the APIC_ICR_BUSY bit should be cleared because KVM has no delay, > >but kvm_apic_write_nodecode() doesn't clear the APIC_ICR_BUSY bit. > > > >Bit12 of ICR is different from other reserved bits(31:20, 17:16 and 13). > >When bit12 is set, it will cause APIC-wirte VM-exit but not #GP. For > > s/wirte/write > > >reading bit12 back as '0' which is a safer approach, clearing bit12 in > >x2APIC mode is needed. > > how about quoting what Sean said: > (w/ a slight change to the last sentence) > > Under the x2APIC section, regarding ICR, the SDM says: > > It remains readable only to aid in debugging; however, software should not > assume the value returned by reading the ICR is the last written value. > > I.e. KVM basically has free reign to do whatever it wants, so long as it doesn't > confuse userspace or break KVM's ABI. > > Clear bit12 so that it reads back as '0'. This approach is safer than "do > nothing" and is consistent with the case where IPI virtualization is > disabled or not supported, i.e., > > handle_fastpath_set_x2apic_icr_irqoff() -> kvm_x2apic_icr_write() > > > > >Although bit12 of ICR is no longer APIC_ICR_BUSY in x2APIC, keeping it > >is far easier to understand what's going on, especially given that it > >may be repurposed for something new. > > Probably you can remove this paragraph. it is not clear w/o the context > that there was an attempt to rename APIC_ICR_BUSY for x2apic while fixing > the issue. Yes, agree with all the above, this is more correct and clear description. Thanks, Tao > > > > >Link: https://lore.kernel.org/all/ZPj6iF0Q7iynn62p@xxxxxxxxxx/ > >Fixes: 5413bcba7ed5 ("KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode") > >Signed-off-by: Tao Su <tao1.su@xxxxxxxxxxxxxxx> > >Tested-by: Yi Lai <yi1.lai@xxxxxxxxx> > > Apart from above nits on the changelog, this patch looks good to me. > > Reviewed-by: Chao Gao <chao.gao@xxxxxxxxx> > > >--- > >Changelog: > > > >v2: > > - Drop the unnecessary alias for bit12 of ICR. > > - Add back kvm_lapic_get_reg64() that was removed by mistake. > > - Modify the commit message to make it clearer. > > > >v1: https://lore.kernel.org/all/20230904013555.725413-1-tao1.su@xxxxxxxxxxxxxxx/ > >--- > > arch/x86/kvm/lapic.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > >diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > >index dcd60b39e794..664d5a78b46a 100644 > >--- a/arch/x86/kvm/lapic.c > >+++ b/arch/x86/kvm/lapic.c > >@@ -2450,13 +2450,13 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset) > > * ICR is a single 64-bit register when x2APIC is enabled. For legacy > > * xAPIC, ICR writes need to go down the common (slightly slower) path > > * to get the upper half from ICR2. > >+ * > >+ * TODO: optimize to just emulate side effect w/o one more write > > */ > > if (apic_x2apic_mode(apic) && offset == APIC_ICR) { > > val = kvm_lapic_get_reg64(apic, APIC_ICR); > >- kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32)); > >- trace_kvm_apic_write(APIC_ICR, val); > >+ kvm_x2apic_icr_write(apic, val); > > } else { > >- /* TODO: optimize to just emulate side effect w/o one more write */ > > val = kvm_lapic_get_reg(apic, offset); > > kvm_lapic_reg_write(apic, offset, (u32)val); > > } > > > >base-commit: a48fa7efaf1161c1c898931fe4c7f0070964233a > >-- > >2.34.1 > >
