On Tue, Sep 27, 2022 at 09:43:08PM +0000, Sean Christopherson wrote:
>The benefits I see are:
>
> 1. The code is very explicit. When clearing SN, check PIR and set ON=1 to
> ensure IRQs aren't ignored.
>
> 2. pi_enable_wakeup_handler() is only responsible for changing the vector,
> i.e. there's no clearing of SN "hidden" in the CMPXCHG loop.
>
> 3. Same for vmx_vcpu_pi_load(), it's only responsible for updating the pCPU
> and the vector, it doesn't touch SN.
>
> 4. The logic is common to all paths that clear SN, i.e. the same helper can
> be used for both VM-Enter and vCPU blocking.
>
>E.g. the VMX hook for both VM-Enter and vCPU blocking could be:
>
> static void vmx_no_idea_what_to_call_this(struct kvm_vcpu *vcpu)
> {
> pi_clear_sn(&vmx->pi_desc);
>
> /*
> * Clear SN before reading the bitmap. The VT-d firmware writes the
> * bitmap and reads SN atomically (5.2.3 in the spec), so it doesn't
> * really have a memory barrier that pairs with this, but we cannot do
> * that and we need one.
> */
> smp_mb__after_atomic();
>
> /* blah blah blah */
> if (!pi_test_on(&vmx->pi_desc) && !pi_is_pir_empty(&vmx->pi_desc))
> pi_set_on(&vmx->pi_desc);
> }
>
>One related thought thing I want to capture:
>
>The proposed approach is having vmx_sync_pir_to_irr() check PIR if ON=1 || SN=1
>is effectively _required_ to avoid breaking halt-polling. I was thinking we could
>keep that path optimized to check only ON=1, but with that approach, KVM won't detect
>the pending IRQ until it actually starts to block the vCPU, i.e. until it clears SN
>and manually checks the PIR. Clearing SN in kvm_arch_vcpu_pending() would be better
>than waiting until the vCPU is "put", but even that is too late from a halt-polling
>perspective.
>
>A comment in vmx_sync_pir_to_irr() is likely needed to capture that.
All your comments make sense to me. I just posted v3 with all issues of v2
addressed.
