Hi Ilias,
On Thu, 18 Aug 2022 at 01:29, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 17 Aug 2022 at 21:54, Simon Glass <s...@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Tue, 16 Aug 2022 at 06:43, Ilias Apalodimas
> > <ilias.apalodi...@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > I know little of this device and the whole patch seems fine apart from
> > > the definitions and declarations of the state functions.
> > >
> > >
> > > On Sat, 13 Aug 2022 at 22:56, Simon Glass <s...@chromium.org> wrote:
> > > >
> > >
> > > >
> > > > drivers/tpm/cr50_i2c.c | 117 +++++++++++++++++++++++++++++++++++++++++
> > > > include/tpm-v2.h | 54 +++++++++++++++++++
> > > > lib/tpm-v2.c | 24 +++++++++
> > >
> > > [...]
> > >
> > > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > > index e79c90b9395..8e90a616220 100644
> > > > --- a/include/tpm-v2.h
> > > > +++ b/include/tpm-v2.h
> > > > @@ -419,6 +419,50 @@ enum {
> > > > HR_NV_INDEX = TPM_HT_NV_INDEX << HR_SHIFT,
> > > > };
> > > >
> > > > +/*
> > > > + * Operations specific to the Cr50 TPM used on Chromium OS and Android
> > > > devices
> > > > + *
> > > > + * FIXME: below is not enough to differentiate between vendors commands
> > > > + * of numerous devices. However, the current tpm2 APIs aren't very
> > > > amenable
> > > > + * to extending generically because the marshaling code is assuming all
> > > > + * knowledge of all commands.
> > > > + */
> > > > +#define TPM2_CC_VENDOR_BIT_MASK 0x20000000
> > > > +
> > > > +#define TPM2_CR50_VENDOR_COMMAND
> > > > (TPM2_CC_VENDOR_BIT_MASK | 0)
> > > > +#define TPM2_CR50_SUB_CMD_IMMEDIATE_RESET 19
> > > > +#define TPM2_CR50_SUB_CMD_NVMEM_ENABLE_COMMITS 21
> > > > +#define TPM2_CR50_SUB_CMD_REPORT_TPM_STATE 23
> > > > +#define TPM2_CR50_SUB_CMD_TURN_UPDATE_ON 24
> > > > +#define TPM2_CR50_SUB_CMD_GET_REC_BTN 29
> > > > +#define TPM2_CR50_SUB_CMD_TPM_MODE 40
> > > > +#define TPM2_CR50_SUB_CMD_GET_BOOT_MODE 52
> > > > +#define TPM2_CR50_SUB_CMD_RESET_EC 53
> > > > +
> > > > +/* Cr50 vendor-specific error codes. */
> > > > +#define VENDOR_RC_ERR 0x00000500
> > > > +enum cr50_vendor_rc {
> > > > + VENDOR_RC_INTERNAL_ERROR = (VENDOR_RC_ERR | 6),
> > > > + VENDOR_RC_NO_SUCH_SUBCOMMAND = (VENDOR_RC_ERR | 8),
> > > > + VENDOR_RC_NO_SUCH_COMMAND = (VENDOR_RC_ERR | 127),
> > > > +};
> > > > +
> > > > +enum cr50_tpm_mode {
> > > > + /*
> > > > + * Default state: TPM is enabled, and may be set to either
> > > > + * TPM_MODE_ENABLED or TPM_MODE_DISABLED.
> > > > + */
> > > > + TPM_MODE_ENABLED_TENTATIVE = 0,
> > > > +
> > > > + /* TPM is enabled, and mode may not be changed. */
> > > > + TPM_MODE_ENABLED = 1,
> > > > +
> > > > + /* TPM is disabled, and mode may not be changed. */
> > > > + TPM_MODE_DISABLED = 2,
> > > > +
> > > > + TPM_MODE_INVALID,
> > > > +};
> > > > +
> > > > /**
> > > > * Issue a TPM2_Startup command.
> > > > *
> > > > @@ -658,4 +702,14 @@ u32 tpm2_disable_platform_hierarchy(struct udevice
> > > > *dev);
> > > > u32 tpm2_submit_command(struct udevice *dev, const u8 *sendbuf,
> > > > u8 *recvbuf, size_t *recv_size);
> > > >
> > > > +/**
> > > > + * tpm_cr50_report_state() - Report the Cr50 internal state
> > > > + *
> > > > + * @dev: TPM device
> > > > + * @recvbuf: Buffer to save the response to
> > > > + * @recv_size: Pointer to the size of the response buffer
> > > > + * Return: result of the operation
> > > > + */
> > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t
> > > > *recv_size);
> > > > +
> > >
> > > I think we should keep the generic include files clean for hardware
> > > specific details.
> > >
> > > > #endif /* __TPM_V2_H */
> > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> > > > index 3e240bb4c67..3de4841974a 100644
> > > > --- a/lib/tpm-v2.c
> > > > +++ b/lib/tpm-v2.c
> > > > @@ -679,3 +679,27 @@ u32 tpm2_submit_command(struct udevice *dev, const
> > > > u8 *sendbuf,
> > > > {
> > > > return tpm_sendrecv_command(dev, sendbuf, recvbuf, recv_size);
> > > > }
> > > > +
> > > > +u32 tpm2_cr50_report_state(struct udevice *dev, u8 *recvbuf, size_t
> > > > *recv_size)
> > > > +{
> > > > + u8 command_v2[COMMAND_BUFFER_SIZE] = {
> > > > + /* header 10 bytes */
> > > > + tpm_u16(TPM2_ST_NO_SESSIONS), /* TAG */
> > > > + tpm_u32(10 + 2), /* Length */
> > > > + tpm_u32(TPM2_CR50_VENDOR_COMMAND), /* Command code
> > > > */
> > > > +
> > > > + tpm_u16(TPM2_CR50_SUB_CMD_REPORT_TPM_STATE),
> > > > + };
> > > > + int ret;
> > > > +
> > > > + ret = tpm_sendrecv_command(dev, command_v2, recvbuf, recv_size);
> > > > + log_debug("ret=%s, %x\n", dev->name, ret);
> > > > + if (ret)
> > > > + return ret;
> > > > + if (*recv_size < 12)
> > > > + return -ENODATA;
> > > > + *recv_size -= 12;
> > > > + memcpy(recvbuf, recvbuf + 12, *recv_size);
> > > > +
> > > > + return 0;
> > > > +}
> > >
> > > Same here, this functions seems ok but shouldn't land in the generic TPM
> > > API
> >
> > So shall I create a new tpm_cr50.h header file? What about the C file?
>
> Yea the header file seems fine (I assume in drivers/tpm?)
>
> About the C file, tpm2_cr50_report_state() is called by
> cr50_i2c_report_state() which is static in the drivers. Can't we just
> move the function there?
Just digging into this but I think you have the wrong end of the stick.
I have added a new TPM method to the uclass for this. We cannot call
functions 'sideways' of that mechanism as it violates how driver model
works.
The feature is implemented for cr50 and sandbox. It could be
implemented for other TPMs if they have any info that can usefully be
provided, such as the state of the PCRs or something else useful or
important.
So I think my patches are already doing all this correctly. Can you
please take another look? I'll send v3 as Heinrich had a minor comment
on a function comment. Also one of your changes.
Regards,
Simon
