[PATCH] EFI: Only set regions uncacheable if they support it

[PATCH] EFI: Only set regions uncacheable if they support it

[Matthew Garrett]

The EFI memory region attributes field indicates whether the region can
be mapped with various cache attributes. Our current implementation always
marks regions uncacheable if they don't have the writeback support flag.
This causes us to mark some regions uncacheable even if they don't
indicate support for being uncacheable, triggering a clflush that may cause
an MCE. Ensure we only do this for regions which support it.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 arch/x86/platform/efi/efi.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 4cf9bd0..12f78e1 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -682,7 +682,8 @@ void __init efi_enter_virtual_mode(void)
 			continue;
 		}
 
-		if (!(md->attribute & EFI_MEMORY_WB)) {
+		if (md->attribute & EFI_MEMORY_UC &&
+		    !(md->attribute & EFI_MEMORY_WB)) {
 			addr = md->virt_addr;
 			npages = md->num_pages;
 			memrange_efi_to_native(&addr, &npages);
-- 
1.7.7.6

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[H. Peter Anvin]

On 03/15/2012 06:56 AM, Matthew Garrett wrote:
> The EFI memory region attributes field indicates whether the region can
> be mapped with various cache attributes. Our current implementation always
> marks regions uncacheable if they don't have the writeback support flag.
> This causes us to mark some regions uncacheable even if they don't
> indicate support for being uncacheable, triggering a clflush that may cause
> an MCE. Ensure we only do this for regions which support it.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

Could you be specific as to what you're seeing in the field?  In
particular, what *do* these memory regions claim to support?

	-hpa

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Matthew Garrett]

On Thu, Mar 15, 2012 at 07:00:06AM -0700, H. Peter Anvin wrote:
> On 03/15/2012 06:56 AM, Matthew Garrett wrote:
> > The EFI memory region attributes field indicates whether the region can
> > be mapped with various cache attributes. Our current implementation always
> > marks regions uncacheable if they don't have the writeback support flag.
> > This causes us to mark some regions uncacheable even if they don't
> > indicate support for being uncacheable, triggering a clflush that may cause
> > an MCE. Ensure we only do this for regions which support it.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> Could you be specific as to what you're seeing in the field?  In
> particular, what *do* these memory regions claim to support?

I have a report of a system that fails to boot with an MCE during EFI 
setup. The memory range is marked reserved and claims not to support any 
caching type, which I think probably translates as "Don't do anything to 
this region ever".

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[H. Peter Anvin]

On 03/15/2012 07:07 AM, Matthew Garrett wrote:
> 
> I have a report of a system that fails to boot with an MCE during EFI 
> setup. The memory range is marked reserved and claims not to support any 
> caching type, which I think probably translates as "Don't do anything to 
> this region ever".
> 

In other words, "don't map me"... not something we really support at the
moment, but perhaps we should; at least until we find systems in the
field that break with that constraint :(

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Matthew Garrett]

On Thu, Mar 15, 2012 at 07:17:56AM -0700, H. Peter Anvin wrote:
> On 03/15/2012 07:07 AM, Matthew Garrett wrote:
> > 
> > I have a report of a system that fails to boot with an MCE during EFI 
> > setup. The memory range is marked reserved and claims not to support any 
> > caching type, which I think probably translates as "Don't do anything to 
> > this region ever".
> > 
> 
> In other words, "don't map me"... not something we really support at the
> moment, but perhaps we should; at least until we find systems in the
> field that break with that constraint :(

Mapping should be harmless as long as we then don't touch it? I can't 
think of any circumstances where we would.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[H. Peter Anvin]

On 03/15/2012 07:24 AM, Matthew Garrett wrote:
> On Thu, Mar 15, 2012 at 07:17:56AM -0700, H. Peter Anvin wrote:
>> On 03/15/2012 07:07 AM, Matthew Garrett wrote:
>>>
>>> I have a report of a system that fails to boot with an MCE during EFI 
>>> setup. The memory range is marked reserved and claims not to support any 
>>> caching type, which I think probably translates as "Don't do anything to 
>>> this region ever".
>>>
>>
>> In other words, "don't map me"... not something we really support at the
>> moment, but perhaps we should; at least until we find systems in the
>> field that break with that constraint :(
> 
> Mapping should be harmless as long as we then don't touch it? I can't 
> think of any circumstances where we would.
> 

If we map it WB software can do speculative loads from that region which
would bring it into the cache.  If we map it UC we might have to CLFLUSH...

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Matthew Garrett]

On Thu, Mar 15, 2012 at 07:28:30AM -0700, H. Peter Anvin wrote:
> On 03/15/2012 07:24 AM, Matthew Garrett wrote:
> > On Thu, Mar 15, 2012 at 07:17:56AM -0700, H. Peter Anvin wrote:
> >> In other words, "don't map me"... not something we really support at the
> >> moment, but perhaps we should; at least until we find systems in the
> >> field that break with that constraint :(
> > 
> > Mapping should be harmless as long as we then don't touch it? I can't 
> > think of any circumstances where we would.
> > 
> 
> If we map it WB software can do speculative loads from that region which
> would bring it into the cache.  If we map it UC we might have to CLFLUSH...

We've already mapped it at this point - we then go back and flag it UC 
if it's not writeback. The latter seems to be causing problems, I'm not 
sure we've seen any indication that the former is. And these regions are 
marked as runtime accessible, so per spec they do need to be mapped into 
address space...

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[H. Peter Anvin]

On 03/15/2012 07:33 AM, Matthew Garrett wrote:
>>
>> If we map it WB software can do speculative loads from that region which
>> would bring it into the cache.  If we map it UC we might have to CLFLUSH...
> 
> We've already mapped it at this point - we then go back and flag it UC 
> if it's not writeback. The latter seems to be causing problems, I'm not 
> sure we've seen any indication that the former is. And these regions are 
> marked as runtime accessible, so per spec they do need to be mapped into 
> address space...
> 

In other words, it's totally f*cked.  I guess at that point mapping it
WB and letting the BIOS-configured MTRRs deal with the caching
attributes is probably the right thing to do.

I would still really like to understand why we're seeing #MCs... that's
bothersome all by itself.

	-hpa

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Matthew Garrett]

On Thu, Mar 15, 2012 at 07:36:46AM -0700, H. Peter Anvin wrote:

> I would still really like to understand why we're seeing #MCs... that's
> bothersome all by itself.

The region is the legacy video RAM, so I can imagine there being 
something magic about it.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[H. Peter Anvin]

On 03/15/2012 07:42 AM, Matthew Garrett wrote:
> On Thu, Mar 15, 2012 at 07:36:46AM -0700, H. Peter Anvin wrote:
> 
>> I would still really like to understand why we're seeing #MCs... that's
>> bothersome all by itself.
> 
> The region is the legacy video RAM, so I can imagine there being 
> something magic about it.
> 

WTF?!  That should be UC at all times... it isn't even WC-capable.
Furthermore, we may need this region (assuming there is actually a video
card there.)

What do the fixed MTRRs look like?

	-hpa

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Matthew Garrett]

On Thu, Mar 15, 2012 at 07:47:57AM -0700, H. Peter Anvin wrote:

> WTF?!  That should be UC at all times... it isn't even WC-capable.
> Furthermore, we may need this region (assuming there is actually a video
> card there.)

I don't think we'll ever use this region on UEFI. What I have from Intel 
is:

"It looks like this is happening when the kernel iterates over the 
various regions in the EFI memory map. It hits a region starting at 
0xA0000 and attempts to do a CLFLUSH instruction on that address. This 
is explicitly forbidden by the SAD in (codename), thus the Machine 
Check."

> What do the fixed MTRRs look like?

Unsure. I'll try to find out.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Alan Cox]

On Thu, 15 Mar 2012 14:42:36 +0000
Matthew Garrett <mjg@redhat.com> wrote:

> On Thu, Mar 15, 2012 at 07:36:46AM -0700, H. Peter Anvin wrote:
> 
> > I would still really like to understand why we're seeing #MCs... that's
> > bothersome all by itself.
> 
> The region is the legacy video RAM, so I can imagine there being 
> something magic about it.

Presumably by the point we are booting the firmware and our own early
activities mean the pages are already marked write-combine and in our CPU
caches somewhere. Is the current boot code flushing any legacy video
mappings from cache before it plays with the cachability ?

Alan

Re: [PATCH] EFI: Only set regions uncacheable if they support it

[Alan Cox]

On Thu, 15 Mar 2012 14:42:36 +0000
Matthew Garrett <mjg@redhat.com> wrote:

> On Thu, Mar 15, 2012 at 07:36:46AM -0700, H. Peter Anvin wrote:
> 
> > I would still really like to understand why we're seeing #MCs... that's
> > bothersome all by itself.
> 
> The region is the legacy video RAM, so I can imagine there being 
> something magic about it.

What hardware platforms are afflicted by this btw ?