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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 10:20, Roger Pau Monné wrote:
> On Wed, Sep 13, 2023 at 08:14:46AM +0200, Jan Beulich wrote:
>> On 12.09.2023 18:23, Roger Pau Monne wrote:
>>> OpenBSD will attempt to unconditionally access PSTATE0 if HWCR.TscFreqSel is
>>> set, and will also attempt to unconditionally access HWCR if the TSC is
>>> reported as Invariant.
>>>
>>> The reasoning for exposing HWCR.TscFreqSel was to avoid Linux from printing 
>>> a
>>> (bogus) warning message, but doing so at the cost of OpenBSD not booting is 
>>> not
>>> a suitable solution.
>>
>> Why is the warning bogus? The PPR doesn't even state what the bit being
>> clear means; it's a r/o one. On respective families it cannot possibly
>> be correct to expose it clear.
> 
> There are other bits in the same MSR that only state the meaning of
> one value and are not r/o, so my take is that being clear means "The
> TSC doesn't increment at the P0 frequency".
> 
> Since it's model specific, it should be possible for some models to
> have the bit clear.

The code that's in there right now already has a family >= 0x10 check.
The Fam10 BKDG says (among other things) "BIOS should program this bit
to 1." That, imo, doesn't leave much room for the bit being clear once
an OS (or hypervisor) gains control from firmware.

>>> In order to fix expose an empty HWCR.
>>>
>>> Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>> Not sure whether we want to expose something when is_cpufreq_controller() is
>>> true, seeing as there's a special wrmsr handler for the same MSR in that 
>>> case.
>>> Likely should be done for PV only, but also likely quite bogus.
>>
>> Well, keying to is_cpufreq_controller() is indeed questionable, but is
>> there any reason to not minimally expose the bit correctly when a
>> domain cannot migrate?
> 
> We would then also need to expose PSTATE0 at least to make OpenBSD
> happy (and any otehr guest that makes the connection between
> TscFreqSel and getting the P0 frequency from PSTATE0.

If any such OSes can be used as Dom0, yes. And as said before, I view
exposing PSTATE0 (with zero value) as a viable alternative to your
partial revert anyway. What varies across families is how many PSTATEn
there are, but at least starting from Fam10 PSTATE0 looks to always be
there, with the top bit indicating validity of the other defined bits.

Jan