| 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: <570F6F76.3060804@arm.com>
Date: Thu, 14 Apr 2016 11:22:46 +0100
From: Suzuki K Poulose <Suzuki.Poulose@....com>
To: Tyler Baicar <tbaicar@...eaurora.org>, fu.wei@...aro.org,
timur@...eaurora.org, harba@...eaurora.org,
rruigrok@...eaurora.org, ahs3@...hat.com, catalin.marinas@....com,
will.deacon@....com, rjw@...ysocki.net, lenb@...nel.org,
matt@...eblueprint.co.uk, robert.moore@...el.com,
lv.zheng@...el.com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-efi@...r.kernel.org, devel@...ica.org
Cc: "Jonathan (Zhixiong) Zhang" <zjzhang@...eaurora.org>,
Naveen Kaje <nkaje@...eaurora.org>
Subject: Re: [PATCH V2 2/9] ras: acpi/apei: cper: generic error data entry v3
per ACPI 6.1
On 06/04/16 16:12, Tyler Baicar wrote:
> Currently when a RAS error is reported it is not timestamped.
> The ACPI 6.1 spec adds the timestamp field to the generic error
> data entry v3 structure. The timestamp of when the firmware
> generated the error is now being reported.
>
> Signed-off-by: Jonathan (Zhixiong) Zhang <zjzhang@...eaurora.org>
> Signed-off-by: Richard Ruigrok <rruigrok@...eaurora.org>
> Signed-off-by: Tyler Baicar <tbaicar@...eaurora.org>
> Signed-off-by: Naveen Kaje <nkaje@...eaurora.org>
> ---
> drivers/acpi/apei/ghes.c | 35 ++++++++++++--
> drivers/firmware/efi/cper.c | 111 +++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 126 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 9b0543e..a6848706 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -419,7 +419,15 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
> int flags = -1;
> int sec_sev = ghes_severity(gdata->error_severity);
> struct cper_sec_mem_err *mem_err;
> - mem_err = (struct cper_sec_mem_err *)(gdata + 1);
> + struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
> +
> + if ((gdata->revision >> 8) >= 0x03)
Could we please make that a macro ? We seem to be using the check everywhere.
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> +
> + if (gdata_v3)
> + mem_err = (struct cper_sec_mem_err *)(gdata_v3 + 1);
> + else
> + mem_err = (struct cper_sec_mem_err *)(gdata + 1);
>
> if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
> return;
> @@ -449,14 +457,27 @@ static void ghes_do_proc(struct ghes *ghes,
> {
> int sev, sec_sev;
> struct acpi_hest_generic_data *gdata;
> + struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
> + uuid_le sec_type;
>
> sev = ghes_severity(estatus->error_severity);
> apei_estatus_for_each_section(estatus, gdata) {
> sec_sev = ghes_severity(gdata->error_severity);
> - if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
> + sec_type = *(uuid_le *)gdata->section_type;
> +
> + if ((gdata->revision >> 8) >= 0x03)
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> +
> + if (!uuid_le_cmp(sec_type,
> CPER_SEC_PLATFORM_MEM)) {
> struct cper_sec_mem_err *mem_err;
> - mem_err = (struct cper_sec_mem_err *)(gdata+1);
> +
> + if (gdata_v3)
> + mem_err = (struct cper_sec_mem_err *)
> + (gdata_v3 + 1);
> + else
> + mem_err = (struct cper_sec_mem_err *)
> + (gdata + 1);
> ghes_edac_report_mem_error(ghes, sev, mem_err);
>
> arch_apei_report_mem_error(sev, mem_err);
> @@ -466,7 +487,13 @@ static void ghes_do_proc(struct ghes *ghes,
> else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
> CPER_SEC_PCIE)) {
> struct cper_sec_pcie *pcie_err;
> - pcie_err = (struct cper_sec_pcie *)(gdata+1);
> +
> + if (gdata_v3)
> + pcie_err = (struct cper_sec_pcie *)
> + (gdata_v3 + 1);
> + else
> + pcie_err = (struct cper_sec_pcie *)
> + (gdata + 1);
> if (sev == GHES_SEV_RECOVERABLE &&
> sec_sev == GHES_SEV_RECOVERABLE &&
> pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..23f62962 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -32,6 +32,8 @@
> #include <linux/acpi.h>
> #include <linux/pci.h>
> #include <linux/aer.h>
> +#include <linux/printk.h>
> +#include <linux/bcd.h>
>
> #define INDENT_SP " "
>
> @@ -392,6 +394,10 @@ static void cper_estatus_print_section(
> uuid_le *sec_type = (uuid_le *)gdata->section_type;
> __u16 severity;
> char newpfx[64];
> + struct acpi_hest_generic_data_v300 *gdata_v3 = NULL;
> +
> + if ((gdata->revision >> 8) >= 0x03)
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
>
> severity = gdata->error_severity;
> printk("%s""Error %d, type: %s\n", pfx, sec_no,
> @@ -403,14 +409,24 @@ static void cper_estatus_print_section(
>
> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
> - struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1);
> + struct cper_sec_proc_generic *proc_err;
> +
> + if (gdata_v3)
> + proc_err = (void *)(gdata_v3 + 1);
> + else
> + proc_err = (void *)(gdata + 1);
> printk("%s""section_type: general processor error\n", newpfx);
> if (gdata->error_data_length >= sizeof(*proc_err))
> cper_print_proc_generic(newpfx, proc_err);
> else
> goto err_section_too_small;
> } else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
> - struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
> + struct cper_sec_mem_err *mem_err;
> +
> + if (gdata_v3)
> + mem_err = (void *)(gdata_v3 + 1);
> + else
> + mem_err = (void *)(gdata + 1);
> printk("%s""section_type: memory error\n", newpfx);
> if (gdata->error_data_length >=
> sizeof(struct cper_sec_mem_err_old))
> @@ -419,7 +435,12 @@ static void cper_estatus_print_section(
> else
> goto err_section_too_small;
> } else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
> - struct cper_sec_pcie *pcie = (void *)(gdata + 1);
> + struct cper_sec_pcie *pcie;
> +
> + if (gdata_v3)
> + pcie = (void *)(gdata_v3 + 1);
> + else
> + pcie = (void *)(gdata + 1);
The only use of the gdata_v3 above cases is to get the payload(or error_record).
So instead of spilling these checks all over could we use something like :
#define acpi_hest_generic_data_version(gdata) \
(gdata->revision >> 8)
static inline void *
acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
{
return acpi_hest_generic_data_version(gdata) >= 3 ?
((struct acpi_hest_generic_data_v300 *)(gdata)) + 1 :
gdata + 1;
}
And then do :
void *payload = acpi_hest_generic_data_payload(gdata);
> printk("%s""section_type: PCIe error\n", newpfx);
> if (gdata->error_data_length >= sizeof(*pcie))
> cper_print_pcie(newpfx, pcie, gdata);
> @@ -434,10 +455,38 @@ err_section_too_small:
> pr_err(FW_WARN "error section length is too small\n");
> }
>
> +static void cper_estatus_print_section_v300(const char *pfx,
> + const struct acpi_hest_generic_data_v300 *gdata, int sec_no)
> +{
> + __u8 hour, min, sec, day, mon, year, century, *timestamp;
> +
> + if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
> + timestamp = (__u8 *)&(gdata->time_stamp);
> + memcpy(&sec, timestamp, 1);
> + memcpy(&min, timestamp + 1, 1);
> + memcpy(&hour, timestamp + 2, 1);
> + memcpy(&day, timestamp + 4, 1);
> + memcpy(&mon, timestamp + 5, 1);
> + memcpy(&year, timestamp + 6, 1);
> + memcpy(¢ury, timestamp + 7, 1);
> + printk("%stime: ", pfx);
> + printk("%7s", 0x01 & *(timestamp + 3) ? "precise" : "");
> + printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> + bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
> + bcd2bin(century), bcd2bin(year), bcd2bin(mon),
> + bcd2bin(day));
> + }
> +
> + cper_estatus_print_section(pfx,
> + (const struct acpi_hest_generic_data *)gdata,
> + sec_no);
> +}
Wouldn't it be better do the v3 header check from cper_erstatus_print_section() and call out
to cper_erstatus_print_section_v300() ? That way, we can leave the callers unaffected,
even for future changes.
> + if (gdata_v3) {
> + while (data_len >= sizeof(*gdata_v3)) {
> + gedata_len = gdata_v3->error_data_length;
> + cper_estatus_print_section_v300(newpfx, gdata_v3,
> + sec_no);
> + data_len -= gedata_len + sizeof(*gdata_v3);
> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> + sec_no++;
> + }
> + } else {
> + while (data_len >= sizeof(*gdata)) {
> + gedata_len = gdata->error_data_length;
> + cper_estatus_print_section(newpfx, gdata, sec_no);
> + data_len -= gedata_len + sizeof(*gdata);
> + gdata = (void *)(gdata + 1) + gedata_len;
> + sec_no++;
> + }
With the change mentioned above and storing the sizeof(), we could make this
hunk a bit more cleaner.
Suzuki
Powered by blists - more mailing lists