Re: [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode

From: Chao Gao <chao.gao@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Ashok Raj" <ashok.raj@intel.com>, "Wei Liu" <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode
Date: Wed, 7 Aug 2019 15:59:58 +0800	[thread overview]
Message-ID: <20190807075957.GA23889@gao-cwp> (raw)
In-Reply-To: <8f6d506f-ee05-ab43-ef5f-8301f3845d4d@suse.com>

On Mon, Aug 05, 2019 at 12:11:01PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> @@ -439,12 +440,37 @@ static int do_microcode_update(void *patch)
>>       return ret;
>>   }
>>   
>> +static int microcode_nmi_callback(const struct cpu_user_regs *regs, int cpu)
>> +{
>> +    bool print = false;
>> +
>> +    /* The first thread of a core is to load an update. Don't block it. */
>> +    if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
>> +        return 0;
>> +
>> +    if ( loading_state == LOADING_ENTERED )
>> +    {
>> +        cpumask_set_cpu(cpu, &cpu_callin_map);
>> +        printk(XENLOG_DEBUG "CPU%u enters %s\n", smp_processor_id(), __func__);
>
>Here  and ...
>
>> +        print = true;
>> +    }
>> +
>> +    while ( loading_state == LOADING_ENTERED )
>> +        rep_nop();
>> +
>> +    if ( print )
>> +        printk(XENLOG_DEBUG "CPU%u exits %s\n", smp_processor_id(), __func__);
>
>... here - why smp_processor_id() when you can use "cpu"? And what
>use is __func__ here?
>
>The rep_nop() above also presumably wants to be cpu_relax() again.
>
>But on the whole I was really hoping for more aggressive disabling
>of NMI handling, more like (but of course not quite as heavy as)
>the crash path wiring the IDT entry to trap_nop().

Hi Jan,

I agree with you that it should be more aggressive. This patch is
problematic considering there is a lot of code before reaching this
callback (especially, SPEC_CTRL_ENTRY_FROM_INTR_IST which may write
MSR_SPEC_CTRL).

In my mind, we have two options to solve the issue:
1. Wire the IDT entry to trap_nop() like the crash path.

2. Enhance this patch: A thread which is not going to load an update
is forced to send an #NMI to itself to enter the callback, do
busy-loop until completion of loading ucode on all cores.
With this method, no #NMI delivery, or MSR write would happen on this
threads during microcode update.

Thanks
Chao

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