[PATCH] Streamline CPU_ADDR_BITS usage

[Uwe Hermann]

See patch.

Uwe.

[Patrick Georgi]

Am 03.10.2010 20:47, schrieb Uwe Hermann:
> movl	$0xff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for K8 (CONFIG_CPU_ADDR_BITS = 40) */
> jmp_if_k8(wbcache_post_fam10_setup)
> movl	$0xffff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for FAM10 (CONFIG_CPU_ADDR_BITS = 48) */
>
> -wbcache_post_fam10_setup:
> movl	$CONFIG_CPU_ADDR_BITS_MASK, %edx

This was to allow a single image to run through CAR with both K8 and
Fam10. I suppose that's not necessary anymore, given that the bootblock
doesn't use CAR, and any chip selection can and should happen there.

So I think it's safe to go ahead with that one - just stating since
you're seriously changing behaviour here.

Patrick

[Carl-Daniel Hailfinger]

On 03.10.2010 21:18, Patrick Georgi wrote:
> Am 03.10.2010 20:47, schrieb Uwe Hermann:
> > movl	$0xff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for K8 (CONFIG_CPU_ADDR_BITS = 40) */
> > jmp_if_k8(wbcache_post_fam10_setup)
> > movl	$0xffff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for FAM10 (CONFIG_CPU_ADDR_BITS = 48) */
> >
> > -wbcache_post_fam10_setup:
> > movl	$CONFIG_CPU_ADDR_BITS_MASK, %edx
>
> This was to allow a single image to run through CAR with both K8 and
> Fam10. I suppose that's not necessary anymore, given that the bootblock
> doesn't use CAR, and any chip selection can and should happen there.

I want to be able to use my socket AM2+ board with K8 and Fam10
processors. If I also want fallback/normal images, this change forces me
to use 4 images instead of 2. And yes, people who use mainboards
actually may expect to be able to use processors from different
generations if the "normal" BIOS can handle that as well.
Due to that, I'd say Nack, but if you have strong technical reasons
(instead of cosmetic reasons) which result in a net benefit to end users
even if you factor in the feature loss, I will retract the Nack.

Regards,
Carl-Daniel

[Patrick Georgi]

Am 03.10.2010 23:04, schrieb Carl-Daniel Hailfinger:
> I want to be able to use my socket AM2+ board with K8 and Fam10
> processors. If I also want fallback/normal images, this change forces me
> to use 4 images instead of 2. And yes, people who use mainboards
> actually may expect to be able to use processors from different
> generations if the "normal" BIOS can handle that as well.
> Due to that, I'd say Nack, but if you have strong technical reasons
> (instead of cosmetic reasons) which result in a net benefit to end users
> even if you factor in the feature loss, I will retract the Nack.

You're still required to have an image for K8 and Fam10 each, simply
because we don't support runtime selection of northbridge code in the
romstage.

The only thing I worked on was getting through CAR on either CPU, then
jumped into the "right" image right afterwards (as was usual in pre-CBFS
images)

So we can debate if we want to keep the current support, but that
requires more work (architecture changes in coreboot) to support
multiple northbridges in all places, not just CAR.

Patrick

[Uwe Hermann]

On Sun, Oct 03, 2010 at 11:07:27PM +0200, Patrick Georgi wrote:
> Am 03.10.2010 23:04, schrieb Carl-Daniel Hailfinger:
> > I want to be able to use my socket AM2+ board with K8 and Fam10
> > processors. If I also want fallback/normal images, this change forces me
> > to use 4 images instead of 2. And yes, people who use mainboards
> > actually may expect to be able to use processors from different
> > generations if the "normal" BIOS can handle that as well.
> > Due to that, I'd say Nack, but if you have strong technical reasons
> > (instead of cosmetic reasons) which result in a net benefit to end users
> > even if you factor in the feature loss, I will retract the Nack.
>
> You're still required to have an image for K8 and Fam10 each, simply
> because we don't support runtime selection of northbridge code in the
> romstage.
>
> The only thing I worked on was getting through CAR on either CPU, then
> jumped into the "right" image right afterwards (as was usual in pre-CBFS
> images)
>
> So we can debate if we want to keep the current support, but that
> requires more work (architecture changes in coreboot) to support
> multiple northbridges in all places, not just CAR.

I don't have plans to remove the possiblity for that in this patch.
Updated version attached which preserves the run-time choice, but
adds some hopefully correct comment.

Uwe.

[Patrick Georgi]

Am 03.10.2010 23:59, schrieb Uwe Hermann:
> /*
> * Important: The code below makes a run-time decision depending on
> * whether this is a K8 or Fam10h system. Depending on which it is,
> * the CONFIG_CPU_ADDR_BITS_MASK value might be be different.
> */
> movl	$MTRRphysMask_MSR(1), %ecx
> movl	$0xff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for K8 (CONFIG_CPU_ADDR_BITS = 40) */
> movl	$CONFIG_CPU_ADDR_BITS_MASK, %edx /* K8 */
> jmp_if_k8(wbcache_post_fam10_setup)
> movl	$0xffff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for FAM10 (CONFIG_CPU_ADDR_BITS = 48) */
> movl	$CONFIG_CPU_ADDR_BITS_MASK, %edx /* Fam10h */
>
> wbcache_post_fam10_setup:
>    movl	$(~(CONFIG_XIP_ROM_SIZE - 1) | 0x800), %eax
>    wrmsr
>
> #endif /* CONFIG_XIP_ROM_SIZE && CONFIG_XIP_ROM_BASE */

How is this supposed to work?
"Set to the build specific value, and if it is fam10 (ie. jmp_if_k8 not
taken), set to the build specific value again"?

Curiously,
Patrick

[Uwe Hermann]

On Mon, Oct 04, 2010 at 12:27:02AM +0200, Patrick Georgi wrote:
> Am 03.10.2010 23:59, schrieb Uwe Hermann:
> > /*
> > * Important: The code below makes a run-time decision depending on
> > * whether this is a K8 or Fam10h system. Depending on which it is,
> > * the CONFIG_CPU_ADDR_BITS_MASK value might be be different.
> > */
> > movl	$MTRRphysMask_MSR(1), %ecx
> > movl	$0xff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for K8 (CONFIG_CPU_ADDR_BITS = 40) */
> > movl	$CONFIG_CPU_ADDR_BITS_MASK, %edx /* K8 */
> > jmp_if_k8(wbcache_post_fam10_setup)
> > movl	$0xffff, %edx /* (1 << (CONFIG_CPU_ADDR_BITS - 32)) - 1 for FAM10 (CONFIG_CPU_ADDR_BITS = 48) */
> > movl	$CONFIG_CPU_ADDR_BITS_MASK, %edx /* Fam10h */
> >
> > wbcache_post_fam10_setup:
> >    movl	$(~(CONFIG_XIP_ROM_SIZE - 1) | 0x800), %eax
> >    wrmsr
> >
> > #endif /* CONFIG_XIP_ROM_SIZE && CONFIG_XIP_ROM_BASE */
>
> How is this supposed to work?
> "Set to the build specific value, and if it is fam10 (ie. jmp_if_k8 not
> taken), set to the build specific value again"?

Hm, seems I misunderstood how this works. I was under the impression
such a two-image solution (K8 + Fam10h code in one coreboot.rom) would
also set two different CONFIG_CPU_ADDR_BITS_MASK values, one for K8, and
one for Fam10h. And if the K8 image is running it will use
CONFIG_CPU_ADDR_BITS_MASK (=40) at runtime, but if the Fam10h image runs
it would use CONFIG_CPU_ADDR_BITS_MASK (=48) instead.
That's probably not how it would work though, it seems?

Anyway, I'll just leave this snippet alone for now. Updated patch will
follow, need to fix another kconfig-related issue brought up by Peter.

But note that the current form is also a bit dangerous. It hardcodes 40bits
for K8 and 48bits for Fam10h here unconditionally. I don't know if this
assumption is always correct for all CPUs. Using the correct per-CPU
CONFIG_CPU_ADDR_BITS_MASK would definately be safer (if this mechanism
can work here at all). Are we sure there are no K8 systems that support
CPUs with bits != 40? Are we sure there are no Fam10h CPUs with
bits != 48 (and that there never will be in the future)?

Uwe.

[Scott Duplichan]

]
]But note that the current form is also a bit dangerous. It hardcodes 40bits
]for K8 and 48bits for Fam10h here unconditionally. I don't know if this
]assumption is always correct for all CPUs. Using the correct per-CPU
]CONFIG_CPU_ADDR_BITS_MASK would definately be safer (if this mechanism
]can work here at all). Are we sure there are no K8 systems that support
]CPUs with bits != 40? Are we sure there are no Fam10h CPUs with
]bits != 48 (and that there never will be in the future)?

Getting this info from cpuid is the way to go (It is reliable on AMD
systems). Hard-coding is next best. It is fixed per family:

0Fh 40
10h 48
11h 40
12h 40
13h (no product)
14h 36
15h 48

Hopefully AMD will start supplying coreboot-ready support code, which
will allow coreboot to run exactly the same reference code as every
other AMD BIOS.

Thanks,
Scott

]Uwe.
]-- 
]http://hermann-uwe.de     | http://sigrok.org
]http://randomprojects.org | http://unmaintained-free-software.org

[Warren Turkal]

Are there processors where that CPU_ADDR_BITS_MASK cannot be reliably
retrieved from CPUID? What is the harm in using a value that is too
small for the CAR setup? In other words could we use the least common
value for any CPU instead of having a different setting on each
different chip?

Thanks,
wt

On Sun, Oct 3, 2010 at 5:14 PM, Scott Duplichan  wrote:
> ]
> ]But note that the current form is also a bit dangerous. It hardcodes 40bits
> ]for K8 and 48bits for Fam10h here unconditionally. I don't know if this
> ]assumption is always correct for all CPUs. Using the correct per-CPU
> ]CONFIG_CPU_ADDR_BITS_MASK would definately be safer (if this mechanism
> ]can work here at all). Are we sure there are no K8 systems that support
> ]CPUs with bits != 40? Are we sure there are no Fam10h CPUs with
> ]bits != 48 (and that there never will be in the future)?
>
> Getting this info from cpuid is the way to go (It is reliable on AMD
> systems). Hard-coding is next best. It is fixed per family:
>
> 0Fh 40
> 10h 48
> 11h 40
> 12h 40
> 13h (no product)
> 14h 36
> 15h 48
>
> Hopefully AMD will start supplying coreboot-ready support code, which
> will allow coreboot to run exactly the same reference code as every
> other AMD BIOS.
>
> Thanks,
> Scott
>
> ]Uwe.
> ]--
> ]http://hermann-uwe.de     | http://sigrok.org
> ]http://randomprojects.org | http://unmaintained-free-software.org

[Uwe Hermann]

On Sun, Oct 03, 2010 at 10:58:30PM -0700, Warren Turkal wrote:
> Are there processors where that CPU_ADDR_BITS_MASK cannot be reliably
> retrieved from CPUID? What is the harm in using a value that is too

Looks like there are, at least a code comment which was in there
suggests that:

/*
 * This routine needs to know how many address bits a given processor supports
 * (CONFIG_CPU_ADDR_BITS). CPUs get grumpy when you set too many bits in
 * their MTRR registers. We could generically use CPUID here and find out how
 * many are physically supported, but some CPUs are buggy, and report more
 * bits than they actually support.
 */

But I agree with you, I'd personally have no objections to using CPUID
per default, and allowing a "black-list" via kconfig variables which
use a value from the CPU's Kconfig if this CPU is known-bad (i.e.
reports an incorrect bit number).

Something like

select CPU_REPORTS_INVALID_ADDR_BITS_NUMBER
> small for the CAR setup? In other words could we use the least common
> value for any CPU instead of having a different setting on each
> different chip?

What do you mean with "chip" here? The value is CPU-specific (not
socket-specific or board-specific). It's also implemented using a
per-CPU mechanism via kconfig in my patch.

Uwe.

[Scott Duplichan]

]Are there processors where that CPU_ADDR_BITS_MASK cannot be reliably
]retrieved from CPUID? What is the harm in using a value that is too
]small for the CAR setup? In other words could we use the least common
]value for any CPU instead of having a different setting on each
]different chip?

My recent experience is with AMD processors. There are no known problems
with the AMD cpuid reporting of max physical address size for families
0Fh and beyond.

In my experience, using too few bits is workable. I once hard-coded
a 36-bit mask for a BIOS that boots both family 12h and family 14h.
Certainly coreboot should use the correct mask though.

Thanks,
Scott

]Thanks,
]wt

[Stefan Reinauer]

On 10/3/10 11:59 PM, Uwe Hermann wrote:
> +config CPU_ADDR_BITS_MASK
> hex
> default 0x00000000 if CPU_ADDR_BITS_32
> default 0x0000000f if CPU_ADDR_BITS_36
> default 0x000000ff if CPU_ADDR_BITS_40
> default 0x0000ffff if CPU_ADDR_BITS_48
> help
>  Map the number of address space bits supported by the CPU to the
>  mask field value as it needs to be written into the upper 32 bits
>  of the various MTRRphysMask_MSR MSRs.

Such stuff belongs into an include file, not into Kconfig.

[Peter Stuge]

Stefan Reinauer wrote:
> > +config CPU_ADDR_BITS_MASK
>
> Such stuff belongs into an include file, not into Kconfig.

Good point! I agree completely if it works in practise.

//Peter

[Uwe Hermann]

On Mon, Oct 04, 2010 at 01:19:07AM +0200, Peter Stuge wrote:
> Stefan Reinauer wrote:
> > > +config CPU_ADDR_BITS_MASK
> >
> > Such stuff belongs into an include file, not into Kconfig.
>
> Good point! I agree completely if it works in practise.

Are you guys suggesting something like this?

Index: src/include/cpu/cpu.h
===================================================================
--- src/include/cpu/cpu.h       (Revision 5909)
+++ src/include/cpu/cpu.h       (Arbeitskopie)
@@ -27,4 +27,21 @@
 /** end of compile time generated pci driver array */
 extern struct cpu_driver ecpu_drivers[];

+/*
+ * Map the number of address space bits supported by the CPU to the
+ * mask field value as it needs to be written into the upper 32 bits
+ * of the various MTRRphysMask_MSR MSRs.
+ */
+#if defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 32)
+#define CONFIG_CPU_ADDR_BITS_MASK 0x00000000
+#elif defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 36)
+#define CONFIG_CPU_ADDR_BITS_MASK 0x0000000f
+#elif defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 40)
+#define CONFIG_CPU_ADDR_BITS_MASK 0x000000ff
+#elif defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 48)
+#define CONFIG_CPU_ADDR_BITS_MASK 0x0000ffff
+#else
+#error No CPU_ADDR_BITS value was selected or an unknown value was selected
+#endif
+
 #endif /* CPU_CPU_H */

I guess that would work, but I'm not sure if that's really a better place for
the code or whether it looks more elegant than in a Kconfig file:

+config CPU_ADDR_BITS_MASK
+       hex
+       default 0x00000000 if CPU_ADDR_BITS_32
+       default 0x0000000f if CPU_ADDR_BITS_36
+       default 0x000000ff if CPU_ADDR_BITS_40
+       default 0x0000ffff if CPU_ADDR_BITS_48
+       help
+         Map the number of address space bits supported by the CPU to the
+         mask field value as it needs to be written into the upper 32 bits
+         of the various MTRRphysMask_MSR MSRs.

Also, if the variable is defined in a header file it should probably not have
the CONFIG_ prefix in the name right? I.e. CPU_ADDR_BITS_MASK, not
CONFIG_CPU_ADDR_BITS_MASK. Which IMHO is also a bit odd, as it _is_ indeed a
value derived from a (not user-visible) kconfig option.

Uwe.

[Myles Watson]

> On Mon, Oct 04, 2010 at 01:19:07AM +0200, Peter Stuge wrote:
> > Stefan Reinauer wrote:
> > > > +config CPU_ADDR_BITS_MASK
> > >
> > > Such stuff belongs into an include file, not into Kconfig.
> >
> > Good point! I agree completely if it works in practise.
>
> Are you guys suggesting something like this?
> Index: src/include/cpu/cpu.h
>
> --- src/include/cpu/cpu.h       (Revision 5909)
> +++ src/include/cpu/cpu.h       (Arbeitskopie)
> @@ -27,4 +27,21 @@
>  /** end of compile time generated pci driver array */
>  extern struct cpu_driver ecpu_drivers[];
>
> +/*
> Map the number of address space bits supported by the CPU to the
> mask field value as it needs to be written into the upper 32 bits
> of the various MTRRphysMask_MSR MSRs.
> */
>
> +#if defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 32)
> +#define CONFIG_CPU_ADDR_BITS_MASK 0x00000000
> +#elif defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 36)
> +#define CONFIG_CPU_ADDR_BITS_MASK 0x0000000f
> +#elif defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 40)
> +#define CONFIG_CPU_ADDR_BITS_MASK 0x000000ff
> +#elif defined(CONFIG_CPU_ADDR_BITS) && (CONFIG_CPU_ADDR_BITS == 48)
> +#define CONFIG_CPU_ADDR_BITS_MASK 0x0000ffff
> +#else
> +#error No CPU_ADDR_BITS value was selected or an unknown value was
> selected
> +#endif
>
> #endif /* CPU_CPU_H */

I have no idea what others were envisioning, but I would think something
like this would work for me.

In a processor-specific file:
#define CPU_ADDR_BITS 40

In cpu.h:
#define CPU_ADDR_BITS_MASK (0xffff >> (48 - CPU_ADDR_BITS))

or for the future:
#define CPU_ADDR_BITS_MASK (0xffffff >> (56 - CPU_ADDR_BITS))

Thanks,
Myles