Messages in this thread | | | Date | Fri, 1 Apr 2022 11:03:27 -0700 | Subject | Re: [PATCH][V2] ACPI: sysfs: copy ACPI data using io memory copying | From | doug rady OS <> |
| |
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) >
| |