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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.02.19 at 09:50, <chao.gao@xxxxxxxxx> wrote:
> On Wed, Feb 13, 2019 at 12:20:51AM -0700, Jan Beulich wrote:
>>>>> On 13.02.19 at 03:30, <chao.gao@xxxxxxxxx> wrote:
>>> On Tue, Feb 12, 2019 at 06:55:20AM -0700, Jan Beulich wrote:
>>>>>>> On 12.02.19 at 14:25, <roger.pau@xxxxxxxxxx> wrote:
>>>>> On Tue, Feb 12, 2019 at 05:51:41AM -0700, Jan Beulich wrote:
>>>>>> >>> On 28.01.19 at 08:06, <chao.gao@xxxxxxxxx> 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'.

Okay, in this case a global lock will perhaps do for now (as boot
time AP bringup is serialized in this regard already anyway). I do
think though that the sibling maps should be set up earlier, but
this doesn't look to be as simple as moving ahead the call to
set_cpu_sibling_map(). I wonder anyway why all sorts of stuff
have accumulated ahead of smp_callin(), thus delaying the boot
process in particular on large systems. Clearly something taking
as long as microcode load should, if at all possible, happen
afterwards.

Jan



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