Re: [Xen-devel] [PATCH v9 13/15] x86/microcode: Synchronize late microcode loading

From: Chao Gao <chao.gao@intel.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Kevin Tian" <kevin.tian@intel.com>,
	"Ashok Raj" <ashok.raj@intel.com>, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	xen-devel@lists.xenproject.org,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Borislav Petkov" <bp@suse.de>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v9 13/15] x86/microcode: Synchronize late microcode loading
Date: Fri, 30 Aug 2019 11:30:18 +0800	[thread overview]
Message-ID: <20190830033018.GB31905@gao-cwp> (raw)
In-Reply-To: <2441e448-5fe1-bdbc-f0b6-720401fd0bf0@suse.com>

On Thu, Aug 29, 2019 at 02:06:39PM +0200, Jan Beulich wrote:
>On 19.08.2019 03:25, Chao Gao wrote:
>> +
>> +static int master_thread_fn(const struct microcode_patch *patch)
>> +{
>> +    unsigned int cpu = smp_processor_id();
>> +    int ret = 0;
>> +
>> +    while ( loading_state != LOADING_CALLIN )
>> +        cpu_relax();
>> +
>> +    cpumask_set_cpu(cpu, &cpu_callin_map);
>> +
>> +    while ( loading_state != LOADING_ENTER )
>> +        cpu_relax();
>> +
>> +    /*
>> +     * If an error happened, control thread would set 'loading_state'
>> +     * to LOADING_EXIT. Don't perform ucode loading for this case
>> +     */
>> +    if ( loading_state == LOADING_EXIT )
>> +        return ret;
>
>Even if the producer transitions this through ENTER to EXIT, the
>observer here may never get to see the ENTER state, and hence
>never exit the loop above. You want either < ENTER or == CALLIN.

Yes. I find stopmachine_action() is a good example to implement
a state machine. I will follow it.

>
>> +    ret = microcode_ops->apply_microcode(patch);
>> +    if ( !ret )
>> +        atomic_inc(&cpu_updated);
>> +    atomic_inc(&cpu_out);
>> +
>> +    while ( loading_state != LOADING_EXIT )
>> +        cpu_relax();
>> +
>> +    return ret;
>> +}
>
>As a cosmetic remark, I don't think "master" and "slave" are
>suitable terms here. "primary" and "secondary" would imo come
>closer to what the threads' relationship is.

Will do.

>> +
>> +    /*
>> +     * We intend to disable interrupt for long time, which may lead to
>> +     * watchdog timeout.
>> +     */
>> +    watchdog_disable();
>> +    /*
>> +     * Late loading dance. Why the heavy-handed stop_machine effort?
>> +     *
>> +     * - HT siblings must be idle and not execute other code while the other
>> +     *   sibling is loading microcode in order to avoid any negative
>> +     *   interactions cause by the loading.
>> +     *
>> +     * - In addition, microcode update on the cores must be serialized until
>> +     *   this requirement can be relaxed in the future. Right now, this is
>> +     *   conservative and good.
>> +     */
>> +    ret = stop_machine_run(do_microcode_update, patch, NR_CPUS);
>> +    watchdog_enable();
>
>Considering that stop_machine_run() doesn't itself disable the watchdog,
>did you consider having the control thread disable/enable the watchdog,
>thus shortening the period where it's not active?

Good idea. It helps to keep the code here clean. I think maybe
microcode_nmi_callback can be registered by control thread as well.

Thanks
Chao

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