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

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

Hi Lorenzo,

Just checking the status on this patch?
Are you planning to submit for Rafael to pull?

It is more important now that the below has gone into 5.18:
'[PATCH] ACPI/APEI: Limit printable size of BERT table data'
https://lore.kernel.org/lkml/CAJZ5v0gMh2ed+ZWOnd-t_uTrZtm=AUfxOAkAKWT7WQK3=gf+7w@mail.gmail.com/

Thank you!
doug


On 22-03-07 13:22, doug rady OS wrote:
> 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)
>