Re: [PATCH] tpm: ibmvtpm: Set TPM_OPS_AUTO_STARTUP flag on driver

[Jarkko Sakkinen]

On Wed Oct 16, 2024 at 12:15 AM EEST, Jarkko Sakkinen wrote:
> On Mon Oct 14, 2024 at 5:16 PM EEST, Stefan Berger wrote:
> > Set the TPM_OPS_AUTO_STARTUP on the driver so that the ibmvtpm driver now
> > uses tpm2_auto_startup and tpm1_auto_startup like many other drivers do.
> > Remove tpm_get_timeouts, tpm2_get_cc_attrs_tbl, and tpm2_sessions_init
> > calls from it since these will all be called in tpm2_auto_startup and
> > tpm1_auto_startup.
> >
> > The exporting of the tpm2_session_init symbol was only necessary while the
> > ibmvtpm driver was calling this function. Since this is not the case
> > anymore, remove this symbol from being exported.
> >
> > What is new for the ibmvtpm driver is that now tpm2_do_selftest and
> > tpm1_do_selftest will be called that send commands to the TPM to perform
> > or continue its selftest. However, the firmware should already have sent
> > these commands so that the TPM will not do much work at this time.
> >
> > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > ---
> >  drivers/char/tpm/tpm2-sessions.c |  1 -
> >  drivers/char/tpm/tpm_ibmvtpm.c   | 15 +--------------
> >  2 files changed, 1 insertion(+), 15 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > index 511c67061728..b1a0a37f14d7 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -1362,5 +1362,4 @@ int tpm2_sessions_init(struct tpm_chip *chip)
> >  
> >  	return rc;
> >  }
> > -EXPORT_SYMBOL(tpm2_sessions_init);
> >  #endif /* CONFIG_TCG_TPM2_HMAC */
> > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> > index 1e5b107d1f3b..76d048f63d55 100644
> > --- a/drivers/char/tpm/tpm_ibmvtpm.c
> > +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> > @@ -450,6 +450,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
> >  }
> >  
> >  static const struct tpm_class_ops tpm_ibmvtpm = {
> > +	.flags = TPM_OPS_AUTO_STARTUP,
> >  	.recv = tpm_ibmvtpm_recv,
> >  	.send = tpm_ibmvtpm_send,
> >  	.cancel = tpm_ibmvtpm_cancel,
> > @@ -690,20 +691,6 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
> >  	if (!strcmp(id->compat, "IBM,vtpm20"))
> >  		chip->flags |= TPM_CHIP_FLAG_TPM2;
> >  
> > -	rc = tpm_get_timeouts(chip);
> > -	if (rc)
> > -		goto init_irq_cleanup;
> > -
> > -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > -		rc = tpm2_get_cc_attrs_tbl(chip);
> > -		if (rc)
> > -			goto init_irq_cleanup;
> > -
> > -		rc = tpm2_sessions_init(chip);
> > -		if (rc)
> > -			goto init_irq_cleanup;
> > -	}
> > -
> >  	return tpm_chip_register(chip);
> >  init_irq_cleanup:
> >  	do {
>
> Not much to say about this ;-)
>
> Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>

Also applied to master. I think that it is better to gather all TPM
bus protection fixes there and send a single pull request for v6.12 with
all the content. So this would include my fixes for AMD issue, this
patch and Mimi's patch.

And also perhaps a few more would squeeze in before that time. E.g.
tpm2_get_random() would be one such place, which could be heavily
optimized by pooling the implementation with couple of thresholds
(lo and hi), and then filling the pool with fixed size requests
from the TPM, i.e.

1: Going below lo: start filling the pool
2. Going above hi: stop filling the pool

perhaps using workqueue. Hwrng would never trigger any TPM command
but instead would be served from the pool what's left there. That
pool could be even something like struct tpm_random_pool with pool
and the tresholds, thus allowing different callers to have their
own.

Sorry, just using this space to document the design to lore :-)

BR, Jarkko