[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Laura Abbott]

Currently, dmi_remap unconditionally calls ioremap_cache for dmi_remap.
The memory that's being remapped may be part of the existing EFI
memory map and already remapped as uncached. Remapping as cached can
created unexpected behavior so check the physical address against the
EFI memory type before remapping.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
Actual bug, not just theoretical problem. I'm marking this as RFC
because I'm not sure what any kind of specificaiton may have to say.
---
 arch/arm64/include/asm/dmi.h |  5 +++--
 arch/arm64/kernel/efi.c      | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/dmi.h b/arch/arm64/include/asm/dmi.h
index 69d37d8..f0cc8d0 100644
--- a/arch/arm64/include/asm/dmi.h
+++ b/arch/arm64/include/asm/dmi.h
@@ -22,9 +22,10 @@
  * request a virtual mapping for configuration tables such as SMBIOS.
  * This means we have to map them before use.
  */
-#define dmi_early_remap(x, l)		ioremap_cache(x, l)
+#define dmi_early_remap(x, l)		dmi_remap(x, l)
 #define dmi_early_unmap(x, l)		iounmap(x)
-#define dmi_remap(x, l)			ioremap_cache(x, l)
+
+extern void __iomem *dmi_remap(phys_addr_t phys_addr, size_t size);
 #define dmi_unmap(x)			iounmap(x)
 #define dmi_alloc(l)			kzalloc(l, GFP_KERNEL)
 
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 82f27c3..139666e 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 
 #include <asm/cacheflush.h>
+#include <asm/io.h>
 #include <asm/efi.h>
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
@@ -321,6 +322,28 @@ void __init efi_init(void)
 	reserve_regions();
 }
 
+static bool is_phys_addr_normal_ram(unsigned long phys_addr)
+{
+	efi_memory_desc_t *md;
+
+	for_each_efi_memory_desc(&memmap, md) {
+		if ((md->phys_addr <= phys_addr) &&
+		    (phys_addr < (md->phys_addr +
+				  (md->num_pages << EFI_PAGE_SHIFT))))
+			return is_normal_ram(md);
+	}
+	return false;
+}
+
+void __iomem *dmi_remap(phys_addr_t phys_addr, size_t size)
+{
+	if (is_phys_addr_normal_ram(phys_addr))
+		return ioremap_cache(phys_addr, size);
+	else
+		return ioremap(phys_addr, size);
+}
+
+
 void __init efi_idmap_init(void)
 {
 	if (!efi_enabled(EFI_BOOT))
-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Ard Biesheuvel]

On 17 January 2015 at 02:24, Laura Abbott <lauraa@codeaurora.org> wrote:
> Currently, dmi_remap unconditionally calls ioremap_cache for dmi_remap.
> The memory that's being remapped may be part of the existing EFI
> memory map and already remapped as uncached. Remapping as cached can
> created unexpected behavior so check the physical address against the
> EFI memory type before remapping.
>

Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
tables are not guaranteed to be aligned. So you should at least use
MT_NORMAL_NC here.

Why does it end up mapped uncached in the first place? Is it backed by
a ROM instead of normal RAM? And does the region have any of the WT/WC
attributes set in addition to UC? What type is it? I suppose it has
the EFI_MEMORY_RUNTIME bit set as well? (Note that the spec seems to
assume that configuration tables always reside in system RAM [UEFI
spec 2.4A 2.3.6])

Also, with my latest virtmap changes, UEFI runtime regions are only
mapped during runtime service invocations, and explicitly when e.g.
the SMBIOS or ACPI layer needs to get at the data. Currently, regions
backed by system RAM are still covered by the linear mapping as well,
but that is something we intend to change too. So if the region is not
system RAM (!EFI_MEMORY_WB), the latest code will not have this region
mapped at DMI scan time, afaict.

Finally, I have been looking into classifying memory as system RAM or
not myself[0]. Even if SMBIOS is UEFI only in arm64, I would prefer
not to add infrastructure that allows us to answer these kind of
questions only when booted via UEFI.

[0] http://article.gmane.org/gmane.linux.kernel.efi/5133


Regards,
Ard.


> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
> Actual bug, not just theoretical problem. I'm marking this as RFC
> because I'm not sure what any kind of specificaiton may have to say.
> ---
>  arch/arm64/include/asm/dmi.h |  5 +++--
>  arch/arm64/kernel/efi.c      | 23 +++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/include/asm/dmi.h b/arch/arm64/include/asm/dmi.h
> index 69d37d8..f0cc8d0 100644
> --- a/arch/arm64/include/asm/dmi.h
> +++ b/arch/arm64/include/asm/dmi.h
> @@ -22,9 +22,10 @@
>   * request a virtual mapping for configuration tables such as SMBIOS.
>   * This means we have to map them before use.
>   */
> -#define dmi_early_remap(x, l)          ioremap_cache(x, l)
> +#define dmi_early_remap(x, l)          dmi_remap(x, l)
>  #define dmi_early_unmap(x, l)          iounmap(x)
> -#define dmi_remap(x, l)                        ioremap_cache(x, l)
> +
> +extern void __iomem *dmi_remap(phys_addr_t phys_addr, size_t size);
>  #define dmi_unmap(x)                   iounmap(x)
>  #define dmi_alloc(l)                   kzalloc(l, GFP_KERNEL)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 82f27c3..139666e 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>
>  #include <asm/cacheflush.h>
> +#include <asm/io.h>
>  #include <asm/efi.h>
>  #include <asm/tlbflush.h>
>  #include <asm/mmu_context.h>
> @@ -321,6 +322,28 @@ void __init efi_init(void)
>         reserve_regions();
>  }
>
> +static bool is_phys_addr_normal_ram(unsigned long phys_addr)
> +{
> +       efi_memory_desc_t *md;
> +
> +       for_each_efi_memory_desc(&memmap, md) {
> +               if ((md->phys_addr <= phys_addr) &&
> +                   (phys_addr < (md->phys_addr +
> +                                 (md->num_pages << EFI_PAGE_SHIFT))))
> +                       return is_normal_ram(md);
> +       }
> +       return false;
> +}
> +
> +void __iomem *dmi_remap(phys_addr_t phys_addr, size_t size)
> +{
> +       if (is_phys_addr_normal_ram(phys_addr))
> +               return ioremap_cache(phys_addr, size);
> +       else
> +               return ioremap(phys_addr, size);
> +}
> +
> +
>  void __init efi_idmap_init(void)
>  {
>         if (!efi_enabled(EFI_BOOT))
> --
> Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Leif Lindholm]

On Sat, Jan 17, 2015 at 08:24:15AM +0000, Ard Biesheuvel wrote:
> On 17 January 2015 at 02:24, Laura Abbott <lauraa@codeaurora.org> wrote:
> > Currently, dmi_remap unconditionally calls ioremap_cache for dmi_remap.
> > The memory that's being remapped may be part of the existing EFI
> > memory map and already remapped as uncached. Remapping as cached can
> > created unexpected behavior so check the physical address against the
> > EFI memory type before remapping.
> 
> Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
> implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
> tables are not guaranteed to be aligned. So you should at least use
> MT_NORMAL_NC here.
> 
> Why does it end up mapped uncached in the first place? Is it backed by
> a ROM instead of normal RAM?

Makes no difference - the SMBIOS spec contains some not naturally
aligned fields. So regardless of the backing store, the tables must be
accessible by Normal memory accesses.

> And does the region have any of the WT/WC
> attributes set in addition to UC? What type is it? I suppose it has
> the EFI_MEMORY_RUNTIME bit set as well? (Note that the spec seems to
> assume that configuration tables always reside in system RAM [UEFI
> spec 2.4A 2.3.6])
> 
> Also, with my latest virtmap changes, UEFI runtime regions are only
> mapped during runtime service invocations, and explicitly when e.g.
> the SMBIOS or ACPI layer needs to get at the data. Currently, regions
> backed by system RAM are still covered by the linear mapping as well,
> but that is something we intend to change too. So if the region is not
> system RAM (!EFI_MEMORY_WB), the latest code will not have this region
> mapped at DMI scan time, afaict.

Which seems like correct behaviour to me. If your firmware gives you
bogus data, most bets are off.

What might make sense would be to if(efi_enabled(EFI_CONFIG_TABLES)),
verify WT/WC are set for the location, and bail out otherwise.

But this would effectively be a pre-emptive firmware bug workaround.

/
    Leif

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Ard Biesheuvel]

On 17 January 2015 at 11:56, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Sat, Jan 17, 2015 at 08:24:15AM +0000, Ard Biesheuvel wrote:
>> On 17 January 2015 at 02:24, Laura Abbott <lauraa@codeaurora.org> wrote:
>> > Currently, dmi_remap unconditionally calls ioremap_cache for dmi_remap.
>> > The memory that's being remapped may be part of the existing EFI
>> > memory map and already remapped as uncached. Remapping as cached can
>> > created unexpected behavior so check the physical address against the
>> > EFI memory type before remapping.
>>
>> Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
>> implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
>> tables are not guaranteed to be aligned. So you should at least use
>> MT_NORMAL_NC here.
>>
>> Why does it end up mapped uncached in the first place? Is it backed by
>> a ROM instead of normal RAM?
>
> Makes no difference - the SMBIOS spec contains some not naturally
> aligned fields. So regardless of the backing store, the tables must be
> accessible by Normal memory accesses.
>

It should be mapped MT_NORMAL regardless, but it would be good to
understand how we ended up in this situation, and whether it is
something allowed by the spec. We currently assume that every
EfiRuntimeServicesData or EfiReserved region is carved out of
EfiConventionalMemory that has the EFI_MEMORY_WB attribute set, but I
suspect that this is a non-RAM region. Note that the spec defines
EFI_MEMORY_UC memory as MT_DEVICE_nGnRnE on AArch64, so if this region
is only UC then it is a firmware bug. If it has WT/WC set, but not WB,
I think we are dealing with a compliant firmware that is not handled
correctly by the kernel. More specifically, we only distinguish
between EFI_MEMORY_WB and !EFI_MEMORY_WB, where we treat the former as
RAM and the latter as device memory. WT and WC are completely ignored.


>> And does the region have any of the WT/W
>> attributes set in addition to UC? What type is it? I suppose it has
>> the EFI_MEMORY_RUNTIME bit set as well? (Note that the spec seems to
>> assume that configuration tables always reside in system RAM [UEFI
>> spec 2.4A 2.3.6])
>>
>> Also, with my latest virtmap changes, UEFI runtime regions are only
>> mapped during runtime service invocations, and explicitly when e.g.
>> the SMBIOS or ACPI layer needs to get at the data. Currently, regions
>> backed by system RAM are still covered by the linear mapping as well,
>> but that is something we intend to change too. So if the region is not
>> system RAM (!EFI_MEMORY_WB), the latest code will not have this region
>> mapped at DMI scan time, afaict.
>
> Which seems like correct behaviour to me. If your firmware gives you
> bogus data, most bets are off.
>

Let's not point fingers just yet :-)

> What might make sense would be to if(efi_enabled(EFI_CONFIG_TABLES)),
> verify WT/WC are set for the location, and bail out otherwise.
>
> But this would effectively be a pre-emptive firmware bug workaround.
>

As I said, I would like to understand better what the nature of the
region in question is.
If it is a EfiRuntimeServicesData or EfiReserved region with WT|WC set
and WB|UC cleared, for instance, then we are doing the wrong thing by
mapping it as device memory.

-- 
Ard.

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Leif Lindholm]

On Sat, Jan 17, 2015 at 12:43:48PM +0000, Ard Biesheuvel wrote:
> >> Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
> >> implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
> >> tables are not guaranteed to be aligned. So you should at least use
> >> MT_NORMAL_NC here.
> >>
> >> Why does it end up mapped uncached in the first place? Is it backed by
> >> a ROM instead of normal RAM?
> >
> > Makes no difference - the SMBIOS spec contains some not naturally
> > aligned fields. So regardless of the backing store, the tables must be
> > accessible by Normal memory accesses.
> 
> It should be mapped MT_NORMAL regardless, but it would be good to
> understand how we ended up in this situation, and whether it is
> something allowed by the spec.

It is not really a question of the spec. If the spec does not
explicitly ban you from doing something that the architecture you are
executing on _does_, then you need to follow the architecture.

Which is not to say that the spec couldn't be more helpful in this
regard.

> We currently assume that every
> EfiRuntimeServicesData or EfiReserved region is carved out of
> EfiConventionalMemory that has the EFI_MEMORY_WB attribute set, but I
> suspect that this is a non-RAM region.

Which does not automatically mean it should not be mapped as either
uncached or non-WC. Flash that is being read/executed from _should_ be
mapped as not only WC, but WB. Only when you are writing to the flash
do you need to change the mappings to a non-WC type.

> Note that the spec defines
> EFI_MEMORY_UC memory as MT_DEVICE_nGnRnE on AArch64, so if this region
> is only UC then it is a firmware bug. If it has WT/WC set, but not WB,
> I think we are dealing with a compliant firmware that is not handled
> correctly by the kernel.

I agree with you on the purely philosophical level, but it is the WC
attribute that can atually cause problems for other things than
software. Even WT does not provide any of the memory ordering
guarantees that would be required for it to become invisible. (This is
also trued for UC, which is why WT can usually be treated as identical
to Normal UC.)

> More specifically, we only distinguish
> between EFI_MEMORY_WB and !EFI_MEMORY_WB, where we treat the former as
> RAM and the latter as device memory. WT and WC are completely ignored.

Ignoring the WT is not a problem. WC should not be completely ignored,
i.e. we need to respect its absence and never map as Normal unless it
is provided. _But_ WB and WT (at least on ARM) both implicitly mean
WC. Setting anything else in the memory map is creating an incorrect
memory map. And hence "fixing up" where firmware does so would hide
firmware issues.

If we are, for example, talking about a flash region flagged as non-WC
in the memory map because some AML routine might directly write to it,
then it cannot simultaneously be legal for that region to contain data
that is expected to require unaligned accesses to read (such as SMBIOS).

> >> And does the region have any of the WT/W
> >> attributes set in addition to UC? What type is it? I suppose it has
> >> the EFI_MEMORY_RUNTIME bit set as well? (Note that the spec seems to
> >> assume that configuration tables always reside in system RAM [UEFI
> >> spec 2.4A 2.3.6])
> >>
> >> Also, with my latest virtmap changes, UEFI runtime regions are only
> >> mapped during runtime service invocations, and explicitly when e.g.
> >> the SMBIOS or ACPI layer needs to get at the data. Currently, regions
> >> backed by system RAM are still covered by the linear mapping as well,
> >> but that is something we intend to change too. So if the region is not
> >> system RAM (!EFI_MEMORY_WB), the latest code will not have this region
> >> mapped at DMI scan time, afaict.
> >
> > Which seems like correct behaviour to me. If your firmware gives you
> > bogus data, most bets are off.
> 
> Let's not point fingers just yet :-)
> 
> > What might make sense would be to if(efi_enabled(EFI_CONFIG_TABLES)),
> > verify WT/WC are set for the location, and bail out otherwise.
> >
> > But this would effectively be a pre-emptive firmware bug workaround.
> 
> As I said, I would like to understand better what the nature of the
> region in question is.
> If it is a EfiRuntimeServicesData or EfiReserved region with WT|WC set
> and WB|UC cleared, for instance, then we are doing the wrong thing by
> mapping it as device memory.

Mmm, maybe. But, if the firmware states that a region that can be
accessed as WT but not WB, then at the very least it is doing
something very fishy. WT does not really make sense on ARM (it does
not support hardware coherency).

It is entirely legal for an ARM processor to treat WT regions as
uncached. Again - if the spec provides a distinction that is not
available in the architecture on which the software runs, the
architecture wins.

If it is an EfiReserved region, surely we shouldn't be touching it at
all?

/
    Leif

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Ard Biesheuvel]

On 17 January 2015 at 14:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Sat, Jan 17, 2015 at 12:43:48PM +0000, Ard Biesheuvel wrote:
>> >> Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
>> >> implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
>> >> tables are not guaranteed to be aligned. So you should at least use
>> >> MT_NORMAL_NC here.
>> >>
>> >> Why does it end up mapped uncached in the first place? Is it backed by
>> >> a ROM instead of normal RAM?
>> >
>> > Makes no difference - the SMBIOS spec contains some not naturally
>> > aligned fields. So regardless of the backing store, the tables must be
>> > accessible by Normal memory accesses.
>>
>> It should be mapped MT_NORMAL regardless, but it would be good to
>> understand how we ended up in this situation, and whether it is
>> something allowed by the spec.
>
> It is not really a question of the spec. If the spec does not

It /is/ a question of the spec: it specifies an exact mapping between
EFI_MEMORY_xx constants and MAIR_ELx values, which is worse than just
unhelpful if using WT cannot be supported by the architecture.

> explicitly ban you from doing something that the architecture you are
> executing on _does_, then you need to follow the architecture.
>
> Which is not to say that the spec couldn't be more helpful in this
> regard.
>
>> We currently assume that every
>> EfiRuntimeServicesData or EfiReserved region is carved out of
>> EfiConventionalMemory that has the EFI_MEMORY_WB attribute set, but I
>> suspect that this is a non-RAM region.
>
> Which does not automatically mean it should not be mapped as either
> uncached or non-WC. Flash that is being read/executed from _should_ be
> mapped as not only WC, but WB. Only when you are writing to the flash
> do you need to change the mappings to a non-WC type.
>

Executing from device memory is not a problem at all, so let's
disregard that for now.

As far as mapping ROM is concerned: I think it makes perfect sense to
specify WT instead of WB in that case, as it will be more helpful in
spotting inadvertent writes.

>> Note that the spec defines
>> EFI_MEMORY_UC memory as MT_DEVICE_nGnRnE on AArch64, so if this region
>> is only UC then it is a firmware bug. If it has WT/WC set, but not WB,
>> I think we are dealing with a compliant firmware that is not handled
>> correctly by the kernel.
>
> I agree with you on the purely philosophical level, but it is the WC
> attribute that can atually cause problems for other things than
> software. Even WT does not provide any of the memory ordering
> guarantees that would be required for it to become invisible. (This is
> also trued for UC, which is why WT can usually be treated as identical
> to Normal UC.)
>

I don't follow, especially the 'invisble' bit ..

>> More specifically, we only distinguish
>> between EFI_MEMORY_WB and !EFI_MEMORY_WB, where we treat the former as
>> RAM and the latter as device memory. WT and WC are completely ignored.
>
> Ignoring the WT is not a problem. WC should not be completely ignored,
> i.e. we need to respect its absence and never map as Normal unless it
> is provided. _But_ WB and WT (at least on ARM) both implicitly mean
> WC. Setting anything else in the memory map is creating an incorrect
> memory map. And hence "fixing up" where firmware does so would hide
> firmware issues.
>

OK, but that is not at all what I am suggesting. Note that the bits
mean 'this region /may/ be mapped as xxxx'
My suspicion is (and @Laura, please confirm) that this region is
WT|WC|UC simply because WB on a ROM is semantically dubious, and
restricting to WT makes it easier to spot errors.

> If we are, for example, talking about a flash region flagged as non-WC
> in the memory map because some AML routine might directly write to it,
> then it cannot simultaneously be legal for that region to contain data
> that is expected to require unaligned accesses to read (such as SMBIOS).
>

Agreed,

>> >> And does the region have any of the WT/W
>> >> attributes set in addition to UC? What type is it? I suppose it has
>> >> the EFI_MEMORY_RUNTIME bit set as well? (Note that the spec seems to
>> >> assume that configuration tables always reside in system RAM [UEFI
>> >> spec 2.4A 2.3.6])
>> >>
>> >> Also, with my latest virtmap changes, UEFI runtime regions are only
>> >> mapped during runtime service invocations, and explicitly when e.g.
>> >> the SMBIOS or ACPI layer needs to get at the data. Currently, regions
>> >> backed by system RAM are still covered by the linear mapping as well,
>> >> but that is something we intend to change too. So if the region is not
>> >> system RAM (!EFI_MEMORY_WB), the latest code will not have this region
>> >> mapped at DMI scan time, afaict.
>> >
>> > Which seems like correct behaviour to me. If your firmware gives you
>> > bogus data, most bets are off.
>>
>> Let's not point fingers just yet :-)
>>
>> > What might make sense would be to if(efi_enabled(EFI_CONFIG_TABLES)),
>> > verify WT/WC are set for the location, and bail out otherwise.
>> >
>> > But this would effectively be a pre-emptive firmware bug workaround.
>>
>> As I said, I would like to understand better what the nature of the
>> region in question is.
>> If it is a EfiRuntimeServicesData or EfiReserved region with WT|WC set
>> and WB|UC cleared, for instance, then we are doing the wrong thing by
>> mapping it as device memory.
>
> Mmm, maybe. But, if the firmware states that a region that can be
> accessed as WT but not WB, then at the very least it is doing
> something very fishy. WT does not really make sense on ARM (it does
> not support hardware coherency).
>

This may be an issue in general, but for immutable data I don't see a
problem here.

> It is entirely legal for an ARM processor to treat WT regions as
> uncached. Again - if the spec provides a distinction that is not
> available in the architecture on which the software runs, the
> architecture wins.
>
> If it is an EfiReserved region, surely we shouldn't be touching it at
> all?
>

We shouldn't, but we don't enforce that.

-- 
Ard.

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Leif Lindholm]

On Mon, Jan 19, 2015 at 09:49:54AM +0000, Ard Biesheuvel wrote:
> On 17 January 2015 at 14:36, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Sat, Jan 17, 2015 at 12:43:48PM +0000, Ard Biesheuvel wrote:
> >> >> Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
> >> >> implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
> >> >> tables are not guaranteed to be aligned. So you should at least use
> >> >> MT_NORMAL_NC here.
> >> >>
> >> >> Why does it end up mapped uncached in the first place? Is it backed by
> >> >> a ROM instead of normal RAM?
> >> >
> >> > Makes no difference - the SMBIOS spec contains some not naturally
> >> > aligned fields. So regardless of the backing store, the tables must be
> >> > accessible by Normal memory accesses.
> >>
> >> It should be mapped MT_NORMAL regardless, but it would be good to
> >> understand how we ended up in this situation, and whether it is
> >> something allowed by the spec.
> >
> > It is not really a question of the spec. If the spec does not
> 
> It /is/ a question of the spec: it specifies an exact mapping between
> EFI_MEMORY_xx constants and MAIR_ELx values, which is worse than just
> unhelpful if using WT cannot be supported by the architecture.

The spec can say whatever it wants. It cannot make write-through
caching a safe mapping for a device that can have side effects from
reordered, repeated or merged reads and writes. Which it also does not
claim to. The spec says that EFI_MEMORY_UC on arm64 must be Device
nGnRnE.

And the spec says that EFI_MEMORY_WT means Normal Memory Outer
Write-through non-transient Inner Write-through non-transient.
This does not guarantee either that the accesses will be treated as
cacheable or that if it is, the caches will treat them as transient.

> > explicitly ban you from doing something that the architecture you are
> > executing on _does_, then you need to follow the architecture.
> >
> > Which is not to say that the spec couldn't be more helpful in this
> > regard.
> >
> >> We currently assume that every
> >> EfiRuntimeServicesData or EfiReserved region is carved out of
> >> EfiConventionalMemory that has the EFI_MEMORY_WB attribute set, but I
> >> suspect that this is a non-RAM region.
> >
> > Which does not automatically mean it should not be mapped as either
> > uncached or non-WC. Flash that is being read/executed from _should_ be
> > mapped as not only WC, but WB. Only when you are writing to the flash
> > do you need to change the mappings to a non-WC type.
> 
> Executing from device memory is not a problem at all, so let's
> disregard that for now.

Of course it's a problem - it's slow.

> As far as mapping ROM is concerned: I think it makes perfect sense to
> specify WT instead of WB in that case, as it will be more helpful in
> spotting inadvertent writes.

If the risk of that access becoming not coherency managed _and_ taking
a substantial performance penalty, then perhaps. Otherwise, no.

> >> Note that the spec defines
> >> EFI_MEMORY_UC memory as MT_DEVICE_nGnRnE on AArch64, so if this region
> >> is only UC then it is a firmware bug. If it has WT/WC set, but not WB,
> >> I think we are dealing with a compliant firmware that is not handled
> >> correctly by the kernel.
> >
> > I agree with you on the purely philosophical level, but it is the WC
> > attribute that can atually cause problems for other things than
> > software. Even WT does not provide any of the memory ordering
> > guarantees that would be required for it to become invisible. (This is
> > also trued for UC, which is why WT can usually be treated as identical
> > to Normal UC.)
> 
> I don't follow, especially the 'invisble' bit ..

"Transparent" is probably a more correct word.

The only differenct between WT and WB is that WT guarantees
writeback. It may write back a line at a time, after having performed
a linefill to bring the location into cache.

> >> More specifically, we only distinguish
> >> between EFI_MEMORY_WB and !EFI_MEMORY_WB, where we treat the former as
> >> RAM and the latter as device memory. WT and WC are completely ignored.
> >
> > Ignoring the WT is not a problem. WC should not be completely ignored,
> > i.e. we need to respect its absence and never map as Normal unless it
> > is provided. _But_ WB and WT (at least on ARM) both implicitly mean
> > WC. Setting anything else in the memory map is creating an incorrect
> > memory map. And hence "fixing up" where firmware does so would hide
> > firmware issues.
> 
> OK, but that is not at all what I am suggesting. Note that the bits
> mean 'this region /may/ be mapped as xxxx'
> My suspicion is (and @Laura, please confirm) that this region is
> WT|WC|UC simply because WB on a ROM is semantically dubious, and
> restricting to WT makes it easier to spot errors.

WB on a ROM is no more semantically dubious than WT on a ROM.
Both are incorrect if there is ever an intent to write. As is WC.
 
> > If we are, for example, talking about a flash region flagged as non-WC
> > in the memory map because some AML routine might directly write to it,
> > then it cannot simultaneously be legal for that region to contain data
> > that is expected to require unaligned accesses to read (such as SMBIOS).
> 
> Agreed,
> 
> >> > What might make sense would be to if(efi_enabled(EFI_CONFIG_TABLES)),
> >> > verify WT/WC are set for the location, and bail out otherwise.
> >> >
> >> > But this would effectively be a pre-emptive firmware bug workaround.
> >>
> >> As I said, I would like to understand better what the nature of the
> >> region in question is.
> >> If it is a EfiRuntimeServicesData or EfiReserved region with WT|WC set
> >> and WB|UC cleared, for instance, then we are doing the wrong thing by
> >> mapping it as device memory.
> >
> > Mmm, maybe. But, if the firmware states that a region that can be
> > accessed as WT but not WB, then at the very least it is doing
> > something very fishy. WT does not really make sense on ARM (it does
> > not support hardware coherency).
> 
> This may be an issue in general, but for immutable data I don't see a
> problem here.

The problem is lower efficiency without any underlying hardware
capability effect. Which usually means someone believes there _is_ an
underlying hardware capability effect.

> > It is entirely legal for an ARM processor to treat WT regions as
> > uncached. Again - if the spec provides a distinction that is not
> > available in the architecture on which the software runs, the
> > architecture wins.
> >
> > If it is an EfiReserved region, surely we shouldn't be touching it at
> > all?
> 
> We shouldn't, but we don't enforce that.

We probably should enforce that.

/
    Leif

[RFC][PATCH] arm64: efi: Obey EFI memory type in dmi_remap

[Laura Abbott]

On 1/17/2015 12:24 AM, Ard Biesheuvel wrote:
> On 17 January 2015 at 02:24, Laura Abbott <lauraa@codeaurora.org> wrote:
>> Currently, dmi_remap unconditionally calls ioremap_cache for dmi_remap.
>> The memory that's being remapped may be part of the existing EFI
>> memory map and already remapped as uncached. Remapping as cached can
>> created unexpected behavior so check the physical address against the
>> EFI memory type before remapping.
>>
>
> Mapping the SMBIOS tables as uncached is a bad idea, as 'uncached'
> implies MT_DEVICE_nGnRnE in the arm64 kernel, and the SMBIOS structure
> tables are not guaranteed to be aligned. So you should at least use
> MT_NORMAL_NC here.
>
> Why does it end up mapped uncached in the first place? Is it backed by
> a ROM instead of normal RAM? And does the region have any of the WT/WC
> attributes set in addition to UC? What type is it? I suppose it has
> the EFI_MEMORY_RUNTIME bit set as well? (Note that the spec seems to
> assume that configuration tables always reside in system RAM [UEFI
> spec 2.4A 2.3.6])
>
> Also, with my latest virtmap changes, UEFI runtime regions are only
> mapped during runtime service invocations, and explicitly when e.g.
> the SMBIOS or ACPI layer needs to get at the data. Currently, regions
> backed by system RAM are still covered by the linear mapping as well,
> but that is something we intend to change too. So if the region is not
> system RAM (!EFI_MEMORY_WB), the latest code will not have this region
> mapped at DMI scan time, afaict.
>
> Finally, I have been looking into classifying memory as system RAM or
> not myself[0]. Even if SMBIOS is UEFI only in arm64, I would prefer
> not to add infrastructure that allows us to answer these kind of
> questions only when booted via UEFI.
>
> [0] http://article.gmane.org/gmane.linux.kernel.efi/5133
>
>

Thanks for the feedback. The tables aren't in ROM, they are in external
memory which needs to be accessed by other devices (e.g. BMC) which
necessitates having it marked as uncached without WT/WC attributes.
I think for now we are going to re-evaluate the need for the support
provided by this patch based on the feedback given.

Thanks,
Laura

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project