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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2023 13:02, Andrew Cooper wrote:
> On 13/09/2023 7:18 am, Jan Beulich wrote:
>> On 12.09.2023 18:35, Andrew Cooper wrote:
>>> On 12/09/2023 5:23 pm, Roger Pau Monne wrote:
>>>> --- 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.
>> I'm afraid I'm still at a loss seeing what wording in which PPR makes you
>> see a connection. If there was a connection, I'd like to ask that we at
>> least properly consider exposing PSTATE0 (or if necessary all PSTATEn) as
>> well, with zero value (i.e. in particular with the valid bit clear),
>> rather than exposing a r/o bit with the wrong value, upsetting Linux.
> 
> This mess is caused by the erroneous advertising of a model specific
> bit, and there's no way in hell that giving the guest even more model
> specific data is going to fix it.
> 
> The PSTATE MSRs are entirely model specific, fully read/write, and the
> Enable bit is not an enable bit; its a "not valid yet" bit that firmware
> is required to adjust to be consistent across the coherency fabric.
> 
> Linux is simply wrong with it's printk() under virt, and wants adjusting.

Assuming Roger agrees with this statement, then I think this is another
item wanting mentioning in the description. I then still wouldn't ack
the change, but I also wouldn't object to it anymore.

Jan