Re: [PATCH 2/2] tpm2: Add a TPMv2 MMIO TIS driver

[Simon Glass <sjg@chromium.org>]

Hi Ilias,

On Mon, 12 Jul 2021 at 12:22, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
> On Sat, Jul 10, 2021 at 06:00:59PM -0600, Simon Glass wrote:
> > Hi Ilias,
> >
> > On Wed, 7 Jul 2021 at 10:26, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Add support for devices that expose a TPMv2 though MMIO.
> > > Apart from those devices, we can use the driver in our QEMU setups and
> > > test TPM related code which is difficult to achieve using the sandbox
> > > driver (e.g test the EFI TCG2 protocol).
> > >
> > > It's worth noting that a previous patch added TPMv2 TIS core functions,
> > > which the current driver is consuming.
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > Changes since v1:
> > > - split off the tis core code into a different file
> > >  drivers/tpm/Kconfig         |   9 +++
> > >  drivers/tpm/Makefile        |   1 +
> > >  drivers/tpm/tpm2_tis_mmio.c | 156 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 166 insertions(+)
> > >  create mode 100644 drivers/tpm/tpm2_tis_mmio.c
> >
> > Looks good to me
>
> Thanks!
>
> > >
> > > +struct tpm_tis_chip_data {
> > > +       unsigned int pcr_count;
> > > +       unsigned int pcr_select_min;
> > > +       unsigned int time_before_first_cmd_ms;
> > > +       void __iomem *iobase;
> > > +};
> >
> > comments
> >
>
> You mean on all the internal functions?
> Sure I can add them

I mean for struct members.

For internal functions a comment is a good idea depending on how
valuable it is - things like return values, args. But if just
implementing a fully documented API, it may not really add much.

>
> > > +
> > > +static int mmio_read_bytes(struct udevice *udev, u32 addr, u16 len,
> > > +                          u8 *result)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       while (len--)
> > > +               *result++ = ioread8(drv_data->iobase + addr);
> >
> > We normally put a blank line before the final return
> >
>
> sure
>
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_write_bytes(struct udevice *udev, u32 addr, u16 len,
> > > +                           const u8 *value)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       while (len--)
> > > +               iowrite8(*value++, drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_read16(struct udevice *udev, u32 addr, u16 *result)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       *result = ioread16(drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_read32(struct udevice *udev, u32 addr, u32 *result)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       *result = ioread32(drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static int mmio_write32(struct udevice *udev, u32 addr, u32 value)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       iowrite32(value, drv_data->iobase + addr);
> > > +       return 0;
> > > +}
> > > +
> > > +static struct tpm_tis_phy_ops phy_ops = {
> >
> > Should be a uclass interface.
> >
>
> Why? A uclass is supposed to describe and abstract hardware.  This is just
> a specific implementation of a TPM, not all TPMs are TIS compliant. We already
> have a uclass for those.

Who told you that a uclass is supposed to describe and abstract hardware? :-)

The uclass is how driver model does APIs, so normally a uclass would
be used for any API. There are exceptions, but this one actually looks
like a useful interface we should have.

>
> > > +       .read_bytes = mmio_read_bytes,
> > > +       .write_bytes = mmio_write_bytes,
> > > +       .read16 = mmio_read16,
> > > +       .read32 = mmio_read32,
> > > +       .write32 = mmio_write32,
> > > +};
> > > +
> > > +static int tpm_tis_probe(struct udevice *udev)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +       struct tpm_chip_priv *priv = dev_get_uclass_priv(udev);
> > > +       int ret = 0;
> > > +       fdt_addr_t ioaddr;
> > > +       u64 sz;
> > > +
> > > +       ioaddr = dev_read_addr(udev);
> > > +       if (ioaddr == FDT_ADDR_T_NONE)
> > > +               return -EINVAL;
> >
> > consider this for easier debugging:
> >
> >    return log_msg_ret("ioaddr", -EINVAL);
> >
>
> Sure
>
> > > +
> > > +       ret = dev_read_u64(udev, "reg", &sz);
> > > +       if (ret)
> > > +               return -EINVAL;
> > > +
> > > +       drv_data->iobase = ioremap(ioaddr, sz);
> > > +       log_info("Remapped TPM2 base: 0x%llx size: 0x%llx\n", ioaddr, sz);
> >
> > log_debug() I think?
> >
> Yea, that's a leftover of my initial code, were I needed to make sure the
> ioremap worked.
>
> > > +       tpm_tis_ops_register(udev, &phy_ops);
> > > +       ret = tpm_tis_init(udev);
> > > +       if (ret)
> > > +               goto iounmap;
> > > +
> > > +       priv->pcr_count = drv_data->pcr_count;
> > > +       priv->pcr_select_min = drv_data->pcr_select_min;
> > > +       /*
> > > +        * Although the driver probably works with a TPMv1 our Kconfig
> > > +        * limits the driver to TPMv2 only
> > > +        */
> > > +       priv->version = TPM_V2;
> > > +
> > > +       return ret;
> > > +iounmap:
> > > +       iounmap(drv_data->iobase);
> > > +       return -EINVAL;
> > > +}
> > > +
> > > +static int tpm_tis_remove(struct udevice *udev)
> > > +{
> > > +       struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev);
> > > +
> > > +       iounmap(drv_data->iobase);
> > > +       return tpm_tis_cleanup(udev);
> > > +}
> > > +
> > > +static const struct tpm_ops tpm_tis_ops = {
> > > +       .open           = tpm_tis_open,
> > > +       .close          = tpm_tis_close,
> > > +       .get_desc       = tpm_tis_get_desc,
> > > +       .send           = tpm_tis_send,
> > > +       .recv           = tpm_tis_recv,
> > > +       .cleanup        = tpm_tis_cleanup,
> > > +};
> > > +
> > > +static const struct tpm_tis_chip_data tpm_tis_std_chip_data = {
> > > +       .pcr_count = 24,
> > > +       .pcr_select_min = 3,
> > > +};
> > > +
> > > +static const struct udevice_id tpm_tis_ids[] = {
> > > +       {
> > > +               .compatible = "tcg,tpm-tis-mmio",
> > > +               .data = (ulong)&tpm_tis_std_chip_data,
> > > +       },
> > > +       { }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(tpm_tis_mmio) = {
> > > +       .name   = "tpm_tis_mmio",
> > > +       .id     = UCLASS_TPM,
> > > +       .of_match = tpm_tis_ids,
> > > +       .ops    = &tpm_tis_ops,
> > > +       .probe  = tpm_tis_probe,
> > > +       .remove = tpm_tis_remove,
> > > +       .priv_auto      = sizeof(struct tpm_chip),
> > > +};
> > > --
> > > 2.32.0.rc0
> > >
> >
> > Regards,
> > Simon
>
> Thanks for looking at this
> /Ilias

Likewise.

Regards,
Simon