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
