[PATCH] V2 devicetree/dt_sysinfo: Add OPAL reference compatible checks
Colin Ian King
colin.king at canonical.com
Tue Jun 21 17:17:06 UTC 2016
On 21/06/16 14:58, Deb McLemore wrote:
> We added a new device tree check for the OPAL compatible property.
> We also added an OpenPOWER reference check to validate that the
> machine model and device tree compatible property are a supported
> match. Future additions will add device tree hardware checks
> (as applicable) to confirm that the device tree matches the topology of
> the expected hardware configurations.
>
> Signed-off-by: Deb McLemore <debmc at linux.vnet.ibm.com>
> ---
> .../arg-show-tests-full-0001.log | 3 +-
> src/devicetree/dt_base/dt_base.c | 82 +++++---
> src/devicetree/dt_sysinfo/dt_sysinfo.c | 215 +++++++++++++++++----
> 3 files changed, 237 insertions(+), 63 deletions(-)
>
> diff --git a/fwts-test/arg-show-tests-full-0001/arg-show-tests-full-0001.log b/fwts-test/arg-show-tests-full-0001/arg-show-tests-full-0001.log
> index 6784ed3..126c786 100644
> --- a/fwts-test/arg-show-tests-full-0001/arg-show-tests-full-0001.log
> +++ b/fwts-test/arg-show-tests-full-0001/arg-show-tests-full-0001.log
> @@ -372,9 +372,10 @@ Batch tests:
> Check device tree presence
> Check device tree baseline validity
> Check device tree warnings
> - dt_sysinfo (2 tests):
> + dt_sysinfo (3 tests):
> Check model property
> Check system-id property
> + Check OpenPOWER Reference compatible
> ebda (1 test):
> Test EBDA is reserved in E820 table.
> ecdt (1 test):
> diff --git a/src/devicetree/dt_base/dt_base.c b/src/devicetree/dt_base/dt_base.c
> index efb05fc..d9a5285 100644
> --- a/src/devicetree/dt_base/dt_base.c
> +++ b/src/devicetree/dt_base/dt_base.c
> @@ -28,8 +28,9 @@
> static int dt_base_check_present(fwts_framework *fw)
> {
> if (fw->fdt == NULL) {
> - fwts_failed(fw, LOG_LEVEL_HIGH, "DeviceTreeBaseAbsent",
> - "No device tree could be loaded");
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DeviceTreeBaseAbsent",
> + "No device tree could be loaded");
> return FWTS_ERROR;
> }
>
> @@ -41,13 +42,18 @@ static int dt_base_check_valid(fwts_framework *fw)
> {
> int rc;
>
> - if (fw->fdt == NULL)
> - return FWTS_ABORTED;
> + if (fw->fdt == NULL) {
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DeviceTreeBaseInvalid",
> + "No device tree could be loaded");
> + return FWTS_ERROR;
> + }
>
> rc = fdt_check_header(fw->fdt);
> if (rc != 0) {
> - fwts_failed(fw, LOG_LEVEL_HIGH, "DeviceTreeBaseInvalid",
> - "Device tree data is invalid");
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DeviceTreeBaseInvalid",
> + "Device tree data is invalid");
> return FWTS_ERROR;
> }
>
> @@ -57,44 +63,56 @@ static int dt_base_check_valid(fwts_framework *fw)
>
> static int dt_base_check_warnings(fwts_framework *fw)
> {
> - int rc, status, in_fd, out_fd, ret = FWTS_ERROR;
> + int status = 0, in_fd = 0, out_fd = 0, ret = FWTS_ERROR;
> ssize_t in_len, out_len;
> const char *command;
> char *output = NULL;
> pid_t pid;
>
> - if (!fw->fdt)
> - return FWTS_ABORTED;
> + if (!fw->fdt) {
> + fwts_failed(fw, LOG_LEVEL_LOW,
> + "DTMissing",
> + "Device Tree is missing,"
> + " unable to continue without DT");
> + goto err;
> + }
>
> command = "dtc -I dtb -O dtb -o /dev/null 2>&1";
> - rc = fwts_pipe_open_rw(command, &pid, &in_fd, &out_fd);
> - if (rc)
> - return FWTS_ABORTED;
> + if (fwts_pipe_open_rw(command, &pid, &in_fd, &out_fd)) {
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DTMissing",
> + "There was a problem obtaining the"
> + " device tree info,"
> + " unable to continue without DT data");
> + goto err;
> + }
>
> in_len = fdt_totalsize(fw->fdt);
>
> - rc = fwts_pipe_readwrite(in_fd, fw->fdt, in_len,
> - out_fd, &output, &out_len);
> - if (rc) {
> - fwts_failed(fw, LOG_LEVEL_HIGH, "DeviceTreeBaseDTCPipe",
> - "failed to pipe data to dtc");
> - fwts_pipe_close2(in_fd, out_fd, pid);
> - return FWTS_ERROR;
> + if (fwts_pipe_readwrite(in_fd, fw->fdt, in_len,
> + out_fd, &output, &out_len)) {
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DeviceTreeBaseDTCPipe",
> + "failed to pipe data to dtc");
> + goto err;
> }
>
> status = fwts_pipe_close2(in_fd, out_fd, pid);
> + in_fd = out_fd = 0;
>
> if (status) {
> - fwts_failed(fw, LOG_LEVEL_HIGH, "DeviceTreeBaseDTCFailed",
> - "dtc reports fatal device tree errors:\n%s\n",
> - output);
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DeviceTreeBaseDTCFailed",
> + "dtc reports fatal device tree errors:%s",
> + output);
> goto err;
> }
>
> if (out_len > 0) {
> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "DeviceTreeBaseDTCWarnings",
> - "dtc reports warnings from device tree:\n%s\n",
> - output);
> + fwts_failed(fw, LOG_LEVEL_MEDIUM,
> + "DeviceTreeBaseDTCWarnings",
> + "dtc reports warnings from device tree:%s",
> + output);
> goto err;
> }
>
> @@ -103,14 +121,20 @@ static int dt_base_check_warnings(fwts_framework *fw)
> ret = FWTS_OK;
>
> err:
> - free(output);
> + if (output)
> + free(output);
> + if (in_fd || out_fd)
> + fwts_pipe_close2(in_fd, out_fd, pid);
> return ret;
> }
>
> static fwts_framework_minor_test dt_base_tests[] = {
> - { dt_base_check_present, "Check device tree presence" },
> - { dt_base_check_valid, "Check device tree baseline validity" },
> - { dt_base_check_warnings, "Check device tree warnings" },
> + { dt_base_check_present,
> + "Check device tree presence" },
> + { dt_base_check_valid,
> + "Check device tree baseline validity" },
> + { dt_base_check_warnings,
> + "Check device tree warnings" },
> { NULL, NULL },
> };
>
> diff --git a/src/devicetree/dt_sysinfo/dt_sysinfo.c b/src/devicetree/dt_sysinfo/dt_sysinfo.c
> index 593c496..477210a 100644
> --- a/src/devicetree/dt_sysinfo/dt_sysinfo.c
> +++ b/src/devicetree/dt_sysinfo/dt_sysinfo.c
> @@ -26,19 +26,42 @@
>
> #include "fwts.h"
>
> -static int check_property_printable(fwts_framework *fw, const char *name,
> - const char *buf, size_t len)
> +static const char op_powernv[] = "ibm,powernv";
> +
> +static const char *firestone_models[] = {
> + "8335-GTA ",
> + "8335-GTX ",
> +};
> +
> +static const char *garrison_models[] = {
> + "8335-GTB ",
> +};
> +
> +static struct reference_platform {
> + const char *compatible;
> + const char **models;
> + int n_models;
> +} openpower_reference_platforms[] = {
> + {"ibm,firestone", firestone_models,
> + FWTS_ARRAY_LEN(firestone_models)},
> + {"ibm,garrison", garrison_models,
> + FWTS_ARRAY_LEN(garrison_models)},
> +};
> +
> +static int check_property_printable(fwts_framework *fw,
> + const char *name,
> + const char *buf,
> + size_t len)
> {
> bool printable = true;
> unsigned int i;
> - int rc;
>
> /* we need at least one character plus a nul */
> if (len < 2) {
> - fwts_failed(fw, LOG_LEVEL_LOW, "DTPrintablePropertyShort",
> - "property %s is too short", name);
> - rc = FWTS_ERROR;
> - goto out;
> + fwts_failed(fw, LOG_LEVEL_LOW,
> + "DTPrintablePropertyShort",
> + "property %s is too short", name);
> + return FWTS_ERROR;
> }
>
> /* check all characters are printable */
> @@ -49,34 +72,42 @@ static int check_property_printable(fwts_framework *fw, const char *name,
> }
>
> if (!printable) {
> - fwts_failed(fw, LOG_LEVEL_LOW, "DTPrintablePropertyInvalid",
> - "property %s contains unprintable characters",
> - name);
> - rc = FWTS_ERROR;
> - goto out;
> + fwts_failed(fw, LOG_LEVEL_LOW,
> + "DTPrintablePropertyInvalid",
> + "property %s contains unprintable characters",
> + name);
> + return FWTS_ERROR;
> }
>
> /* check for a trailing nul */
> if (buf[len-1] != '\0') {
> - fwts_failed(fw, LOG_LEVEL_LOW, "DTPrintablePropertyNoNul",
> - "property %s isn't nul-terminated", name);
> - rc = FWTS_ERROR;
> - goto out;
> + fwts_failed(fw, LOG_LEVEL_LOW,
> + "DTPrintablePropertyNoNul",
> + "property %s isn't nul-terminated", name);
> + return FWTS_ERROR;
> }
>
> - rc = FWTS_OK;
> + fwts_log_info_verbatim(fw,
> + "DTPrintableProperty with a string"
> + " value of \"%s\" passed",
> + buf);
>
> -out:
> - return rc;
> + return FWTS_OK;
> }
>
> -static int dt_sysinfo_check_root_property(fwts_framework *fw, const char *name)
> +static int dt_sysinfo_check_root_property(
> + fwts_framework *fw,
> + const char *name,
> + bool print_flag)
> {
> - int rc, node, len;
> + int node, len;
> const char *buf;
>
> - if (!fw->fdt)
> + if (!fw->fdt) {
> + fwts_failed(fw, LOG_LEVEL_LOW, "DTMissing",
> + "Device Tree is missing, aborting");
> return FWTS_ABORTED;
> + }
>
> node = fdt_path_offset(fw->fdt, "/");
> if (node < 0) {
> @@ -87,33 +118,151 @@ static int dt_sysinfo_check_root_property(fwts_framework *fw, const char *name)
>
> buf = fdt_getprop(fw->fdt, node, name, &len);
> if (buf == NULL) {
> - fwts_failed(fw, LOG_LEVEL_LOW, "DTSyinfoPropertyMissing",
> - "can't read property %s: %s",
> - name, fdt_strerror(len));
> + fwts_failed(fw, LOG_LEVEL_LOW,
> + "DTSysinfoPropertyMissing",
> + "can't read property %s: %s",
> + name, fdt_strerror(len));
> return FWTS_ERROR;
> }
>
> - rc = check_property_printable(fw, name, buf, len);
> + if (print_flag) {
> + if (check_property_printable(fw, name, buf, len))
> + return FWTS_ERROR; /* failures logged prior */
> + }
>
> - if (rc == FWTS_OK)
> - fwts_passed(fw, "sysinfo property %s is valid", name);
> + fwts_passed(fw, "sysinfo property %s is valid", name);
>
> - return rc;
> + return FWTS_OK;
> }
>
> static int dt_sysinfo_check_model(fwts_framework *fw)
> {
> - return dt_sysinfo_check_root_property(fw, "model");
> + return dt_sysinfo_check_root_property(fw, "model", true);
> }
>
> static int dt_sysinfo_check_system_id(fwts_framework *fw)
> {
> - return dt_sysinfo_check_root_property(fw, "system-id");
> + return dt_sysinfo_check_root_property(fw, "system-id", true);
> +}
> +
> +static int prd_fdt_stringlist_contains_last(const char *strlist,
> + int listlen,
> + const char *str)
> +{
> + int len = strlen(str);
> + const char *p;
> +
> + /* checking for either str only or last in string */
> + if (listlen < 2 ) /* need at least one byte plus nul */
> + return 0;
> +
> + p = memrchr(strlist, '\0', listlen-1);
> + if (!p) {
> + if (memcmp(str, strlist, len+1) == 0)
> + return 1;
> + } else {
> + if (memcmp(str, p+1, len+1) == 0)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static bool machine_matches_reference_model(fwts_framework *fw,
> + const char *compatible,
> + int compat_len,
> + const char *model)
> +{
> + int i, j;
> + bool found = false;
> +
> + for (i = 0;
> + i < (int)FWTS_ARRAY_LEN(openpower_reference_platforms);
> + i++) {
> + struct reference_platform *plat =
> + &openpower_reference_platforms[i];
> + if (prd_fdt_stringlist_contains_last(compatible,
> + compat_len, plat->compatible)) {
> + for (j = 0; j < plat->n_models; j++) {
> + if (!strcmp(model, plat->models[j])) {
> + fwts_log_info_verbatim(fw,
> + "Matched reference model, "
> + "device tree \"compatible\" is \"%s\" and "
> + "\"model\" is \"%s\"",
> + plat->compatible, model);
> + found = true;
> + break;
> + }
> + }
> + } else {
> + continue;
> + }
> + if (found)
> + break;
> + }
> + return found;
> +}
> +
> +static int dt_sysinfo_check_ref_plat_compatible(fwts_framework *fw)
> +{
> + int node, compat_len, model_len;
> + const char *model_buf, *compat_buf;
> +
> + node = fdt_path_offset(fw->fdt, "/");
> + if (node < 0) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "DTRootNodeMissing",
> + "root device tree node is missing");
> + return FWTS_ERROR;
> + }
> + if (fdt_node_check_compatible(fw->fdt, node, op_powernv)) {
> + fwts_failed(fw, LOG_LEVEL_HIGH, "DTCompatibleMissing",
> + "DeviceTree failed validation, could not find"
> + " the \"compatible\" property of \"%s\" in the "
> + "root of the device tree", "ibm,powernv");
> + return FWTS_ERROR;
> + } else {
> + compat_buf = fdt_getprop(fw->fdt, node,
> + "compatible", &compat_len);
> + model_buf = fdt_getprop(fw->fdt, node,
> + "model", &model_len);
> +
> + if (!model_buf || !compat_buf) {
> + fwts_failed(fw,LOG_LEVEL_HIGH,
> + "DTSysinfoPropertyMissing",
> + "can't read properties for OpenPOWER"
> + " Reference Compatible check");
> + return FWTS_ERROR;
> + }
> +
> + if (machine_matches_reference_model(fw,
> + compat_buf,
> + compat_len,
> + model_buf)) {
> + fwts_passed(fw, "OpenPOWER Reference "
> + "Compatible passed");
> + } else {
> + fwts_failed(fw, LOG_LEVEL_HIGH,
> + "DTOpenPOWERReferenceFailed",
> + "Unable to find an OpenPOWER supported"
> + " match");
> + /* adding verbatim to show proper string */
> + fwts_log_info_verbatim(fw,
> + "Unable to find an OpenPOWER reference"
> + " match for \"%s\"", model_buf);
> + return FWTS_ERROR;
> + }
> + }
> +
> + return FWTS_OK;
> }
>
> static fwts_framework_minor_test dt_sysinfo_tests[] = {
> - { dt_sysinfo_check_model, "Check model property" },
> - { dt_sysinfo_check_system_id, "Check system-id property" },
> + { dt_sysinfo_check_model,
> + "Check model property" },
> + { dt_sysinfo_check_system_id,
> + "Check system-id property" },
> + { dt_sysinfo_check_ref_plat_compatible,
> + "Check OpenPOWER Reference compatible" },
> { NULL, NULL },
> };
>
>
I've not been able to test these, but it passes static analysis and I'm
OK with the code here, so...
Acked-by: Colin Ian King <colin.king at canonical.com>
Ivan/Alex: Can you remove the V2 from the commit message before applying it.
Deb: if you put the V2 in square brackets, such as [V2] then git will
squish these when we apply the patch.
More information about the fwts-devel
mailing list