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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2023 15:39, Roger Pau Monné wrote:
> On Thu, Sep 14, 2023 at 03:16:40PM +0200, Jan Beulich wrote:
>> On 14.09.2023 14:57, Roger Pau Monné wrote:
>>> On Thu, Sep 14, 2023 at 02:49:45PM +0200, Jan Beulich wrote:
>>>> On 14.09.2023 14:37, Roger Pau Monné wrote:
>>>>> On Thu, Sep 14, 2023 at 07:52:33AM +0200, Jan Beulich wrote:
>>>>>> On 13.09.2023 16:52, Roger Pau Monne wrote:
>>>>>>> OpenBSD 7.3 will unconditionally access HWCR if the TSC is reported as
>>>>>>> Invariant, and it will then attempt to also unconditionally access 
>>>>>>> PSTATE0 if
>>>>>>> HWCR.TscFreqSel is set (currently the case on Xen).
>>>>>>>
>>>>>>> The relation between HWCR.TscFreqSel and PSTATE0 is not clearly written 
>>>>>>> down in
>>>>>>> the PPR, but it's natural for OSes to attempt to fetch the P0 frequency 
>>>>>>> if the
>>>>>>> TSC increments at the P0 frequency.
>>>>>>>
>>>>>>> Exposing PSTATEn (PSTATE0 at least) with all zeroes is not a suitable 
>>>>>>> solution
>>>>>>> because the PstateEn bit is read-write, and OSes could legitimately 
>>>>>>> attempt to
>>>>>>> set PstateEn=1 which Xen couldn't handle.
>>>>>>>
>>>>>>> In order to fix expose an empty HWCR, which is an architectural MSR and 
>>>>>>> so must
>>>>>>> be accessible.
>>>>>>>
>>>>>>> Note it was not safe to expose the TscFreqSel bit because it is not
>>>>>>> architectural, and could change meaning between models.
>>>>>>
>>>>>> This imo wants (or even needs) extending to address the aspect of then
>>>>>> exposing, on newer families, a r/o bit with the wrong value.
>>>>>
>>>>> We could always be exposing bits with the wrong values on newer
>>>>> (unreleased?) families, I'm not sure why it needs explicit mentioning
>>>>> here.
>>>>
>>>> Hmm, yes, that's one way to look at things. Yet exposing plain zero is
>>>> pretty clearly not within spec here,
>>>
>>> As I understand it, the fact that HWCR.TscFreqSel is read-only doesn't
>>> exclude the possibility of it changing using other means (iow: we
>>> should consider that a write to a different register could have the
>>> side effect of toggling the bit).
>>>
>>> The PPR I'm reading doesn't mention that the bit must be 1, just that
>>> it's 1 on reset and read-only.
>>
>> Sure; the PPR being incomplete doesn't help here. My interpretation, based
>> on the bit having been r/w in earlier families, is that AMD wanted to retain
>> its meaning without allowing it to be configurable anymore. Possibly a sign
>> of it remaining so going forward.
> 
> What about:
> 
> "Note it was not safe to expose the TscFreqSel bit because it is not
> architectural, and could change meaning between models.  Since HWCR
> contains both architectural and non-architectural bits, going forward
> care must be taken to assert the exposed value is correct on newer
> CPU families."

Fine with me.

Jan