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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> 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

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

> ---
>  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, 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.

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>

~Andrew