| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <270ca5910c454f8f85ccba45d5ec00aa@huawei.com>
Date: Fri, 19 Jun 2020 13:42:04 +0000
From: Shiju Jose <shiju.jose@...wei.com>
To: James Morse <james.morse@....com>
CC: "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"rjw@...ysocki.net" <rjw@...ysocki.net>,
"bp@...en8.de" <bp@...en8.de>, "lenb@...nel.org" <lenb@...nel.org>,
"tony.luck@...el.com" <tony.luck@...el.com>,
"dan.carpenter@...cle.com" <dan.carpenter@...cle.com>,
"zhangliguang@...ux.alibaba.com" <zhangliguang@...ux.alibaba.com>,
"andriy.shevchenko@...ux.intel.com"
<andriy.shevchenko@...ux.intel.com>,
"Wangkefeng (OS Kernel Lab)" <wangkefeng.wang@...wei.com>,
"jroedel@...e.de" <jroedel@...e.de>,
yangyicong <yangyicong@...wei.com>,
Jonathan Cameron <jonathan.cameron@...wei.com>,
tanxiaofei <tanxiaofei@...wei.com>,
Linuxarm <linuxarm@...wei.com>
Subject: RE: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor
specific HW errors
Hi James,
Thanks for reviewing the patch and the modifications.
>-----Original Message-----
>From: James Morse [mailto:james.morse@....com]
>Sent: 18 June 2020 19:20
>To: Shiju Jose <shiju.jose@...wei.com>
>Cc: linux-acpi@...r.kernel.org; linux-pci@...r.kernel.org; linux-
>kernel@...r.kernel.org; rjw@...ysocki.net; bp@...en8.de; lenb@...nel.org;
>tony.luck@...el.com; dan.carpenter@...cle.com;
>zhangliguang@...ux.alibaba.com; andriy.shevchenko@...ux.intel.com;
>Wangkefeng (OS Kernel Lab) <wangkefeng.wang@...wei.com>;
>jroedel@...e.de; yangyicong <yangyicong@...wei.com>; Jonathan Cameron
><jonathan.cameron@...wei.com>; tanxiaofei <tanxiaofei@...wei.com>
>Subject: Re: [PATCH v9 1/2] ACPI / APEI: Add support to notify the vendor
>specific HW errors
>
>Hi Shiju,
>
>On 15/06/2020 10:53, Shiju Jose wrote:
>> Add support to notify the vendor specific non-fatal HW errors to the
>> drivers for the error recovery.
>
>This doesn't apply cleanly to v5.8-rc1... thanks for waiting for the merge
>window to finish, but please rebase onto the latest and greatest kernel!
V10 was posted based on v5.8-rc1.
>
>I'm glad the notifier chains for stuff that should be built-in has gone.
>(In my opinion, the RAS code should be moving in the direction of having less
>code run between being told of an error, and the handler running. Notifier
>chains for things like memory-errors was moving in the wrong direction!)
>
>
>The Kfifo and pool are adding complexity I don't think you need.
>Please make it clear from the naming this is for vendor records. (what is an
>event?)
>
>The memcpy() for the records is annoying, but eliminating it takes some
>really invasive changes. Lets live with it for now.
Ok.
>
>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index
>> 24c9642e8fc7..854d8115cdfc 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -63,6 +64,11 @@
>> #define GHES_ESTATUS_CACHES_SIZE 4
>>
>> #define GHES_ESTATUS_IN_CACHE_MAX_NSEC 10000000000ULL
>> +
>> +#define GHES_EVENT_RING_SIZE 256
>> +#define GHES_GDATA_POOL_MIN_ALLOC_ORDER 3
>> +#define GHES_GDATA_POOL_MIN_SIZE 65536
>
>Huh. Another pool of memory, and we don't know if this will ever be used.
>Can we allocate from ghes_estatus_pool instead?
>
>ghes_estatus_pool is already scaled with the number of error sources
>firmware describes in ghes_estatus_pool_init(), so it should be big enough.
>
>ghes_estatus_pool already has multiple users, estatus_nodes for work
>deferred from NMI come from here, as do ghes_estatus_caches for the low-
>pass filter thing.
Ok.
>
>
>> @@ -122,6 +128,19 @@ static DEFINE_MUTEX(ghes_list_mutex);
>> */
>> static DEFINE_SPINLOCK(ghes_notify_lock_irq);
>>
>> +struct ghes_event_entry {
>
>ghes_vendor_record_entry ?
>
>> + struct acpi_hest_generic_data *gdata;
>> + int error_severity;
>> +};
>
>> +static DEFINE_KFIFO(ghes_event_ring, struct ghes_event_entry,
>> + GHES_EVENT_RING_SIZE);
>> +
>> +static DEFINE_SPINLOCK(ghes_event_ring_lock);
>
>Do you need the FIFO behaviour?
>If you put a work_struct in the struct and schedule_work() that, these would
>run in any order, and it would be less code.
>
>
>> +static struct gen_pool *ghes_gdata_pool; static unsigned long
>> +ghes_gdata_pool_size_request;
>> +
>> static struct gen_pool *ghes_estatus_pool; static unsigned long
>> ghes_estatus_pool_size_request;
>
>Please use the existing ghes_estatus_pool.
>
>
>> @@ -188,6 +207,40 @@ int ghes_estatus_pool_init(int num_ghes)
>
>[...]
>
>> +static int ghes_gdata_pool_init(void) {
>> + unsigned long addr, len;
>> + int rc;
>> +
>> + ghes_gdata_pool =
>gen_pool_create(GHES_GDATA_POOL_MIN_ALLOC_ORDER, -1);
>> + if (!ghes_gdata_pool)
>> + return -ENOMEM;
>> +
>> + if (ghes_gdata_pool_size_request < GHES_GDATA_POOL_MIN_SIZE)
>> + ghes_gdata_pool_size_request =
>GHES_GDATA_POOL_MIN_SIZE;
>> +
>> + len = ghes_gdata_pool_size_request;
>> + addr = (unsigned long)vmalloc(PAGE_ALIGN(len));
>> + if (!addr)
>> + goto err_pool_alloc;
>
>> + vmalloc_sync_mappings();
>(This isn't needed anymore. See commit 73f693c3a705 ("mm: remove
>vmalloc_sync_(un)mappings()"))
>
>
>> + rc = gen_pool_add(ghes_gdata_pool, addr, PAGE_ALIGN(len), -1);
>> + if (rc)
>> + goto err_pool_add;
>> +
>> + return 0;
>> +
>> +err_pool_add:
>> + vfree((void *)addr);
>> +
>> +err_pool_alloc:
>> + gen_pool_destroy(ghes_gdata_pool);
>> +
>> + return -ENOMEM;
>> +}
>
>But: using ghes_estatus_pool would avoid this duplication.
>
>
>> @@ -247,6 +300,10 @@ static struct ghes *ghes_new(struct
>acpi_hest_generic *generic)
>> goto err_unmap_status_addr;
>> }
>>
>> + ghes_gdata_pool_size_request += generic->records_to_preallocate *
>> + generic->max_sections_per_record *
>> + generic->max_raw_data_length;
>> +
>
>Careful, I think ghes_probe() can run in parallel on different CPUs. You can
>certainly unbind/rebind it from user-space.
>
>I recall these max this/that/preallocate stuff are junk values on some
>platform.
>You'd at least need to cap it to sane maximum value.
>
>But: Using ghes_estatus_pool would use ghes_estatus_pool_init()'s sizes,
>which allocates 64K for each error source.
>
>History: https://www.spinics.net/lists/linux-acpi/msg84238.html
>
>
>> @@ -490,6 +547,68 @@ static void ghes_handle_aer(struct
>> acpi_hest_generic_data *gdata)
>
>[...]
>
>> +static void ghes_event_work_func(struct work_struct *work) {
>> + struct ghes_event_entry entry;
>> + u32 len;
>> +
>> + while (kfifo_get(&ghes_event_ring, &entry)) {
>> + blocking_notifier_call_chain(&ghes_event_notify_list,
>> + entry.error_severity,
>> + entry.gdata);
>> + len = acpi_hest_get_record_size(entry.gdata);
>> + gen_pool_free(ghes_gdata_pool, (unsigned long)entry.gdata,
>len);
>> + }
>> +}
>> +
>> +static DECLARE_WORK(ghes_event_work, ghes_event_work_func);
>> +
>> +static void ghes_handle_non_standard_event(struct
>acpi_hest_generic_data *gdata,
>> + int sev)
>> +{
>> + u32 len;
>
>> + struct ghes_event_entry event_entry;
>
>> + len = acpi_hest_get_record_size(gdata);
>> + event_entry.gdata = (void *)gen_pool_alloc(ghes_gdata_pool, len);
>> + if (event_entry.gdata) {
>> + memcpy(event_entry.gdata, gdata, len);
>> + event_entry.error_severity = sev;
>> +
>> + if (kfifo_in_spinlocked(&ghes_event_ring, &event_entry, 1,
>
>... event_entry is on the stack ...
>
Ok.
>
>> + &ghes_event_ring_lock))
>> + schedule_work(&ghes_event_work);
>> + else
>> + pr_warn(GHES_PFX "ghes event queue full\n");
>> + }
>> +}
>
>
>I think the kfifo is adding un-needed complexity here.
Ok.
>
>
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h index
>> e3f1cddb4ac8..a3dd82069069 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -50,6 +50,34 @@ enum {
>
[...]
>> +void ghes_unregister_event_notifier(struct notifier_block *nb); #else
>
>Please make it clear from the names these are for vendor events, that the
>kernel would otherwise ignore. It looks like these are for everything. Drivers
>have no business trying to handle the errors that are handled by things like
>memory_failure().
>
>~
>
>I would post a version of this to illustrate, but there are comments on patch 2
>too.
>
>Something like:
>http://www.linux-arm.org/git?p=linux-
>jm.git;a=commitdiff;h=9c6859f3146001cd9f8edfaf965232cb99c7dc42
>
>(caveat emptor: I've only build tested it)
I tested your changes and worked fine.
Should I send this patch along with the updated patch 2?
>
>
>Thanks,
>
>James
Thanks,
Shiju
Powered by blists - more mailing lists