[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 <marcandre.lureau at redhat.com>
> 
> 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 <marcandre.lureau at redhat.com>
> Reviewed-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> Tested-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> ---
>  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