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?
