Re: [Xen-devel] [PATCH 06/10] x86/spec_ctrl: Split X86_FEATURE_SC_MSR into PV and HVM variants

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/05/18 11:49, Jan Beulich wrote:
>>>> On 16.05.18 at 12:28, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 16/05/18 07:38, Jan Beulich wrote:
>>>>>> On 15.05.18 at 21:52, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 14/05/18 16:27, Jan Beulich wrote:
>>>>>>>> On 11.05.18 at 12:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>> --- a/xen/arch/x86/spec_ctrl.c
>>>>>> +++ b/xen/arch/x86/spec_ctrl.c
>>>>>> @@ -128,7 +128,8 @@ static void __init print_details(enum ind_thunk 
>>>>>> thunk, 
>> uint64_t caps)
>>>>>>             thunk == THUNK_RETPOLINE ? "RETPOLINE" :
>>>>>>             thunk == THUNK_LFENCE    ? "LFENCE" :
>>>>>>             thunk == THUNK_JMP       ? "JMP" : "?",
>>>>>> -           boot_cpu_has(X86_FEATURE_SC_MSR) ?
>>>>>> +           (boot_cpu_has(X86_FEATURE_SC_MSR_PV) ||
>>>>>> +            boot_cpu_has(X86_FEATURE_SC_MSR_HVM)) ?
>>>>>>             default_xen_spec_ctrl & SPEC_CTRL_IBRS    ? " IBRS+" :
>>>>>>                                                         " IBRS-"      : 
>>>>>> "",
>>>>>>             opt_ibpb                                  ? " IBPB"       : 
>>>>>> "",
>>>>>> @@ -367,7 +368,8 @@ void __init init_speculation_mitigations(void)
>>>>>>           * need the IBRS entry/exit logic to virtualise IBRS support for
>>>>>>           * guests.
>>>>>>           */
>>>>>> -        setup_force_cpu_cap(X86_FEATURE_SC_MSR);
>>>>>> +        setup_force_cpu_cap(X86_FEATURE_SC_MSR_PV);
>>>>>> +        setup_force_cpu_cap(X86_FEATURE_SC_MSR_HVM);
>>>>> Besides these sort of open coding alternative_io_2() (you'd really want an
>>>>> output-less variant here, I agree) these are slightly bending the rules of
>>>>> when/how to use multiple alternatives: The above ends up correct only
>>>>> because of both replacements being identical.
>>>> Actually, by reordering patch 10 ahead of this patch, we never get to
>>>> needing the ALTERNATIVE_2()'s in the first place, and lose any concerns
>>>> with bending the rules along the series.
>>> Ah yes, indeed. And you would better use alternative_input() there then,
>>> instead of open coding it.
>> The reason this doesn't use alternative_input() at the moment is because
>> of the memory clobber.  (And the lack of a memory clobber is called out
>> as a peculiarity in comment).  The current code looks dangerously
>> inconsistent WRT barriers.
>>
>> As for bending the rules, I now disagree with your assessment.  The
>> alternative_*() wrappers do nothing but make it harder to express the
>> parameters, as perfectly demonstrated by the ASM_OUTPUT2() bodge.
> The "bending the rules" comment was unrelated to alternative_*() vs
> ALTERNATIVE*() use, and instead was solely related to there being a
> dependency here on both pieces of replacement code being identical.
>
>> I don't see their value, and they have a cost of making an asm volatile
>> statement not look and work quite as an asm volatile statement does in
>> all other callsites.
> I don't mind consistency being achieved to other way around (i.e. by
> dropping those wrappers). But I'd prefer if we didn't mix things unless
> there's a compelling reason to do so.

I'll see about doing some cleanup of the overall tree for 4.12.  For
now, its already mixed, and this doesn't make anything worse.

~Andrew

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