date:20180329

Re: [PATCH] efi/cper: Remove the INDENT_SP silliness

2018-03-29 Thread Ard Biesheuvel

On 29 March 2018 at 16:14, Borislav Petkov  wrote:
> From: Borislav Petkov 
>
> A separate define just to print a space character is silly and
> completely unneeded. Remove it.
>
> Signed-off-by: Borislav Petkov 
> Cc: Ard Biesheuvel 
> Cc: linux-efi@vger.kernel.org

Acked-by: Ard Biesheuvel 

I'll pick this up after the merge window

> ---
>  drivers/firmware/efi/cper-arm.c | 6 ++
>  drivers/firmware/efi/cper.c | 6 ++
>  2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
> index 698e5c8e0c8d..502811344e81 100644
> --- a/drivers/firmware/efi/cper-arm.c
> +++ b/drivers/firmware/efi/cper-arm.c
> @@ -30,8 +30,6 @@
>  #include 
>  #include 
>
> -#define INDENT_SP  " "
> -
>  static const char * const arm_reg_ctx_strs[] = {
> "AArch32 general purpose registers",
> "AArch32 EL1 context registers",
> @@ -283,7 +281,7 @@ void cper_print_proc_arm(const char *pfx,
> pfx, proc->psci_state);
> }
>
> -   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +   snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>
> err_info = (struct cper_arm_err_info *)(proc + 1);
> for (i = 0; i < proc->err_info_num; i++) {
> @@ -310,7 +308,7 @@ void cper_print_proc_arm(const char *pfx,
> if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO) 
> {
> printk("%serror_info: 0x%016llx\n", newpfx,
>err_info->error_info);
> -   snprintf(infopfx, sizeof(infopfx), "%s%s", newpfx, 
> INDENT_SP);
> +   snprintf(infopfx, sizeof(infopfx), "%s ", newpfx);
> cper_print_arm_err_info(infopfx, err_info->type,
> err_info->error_info);
> }
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index 5a59b582c9aa..3bf0dca378a6 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -37,8 +37,6 @@
>  #include 
>  #include 
>
> -#define INDENT_SP  " "
> -
>  static char rcd_decode_str[CPER_REC_LEN];
>
>  /*
> @@ -433,7 +431,7 @@ cper_estatus_print_section(const char *pfx, struct 
> acpi_hest_generic_data *gdata
> if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
> printk("%s""fru_text: %.20s\n", pfx, gdata->fru_text);
>
> -   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +   snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
> if (guid_equal(sec_type, _SEC_PROC_GENERIC)) {
> struct cper_sec_proc_generic *proc_err = 
> acpi_hest_get_payload(gdata);
>
> @@ -510,7 +508,7 @@ void cper_estatus_print(const char *pfx,
>"It has been corrected by h/w "
>"and requires no further action");
> printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
> -   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +   snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
>
> apei_estatus_for_each_section(estatus, gdata) {
> cper_estatus_print_section(newpfx, gdata, sec_no);
> --
> 2.13.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] efi/cper: Remove the INDENT_SP silliness

2018-03-29 Thread Borislav Petkov

From: Borislav Petkov 

A separate define just to print a space character is silly and
completely unneeded. Remove it.

Signed-off-by: Borislav Petkov 
Cc: Ard Biesheuvel 
Cc: linux-efi@vger.kernel.org
---
 drivers/firmware/efi/cper-arm.c | 6 ++
 drivers/firmware/efi/cper.c | 6 ++
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/cper-arm.c b/drivers/firmware/efi/cper-arm.c
index 698e5c8e0c8d..502811344e81 100644
--- a/drivers/firmware/efi/cper-arm.c
+++ b/drivers/firmware/efi/cper-arm.c
@@ -30,8 +30,6 @@
 #include 
 #include 
 
-#define INDENT_SP  " "
-
 static const char * const arm_reg_ctx_strs[] = {
"AArch32 general purpose registers",
"AArch32 EL1 context registers",
@@ -283,7 +281,7 @@ void cper_print_proc_arm(const char *pfx,
pfx, proc->psci_state);
}
 
-   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
+   snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
 
err_info = (struct cper_arm_err_info *)(proc + 1);
for (i = 0; i < proc->err_info_num; i++) {
@@ -310,7 +308,7 @@ void cper_print_proc_arm(const char *pfx,
if (err_info->validation_bits & CPER_ARM_INFO_VALID_ERR_INFO) {
printk("%serror_info: 0x%016llx\n", newpfx,
   err_info->error_info);
-   snprintf(infopfx, sizeof(infopfx), "%s%s", newpfx, 
INDENT_SP);
+   snprintf(infopfx, sizeof(infopfx), "%s ", newpfx);
cper_print_arm_err_info(infopfx, err_info->type,
err_info->error_info);
}
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 5a59b582c9aa..3bf0dca378a6 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -37,8 +37,6 @@
 #include 
 #include 
 
-#define INDENT_SP  " "
-
 static char rcd_decode_str[CPER_REC_LEN];
 
 /*
@@ -433,7 +431,7 @@ cper_estatus_print_section(const char *pfx, struct 
acpi_hest_generic_data *gdata
if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
printk("%s""fru_text: %.20s\n", pfx, gdata->fru_text);
 
-   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
+   snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
if (guid_equal(sec_type, _SEC_PROC_GENERIC)) {
struct cper_sec_proc_generic *proc_err = 
acpi_hest_get_payload(gdata);
 
@@ -510,7 +508,7 @@ void cper_estatus_print(const char *pfx,
   "It has been corrected by h/w "
   "and requires no further action");
printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
-   snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
+   snprintf(newpfx, sizeof(newpfx), "%s ", pfx);
 
apei_estatus_for_each_section(estatus, gdata) {
cper_estatus_print_section(newpfx, gdata, sec_no);
-- 
2.13.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 3/8] efi: Decode IA32/X64 Processor Error Info Structure

2018-03-29 Thread Ghannam, Yazen

> -Original Message-
> From: Borislav Petkov 
> Sent: Thursday, March 29, 2018 6:55 AM
> To: Ghannam, Yazen 
> Cc: linux-efi@vger.kernel.org; linux-ker...@vger.kernel.org;
> ard.biesheu...@linaro.org; x...@kernel.org; tony.l...@intel.com
> Subject: Re: [PATCH v3 3/8] efi: Decode IA32/X64 Processor Error Info
> Structure
> 
> On Sat, Mar 24, 2018 at 01:49:35PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam 
> >
> > Print the fields in the IA32/X64 Processor Error Info Structure.
> >
> > Based on UEFI 2.7 Table 253. IA32/X64 Processor Error Information
> > Structure.
> >
> > Signed-off-by: Yazen Ghannam 
> > ---
> > Link:
> > https://lkml.kernel.org/r/20180226193904.20532-4-
> yazen.ghan...@amd.com
> >
> > v2->v3:
> > * Fix table number in commit message.
> > * Don't print raw validation bits.
> >
> > v1->v2:
> > * Add parantheses around "bits" expression in macro.
> > * Fix indentation on multi-line statements.
> >
> >  drivers/firmware/efi/cper-x86.c | 50
> +
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
> x86.c
> > index 863f0cd2a0ff..a9ab3bbf7986 100644
> > --- a/drivers/firmware/efi/cper-x86.c
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -3,15 +3,28 @@
> >
> >  #include 
> >
> > +#define INDENT_SP  " "
> 
> There's that thing again. So it was a total waste of time discussing
> this last time. So let me save my time this time:
> 
> NAKed-by: Borislav Petkov 
> 

IIRC, the arguments for keeping this are
1) convention for CPER
2) code readability

The argument against was
1) it's dumb

So I decided to keep it. I don't really mind either way so I'll change it
if there's a second opinion.

Thanks,
Yazen

RE: [PATCH v3 2/8] efi: Decode IA32/X64 Processor Error Section

2018-03-29 Thread Ghannam, Yazen

> -Original Message-
> From: Borislav Petkov 
> Sent: Thursday, March 29, 2018 5:46 AM
> To: Ghannam, Yazen 
> Cc: linux-efi@vger.kernel.org; linux-ker...@vger.kernel.org;
> ard.biesheu...@linaro.org; x...@kernel.org; tony.l...@intel.com
> Subject: Re: [PATCH v3 2/8] efi: Decode IA32/X64 Processor Error Section
> 
> On Sat, Mar 24, 2018 at 01:49:34PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam 
> >
> > Recognize the IA32/X64 Processor Error Section.
> >
> > Do the section decoding in a new "cper-x86.c" file and add this to the
> > Makefile depending on a new "UEFI_CPER_X86" config option.
> >
> > Print the Local APIC ID and CPUID info from the Processor Error Record.
> >
> > The "Processor Error Info" and "Processor Context" fields will be
> > decoded in following patches.
> >
> > Based on UEFI 2.7 Table 252. Processor Error Record.
> >
> > Signed-off-by: Yazen Ghannam 
> > ---
> > Link:
> > https://lkml.kernel.org/r/20180226193904.20532-3-
> yazen.ghan...@amd.com
> >
> > v2->v3:
> > * Fix table number in commit message.
> > * Don't print raw validation bits.
> >
> > v1->v2:
> > * Change config option depends to "X86" instead of "X86_32 || X64_64".
> > * Remove extra newline in Makefile changes.
> > * Drop author copyright line.
> >
> >  drivers/firmware/efi/Kconfig|  5 +
> >  drivers/firmware/efi/Makefile   |  1 +
> >  drivers/firmware/efi/cper-x86.c | 23 +++
> 
> I'd prefer if that were:
> 
> drivers/firmware/efi/cper/x86.c
> drivers/firmware/efi/cper/arm.c
> 
> but that's Ard's call.
> 
> >  drivers/firmware/efi/cper.c | 10 ++
> >  include/linux/cper.h|  2 ++
> >  5 files changed, 41 insertions(+)
> >  create mode 100644 drivers/firmware/efi/cper-x86.c
> >
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 3098410abad8..781a4a337557 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -174,6 +174,11 @@ config UEFI_CPER_ARM
> > depends on UEFI_CPER && ( ARM || ARM64 )
> > default y
> >
> > +config UEFI_CPER_X86
> > +   bool
> > +   depends on UEFI_CPER && X86
> > +   default y
> > +
> >  config EFI_DEV_PATH_PARSER
> > bool
> > depends on ACPI
> > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > index cb805374f4bc..5f9f5039de50 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -31,3 +31,4 @@ obj-$(CONFIG_ARM) += $(arm-obj-
> y)
> >  obj-$(CONFIG_ARM64)+= $(arm-obj-y)
> >  obj-$(CONFIG_EFI_CAPSULE_LOADER)   += capsule-loader.o
> >  obj-$(CONFIG_UEFI_CPER_ARM)+= cper-arm.o
> > +obj-$(CONFIG_UEFI_CPER_X86)+= cper-x86.o
> > diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-
> x86.c
> > new file mode 100644
> > index ..863f0cd2a0ff
> > --- /dev/null
> > +++ b/drivers/firmware/efi/cper-x86.c
> > @@ -0,0 +1,23 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2018, Advanced Micro Devices, Inc.
> > +
> > +#include 
> > +
> > +/*
> > + * We don't need a "CPER_IA" prefix since these are all locally defined.
> > + * This will save us a lot of line space.
> > + */
> > +#define VALID_LAPIC_ID BIT_ULL(0)
> > +#define VALID_CPUID_INFO   BIT_ULL(1)
> > +
> > +void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia
> *proc)
> > +{
> > +   if (proc->validation_bits & VALID_LAPIC_ID)
> > +   printk("%sLocal APIC_ID: 0x%llx\n", pfx, proc->lapic_id);
> > +
> > +   if (proc->validation_bits & VALID_CPUID_INFO) {
> > +   printk("%sCPUID Info:\n", pfx);
> > +   print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc-
> >cpuid,
> > +  sizeof(proc->cpuid), 0);
> 
> That's dumping 48 bytes of static information for every error!
> Information which can be dumped only once when collecting system info.
> 

Right, but here we just print the record as-is. We can do it different when
this hooks into the MCA decoding. And the use-case at the moment is for
BERT which shouldn't have very many errors.

Thanks,
Yazen

Re: [PATCH v3 3/8] efi: Decode IA32/X64 Processor Error Info Structure

2018-03-29 Thread Borislav Petkov

On Sat, Mar 24, 2018 at 01:49:35PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam 
> 
> Print the fields in the IA32/X64 Processor Error Info Structure.
> 
> Based on UEFI 2.7 Table 253. IA32/X64 Processor Error Information
> Structure.
> 
> Signed-off-by: Yazen Ghannam 
> ---
> Link:
> https://lkml.kernel.org/r/20180226193904.20532-4-yazen.ghan...@amd.com
> 
> v2->v3:
> * Fix table number in commit message.
> * Don't print raw validation bits.
> 
> v1->v2:
> * Add parantheses around "bits" expression in macro.
> * Fix indentation on multi-line statements.
> 
>  drivers/firmware/efi/cper-x86.c | 50 
> +
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> index 863f0cd2a0ff..a9ab3bbf7986 100644
> --- a/drivers/firmware/efi/cper-x86.c
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -3,15 +3,28 @@
>  
>  #include 
>  
> +#define INDENT_SP" "

There's that thing again. So it was a total waste of time discussing
this last time. So let me save my time this time:

NAKed-by: Borislav Petkov 

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/8] efi: Decode IA32/X64 Processor Error Section

2018-03-29 Thread Borislav Petkov

On Sat, Mar 24, 2018 at 01:49:34PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam 
> 
> Recognize the IA32/X64 Processor Error Section.
> 
> Do the section decoding in a new "cper-x86.c" file and add this to the
> Makefile depending on a new "UEFI_CPER_X86" config option.
> 
> Print the Local APIC ID and CPUID info from the Processor Error Record.
> 
> The "Processor Error Info" and "Processor Context" fields will be
> decoded in following patches.
> 
> Based on UEFI 2.7 Table 252. Processor Error Record.
> 
> Signed-off-by: Yazen Ghannam 
> ---
> Link:
> https://lkml.kernel.org/r/20180226193904.20532-3-yazen.ghan...@amd.com
> 
> v2->v3:
> * Fix table number in commit message.
> * Don't print raw validation bits.
> 
> v1->v2:
> * Change config option depends to "X86" instead of "X86_32 || X64_64".
> * Remove extra newline in Makefile changes.
> * Drop author copyright line.
> 
>  drivers/firmware/efi/Kconfig|  5 +
>  drivers/firmware/efi/Makefile   |  1 +
>  drivers/firmware/efi/cper-x86.c | 23 +++

I'd prefer if that were:

drivers/firmware/efi/cper/x86.c
drivers/firmware/efi/cper/arm.c

but that's Ard's call.

>  drivers/firmware/efi/cper.c | 10 ++
>  include/linux/cper.h|  2 ++
>  5 files changed, 41 insertions(+)
>  create mode 100644 drivers/firmware/efi/cper-x86.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 3098410abad8..781a4a337557 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -174,6 +174,11 @@ config UEFI_CPER_ARM
>   depends on UEFI_CPER && ( ARM || ARM64 )
>   default y
>  
> +config UEFI_CPER_X86
> + bool
> + depends on UEFI_CPER && X86
> + default y
> +
>  config EFI_DEV_PATH_PARSER
>   bool
>   depends on ACPI
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index cb805374f4bc..5f9f5039de50 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_ARM)   += $(arm-obj-y)
>  obj-$(CONFIG_ARM64)  += $(arm-obj-y)
>  obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
>  obj-$(CONFIG_UEFI_CPER_ARM)  += cper-arm.o
> +obj-$(CONFIG_UEFI_CPER_X86)  += cper-x86.o
> diff --git a/drivers/firmware/efi/cper-x86.c b/drivers/firmware/efi/cper-x86.c
> new file mode 100644
> index ..863f0cd2a0ff
> --- /dev/null
> +++ b/drivers/firmware/efi/cper-x86.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018, Advanced Micro Devices, Inc.
> +
> +#include 
> +
> +/*
> + * We don't need a "CPER_IA" prefix since these are all locally defined.
> + * This will save us a lot of line space.
> + */
> +#define VALID_LAPIC_ID   BIT_ULL(0)
> +#define VALID_CPUID_INFO BIT_ULL(1)
> +
> +void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia *proc)
> +{
> + if (proc->validation_bits & VALID_LAPIC_ID)
> + printk("%sLocal APIC_ID: 0x%llx\n", pfx, proc->lapic_id);
> +
> + if (proc->validation_bits & VALID_CPUID_INFO) {
> + printk("%sCPUID Info:\n", pfx);
> + print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc->cpuid,
> +sizeof(proc->cpuid), 0);

That's dumping 48 bytes of static information for every error!
Information which can be dumped only once when collecting system info.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] efi/cper: Remove the INDENT_SP silliness

[PATCH] efi/cper: Remove the INDENT_SP silliness

RE: [PATCH v3 3/8] efi: Decode IA32/X64 Processor Error Info Structure

RE: [PATCH v3 2/8] efi: Decode IA32/X64 Processor Error Section

Re: [PATCH v3 3/8] efi: Decode IA32/X64 Processor Error Info Structure

Re: [PATCH v3 2/8] efi: Decode IA32/X64 Processor Error Section

6 matches

Site Navigation

Mail list logo

Footer information