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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/09/2023 8:50 am, Roger Pau Monné wrote:
> On Tue, Sep 12, 2023 at 05:35:15PM +0100, Andrew Cooper wrote:
>> On 12/09/2023 5:23 pm, Roger Pau Monne wrote:
>>> ---
>>>  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).

An OS which keeps itself to architectural details that we do our very
best to be correct with, ought to function even if it ignores the HV bit.

But we're (deliberately) not doing model-specific-accurate emulations of
a specific platform.  An OS which ignores details about the environment
it is operating in gets to keep the faults/malfunctions when it does
something illegal.

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

The PPR is a reference of mostly autogenerated details and misc notes,
put together in a non- hand-write way, unlike the older BKWGs.

Lots of the information elided from public and partner-NDA versions is
"see TICKET/LINK for rational" type comments.

It is not a spec - it is a reference (the clue is even in the name)
aimed at people already familiar with the area.  Do not fall into the
trap of thinking it it can be read as a spec.

~Andrew