Re: [PATCH v3 10/22] x86/tpm.c: code for early hashing and extending PCRs (for TPM1.2)

[Sergii Dmytruk <sergii.dmytruk@3mdeb.com>]

On Thu, Jun 05, 2025 at 10:43:10AM -0700, ross.philipson@oracle.com wrote:
> > +static void send_cmd(unsigned loc, uint8_t *buf, unsigned i_size,
> > +                     unsigned *o_size)
> > +{
> > +    /*
> > +     * Value of "data available" bit counts only when "valid" field is set as
> > +     * well.
> > +     */
> > +    const unsigned data_avail = STS_VALID | STS_DATA_AVAIL;
> > +
> > +    unsigned i;
> > +
> > +    /* Make sure TPM can accept a command. */
> > +    if ( (tis_read8(TPM_STS_(loc)) & STS_COMMAND_READY) == 0 )
> > +    {
> > +        /* Abort current command. */
> > +        tis_write8(TPM_STS_(loc), STS_COMMAND_READY);
> > +        /* Wait until TPM is ready for a new one. */
> > +        while ( (tis_read8(TPM_STS_(loc)) & STS_COMMAND_READY) == 0 );
> > +    }
> > +
> > +    for ( i = 0; i < i_size; i++ )
> > +        tis_write8(TPM_DATA_FIFO_(loc), buf[i]);
> > +
> > +    tis_write8(TPM_STS_(loc), STS_TPM_GO);
> > +
> > +    /* Wait for the first byte of response. */
> > +    while ( (tis_read8(TPM_STS_(loc)) & data_avail) != data_avail);
> > +
> > +    for ( i = 0; i < *o_size && tis_read8(TPM_STS_(loc)) & data_avail; i++ )
> > +        buf[i] = tis_read8(TPM_DATA_FIFO_(loc));
> > +
> > +    if ( i < *o_size )
> > +        *o_size = i;
> > +
> > +    tis_write8(TPM_STS_(loc), STS_COMMAND_READY);
> > +}

> Is this all that is needed to do the send? I would have thought you would at
> least also need that burst count logic to know when the TPM is ready for
> more data. There are also a number of timeouts that are supposed to be used.
> Maybe Daniel has some thoughts too.

The code in this form works without any issues.  Don't know why
burst count isn't taken into account here or why nothing breaks without
it, but this does seem wrong.  I think we needed something to work
before Daniel's version was ready and this implementation possibly
wasn't meant to stay.

> > +static inline bool is_tpm12(void)
> > +{
> > +    /*
> > +     * If one of these conditions is true:
> > +     *  - INTF_CAPABILITY_x.interfaceVersion is 0 (TIS <= 1.21)
> > +     *  - INTF_CAPABILITY_x.interfaceVersion is 2 (TIS == 1.3)
> > +     *  - STS_x.tpmFamily is 0
> > +     * we're dealing with TPM1.2.
> > +     */
> > +    uint32_t intf_version = tis_read32(TPM_INTF_CAPABILITY_(0))
> > +                          & INTF_VERSION_MASK;
> > +    return (intf_version == 0x00000000 || intf_version == 0x20000000 ||
> > +            (tis_read32(TPM_STS_(0)) & TPM_FAMILY_MASK) == 0);
> > +}
> > +
> > +/****************************** TPM1.2 specific *******************************/
> > +#define TPM_ORD_Extend              0x00000014
> > +#define TPM_ORD_SHA1Start           0x000000A0
> > +#define TPM_ORD_SHA1Update          0x000000A1
> > +#define TPM_ORD_SHA1CompleteExtend  0x000000A3
> > +
> > +#define TPM_TAG_RQU_COMMAND         0x00C1
> > +#define TPM_TAG_RSP_COMMAND         0x00C4
>
> I am not sure what the long term goal for a TPM driver in Xen might be but
> my suggestion is to lay out the driver more cleanly up front. Split the
> specification defined things (e.g. these things and other from TCG etc) from
> the driver implementation specific definitions and put them in separate
> headers. There is little enough core code now that just putting it all in
> tpm.c seems fine. Just my $0.02...
>
> Thanks,
> Ross

It could help readability to not have it all in one file.  Don't know
about plans for evolving the code for future use cases.

Regards