Re: [PATCH V10 4/5] arm64: apei: implement arch_apei_get_mem_attributes()

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Matt Fleming <matt@codeblueprint.co.uk>
Cc: "Jonathan (Zhixiong) Zhang" <zjzhang@codeaurora.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>, Fu Wei <fu.wei@linaro.org>,
	Al Stone <al.stone@linaro.org>, Borislav Petkov <bp@alien8.de>,
	Matt Fleming <matt.fleming@intel.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Hanjun Guo <hanjun.guo@linaro.org>,
	Leif Lindholm <leif.lindholm@linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Linaro ACPI Mailman List <linaro-acpi@lists.linaro.org>,
	Timur Tabi <timur@codeaurora.org>
Subject: Re: [PATCH V10 4/5] arm64: apei: implement arch_apei_get_mem_attributes()
Date: Fri, 7 Aug 2015 11:50:16 +0200	[thread overview]
Message-ID: <CAKv+Gu-mw1Z2w+t5d=UafJd8Lz4_qH-G=R4hLBHkK0CZXHNggg@mail.gmail.com> (raw)
In-Reply-To: <20150807093750.GB2625@codeblueprint.co.uk>

On 7 August 2015 at 11:37, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Fri, 07 Aug, at 11:00:17AM, Ard Biesheuvel wrote:
>>
>> The EFI memory types are not exclusive, and so many regions will have
>> all of the above set. The UEFI spec does not define how to interpret
>> these superimposed attributes, it is up to the OS to decide on a
>> consistent approach.
>>
>> For instance, this region (captured from a arm64 boot log with
>> uefi_debug set on the command line)
>>
>> [Runtime Data       |RUN|  |  |  |   |WB|WT|WC|UC]
>>
>> would be mapped uncached when following the above logic, while it
>> makes more sense to map using PAGE_KERNEL in this case.
>
> Urgh... good point Ard. Right now this is limited to the GHES driver, so
> it's unclear whether this patch is buggy in practice or not.
>
> Does it *ever* make sense to map a region as cacheable (WB/WT/WC) on
> arm64 for the APEI/GHES case? Does the firmware handle the necessary
> cache flushing?
>

No it does not. Currently, we only consider EFI_MEMORY_WB when
discovering system RAM from the UEFI memory map, so the direct linear
mapping should have a hole where the APEI/GHES regions lives if it
doesn't have the WB attribute set. This means we can map it WT/WC/UC
without violating architectural rules regarding mismatches attributes,
but it requires an explicit ioremap()

>> From the spec:
>>
>> """
>> EFI_MEMORY_UC: The memory region supports being configured as not cacheable.
>> EFI_MEMORY_WC: The memory region supports being configured as write combining.
>> EFI_MEMORY_WT: The memory region supports being configured as
>> cacheable with a “write through” policy. Writes that hit in the cache
>> will also be written to main memory.
>> EFI_MEMORY_WB: The memory region supports being configured as
>> cacheable with a “write back” policy. Reads and writes that hit in the
>> cache do not propagate to main memory. Dirty data is written back to
>> main memory when a new cache line is allocated.
>> """
>
> Jonathan, can you please provide the EFI memory map region attributes
> for the GHES region that requires this series?
>
> --
> Matt Fleming, Intel Open Source Technology Center