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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 11:16, Roger Pau Monné wrote:
> On Wed, Sep 13, 2023 at 10:35:17AM +0200, Jan Beulich wrote:
>> 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.
> 
> OpenBSD can't be used as dom0, but QubesOS does use the nomigrate flag
> for domUs.
> 
>> 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.
> 
> I did consider this, but it seemed to just dig us deeper into exposing
> non-architectural MSRs, which in the long run is more likely to be
> troublesome if newer models change the meaning of bits in PSTATEn.

Not sure about "deeper" - we're discussing to expose a non-architectural
bit in HWCR with the wrong value vs exposing a non-architectural MSR
where we'd rely on only the top bit retaining its meaning going forward.

Jan