On 30.05.2023 12:00, Andrew Cooper wrote: > On 30/05/2023 10:18 am, Jan Beulich wrote: >> On 26.05.2023 13:06, Andrew Cooper wrote: >>> @@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void) >>> if ( safe ) >>> return true; >>> >>> + /* >>> + * The meaning of the RSBA and RRSBA bits have evolved over time. The >>> + * agreed upon meaning at the time of writing (May 2023) is thus: >>> + * >>> + * - RSBA (RSB Alterantive) means that an RSB may fall back to an >>> + * alternative predictor on underflow. Skylake uarch and later all >>> have >>> + * this property. Broadwell too, when running microcode versions >>> prior >>> + * to Jan 2018. >>> + * >>> + * - All eIBRS-capable processors suffer RSBA, but eIBRS also >>> introduces >>> + * tagging of predictions with the mode in which they were learned. >>> So >>> + * when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA). >>> + * >>> + * Some parts (Broadwell) are not expected to ever enumerate this >>> + * behaviour directly. Other parts have differing enumeration with >>> + * microcode version. Fix up Xen's idea, so we can advertise them >>> safely >>> + * to guests, and so toolstacks can level a VM safelty for migration. >>> + */ >> If the difference between the two is whether eIBRS is active (as you did >> word it yet more explicitly in e.g. [1]), then ... >> >>> + unsafe_maybe_fixup_rrsba: >>> + if ( !cpu_has_rrsba ) >>> + setup_force_cpu_cap(X86_FEATURE_RRSBA); >>> + >>> + unsafe_maybe_fixup_rsba: >>> + if ( !cpu_has_rsba ) >>> + setup_force_cpu_cap(X86_FEATURE_RSBA); >>> + >>> return false; >>> } >> ... can both actually be active at the same time? IOW is there a "return >> false" missing ahead of the 2nd label? > > I've already got a question out to Intel to this effect. (I didn't say > the enumeration made much sense...) > >> Not having looked at further patches yet it also strikes me as odd that >> each of the two labels is used exactly once only. Leaving the shared >> comment aside, imo this would then better avoid "goto". > > They're both used twice, not once. You asked why it wasn't "return > safe;" in the previous patch? Well this is why.
Ouch, yes. The labels themselves are used just once, but there's important fall-through from above here. >> Finally, what use are the two if()s? There's nothing wrong with forcing >> a feature which is already available. > > It breaks is_forced_cpu_cap(). Hmm, yes, but is that important here? (If you decide to keep the if()s, which I'm not opposed to, would you mind adding half a sentence to the description or maybe a brief code comment?) Jan > Also, I considered having a printk() here. I've still got it around in > a debug patch, but I decided against it. > > ~Andrew
