| 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: <3f17d0a8-6b63-5792-903a-341effaae432@arm.com>
Date: Tue, 11 Oct 2016 18:28:41 +0100
From: Suzuki K Poulose <Suzuki.Poulose@....com>
To: Tyler Baicar <tbaicar@...eaurora.org>, christoffer.dall@...aro.org,
marc.zyngier@....com, pbonzini@...hat.com, rkrcmar@...hat.com,
linux@...linux.org.uk, catalin.marinas@....com,
will.deacon@....com, rjw@...ysocki.net, lenb@...nel.org,
matt@...eblueprint.co.uk, robert.moore@...el.com,
lv.zheng@...el.com, mark.rutland@....com, james.morse@....com,
akpm@...ux-foundation.org, sandeepa.s.prabhu@...il.com,
shijie.huang@....com, paul.gortmaker@...driver.com,
tomasz.nowicki@...aro.org, fu.wei@...aro.org, rostedt@...dmis.org,
bristot@...hat.com, linux-arm-kernel@...ts.infradead.org,
kvmarm@...ts.cs.columbia.edu, Dkvm@...r.kernel.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>,
Richard Ruigrok <rruigrok@...eaurora.org>,
Naveen Kaje <nkaje@...eaurora.org>
Subject: Re: [PATCH V3 02/10] ras: acpi/apei: cper: generic error data entry
v3 per ACPI 6.1
On 07/10/16 22:31, 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>
Please could you keep the people who reviewed/commented on your series in the past,
whenever you post a new version ?
> ---
> drivers/acpi/apei/ghes.c | 25 ++++++++++--
> drivers/firmware/efi/cper.c | 97 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 105 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3021f0e..c8488f1 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -80,6 +80,10 @@
> ((struct acpi_hest_generic_status *) \
> ((struct ghes_estatus_node *)(estatus_node) + 1))
>
> +#define acpi_hest_generic_data_version(gdata) \
> + (gdata->revision >> 8)
...
> +inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
> +{
> + return acpi_hest_generic_data_version(gdata) >= 3 ?
> + (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
> + gdata + 1;
> +}
> +
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..9fa1317 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> +#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 ?
> + (void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
> + gdata + 1;
> +}
Could these go to a header file, so that we don't need duplicate definitions of these helpers in
different files ?
> +
> +static void cper_estatus_print_section_v300(const char *pfx,
> + const struct acpi_hest_generic_data_v300 *gdata)
> +{
> + __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" : "");
What format is the (timestamp + 3) stored in ? Does it need conversion ?
> + printk(" %02d:%02d:%02d %02d%02d-%02d-%02d\n",
> + bcd2bin(hour), bcd2bin(min), bcd2bin(sec),
> + bcd2bin(century), bcd2bin(year), bcd2bin(mon),
> + bcd2bin(day));
> + }
minor nit: Would it be easier to order/parse the error messages if the date
is printed first followed by time ?
i.e,
17:20:14 2016-09-15 Mon
vs
2016-09-15 Mon 17:20:14
e.g, people looking at a huge log, looking for logs from a specific date might
find the latter more useful to skip the messages.
> +}
> +
> static void cper_estatus_print_section(
> - const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
> + const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
> {
> uuid_le *sec_type = (uuid_le *)gdata->section_type;
> __u16 severity;
> char newpfx[64];
>
> + if ((gdata->revision >> 8) >= 0x03)
Could we use the helper defined above ?
> @@ -451,12 +497,22 @@ void cper_estatus_print(const char *pfx,
> printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
> data_len = estatus->data_length;
> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> + if ((gdata->revision >> 8) >= 0x03)
Same as above, use the macro ?
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> +
> snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
> +
> 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;
> + if(gdata_v3) {
> + data_len -= gedata_len + sizeof(*gdata_v3);
> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> + gdata = (struct acpi_hest_generic_data *)gdata_v3;
> + } else {
> + data_len -= gedata_len + sizeof(*gdata);
> + gdata = (void *)(gdata + 1) + gedata_len;
> + }
> sec_no++;
> }
...
>
> @@ -486,15 +543,29 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
> return rc;
> data_len = estatus->data_length;
> gdata = (struct acpi_hest_generic_data *)(estatus + 1);
> - while (data_len >= sizeof(*gdata)) {
> - gedata_len = gdata->error_data_length;
> - if (gedata_len > data_len - sizeof(*gdata))
> +
> + if ((gdata->revision >> 8) >= 0x03) {
> + gdata_v3 = (struct acpi_hest_generic_data_v300 *)gdata;
> + while (data_len >= sizeof(*gdata_v3)) {
> + gedata_len = gdata_v3->error_data_length;
> + if (gedata_len > data_len - sizeof(*gdata_v3))
> + return -EINVAL;
> + data_len -= gedata_len + sizeof(*gdata_v3);
> + gdata_v3 = (void *)(gdata_v3 + 1) + gedata_len;
> + }
> + if (data_len)
> + return -EINVAL;
> + } else {
> + while (data_len >= sizeof(*gdata)) {
> + gedata_len = gdata->error_data_length;
> + if (gedata_len > data_len - sizeof(*gdata))
> + return -EINVAL;
> + data_len -= gedata_len + sizeof(*gdata);
> + gdata = (void *)(gdata + 1) + gedata_len;
> + }
> + if (data_len)
As mentioned in the previous version, would it make sense to add some more
helpers to deal with record versions ? We seem to be doing the version switch and
code duplication at different places.
Does the following help ? Thoughts ?
#define acpi_hest_generic_data_error_length(gdata) (((struct acpi_hest_generic_data *)(gdata))->error_data_length)
#define acpi_hest_generic_data_size(gdata) \
((acpi_hest_generic_data_version(gdata) >= 3) ? \
sizeof(struct acpi_hest_generic_data_v300) : \
sizeof(struct acpi_hest_generic_data))
#define acpi_hest_generic_data_record_size(gdata)
(acpi_hest_generic_data_size(gdata) + \
acpi_hest_generic_data_error_length(gdata))
#define acpi_hest_generic_data_next(gdata) \
((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))
Suzuki
Powered by blists - more mailing lists