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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew