Re: [Xen-devel] [PATCH v5 6/8] microcode: delete microcode pointer and size from microcode_info

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 28, 2019 at 03:06:48PM +0800, Chao Gao wrote:
> microcode pointer and size were passed to other CPUs to parse
> microcode locally. Now, parsing microcode is done on one CPU.
> Other CPUs needn't parse the microcode blob; the pointer and
> size can be removed.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  xen/arch/x86/microcode.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 936f0b8..3c2274f 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -190,9 +190,7 @@ DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
>  
>  struct microcode_info {
>      unsigned int cpu;

I think cpu can also be removed as my previous reply to patch 5, at
which point you only need to store an error? In which case you should
also remove this struct then.

> -    uint32_t buffer_size;
>      int error;
> -    char buffer[1];
>  };
>  
>  static void microcode_fini_cpu(unsigned int cpu)
> @@ -316,6 +314,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) 
> buf, unsigned long len)
>  {
>      int ret;
>      struct microcode_info *info;
> +    void * buffer;
>  
>      if ( len != (uint32_t)len )
>          return -E2BIG;
> @@ -323,28 +322,26 @@ 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 )
> -        return -ENOMEM;
> -
> -    ret = copy_from_guest(info->buffer, buf, len);
> -    if ( ret != 0 )
> +    info = xmalloc(struct microcode_info);
> +    buffer = xmalloc_bytes(len);
> +    if ( !info || !buffer )
>      {
> -        xfree(info);
> -        return ret;
> +        ret = -ENOMEM;
> +        goto free;
>      }
>  
> +    ret = copy_from_guest(buffer, buf, len);
> +    if ( ret != 0 )
> +        goto free;

copy_from_guest doesn't return an errno value, you have to set ret to
EFAULT:

if ( copy_from_guest(buffer, buf, len) )
{
    ret = -EFAULT;
    goto free;
}

> +
>      if ( microcode_ops->start_update )
>      {
>          ret = microcode_ops->start_update();
>          if ( ret != 0 )
> -        {
> -            xfree(info);
> -            return ret;
> -        }
> +            goto free;
>      }
>  
> -    ret = parse_microcode_blob(info->buffer, len);
> +    ret = parse_microcode_blob(buffer, len);
>      if ( ret <= 0 )

Don't you need to free info and buffer here also?

Thanks, Roger.

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