Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
Hi Andrew, On 26/03/2020 14:53, Andrew Cooper wrote: On 21/03/2020 22:19, Julien Grall wrote: 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? I'd really prefer to, but sadly not. That requires untangling headers sufficiently so we can include string.h, to be able to use memcpy. I don't have time at the moment to sort that out. Ok :(. We will have to live with the macro for the time being then. Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
On 26.03.2020 16:38, Andrew Cooper wrote: > On 23/03/2020 08:38, Jan Beulich wrote: >> On 21.03.2020 23:19, Julien Grall wrote: >>> On Fri, 20 Mar 2020 at 21:26, Andrew Cooper >>> wrote: --- 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? >> And even if that wouldn't work out - what's the point of having >> macro argument names with leading underscores? > > Consistency with all the other code in this file. I value consistency quite high, but then please consistent with something that properly follows other rules. >> This isn't any >> better standard-wise (afaict) than other uses of leading >> underscores for identifiers which aren't CU-scope. > > It is a parameter describing textural replacement within the body. > There is 0 interaction with external namespacing standards. Please forgive me saying so, but a typical reply I might get back from you would be: And? I'm not going to insist nor nak the patch, but I don't welcome widening existing issues we have. Jan
Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
On 23/03/2020 08:38, Jan Beulich wrote: > On 21.03.2020 23:19, Julien Grall wrote: >> On Fri, 20 Mar 2020 at 21:26, Andrew Cooper >> wrote: >>> --- 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? > And even if that wouldn't work out - what's the point of having > macro argument names with leading underscores? Consistency with all the other code in this file. > This isn't any > better standard-wise (afaict) than other uses of leading > underscores for identifiers which aren't CU-scope. It is a parameter describing textural replacement within the body. There is 0 interaction with external namespacing standards. ~Andrew
Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
On 21/03/2020 22:19, Julien Grall wrote: >> 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? I'd really prefer to, but sadly not. That requires untangling headers sufficiently so we can include string.h, to be able to use memcpy. I don't have time at the moment to sort that out. ~Andrew
Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
On 21.03.2020 23:19, Julien Grall wrote: > On Fri, 20 Mar 2020 at 21:26, Andrew Cooper wrote: >> --- 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? And even if that wouldn't work out - what's the point of having macro argument names with leading underscores? This isn't any better standard-wise (afaict) than other uses of leading underscores for identifiers which aren't CU-scope. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
Hi Andrew, On Fri, 20 Mar 2020 at 21:26, Andrew Cooper wrote: > > Use it to simplify the x86 microcode logic, taking the opportunity to drop the > -ENOMEM printks. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > --- > 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
Re: [Xen-devel] [PATCH 4/4] xen: Introduce a xmemdup_bytes() helper
On Fri, Mar 20, 2020 at 09:24:52PM +, Andrew Cooper wrote: > Use it to simplify the x86 microcode logic, taking the opportunity to drop the > -ENOMEM printks. > > Signed-off-by: Andrew Cooper Reviewed-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel