Re: [PATCH v5 8/8] microcode: update microcode on cores in parallel

From: Chao Gao <chao.gao@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v5 8/8] microcode: update microcode on cores in parallel
Date: Wed, 13 Feb 2019 16:50:03 +0800	[thread overview]
Message-ID: <20190213085000.GA9109@gao-cwp> (raw)
In-Reply-To: <5C63C55302000078002162C4@prv1-mh.provo.novell.com>

On Wed, Feb 13, 2019 at 12:20:51AM -0700, Jan Beulich wrote:
>>>> On 13.02.19 at 03:30, <chao.gao@intel.com> wrote:
>> On Tue, Feb 12, 2019 at 06:55:20AM -0700, Jan Beulich wrote:
>>>>>> On 12.02.19 at 14:25, <roger.pau@citrix.com> wrote:
>>>> On Tue, Feb 12, 2019 at 05:51:41AM -0700, Jan Beulich wrote:
>>>>> >>> On 28.01.19 at 08:06, <chao.gao@intel.com> wrote:
>>>>> > @@ -314,9 +310,7 @@ static int apply_microcode(unsigned int cpu)
>>>>> >  
>>>>> >      mc_intel = patch->data;
>>>>> >      BUG_ON(!mc_intel);
>>>>> > -
>>>>> > -    /* serialize access to the physical write to MSR 0x79 */
>>>>> > -    spin_lock_irqsave(&microcode_update_lock, flags);
>>>>> > +    BUG_ON(local_irq_is_enabled());
>>>>> >  
>>>>> >      /* write microcode via MSR 0x79 */
>>>>> >      wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
>>>>> > @@ -329,7 +323,6 @@ static int apply_microcode(unsigned int cpu)
>>>>> >      rdmsrl(MSR_IA32_UCODE_REV, msr_content);
>>>>> >      val[1] = (uint32_t)(msr_content >> 32);
>>>>> >  
>>>>> > -    spin_unlock_irqrestore(&microcode_update_lock, flags);
>>>>> >      if ( val[1] != mc_intel->hdr.rev )
>>>>> >      {
>>>>> >          printk(KERN_ERR "microcode: CPU%d update from revision "
>>>>> 
>>>>> Am I understanding right that you now rely on upper layers in the
>>>>> call tree to avoid calling into here in parallel for two hyperthreads
>>>>> of the same core? I can't see how you avoid this situation during
>>>>> AP bringup, for example. Did I overlook anything in this regard?
>>>> 
>>>> IIRC microcode update is done in the serialized part of AP bringup,
>>>> before the call to smp_callin, which guarantees serialization.
>>>
>>>Hmm, yes, right now it is. But I'd call this "happens to be that way"
>>>rather than "is guaranteed to be that way" - prior to commit
>>>f97838bbd9 it did happen later.
>> 
>> How about employing another lock, "early_ucode_update_lock", to
>> guarantee serialization.
>> 
>> In particular, early_microcode_update_cpu() and microcode_resume_cpu()
>> will acquire this lock before ucode update.
>
>That's a (temporary) option, but would over-serialize things again.
>I was rather trying to hint towards a per-core lock.

To implement a per-core lock, a core should be conscious of its siblings.
Unfortunately, during AP bringup, ucode update is done prior to the
initialization of 'cpu_sibling_mask'.

Thanks
Chao

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