Re: [PATCH 1/2 v2] tpm2: Introduce TIS tpm core

[Simon Glass <sjg@chromium.org>]

Hi Ilias,

On Mon, 12 Jul 2021 at 00:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Sat, Jul 10, 2021 at 06:00:57PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > There's a lot of code duplication in U-Boot right now.  All the TPM TIS
> >
> > You mean in the TPM code I think.
> >
>
> Yes. Basically al TPM drivers duplicate this.
>
> > > compatible drivers we have at the moment have their own copy of a TIS
> > > implementation.
> > >
> > > So let's create a common layer which implements the core TIS functions.
> > > Any driver added from now own, which is compatible with the TIS spec, will
> > > only have to provide the underlying bus communication mechanisms.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v1:
> > > -
> > >  drivers/tpm/tpm2_tis_core.c | 545 ++++++++++++++++++++++++++++++++++++
> > >  drivers/tpm/tpm_tis.h       |  40 +++
> > >  include/tpm-v2.h            |   1 +
> > >  3 files changed, 586 insertions(+)
> > >  create mode 100644 drivers/tpm/tpm2_tis_core.c
> >
> > [..]
> >
> > > diff --git a/drivers/tpm/tpm_tis.h b/drivers/tpm/tpm_tis.h
> > > index 2a160fe05c9a..fde3bb71f7c2 100644
> > > --- a/drivers/tpm/tpm_tis.h
> > > +++ b/drivers/tpm/tpm_tis.h
> > > @@ -21,6 +21,37 @@
> > >  #include <linux/compiler.h>
> > >  #include <linux/types.h>
> > >
> > > +struct tpm_tis_phy_ops {
> > > +       int (*read_bytes)(struct udevice *udev, u32 addr, u16 len,
> > > +                         u8 *result);
> > > +       int (*write_bytes)(struct udevice *udev, u32 addr, u16 len,
> > > +                          const u8 *value);
> > > +       int (*read16)(struct udevice *udev, u32 addr, u16 *result);
> > > +       int (*read32)(struct udevice *udev, u32 addr, u32 *result);
> > > +       int (*write32)(struct udevice *udev, u32 addr, u32 src);
> >
> > A few points:
> >
> > - these need comments
> > - can we use uint instead of u32 for the value args? We should use
> > native types where we can
>
> Yes probably, I'll have a look
> `
> > - it seems like this should be a driver interface - see for example
> > how cros_ec.c works. It has a shared code library and the drivers each
> > implement an interface similar to the above, but on different buses.
> > In general function pointers are a sign we should be using a driver
> >
>
> I am not sure I am following, but I'll have a look on the code you pointed
> out.
>
> > > +};
> > > +
> > > +enum tis_int_flags {
> > > +       TPM_GLOBAL_INT_ENABLE = 0x80000000,
> > > +       TPM_INTF_BURST_COUNT_STATIC = 0x100,
> > > +       TPM_INTF_CMD_READY_INT = 0x080,
> > > +       TPM_INTF_INT_EDGE_FALLING = 0x040,
> > > +       TPM_INTF_INT_EDGE_RISING = 0x020,
> > > +       TPM_INTF_INT_LEVEL_LOW = 0x010,
> > > +       TPM_INTF_INT_LEVEL_HIGH = 0x008,
> > > +       TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
> > > +       TPM_INTF_STS_VALID_INT = 0x002,
> > > +       TPM_INTF_DATA_AVAIL_INT = 0x001,
> > > +};
> > > +
> > > +#define TPM_ACCESS(l)                   (0x0000 | ((l) << 12))
> > > +#define TPM_INT_ENABLE(l)               (0x0008 | ((l) << 12))
> > > +#define TPM_STS(l)                      (0x0018 | ((l) << 12))
> > > +#define TPM_DATA_FIFO(l)                (0x0024 | ((l) << 12))
> > > +#define TPM_DID_VID(l)                  (0x0F00 | ((l) << 12))
> > > +#define TPM_RID(l)                      (0x0F04 | ((l) << 12))
> > > +#define TPM_INTF_CAPS(l)                (0x0014 | ((l) << 12))
> > > +
> > >  enum tpm_timeout {
> > >         TPM_TIMEOUT_MS                  = 5,
> > >         TIS_SHORT_TIMEOUT_MS            = 750,
> > > @@ -43,6 +74,7 @@ struct tpm_chip {
> > >         u8 rid;
> > >         unsigned long timeout_a, timeout_b, timeout_c, timeout_d;  /* msec */
> > >         ulong chip_type;
> > > +       struct tpm_tis_phy_ops *phy_ops;
> > >  };
> > >
> > >  struct tpm_input_header {
> > > @@ -130,4 +162,12 @@ enum tis_status {
> > >  };
> > >  #endif
> > >
> > > +int tpm_tis_open(struct udevice *udev);
> > > +int tpm_tis_close(struct udevice *udev);
> > > +int tpm_tis_cleanup(struct udevice *udev);
> > > +int tpm_tis_send(struct udevice *udev, const u8 *buf, size_t len);
> > > +int tpm_tis_recv(struct udevice *udev, u8 *buf, size_t count);
> > > +int tpm_tis_get_desc(struct udevice *udev, char *buf, int size);
> > > +int tpm_tis_init(struct udevice *udev);
> > > +void tpm_tis_ops_register(struct udevice *udev, struct tpm_tis_phy_ops *ops);
> >
> > comments on all of these
> >
> > >  #endif
> > > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > > index 247b38696766..3e48e358613f 100644
> > > --- a/include/tpm-v2.h
> > > +++ b/include/tpm-v2.h
> > > @@ -378,6 +378,7 @@ enum {
> > >         TPM_STS_DATA_EXPECT             = 1 << 3,
> > >         TPM_STS_SELF_TEST_DONE          = 1 << 2,
> > >         TPM_STS_RESPONSE_RETRY          = 1 << 1,
> > > +       TPM_STS_READ_ZERO               = 0x23
> >
> > Does this below in another patch?
> >
>
> It's a general tpm2 update. I can move it to the driver patch if it makes
> more sense.
>
> > >  };
> > >
> > >  enum {
> > > --
> > > 2.32.0.rc0
> > >
> >
> > I feel that this API could be useful in reducing code duplication, but
> > in fact it has just created more, so far as I can see from this series
> > :-) So I think you should convert at least one driver to show its
> > value (and not make things any worse).
>
> The mmio tpm driver uses it and instead of ~700 lines (like the tpmv2 spi
> driver) it drops down to ~100.  I don't have access to any other TPM
> hardware to rewrite any of those.

Yes, but I hope you see my point, that you have added a new interface.
It is definitely better than adding a new driver and duplicating all
the code, but it is still one more copy and in fact, the code is
duplicated.

Can you get access to TPM hardware? I see that you have offered to be
the maintainer for this subsystem, so I think that would be useful.
Can sandbox use your new API?

Regards,
Simon