Re: [SeaBIOS] [PATCH v2 4/4] tpm: add TPM CRB device support

[Kevin O'Connor]

On Tue, Feb 13, 2018 at 11:08:07AM -0500, Stefan Berger wrote:
> From: Marc-André Lureau
>
> The CRB device was introduced with TPM 2.0 to be physical-bus agnostic
> and defined in TCG PC Client Platform TPM Profile (PTP) Specification
> Family “2.0” Level 00 Revision 01.03 v22
>
> It seems to be required with Windows 10. It is also a simpler device
> than FIFO/TIS.
>
> This patch only support locality 0 since also the CRB device in QEMU
> only supports this locality.
>
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Stefan Berger 
> Tested-by: Stefan Berger
>
> src/hw/tpm_drivers.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  src/hw/tpm_drivers.h |  26 +++++++
>  2 files changed, 223 insertions(+), 1 deletion(-)
>
> diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
> index 5cee9d8..24ce9b7 100644
> --- a/src/hw/tpm_drivers.c
> +++ b/src/hw/tpm_drivers.c
> @@ -17,6 +17,8 @@
>  #include "util.h" // timer_calc_usec
>  #include "x86.h" // readl
>
> +#define MIN(x,y) ((x) < (y) ? (x) : (y))

I'd prefer not to introduce adhoc MIN/MAX macros - they tend to be
confusing as sometimes they're evaluation safe and sometimes not.  I'm
okay with a separate patch that adds min/max globally to seabios, but
otherwise I suggest just implementing this directly in its one call
site.

[...]
> +/* if device is not there, return '0', '1' otherwise */
> +static u32 crb_probe(void)
> +{
> if (!CONFIG_TCGBIOS)
>    return 0;
> u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
> if ((ifaceid & 0xf) != 0xf) {
>    if ((ifaceid & 0xf) == 1) {
>        /* CRB is active */
>        return 1;
>    }
>    if ((ifaceid & (1 << 14)) == 0) {
>        /* CRB cannot be selected */
>        return 0;
>    }
>    /* write of 1 to bits 17-18 selects CRB */
>    writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>    /* lock it */
>    writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
> }
> /* no support for 64 bit addressing yet */
> if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>    return 1;
> u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
> if (addr > __UINT32_MAX__)

Where does the __UINT32_MAX__ constant come from?  We don't normally
depend on any system libraries.  Again, probably best to just use
0xffffffff.

Otherwise the series looks good to me.

Thanks,
-Kevin