[PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351

[PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351

[Andrew Cooper]

This is slightly complicated.  Patches 1 and 2 rearrange the code to look and
behave more like 4.14, and patch 3 fixes a Solaris (and turbostat) bug in a
manner which can be backported to all security trees.

Andrew Cooper (3):
  Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"
  x86/msr: Forward port XSA-351 changes from 4.14
  x86/msr: Fix Solaris and turbostat following XSA-351

 xen/arch/x86/msr.c              | 78 +++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/msr-index.h | 39 +++++++++++++++++++++
 2 files changed, 117 insertions(+)

-- 
2.11.0

[PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Andrew Cooper]

In hindsight, this was a poor move.  Some of these MSRs require probing for,
causing unhelpful spew into xl dmesg, as well as spew from unit tests
explicitly checking behaviour.

This restores behaviour close to that of Xen 4.14.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Ian Jackson <iwj@xenproject.org>

For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix needing
backporting.
---
 xen/arch/x86/msr.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 0ebcb04259..c3a988bd11 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -175,6 +175,30 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
 
     switch ( msr )
     {
+        /* Write-only */
+    case MSR_AMD_PATCHLOADER:
+    case MSR_IA32_UCODE_WRITE:
+    case MSR_PRED_CMD:
+    case MSR_FLUSH_CMD:
+
+        /* Not offered to guests. */
+    case MSR_TEST_CTRL:
+    case MSR_CORE_CAPABILITIES:
+    case MSR_TSX_FORCE_ABORT:
+    case MSR_TSX_CTRL:
+    case MSR_MCU_OPT_CTRL:
+    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
+    case MSR_U_CET:
+    case MSR_S_CET:
+    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
+    case MSR_AMD64_LWP_CFG:
+    case MSR_AMD64_LWP_CBADDR:
+    case MSR_PPIN_CTL:
+    case MSR_PPIN:
+    case MSR_AMD_PPIN_CTL:
+    case MSR_AMD_PPIN:
+        goto gp_fault;
+
     case MSR_IA32_FEATURE_CONTROL:
         /*
          * Architecturally, availability of this MSR is enumerated by the
@@ -382,6 +406,30 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     {
         uint64_t rsvd;
 
+        /* Read-only */
+    case MSR_IA32_PLATFORM_ID:
+    case MSR_CORE_CAPABILITIES:
+    case MSR_INTEL_CORE_THREAD_COUNT:
+    case MSR_INTEL_PLATFORM_INFO:
+    case MSR_ARCH_CAPABILITIES:
+
+        /* Not offered to guests. */
+    case MSR_TEST_CTRL:
+    case MSR_TSX_FORCE_ABORT:
+    case MSR_TSX_CTRL:
+    case MSR_MCU_OPT_CTRL:
+    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
+    case MSR_U_CET:
+    case MSR_S_CET:
+    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
+    case MSR_AMD64_LWP_CFG:
+    case MSR_AMD64_LWP_CBADDR:
+    case MSR_PPIN_CTL:
+    case MSR_PPIN:
+    case MSR_AMD_PPIN_CTL:
+    case MSR_AMD_PPIN:
+        goto gp_fault;
+
     case MSR_AMD_PATCHLEVEL:
         BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
         /*
-- 
2.11.0

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Jan Beulich]

On 16.03.2021 17:18, Andrew Cooper wrote:
> In hindsight, this was a poor move.  Some of these MSRs require probing for,
> causing unhelpful spew into xl dmesg, as well as spew from unit tests
> explicitly checking behaviour.

I can indeed see your point for MSRs that require probing. But what about
the others (which, as it seems, is the majority)? And perhaps specifically
what about the entire WRMSR side, which won't be related to probing? I'm
not opposed to the change, but I'd like to understand the reasoning for
every one of the MSRs, not just a subset.

Of course such ever-growing lists of case labels aren't very nice - this
going away was one of the things I particularly liked about the original
change.

Jan

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -175,6 +175,30 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>  
>      switch ( msr )
>      {
> +        /* Write-only */
> +    case MSR_AMD_PATCHLOADER:
> +    case MSR_IA32_UCODE_WRITE:
> +    case MSR_PRED_CMD:
> +    case MSR_FLUSH_CMD:
> +
> +        /* Not offered to guests. */
> +    case MSR_TEST_CTRL:
> +    case MSR_CORE_CAPABILITIES:
> +    case MSR_TSX_FORCE_ABORT:
> +    case MSR_TSX_CTRL:
> +    case MSR_MCU_OPT_CTRL:
> +    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> +    case MSR_U_CET:
> +    case MSR_S_CET:
> +    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
> +    case MSR_AMD64_LWP_CFG:
> +    case MSR_AMD64_LWP_CBADDR:
> +    case MSR_PPIN_CTL:
> +    case MSR_PPIN:
> +    case MSR_AMD_PPIN_CTL:
> +    case MSR_AMD_PPIN:
> +        goto gp_fault;
> +
>      case MSR_IA32_FEATURE_CONTROL:
>          /*
>           * Architecturally, availability of this MSR is enumerated by the
> @@ -382,6 +406,30 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>      {
>          uint64_t rsvd;
>  
> +        /* Read-only */
> +    case MSR_IA32_PLATFORM_ID:
> +    case MSR_CORE_CAPABILITIES:
> +    case MSR_INTEL_CORE_THREAD_COUNT:
> +    case MSR_INTEL_PLATFORM_INFO:
> +    case MSR_ARCH_CAPABILITIES:
> +
> +        /* Not offered to guests. */
> +    case MSR_TEST_CTRL:
> +    case MSR_TSX_FORCE_ABORT:
> +    case MSR_TSX_CTRL:
> +    case MSR_MCU_OPT_CTRL:
> +    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> +    case MSR_U_CET:
> +    case MSR_S_CET:
> +    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
> +    case MSR_AMD64_LWP_CFG:
> +    case MSR_AMD64_LWP_CBADDR:
> +    case MSR_PPIN_CTL:
> +    case MSR_PPIN:
> +    case MSR_AMD_PPIN_CTL:
> +    case MSR_AMD_PPIN:
> +        goto gp_fault;
> +
>      case MSR_AMD_PATCHLEVEL:
>          BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
>          /*
> 

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Andrew Cooper]

On 16/03/2021 16:58, Jan Beulich wrote:
> On 16.03.2021 17:18, Andrew Cooper wrote:
>> In hindsight, this was a poor move.  Some of these MSRs require probing for,
>> causing unhelpful spew into xl dmesg, as well as spew from unit tests
>> explicitly checking behaviour.
> I can indeed see your point for MSRs that require probing. But what about
> the others (which, as it seems, is the majority)? And perhaps specifically
> what about the entire WRMSR side, which won't be related to probing? I'm
> not opposed to the change, but I'd like to understand the reasoning for
> every one of the MSRs, not just a subset.
>
> Of course such ever-growing lists of case labels aren't very nice - this
> going away was one of the things I particularly liked about the original
> change.

The logging in the default case is only useful when it is genuinely MSRs
we haven't considered.

It is very useful at pointing bugs in guests, or bugs in Xen, but only
when the logging is not drowned out by things we know about.

~Andrew

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Jan Beulich]

On 19.03.2021 13:59, Andrew Cooper wrote:
> On 16/03/2021 16:58, Jan Beulich wrote:
>> On 16.03.2021 17:18, Andrew Cooper wrote:
>>> In hindsight, this was a poor move.  Some of these MSRs require probing for,
>>> causing unhelpful spew into xl dmesg, as well as spew from unit tests
>>> explicitly checking behaviour.
>> I can indeed see your point for MSRs that require probing. But what about
>> the others (which, as it seems, is the majority)? And perhaps specifically
>> what about the entire WRMSR side, which won't be related to probing? I'm
>> not opposed to the change, but I'd like to understand the reasoning for
>> every one of the MSRs, not just a subset.
>>
>> Of course such ever-growing lists of case labels aren't very nice - this
>> going away was one of the things I particularly liked about the original
>> change.
> 
> The logging in the default case is only useful when it is genuinely MSRs
> we haven't considered.
> 
> It is very useful at pointing bugs in guests, or bugs in Xen, but only
> when the logging is not drowned out by things we know about.

So would you mind adjusting the description accordingly? Right now, the
way it's written, it reads (to my non-native interpretation) as entirely
focusing on guests' probing needs. Even an adjustment as simple as

"In hindsight, this was a poor move.  Some of these MSRs require probing for,
 cause unhelpful spew into xl dmesg, or cause spew from unit tests
 explicitly checking behaviour."

would already shift the focus imo.

Jan

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Andrew Cooper]

On 19/03/2021 13:56, Jan Beulich wrote:
> On 19.03.2021 13:59, Andrew Cooper wrote:
>> On 16/03/2021 16:58, Jan Beulich wrote:
>>> On 16.03.2021 17:18, Andrew Cooper wrote:
>>>> In hindsight, this was a poor move.  Some of these MSRs require probing for,
>>>> causing unhelpful spew into xl dmesg, as well as spew from unit tests
>>>> explicitly checking behaviour.
>>> I can indeed see your point for MSRs that require probing. But what about
>>> the others (which, as it seems, is the majority)? And perhaps specifically
>>> what about the entire WRMSR side, which won't be related to probing? I'm
>>> not opposed to the change, but I'd like to understand the reasoning for
>>> every one of the MSRs, not just a subset.
>>>
>>> Of course such ever-growing lists of case labels aren't very nice - this
>>> going away was one of the things I particularly liked about the original
>>> change.
>> The logging in the default case is only useful when it is genuinely MSRs
>> we haven't considered.
>>
>> It is very useful at pointing bugs in guests, or bugs in Xen, but only
>> when the logging is not drowned out by things we know about.
> So would you mind adjusting the description accordingly? Right now, the
> way it's written, it reads (to my non-native interpretation) as entirely
> focusing on guests' probing needs. Even an adjustment as simple as
>
> "In hindsight, this was a poor move.  Some of these MSRs require probing for,
>  cause unhelpful spew into xl dmesg, or cause spew from unit tests
>  explicitly checking behaviour."
>
> would already shift the focus imo.

Sure.  I'll try to make this clearer.

~Andrew

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Roger Pau Monné]

On Tue, Mar 16, 2021 at 04:18:42PM +0000, Andrew Cooper wrote:
> In hindsight, this was a poor move.  Some of these MSRs require probing for,
> causing unhelpful spew into xl dmesg, as well as spew from unit tests
> explicitly checking behaviour.
> 
> This restores behaviour close to that of Xen 4.14.

I think it might be worth adding that guest access to those MSRs will
now always trigger a #GP, even when msr_relaxed is used. This is
however fine, as that's not a regression when compared to older Xen
versions, where access to the MSRs also trigger a #GP
unconditionally.

I assume the wrmsr side is added so that when using msr_relaxed Xen
also injects a #GP for writes to those MSRs, as it would do for
reads?

Thanks, Roger.

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Ian Jackson]

I have read this thread and with my release manager hat on I feel
confused and/or ignorant.

Patch 3/ has a good explanation of what the problem is it is
addressing and why this is important for 4.15.  But then there is
Jan's most recent reply starting "I find all of this confusing".  Jan,
can you please tell me in words of one syllable what the implication
of that message is ?  In particular is any of what you say a reason
for me to withhold my release-ack ?

AFAICT there is no explanation for why patches 1/ and 2/ deserve to go
into 4.15.  We are late in the freeze now, so I would ideally be
looking for a clear and compelling argument.  I'd also like to
understand what the risks are of taking these.  Can someone please
enlighten me ?

Thanks,
Ian.

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Andrew Cooper]

On 17/03/2021 13:37, Ian Jackson wrote:
> I have read this thread and with my release manager hat on I feel
> confused and/or ignorant.
>
> Patch 3/ has a good explanation of what the problem is it is
> addressing and why this is important for 4.15.  But then there is
> Jan's most recent reply starting "I find all of this confusing".  Jan,
> can you please tell me in words of one syllable what the implication
> of that message is ?  In particular is any of what you say a reason
> for me to withhold my release-ack ?
>
> AFAICT there is no explanation for why patches 1/ and 2/ deserve to go
> into 4.15.  We are late in the freeze now, so I would ideally be
> looking for a clear and compelling argument.  I'd also like to
> understand what the risks are of taking these.  Can someone please
> enlighten me ?

To make the code in 4.15 match 4.14, so patch 3 can be written in the
first place.

Also, as a side benefit, patches 1 and 2 reduce the quantity of logspew
from the impacted MSRs.

We cannot simply take patch 3 as-is, and say "4.14 and earlier" for
backport, because that still forces end users to specify msr_relaxed to
unbreak their Solaris guests, which is usability regression vs 4.14

~Andrew

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()""):
> On 17/03/2021 13:37, Ian Jackson wrote:
> > AFAICT there is no explanation for why patches 1/ and 2/ deserve to go
> > into 4.15.

I see now, rereading the thread, that there was a sentence about this
in each patch betwen the commit message and the diff.  Sorry for
missing that.  (Although TBH at least one of those messages could
usefully have gone into the commit message, as useful motivational
background.)

> >   We are late in the freeze now, so I would ideally be
> > looking for a clear and compelling argument.  I'd also like to
> > understand what the risks are of taking these.  Can someone please
> > enlighten me ?
> 
> To make the code in 4.15 match 4.14, so patch 3 can be written in the
> first place.
> 
> Also, as a side benefit, patches 1 and 2 reduce the quantity of logspew
> from the impacted MSRs.
> 
> We cannot simply take patch 3 as-is, and say "4.14 and earlier" for
> backport, because that still forces end users to specify msr_relaxed to
> unbreak their Solaris guests, which is usability regression vs 4.14

This is plausible and going in the right direction but I still feel
uncertain.

Jan, what is your summary opinion about patch 3 ?

Roger, can I get your opinion about the possible downside risks of
this patch ?

Thanks,
Ian.

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Roger Pau Monné]

On Wed, Mar 17, 2021 at 02:46:20PM +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()""):
> > On 17/03/2021 13:37, Ian Jackson wrote:
> > > AFAICT there is no explanation for why patches 1/ and 2/ deserve to go
> > > into 4.15.
> 
> I see now, rereading the thread, that there was a sentence about this
> in each patch betwen the commit message and the diff.  Sorry for
> missing that.  (Although TBH at least one of those messages could
> usefully have gone into the commit message, as useful motivational
> background.)
> 
> > >   We are late in the freeze now, so I would ideally be
> > > looking for a clear and compelling argument.  I'd also like to
> > > understand what the risks are of taking these.  Can someone please
> > > enlighten me ?
> > 
> > To make the code in 4.15 match 4.14, so patch 3 can be written in the
> > first place.
> > 
> > Also, as a side benefit, patches 1 and 2 reduce the quantity of logspew
> > from the impacted MSRs.
> > 
> > We cannot simply take patch 3 as-is, and say "4.14 and earlier" for
> > backport, because that still forces end users to specify msr_relaxed to
> > unbreak their Solaris guests, which is usability regression vs 4.14
> 
> This is plausible and going in the right direction but I still feel
> uncertain.
> 
> Jan, what is your summary opinion about patch 3 ?
> 
> Roger, can I get your opinion about the possible downside risks of
> this patch ?

For patches 1 and 2 the risk is low I think. This is already the same
handling that we do in pre-4.15, so it's unlikely to cause issues.
From a guests PoV they don't change the result of trying to access any
of the modified MSRs, accessing them will still result in a #GP being
injected to the guest.

The main risk for patch 3 would be that reporting 0 for
MSR_RAPL_POWER_UNIT would result in some kind of issue on certain
guests, or that it triggers the poking of other MSRs in the
expectation that they would be available. I think those are quite
unlikely, and the patch fixes a real issue with Solaris guests.

Roger.

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Ian Jackson]

Roger Pau Monné writes ("Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()""):
> On Wed, Mar 17, 2021 at 02:46:20PM +0000, Ian Jackson wrote:
> > Roger, can I get your opinion about the possible downside risks of
> > this patch ?
> 
> For patches 1 and 2 the risk is low I think. This is already the same
> handling that we do in pre-4.15, so it's unlikely to cause issues.
> >From a guests PoV they don't change the result of trying to access any
> of the modified MSRs, accessing them will still result in a #GP being
> injected to the guest.
> 
> The main risk for patch 3 would be that reporting 0 for
> MSR_RAPL_POWER_UNIT would result in some kind of issue on certain
> guests, or that it triggers the poking of other MSRs in the
> expectation that they would be available. I think those are quite
> unlikely, and the patch fixes a real issue with Solaris guests.

Thanks.  That is very helpful.

All three patches 

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

but subject to Jan's questions on patch 3 being resolved somehow.

Ian.

Re: [PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351 [and 1 more messages]

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351"):
> This is slightly complicated.  Patches 1 and 2 rearrange the code to look and
> behave more like 4.14, and patch 3 fixes a Solaris (and turbostat) bug in a
> manner which can be backported to all security trees.

As far as I can tell this series needs a respin ?

I have been through the thread and AFAICT the only comments were on
the commit message for patch 2.  Patchex 1 and 3 already have a
release-ack.  Patch 2 does not have any mind of maintainer review.

I would like this series to go in today.

Jan, since Andrew doesn't seem to have been able to do that respin
yet, would you be able to rewrite the commit message of message 2
taking into account the two comments from you an from Roger ?

I think that is all that's needed for these three to go into tree.

Ian.

Re: [PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351 [and 1 more messages]

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351 [and 1 more messages]"):
> Andrew Cooper writes ("[PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351"):
> > This is slightly complicated.  Patches 1 and 2 rearrange the code to look and
> > behave more like 4.14, and patch 3 fixes a Solaris (and turbostat) bug in a
> > manner which can be backported to all security trees.
> 
> As far as I can tell this series needs a respin ?
> 
> I have been through the thread and AFAICT the only comments were on
> the commit message for patch 2.  Patchex 1 and 3 already have a
> release-ack.  Patch 2 does not have any mind of maintainer review.

Err, this is wrong.

It is

  [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in
  guest_{rd, wr}msr()"

which has comments on the commit message from Jan:

  So would you mind adjusting the description accordingly? [...]
  "In hindsight, this was a poor move [..,]

and Roger:

  I think it might be worth adding that guest access to those MSRs
  will now always trigger a #GP [...]

I could easily fold in Jan's comments but it would be better for
someone more familiar with the code do handle Roger's since Roger
doesn't provide a precise wording.

These two

  [PATCH 2/3] x86/msr: Forward port XSA-351 changes from 4.14
  [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

have reviews from Roger.

> I would like this series to go in today.
> 
> Jan, since Andrew doesn't seem to have been able to do that respin
> yet, would you be able to rewrite the commit message of message 2
> taking into account the two comments from you an from Roger ?
> 
> I think that is all that's needed for these three to go into tree.

Ian.

Re: [PATCH for-4.15 0/3] x86/msr: Fixes for XSA-351 [and 1 more messages]

[Roger Pau Monné]

On Fri, Mar 26, 2021 at 02:30:29PM +0000, Ian Jackson wrote:
> These two
> 
>   [PATCH 2/3] x86/msr: Forward port XSA-351 changes from 4.14
>   [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351
> 
> have reviews from Roger.

From what I read on IRC patch 3 could cause issues with some Linux
versions?

I'm afraid it's not clear to me from the IRC conversation what's the
issue exactly, so for the sake of not going over this again in the
future, can we please have the issue with patch 3 mentioned in a reply
to the patch itself?

Or else it seems weird that a patch with a RB and a RAB didn't go in
for no apparent reason.

Thanks, Roger.

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Jan Beulich]

On 17.03.2021 15:46, Ian Jackson wrote:
> Jan, what is your summary opinion about patch 3 ?

Just to answer this question explicitly: I can't form one yet
without further information provided to me.

Jan

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Jan Beulich]

On 17.03.2021 14:37, Ian Jackson wrote:
> I have read this thread and with my release manager hat on I feel
> confused and/or ignorant.
> 
> Patch 3/ has a good explanation of what the problem is it is
> addressing and why this is important for 4.15.  But then there is
> Jan's most recent reply starting "I find all of this confusing".  Jan,
> can you please tell me in words of one syllable what the implication
> of that message is ?  In particular is any of what you say a reason
> for me to withhold my release-ack ?

Answering the last question first - I don't think so. Something may
indeed want doing here beyond what we already have, and it may well
be precisely what Andrew is proposing, possibly just with extended
descriptions and/or comments. My confusion about patch 3 is that it
(a) claims behavior in turbostat that I can't locate and (b) implies
(describes) behavior of code that I find entirely unexpected (as in:
not making sense to me).

And I'm sorry: Not all of the words are of one syllable.

Jan

Re: [PATCH 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Andrew Cooper]

On 18/03/2021 09:35, Jan Beulich wrote:
> On 17.03.2021 14:37, Ian Jackson wrote:
>> I have read this thread and with my release manager hat on I feel
>> confused and/or ignorant.
>>
>> Patch 3/ has a good explanation of what the problem is it is
>> addressing and why this is important for 4.15.  But then there is
>> Jan's most recent reply starting "I find all of this confusing".  Jan,
>> can you please tell me in words of one syllable what the implication
>> of that message is ?  In particular is any of what you say a reason
>> for me to withhold my release-ack ?
> Answering the last question first - I don't think so. Something may
> indeed want doing here beyond what we already have, and it may well
> be precisely what Andrew is proposing, possibly just with extended
> descriptions and/or comments. My confusion about patch 3 is that it
> (a) claims behavior in turbostat that I can't locate and (b) implies
> (describes) behavior of code that I find entirely unexpected (as in:
> not making sense to me).

Turbostat was discussed on the LKML thread where KVM fixed the same bug,
IIRC, but I'm struggling to find the reference.

~Andrew

[PATCH 2/3] x86/msr: Forward port XSA-351 changes from 4.14

[Andrew Cooper]

staging was not impacted by XSA-351 at the time of release, due to c/s
322ec7c89f and 84e848fd7a which disallows read access by default.

Forward port the XSA-351 changes to make the code structure consistent between
4.14 and 4.15.

This removes logspew for guests probing for the RAPL interface.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Ian Jackson <iwj@xenproject.org>

Technically this breaks Solaris/turbostat insofar as you can no longer use
msr_relaxed to "fix" the guest.  The subsequent patch will unbreak it
differently.

For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix needing
backporting.
---
 xen/arch/x86/msr.c              | 19 +++++++++++++++++++
 xen/include/asm-x86/msr-index.h | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index c3a988bd11..5927b6811b 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -188,6 +188,13 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
     case MSR_TSX_CTRL:
     case MSR_MCU_OPT_CTRL:
     case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
+    case MSR_RAPL_POWER_UNIT:
+    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
+    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
+    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
+    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
+    case MSR_PLATFORM_ENERGY_COUNTER:
+    case MSR_PLATFORM_POWER_LIMIT:
     case MSR_U_CET:
     case MSR_S_CET:
     case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
@@ -195,6 +202,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
     case MSR_AMD64_LWP_CBADDR:
     case MSR_PPIN_CTL:
     case MSR_PPIN:
+    case MSR_F15H_CU_POWER ... MSR_F15H_CU_MAX_POWER:
+    case MSR_AMD_RAPL_POWER_UNIT ... MSR_AMD_PKG_ENERGY_STATUS:
     case MSR_AMD_PPIN_CTL:
     case MSR_AMD_PPIN:
         goto gp_fault;
@@ -412,6 +421,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     case MSR_INTEL_CORE_THREAD_COUNT:
     case MSR_INTEL_PLATFORM_INFO:
     case MSR_ARCH_CAPABILITIES:
+    case MSR_IA32_PERF_STATUS:
 
         /* Not offered to guests. */
     case MSR_TEST_CTRL:
@@ -419,6 +429,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     case MSR_TSX_CTRL:
     case MSR_MCU_OPT_CTRL:
     case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
+    case MSR_RAPL_POWER_UNIT:
+    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
+    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
+    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
+    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
+    case MSR_PLATFORM_ENERGY_COUNTER:
+    case MSR_PLATFORM_POWER_LIMIT:
     case MSR_U_CET:
     case MSR_S_CET:
     case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
@@ -426,6 +443,8 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     case MSR_AMD64_LWP_CBADDR:
     case MSR_PPIN_CTL:
     case MSR_PPIN:
+    case MSR_F15H_CU_POWER ... MSR_F15H_CU_MAX_POWER:
+    case MSR_AMD_RAPL_POWER_UNIT ... MSR_AMD_PKG_ENERGY_STATUS:
     case MSR_AMD_PPIN_CTL:
     case MSR_AMD_PPIN:
         goto gp_fault;
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index f2e34dd22b..49c1afdd22 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -101,6 +101,38 @@
 #define MSR_RTIT_ADDR_A(n)                 (0x00000580 + (n) * 2)
 #define MSR_RTIT_ADDR_B(n)                 (0x00000581 + (n) * 2)
 
+/*
+ * Intel Runtime Average Power Limiting (RAPL) interface.  Power plane base
+ * addresses (MSR_*_POWER_LIMIT) are model specific, but have so-far been
+ * consistent since their introduction in SandyBridge.
+ *
+ * Offsets of functionality from the power plane base is architectural, but
+ * not all power planes support all functionality.
+ */
+#define MSR_RAPL_POWER_UNIT                 0x00000606
+
+#define MSR_PKG_POWER_LIMIT                 0x00000610
+#define MSR_PKG_ENERGY_STATUS               0x00000611
+#define MSR_PKG_PERF_STATUS                 0x00000613
+#define MSR_PKG_POWER_INFO                  0x00000614
+
+#define MSR_DRAM_POWER_LIMIT                0x00000618
+#define MSR_DRAM_ENERGY_STATUS              0x00000619
+#define MSR_DRAM_PERF_STATUS                0x0000061b
+#define MSR_DRAM_POWER_INFO                 0x0000061c
+
+#define MSR_PP0_POWER_LIMIT                 0x00000638
+#define MSR_PP0_ENERGY_STATUS               0x00000639
+#define MSR_PP0_POLICY                      0x0000063a
+
+#define MSR_PP1_POWER_LIMIT                 0x00000640
+#define MSR_PP1_ENERGY_STATUS               0x00000641
+#define MSR_PP1_POLICY                      0x00000642
+
+/* Intel Platform-wide power interface. */
+#define MSR_PLATFORM_ENERGY_COUNTER         0x0000064d
+#define MSR_PLATFORM_POWER_LIMIT            0x0000065c
+
 #define MSR_U_CET                           0x000006a0
 #define MSR_S_CET                           0x000006a2
 #define  CET_SHSTK_EN                       (_AC(1, ULL) <<  0)
@@ -116,10 +148,17 @@
 #define  PASID_PASID_MASK                   0x000fffff
 #define  PASID_VALID                        (_AC(1, ULL) << 31)
 
+#define MSR_F15H_CU_POWER                   0xc001007a
+#define MSR_F15H_CU_MAX_POWER               0xc001007b
+
 #define MSR_K8_VM_CR                        0xc0010114
 #define  VM_CR_INIT_REDIRECTION             (_AC(1, ULL) <<  1)
 #define  VM_CR_SVM_DISABLE                  (_AC(1, ULL) <<  4)
 
+#define MSR_AMD_RAPL_POWER_UNIT             0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS          0xc001029a
+#define MSR_AMD_PKG_ENERGY_STATUS           0xc001029b
+
 /*
  * Legacy MSR constants in need of cleanup.  No new MSRs below this comment.
  */
-- 
2.11.0

Re: [PATCH 2/3] x86/msr: Forward port XSA-351 changes from 4.14

[Roger Pau Monné]

On Tue, Mar 16, 2021 at 04:18:43PM +0000, Andrew Cooper wrote:
> staging was not impacted by XSA-351 at the time of release, due to c/s
> 322ec7c89f and 84e848fd7a which disallows read access by default.
> 
> Forward port the XSA-351 changes to make the code structure consistent between
> 4.14 and 4.15.
> 
> This removes logspew for guests probing for the RAPL interface.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Ian Jackson <iwj@xenproject.org>
> 
> Technically this breaks Solaris/turbostat insofar as you can no longer use
> msr_relaxed to "fix" the guest.  The subsequent patch will unbreak it
> differently.
> 
> For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix needing
> backporting.
> ---
>  xen/arch/x86/msr.c              | 19 +++++++++++++++++++
>  xen/include/asm-x86/msr-index.h | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index c3a988bd11..5927b6811b 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -188,6 +188,13 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>      case MSR_TSX_CTRL:
>      case MSR_MCU_OPT_CTRL:
>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> +    case MSR_RAPL_POWER_UNIT:
> +    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
> +    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
> +    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
> +    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
> +    case MSR_PLATFORM_ENERGY_COUNTER:
> +    case MSR_PLATFORM_POWER_LIMIT:
>      case MSR_U_CET:
>      case MSR_S_CET:
>      case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
> @@ -195,6 +202,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>      case MSR_AMD64_LWP_CBADDR:
>      case MSR_PPIN_CTL:
>      case MSR_PPIN:
> +    case MSR_F15H_CU_POWER ... MSR_F15H_CU_MAX_POWER:
> +    case MSR_AMD_RAPL_POWER_UNIT ... MSR_AMD_PKG_ENERGY_STATUS:
>      case MSR_AMD_PPIN_CTL:
>      case MSR_AMD_PPIN:
>          goto gp_fault;
> @@ -412,6 +421,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>      case MSR_INTEL_CORE_THREAD_COUNT:
>      case MSR_INTEL_PLATFORM_INFO:
>      case MSR_ARCH_CAPABILITIES:
> +    case MSR_IA32_PERF_STATUS:

Should the MSR_IA32_PERF_STATUS addition maybe be part of the previous
commit, as it's not related to the XSA-351 content?

The rest LGTM:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I wonder whether we could squash this with 3/3 for staging commit, and
then only backport 3/3 for older branches. But it's likely too much
work just to prevent breaking msr_relaxed for Solaris for a single
commit time span.

Thanks, Roger.

Re: [PATCH 2/3] x86/msr: Forward port XSA-351 changes from 4.14

[Andrew Cooper]

On 17/03/2021 08:52, Roger Pau Monné wrote:
> On Tue, Mar 16, 2021 at 04:18:43PM +0000, Andrew Cooper wrote:
>> staging was not impacted by XSA-351 at the time of release, due to c/s
>> 322ec7c89f and 84e848fd7a which disallows read access by default.
>>
>> Forward port the XSA-351 changes to make the code structure consistent between
>> 4.14 and 4.15.
>>
>> This removes logspew for guests probing for the RAPL interface.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Ian Jackson <iwj@xenproject.org>
>>
>> Technically this breaks Solaris/turbostat insofar as you can no longer use
>> msr_relaxed to "fix" the guest.  The subsequent patch will unbreak it
>> differently.
>>
>> For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix needing
>> backporting.
>> ---
>>  xen/arch/x86/msr.c              | 19 +++++++++++++++++++
>>  xen/include/asm-x86/msr-index.h | 39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>> index c3a988bd11..5927b6811b 100644
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -188,6 +188,13 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>      case MSR_TSX_CTRL:
>>      case MSR_MCU_OPT_CTRL:
>>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
>> +    case MSR_RAPL_POWER_UNIT:
>> +    case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
>> +    case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
>> +    case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
>> +    case MSR_PP1_POWER_LIMIT  ... MSR_PP1_POLICY:
>> +    case MSR_PLATFORM_ENERGY_COUNTER:
>> +    case MSR_PLATFORM_POWER_LIMIT:
>>      case MSR_U_CET:
>>      case MSR_S_CET:
>>      case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
>> @@ -195,6 +202,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>      case MSR_AMD64_LWP_CBADDR:
>>      case MSR_PPIN_CTL:
>>      case MSR_PPIN:
>> +    case MSR_F15H_CU_POWER ... MSR_F15H_CU_MAX_POWER:
>> +    case MSR_AMD_RAPL_POWER_UNIT ... MSR_AMD_PKG_ENERGY_STATUS:
>>      case MSR_AMD_PPIN_CTL:
>>      case MSR_AMD_PPIN:
>>          goto gp_fault;
>> @@ -412,6 +421,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>>      case MSR_INTEL_CORE_THREAD_COUNT:
>>      case MSR_INTEL_PLATFORM_INFO:
>>      case MSR_ARCH_CAPABILITIES:
>> +    case MSR_IA32_PERF_STATUS:
> Should the MSR_IA32_PERF_STATUS addition maybe be part of the previous
> commit, as it's not related to the XSA-351 content?

It is XSA-351.  We sent out two patches in the end.

c/s 3059178798a (the PERF_STATUS/CTL fix in 4.15) was backported to 4.14
as one half of XSA-351, and gained the above hunk because it went
backwards over the #GP-default change.

In light of patch 1, it now needs reintroducing.

It doesn't really matter if this hunk is in patch 1 or 2, but it needs
to be present, and fits better here IMO.

>
> The rest LGTM:
>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> I wonder whether we could squash this with 3/3 for staging commit, and
> then only backport 3/3 for older branches. But it's likely too much
> work just to prevent breaking msr_relaxed for Solaris for a single
> commit time span.

I did consider the bisectability, but the reality is that it has only
been a week or so with msr_relaxed working at all.

Splitting them apart is simpler to review.

~Andrew

[PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Ian Jackson <iwj@xenproject.org>

For 4.15 This wants backporting to all security trees, as it is a fix to a
regression introduced in XSA-351.

Also it means that users don't need msr_relaxed=1 to unbreak Solaris guests,
which is a strict useability improvement.
---
 xen/arch/x86/msr.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 5927b6811b..a83a1d7fba 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -188,7 +188,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
     case MSR_TSX_CTRL:
     case MSR_MCU_OPT_CTRL:
     case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
-    case MSR_RAPL_POWER_UNIT:
     case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
     case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
     case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
@@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
             goto gp_fault;
         break;
 
+    case MSR_RAPL_POWER_UNIT:
+        /*
+         * This MSR is non-architectural.  However, some versions of Solaris
+         * blindly reads it without a #GP guard, and some versions of
+         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
+         * fail.  Read as zero on Intel hardware.
+         */
+        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
+            goto gp_fault;
+        *val = 0;
+        break;
+
         /*
          * These MSRs are not enumerated in CPUID.  They have been around
          * since the Pentium 4, and implemented by other vendors.
-- 
2.11.0

Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Roger Pau Monné]

On Tue, Mar 16, 2021 at 04:18:44PM +0000, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks!

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Ian Jackson <iwj@xenproject.org>
> 
> For 4.15 This wants backporting to all security trees, as it is a fix to a
> regression introduced in XSA-351.
> 
> Also it means that users don't need msr_relaxed=1 to unbreak Solaris guests,
> which is a strict useability improvement.
> ---
>  xen/arch/x86/msr.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 5927b6811b..a83a1d7fba 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -188,7 +188,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>      case MSR_TSX_CTRL:
>      case MSR_MCU_OPT_CTRL:
>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> -    case MSR_RAPL_POWER_UNIT:
>      case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
>      case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
>      case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>              goto gp_fault;
>          break;
>  
> +    case MSR_RAPL_POWER_UNIT:
> +        /*
> +         * This MSR is non-architectural.  However, some versions of Solaris
> +         * blindly reads it without a #GP guard, and some versions of
> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
> +         * fail.  Read as zero on Intel hardware.
> +         */
> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
> +            goto gp_fault;

AFAICT from Linux usage this is Intel specific (not present in any of
the clones).

> +        *val = 0;
> +        break;

Do we also need to care about MSR_AMD_RAPL_POWER_UNIT (0xc0010299) for
Solaris?

Thanks, Roger.

Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Boris Ostrovsky]

On 3/16/21 12:56 PM, Roger Pau Monné wrote:
>
> Do we also need to care about MSR_AMD_RAPL_POWER_UNIT (0xc0010299) for
> Solaris?


I can't test it but I don't believe Solaris accesses this register.


-boris

Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Andrew Cooper]

On 16/03/2021 16:56, Roger Pau Monné wrote:
> On Tue, Mar 16, 2021 at 04:18:44PM +0000, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks!
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Ian Jackson <iwj@xenproject.org>
>>
>> For 4.15 This wants backporting to all security trees, as it is a fix to a
>> regression introduced in XSA-351.
>>
>> Also it means that users don't need msr_relaxed=1 to unbreak Solaris guests,
>> which is a strict useability improvement.
>> ---
>>  xen/arch/x86/msr.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>> index 5927b6811b..a83a1d7fba 100644
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -188,7 +188,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>      case MSR_TSX_CTRL:
>>      case MSR_MCU_OPT_CTRL:
>>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
>> -    case MSR_RAPL_POWER_UNIT:
>>      case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
>>      case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
>>      case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
>> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>              goto gp_fault;
>>          break;
>>  
>> +    case MSR_RAPL_POWER_UNIT:
>> +        /*
>> +         * This MSR is non-architectural.  However, some versions of Solaris
>> +         * blindly reads it without a #GP guard, and some versions of
>> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
>> +         * fail.  Read as zero on Intel hardware.
>> +         */
>> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
>> +            goto gp_fault;
> AFAICT from Linux usage this is Intel specific (not present in any of
> the clones).

Indeed.

>
>> +        *val = 0;
>> +        break;
> Do we also need to care about MSR_AMD_RAPL_POWER_UNIT (0xc0010299) for
> Solaris?

AMD has a CPUID bit for this interface, 0x80000007.EDX[14].

~Andrew

Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Roger Pau Monné]

On Tue, Mar 16, 2021 at 09:20:10PM +0000, Andrew Cooper wrote:
> On 16/03/2021 16:56, Roger Pau Monné wrote:
> > On Tue, Mar 16, 2021 at 04:18:44PM +0000, Andrew Cooper wrote:
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Thanks!
> >
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Wei Liu <wl@xen.org>
> >> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> CC: Ian Jackson <iwj@xenproject.org>
> >>
> >> For 4.15 This wants backporting to all security trees, as it is a fix to a
> >> regression introduced in XSA-351.
> >>
> >> Also it means that users don't need msr_relaxed=1 to unbreak Solaris guests,
> >> which is a strict useability improvement.
> >> ---
> >>  xen/arch/x86/msr.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> >> index 5927b6811b..a83a1d7fba 100644
> >> --- a/xen/arch/x86/msr.c
> >> +++ b/xen/arch/x86/msr.c
> >> @@ -188,7 +188,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
> >>      case MSR_TSX_CTRL:
> >>      case MSR_MCU_OPT_CTRL:
> >>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
> >> -    case MSR_RAPL_POWER_UNIT:
> >>      case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
> >>      case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
> >>      case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
> >> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
> >>              goto gp_fault;
> >>          break;
> >>  
> >> +    case MSR_RAPL_POWER_UNIT:
> >> +        /*
> >> +         * This MSR is non-architectural.  However, some versions of Solaris
> >> +         * blindly reads it without a #GP guard, and some versions of
> >> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
> >> +         * fail.  Read as zero on Intel hardware.
> >> +         */
> >> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
> >> +            goto gp_fault;
> > AFAICT from Linux usage this is Intel specific (not present in any of
> > the clones).
> 
> Indeed.
> 
> >
> >> +        *val = 0;
> >> +        break;
> > Do we also need to care about MSR_AMD_RAPL_POWER_UNIT (0xc0010299) for
> > Solaris?
> 
> AMD has a CPUID bit for this interface, 0x80000007.EDX[14].

Right, I see now on the manual. I guess I was confused because I don't
seem to see Linux checking this CPUID bit in:

https://elixir.bootlin.com/linux/latest/source/arch/x86/events/rapl.c#L773

And instead it seems to attach the RAPL driver based on CPU model
information. That's fine on Linux because accesses are using the _safe
variants.

The patch looks good to me, I wonder whether you should move the
"users don't need msr_relaxed=1..." to the commit log, but that might
be weird if the patch is backported, because it won't make sense for
any older Xen version.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Andrew Cooper]

On 17/03/2021 08:32, Roger Pau Monné wrote:
> On Tue, Mar 16, 2021 at 09:20:10PM +0000, Andrew Cooper wrote:
>> On 16/03/2021 16:56, Roger Pau Monné wrote:
>>> On Tue, Mar 16, 2021 at 04:18:44PM +0000, Andrew Cooper wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Thanks!
>>>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Wei Liu <wl@xen.org>
>>>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> CC: Ian Jackson <iwj@xenproject.org>
>>>>
>>>> For 4.15 This wants backporting to all security trees, as it is a fix to a
>>>> regression introduced in XSA-351.
>>>>
>>>> Also it means that users don't need msr_relaxed=1 to unbreak Solaris guests,
>>>> which is a strict useability improvement.
>>>> ---
>>>>  xen/arch/x86/msr.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
>>>> index 5927b6811b..a83a1d7fba 100644
>>>> --- a/xen/arch/x86/msr.c
>>>> +++ b/xen/arch/x86/msr.c
>>>> @@ -188,7 +188,6 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>      case MSR_TSX_CTRL:
>>>>      case MSR_MCU_OPT_CTRL:
>>>>      case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
>>>> -    case MSR_RAPL_POWER_UNIT:
>>>>      case MSR_PKG_POWER_LIMIT  ... MSR_PKG_POWER_INFO:
>>>>      case MSR_DRAM_POWER_LIMIT ... MSR_DRAM_POWER_INFO:
>>>>      case MSR_PP0_POWER_LIMIT  ... MSR_PP0_POLICY:
>>>> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>>>              goto gp_fault;
>>>>          break;
>>>>  
>>>> +    case MSR_RAPL_POWER_UNIT:
>>>> +        /*
>>>> +         * This MSR is non-architectural.  However, some versions of Solaris
>>>> +         * blindly reads it without a #GP guard, and some versions of
>>>> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
>>>> +         * fail.  Read as zero on Intel hardware.
>>>> +         */
>>>> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
>>>> +            goto gp_fault;
>>> AFAICT from Linux usage this is Intel specific (not present in any of
>>> the clones).
>> Indeed.
>>
>>>> +        *val = 0;
>>>> +        break;
>>> Do we also need to care about MSR_AMD_RAPL_POWER_UNIT (0xc0010299) for
>>> Solaris?
>> AMD has a CPUID bit for this interface, 0x80000007.EDX[14].
> Right, I see now on the manual. I guess I was confused because I don't
> seem to see Linux checking this CPUID bit in:
>
> https://elixir.bootlin.com/linux/latest/source/arch/x86/events/rapl.c#L773
>
> And instead it seems to attach the RAPL driver based on CPU model
> information. That's fine on Linux because accesses are using the _safe
> variants.

Borislav also wants a bugfix for that, seeing as there is a CPUID bit.

>
> The patch looks good to me, I wonder whether you should move the
> "users don't need msr_relaxed=1..." to the commit log, but that might
> be weird if the patch is backported, because it won't make sense for
> any older Xen version.

Will do.

> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

~Andrew

Re: [PATCH 3/3] x86/msr: Fix Solaris and turbostat following XSA-351

[Jan Beulich]

On 16.03.2021 17:18, Andrew Cooper wrote:
> @@ -284,6 +283,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>              goto gp_fault;
>          break;
>  
> +    case MSR_RAPL_POWER_UNIT:
> +        /*
> +         * This MSR is non-architectural.  However, some versions of Solaris
> +         * blindly reads it without a #GP guard, and some versions of
> +         * turbostat crash after expecting a read of /proc/cpu/0/msr not to
> +         * fail.  Read as zero on Intel hardware.
> +         */
> +        if ( !(cp->x86_vendor & X86_VENDOR_INTEL) )
> +            goto gp_fault;
> +        *val = 0;
> +        break;
> +
>          /*
>           * These MSRs are not enumerated in CPUID.  They have been around
>           * since the Pentium 4, and implemented by other vendors.
> 

I find all of this confusing, at best: I'd expect any entity reading
this MSR to - when successful - go on and read further MSRs. I have a
hard time seeing why those subsequent reads would be any less prone
to being unguarded. In fact I went and looked at the turbostat
sources. From its introduction the read of this MSR was done with -
afaict - appropriate error handling. There are anomalies (do_rapl
getting set prior to the probing of this MSR), but they look to not
matter stability-wise as the respective MSR reads are similarly
guarded. Were the observed crashes perhaps just in some private
versions of the tool? If so, I guess saying so in the comment (or
description) would be appropriate, but this still wouldn't invalidate
the general aspect of my remark.

On the good side the value of zero isn't entirely invalid, at least
as far as defined bitfields of the MSR go.

While looking around I also came across MSR_PLATFORM_ENERGY_COUNTER.
This one has specific precautions in the SDM: "This MSR is valid only
if both platform vendor hardware implementation and BIOS enablement
support it. This MSR will read 0 if not valid." Isn't this a fairly
strong suggestion that instead of raising #GP for it, we'd better
return zero as well? Because of the remark, unlike for certain other
MSRs, consumers have to expect zero possibly coming back.

Jan

[PATCH for-4.15 v1.1 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Jan Beulich]

From: Andrew Cooper <andrew.cooper3@citrix.com>

In hindsight, this was a poor move.  Some of these MSRs require probing for,
cause unhelpful spew into xl dmesg, or cause spew from unit tests explicitly
checking behaviour.

This restores behaviour close to that of Xen 4.14, meaning in particular
that for all of the MSRs getting re-added explicitly a #GP fault will get
raised irrespective of the new "msr_relaxed" setting.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
---
v1.1: Fold in suggested description adjustments.
---
For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix needing
backporting.
---
 xen/arch/x86/msr.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 0ebcb04259..c3a988bd11 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -175,6 +175,30 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
 
     switch ( msr )
     {
+        /* Write-only */
+    case MSR_AMD_PATCHLOADER:
+    case MSR_IA32_UCODE_WRITE:
+    case MSR_PRED_CMD:
+    case MSR_FLUSH_CMD:
+
+        /* Not offered to guests. */
+    case MSR_TEST_CTRL:
+    case MSR_CORE_CAPABILITIES:
+    case MSR_TSX_FORCE_ABORT:
+    case MSR_TSX_CTRL:
+    case MSR_MCU_OPT_CTRL:
+    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
+    case MSR_U_CET:
+    case MSR_S_CET:
+    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
+    case MSR_AMD64_LWP_CFG:
+    case MSR_AMD64_LWP_CBADDR:
+    case MSR_PPIN_CTL:
+    case MSR_PPIN:
+    case MSR_AMD_PPIN_CTL:
+    case MSR_AMD_PPIN:
+        goto gp_fault;
+
     case MSR_IA32_FEATURE_CONTROL:
         /*
          * Architecturally, availability of this MSR is enumerated by the
@@ -382,6 +406,30 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     {
         uint64_t rsvd;
 
+        /* Read-only */
+    case MSR_IA32_PLATFORM_ID:
+    case MSR_CORE_CAPABILITIES:
+    case MSR_INTEL_CORE_THREAD_COUNT:
+    case MSR_INTEL_PLATFORM_INFO:
+    case MSR_ARCH_CAPABILITIES:
+
+        /* Not offered to guests. */
+    case MSR_TEST_CTRL:
+    case MSR_TSX_FORCE_ABORT:
+    case MSR_TSX_CTRL:
+    case MSR_MCU_OPT_CTRL:
+    case MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7):
+    case MSR_U_CET:
+    case MSR_S_CET:
+    case MSR_PL0_SSP ... MSR_INTERRUPT_SSP_TABLE:
+    case MSR_AMD64_LWP_CFG:
+    case MSR_AMD64_LWP_CBADDR:
+    case MSR_PPIN_CTL:
+    case MSR_PPIN:
+    case MSR_AMD_PPIN_CTL:
+    case MSR_AMD_PPIN:
+        goto gp_fault;
+
     case MSR_AMD_PATCHLEVEL:
         BUILD_BUG_ON(MSR_IA32_UCODE_REV != MSR_AMD_PATCHLEVEL);
         /*

Re: [PATCH for-4.15 v1.1 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()"

[Ian Jackson]

Jan Beulich writes ("[PATCH for-4.15 v1.1 1/3] Revert "x86/msr: drop compatibility #GP handling in guest_{rd,wr}msr()""):
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> In hindsight, this was a poor move.  Some of these MSRs require probing for,
> cause unhelpful spew into xl dmesg, or cause spew from unit tests explicitly
> checking behaviour.
> 
> This restores behaviour close to that of Xen 4.14, meaning in particular
> that for all of the MSRs getting re-added explicitly a #GP fault will get
> raised irrespective of the new "msr_relaxed" setting.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>
> ---
> v1.1: Fold in suggested description adjustments.
> ---
> For 4.15.  Restoring behaviour closer to 4.14, and prereq for a bugfix needing
> backporting.

Thank you, and thanks for the helpful irc discussion.

Acked-by: Ian Jackson <iwj@xenproject.org>

I will commit this shortly.

For the record, we concluded on irc that we weren't sure that 2/ and
3/ of this series were right, so we are not going to include them in
4.15, pending further consideration/investigation.

Ian.