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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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