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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/04/18 16:43, Jan Beulich wrote:
>>>> On 18.04.18 at 12:43, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel