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

[ross.philipson@oracle.com]

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