[PATCH] Warn if we run out of MTRRs

[Carl-Daniel Hailfinger]

Print a loud warning message if we run out of MTRRs.

Signed-off-by: Carl-Daniel Hailfinger

==================================================================
--- corebootv2/src/cpu/x86/mtrr/mtrr.c	(revision 3935)
+++ corebootv2/src/cpu/x86/mtrr/mtrr.c	(working copy)
@@ -228,9 +228,14 @@
    unsigned long range_startk, unsigned long range_sizek,
    unsigned long next_range_startk, unsigned char type, unsigned address_bits)
 {
-	if (!range_sizek || (reg >= BIOS_MTRRS)) {
+	if (!range_sizek) {
+		printk_debug("range_to_mtrr called for empty range\n");
    	return reg;
    }
+	if (reg >= BIOS_MTRRS) {
+		printk_emerg("Running out of variable MTRRs!\n");
+		return reg;
+	}
    while(range_sizek) {
    	unsigned long max_align, align;
    	unsigned long sizek;
@@ -249,8 +254,10 @@
    	set_var_mtrr(reg++, range_startk, sizek, type, address_bits);
    	range_startk += sizek;
    	range_sizek -= sizek;
-		if (reg >= BIOS_MTRRS)
+		if (reg >= BIOS_MTRRS) {
+			printk_emerg("Running out of variable MTRRs!\n");
    		break;
+		}
    }
    return reg;
 }

[Myles Watson]

On Wed, Feb 11, 2009 at 7:24 AM, Carl-Daniel Hailfinger
 wrote:
> Print a loud warning message if we run out of MTRRs.

Is it really an emergency?  I guess it doesn't matter too much, but it
could be printk_info.
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Myles Watson

Myles
> ==================================================================
> --- corebootv2/src/cpu/x86/mtrr/mtrr.c  (revision 3935)
> +++ corebootv2/src/cpu/x86/mtrr/mtrr.c  (working copy)
> @@ -228,9 +228,14 @@
>        unsigned long range_startk, unsigned long range_sizek,
>        unsigned long next_range_startk, unsigned char type, unsigned address_bits)
>  {
>   if (!range_sizek || (reg >= BIOS_MTRRS)) {
>   if (!range_sizek) {
>           printk_debug("range_to_mtrr called for empty range\n");
>          return reg;
>  }
>   if (reg >= BIOS_MTRRS) {
>           printk_emerg("Running out of variable MTRRs!\n");
>           return reg;
>   }
>  while(range_sizek) {
>          unsigned long max_align, align;
>          unsigned long sizek;
>
> @@ -249,8 +254,10 @@
>                set_var_mtrr(reg++, range_startk, sizek, type, address_bits);
>                range_startk += sizek;
>                range_sizek -= sizek;
>           if (reg >= BIOS_MTRRS)
>           if (reg >= BIOS_MTRRS) {
>                   printk_emerg("Running out of variable MTRRs!\n");
>                  break;
>           }
>  }
>  return reg;
>
> }

[ron minnich]

I think it is an emergency as you always want a print.

Ron

On 2/11/09, Myles Watson  wrote:
> On Wed, Feb 11, 2009 at 7:24 AM, Carl-Daniel Hailfinger
>  wrote:
> > Print a loud warning message if we run out of MTRRs.
>
> Is it really an emergency?  I guess it doesn't matter too much, but it
> could be printk_info.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Myles Watson
>
> Myles
> > ==================================================================
> > --- corebootv2/src/cpu/x86/mtrr/mtrr.c  (revision 3935)
> > +++ corebootv2/src/cpu/x86/mtrr/mtrr.c  (working copy)
> > @@ -228,9 +228,14 @@
> >        unsigned long range_startk, unsigned long range_sizek,
> >        unsigned long next_range_startk, unsigned char type, unsigned
> > address_bits)
> >  {
> >   if (!range_sizek || (reg >= BIOS_MTRRS)) {
> >   if (!range_sizek) {
> >           printk_debug("range_to_mtrr called for empty range\n");
> >          return reg;
> >  }
> >   if (reg >= BIOS_MTRRS) {
> >           printk_emerg("Running out of variable MTRRs!\n");
> >           return reg;
> >   }
> >  while(range_sizek) {
> >          unsigned long max_align, align;
> >          unsigned long sizek;
> >
> > @@ -249,8 +254,10 @@
> >                set_var_mtrr(reg++, range_startk, sizek, type,
> > address_bits);
> >                range_startk += sizek;
> >                range_sizek -= sizek;
> >           if (reg >= BIOS_MTRRS)
> >           if (reg >= BIOS_MTRRS) {
> >                   printk_emerg("Running out of variable MTRRs!\n");
> >                  break;
> >           }
> >  }
> >  return reg;
> >
> > }

[Carl-Daniel Hailfinger]

On 11.02.2009 16:56, Myles Watson wrote:
> On Wed, Feb 11, 2009 at 7:24 AM, Carl-Daniel Hailfinger
>  wrote:
> > Print a loud warning message if we run out of MTRRs.
>
> Is it really an emergency?  I guess it doesn't matter too much, but it
> could be printk_info.

Well, it can lead to >10 minute boot times.

I made it printk_err for now.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Myles Watson

Thanks, committed in r3937.

Regards,
Carl-Daniel

[ron minnich]

I agree it's an emergency however :-)

10 minute boot times! call the hospital, the computer is sick!

ron

[Stefan Reinauer]

On 11.02.2009 18:11 Uhr, ron minnich wrote:
> I agree it's an emergency however :-)
>
> 10 minute boot times! call the hospital, the computer is sick!
>
> ron

I did a very similar patch two days ago. Sorry this is not against
Carl-Daniel's latest changes, but I thought I'd put it out for
discussion first.

There's one case when MTRR setup goes severely wrong, and that is all
those boards with UMA graphics onboard. That's all the 945GM boards, the
690 boards, some VIA boards, etc etc.

What the patch does is create a special case for UMA. It checks whether
there's an MTRR hole, and if there is none, it creates one for the UMA area.

With the patch applied my test board went down from 8 used MTRRs to 2
(one for RAM and one for UMA) and at the same time the complete RAM is
covered.

I remember there was a suggestion for a complete "optimal" MTRR setup
algorithm that attempts to always do the right thing with major
complexity. This patch is much simpler. It takes the most common case
and fixes it without trying to be perfect. So I think this is a good
solution for v2, and maybe someone wants to do the perfect thing for v3
at some point.

Comments? Acks?

Stefan

[ron minnich]

I'm a big fan of simple for a simple reason: more people read simple,
and people run away from Complex or treat is as Black Magic. So
non-optimal code that people are comfortable reading, is preferable.

ron

[Carl-Daniel Hailfinger]

On 12.02.2009 00:22, ron minnich wrote:
> I'm a big fan of simple for a simple reason: more people read simple,
> and people run away from Complex or treat is as Black Magic. So
> non-optimal code that people are comfortable reading, is preferable.

Absolutely agreed. Black Magic is cool, but understandable code which
fits the constraints has an obvious advantage.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 12.02.2009 00:19, Stefan Reinauer wrote:
> On 11.02.2009 18:11 Uhr, ron minnich wrote:
> > I agree it's an emergency however :-)
> >
> > 10 minute boot times! call the hospital, the computer is sick!

Heh. :-D
> I did a very similar patch two days ago. Sorry this is not against
> Carl-Daniel's latest changes, but I thought I'd put it out for
> discussion first.

No problem. Two patches are better than no patch.
> There's one case when MTRR setup goes severely wrong, and that is all
> those boards with UMA graphics onboard. That's all the 945GM boards, the
> 690 boards, some VIA boards, etc etc.
>
> What the patch does is create a special case for UMA. It checks whether
> there's an MTRR hole, and if there is none, it creates one for the UMA area.
>
> With the patch applied my test board went down from 8 used MTRRs to 2
> (one for RAM and one for UMA) and at the same time the complete RAM is
> covered.
>
> I remember there was a suggestion for a complete "optimal" MTRR setup
> algorithm that attempts to always do the right thing with major
> complexity. This patch is much simpler. It takes the most common case
> and fixes it without trying to be perfect. So I think this is a good
> solution for v2, and maybe someone wants to do the perfect thing for v3
> at some point.
>
> Comments? Acks?

The patch has three parts:
- Reduction of BIOS MTRRs from 8 to 6. Looks fine to me. I actually have
that in my tree as well. Ack.
- Verbose complaining if we run out of MTRRs. A variant of this has
already been committed, but we may want to use your detailed message. Good.
- UMA MTRR hole special case.
AFAICS your algorithm won't help for any board with uncached SMM memory
before UMA. One example that comes to mind is the VIA board where the
problems were noticed first. Sorry.

I'll look into creating a simplified version of my algorithm which
should be free of special cases and still understandable.

Regards,
Carl-Daniel

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> AFAICS your algorithm won't help for any board with uncached SMM
> memory before UMA.

Would it work to add a third negative MTRR for that region?

If yes, that's what I would like, and then I say to Stefan's patch:

Acked-by: Peter Stuge

[Carl-Daniel Hailfinger]

On 12.02.2009 02:00, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
> > AFAICS your algorithm won't help for any board with uncached SMM
> > memory before UMA.
>
> Would it work to add a third negative MTRR for that region?

It would be two negative MTRRs in that case.
> If yes, that's what I would like, and then I say to Stefan's patch:

You do realize that solving this problem means either adding another
special case for SMM or discarding Stefan's patch?
> Acked-by: Peter Stuge

Regards,
Carl-Daniel

[Stefan Reinauer]

On 12.02.2009 2:00 Uhr, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
> > AFAICS your algorithm won't help for any board with uncached SMM
> > memory before UMA.
>
> Would it work to add a third negative MTRR for that region?

Yes, that is also what other BIOSes do.
> If yes, that's what I would like, and then I say to Stefan's patch:
>
> Acked-by: Peter Stuge

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> > Would it work to add a third negative MTRR for that region?
>
> It would be two negative MTRRs in that case.

Yes. And the new one would be the third.
> > If yes, that's what I would like, and then I say to Stefan's patch:
>
> You do realize that solving this problem means either adding another
> special case for SMM or discarding Stefan's patch?

The former. KISS.

Stefan Reinauer wrote:
> > Would it work to add a third negative MTRR for that region?
>
> Yes, that is also what other BIOSes do.

Go for it.
> > Acked-by: Peter Stuge

This still holds.

//Peter

[Stefan Reinauer]

Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
> > > Would it work to add a third negative MTRR for that region?
> >
> > It would be two negative MTRRs in that case.
>
> Yes. And the new one would be the third.

The third used MTRR. The second negative MTRR.
> > > If yes, that's what I would like, and then I say to Stefan's patch:
> >
> > You do realize that solving this problem means either adding another
> > special case for SMM or discarding Stefan's patch?
>
> The former. KISS.

Carl-Daniel: if you solve the problem wit an algorithm that is easier to
understand and/or shorter than the current code with 2 special cases,
let's review your _code_ and I'm sure enough people will Ack it. Don't
discuss discarding my patches with empty hands, I tend to react allergic
to that.

I'm not getting tired of repeating myself.:There should not be a reason
to un-cache the upper SMM area, or the problem is elsewhere. There is
definitely no issue with any of my SMM code, and there is no other SMM
code in the public tree or on the mailing list. So once that problem
will occur, we can solve that problem.

Stefan

[ron minnich]

On Thu, Feb 12, 2009 at 8:35 AM, Stefan Reinauer  wrote:
> I'm not getting tired of repeating myself.:There should not be a reason
> to un-cache the upper SMM area, or the problem is elsewhere. There is
> definitely no issue with any of my SMM code, and there is no other SMM
> code in the public tree or on the mailing list. So once that problem
> will occur, we can solve that problem.

I strongly agree. We've got a working cached SMM, from Stefan, so we
know that one can cache SMM. For the counterargument that SMM can not
be cached, we have a hint from someone that someone else had trouble
-- and we have no code to inspect -- sorry, that's not strong enough
evidence for me to accept that we need another change to the MTRR
code.

I would prefer that we take Stefan's code as-is.

Acked-by: Ronald G. Minnich

thanks

ron

[Stefan Reinauer]

ron minnich wrote:
> On Thu, Feb 12, 2009 at 8:35 AM, Stefan Reinauer  wrote:
> > I'm not getting tired of repeating myself.:There should not be a reason
> > to un-cache the upper SMM area, or the problem is elsewhere. There is
> > definitely no issue with any of my SMM code, and there is no other SMM
> > code in the public tree or on the mailing list. So once that problem
> > will occur, we can solve that problem.
>
> I strongly agree. We've got a working cached SMM, from Stefan, so we
> know that one can cache SMM. For the counterargument that SMM can not
> be cached, we have a hint from someone that someone else had trouble
> -- and we have no code to inspect -- sorry, that's not strong enough
> evidence for me to accept that we need another change to the MTRR
> code.

Actually my SMM code is uncached, too, because it runs in ASEG. I have a
pretty clear idea what to do to get caching of the HSEG/TSEG working, if
we really ever need it. (see other mails to the list a few days ago)

Right now the SMM handler only runs when called by ACPI (as in: display
darker, display brighter buttoon pressed) or when trying to write to the
BIOS without "permission" to do so. No regular calls are made (timer
etc), so the impact of making sure we copy to HSEG and running cached
from there is probably higher than just running 2 SMM calls uncached in
the time from start-up to shut-down.

But let's get SMM for another platform into the tree and see what's best
then.

Stefan

[Joseph Smith]

> But let's get SMM for another platform into the tree and see what's best
> then.

SMM for the i830 is on my very soon todo list. I think an SMI handler is
the only way to get the tv-out (Intel calls the low level driver an AIM
module) talking to the VBIOS.

[Corey Osgood]

Hi,

Sorry, I've skimmed through this thread but I think I'm missing some
messages. If I'm understanding this thread right, we now have special cases
for UMA and SMM memory to mark them as uncachable. But a proper subtractive
mtrr scheme should handle that automatically, right? I guess what it boils
down to is, is memory reserved for SMM reported as a ram resource or no? Can
SMM memory be marked as uncachable or does it need to be untouched by MTRRs?
Also, does the ram resource code properly handle areas that need to be
reserved for ACPI, etc tables and the flash chip?

I've subtractive mtrr setup code that can handle the most basic cases, a
memory of a power of 2 size <4gb minus any number of power of 2 sized areas,
but I'm not sure how to work at around 4GB, and I don't have any systems
that have onboard video and can handle 4GB of ram to poke at.

Thanks,
Corey

[Carl-Daniel Hailfinger]

On 12.02.2009 17:35, Stefan Reinauer wrote:
> Carl-Daniel: if you solve the problem wit an algorithm that is easier to
> understand and/or shorter than the current code with 2 special cases,
> let's review your _code_ and I'm sure enough people will Ack it. Don't
> discuss discarding my patches with empty hands, I tend to react allergic
> to that.

Sorry about that. It seems my wording carried a meaning I didn't intend
in the reply to Peter. Your solution is fine.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 12.02.2009 17:35, Stefan Reinauer wrote:
> Carl-Daniel: if you solve the problem wit an algorithm that is easier to
> understand and/or shorter than the current code with 2 special cases,
> let's review your _code_ and I'm sure enough people will Ack it. Don't
> discuss discarding my patches with empty hands, I tend to react allergic
> to that.

Unfortunately, this patch
- broke every target using CONFIG_GXFUMA (AMD Pistachio and DBM690T)
- is inactive on every other target with UMA graphics and won't solve
the problem there.

Is there any in-tree board where the patch has brought an improvement?

Regards,
Carl-Daniel

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> Is there any in-tree board where the patch has brought an
> improvement?

I think that is somewhat irrelevant because the patch is a step in
the right direction. Seems it needs a little more work however.

//Peter

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 12.02.2009 17:35, Stefan Reinauer wrote:
> > Carl-Daniel: if you solve the problem wit an algorithm that is easier to
> > understand and/or shorter than the current code with 2 special cases,
> > let's review your _code_ and I'm sure enough people will Ack it. Don't
> > discuss discarding my patches with empty hands, I tend to react allergic
> > to that.
>
> Unfortunately, this patch
> broke every target using CONFIG_GXFUMA (AMD Pistachio and DBM690T)
> is inactive on every other target with UMA graphics and won't solve
>
> the problem there.
>
> Is there any in-tree board where the patch has brought an improvement?

Yes, the i945 (and other chipsets) can improve from this. I have an i945
patch in the queue to implement this, that will come shortly. It was
kind of urgent to get the generic, infrastructure parts of the patches
public before they bit-rot.

Sorry for the inconvenience and happy valentines day (it's a bit late
for this timezone but I didn't wake up until 30min ago)

Stefan

[Stefan Reinauer]

On 12.02.2009 1:19 Uhr, Carl-Daniel Hailfinger wrote:
> UMA MTRR hole special case.
>
> AFAICS your algorithm won't help for any board with uncached SMM memory
> before UMA. One example that comes to mind is the VIA board where the
> problems were noticed first. Sorry.

As I wrote, it never tried to solve that case, because it is generally
not needed ;-)

Making that SMM memory uncached seems wrong. But that's a completely
different discussion and has nothing to do with the problem I solved.

Stefan