[patch][v2][2/4] Family 10

[Stefan Reinauer]

* Marc Jones  [080416 19:19]:
> Update the FAM10 microcode to current versions. In addition, AP microcode is 
> now updated in early initialization to support errata settings that require it.
>
> Signed-off-by: Marc Jones ([e-mail])

Acked-by: Stefan Reinauer

See a few comments below. They're mostly future ideas.

Stefan
> Index: coreboot-v2/src/cpu/amd/microcode/microcode.c
>
> --- coreboot-v2.orig/src/cpu/amd/microcode/microcode.c	2008-04-16 10:20:04.000000000 -0600
> +++ coreboot-v2/src/cpu/amd/microcode/microcode.c	2008-04-16 10:27:43.000000000 -0600
> @@ -61,8 +61,8 @@
>  {
>
> if (m->processor_rev_id != equivalent_processor_rev_id) {
> printk_debug("microcode: rev id does not match this patch.\n");
> printk_debug("microcode: Not updated! Fix microcode_updates[] \n");
> printk_err("microcode: rev id (%x) does not match this patch.\n", m->processor_rev_id);
> printk_err("microcode: Not updated! Fix microcode_updates[] \n");
> return 0;

Is this really an error condition? Or rather a warning? How serious is
it?
> Index: coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h
>
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h	2008-04-16 09:49:40.000000000 -0600
> @@ -0,0 +1,164 @@
> +/*
> ============================================================
> (c) Advanced Micro Devices, Inc., 2004-2008
> The  enclosed microcode  is intended  to be  used  with AMD
> Microprocessors.  You  may   copy,  view  and  install  the
> enclosed microcode  only for development  and deployment of
> firmware,  BIOS,  or  operating  system code  for  computer
> systems   that  contain  AMD   processors.   You   are  not
> authorized  to use  the  enclosed microcode  for any  other
> purpose.
> THE  MICROCODE IS PROVIDED  "AS IS"  WITHOUT ANY  EXPRESS OR
> IMPLIED WARRANTY  OF ANY KIND, INCLUDING BUT  NOT LIMITED TO
> WARRANTIES    OF    MERCHANTABILITY,   NON-    INFRINGEMENT,
> TITLE,FITNESS  FOR  ANY  PARTICULAR PURPOSE,  OR  WARRANTIES
> ARISING FROM CONDUCT, COURSE  OF DEALING, OR USAGE OF TRADE.
> AMD does not assume  any responsibility for any errors which
> may  appear   in  this   microcode  or  any   other  related
> information provided  to you by  AMD, or result from  use of
> this microcode.   AMD is not obligated  to furnish, support,
> or  make   any  further  information,   software,  technical
> information, know-how, or show-how available related to this
> microcode.
> The  microcode is provided  with "RESTRICTED  RIGHTS."  Use,
> duplication, or disclosure by the U.S. Government is subject
> to  the  restrictions as  set  forth  in  FAR 52.227-14  and
> DFAR252.227-7013,  et seq.,  or its  successor.  Use  of the
> microcode    by    the    U.S.     Government    constitutes
> acknowledgement  of   AMD's  proprietary  rights   in  them.
> ============================================================
>
> +*/

I know this is an old license and we have been accepting this before.
We are fine with linking this into our code, are we?

Would it make sense for v3 to pack this into a lar? and load it
externally?
> @@ -455,6 +450,7 @@
>    { X86_VENDOR_AMD, 0x100f21 },
>    { X86_VENDOR_AMD, 0x100f2A },
>    { X86_VENDOR_AMD, 0x100f22 },
> { X86_VENDOR_AMD, 0x100f23 },
> { 0, 0 },
>
> };

I want to suggest here that the list of CPUs is changed to only use
family and model but not stepping. AMD CPUs are not broken enough that
we need different cpu init code per stepping. So it is enough to write

{ X86_VENDOR_AMD, 0x100f20 },

and it will match 1, a, 2, 3, ....

I am not a fan of artificially restricting the number of machines that
are correctly initialized just because a developer didnt have it in
her/his hands yet.

[Marc Jones]

Stefan Reinauer wrote:
> Marc Jones  [080416 19:19]:
> > Update the FAM10 microcode to current versions. In addition, AP microcode is 
> > now updated in early initialization to support errata settings that require it.
> >
> > Signed-off-by: Marc Jones ([e-mail])
>
> Acked-by: Stefan Reinauer
> See a few comments below. They're mostly future ideas.
>
> Stefan
> > printk_err("microcode: Not updated! Fix microcode_updates[] \n");
> > return 0;
>
> Is this really an error condition? Or rather a warning? How serious is
> it?

I think it should always be reported which is why I made it an _err. It 
will continue to boot but you could have any number of instabilities due 
to errata (publicly documented or not).
> > Index: coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h
> >
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h	2008-04-16 09:49:40.000000000 -0600
> > @@ -0,0 +1,164 @@
> > +/*
> > ============================================================
> > (c) Advanced Micro Devices, Inc., 2004-2008
> > The  enclosed microcode  is intended  to be  used  with AMD
> > Microprocessors.  You  may   copy,  view  and  install  the
>
> I know this is an old license and we have been accepting this before.
> We are fine with linking this into our code, are we?
>
> Would it make sense for v3 to pack this into a lar? and load it
> externally?

That would depend on how we incorporate AGESA in v3, but it is a good 
idea since this is a binary blob.
> > @@ -455,6 +450,7 @@
> >    { X86_VENDOR_AMD, 0x100f21 },
> >    { X86_VENDOR_AMD, 0x100f2A },
> >    { X86_VENDOR_AMD, 0x100f22 },
> > { X86_VENDOR_AMD, 0x100f23 },
> > { 0, 0 },
> >
> > };
>
> I want to suggest here that the list of CPUs is changed to only use
> family and model but not stepping. AMD CPUs are not broken enough that
> we need different cpu init code per stepping. So it is enough to write
>
> { X86_VENDOR_AMD, 0x100f20 },
>
> and it will match 1, a, 2, 3, ....
>
> I am not a fan of artificially restricting the number of machines that
> are correctly initialized just because a developer didnt have it in
> her/his hands yet.

I have mixed feelings. I think it is important to have all the stepping 
errata in place. This is important for stability and performance. I 
agree that AMD aren't broken enough that things will catch on fire 
without the errata but I would prefer coreboot have the same (or better) 
quality as the IBVs.

Marc

[Stefan Reinauer]

Marc Jones wrote:
> > > @@ -455,6 +450,7 @@
> > >      { X86_VENDOR_AMD, 0x100f21 },
> > >      { X86_VENDOR_AMD, 0x100f2A },
> > >      { X86_VENDOR_AMD, 0x100f22 },
> > > { X86_VENDOR_AMD, 0x100f23 },
> > > { 0, 0 },
> > >
> > > };
> >
> > I want to suggest here that the list of CPUs is changed to only use
> > family and model but not stepping. AMD CPUs are not broken enough that
> > we need different cpu init code per stepping. So it is enough to write
> >
> > { X86_VENDOR_AMD, 0x100f20 },
> >
> > and it will match 1, a, 2, 3, ....
> >
> > I am not a fan of artificially restricting the number of machines that
> > are correctly initialized just because a developer didnt have it in
> > her/his hands yet.
>
> I have mixed feelings. I think it is important to have all the 
> stepping errata in place. This is important for stability and 
> performance. I agree that AMD aren't broken enough that things will 
> catch on fire without the errata but I would prefer coreboot have the 
> same (or better) quality as the IBVs.

Wait, I am not asking to remove the code. But all the Fam10 code has a 
single table of CPU steppings and that hits a single function for 
executing the errata.
only and match if those two are correct.The way the matching works is if 
there is no entry for family, model, stepping in the table, it will look 
for family, model (ie &ffffff00) You could still add an extra table for 
single steppings if required for what ever reason.

Stefan

[Carl-Daniel Hailfinger]

On 22.04.2008 20:05, Marc Jones wrote:
> Stefan Reinauer wrote:
> > Marc Jones  [080416 19:19]:
> > > Update the FAM10 microcode to current versions. In addition, AP microcode is 
> > > now updated in early initialization to support errata settings that require it.
> > >
> > > Signed-off-by: Marc Jones ([e-mail])
> >
> > Acked-by: Stefan Reinauer
> > See a few comments below. They're mostly future ideas.
> >
> > Stefan
> > > Index: coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h
> > >
> > > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > > +++ coreboot-v2/src/cpu/amd/model_10xxx/mc_patch_01000065.h	2008-04-16 09:49:40.000000000 -0600
> > > @@ -0,0 +1,164 @@
> > > +/*
> > > ============================================================
> > > (c) Advanced Micro Devices, Inc., 2004-2008
> > > The  enclosed microcode  is intended  to be  used  with AMD
> > > Microprocessors.  You  may   copy,  view  and  install  the
> >
> > I know this is an old license and we have been accepting this before.
> > We are fine with linking this into our code, are we?
> >
> > Would it make sense for v3 to pack this into a lar? and load it
> > externally?
>
> That would depend on how we incorporate AGESA in v3, but it is a good 
> idea since this is a binary blob.

As the resident "everything in LAR" guy I'm strongly in favour of having
microcode updates as blobs in a LAR. Two points need to be made sure,
though:
- If we compress the update, a decompression routine must not be able to
trigger data-corruption bugs.
- We have to know whether microcode updates can be deferred to a point
after RAMinit.
> > > @@ -455,6 +450,7 @@
> > >    { X86_VENDOR_AMD, 0x100f21 },
> > >    { X86_VENDOR_AMD, 0x100f2A },
> > >    { X86_VENDOR_AMD, 0x100f22 },
> > > { X86_VENDOR_AMD, 0x100f23 },
> > > { 0, 0 },
> > >
> > > };
> >
> > I want to suggest here that the list of CPUs is changed to only use
> > family and model but not stepping. AMD CPUs are not broken enough that
> > we need different cpu init code per stepping. So it is enough to write
> >
> > { X86_VENDOR_AMD, 0x100f20 },
> >
> > and it will match 1, a, 2, 3, ....
> >
> > I am not a fan of artificially restricting the number of machines that
> > are correctly initialized just because a developer didnt have it in
> > her/his hands yet.
>
> I have mixed feelings. I think it is important to have all the stepping 
> errata in place. This is important for stability and performance. I 
> agree that AMD aren't broken enough that things will catch on fire 
> without the errata but I would prefer coreboot have the same (or better) 
> quality as the IBVs.

Agreed. Can we handle this like IP routing tables?
Store familiy/model/stepping together with number of significant bits
from the left, then pick the one which matches and has most significant
digits.

That even allows for append-only tables with overrides.

We have to make sure that the table is stored in a separate LAR blob to
avoid having to update stage2 for microcode updates and more supported
processors.

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> As the resident "everything in LAR" guy I'm strongly in favour of having
> microcode updates as blobs in a LAR. Two points need to be made sure,
> though:
> If we compress the update, a decompression routine must not be able to
>
> trigger data-corruption bugs.

You mean you want to make our lzma algorithm more solid? That's welcome.
> We have to know whether microcode updates can be deferred to a point
>
> after RAMinit.

Why do you think it should be deferred? It currently happens in what is 
stage2. That's long time after ram is initialized. It's even after all 
the PCI init.
> Agreed. Can we handle this like IP routing tables?
> Store familiy/model/stepping together with number of significant bits
> from the left, then pick the one which matches and has most significant
> digits.

That's basically how it has been working for the last 5 years, except 
that you can not omit family or model.
Adding that complexity is questionable, given that we don't even use the 
flexibility we have since 5+ ys except on the VIA CPUs where I recently 
introduced it.
> That even allows for append-only tables with overrides.

This should work quite nicely. If someone would use it.
> We have to make sure that the table is stored in a separate LAR blob to
> avoid having to update stage2 for microcode updates and more supported
> processors.

The table has nothing to do with the microcode. Microcode has its own 
format stating for which CPU revision it can be used. What we're talking 
about is the cpu_driver's id_table. Go have a look at the code to see 
how it is implemented. We're really just cutting out cpu steppings 
artificially.

In the following code snippet

static struct device_operations cpu_dev_ops = {
        .init = model_10xxx_init,
};
static struct cpu_device_id cpu_table[] = {
        { X86_VENDOR_AMD, 0x100f00 },           /* SH-F0 L1 */
        { X86_VENDOR_AMD, 0x100f10 },           /* M2 */
        { X86_VENDOR_AMD, 0x100f20 },           /* S1g1 */
        { X86_VENDOR_AMD, 0x100f21 },
        { X86_VENDOR_AMD, 0x100f2A },
        { X86_VENDOR_AMD, 0x100f22 },
        { 0, 0 },
};
static struct cpu_driver model_10xxx __cpu_driver = {
        .ops      = &cpu_dev_ops,
        .id_table = cpu_table,
};

it would be 100% the same to have a cpu_table struct looking like this:

static struct cpu_device_id cpu_table[] = {
        { X86_VENDOR_AMD, 0x100f00 },           /* SH-F0 L1 */
        { X86_VENDOR_AMD, 0x100f10 },           /* M2 */
        { X86_VENDOR_AMD, 0x100f20 },           /* S1g1 */
        { 0, 0 }
};

No cpu caught by the version with the 6 IDs would ever fall through the 
3 ID version. It's just duplicate code.
If we really wanted to have special code in place for the 0x100f2A, we'd 
have to have a seperate struct cpu_driver anyways.

Stefan

[Carl-Daniel Hailfinger]

On 22.04.2008 22:37, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > As the resident "everything in LAR" guy I'm strongly in favour of having
> > microcode updates as blobs in a LAR. Two points need to be made sure,
> > though:
> > If we compress the update, a decompression routine must not be able to
> >
> > trigger data-corruption bugs.
>
> You mean you want to make our lzma algorithm more solid? That's welcome.

This was meant as "I hope there are no errata which could be triggered
by the lzma code".
> > We have to know whether microcode updates can be deferred to a point
> >
> > after RAMinit.
>
> Why do you think it should be deferred? It currently happens in what
> is stage2. That's long time after ram is initialized. It's even after
> all the PCI init.

Sorry, I have to admit I'm rather ignorant about microcode updates.
Thanks for educating me.
> > Agreed. Can we handle this like IP routing tables?
> > Store familiy/model/stepping together with number of significant bits
> > from the left, then pick the one which matches and has most significant
> > digits.
>
> That's basically how it has been working for the last 5 years, except
> that you can not omit family or model.
> Adding that complexity is questionable, given that we don't even use
> the flexibility we have since 5+ ys except on the VIA CPUs where I
> recently introduced it.

OK. I was just trying to address Marc's concerns.
> > That even allows for append-only tables with overrides.
>
> This should work quite nicely. If someone would use it.

OK, let's defer that to a point in time when we need it.
> > We have to make sure that the table is stored in a separate LAR blob to
> > avoid having to update stage2 for microcode updates and more supported
> > processors.
>
> The table has nothing to do with the microcode. Microcode has its own
> format stating for which CPU revision it can be used. What we're
> talking about is the cpu_driver's id_table. Go have a look at the code
> to see how it is implemented. We're really just cutting out cpu
> steppings artificially.
>
> In the following code snippet
>
> static struct device_operations cpu_dev_ops = {
>        .init = model_10xxx_init,
> };
> static struct cpu_device_id cpu_table[] = {
>        { X86_VENDOR_AMD, 0x100f00 },           /* SH-F0 L1 */
>        { X86_VENDOR_AMD, 0x100f10 },           /* M2 */
>        { X86_VENDOR_AMD, 0x100f20 },           /* S1g1 */
>        { X86_VENDOR_AMD, 0x100f21 },
>        { X86_VENDOR_AMD, 0x100f2A },
>        { X86_VENDOR_AMD, 0x100f22 },
>        { 0, 0 },
> };
> static struct cpu_driver model_10xxx __cpu_driver = {
>        .ops      = &cpu_dev_ops,
>        .id_table = cpu_table,
> };
>
> it would be 100% the same to have a cpu_table struct looking like this:
>
> static struct cpu_device_id cpu_table[] = {
>        { X86_VENDOR_AMD, 0x100f00 },           /* SH-F0 L1 */
>        { X86_VENDOR_AMD, 0x100f10 },           /* M2 */
>        { X86_VENDOR_AMD, 0x100f20 },           /* S1g1 */
>        { 0, 0 }
> };
>
> No cpu caught by the version with the 6 IDs would ever fall through
> the 3 ID version. It's just duplicate code.
> If we really wanted to have special code in place for the 0x100f2A,
> we'd have to have a seperate struct cpu_driver anyways.

Thanks for clarifying!

Regards,
Carl-Daniel

[Marc Jones]

Stefan Reinauer wrote:
> In the following code snippet
>
> static struct device_operations cpu_dev_ops = {
>        .init = model_10xxx_init,
> };
> static struct cpu_device_id cpu_table[] = {
>        { X86_VENDOR_AMD, 0x100f00 },           /* SH-F0 L1 */
>        { X86_VENDOR_AMD, 0x100f10 },           /* M2 */
>        { X86_VENDOR_AMD, 0x100f20 },           /* S1g1 */
>        { X86_VENDOR_AMD, 0x100f21 },
>        { X86_VENDOR_AMD, 0x100f2A },
>        { X86_VENDOR_AMD, 0x100f22 },
>        { 0, 0 },
> };
> static struct cpu_driver model_10xxx __cpu_driver = {
>        .ops      = &cpu_dev_ops,
>        .id_table = cpu_table,
> };
>
> it would be 100% the same to have a cpu_table struct looking like this:
>
> static struct cpu_device_id cpu_table[] = {
>        { X86_VENDOR_AMD, 0x100f00 },           /* SH-F0 L1 */
>        { X86_VENDOR_AMD, 0x100f10 },           /* M2 */
>        { X86_VENDOR_AMD, 0x100f20 },           /* S1g1 */
>        { 0, 0 }
> };
>
> No cpu caught by the version with the 6 IDs would ever fall through the 
> 3 ID version. It's just duplicate code.
> If we really wanted to have special code in place for the 0x100f2A, we'd 
> have to have a seperate struct cpu_driver anyways.

But if you don't have the stepping in the table you get: "WARNING: Using 
generic cpu ops". I think that I like warning if there is a stepping 
that hasn't been updated yet and no warning if it has.

Marc

[Marc Jones]

Stefan Reinauer wrote:
> Marc Jones  [080416 19:19]:
> > Update the FAM10 microcode to current versions. In addition, AP microcode is 
> > now updated in early initialization to support errata settings that require it.
> >
> > Signed-off-by: Marc Jones ([e-mail])
>
> Acked-by: Stefan Reinauer

r3250

Thanks,
Marc