[PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Andrew Cooper]

This avoids opencoding the functionality (and missing one bit of it), and and
some comments explaining what is going on.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Juergen Gross <jgross@suse.com>

This is effectively a bugfix of c/s 710a8eb "x86: suppress BTI mitigations
around S3 suspend/resume" so should be considered for 4.11 at this point.
---
 xen/arch/x86/acpi/power.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 0763846..bb0d095 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -213,7 +213,8 @@ static int enter_state(u32 state)
         error = 0;
 
     ci = get_cpu_info();
-    ci->use_shadow_spec_ctrl = 0;
+    spec_ctrl_enter_idle(ci);
+    /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */
     ci->bti_ist_info = 0;
 
     ACPI_FLUSH_CPU_CACHE();
@@ -257,10 +258,9 @@ static int enter_state(u32 state)
     if ( !recheck_cpu_features(0) )
         panic("Missing previously available feature(s).");
 
+    /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */
     ci->bti_ist_info = default_bti_ist_info;
-    asm volatile (ALTERNATIVE("", "wrmsr", X86_FEATURE_XEN_IBRS_SET)
-                  :: "a" (SPEC_CTRL_IBRS), "c" (MSR_SPEC_CTRL), "d" (0)
-                  : "memory");
+    spec_ctrl_exit_idle(ci);
 
  done:
     spin_debug_enable();
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Jan Beulich]

>>> On 18.04.18 at 12:43, <andrew.cooper3@citrix.com> wrote:
> This avoids opencoding the functionality (and missing one bit of it), and and
> some comments explaining what is going on.

Missing which bit of it? The MSR writes aren't strictly necessary afaict, and
functionally clearing bti_ist_info is all that's needed for the entry path of
interest, while clearing use_shadow_spec_ctrl is all that's needed for the
exit-to-Xen path. Hence I'm don't see (yet) what bug it is you think this
fixes.

Also s/and$/add/.

> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -213,7 +213,8 @@ static int enter_state(u32 state)
>          error = 0;
>  
>      ci = get_cpu_info();
> -    ci->use_shadow_spec_ctrl = 0;
> +    spec_ctrl_enter_idle(ci);
> +    /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */
>      ci->bti_ist_info = 0;
>  
>      ACPI_FLUSH_CPU_CACHE();
> @@ -257,10 +258,9 @@ static int enter_state(u32 state)
>      if ( !recheck_cpu_features(0) )
>          panic("Missing previously available feature(s).");
>  
> +    /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */
>      ci->bti_ist_info = default_bti_ist_info;
> -    asm volatile (ALTERNATIVE("", "wrmsr", X86_FEATURE_XEN_IBRS_SET)
> -                  :: "a" (SPEC_CTRL_IBRS), "c" (MSR_SPEC_CTRL), "d" (0)
> -                  : "memory");
> +    spec_ctrl_exit_idle(ci);

The use of "idle" methods is a slight abuse here (fundamentally "idle" is
about C states), but yes, perhaps in a way a system in S3/S5 can be
viewed sort of (in a heavyweight way) idle.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Andrew Cooper]

On 18/04/18 16:43, Jan Beulich wrote:
>>>> On 18.04.18 at 12:43, <andrew.cooper3@citrix.com> wrote:
>> This avoids opencoding the functionality (and missing one bit of it), and and
>> some comments explaining what is going on.
> Missing which bit of it? The MSR writes aren't strictly necessary afaict, and
> functionally clearing bti_ist_info is all that's needed for the entry path of
> interest, while clearing use_shadow_spec_ctrl is all that's needed for the
> exit-to-Xen path. Hence I'm don't see (yet) what bug it is you think this
> fixes.
>
> Also s/and$/add/.

I hoped noone else would notice.  Already found and fixed.

It is expected that we don't hand off control to the firmware in cases
like this with the mitigations in place.  Here, it doesn't actually
matter too much, but there is no point having the mitigations in effect
when we're the final CPU and trying to shut down.

>
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -213,7 +213,8 @@ static int enter_state(u32 state)
>>          error = 0;
>>  
>>      ci = get_cpu_info();
>> -    ci->use_shadow_spec_ctrl = 0;
>> +    spec_ctrl_enter_idle(ci);
>> +    /* Avoid NMI/#MC using MSR_SPEC_CTRL until we've reloaded microcode. */
>>      ci->bti_ist_info = 0;
>>  
>>      ACPI_FLUSH_CPU_CACHE();
>> @@ -257,10 +258,9 @@ static int enter_state(u32 state)
>>      if ( !recheck_cpu_features(0) )
>>          panic("Missing previously available feature(s).");
>>  
>> +    /* Re-enabled default NMI/#MC use of MSR_SPEC_CTRL. */
>>      ci->bti_ist_info = default_bti_ist_info;
>> -    asm volatile (ALTERNATIVE("", "wrmsr", X86_FEATURE_XEN_IBRS_SET)
>> -                  :: "a" (SPEC_CTRL_IBRS), "c" (MSR_SPEC_CTRL), "d" (0)
>> -                  : "memory");
>> +    spec_ctrl_exit_idle(ci);
> The use of "idle" methods is a slight abuse here (fundamentally "idle" is
> about C states), but yes, perhaps in a way a system in S3/S5 can be
> viewed sort of (in a heavyweight way) idle.

That is a matter of interpretation of "idle" and the use here is fine. 
Either way, the behaviour WRT MSR_SPEC_CTRL is identical whether we are
talking about C states or S states, and the purpose of the change is to
not opencode the recovery logic.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Jan Beulich]

>>> On 18.04.18 at 18:02, <andrew.cooper3@citrix.com> wrote:
> On 18/04/18 16:43, Jan Beulich wrote:
>>>>> On 18.04.18 at 12:43, <andrew.cooper3@citrix.com> wrote:
>>> This avoids opencoding the functionality (and missing one bit of it), and and
>>> some comments explaining what is going on.
>> Missing which bit of it? The MSR writes aren't strictly necessary afaict, and
>> functionally clearing bti_ist_info is all that's needed for the entry path of
>> interest, while clearing use_shadow_spec_ctrl is all that's needed for the
>> exit-to-Xen path. Hence I'm don't see (yet) what bug it is you think this
>> fixes.
> 
> It is expected that we don't hand off control to the firmware in cases
> like this with the mitigations in place.  Here, it doesn't actually
> matter too much, but there is no point having the mitigations in effect
> when we're the final CPU and trying to shut down.

With an improved description the patch can certainly have my R-b; the
code change itself is clearly fine with me.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Andrew Cooper]

On 18/04/18 17:14, Jan Beulich wrote:
>>>> On 18.04.18 at 18:02, <andrew.cooper3@citrix.com> wrote:
>> On 18/04/18 16:43, Jan Beulich wrote:
>>>>>> On 18.04.18 at 12:43, <andrew.cooper3@citrix.com> wrote:
>>>> This avoids opencoding the functionality (and missing one bit of it), and and
>>>> some comments explaining what is going on.
>>> Missing which bit of it? The MSR writes aren't strictly necessary afaict, and
>>> functionally clearing bti_ist_info is all that's needed for the entry path of
>>> interest, while clearing use_shadow_spec_ctrl is all that's needed for the
>>> exit-to-Xen path. Hence I'm don't see (yet) what bug it is you think this
>>> fixes.
>> It is expected that we don't hand off control to the firmware in cases
>> like this with the mitigations in place.  Here, it doesn't actually
>> matter too much, but there is no point having the mitigations in effect
>> when we're the final CPU and trying to shut down.
> With an improved description the patch can certainly have my R-b; the
> code change itself is clearly fine with me.

How about:

The main purpose of this patch is to avoid opencoding the recovery logic
at the end, but also has the positive side effect of relaxing the
SPEC_CTRL mitigations when working to shut the final CPU down.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Juergen Gross]

On 18/04/18 12:43, Andrew Cooper wrote:
> This avoids opencoding the functionality (and missing one bit of it), and and
> some comments explaining what is going on.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH for-4.11] x86: Use spec_ctrl_{enter, exit}_idle() in the S3/S5 path

[Jan Beulich]

>>> On 18.04.18 at 18:27, <andrew.cooper3@citrix.com> wrote:
> On 18/04/18 17:14, Jan Beulich wrote:
>>>>> On 18.04.18 at 18:02, <andrew.cooper3@citrix.com> wrote:
>>> On 18/04/18 16:43, Jan Beulich wrote:
>>>>>>> On 18.04.18 at 12:43, <andrew.cooper3@citrix.com> wrote:
>>>>> This avoids opencoding the functionality (and missing one bit of it), and and
>>>>> some comments explaining what is going on.
>>>> Missing which bit of it? The MSR writes aren't strictly necessary afaict, and
>>>> functionally clearing bti_ist_info is all that's needed for the entry path of
>>>> interest, while clearing use_shadow_spec_ctrl is all that's needed for the
>>>> exit-to-Xen path. Hence I'm don't see (yet) what bug it is you think this
>>>> fixes.
>>> It is expected that we don't hand off control to the firmware in cases
>>> like this with the mitigations in place.  Here, it doesn't actually
>>> matter too much, but there is no point having the mitigations in effect
>>> when we're the final CPU and trying to shut down.
>> With an improved description the patch can certainly have my R-b; the
>> code change itself is clearly fine with me.
> 
> How about:
> 
> The main purpose of this patch is to avoid opencoding the recovery logic
> at the end, but also has the positive side effect of relaxing the
> SPEC_CTRL mitigations when working to shut the final CPU down.

Much better. With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel