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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.19 at 08:06, <chao.gao@xxxxxxxxx> wrote:
> @@ -213,21 +214,25 @@ static void microcode_fini_cpu(unsigned int cpu)
>  bool save_patch(struct microcode_patch *new_patch)
>  {
>      struct microcode_patch *microcode_patch;
> +    enum microcode_match_result result = MIS_UCODE;
> +    bool ret;
> +    unsigned long flag;
> +
> +    write_lock_irqsave(&cache_rwlock, flag);

Can the new variable please be named "flags", like we do (almost?)
everywhere else?

>      list_for_each_entry(microcode_patch, &microcode_cache, list)
>      {
> -        enum microcode_match_result result =
> -            microcode_ops->replace_patch(new_patch, microcode_patch);
> +        result = microcode_ops->replace_patch(new_patch, microcode_patch);
>  
>          switch ( result )
>          {
>          case OLD_UCODE:
> -            microcode_ops->free_patch(new_patch);
> -            return false;
> +            ret = false;
> +            goto out;
>  
>          case NEW_UCODE:
> -            microcode_ops->free_patch(microcode_patch);
> -            return true;
> +            ret = true;
> +            goto out;
>  
>          case MIS_UCODE:
>              continue;
> @@ -238,7 +243,27 @@ bool save_patch(struct microcode_patch *new_patch)
>          }
>      }
>      list_add_tail(&new_patch->list, &microcode_cache);
> -    return true;
> +    ret = true;

I don't see the need to update "ret" upwards from here. Afaict all
derivation can be done from "result" below here. Which then puts
under question the utility of the entire switch() above.

I have to admit that I also dislike the use of "goto" here: While
I've learned to accept its use in particular on some error handling
paths, I'm unconvinced that this function can't be written without
its use.

> @@ -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?

Jan



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