Re: [PATCH v15 19/28] x86/tpm: Early TPM PCR extending driver

ross.philipson
2025-12-17T18:41:04Z

On 12/16/25 1:53 PM, Dave Hansen wrote:
> I'm mostly spot-checking this to see what kind of shape it's in and how
> much work and diligence has been applied in the last 8 months since v14.
>
> On 12/15/25 15:33, Ross Philipson wrote:
> ...
>> The driver could be extended for further operations if needed. This
>> TPM dirver implementation relies as much as possible on existing mainline
>
> <sigh>
>
> v15 and no spell checking. :(

Will fix.

>
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/early_tpm_extend.c
>> @@ -0,0 +1,601 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2010-2012 United States Government, as represented by
>> + * the Secretary of Defense. All rights reserved.
>
> IANAL, but this looks fishy.
>
> It's theoretically fine to go grab random code off the Internet and
> submit it to the kernel, given the correct license. But I do want to
> know what its story is and where it came from.
>
> I also seem to remember that there are special rules around the US
> federal government's inability to hold copyrights. This seems worth at
> least a mention ... somewhere.
>
> This is helpful, for instance:
>
>> + * based off of the original tools/vtpm_manager code base which is:
>> + * Copyright (c) 2005, Intel Corp.
>> + * All rights reserved.
>
> so thanks for that one.
>
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * * Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * * Redistributions in binary form must reproduce the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer in the documentation and/or other materials provided
>> + * with the distribution.
>> + * * Neither the name of Intel Corporation nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
>> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
>> + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
>> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
>> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
>> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
>> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
>> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
>> + * OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>
> Also, IANAL, but this looks BSD-ish.
>
> I would have kinda expected the SPDX header to say BSD-blah-blah and not
> GPL-2.0-only.
>
> I'd really appreciate if you could go have a huddle with your corporate
> Open Source folks and make sure this is all proper. To me, it looks
> fishy at _best_.

Yes, we will do exactly that for all the licensing and sort it out.

>
> ...
>> +/*
>> + * We're far too early to calibrate time. Assume a 5GHz processor (the upper
>> + * end of the Fam19h range), which causes us to be wrong in the safe direction
>> + * on slower systems.
>> + */
>
> https://urldefense.com/v3/__https://docs.kernel.org/process/maintainer-tip.html*changelog__;Iw!!ACWV5N9M2RV99hQ!ODp_iKdXfPuA60ae9ZCFdElNvGZjrd7ffPYtSVs3cwOTY2kzGN_tgsRLYawnxEGiHE0jMDN2kgOxBBtMtmu-7ohw$
>
> Imperative voice please.

+1

>
> ...
>> +static int __tis_recv_data(struct tpm_chip *chip, u8 *buf, int count)
>> +{
>> + int size = 0;
>> + int burstcnt;
>> +
>> + while (size < count && __tis_wait_for_stat(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, chip->timeout_c) == 0) {
>> + burstcnt = __tis_get_burstcount(chip);
>> +
>> + for ( ; burstcnt > 0 && size < count; --burstcnt)
>> + buf[size++] = tpm_read8(chip, TPM_DATA_FIFO(chip->locality));
>> + }
>> +
>> + return size;
>> +}
>> +
>> +/**
>> + * tpm_tis_check_locality - Check if the given locality is the active one
>> + * @chip: The TPM chip instance
>> + * @loc: The locality to check
>> + *
>> + * Return: true - locality active, false - not active
>> + */
>> +bool tpm_tis_check_locality(struct tpm_chip *chip, int loc)
>> +{
>> + if ((tpm_read8(chip, TPM_ACCESS(loc)) & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) == (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
>> + chip->locality = loc;
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +/**
>> + * tpm_tis_release_locality - Release the active locality
>> + * @chip: The TPM chip instance
>> + */
>> +void tpm_tis_release_locality(struct tpm_chip *chip)
>> +{
>> + if ((tpm_read8(chip, TPM_ACCESS(chip->locality)) & (TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID)) == (TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID))
>> + tpm_write8(chip, TPM_ACCESS(chip->locality), TPM_ACCESS_RELINQUISH_LOCALITY);
>> +
>> + chip->locality = 0;
>> +}
>
> I guess some folks aren't enforcing the 80-column limits. But this is
> not even close. It's almost 80x2.
>
> Has there even been an attempt to make this conform to kernel coding
> style? What other checkpatch.pl warnings are being ignored?
>

We do run checkpatch.pl and fix the issues it points out. I feel it is
not clear how to approach the 80 character limit rule though. I have
been told in other reviews that the 80 char rule is not really followed
and certain things would read better w/o it. Reading the guide again, it
does not really spell out details other than try to keep it 80 and
under. Maybe there should be a hard limit (< 80x2) if you exceed it?

But ultimately I will format the code in whatever manner is requested.

Thank you,
Ross