Remove hardcoded maximum size a microcode patch can have. This is dynamic now. The microcode patch for family15h can be larger than 2048 bytes and gets silently truncated. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too. -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 12/12/2011 14:02, "Christoph Egger" <Christoph.Egger@amd.com> wrote:> > Remove hardcoded maximum size a microcode patch can have. > This is dynamic now.The patch doesn''t apply to xen-unstable tip. -- Keir> The microcode patch for family15h can be larger than 2048 bytes > and gets silently truncated. > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> > > P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.
>>> On 12.12.11 at 15:02, Christoph Egger <Christoph.Egger@amd.com> wrote: > Remove hardcoded maximum size a microcode patch can have. > This is dynamic now. > > The microcode patch for family15h can be larger than 2048 bytes > and gets silently truncated. > > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> > > P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too.Please sync up with latest -unstable. At least this hunk (which also is completely unrelated to anything mentioned in the description)>@@ -99,7 +91,7 @@ static int microcode_fits(void *mc, int > } > > if ( mc_header->patch_id <= uci->cpu_sig.rev ) >- return -EINVAL; >+ return 1; > > printk(KERN_DEBUG "microcode: CPU%d found a matching microcode " > "update with version 0x%x (current=0x%x)\n",would not apply anymore (and is wrong - zero is being and should be returned in this case). There''s also a stray hunk (mis?)adjusting only formatting, which should be removed. Jan
On 12/12/11 15:33, Keir Fraser wrote:> On 12/12/2011 14:02, "Christoph Egger"<Christoph.Egger@amd.com> wrote: > >> >> Remove hardcoded maximum size a microcode patch can have. >> This is dynamic now. > > The patch doesn''t apply to xen-unstable tip.Ported to xen-unstable. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>> > -- Keir > >> The microcode patch for family15h can be larger than 2048 bytes >> and gets silently truncated. >> >> Signed-off-by: Christoph Egger<Christoph.Egger@amd.com> >> >> P.S. Please apply this to Xen 4.1, 4.0, 3.4 and 3.3, too. > > >-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 13.12.11 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote: >@@ -335,17 +352,36 @@ static int microcode_resume_match(int cp > > if ( src != mc_amd ) > { >+ xfree(mc_amd->equiv_cpu_table); >+ xfree(mc_amd->mpb);mc_amd can be NULL here.> xfree(mc_amd); >- mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size); >+ >+ mc_amd = xmalloc_bytes(sizeof(*src));xmalloc(struct microcode_amd)>+ if ( !mc_amd ) >+ goto err0;The error handling is broken here. You need to clear out uci->mc.mc_amd (i.e. move up the respective assignment). Jan
On 12/13/11 16:03, Jan Beulich wrote:>>>> On 13.12.11 at 15:22, Christoph Egger<Christoph.Egger@amd.com> wrote: >> @@ -335,17 +352,36 @@ static int microcode_resume_match(int cp >> >> if ( src != mc_amd ) >> { >> + xfree(mc_amd->equiv_cpu_table); >> + xfree(mc_amd->mpb); > > mc_amd can be NULL here. > >> xfree(mc_amd); >> - mc_amd = xmalloc_bytes(sizeof(*src) + src->equiv_cpu_table_size); >> + >> + mc_amd = xmalloc_bytes(sizeof(*src)); > > xmalloc(struct microcode_amd) > >> + if ( !mc_amd ) >> + goto err0; > > The error handling is broken here. You need to clear out > uci->mc.mc_amd (i.e. move up the respective assignment).Thanks for review. New version attached. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 14.12.11 at 11:17, Christoph Egger <Christoph.Egger@amd.com> wrote: >+struct mpbhdr { >+ uint32_t type; >+ uint32_t len; >+ const uint8_t data;What''s this? If anything, this should be data[] (and no need for const).>@@ -141,16 +142,19 @@ static int apply_microcode(int cpu) > struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); > uint32_t rev; > struct microcode_amd *mc_amd = uci->mc.mc_amd; >+ struct microcode_header_amd *hdr = mc_amd->mpb;Using mc_amd here, but ...> /* We should bind the task to the CPU */ > BUG_ON(raw_smp_processor_id() != cpu); > > if ( mc_amd == NULL ) > return -EINVAL;... the NULL check is only here.>+ if ( mc_amd->mpb == NULL ) >+ return -EINVAL;And quite obviously you should preferably use the local variable here...>- wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code); >+ wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb);... and here.>+ printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n", >+ (unsigned long)bufsize, mpbuf->len, off);The cast is pointless; to avoid it, use %zu as format string or declare the parameter as ''unsigned long''.>+ if (mc_amd->mpb_size < mpbuf->len) { >+ xfree(mc_amd->mpb); >+ mc_amd->mpb = xmalloc_bytes(mpbuf->len); >+ mc_amd->mpb_size = mpbuf->len;NULL check missing here (and in such event the clearing of ->mpb_size).>+ memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len);Unnecessary cast.> printk(KERN_ERR "microcode: error! Wrong " >- "microcode patch file magic\n"); >+ "microcode patch file magic\n");Why are you still mis-adjusting indentation here?>+ mc_amd = xmalloc_bytes(sizeof(*mc_amd));As said during the first review round - this ought to be mc_amd = xmalloc(struct microcode_amd);>+ while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, >+ buf, bufsize, &offset)) == 0 )Bad indentation.>+ ASSERT(src != NULL);Pointless.>+ if (mc_amd != NULL) {While this NULL check is needed, ...>+ if (mc_amd->equiv_cpu_table)... this and ...>+ xfree(mc_amd->equiv_cpu_table); >+ if (mc_amd->mpb)... this are pointless.>+ xfree(mc_amd->mpb); >+ xfree(mc_amd); >+ } >+ >+ mc_amd = xmalloc(struct microcode_amd);uci->mc.mc_amd = mc_amd;> if ( !mc_amd )(as was the case in the original code). No need to do this once in the success path and once in the error one.>+ uci->mc.mc_amd = mc_amd = NULL; >+ return -ENOMEM;Even if it was necessary to keep it this way, initializing a local variable immediately before returning is bogus (and calling for a compiler warning and hence a build failure due to -Werror sooner or later). Jan
On 12/14/11 13:42, Jan Beulich wrote:>>>> On 14.12.11 at 11:17, Christoph Egger<Christoph.Egger@amd.com> wrote: >> +struct mpbhdr { >> + uint32_t type; >> + uint32_t len; >> + const uint8_t data; > > What''s this?This little structure eliminates the uses buf_pos[x] throughout the code which is much easier to read and understand.> If anything, this should be data[] (and no need for const).Then it is data[]>> @@ -141,16 +142,19 @@ static int apply_microcode(int cpu) >> struct ucode_cpu_info *uci =&per_cpu(ucode_cpu_info, cpu); >> uint32_t rev; >> struct microcode_amd *mc_amd = uci->mc.mc_amd; >> + struct microcode_header_amd *hdr = mc_amd->mpb; > > Using mc_amd here, but ... > >> /* We should bind the task to the CPU */ >> BUG_ON(raw_smp_processor_id() != cpu); >> >> if ( mc_amd == NULL ) >> return -EINVAL; > > ... the NULL check is only here. > >> + if ( mc_amd->mpb == NULL ) >> + return -EINVAL; > > And quite obviously you should preferably use the local variable > here... > >> - wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)&mc_amd->hdr.data_code); >> + wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)mc_amd->mpb); > > ... and here.done (assuming you mean ''hdr'').>> + printk(KERN_DEBUG "microcode: size %lu, block size %u, offset %ld\n", >> + (unsigned long)bufsize, mpbuf->len, off); > > The cast is pointless; to avoid it, use %zu as format string or declare > the parameter as ''unsigned long''.done.>> + if (mc_amd->mpb_size< mpbuf->len) { >> + xfree(mc_amd->mpb); >> + mc_amd->mpb = xmalloc_bytes(mpbuf->len); >> + mc_amd->mpb_size = mpbuf->len; > > NULL check missing here (and in such event the clearing of > ->mpb_size).done.>> + memcpy(mc_amd->mpb, (const void *)&mpbuf->data, mpbuf->len); > > Unnecessary cast.Right.>> printk(KERN_ERR "microcode: error! Wrong " >> - "microcode patch file magic\n"); >> + "microcode patch file magic\n"); > > Why are you still mis-adjusting indentation here?oh...>> + mc_amd = xmalloc_bytes(sizeof(*mc_amd)); > > As said during the first review round - this ought to be > > mc_amd = xmalloc(struct microcode_amd);sorry, didn''t realize that it was not only relevant to the shown line of code.>> + while ( (ret = get_next_ucode_from_buffer_amd(mc_amd, >> + buf, bufsize,&offset)) == 0 ) > > Bad indentation.Fixed.>> + ASSERT(src != NULL); > > Pointless.Maybe. Maybe not. Anyway, I removed it.>> + if (mc_amd != NULL) { > > While this NULL check is needed, ... > >> + if (mc_amd->equiv_cpu_table) > > ... this and ... > >> + xfree(mc_amd->equiv_cpu_table); >> + if (mc_amd->mpb) > > ... this are pointless. > >> + xfree(mc_amd->mpb); >> + xfree(mc_amd); >> + } >> + >> + mc_amd = xmalloc(struct microcode_amd); > > uci->mc.mc_amd = mc_amd; > >> if ( !mc_amd ) > > (as was the case in the original code). No need to do this once in the > success path and once in the error one. > >> + uci->mc.mc_amd = mc_amd = NULL; >> + return -ENOMEM; > > Even if it was necessary to keep it this way, initializing a local variable > immediately before returning is bogus (and calling for a compiler > warning and hence a build failure due to -Werror sooner or later).Fixed. New version attached. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>> Jan > >-- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85689 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel