[Skiboot] [PATCH 03/19] hdata/tpmrel.c: add firmware event log info to the tpm node

[Stewart Smith]

Oliver <oohall at gmail.com> writes:
>> --- /dev/null
>> +++ b/hdata/tpmrel.c
>> @@ -0,0 +1,90 @@
>> +/* Copyright 2013-2017 IBM Corp.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> + * implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#ifndef pr_fmt
>> +#define pr_fmt(fmt) "TPMREL: " fmt
>> +#endif
>> +
>> +#include <skiboot.h>
>> +#include <device.h>
>> +
>> +#include "spira.h"
>> +#include "hdata.h"
>> +#include "hdif.h"
>> +
>> +static void tpmrel_add_firmware_event_log(const struct HDIF_common_hdr *hdif_hdr)
>> +{
>> +       const struct secureboot_tpm_info *stinfo;
>> +       struct dt_node *xscom, *node;
>> +       uint64_t addr;
>> +       bool tpmfound = false;
>> +       int count, i;
>> +
>> +       count = HDIF_get_iarray_size(hdif_hdr, TPMREL_IDATA_SECUREBOOT_TPM_INFO);
>> +       if (count > 1) {
>> +               prlog(PR_ERR, "multinode not supported, count=%d\n", count);
>> +               return;
>> +       }
>> +
>> +       for (i = 0; i < count; i++) {
>> +
>> +               stinfo = HDIF_get_iarray_item(hdif_hdr,
>> +                                             TPMREL_IDATA_SECUREBOOT_TPM_INFO,
>> +                                             i, NULL);
>> +
>
>> +               xscom = find_xscom_for_chip(be32_to_cpu(stinfo->chip_id));
>> +               if (xscom) {
>> +                       dt_for_each_node(xscom, node) {
>> +                               if (dt_has_node_property(node, "label", "tpm")) {
>> +                                       tpmfound=true;
>
> I usually write this sort of thing so that it breaks out of the loop
> when it finds the target node. Then you can just check if !node to see
> if the scan failed/succeeded rather than having a separate variable.
> Up to you though.

I'm inclined to agree, I think it ends up being a bit neater.

>
>> +                                       addr = (uint64_t) stinfo +
>> +                                               be32_to_cpu(stinfo->srtm_log_offset);
>> +                                       dt_add_property_u64s(node, "linux,sml-base", addr);
>> +                                       dt_add_property_cells(node, "linux,sml-size",
>> +                                                             be32_to_cpu(stinfo->srtm_log_size));
>> +                                       break;
>> +                               }
>> +                       }
>> +                       if (!tpmfound &&
>
> Also, can you add a comment explaining what you're doing here. It took
> me a minute to work out it was looking up the I2C device node. It
> might be worth adding the I2C link ID as a DT_PRIVATE property to the
> I2C device node and looking for that instead.
>
>> +                           stinfo->tpm_status == TPM_PRESENT_AND_FUNCTIONAL) {
>
> Hmm, won't the sml-base and size properties that you just added be
> invalid if the TPM is non-functional? You might want to move this
> check to the start of the loop.

+1

-- 
Stewart Smith
OPAL Architect, IBM.