Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper

[Julien Grall <julien.grall.oss@gmail.com>]

Hi Andrew,

On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> Use it to simplify the x86 microcode logic, taking the opportunity to drop the
> -ENOMEM printks.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpu/microcode/amd.c   |  9 ++-------
>  xen/arch/x86/cpu/microcode/intel.c |  7 ++-----
>  xen/include/xen/xmalloc.h          | 11 +++++++++++

I did notice a few times in the past months where only the x86 folks
where CCed even when there are changes in common code.
Even if I am mostly likely going to be happy with the changes, you
should at least give the other maintainers an opportunity to
object/comment.

May I ask to CC all the relevant maintainers in the future?
scripts/add_maintainers.pl should do the job for you.

>  3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> index 0998a36b5c..12a3b6b32c 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -299,11 +299,10 @@ static int get_ucode_from_buffer_amd(
>          return -EINVAL;
>      }
>
> -    mc_amd->mpb = xmalloc_bytes(mpbuf->len);
> +    mc_amd->mpb = xmemdup_bytes(mpbuf->data, mpbuf->len);
>      if ( !mc_amd->mpb )
>          return -ENOMEM;
>      mc_amd->mpb_size = mpbuf->len;
> -    memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
>
>      pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
>               smp_processor_id(), bufsize, mpbuf->len, *offset,
> @@ -336,14 +335,10 @@ static int install_equiv_cpu_table(
>          return -EINVAL;
>      }
>
> -    mc_amd->equiv_cpu_table = xmalloc_bytes(mpbuf->len);
> +    mc_amd->equiv_cpu_table = xmemdup_bytes(mpbuf->data, mpbuf->len);
>      if ( !mc_amd->equiv_cpu_table )
> -    {
> -        printk(KERN_ERR "microcode: Cannot allocate memory for equivalent cpu table\n");
>          return -ENOMEM;
> -    }
>
> -    memcpy(mc_amd->equiv_cpu_table, mpbuf->data, mpbuf->len);
>      mc_amd->equiv_cpu_table_size = mpbuf->len;
>
>      return 0;
> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
> index 6ac5f98694..f26511da98 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -339,13 +339,10 @@ static long get_next_ucode_from_buffer(struct microcode_intel **mc,
>          return -EINVAL;
>      }
>
> -    *mc = xmalloc_bytes(total_size);
> +    *mc = xmemdup_bytes(mc_header, total_size);
>      if ( *mc == NULL )
> -    {
> -        printk(KERN_ERR "microcode: error! Can not allocate memory\n");
>          return -ENOMEM;
> -    }
> -    memcpy(*mc, (const void *)(buf + offset), total_size);
> +
>      return offset + total_size;
>  }
>
> diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h
> index f515ceee2a..16979a117c 100644
> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -51,6 +51,17 @@
>  #define xmalloc_bytes(_bytes) _xmalloc(_bytes, SMP_CACHE_BYTES)
>  #define xzalloc_bytes(_bytes) _xzalloc(_bytes, SMP_CACHE_BYTES)
>
> +/* Allocate untyped storage and copying an existing instance. */
> +#define xmemdup_bytes(_src, _nr)                \
> +    ({                                          \
> +        unsigned long nr_ = (_nr);              \
> +        void *dst_ = xmalloc_bytes(nr_);        \

The nr_ vs _nr is really confusing to read. Could you re-implement the
function as a static inline?

Cheers,

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