Re: [PATCH][V2] ACPI: sysfs: copy ACPI data using io memory copying

[Henry Willard <henry.willard@oracle.com>]

> On Apr 1, 2020, at 5:44 AM, James Morse <james.morse@arm.com> wrote:
> 
> Hello!
> 
> On 3/20/20 1:19 PM, Mark Rutland wrote:
>> [adding James and Lorenzo]
> 
> (but not actually...)
> 
> 
>> On Tue, Mar 17, 2020 at 04:54:09PM +0000, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>> 
>>> Reading ACPI data on ARM64 at a non-aligned offset from
>>> /sys/firmware/acpi/tables/data/BERT will cause a splat because
>>> the data is I/O memory mapped
> 
> On your platform, on someone else's it may be in memory.
> 
> Which platform is this on?
> (I've never seen one generate a BERT!)

I have seen this on several platforms. The latest is an Altra based machine. It shows up in the Linux Test Project: ltp test "read_all -d /sys -q -r 10”. 
> 
> 
>>> and being read with just a memcpy.
>>> Fix this by introducing an I/O variant of memory_read_from_buffer
>>> and using I/O memory mapped copies instead.
> 
>> Just to check, is that correct is it correct to map those tables with
>> Device attributes in the first place, or should we be mapping the tables
>> with Normal Cacheable attributes with memremap()?
>> 
>> If the FW placed those into memory using cacheavble attributes, reading
>> them using Device attributes could result in stale values, which could
>> be garbage.
> 
> Yes. The BERT code should be using arch_apei_get_mem_attribute() to use the
> correct attributes. See ghes_map() for an example. bert_init() will need to use
> a version of ioremap() that takes the pgprot_t.
> 
> Always using ioremap_cache() means you get a cacheable mapping, regardless of
> how firmware described this region in the UEFI memory map. This doesn't explain
> why you got an alignment fault.

The BERT error region doesn’t appear in the UEFI memory map on any of the systems I have looked at. This means that acpi_os_map_memory() will always map the area  as PROT_DEVICE_nGnRnE, which results in an alignment fault on an unaligned access. For some reason this does not fail on some implementations.

It isn’t clear to me from the ACPI spec whether this can be in anything other than normal memory as bert_init() seems to assume it is.

We have used this patch to resolve this problem on the assumption it will eventually make it into the mainline kernel. Is there any chance this will happen? 

Thanks,

Henry
> 
> Otherwise, looks fine to me.
> 
> 
> (N.B. I ignored this patch as it wasn't copied to linux-arm-kernel and the
> subject says its about sysfs<->ACPI, nothing to do with APEI!)
> 
> 
> Thanks,
> 
> James
>