Re: [Xen-devel] [PATCH v10 14/16] microcode: rendezvous CPUs in NMI handler and load ucode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.09.2019 05:18, Chao Gao wrote:
> On Fri, Sep 13, 2019 at 11:14:59AM +0200, Jan Beulich wrote:
>> On 12.09.2019 09:22, Chao Gao wrote:
>>> @@ -354,6 +360,50 @@ static void set_state(unsigned int state)
>>>      smp_wmb();
>>>  }
>>>  
>>> +static int secondary_thread_work(void)
>>> +{
>>> +    cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>>> +
>>> +    return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
>>> +}
>>> +
>>> +static int primary_thread_work(const struct microcode_patch *patch)
>>
>> I think it would be nice if both functions carried "nmi" in their
>> names - how about {primary,secondary}_nmi_work()? Or wait - the
>> primary one gets used outside of NMI as well, so I'm fine with its
>> name.
>> The secondary one, otoh, is NMI-specific and also its only
>> caller doesn't care about the return value, so I'd suggest making
>> it return void alongside adding some form of "nmi" to its name. Or,
> 
> Will do.
> 
>> perhaps even better, have secondary_thread_fn() call it, moving the
>> cpu_sig update here (and of course then there shouldn't be any
>> "nmi" added to its name).
> 
> Even with "ucode=no-nmi", secondary threads have to do busy-loop in
> NMI handling util primary threads completing the update. Otherwise,
> it may access MSRs (like SPEC_CTRL) which is considered unsafe.

Of course. Note that I said "call it"; I did not suggest to replace
secondary_thread_fn().

>>> +static int microcode_nmi_callback(const struct cpu_user_regs *regs, int 
>>> cpu)
>>> +{
>>> +    unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
>>> +    unsigned int controller = cpumask_first(&cpu_online_map);
>>> +
>>> +    /* System-generated NMI, will be ignored */
>>> +    if ( loading_state != LOADING_CALLIN )
>>> +        return 0;
>>
>> I'm not happy at all to see NMIs being ignored. But by returning
>> zero, you do _not_ ignore it. Did you perhaps mean "will be ignored
>> here", in which case perhaps better "leave to main handler"? And
>> for the comment to extend to the other two conditions right below,
>> I think it would be better to combine them all into a single if().
>>
>> Also, throughout the series, I think you want to consistently use
>> ACCESS_ONCE() for reads/writes from/to loading_state.
>>
>>> +    if ( cpu == controller || (!opt_ucode_loading_in_nmi && cpu == 
>>> primary) )
>>> +        return 0;
>>
>> Why not
> 
> As I said above, secondary threads are expected to stay in NMI handler
> regardless the setting of opt_ucode_loading_in_nmi.

Oh, here I see how your remark above matters. Please add code
comments then to make this clear to the reader.

>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -126,6 +126,8 @@ boolean_param("ler", opt_ler);
>>>  /* LastExceptionFromIP on this hardware.  Zero if LER is not in use. */
>>>  unsigned int __read_mostly ler_msr;
>>>  
>>> +unsigned int __read_mostly nmi_cpu;
>>
>> Since this variable (for now) is never written to it should gain a
>> comment saying why this is, and perhaps it would then also better be
>> const rather than __read_mostly.
> 
> How about use the macro below:
> #define NMI_CPU 0

This is another option, yes. If there's any intention to ever allow
offlining CPU 0, then having the variable in place would seem better
to me. But I'll leave it to you at this point.

Jan

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