Boris Ostrovsky
2012-Dec-06 18:28 UTC
[PATCH] x86/ucode: Improve error handling and container file processing on AMD
Do not report error when a patch is not appplicable to current processor,
simply skip it and move on to next patch in container file.
Process container file to the end instead of stopping at the first
applicable patch.
Log the fact that a patch has been applied at KERN_WARNING level, add
CPU number to debug messages.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>
---
xen/arch/x86/microcode_amd.c | 69 +++++++++++++++++++++++-------------------
1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 7a54001..bb5968e 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -88,13 +88,13 @@ static int collect_cpu_info(int cpu, struct cpu_signature
*csig)
rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev);
- printk(KERN_DEBUG "microcode: collect_cpu_info: patch_id=%#x\n",
- csig->rev);
+ printk(KERN_DEBUG "microcode: CPU%d collect_cpu_info:
patch_id=%#x\n",
+ cpu, csig->rev);
return 0;
}
-static int microcode_fits(const struct microcode_amd *mc_amd, int cpu)
+static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu)
{
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
const struct microcode_header_amd *mc_header = mc_amd->mpb;
@@ -125,11 +125,16 @@ static int microcode_fits(const struct microcode_amd
*mc_amd, int cpu)
printk(KERN_DEBUG "microcode: CPU%d patch does not match "
"(patch is %x, cpu base id is %x) \n",
cpu, mc_header->processor_rev_id, equiv_cpu_id);
- return -EINVAL;
+ return 0;
}
if ( mc_header->patch_id <= uci->cpu_sig.rev )
+ {
+ printk(KERN_DEBUG "microcode: CPU%d patching is not needed: "
+ "patch provides level 0x%x, cpu is at 0x%x \n",
+ cpu, mc_header->patch_id, uci->cpu_sig.rev);
return 0;
+ }
printk(KERN_DEBUG "microcode: CPU%d found a matching microcode "
"update with version %#x (current=%#x)\n",
@@ -173,7 +178,7 @@ static int apply_microcode(int cpu)
return -EIO;
}
- printk(KERN_INFO "microcode: CPU%d updated from revision %#x to
%#x\n",
+ printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to
%#x\n",
cpu, uci->cpu_sig.rev, hdr->patch_id);
uci->cpu_sig.rev = rev;
@@ -181,7 +186,7 @@ static int apply_microcode(int cpu)
return 0;
}
-static int get_next_ucode_from_buffer_amd(
+static int get_ucode_from_buffer_amd(
struct microcode_amd *mc_amd,
const void *buf,
size_t bufsize,
@@ -194,8 +199,12 @@ static int get_next_ucode_from_buffer_amd(
off = *offset;
/* No more data */
- if ( off >= bufsize )
- return 1;
+ if ( off >= bufsize )
+ {
+ printk(KERN_ERR "microcode: error! "
+ "ucode buffer overrun\n");
+ return -EINVAL;
+ }
mpbuf = (const struct mpbhdr *)&bufp[off];
if ( mpbuf->type != UCODE_UCODE_TYPE )
@@ -205,8 +214,8 @@ static int get_next_ucode_from_buffer_amd(
return -EINVAL;
}
- printk(KERN_DEBUG "microcode: size %zu, block size %u, offset
%zu\n",
- bufsize, mpbuf->len, off);
+ printk(KERN_DEBUG "microcode: CPU%d size %zu, block size %u, offset
%zu\n",
+ raw_smp_processor_id(), bufsize, mpbuf->len, off);
if ( (off + mpbuf->len) > bufsize )
{
@@ -278,8 +287,8 @@ static int cpu_request_microcode(int cpu, const void *buf,
size_t bufsize)
{
struct microcode_amd *mc_amd, *mc_old;
size_t offset = bufsize;
+ size_t last_offset, applied_offset = 0;
int error = 0;
- int ret;
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
/* We should bind the task to the CPU */
@@ -321,33 +330,32 @@ static int cpu_request_microcode(int cpu, const void *buf,
size_t bufsize)
*/
mc_amd->mpb = NULL;
mc_amd->mpb_size = 0;
- while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, buf, bufsize,
- &offset)) == 0 )
+ last_offset = offset;
+ while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
+ &offset)) == 0 )
{
- error = microcode_fits(mc_amd, cpu);
- if (error <= 0)
- continue;
+ if ( microcode_fits(mc_amd, cpu) )
+ if ( apply_microcode(cpu) == 0 )
+ applied_offset = last_offset;
- error = apply_microcode(cpu);
- if (error == 0)
- {
- error = 1;
+ last_offset = offset;
+
+ if ( offset >= bufsize )
break;
- }
}
- if ( ret < 0 )
- error = ret;
-
/* On success keep the microcode patch for
* re-apply on resume.
*/
- if ( error == 1 )
+ if ( applied_offset != 0 )
{
- xfree(mc_old);
- error = 0;
+ error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
+ &applied_offset);
+ if (error == 0)
+ xfree(mc_old);
}
- else
+
+ if ( applied_offset == 0 || error != 0 )
{
xfree(mc_amd);
uci->mc.mc_amd = mc_old;
@@ -364,10 +372,9 @@ static int microcode_resume_match(int cpu, const void *mc)
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
struct microcode_amd *mc_amd = uci->mc.mc_amd;
const struct microcode_amd *src = mc;
- int res = microcode_fits(src, cpu);
- if ( res <= 0 )
- return res;
+ if ( microcode_fits(src, cpu) == 0 )
+ return 0;
if ( src != mc_amd )
{
--
1.7.10.4
Ian Campbell
2012-Dec-07 10:21 UTC
Re: [PATCH] x86/ucode: Improve error handling and container file processing on AMD
On Thu, 2012-12-06 at 18:28 +0000, Boris Ostrovsky wrote:> Do not report error when a patch is not appplicable to current processor, > simply skip it and move on to next patch in container file. > > Process container file to the end instead of stopping at the first > applicable patch. > > Log the fact that a patch has been applied at KERN_WARNING level, add > CPU number to debug messages. > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com>This works for me, the logs are: (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626 (XEN) microcode: CPU0 size 5260, block size 2592, offset 60 (XEN) microcode: CPU0 found a matching microcode update with version 0x6000629 (current=0x6000626) (XEN) microcode: CPU0 updated from revision 0x6000626 to 0x6000629 (XEN) microcode: CPU0 size 5260, block size 2592, offset 2660 (XEN) microcode: CPU0 patch does not match (patch is 6101, cpu base id is 6012) (XEN) microcode: CPU0 size 5260, block size 2592, offset 60 (XEN) microcode: CPU1 collect_cpu_info: patch_id=0x6000629 (XEN) microcode: CPU1 size 5260, block size 2592, offset 60 (XEN) microcode: CPU1 patching is not needed: patch provides level 0x6000629, cpu is at 0x6000629 (XEN) microcode: CPU1 size 5260, block size 2592, offset 2660 (XEN) microcode: CPU1 patch does not match (patch is 6101, cpu base id is 6012) and the overall hypercall reports success. Tested-by: Ian Campbell <ian.campbell@citrix.com> Thanks!> --- > xen/arch/x86/microcode_amd.c | 69 +++++++++++++++++++++++------------------- > 1 file changed, 38 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c > index 7a54001..bb5968e 100644 > --- a/xen/arch/x86/microcode_amd.c > +++ b/xen/arch/x86/microcode_amd.c > @@ -88,13 +88,13 @@ static int collect_cpu_info(int cpu, struct cpu_signature *csig) > > rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev); > > - printk(KERN_DEBUG "microcode: collect_cpu_info: patch_id=%#x\n", > - csig->rev); > + printk(KERN_DEBUG "microcode: CPU%d collect_cpu_info: patch_id=%#x\n", > + cpu, csig->rev); > > return 0; > } > > -static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) > +static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) > { > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > const struct microcode_header_amd *mc_header = mc_amd->mpb; > @@ -125,11 +125,16 @@ static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) > printk(KERN_DEBUG "microcode: CPU%d patch does not match " > "(patch is %x, cpu base id is %x) \n", > cpu, mc_header->processor_rev_id, equiv_cpu_id); > - return -EINVAL; > + return 0; > } > > if ( mc_header->patch_id <= uci->cpu_sig.rev ) > + { > + printk(KERN_DEBUG "microcode: CPU%d patching is not needed: " > + "patch provides level 0x%x, cpu is at 0x%x \n", > + cpu, mc_header->patch_id, uci->cpu_sig.rev); > return 0; > + } > > printk(KERN_DEBUG "microcode: CPU%d found a matching microcode " > "update with version %#x (current=%#x)\n", > @@ -173,7 +178,7 @@ static int apply_microcode(int cpu) > return -EIO; > } > > - printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n", > + printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n", > cpu, uci->cpu_sig.rev, hdr->patch_id); > > uci->cpu_sig.rev = rev; > @@ -181,7 +186,7 @@ static int apply_microcode(int cpu) > return 0; > } > > -static int get_next_ucode_from_buffer_amd( > +static int get_ucode_from_buffer_amd( > struct microcode_amd *mc_amd, > const void *buf, > size_t bufsize, > @@ -194,8 +199,12 @@ static int get_next_ucode_from_buffer_amd( > off = *offset; > > /* No more data */ > - if ( off >= bufsize ) > - return 1; > + if ( off >= bufsize ) > + { > + printk(KERN_ERR "microcode: error! " > + "ucode buffer overrun\n"); > + return -EINVAL; > + } > > mpbuf = (const struct mpbhdr *)&bufp[off]; > if ( mpbuf->type != UCODE_UCODE_TYPE ) > @@ -205,8 +214,8 @@ static int get_next_ucode_from_buffer_amd( > return -EINVAL; > } > > - printk(KERN_DEBUG "microcode: size %zu, block size %u, offset %zu\n", > - bufsize, mpbuf->len, off); > + printk(KERN_DEBUG "microcode: CPU%d size %zu, block size %u, offset %zu\n", > + raw_smp_processor_id(), bufsize, mpbuf->len, off); > > if ( (off + mpbuf->len) > bufsize ) > { > @@ -278,8 +287,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) > { > struct microcode_amd *mc_amd, *mc_old; > size_t offset = bufsize; > + size_t last_offset, applied_offset = 0; > int error = 0; > - int ret; > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > > /* We should bind the task to the CPU */ > @@ -321,33 +330,32 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) > */ > mc_amd->mpb = NULL; > mc_amd->mpb_size = 0; > - while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, buf, bufsize, > - &offset)) == 0 ) > + last_offset = offset; > + while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > + &offset)) == 0 ) > { > - error = microcode_fits(mc_amd, cpu); > - if (error <= 0) > - continue; > + if ( microcode_fits(mc_amd, cpu) ) > + if ( apply_microcode(cpu) == 0 ) > + applied_offset = last_offset; > > - error = apply_microcode(cpu); > - if (error == 0) > - { > - error = 1; > + last_offset = offset; > + > + if ( offset >= bufsize ) > break; > - } > } > > - if ( ret < 0 ) > - error = ret; > - > /* On success keep the microcode patch for > * re-apply on resume. > */ > - if ( error == 1 ) > + if ( applied_offset != 0 ) > { > - xfree(mc_old); > - error = 0; > + error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > + &applied_offset); > + if (error == 0) > + xfree(mc_old); > } > - else > + > + if ( applied_offset == 0 || error != 0 ) > { > xfree(mc_amd); > uci->mc.mc_amd = mc_old; > @@ -364,10 +372,9 @@ static int microcode_resume_match(int cpu, const void *mc) > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > struct microcode_amd *mc_amd = uci->mc.mc_amd; > const struct microcode_amd *src = mc; > - int res = microcode_fits(src, cpu); > > - if ( res <= 0 ) > - return res; > + if ( microcode_fits(src, cpu) == 0 ) > + return 0; > > if ( src != mc_amd ) > {
Jan Beulich
2012-Dec-07 11:41 UTC
Re: [PATCH] x86/ucode: Improve error handling and container file processing on AMD
>>> On 06.12.12 at 19:28, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote: > Do not report error when a patch is not appplicable to current processor, > simply skip it and move on to next patch in container file. > > Process container file to the end instead of stopping at the first > applicable patch. > > Log the fact that a patch has been applied at KERN_WARNING level, add > CPU number to debug messages. > > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com> > --- > xen/arch/x86/microcode_amd.c | 69 +++++++++++++++++++++++------------------- > 1 file changed, 38 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c > index 7a54001..bb5968e 100644 > --- a/xen/arch/x86/microcode_amd.c > +++ b/xen/arch/x86/microcode_amd.c > @@ -88,13 +88,13 @@ static int collect_cpu_info(int cpu, struct cpu_signature > *csig) > > rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev); > > - printk(KERN_DEBUG "microcode: collect_cpu_info: patch_id=%#x\n", > - csig->rev); > + printk(KERN_DEBUG "microcode: CPU%d collect_cpu_info: patch_id=%#x\n", > + cpu, csig->rev); > > return 0; > } > > -static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) > +static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) > { > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > const struct microcode_header_amd *mc_header = mc_amd->mpb; > @@ -125,11 +125,16 @@ static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) > printk(KERN_DEBUG "microcode: CPU%d patch does not match " > "(patch is %x, cpu base id is %x) \n", > cpu, mc_header->processor_rev_id, equiv_cpu_id); > - return -EINVAL; > + return 0; > } > > if ( mc_header->patch_id <= uci->cpu_sig.rev ) > + { > + printk(KERN_DEBUG "microcode: CPU%d patching is not needed: " > + "patch provides level 0x%x, cpu is at 0x%x \n",Did you not notice that all the other messages now use %#x? Also, I''m not really convinced we need this added verbosity. I personally tend to run all my systems with "loglvl=all", and the amount of output produced by the code in this file made me change that for just the one AMD machine I have that is new enough to support microcode loading. Minimally I''d expect nothing to be printed here if the two versions match.> + cpu, mc_header->patch_id, uci->cpu_sig.rev); > return 0; > + } > > printk(KERN_DEBUG "microcode: CPU%d found a matching microcode " > "update with version %#x (current=%#x)\n", > @@ -173,7 +178,7 @@ static int apply_microcode(int cpu) > return -EIO; > } > > - printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n", > + printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n", > cpu, uci->cpu_sig.rev, hdr->patch_id); > > uci->cpu_sig.rev = rev; > @@ -181,7 +186,7 @@ static int apply_microcode(int cpu) > return 0; > } > > -static int get_next_ucode_from_buffer_amd( > +static int get_ucode_from_buffer_amd( > struct microcode_amd *mc_amd, > const void *buf, > size_t bufsize, > @@ -194,8 +199,12 @@ static int get_next_ucode_from_buffer_amd( > off = *offset; > > /* No more data */ > - if ( off >= bufsize ) > - return 1; > + if ( off >= bufsize ) > + { > + printk(KERN_ERR "microcode: error! " > + "ucode buffer overrun\n");All on one line please (and the "error!" probably is superfluous). Also, is printing this really correct when off == bufsize? Or can this not happen at all?> + return -EINVAL; > + } > > mpbuf = (const struct mpbhdr *)&bufp[off]; > if ( mpbuf->type != UCODE_UCODE_TYPE ) > @@ -205,8 +214,8 @@ static int get_next_ucode_from_buffer_amd( > return -EINVAL; > } > > - printk(KERN_DEBUG "microcode: size %zu, block size %u, offset %zu\n", > - bufsize, mpbuf->len, off); > + printk(KERN_DEBUG "microcode: CPU%d size %zu, block size %u, offset %zu\n", > + raw_smp_processor_id(), bufsize, mpbuf->len, off); > > if ( (off + mpbuf->len) > bufsize ) > { > @@ -278,8 +287,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) > { > struct microcode_amd *mc_amd, *mc_old; > size_t offset = bufsize; > + size_t last_offset, applied_offset = 0; > int error = 0; > - int ret; > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > > /* We should bind the task to the CPU */ > @@ -321,33 +330,32 @@ static int cpu_request_microcode(int cpu, const void > *buf, size_t bufsize) > */ > mc_amd->mpb = NULL; > mc_amd->mpb_size = 0; > - while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, buf, bufsize, > - &offset)) == 0 ) > + last_offset = offset; > + while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > + &offset)) == 0 ) > { > - error = microcode_fits(mc_amd, cpu); > - if (error <= 0) > - continue; > + if ( microcode_fits(mc_amd, cpu) ) > + if ( apply_microcode(cpu) == 0 )Fold the two if()-s into one, please. But then again you lose the return value of apply_microcode() here, which is wrong.> + applied_offset = last_offset; > > - error = apply_microcode(cpu); > - if (error == 0) > - { > - error = 1; > + last_offset = offset; > + > + if ( offset >= bufsize ) > break; > - } > } > > - if ( ret < 0 ) > - error = ret; > - > /* On success keep the microcode patch for > * re-apply on resume. > */ > - if ( error == 1 ) > + if ( applied_offset != 0 ) > { > - xfree(mc_old); > - error = 0; > + error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, > + &applied_offset); > + if (error == 0) > + xfree(mc_old); > } > - else > + > + if ( applied_offset == 0 || error != 0 ) > { > xfree(mc_amd); > uci->mc.mc_amd = mc_old; > @@ -364,10 +372,9 @@ static int microcode_resume_match(int cpu, const void *mc) > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > struct microcode_amd *mc_amd = uci->mc.mc_amd; > const struct microcode_amd *src = mc; > - int res = microcode_fits(src, cpu); > > - if ( res <= 0 ) > - return res; > + if ( microcode_fits(src, cpu) == 0 )microcode_fits() now returning bool_t clearly asks for using ! instead of == 0 here. Jan> + return 0; > > if ( src != mc_amd ) > { > -- > 1.7.10.4
Boris Ostrovsky
2012-Dec-07 13:46 UTC
Re: [PATCH] x86/ucode: Improve error handling and container file processing on AMD
Inline. On 12/07/2012 06:41 AM, Jan Beulich wrote:>>>> On 06.12.12 at 19:28, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote: >> Do not report error when a patch is not appplicable to current processor, >> simply skip it and move on to next patch in container file. >> >> Process container file to the end instead of stopping at the first >> applicable patch. >> >> Log the fact that a patch has been applied at KERN_WARNING level, add >> CPU number to debug messages. >> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@amd.com> >> --- >> xen/arch/x86/microcode_amd.c | 69 +++++++++++++++++++++++------------------- >> 1 file changed, 38 insertions(+), 31 deletions(-) >> >> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c >> index 7a54001..bb5968e 100644 >> --- a/xen/arch/x86/microcode_amd.c >> +++ b/xen/arch/x86/microcode_amd.c >> @@ -88,13 +88,13 @@ static int collect_cpu_info(int cpu, struct cpu_signature >> *csig) >> >> rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev); >> >> - printk(KERN_DEBUG "microcode: collect_cpu_info: patch_id=%#x\n", >> - csig->rev); >> + printk(KERN_DEBUG "microcode: CPU%d collect_cpu_info: patch_id=%#x\n", >> + cpu, csig->rev); >> >> return 0; >> } >> >> -static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) >> +static bool_t microcode_fits(const struct microcode_amd *mc_amd, int cpu) >> { >> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); >> const struct microcode_header_amd *mc_header = mc_amd->mpb; >> @@ -125,11 +125,16 @@ static int microcode_fits(const struct microcode_amd *mc_amd, int cpu) >> printk(KERN_DEBUG "microcode: CPU%d patch does not match " >> "(patch is %x, cpu base id is %x) \n", >> cpu, mc_header->processor_rev_id, equiv_cpu_id); >> - return -EINVAL; >> + return 0; >> } >> >> if ( mc_header->patch_id <= uci->cpu_sig.rev ) >> + { >> + printk(KERN_DEBUG "microcode: CPU%d patching is not needed: " >> + "patch provides level 0x%x, cpu is at 0x%x \n", > > Did you not notice that all the other messages now use %#x? > > Also, I''m not really convinced we need this added verbosity. I > personally tend to run all my systems with "loglvl=all", and the > amount of output produced by the code in this file made me > change that for just the one AMD machine I have that is new > enough to support microcode loading. > > Minimally I''d expect nothing to be printed here if the two > versions match.Yes, I may have gone a bit overboard with chattiness. This is actually made worse by the fact that we are going through patch loading twice during boot --- once through start_secondary() (or via microcode_presmp_init() on CPU0) and second time when the driver is initialized. Do we really need what microcode_init() does?> >> + cpu, mc_header->patch_id, uci->cpu_sig.rev); >> return 0; >> + } >> >> printk(KERN_DEBUG "microcode: CPU%d found a matching microcode " >> "update with version %#x (current=%#x)\n", >> @@ -173,7 +178,7 @@ static int apply_microcode(int cpu) >> return -EIO; >> } >> >> - printk(KERN_INFO "microcode: CPU%d updated from revision %#x to %#x\n", >> + printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n", >> cpu, uci->cpu_sig.rev, hdr->patch_id); >> >> uci->cpu_sig.rev = rev; >> @@ -181,7 +186,7 @@ static int apply_microcode(int cpu) >> return 0; >> } >> >> -static int get_next_ucode_from_buffer_amd( >> +static int get_ucode_from_buffer_amd( >> struct microcode_amd *mc_amd, >> const void *buf, >> size_t bufsize, >> @@ -194,8 +199,12 @@ static int get_next_ucode_from_buffer_amd( >> off = *offset; >> >> /* No more data */ >> - if ( off >= bufsize ) >> - return 1; >> + if ( off >= bufsize ) >> + { >> + printk(KERN_ERR "microcode: error! " >> + "ucode buffer overrun\n"); > > All on one line please (and the "error!" probably is superfluous). > > Also, is printing this really correct when off == bufsize? Or can > this not happen at all?Prior to this patch off >= bufsize was not an error but actually loop termination condition for the caller ("==" was the most likely case). Now the loop is broken out explicitly so when off >= bufsize (including "==") we have an error. Having said this, it is pretty much impossible to get it with current code.> >> + return -EINVAL; >> + } >> >> mpbuf = (const struct mpbhdr *)&bufp[off]; >> if ( mpbuf->type != UCODE_UCODE_TYPE ) >> @@ -205,8 +214,8 @@ static int get_next_ucode_from_buffer_amd( >> return -EINVAL; >> } >> >> - printk(KERN_DEBUG "microcode: size %zu, block size %u, offset %zu\n", >> - bufsize, mpbuf->len, off); >> + printk(KERN_DEBUG "microcode: CPU%d size %zu, block size %u, offset %zu\n", >> + raw_smp_processor_id(), bufsize, mpbuf->len, off); >> >> if ( (off + mpbuf->len) > bufsize ) >> { >> @@ -278,8 +287,8 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize) >> { >> struct microcode_amd *mc_amd, *mc_old; >> size_t offset = bufsize; >> + size_t last_offset, applied_offset = 0; >> int error = 0; >> - int ret; >> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); >> >> /* We should bind the task to the CPU */ >> @@ -321,33 +330,32 @@ static int cpu_request_microcode(int cpu, const void >> *buf, size_t bufsize) >> */ >> mc_amd->mpb = NULL; >> mc_amd->mpb_size = 0; >> - while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, buf, bufsize, >> - &offset)) == 0 ) >> + last_offset = offset; >> + while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >> + &offset)) == 0 ) >> { >> - error = microcode_fits(mc_amd, cpu); >> - if (error <= 0) >> - continue; >> + if ( microcode_fits(mc_amd, cpu) ) >> + if ( apply_microcode(cpu) == 0 ) > > Fold the two if()-s into one, please. > > But then again you lose the return value of apply_microcode() > here, which is wrong.Right, I need to deal with it. Unfortunately apply_microcode() may get an error after a previous patch has been successfully loaded (for the very unlikely cases when container has multiple matches). I will probably still make the driver return an error in this case too. Thanks. -boris> >> + applied_offset = last_offset; >> >> - error = apply_microcode(cpu); >> - if (error == 0) >> - { >> - error = 1; >> + last_offset = offset; >> + >> + if ( offset >= bufsize ) >> break; >> - } >> } >> >> - if ( ret < 0 ) >> - error = ret; >> - >> /* On success keep the microcode patch for >> * re-apply on resume. >> */ >> - if ( error == 1 ) >> + if ( applied_offset != 0 ) >> { >> - xfree(mc_old); >> - error = 0; >> + error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize, >> + &applied_offset); >> + if (error == 0) >> + xfree(mc_old); >> } >> - else >> + >> + if ( applied_offset == 0 || error != 0 ) >> { >> xfree(mc_amd); >> uci->mc.mc_amd = mc_old; >> @@ -364,10 +372,9 @@ static int microcode_resume_match(int cpu, const void *mc) >> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); >> struct microcode_amd *mc_amd = uci->mc.mc_amd; >> const struct microcode_amd *src = mc; >> - int res = microcode_fits(src, cpu); >> >> - if ( res <= 0 ) >> - return res; >> + if ( microcode_fits(src, cpu) == 0 ) > > microcode_fits() now returning bool_t clearly asks for using ! instead > of == 0 here. > > Jan > >> + return 0; >> >> if ( src != mc_amd ) >> { >> -- >> 1.7.10.4 > > >
Jan Beulich
2012-Dec-07 15:08 UTC
Re: [PATCH] x86/ucode: Improve error handling and container file processing on AMD
>>> On 07.12.12 at 14:46, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote: > Do we really need what microcode_init() does?Oh yes, absolutely. How else would boot time microcode loading work for secondary CPUs without this? Note that the notifier only deals with the CPU_DEAD case. Jan
Boris Ostrovsky
2012-Dec-07 15:25 UTC
Re: [PATCH] x86/ucode: Improve error handling and container file processing on AMD
On 12/07/2012 10:08 AM, Jan Beulich wrote:>>>> On 07.12.12 at 14:46, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote: >> Do we really need what microcode_init() does? > > Oh yes, absolutely. How else would boot time microcode loading > work for secondary CPUs without this? Note that the notifier only > deals with the CPU_DEAD case.start_secondary() -> microcode_resume_cpu() -> microcode_ops->apply_microcode() will update non-boot CPUs in most cases. This won''t quite work when secondary CPU is different from (or at different patch level than) boot CPU but that can be fixed. -boris
Jan Beulich
2012-Dec-07 15:42 UTC
Re: [PATCH] x86/ucode: Improve error handling and container file processing on AMD
>>> On 07.12.12 at 16:25, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote: > On 12/07/2012 10:08 AM, Jan Beulich wrote: >>>>> On 07.12.12 at 14:46, Boris Ostrovsky <boris.ostrovsky@amd.com> wrote: >>> Do we really need what microcode_init() does? >> >> Oh yes, absolutely. How else would boot time microcode loading >> work for secondary CPUs without this? Note that the notifier only >> deals with the CPU_DEAD case. > > start_secondary() -> microcode_resume_cpu() -> > microcode_ops->apply_microcode() will update non-boot CPUs in most cases. > > This won''t quite work when secondary CPU is different from (or at > different patch level than) boot CPU but that can be fixed.Exactly. And I don''t believe it can be done differently as easily as you seem to think. Jan