Re: [Xen-devel] [PATCH v9 10/15] microcode: split out apply_microcode() from cpu_request_microcode()

[Jan Beulich <jbeulich@suse.com>]

On 19.08.2019 03:25, Chao Gao wrote:
> @@ -300,32 +322,44 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>      if ( microcode_ops == NULL )
>          return -EINVAL;
>  
> -    info = xmalloc_bytes(sizeof(*info) + len);
> -    if ( info == NULL )
> +    buffer = xmalloc_bytes(len);
> +    if ( !buffer )
>          return -ENOMEM;
>  
> -    ret = copy_from_guest(info->buffer, buf, len);
> -    if ( ret != 0 )
> +    if ( copy_from_guest(buffer, buf, len) )
>      {
> -        xfree(info);
> -        return ret;
> +        ret = -EFAULT;
> +        goto free;
>      }
>  
> -    info->buffer_size = len;
> -    info->error = 0;
> -    info->cpu = cpumask_first(&cpu_online_map);
> -
>      if ( microcode_ops->start_update )
>      {
>          ret = microcode_ops->start_update();
>          if ( ret != 0 )
> -        {
> -            xfree(info);
> -            return ret;
> -        }
> +            goto free;
>      }
>  
> -    return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> +    patch = parse_blob(buffer, len);
> +    if ( IS_ERR(patch) )
> +    {
> +        ret = PTR_ERR(patch);
> +        printk(XENLOG_INFO "Parsing microcode blob error %d\n", ret);

I think this wants to be at least XENLOG_WARNING.

> @@ -372,23 +406,46 @@ int __init early_microcode_update_cpu(bool start_update)
>  
>      microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
>  
> -    if ( data )
> +    if ( !data )
> +        return -ENOMEM;
> +
> +    if ( start_update )
>      {
> -        if ( start_update && microcode_ops->start_update )
> +        struct microcode_patch *patch;
> +
> +        if ( microcode_ops->start_update )
>              rc = microcode_ops->start_update();
>  
>          if ( rc )
>              return rc;
>  
> -        rc = microcode_update_cpu(data, len);
> +        patch = parse_blob(data, len);
> +        if ( IS_ERR(patch) )
> +        {
> +            printk(XENLOG_INFO "Parsing microcode blob error %ld\n",

Same here.

> +                   PTR_ERR(patch));
> +            return PTR_ERR(patch);
> +        }
> +
> +        if ( !patch )
> +        {
> +            printk(XENLOG_INFO "No ucode found. Update aborted!\n");

Here I'm not sure the message is worthwhile to have.

> @@ -41,8 +42,6 @@ struct cpu_signature {
>  DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
>  extern const struct microcode_ops *microcode_ops;
>  
> -const struct microcode_patch *microcode_get_cache(void);
> -bool microcode_update_cache(struct microcode_patch *patch);

If you remove the declaration but not the definition, then the
latter should become static.

Jan

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