On Tue, Sep 05, 2023 at 04:03:36PM -0700, Sean Christopherson wrote:
> +Suravee
>
> On Mon, Sep 04, 2023, 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.
> >
> > Since bit12 of ICR is no longer BUSY bit but UNUSED bit in x2APIC mode,
> > and SDM has no detail about how hardware will handle the UNUSED bit12
> > set, we tested on Intel CPU (SRF/GNR) with IPI virtualization and found
> > the UNUSED bit12 was also cleared by hardware without #GP. Therefore,
> > the clearing of bit12 should be still kept being consistent with the
> > hardware behavior.
>
> I'm confused. If hardware clears the bit, then why is it set in the vAPIC page
> after a trap-like APIC-write VM-Exit? In other words, how is this not a ucode
> or hardware bug?
Sorry, I didn't describe it clearly.
On bare-metal, bit12 of ICR MSR will be cleared after setting this bit.
If bit12 is set in guest, the bit is not cleared in the vAPIC page after APIC-write
VM-Exit. So whether to clear bit12 in vAPIC page needs to be considered.
>
> Suravee, can you confirm what happens on AMD with x2AVIC? Does hardware *always*
> clear the busy bit if it's set by the guest? If so, then we could "optimize"
> avic_incomplete_ipi_interception() to skip the busy check, e.g.
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index cfc8ab773025..4bf0bb250147 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -513,7 +513,7 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
> * in which case KVM needs to emulate the ICR write as well in
> * order to clear the BUSY flag.
> */
> - if (icrl & APIC_ICR_BUSY)
> + if (!apic_x2apic_mode(apic) && (icrl & APIC_ICR_BUSY))
> kvm_apic_write_nodecode(vcpu, APIC_ICR);
> else
> kvm_apic_send_ipi(apic, icrl, icrh);
>
> > 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>
> > ---
> > arch/x86/kvm/lapic.c | 27 ++++++++++++++++++++-------
> > 1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index a983a16163b1..09a376aeb4a0 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1482,8 +1482,17 @@ 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);
> > + /*
> > + * In non-x2apic mode, KVM has no delay and should always clear the
> > + * BUSY/PENDING flag. In x2apic mode, KVM should clear the unused bit12
> > + * of ICR since hardware will also clear this bit. Although
> > + * APIC_ICR_BUSY and X2APIC_ICR_UNUSED_12 are same, they mean different
> > + * things in different modes.
> > + */
> > + if (!apic_x2apic_mode(apic))
> > + WARN_ON_ONCE(icr_low & APIC_ICR_BUSY);
> > + else
> > + WARN_ON_ONCE(icr_low & X2APIC_ICR_UNUSED_12);
>
> NAK to the new name, KVM is absolutely not going to zero an arbitrary "unused"
> bit. If Intel wants to reclaim bit 12 for something useful in the future, then
> Intel can ship CPUs that don't touch the "reserved" bit, and deal with all the
> fun of finding and updating all software that unnecessarily sets the busy bit in
> x2apic mode.
>
> If we really want to pretend that Intel has more than a snowball's chance in hell
> of doing something useful with bit 12, then the right thing to do in KVM is to
> ignore the bit entirely and let the guest keep the pieces, e.g.
Yes, agree. Currently bit12 is unused and cleared by hardware on bare-metal, clearing
bit12 in guest is for keeping the same behavior as bare-metal. But KVM should not to
zero the unused bit until SDM reclaims it for something, so ignoring the bit in KVM
should be better.
Thanks,
Tao
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 113ca9661ab2..36ec195a3339 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1473,8 +1473,13 @@ 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);
> + /*
> + * KVM has no delay and should always clear the BUSY/PENDING flag.
> + * The flag doesn't exist in x2APIC mode; both the SDM and APM state
> + * that the flag "Must Be Zero", but neither Intel nor AMD enforces
> + * that (or any other reserved bits in ICR).
> + */
> + WARN_ON_ONCE(!apic_x2apic_mode(apic) && (icr_low & APIC_ICR_BUSY));
>
> irq.vector = icr_low & APIC_VECTOR_MASK;
> irq.delivery_mode = icr_low & APIC_MODE_MASK;
> @@ -3113,8 +3118,6 @@ int kvm_lapic_set_vapic_addr(struct kvm_vcpu *vcpu, gpa_t vapic_addr)
>
> 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);
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index cfc8ab773025..4bf0bb250147 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -513,7 +513,7 @@ int avic_incomplete_ipi_interception(struct kvm_vcpu *vcpu)
> * in which case KVM needs to emulate the ICR write as well in
> * order to clear the BUSY flag.
> */
> - if (icrl & APIC_ICR_BUSY)
> + if (!apic_x2apic_mode(apic) && (icrl & APIC_ICR_BUSY))
> kvm_apic_write_nodecode(vcpu, APIC_ICR);
> else
> kvm_apic_send_ipi(apic, icrl, icrh);
>
