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

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

On Tue, Sep 12, 2023 at 05:35:15PM +0100, Andrew Cooper wrote:
> On 12/09/2023 5:23 pm, 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.
> >
> > In order to fix expose an empty HWCR.
> 
> At first I was thinking a straight up revert, but AMD's CPUID Faulting
> is an architectural bit in here so it's worth keeping the register around.

A straight up revert won't work, because (as you notice below)
HWCR is architectural, so accesses must not fault.

> >
> > 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.
> >
> > Missing reported by as the issue came from the QubesOS tracker.
> 
> Well - we can at least have a:
> 
> Link: https://github.com/QubesOS/qubes-issues/issues/8502

Sure.

> in the commit message, and it's probably worth asking Solène / Marek
> (both CC'd) if they want a Reported-by tag.

I'm happy to add a Reported-by tag, just didn't have an email to use.

> > ---
> >  xen/arch/x86/msr.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> > index 3f0450259cdf..964d500ff8a1 100644
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -240,8 +240,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
> > *val)
> >      case MSR_K8_HWCR:
> >          if ( !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) )
> >              goto gp_fault;
> > -        *val = get_cpu_family(cp->basic.raw_fms, NULL, NULL) >= 0x10
> > -               ? K8_HWCR_TSC_FREQ_SEL : 0;
> > +        /*
> > +         * OpenBSD 7.3 accesses HWCR unconditionally if the TSC is 
> > reported as
> > +         * Invariant.  Do not set TSC_FREQ_SEL as that would trigger 
> > OpenBSD to
> > +         * also poke at PSTATE0.
> > +         */
> 
> While this is true, the justification for removing this is because
> TSC_FREQ_SEL is a model specific bit, not an architectural bit in HWCR.
> 
> Also because it's addition without writing into the migration stream was
> bogus irrespective of the specifics of the bit.
> 
> I'm still of the opinion that it's buggy for OpenBSD to be looking at
> model specific bits when virtualised,

Well, I think we can argue that an OS is free to ignore the CPUID HV
bit and still boot on Xen (even if that leads to non-ideal decisions).

> but given my latest reading of the
> AMD manuals, I think OpenBSD *is* well behaved looking at PSTATE0 if it
> can see TSC_FREQ_SEL.

Hm, there's no written down note that TSC_FREQ_SEL implies PSTATE0 to
be available (and PSTATE0 is not an architectural MSR), but I can see
how a guest can expect to fetch the P0 frequency if it sees
TSC_FREQ_SEL.  It might have been more fail safe to check for
PSTATE_LIMIT not faulting before attempting to access PSTATE0.

> In some theoretical future where the toolstack better understands MSRs
> and (non)migratable VMs (which is the QubesOS usecase), then it would in
> principle be fine to construct a VM which can see the host TSC_FREQ_SEL
> and PSTATE* values.
> 
> Preferably with an adjusted comment, Reviewed-by: Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>

Thanks, will reply to other comments before taking the RB and
resending.

Roger.