Re: [PATCH 01/11] ACPI / APEI: Move the estatus queue code up, and under its own ifdef

[Borislav Petkov]

On Fri, Feb 23, 2018 at 06:02:21PM +, James Morse wrote:
> Sure. I reckon your English grammar is better than mine, is this better?:

Bah, you must be joking :-)

> | In any NMI-like handler, memory from ghes_estatus_pool is used to save
> | estatus, and added to the ghes_estatus_llist. irq_work_queue() causes
> | ghes_proc_in_irq() to run in IRQ context where each estatus in
> | ghes_estatus_llist are processed. Each NMI-like error source must grow

s/are/is/ reads better to me, for some reason :)

> | the ghes_estatus_pool to ensure memory is available.

Other than that, yap, much better!

Thx.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 01/11] ACPI / APEI: Move the estatus queue code up, and under its own ifdef

[James Morse]

Hi Borislav,

On 20/02/18 19:28, Borislav Petkov wrote:
> On Thu, Feb 15, 2018 at 06:55:56PM +, James Morse wrote:
>> +#ifdef CONFIG_HAVE_ACPI_APEI_NMI
>> +/*
>> + * While printk() now has an in_nmi() path, the handling for CPER records
>> + * does not. For example, memory_failure_queue() takes spinlocks and calls
>> + * schedule_work_on().
>> + *
>> + * So in any NMI-like handler, we allocate required memory from lock-less
>> + * memory allocator (ghes_estatus_pool), save estatus into it, put them into
>> + * lock-less list (ghes_estatus_llist), then delay printk into IRQ context 
>> via
>> + * irq_work (ghes_proc_irq_work).  ghes_estatus_size_request record
>> + * required pool size by all NMI error source.
> 
> Since you're touching this, pls correct the grammar too, while at it,
> and correct them into proper sentences.
> Also, end function names with "()".
> Also the "we" pronoun and tense sounds funny - let's make it passive.

Sure. I reckon your English grammar is better than mine, is this better?:

| In any NMI-like handler, memory from ghes_estatus_pool is used to save
| estatus, and added to the ghes_estatus_llist. irq_work_queue() causes
| ghes_proc_in_irq() to run in IRQ context where each estatus in
| ghes_estatus_llist are processed. Each NMI-like error source must grow
| the ghes_estatus_pool to ensure memory is available.



Thanks,

James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 01/11] ACPI / APEI: Move the estatus queue code up, and under its own ifdef

[Borislav Petkov]

On Thu, Feb 15, 2018 at 06:55:56PM +, James Morse wrote:
> +#ifdef CONFIG_HAVE_ACPI_APEI_NMI
> +/*
> + * While printk() now has an in_nmi() path, the handling for CPER records
> + * does not. For example, memory_failure_queue() takes spinlocks and calls
> + * schedule_work_on().
> + *
> + * So in any NMI-like handler, we allocate required memory from lock-less
> + * memory allocator (ghes_estatus_pool), save estatus into it, put them into
> + * lock-less list (ghes_estatus_llist), then delay printk into IRQ context 
> via
> + * irq_work (ghes_proc_irq_work).  ghes_estatus_size_request record
> + * required pool size by all NMI error source.

Since you're touching this, pls correct the grammar too, while at it,
and correct them into proper sentences. Also, end function names with
"()". Also the "we" pronoun and tense sounds funny - let's make it
passive.

Thx.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 01/11] ACPI / APEI: Move the estatus queue code up, and under its own ifdef

[Punit Agrawal]

Hi James,

A couple of nitpicks below.

James Morse  writes:

> To support asynchronous NMI-like notifications on arm64 we need to use
> the estatus-queue. These patches refactor it to allow multiple APEI
> notification types to use it.
>
> First we move the estatus-queue code higher in the file so that any
> notify_foo() handler can make user of it.
^
use

>
> This patch moves code around ... and makes the following trivial change:
> Freshen the dated comment above ghes_estatus_llist. printk() is no
> longer the issue, its the helpers like memory_failure_queue() that
> still aren't nmi safe.
>
> Signed-off-by: James Morse 
> ---
>  drivers/acpi/apei/ghes.c | 267 
> ---
>  1 file changed, 139 insertions(+), 128 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 1efefe919555..e42b587c509b 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -545,6 +545,16 @@ static int ghes_print_estatus(const char *pfx,
>   return 0;
>  }
>  
> +static void __ghes_panic(struct ghes *ghes)
> +{
> + __ghes_print_estatus(KERN_EMERG, ghes->generic, ghes->estatus);
> +
> + /* reboot to log the error! */
> + if (!panic_timeout)
> + panic_timeout = ghes_panic_timeout;
> + panic("Fatal hardware error!");
> +}
> +
>  /*
>   * GHES error status reporting throttle, to report more kinds of
>   * errors, instead of just most frequently occurred errors.
> @@ -672,6 +682,135 @@ static void ghes_estatus_cache_add(
>   rcu_read_unlock();
>  }
>  
> +#ifdef CONFIG_HAVE_ACPI_APEI_NMI
> +/*
> + * While printk() now has an in_nmi() path, the handling for CPER records
> + * does not. For example, memory_failure_queue() takes spinlocks and calls
> + * schedule_work_on().
> + *
> + * So in any NMI-like handler, we allocate required memory from lock-less
> + * memory allocator (ghes_estatus_pool), save estatus into it, put them into
> + * lock-less list (ghes_estatus_llist), then delay printk into IRQ context 
> via
> + * irq_work (ghes_proc_irq_work).  ghes_estatus_size_request record
> + * required pool size by all NMI error source.

I am not sure it is worth keeping specific references to printk
around. As you're refreshing the comment, I'd suggest replacing the
above reference with "...processing of error status reported by the
NMI..." or something similar.

Thanks,
Punit


[...]

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 01/11] ACPI / APEI: Move the estatus queue code up, and under its own ifdef

[James Morse]

To support asynchronous NMI-like notifications on arm64 we need to use
the estatus-queue. These patches refactor it to allow multiple APEI
notification types to use it.

First we move the estatus-queue code higher in the file so that any
notify_foo() handler can make user of it.

This patch moves code around ... and makes the following trivial change:
Freshen the dated comment above ghes_estatus_llist. printk() is no
longer the issue, its the helpers like memory_failure_queue() that
still aren't nmi safe.

Signed-off-by: James Morse 
---
 drivers/acpi/apei/ghes.c | 267 ---
 1 file changed, 139 insertions(+), 128 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 1efefe919555..e42b587c509b 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -545,6 +545,16 @@ static int ghes_print_estatus(const char *pfx,
return 0;
 }
 
+static void __ghes_panic(struct ghes *ghes)
+{
+   __ghes_print_estatus(KERN_EMERG, ghes->generic, ghes->estatus);
+
+   /* reboot to log the error! */
+   if (!panic_timeout)
+   panic_timeout = ghes_panic_timeout;
+   panic("Fatal hardware error!");
+}
+
 /*
  * GHES error status reporting throttle, to report more kinds of
  * errors, instead of just most frequently occurred errors.
@@ -672,6 +682,135 @@ static void ghes_estatus_cache_add(
rcu_read_unlock();
 }
 
+#ifdef CONFIG_HAVE_ACPI_APEI_NMI
+/*
+ * While printk() now has an in_nmi() path, the handling for CPER records
+ * does not. For example, memory_failure_queue() takes spinlocks and calls
+ * schedule_work_on().
+ *
+ * So in any NMI-like handler, we allocate required memory from lock-less
+ * memory allocator (ghes_estatus_pool), save estatus into it, put them into
+ * lock-less list (ghes_estatus_llist), then delay printk into IRQ context via
+ * irq_work (ghes_proc_irq_work).  ghes_estatus_size_request record
+ * required pool size by all NMI error source.
+ *
+ * Memory from the ghes_estatus_pool is also used with the ghes_estatus_cache
+ * to suppress frequent messages.
+ */
+static struct llist_head ghes_estatus_llist;
+static struct irq_work ghes_proc_irq_work;
+
+static void ghes_print_queued_estatus(void)
+{
+   struct llist_node *llnode;
+   struct ghes_estatus_node *estatus_node;
+   struct acpi_hest_generic *generic;
+   struct acpi_hest_generic_status *estatus;
+
+   llnode = llist_del_all(_estatus_llist);
+   /*
+* Because the time order of estatus in list is reversed,
+* revert it back to proper order.
+*/
+   llnode = llist_reverse_order(llnode);
+   while (llnode) {
+   estatus_node = llist_entry(llnode, struct ghes_estatus_node,
+  llnode);
+   estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
+   generic = estatus_node->generic;
+   ghes_print_estatus(NULL, generic, estatus);
+   llnode = llnode->next;
+   }
+}
+
+/* Save estatus for further processing in IRQ context */
+static void __process_error(struct ghes *ghes)
+{
+#ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
+   u32 len, node_len;
+   struct ghes_estatus_node *estatus_node;
+   struct acpi_hest_generic_status *estatus;
+
+   if (ghes_estatus_cached(ghes->estatus))
+   return;
+
+   len = cper_estatus_len(ghes->estatus);
+   node_len = GHES_ESTATUS_NODE_LEN(len);
+
+   estatus_node = (void *)gen_pool_alloc(ghes_estatus_pool, node_len);
+   if (!estatus_node)
+   return;
+
+   estatus_node->ghes = ghes;
+   estatus_node->generic = ghes->generic;
+   estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
+   memcpy(estatus, ghes->estatus, len);
+   llist_add(_node->llnode, _estatus_llist);
+#endif
+}
+
+static unsigned long ghes_esource_prealloc_size(
+   const struct acpi_hest_generic *generic)
+{
+   unsigned long block_length, prealloc_records, prealloc_size;
+
+   block_length = min_t(unsigned long, generic->error_block_length,
+GHES_ESTATUS_MAX_SIZE);
+   prealloc_records = max_t(unsigned long,
+generic->records_to_preallocate, 1);
+   prealloc_size = min_t(unsigned long, block_length * prealloc_records,
+ GHES_ESOURCE_PREALLOC_MAX_SIZE);
+
+   return prealloc_size;
+}
+
+static void ghes_estatus_pool_shrink(unsigned long len)
+{
+   ghes_estatus_pool_size_request -= PAGE_ALIGN(len);
+}
+
+static void ghes_proc_in_irq(struct irq_work *irq_work)
+{
+   struct llist_node *llnode, *next;
+   struct ghes_estatus_node *estatus_node;
+   struct acpi_hest_generic *generic;
+   struct acpi_hest_generic_status *estatus;
+   u32 len, node_len;
+
+   llnode = llist_del_all(_estatus_llist);
+   /*
+* Because the time order of estatus