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

[doug rady OS <dcrady@os.amperecomputing.com>]

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)