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.

Reported-by: Solène Rapenne <solene@openbsd.org>
Link: https://github.com/QubesOS/qubes-issues/issues/8502
Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/msr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 3f0450259cdf..c33dc78cd8f6 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -240,8 +240,7 @@ 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;
+        *val = 0;
         break;
 
     case MSR_VIRT_SPEC_CTRL:
-- 
2.42.0

On 13/09/2023 3:52 pm, 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.

"TSC is stated to increment at ..."  would be slightly clearer IMO.

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

I'd suggest rearranging and adjusting these two paragraphs.

---
Furthermore, the TscFreqSel bit is model specific and was never safe to
expose like this in the first place.  At a minimum it should have had a
toolstack adjustment to know not to migrate such a VM.

Therefore, simply remove the bit.  Note the MSR_HWCR itself is an
architectural register, and does need to be accessible by the guest.
---

>
> Reported-by: Solène Rapenne <solene@openbsd.org>
> Link: https://github.com/QubesOS/qubes-issues/issues/8502
> Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

For the change itself, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

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.

> Reported-by: Solène Rapenne <solene@openbsd.org>
> Link: https://github.com/QubesOS/qubes-issues/issues/8502
> Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

As mentioned before, with this Fixes: tag I think the description
absolutely needs to mention the (changed) view on the Linux log message
that had triggered the original change. It certainly helps here that
Thomas has already signaled agreement to a Linux side change.

Jan

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.

> > Reported-by: Solène Rapenne <solene@openbsd.org>
> > Link: https://github.com/QubesOS/qubes-issues/issues/8502
> > Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> As mentioned before, with this Fixes: tag I think the description
> absolutely needs to mention the (changed) view on the Linux log message
> that had triggered the original change. It certainly helps here that
> Thomas has already signaled agreement to a Linux side change.

Sure, what about:

"The motivation for exposing HWCR.TscFreqSel was to avoid warning
messages from Linux.  It has been agreed that Linux should be changed
instead to not complaint about missing HWCR.TscFreqSel when running
virtualized."

Thanks, Roger.

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, and any such quirks we add
generally want justifying imo (as they might bite us again later).

>>> Reported-by: Solène Rapenne <solene@openbsd.org>
>>> Link: https://github.com/QubesOS/qubes-issues/issues/8502
>>> Fixes: 14b95b3b8546 ('x86/AMD: expose HWCR.TscFreqSel to guests')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> As mentioned before, with this Fixes: tag I think the description
>> absolutely needs to mention the (changed) view on the Linux log message
>> that had triggered the original change. It certainly helps here that
>> Thomas has already signaled agreement to a Linux side change.
> 
> Sure, what about:
> 
> "The motivation for exposing HWCR.TscFreqSel was to avoid warning
> messages from Linux.  It has been agreed that Linux should be changed
> instead to not complaint about missing HWCR.TscFreqSel when running
> virtualized."

Reads okay to me, thanks.

Jan

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.

Thanks, Roger.

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.

Jan

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

Thanks, Roger.

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