On Wed, Oct 16, 2013 at 08:00:38PM +0200, Borislav Petkov wrote:
> Right, the only difference I can see is that include/ras/ras_event.h
> doesn't have those below:
>
> #undef TRACE_INCLUDE_PATH
> #undef TRACE_INCLUDE_FILE
> #define TRACE_INCLUDE_PATH .
>
> Perhaps that is the problem?
>
> Gong, what is exactly the issue you're observing?

Ok, I think I know what the issue is:

Gong has

diff --git a/drivers/acpi/extlog_trace.c b/drivers/acpi/extlog_trace.c
new file mode 100644
index 000000000000..28640807fb09
--- /dev/null
+++ b/drivers/acpi/extlog_trace.c
@@ -0,0 +1,107 @@
+#include <linux/export.h>
+#include <linux/dmi.h>
+#include "extlog_trace.h"
+
+#define CREATE_TRACE_POINTS
+#define TRACE_INCLUDE_PATH ../../include/ras
+#include <ras/ras_event.h>

for the ras tracepoint although this is done already in
drivers/edac/edac_mc.c

Gong, can you try moving the CREATE_TRACE_POINTS line to a new file -
arch/x86/ras/ras.c and define it there and not anywhere else, i.e. move
it away from edac_mc.c. Does that help?

Also, see Documentation/trace/tracepoints.txt for more info.

HTH.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-10-17 02:46:44

On Wed, Oct 16, 2013 at 07:59:09PM -0700, Joe Perches wrote:
> Date: Wed, 16 Oct 2013 19:59:09 -0700
> From: Joe Perches <[email protected]>
> To: "Chen, Gong" <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected], [email protected],
> [email protected], Thomas Winischhofer
> <[email protected]>, Jean-Christophe Plagniol-Villard
> <[email protected]>, Tomi Valkeinen <[email protected]>
> Subject: Re: [PATCH v2 3/9] bitops: Introduce a more generic BITMASK macro
> X-Mailer: Evolution 3.6.4-0ubuntu1
>
> On Wed, 2013-10-16 at 10:56 -0400, Chen, Gong wrote:
> > GENMASK is used to create a contiguous bitmask([hi:lo]). It is
> > implemented twice in current kernel. One is in EDAC driver, the other
> > is in SiS/XGI FB driver. Move it to a more generic place for other
> > usage.
> []
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> []
> > @@ -10,6 +10,14 @@
> > #define BITS_TO_LONGS(nr) DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
> > #endif
> >
> > +/*
> > + * Create a contiguous bitmask starting at bit position @l and ending at
> > + * position @h. For example
> > + * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
>
> ull
>
> > + */
> > +#define GENMASK(h, l) (((U32_C(1) << ((h) - (l) + 1)) - 1) << (l))
> > +#define GENMASK_ULL(h, l) (((U64_C(1) << ((h) - (l) + 1)) - 1) << (l))
>
>
> Maybe add a
>
> BUILD_BUG_ON(__builtin_constant_p(l) && __builtin_constant_p(h) && \
> (h) < (l))
>
No, if so, users can't use variables for this macro.

Attachments:

(No filename) (1.60 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-10-17 06:59:03

by Joe Perches

[permalink] [raw]

Subject: Re: [PATCH v2 3/9] bitops: Introduce a more generic BITMASK macro

On Thu, 2013-10-17 at 02:30 -0400, Chen Gong wrote:
> On Wed, Oct 16, 2013 at 07:59:09PM -0700, Joe Perches wrote:
[]
> > Maybe add a
> >
> > BUILD_BUG_ON(__builtin_constant_p(l) && __builtin_constant_p(h) && \
> > (h) < (l))
> >
> No, if so, users can't use variables for this macro.

__builtin_constant_p checks for constants

Built-in Function: int __builtin_constant_p (exp)
You can use the built-in function __builtin_constant_p to
determine if a value is known to be constant at compile-time and
hence that GCC can perform constant-folding on expressions
involving that value. The argument of the function is the value
to test. The function returns the integer 1 if the argument is
known to be a compile-time constant and 0 if it is not known to
be a compile-time constant. A return of 0 does not indicate that
the value is not a constant, but merely that GCC cannot prove it
is a constant with the specified value of the -O option.

2013-10-17 07:53:24

On Thu, Oct 17, 2013 at 07:23:06AM -0300, Mauro Carvalho Chehab wrote:
> Date: Thu, 17 Oct 2013 07:23:06 -0300
> From: Mauro Carvalho Chehab <[email protected]>
> To: "Chen, Gong" <[email protected]>
> Cc: [email protected], [email protected], [email protected],
> [email protected], [email protected],
> [email protected], [email protected]
> Subject: Re: [PATCH v2 6/9] ACPI, APEI, CPER: Add UEFI 2.4 support for
> memory error
> X-Mailer: Claws Mail 3.9.2 (GTK+ 2.24.19; x86_64-redhat-linux-gnu)
>
> Em Wed, 16 Oct 2013 10:56:03 -0400
> "Chen, Gong" <[email protected]> escreveu:
>
> > In latest UEFI spec(by now it is 2.4) memory error definition
> > for CPER (UEFI 2.4 Appendix N Common Platform Error Record)
> > adds some new fields. These fields help people to locate
> > memory error on actual DIMM location.
> >
> > Original-author: Tony Luck <[email protected]>
> > Signed-off-by: Chen, Gong <[email protected]>
> > Reviewed-by: Borislav Petkov <[email protected]>
> > ---
> > arch/x86/kernel/cpu/mcheck/mce-apei.c | 3 +--
> > drivers/acpi/apei/cper.c | 7 ++++---
> > drivers/acpi/apei/ghes.c | 4 ++--
> > drivers/edac/ghes_edac.c | 5 ++---
> > include/linux/cper.h | 11 +++++++++--
> > 5 files changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce-apei.c b/arch/x86/kernel/cpu/mcheck/mce-apei.c
> > index cd8b166..de8b60a 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce-apei.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce-apei.c
> > @@ -42,8 +42,7 @@ void apei_mce_report_mem_error(int corrected, struct cper_sec_mem_err *mem_err)
> > struct mce m;
> >
> > /* Only corrected MC is reported */
> > - if (!corrected || !(mem_err->validation_bits &
> > - CPER_MEM_VALID_PHYSICAL_ADDRESS))
> > + if (!corrected || !(mem_err->validation_bits & CPER_MEM_VALID_PA))
> > return;
> >
> > mce_setup(&m);
> > diff --git a/drivers/acpi/apei/cper.c b/drivers/acpi/apei/cper.c
> > index eb5f6d6..946ef52 100644
> > --- a/drivers/acpi/apei/cper.c
> > +++ b/drivers/acpi/apei/cper.c
> > @@ -8,7 +8,7 @@
> > * various tables, such as ERST, BERT and HEST etc.
> > *
> > * For more information about CPER, please refer to Appendix N of UEFI
> > - * Specification version 2.3.
> > + * Specification version 2.4.
> > *
> > * This program is free software; you can redistribute it and/or
> > * modify it under the terms of the GNU General Public License version
> > @@ -191,16 +191,17 @@ static const char *cper_mem_err_type_strs[] = {
> > "memory sparing",
> > "scrub corrected error",
> > "scrub uncorrected error",
> > + "physical memory map-out event",
> > };
> >
> > static void cper_print_mem(const char *pfx, const struct cper_sec_mem_err *mem)
> > {
> > if (mem->validation_bits & CPER_MEM_VALID_ERROR_STATUS)
> > printk("%s""error_status: 0x%016llx\n", pfx, mem->error_status);
> > - if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)
> > + if (mem->validation_bits & CPER_MEM_VALID_PA)
> > printk("%s""physical_address: 0x%016llx\n",
> > pfx, mem->physical_addr);
> > - if (mem->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK)
> > + if (mem->validation_bits & CPER_MEM_VALID_PA_MASK)
> > printk("%s""physical_address_mask: 0x%016llx\n",
> > pfx, mem->physical_addr_mask);
> > if (mem->validation_bits & CPER_MEM_VALID_NODE)
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index 0db6e4f..a30bc31 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -419,7 +419,7 @@ static void ghes_handle_memory_failure(struct acpi_generic_data *gdata, int sev)
> >
> > if (sec_sev == GHES_SEV_CORRECTED &&
> > (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED) &&
> > - (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS)) {
> > + (mem_err->validation_bits & CPER_MEM_VALID_PA)) {
> > pfn = mem_err->physical_addr >> PAGE_SHIFT;
> > if (pfn_valid(pfn))
> > memory_failure_queue(pfn, 0, MF_SOFT_OFFLINE);
> > @@ -430,7 +430,7 @@ static void ghes_handle_memory_failure(struct acpi_generic_data *gdata, int sev)
> > }
> > if (sev == GHES_SEV_RECOVERABLE &&
> > sec_sev == GHES_SEV_RECOVERABLE &&
> > - mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
> > + mem_err->validation_bits & CPER_MEM_VALID_PA) {
> > pfn = mem_err->physical_addr >> PAGE_SHIFT;
> > memory_failure_queue(pfn, 0, 0);
> > }
> > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> > index bb53467..0ad797b 100644
> > --- a/drivers/edac/ghes_edac.c
> > +++ b/drivers/edac/ghes_edac.c
> > @@ -297,15 +297,14 @@ void ghes_edac_report_mem_error(struct ghes *ghes, int sev,
> > }
> >
> > /* Error address */
> > - if (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS) {
> > + if (mem_err->validation_bits & CPER_MEM_VALID_PA) {
> > e->page_frame_number = mem_err->physical_addr >> PAGE_SHIFT;
> > e->offset_in_page = mem_err->physical_addr & ~PAGE_MASK;
> > }
> >
> > /* Error grain */
> > - if (mem_err->validation_bits & CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK) {
> > + if (mem_err->validation_bits & CPER_MEM_VALID_PA_MASK)
> > e->grain = ~(mem_err->physical_addr_mask & ~PAGE_MASK);
> > - }
> >
> > /* Memory error location, mapped on e->location */
> > p = e->location;
> > diff --git a/include/linux/cper.h b/include/linux/cper.h
> > index 09ebe21..2fc0ec3 100644
> > --- a/include/linux/cper.h
> > +++ b/include/linux/cper.h
> > @@ -218,8 +218,8 @@ enum {
> > #define CPER_PROC_VALID_IP 0x1000
> >
> > #define CPER_MEM_VALID_ERROR_STATUS 0x0001
> > -#define CPER_MEM_VALID_PHYSICAL_ADDRESS 0x0002
> > -#define CPER_MEM_VALID_PHYSICAL_ADDRESS_MASK 0x0004
> > +#define CPER_MEM_VALID_PA 0x0002
> > +#define CPER_MEM_VALID_PA_MASK 0x0004
> > #define CPER_MEM_VALID_NODE 0x0008
> > #define CPER_MEM_VALID_CARD 0x0010
> > #define CPER_MEM_VALID_MODULE 0x0020
> > @@ -232,6 +232,9 @@ enum {
> > #define CPER_MEM_VALID_RESPONDER_ID 0x1000
> > #define CPER_MEM_VALID_TARGET_ID 0x2000
> > #define CPER_MEM_VALID_ERROR_TYPE 0x4000
> > +#define CPER_MEM_VALID_RANK_NUMBER 0x8000
> > +#define CPER_MEM_VALID_CARD_HANDLE 0x10000
> > +#define CPER_MEM_VALID_MODULE_HANDLE 0x20000
> >
> > #define CPER_PCIE_VALID_PORT_TYPE 0x0001
> > #define CPER_PCIE_VALID_VERSION 0x0002
> > @@ -347,6 +350,10 @@ struct cper_sec_mem_err {
> > __u64 responder_id;
> > __u64 target_id;
> > __u8 error_type;
> > + __u8 reserved;
> > + __u16 rank;
> > + __u16 mem_array_handle; /* card handle in UEFI 2.4 */
> > + __u16 mem_dev_handle; /* module handle in UEFI 2.4 */
>
> Hmm... you're adding 3 new types here and the corresponding space inside the
> structure (rank, card_handle and module_handle), but the code that parses and
> prints it is missing, at apei_mce_report_mem_error(), cper_print_mem(),
> ghes_handle_memory_failure() and ghes_edac_report_mem_error().
>
>

1. This patch is just for definition update.
2. apei_mce_report_mem_error/cper_print_mem/ghes_handle_memory_failure
finally point to apei/cper. So patch [8/9] can cover it. As for
EDAC part (ghes_edac_report_mem_error), I can add a new separate
patch to fix missed part.

> > };
> >
> > struct cper_sec_pcie {
>
>
> --
>
> Cheers,
> Mauro

Attachments:

(No filename) (7.26 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-10-17 14:48:43

On Thu, 2013-10-17 at 09:10 -0700, Tony Luck wrote:
> On Thu, Oct 17, 2013 at 1:55 AM, Joe Perches <[email protected]> wrote:
> > It's cost free to add the BUILD_BUG_ON
> > and perhaps you underestimate the runtime
> > bug checking effort,
>
> This looks OK to me. Gong: This doesn't stop people from using variables
> as arguments ... they just won't get a check for (h) < (l).

Another possibility is to swap high and low if necessary and
maybe warn when either is negative or too large for the bit width.

#define GENMASK(h, l) \
({ \
size_t high = h; \
size_t low = l; \
BUILD_BUG_ON(__builtin_constant_p(l) && \
__builtin_constant_p(h) && \
(l) > (h)); \
BUILD_BUG_ON(__builtin_constant_p(l) && \
__builtin_constant_p(h) && \
((l) >= BITS_PER_LONG || \
(h) >= BITS_PER_LONG)); \
WARN_ONCE((!__builtin_constant_p(l) && \
((l) < 0 || (l) > BITS_PER_LONG)) || \
(!__builtin_constant_p(h) && \
((h) < 0 || (h) > BITS_PER_LONG)) || \
(l) > (h), \
"GENMASK: invalid mask values: l: %u, h: %d\n", \
(l), (h)); \
if (low > high) \
swap(low, high); \
(((U32_C(1) << (high - low + 1)) - 1) << low); \
})

And maybe this should be renamed something like
#define BIT_MASK_RANGE(h, l)
or
#define BIT_MASK_SHIFTED(h, l)