Message ID | 1488493596-3437-9-git-send-email-supreeth.venkatesh@arm.com |
---|---|
State | Superseded |
Headers | show |
I'm getting white space issues when I apply the patch: Applying: sbbr/rsdp: Add initial rsdp acpi tests as per sbbr. .git/rebase-apply/patch:120: space before tab in indent. fwts_passed(fw, "SBBR: Structure of RSDP Table is consistent with ACPI 6.0 or later and uses 64 bit xsdt addresses."); .git/rebase-apply/patch:124: space before tab in indent. fwts_failed(fw, LOG_LEVEL_CRITICAL, .git/rebase-apply/patch:125: space before tab in indent. "SBBR RSDP:", .git/rebase-apply/patch:126: space before tab in indent. "Structure of RSDP Table is not consistent with ACPI 6.0 or later and/or does not use 64 bit xsdt addresses."); warning: 4 lines add whitespace errors. On 02/03/17 22:26, Supreeth Venkatesh wrote: > Server Base Boot Requirements (SBBR) specification is intended for SBSA- > compliant 64-bit ARMv8 servers. > It defines the base firmware requirements for out-of-box support of any > ARM SBSA-compatible Operating System or hypervisor. > The requirements in this specification are expected to be minimal yet > complete for booting a multi-core ARMv8 server platform, while leaving > plenty of room for OEM or ODM innovations and design details. > For more information, download the SBBR specification here: > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html > > This change introduces test cases as per SBBR specification to rsdp > acpi. These test cases may be subset/superset of rsdp acpi tests already > existing. However, to preserve "sbbr" classification, new file is > created, even when most of the code is re-used from acpi/rsdp. > > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > --- > src/sbbr/rsdp/rsdp.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 134 insertions(+) > create mode 100644 src/sbbr/rsdp/rsdp.c > > diff --git a/src/sbbr/rsdp/rsdp.c b/src/sbbr/rsdp/rsdp.c > new file mode 100644 > index 0000000..0a11f91 > --- /dev/null > +++ b/src/sbbr/rsdp/rsdp.c > @@ -0,0 +1,134 @@ > +/* > + * Copyright (C) 2015-2017 Canonical > + * Copyright (C) 2017 ARM Ltd > + * > + * Portions of this code original from the Linux-ready Firmware Developer Kit > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version 2 > + * of the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > +#include "fwts.h" > + > +#if defined(FWTS_HAS_SBBR) > + > +#include <stdlib.h> > +#include <stdio.h> > +#include <unistd.h> > +#include <inttypes.h> > +#include <string.h> > +#include <ctype.h> > + > +static fwts_acpi_table_info *table; > + > +static int sbbr_rsdp_init(fwts_framework *fw) > +{ > + if (fwts_acpi_find_table(fw, "RSDP", 0, &table) != FWTS_OK) { > + fwts_log_error(fw, "Cannot read ACPI tables."); > + return FWTS_ERROR; > + } > + if (!table) { > + > + fwts_log_error(fw, > + "ACPI RSDP is required for the " > + "%s target architecture.", > + fwts_arch_get_name(fw->target_arch)); > + return FWTS_ERROR; > + } formatting has extra tabs, please clean that up. > + > + /* We know there is an RSDP now, so do a quick sanity check */ > + if (table->length == 0) { > + fwts_log_error(fw, > + "ACPI RSDP table has zero length"); > + return FWTS_ERROR; > + } > + return FWTS_OK; > +} > + > +/* > + * RSDP Root System Description Pointer > + */ > +static int sbbr_rsdp_test1(fwts_framework *fw) > +{ > + fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp *)table->data; > + uint8_t checksum; > + > + /*This includes only the first 20 bytes of this table, bytes > + 0 to 19, including the checksum field. These bytes must sum to > + zero. */ I'd prefer if the comments in the code (and other patches) would match the general fwts block style, e.g. /* * This includes only the first 20 bytes of this table, bytes * 0 to 19, including the checksum field. These bytes must * sum to zero. */ > + const char RSDP_SIGNATURE[] = {'R', 'S', 'D', ' ', 'P', 'T', 'R', ' '}; > + bool signature_pass = false; you don't need to initialize the above boolean > + const int CHECKSUM_BYTES = 20; > + bool checksum_pass = false; ..and this one ^ > + const int SBBR_RSDP_REVISION = 2; > + bool rsdp_revision_pass = false; ..and this one ^ > + const uint32_t SBBR_RSDP_LENGTH = 36; > + bool rsdp_length_pass = false; ..and this one ^ > + const int EXT_CHECKSUM_BYTES = 36; > + bool ext_checksum_pass = false; > + bool xsdt_address_pass = false; please don't make const variables all caps. lowercase is the convention. > + > + fwts_log_info(fw, "RSDP Signature = %.8s", rsdp->signature); > + signature_pass = strncmp(rsdp->signature, RSDP_SIGNATURE, sizeof(rsdp->signature))? false : true; quite a long line, can it be less than 80 cols wide > + > + /* verify first checksum */ > + checksum = fwts_checksum(table->data, CHECKSUM_BYTES); > + fwts_log_info(fw, "RSDP Checksum = 0x%x", checksum); > + checksum_pass = (checksum == 0)? true : false; this is better: checksum_pass = (checksum == 0); > + > + fwts_log_info(fw, "RSDP Revision = 0x%x", rsdp->revision); > + rsdp_revision_pass = (rsdp->revision >= SBBR_RSDP_REVISION)? true : false; space please between ) and ? ... SBBR_RSDP_REVISION) ? true : false; > + > + fwts_log_info(fw, "RSDP Length = 0x%x", rsdp->length); > + rsdp_length_pass = (rsdp->length == SBBR_RSDP_LENGTH)? true : false; space between )? > + > + checksum = fwts_checksum(table->data, EXT_CHECKSUM_BYTES); > + fwts_log_info(fw, "RSDP Extended Checksum = 0x%x", checksum); > + ext_checksum_pass = (checksum == 0)? true : false; space between )? > + > + if ( (rsdp->xsdt_address != 0) && > + (rsdp->rsdt_address == 0) ) > + { > + xsdt_address_pass = true; > + } format the above as follows is preferred: if ((rsdp->xsdt_address != 0) && (rsdp->rsdt_address == 0)) xsdt_address_pass = true; > + > + if ( (signature_pass == true) && > + (checksum_pass == true) && > + (rsdp_revision_pass == true) && > + (rsdp_length_pass == true) && > + (ext_checksum_pass == true) && > + (xsdt_address_pass == true) ) > + { > + fwts_passed(fw, "SBBR: Structure of RSDP Table is consistent with ACPI 6.0 or later and uses 64 bit xsdt addresses."); > + } > + else > + { > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > + "SBBR RSDP:", > + "Structure of RSDP Table is not consistent with ACPI 6.0 or later and/or does not use 64 bit xsdt addresses."); > + } > + The formatting style above needs reworking, if (( ... ) && ( ... ) && .... ( ... )) fwts_passed(...) else fwts_failed(...) > + return FWTS_OK; > +} > + > +static fwts_framework_minor_test sbbr_rsdp_tests[] = { > + { sbbr_rsdp_test1, "RSDP Root System Description Pointer test." }, > + { NULL, NULL } > +}; > + > +static fwts_framework_ops sbbr_rsdp_ops = { > + .description = "SBBR RSDP Root System Description Pointer tests.", > + .init = sbbr_rsdp_init, > + .minor_tests = sbbr_rsdp_tests > +}; > + > +FWTS_REGISTER("sbbr_rsdp", &sbbr_rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_SBBR) > + > +#endif >
On Thu, 2017-03-02 at 23:52 +0000, Colin Ian King wrote: > I'm getting white space issues when I apply the patch: > > Applying: sbbr/rsdp: Add initial rsdp acpi tests as per sbbr. > .git/rebase-apply/patch:120: space before tab in indent. > fwts_passed(fw, "SBBR: Structure of RSDP Table is > consistent with > ACPI 6.0 or later and uses 64 bit xsdt addresses."); > .git/rebase-apply/patch:124: space before tab in indent. > fwts_failed(fw, LOG_LEVEL_CRITICAL, > .git/rebase-apply/patch:125: space before tab in indent. > "SBBR RSDP:", > .git/rebase-apply/patch:126: space before tab in indent. > "Structure of RSDP Table is not > consistent with ACPI 6.0 or > later and/or does not use 64 bit xsdt addresses."); > warning: 4 lines add whitespace errors. > Sorry, I may have messed up. Do you have recommendation like patchcheck.py or something similar which checks style and whitespace issues to check patches before sending it out? > On 02/03/17 22:26, Supreeth Venkatesh wrote: > > > > Server Base Boot Requirements (SBBR) specification is intended for > > SBSA- > > compliant 64-bit ARMv8 servers. > > It defines the base firmware requirements for out-of-box support of > > any > > ARM SBSA-compatible Operating System or hypervisor. > > The requirements in this specification are expected to be minimal > > yet > > complete for booting a multi-core ARMv8 server platform, while > > leaving > > plenty of room for OEM or ODM innovations and design details. > > For more information, download the SBBR specification here: > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044 > > b/index.html > > > > This change introduces test cases as per SBBR specification to rsdp > > acpi. These test cases may be subset/superset of rsdp acpi tests > > already > > existing. However, to preserve "sbbr" classification, new file is > > created, even when most of the code is re-used from acpi/rsdp. > > > > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com> > > --- > > src/sbbr/rsdp/rsdp.c | 134 > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 134 insertions(+) > > create mode 100644 src/sbbr/rsdp/rsdp.c > > > > diff --git a/src/sbbr/rsdp/rsdp.c b/src/sbbr/rsdp/rsdp.c > > new file mode 100644 > > index 0000000..0a11f91 > > --- /dev/null > > +++ b/src/sbbr/rsdp/rsdp.c > > @@ -0,0 +1,134 @@ > > +/* > > + * Copyright (C) 2015-2017 Canonical > > + * Copyright (C) 2017 ARM Ltd > > + * > > + * Portions of this code original from the Linux-ready Firmware > > Developer Kit > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; either version 2 > > + * of the License, or (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + */ > > +#include "fwts.h" > > + > > +#if defined(FWTS_HAS_SBBR) > > + > > +#include <stdlib.h> > > +#include <stdio.h> > > +#include <unistd.h> > > +#include <inttypes.h> > > +#include <string.h> > > +#include <ctype.h> > > + > > +static fwts_acpi_table_info *table; > > + > > +static int sbbr_rsdp_init(fwts_framework *fw) > > +{ > > + if (fwts_acpi_find_table(fw, "RSDP", 0, &table) != > > FWTS_OK) { > > + fwts_log_error(fw, "Cannot read ACPI tables."); > > + return FWTS_ERROR; > > + } > > + if (!table) { > > + > > + fwts_log_error(fw, > > + "ACPI RSDP is required for > > the " > > + "%s target architecture.", > > + fwts_arch_get_name(fw- > > >target_arch)); > > + return FWTS_ERROR; > > + } > formatting has extra tabs, please clean that up. > > > > > + > > + /* We know there is an RSDP now, so do a quick sanity > > check */ > > + if (table->length == 0) { > > + fwts_log_error(fw, > > + "ACPI RSDP table has zero length"); > > + return FWTS_ERROR; > > + } > > + return FWTS_OK; > > +} > > + > > +/* > > + * RSDP Root System Description Pointer > > + */ > > +static int sbbr_rsdp_test1(fwts_framework *fw) > > +{ > > + fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp > > *)table->data; > > + uint8_t checksum; > > + > > + /*This includes only the first 20 bytes of this table, > > bytes > > + 0 to 19, including the checksum field. These bytes must > > sum to > > + zero. */ > I'd prefer if the comments in the code (and other patches) would > match > the general fwts block style, e.g. > > /* > * This includes only the first 20 bytes of this table, bytes > * 0 to 19, including the checksum field. These bytes must > * sum to zero. > */ > > > > > + const char RSDP_SIGNATURE[] = {'R', 'S', 'D', ' ', 'P', > > 'T', 'R', ' '}; > > + bool signature_pass = false; > you don't need to initialize the above boolean > > > > > + const int CHECKSUM_BYTES = 20; > > + bool checksum_pass = false; > ..and this one ^ > > > > > + const int SBBR_RSDP_REVISION = 2; > > + bool rsdp_revision_pass = false; > ..and this one ^ > > > > > + const uint32_t SBBR_RSDP_LENGTH = 36; > > + bool rsdp_length_pass = false; > ..and this one ^ > > > > > + const int EXT_CHECKSUM_BYTES = 36; > > + bool ext_checksum_pass = false; > > + bool xsdt_address_pass = false; > please don't make const variables all caps. lowercase is the > convention. > > > > > + > > + fwts_log_info(fw, "RSDP Signature = %.8s", rsdp- > > >signature); > > + signature_pass = strncmp(rsdp->signature, RSDP_SIGNATURE, > > sizeof(rsdp->signature))? false : true; > quite a long line, can it be less than 80 cols wide > > > > > + > > + /* verify first checksum */ > > + checksum = fwts_checksum(table->data, CHECKSUM_BYTES); > > + fwts_log_info(fw, "RSDP Checksum = 0x%x", checksum); > > + checksum_pass = (checksum == 0)? true : false; > this is better: > > checksum_pass = (checksum == 0); > > > > > + > > + fwts_log_info(fw, "RSDP Revision = 0x%x", rsdp->revision); > > + rsdp_revision_pass = (rsdp->revision >= > > SBBR_RSDP_REVISION)? true : false; > space please between ) and ? > ... SBBR_RSDP_REVISION) ? true : false; > > > > + > > + fwts_log_info(fw, "RSDP Length = 0x%x", rsdp->length); > > + rsdp_length_pass = (rsdp->length == SBBR_RSDP_LENGTH)? > > true : false; > space between )? > > > > > + > > + checksum = fwts_checksum(table->data, EXT_CHECKSUM_BYTES); > > + fwts_log_info(fw, "RSDP Extended Checksum = 0x%x", > > checksum); > > + ext_checksum_pass = (checksum == 0)? true : false; > space between )? > > > > + > > + if ( (rsdp->xsdt_address != 0) && > > + (rsdp->rsdt_address == 0) ) > > + { > > + xsdt_address_pass = true; > > + } > format the above as follows is preferred: > > if ((rsdp->xsdt_address != 0) && > (rsdp->rsdt_address == 0)) > xsdt_address_pass = true; > > > > > + > > + if ( (signature_pass == true) && > > + (checksum_pass == true) && > > + (rsdp_revision_pass == true) && > > + (rsdp_length_pass == true) && > > + (ext_checksum_pass == true) && > > + (xsdt_address_pass == true) ) > > + { > > + fwts_passed(fw, "SBBR: Structure of RSDP Table is > > consistent with ACPI 6.0 or later and uses 64 bit xsdt > > addresses."); > > + } > > + else > > + { > > + fwts_failed(fw, LOG_LEVEL_CRITICAL, > > + "SBBR RSDP:", > > + "Structure of RSDP Table is > > not consistent with ACPI 6.0 or later and/or does not use 64 bit > > xsdt addresses."); > > + } > > + > The formatting style above needs reworking, > > if (( ... ) && > ( ... ) && > .... > ( ... )) > fwts_passed(...) > else > fwts_failed(...) > > > > > > + return FWTS_OK; > > +} > > + > > +static fwts_framework_minor_test sbbr_rsdp_tests[] = { > > + { sbbr_rsdp_test1, "RSDP Root System Description Pointer > > test." }, > > + { NULL, NULL } > > +}; > > + > > +static fwts_framework_ops sbbr_rsdp_ops = { > > + .description = "SBBR RSDP Root System Description Pointer > > tests.", > > + .init = sbbr_rsdp_init, > > + .minor_tests = sbbr_rsdp_tests > > +}; > > + > > +FWTS_REGISTER("sbbr_rsdp", &sbbr_rsdp_ops, FWTS_TEST_ANYTIME, > > FWTS_FLAG_TEST_SBBR) > > + > > +#endif > >
diff --git a/src/sbbr/rsdp/rsdp.c b/src/sbbr/rsdp/rsdp.c new file mode 100644 index 0000000..0a11f91 --- /dev/null +++ b/src/sbbr/rsdp/rsdp.c @@ -0,0 +1,134 @@ +/* + * Copyright (C) 2015-2017 Canonical + * Copyright (C) 2017 ARM Ltd + * + * Portions of this code original from the Linux-ready Firmware Developer Kit + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ +#include "fwts.h" + +#if defined(FWTS_HAS_SBBR) + +#include <stdlib.h> +#include <stdio.h> +#include <unistd.h> +#include <inttypes.h> +#include <string.h> +#include <ctype.h> + +static fwts_acpi_table_info *table; + +static int sbbr_rsdp_init(fwts_framework *fw) +{ + if (fwts_acpi_find_table(fw, "RSDP", 0, &table) != FWTS_OK) { + fwts_log_error(fw, "Cannot read ACPI tables."); + return FWTS_ERROR; + } + if (!table) { + + fwts_log_error(fw, + "ACPI RSDP is required for the " + "%s target architecture.", + fwts_arch_get_name(fw->target_arch)); + return FWTS_ERROR; + } + + /* We know there is an RSDP now, so do a quick sanity check */ + if (table->length == 0) { + fwts_log_error(fw, + "ACPI RSDP table has zero length"); + return FWTS_ERROR; + } + return FWTS_OK; +} + +/* + * RSDP Root System Description Pointer + */ +static int sbbr_rsdp_test1(fwts_framework *fw) +{ + fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp *)table->data; + uint8_t checksum; + + /*This includes only the first 20 bytes of this table, bytes + 0 to 19, including the checksum field. These bytes must sum to + zero. */ + const char RSDP_SIGNATURE[] = {'R', 'S', 'D', ' ', 'P', 'T', 'R', ' '}; + bool signature_pass = false; + const int CHECKSUM_BYTES = 20; + bool checksum_pass = false; + const int SBBR_RSDP_REVISION = 2; + bool rsdp_revision_pass = false; + const uint32_t SBBR_RSDP_LENGTH = 36; + bool rsdp_length_pass = false; + const int EXT_CHECKSUM_BYTES = 36; + bool ext_checksum_pass = false; + bool xsdt_address_pass = false; + + fwts_log_info(fw, "RSDP Signature = %.8s", rsdp->signature); + signature_pass = strncmp(rsdp->signature, RSDP_SIGNATURE, sizeof(rsdp->signature))? false : true; + + /* verify first checksum */ + checksum = fwts_checksum(table->data, CHECKSUM_BYTES); + fwts_log_info(fw, "RSDP Checksum = 0x%x", checksum); + checksum_pass = (checksum == 0)? true : false; + + fwts_log_info(fw, "RSDP Revision = 0x%x", rsdp->revision); + rsdp_revision_pass = (rsdp->revision >= SBBR_RSDP_REVISION)? true : false; + + fwts_log_info(fw, "RSDP Length = 0x%x", rsdp->length); + rsdp_length_pass = (rsdp->length == SBBR_RSDP_LENGTH)? true : false; + + checksum = fwts_checksum(table->data, EXT_CHECKSUM_BYTES); + fwts_log_info(fw, "RSDP Extended Checksum = 0x%x", checksum); + ext_checksum_pass = (checksum == 0)? true : false; + + if ( (rsdp->xsdt_address != 0) && + (rsdp->rsdt_address == 0) ) + { + xsdt_address_pass = true; + } + + if ( (signature_pass == true) && + (checksum_pass == true) && + (rsdp_revision_pass == true) && + (rsdp_length_pass == true) && + (ext_checksum_pass == true) && + (xsdt_address_pass == true) ) + { + fwts_passed(fw, "SBBR: Structure of RSDP Table is consistent with ACPI 6.0 or later and uses 64 bit xsdt addresses."); + } + else + { + fwts_failed(fw, LOG_LEVEL_CRITICAL, + "SBBR RSDP:", + "Structure of RSDP Table is not consistent with ACPI 6.0 or later and/or does not use 64 bit xsdt addresses."); + } + + return FWTS_OK; +} + +static fwts_framework_minor_test sbbr_rsdp_tests[] = { + { sbbr_rsdp_test1, "RSDP Root System Description Pointer test." }, + { NULL, NULL } +}; + +static fwts_framework_ops sbbr_rsdp_ops = { + .description = "SBBR RSDP Root System Description Pointer tests.", + .init = sbbr_rsdp_init, + .minor_tests = sbbr_rsdp_tests +}; + +FWTS_REGISTER("sbbr_rsdp", &sbbr_rsdp_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_TEST_SBBR) + +#endif
Server Base Boot Requirements (SBBR) specification is intended for SBSA- compliant 64-bit ARMv8 servers. It defines the base firmware requirements for out-of-box support of any ARM SBSA-compatible Operating System or hypervisor. The requirements in this specification are expected to be minimal yet complete for booting a multi-core ARMv8 server platform, while leaving plenty of room for OEM or ODM innovations and design details. For more information, download the SBBR specification here: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0044b/index.html This change introduces test cases as per SBBR specification to rsdp acpi. These test cases may be subset/superset of rsdp acpi tests already existing. However, to preserve "sbbr" classification, new file is created, even when most of the code is re-used from acpi/rsdp. Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com> --- src/sbbr/rsdp/rsdp.c | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) create mode 100644 src/sbbr/rsdp/rsdp.c