| Message ID | 20200320212453.21685-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ucode: Cleanup - Part 2/n | expand |
On Fri, Mar 20, 2020 at 09:24:52PM +0000, Andrew Cooper 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> Reviewed-by: Wei Liu <wl@xen.org>
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,
On 21.03.2020 23:19, Julien Grall wrote: > On Fri, 20 Mar 2020 at 21:26, Andrew Cooper <andrew.cooper3@citrix.com> 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
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
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 <andrew.cooper3@citrix.com> 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
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 <andrew.cooper3@citrix.com> 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
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 <jgrall@amazon.com> Cheers,
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_); \ + \ + if ( dst_ ) \ + memcpy(dst_, _src, nr_); \ + dst_; \ + }) + /* Free any of the above. */ extern void xfree(void *);
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 +++++++++++ 3 files changed, 15 insertions(+), 12 deletions(-)