Re: [PATCH v2] x86/amd: do not expose HWCR.TscFreqSel to guests

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/09/2023 3:52 pm, Roger Pau Monne wrote:
> OpenBSD 7.3 will unconditionally access HWCR if the TSC is reported as
> Invariant, and it will then attempt to also unconditionally access PSTATE0 if
> HWCR.TscFreqSel is set (currently the case on Xen).
>
> The relation between HWCR.TscFreqSel and PSTATE0 is not clearly written down 
> in
> the PPR, but it's natural for OSes to attempt to fetch the P0 frequency if the
> TSC increments at the P0 frequency.

"TSC is stated to increment at ..."  would be slightly clearer IMO.

>
> Exposing PSTATEn (PSTATE0 at least) with all zeroes is not a suitable solution
> because the PstateEn bit is read-write, and OSes could legitimately attempt to
> set PstateEn=1 which Xen couldn't handle.
>
> In order to fix expose an empty HWCR, which is an architectural MSR and so 
> must
> be accessible.
>
> Note it was not safe to expose the TscFreqSel bit because it is not
> architectural, and could change meaning between models.

I'd suggest rearranging and adjusting these two paragraphs.

---
Furthermore, the TscFreqSel bit is model specific and was never safe to
expose like this in the first place.  At a minimum it should have had a
toolstack adjustment to know not to migrate such a VM.

Therefore, simply remove the bit.  Note the MSR_HWCR itself is an
architectural register, and does need to be accessible by the guest.
---

>
> Reported-by: Solène Rapenne <solene@xxxxxxxxxxx>
> Link: https://github.com/QubesOS/qubes-issues/issues/8502
> Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

For the change itself, Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>