lkml.org 
[lkml]   [2022]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH][V2] ACPI: sysfs: copy ACPI data using io memory copying
From
On 22-03-01 08:00, Lorenzo Pieralisi wrote:
> On Mon, Feb 28, 2022 at 11:51:23PM +0000, Henry Willard wrote:
>>
>>
>>> 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?
>
> Yes this should be fixed. I put together a patch that I needed to post,
> below.
>
> -- >8 --
> Subject: [PATCH] ACPI: osl: Fix BERT error region memory mapping

I hit this read issue while debugging something else and this addressed
the problem for me on Ampere Altra and AltraMax systems. Previously I
could only read the BERT file using dd with BS of 1-4.
With this patch, now I can 'cp' the BERT file.

Tested-by: Doug Rady <dcrady@os.amperecomputing.com>


>
> Currently the sysfs interface maps the BERT error region as "memory"
> (through acpi_os_map_memory()) in order to copy the error records into
> memory buffers through memory operations (eg memory_read_from_buffer()).
>
> The OS system cannot detect whether the BERT error region is part of
> system RAM or it is "device memory" (eg BMC memory) and therefore it
> cannot detect which memory attributes the bus to memory support (and
> corresponding kernel mapping, unless firmware provides the required
> information).
>
> The acpi_os_map_memory() arch backend implementation determines the
> mapping attributes. On arm64, if the BERT error region is not present in
> the EFI memory map, the error region is mapped as device-nGnRnE; this
> triggers alignment faults since memcpy unaligned accesses are not
> allowed in device-nGnRnE regions.
>
> The ACPI sysfs code cannot therefore map by default the BERT error
> region with memory semantics but should use a safer default.
>
> Change the sysfs code to map the BERT error region as MMIO (through
> acpi_os_map_iomem()) and use the memcpy_fromio() interface to read the
> error region into the kernel buffer.
>
> Link: https://lore.kernel.org/linux-arm-kernel/31ffe8fc-f5ee-2858-26c5-0fd8bdd68702@arm.com
> Link: https://lore.kernel.org/linux-acpi/CAJZ5v0g+OVbhuUUDrLUCfX_mVqY_e8ubgLTU98=jfjTeb4t+Pw@mail.gmail.com
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Hanjun Guo <guohanjun@huawei.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> ---
> drivers/acpi/sysfs.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index a4b638bea6f1..cc2fe0618178 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -415,19 +415,30 @@ static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
> loff_t offset, size_t count)
> {
> struct acpi_data_attr *data_attr;
> - void *base;
> - ssize_t rc;
> + void __iomem *base;
> + ssize_t size;
>
> data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
> + size = data_attr->attr.size;
> +
> + if (offset < 0)
> + return -EINVAL;
> +
> + if (offset >= size)
> + return 0;
>
> - base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size);
> + if (count > size - offset)
> + count = size - offset;
> +
> + base = acpi_os_map_iomem(data_attr->addr, size);
> if (!base)
> return -ENOMEM;
> - rc = memory_read_from_buffer(buf, count, &offset, base,
> - data_attr->attr.size);
> - acpi_os_unmap_memory(base, data_attr->attr.size);
>
> - return rc;
> + memcpy_fromio(buf, base + offset, count);
> +
> + acpi_os_unmap_iomem(base, size);
> +
> + return count;
> }
>
> static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)

\
 
 \ /
  Last update: 2022-03-07 22:23    [W:0.109 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site