Re: [PATCH v6 2/2] x86/tlb: skip redundant sync IPIs for native TLB flush

[Dave Hansen <dave.hansen@intel.com>]

On 3/3/26 18:10, Lance Yang wrote:
...
> +	if (pv_ops.mmu.flush_tlb_multi == native_flush_tlb_multi &&
> +	    !cpu_feature_enabled(X86_FEATURE_INVLPGB)) {
> +		pv_ops.mmu.flush_tlb_multi_implies_ipi_broadcast = true;
> +		static_branch_enable(&tlb_ipi_broadcast_key);
> +	}
> +}
...
> +#ifndef CONFIG_PARAVIRT
> +void __init native_pv_tlb_init(void)
> +{
> +	/*
> +	 * For non-PARAVIRT builds, check if native TLB flush sends real IPIs
> +	 * (i.e., not using INVLPGB broadcast invalidation).
> +	 */
> +	if (!cpu_feature_enabled(X86_FEATURE_INVLPGB))
> +		static_branch_enable(&tlb_ipi_broadcast_key);
> +}
> +#endif

I really despise duplicated logic. The X86_FEATURE_INVLPGB check is
small, but it is duplicated. You're also setting the static branch in a
*bunch* of different places.

Can this be arranged so that the PV code just tells the core code that
it is compatible with flush_tlb_multi_implies_ipi_broadcast?

void __init bool is_pv_ok(void)
{
	/* This check is super sketchy an unexplained: */
	if (pv_ops.mmu.flush_tlb_multi_implies_ipi_broadcast)
		return true;

	if (pv_ops.mmu.flush_tlb_multi != native_flush_tlb_multi)
		return false;

	pv_ops.mmu.flush_tlb_multi_implies_ipi_broadcast = true;

	return true;
}

void __init tlb_init(void)
{
	if (!is_pv_ok())
		return;

	if (cpu_feature_enabled(X86_FEATURE_INVLPGB))
		return;
		
	static_branch_enable(&tlb_ipi_broadcast_key);
}

Isn't that like a billion times more readable? It has one
X86_FEATURE_INVLPGB check and one static_branch_enable() point and no
#ifdeffery other than defining a stub is_pv_ok().

BTW, why is there even an early return for the case where
flush_tlb_multi_implies_ipi_broadcast is already set? Isn't this
decision made once on the boot CPU and then never touched again? Do any
PV instances actually set the bit?