Mailing List Archive

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.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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