On 16/08/2023 5:41 pm, Solar Designer wrote: On Tue, Aug 08, 2023 at 07:18:51PM +0100, Andrew Cooper wrote: On 08/08/2023 7:00 pm, Solar Designer wrote: + / + Microcode is the preferred mitigation, in terms of performance. + However, without microcode, this chickenbit (specific to the Zen2 + uarch) disables Floating Point Mov-Elimination to mitigate the + issue. + / + val &= ~chickenbit; + if (sig->rev < goodrev) + val |= chickenbit;

This leaves me wondering: why have this line at all? I understand Xen wanting to enable the chicken bit on vulnerable CPUs, but why disable it on other AMD CPUs? If someone or something had enabled the bit, that's probably intentional, and even if not it probably shouldn't be Xen's business to alter CPU behavior beyond what's necessary for Xen itself to work reliably and securely.

Am I missing something? There is an earlier exit in this function for any non-Zen2 system.

So here, we are strictly on Zen2 (all vulnerable), and either have good microcode or not.

The microcode fix is far more performant than the chickenbit. Sure, but that's orthogonal to my concern, which was about areas of responsibility and control (such as sysadmin vs. tools).

Anyway, it was pointed out to me off-list that Linux kernel does the same thing, also explicitly disabling chickenbit when deemed safe:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=522b1d69219d8f083173819fde04f994aa051a98

+ if (!cpuhaszenbleedmicrocode()) { + prnoticeonce("Zenbleed: please update your microcode for the most optimal fix\n"); + msrsetbit(MSRAMD64DECFG, MSRAMD64DECFGZEN2FPBACKUPFIXBIT); + } else { + msrclearbit(MSRAMD64DECFG, MSRAMD64DECFGZEN2FPBACKUPFIXBIT); + }

So at least it's a consistent approach by these two projects, and a reason for Xen to be doing it this way. It is not a coincidence that Xen and Linux are similar here.  The areas of responsibility aspect was raised during review - we did consider combining with the old value.

AMD's position AIUI is that prior to Zenbleed, this bit was unsupported and not used.  Therefore we went for the simpler approach (and as you saw, still managed to screw that up).

~Andrew