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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.