[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