[PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver

[PATCH 0/3] Enable vTPM 2.0 for the IBM vTPM driver

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

QEMU 5.0 will support the PAPR vTPM device model for TPM 1.2 and TPM 2.0.
This series of patches enables vTPM 2.0 support for the IBM vTPM driver.

Regards,
   Stefan

Stefan Berger (3):
  tpm: of: Handle IBM,vtpm20 case when getting log parameters
  tpm: ibmvtpm: Wait for buffer to be set before proceeding
  tpm: ibmvtpm: Add support for TPM 2

 drivers/char/tpm/eventlog/of.c |  3 ++-
 drivers/char/tpm/tpm_ibmvtpm.c | 17 ++++++++++++++++-
 drivers/char/tpm/tpm_ibmvtpm.h |  1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

-- 
2.23.0

[PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
the device tree. Handle it in the same way as 'IBM,vtpm'.

The vTPM 2.0's log is written in little endian format so that for this
aspect we can rely on existing code.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/of.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index af347c190819..a9ce66d09a75 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	 * endian format. For this reason, vtpm doesn't need conversion
 	 * but physical tpm needs the conversion.
 	 */
-	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
+	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
 		size = be32_to_cpup((__force __be32 *)sizep);
 		base = be64_to_cpup((__force __be64 *)basep);
 	} else {
-- 
2.23.0

[PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Synchronize with the results from the CRQs before continuing with
the initialization. This avoids trying to send TPM commands while
the rtce buffer has not been allocated, yet.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 9 +++++++++
 drivers/char/tpm/tpm_ibmvtpm.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 78cc52690177..eee566eddb35 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -571,6 +571,7 @@ static irqreturn_t ibmvtpm_interrupt(int irq, void *vtpm_instance)
 	 */
 	while ((crq = ibmvtpm_crq_get_next(ibmvtpm)) != NULL) {
 		ibmvtpm_crq_process(crq, ibmvtpm);
+		wake_up_interruptible(&ibmvtpm->crq_queue.wq);
 		crq->valid = 0;
 		smp_wmb();
 	}
@@ -618,6 +619,7 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	}
 
 	crq_q->num_entry = CRQ_RES_BUF_SIZE / sizeof(*crq_q->crq_addr);
+	init_waitqueue_head(&crq_q->wq);
 	ibmvtpm->crq_dma_handle = dma_map_single(dev, crq_q->crq_addr,
 						 CRQ_RES_BUF_SIZE,
 						 DMA_BIDIRECTIONAL);
@@ -670,6 +672,13 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	if (rc)
 		goto init_irq_cleanup;
 
+	if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
+				ibmvtpm->rtce_buf != NULL,
+				HZ)) {
+		dev_err(dev, "Initialization failed\n");
+		goto init_irq_cleanup;
+	}
+
 	return tpm_chip_register(chip);
 init_irq_cleanup:
 	do {
diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
index 7983f1a33267..b92aa7d3e93e 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.h
+++ b/drivers/char/tpm/tpm_ibmvtpm.h
@@ -26,6 +26,7 @@ struct ibmvtpm_crq_queue {
 	struct ibmvtpm_crq *crq_addr;
 	u32 index;
 	u32 num_entry;
+	wait_queue_head_t wq;
 };
 
 struct ibmvtpm_dev {
-- 
2.23.0

[PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what
version of TPM is connected through the vio_device_id.

In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to
have properly initialize the TPM and driver.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index eee566eddb35..d479d64a65aa 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm";
 
 static const struct vio_device_id tpm_ibmvtpm_device_table[] = {
 	{ "IBM,vtpm", "IBM,vtpm"},
+	{ "IBM,vtpm", "IBM,vtpm20"},
 	{ "", "" }
 };
 MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
@@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
 	return (status == 0);
 }
 
-static const struct tpm_class_ops tpm_ibmvtpm = {
+static struct tpm_class_ops tpm_ibmvtpm = {
 	.recv = tpm_ibmvtpm_recv,
 	.send = tpm_ibmvtpm_send,
 	.cancel = tpm_ibmvtpm_cancel,
@@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 	if (rc)
 		goto init_irq_cleanup;
 
+	if (!strcmp(id->compat, "IBM,vtpm20")) {
+		chip->flags |= TPM_CHIP_FLAG_TPM2;
+		tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP;
+	}
+
 	if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
 				ibmvtpm->rtce_buf != NULL,
 				HZ)) {
-- 
2.23.0

Re: [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters

[Nayna]

On 2/4/20 8:27 AM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
> the device tree. Handle it in the same way as 'IBM,vtpm'.
>
> The vTPM 2.0's log is written in little endian format so that for this
> aspect we can rely on existing code.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   drivers/char/tpm/eventlog/of.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index af347c190819..a9ce66d09a75 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>   	 * endian format. For this reason, vtpm doesn't need conversion
>   	 * but physical tpm needs the conversion.
>   	 */
> -	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
> +	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> +	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {

How about changing this to use of_device_compatible_match() ?

Thanks & Regards,

       - Nayna

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Nayna]

On 2/4/20 8:27 AM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what
> version of TPM is connected through the vio_device_id.
>
> In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to
> have properly initialize the TPM and driver.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index eee566eddb35..d479d64a65aa 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm";
>
>   static const struct vio_device_id tpm_ibmvtpm_device_table[] = {
>   	{ "IBM,vtpm", "IBM,vtpm"},
> +	{ "IBM,vtpm", "IBM,vtpm20"},
>   	{ "", "" }
>   };
>   MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
> @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
>   	return (status == 0);
>   }
>
> -static const struct tpm_class_ops tpm_ibmvtpm = {
> +static struct tpm_class_ops tpm_ibmvtpm = {
>   	.recv = tpm_ibmvtpm_recv,
>   	.send = tpm_ibmvtpm_send,
>   	.cancel = tpm_ibmvtpm_cancel,
> @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
>   	if (rc)
>   		goto init_irq_cleanup;
>
> +	if (!strcmp(id->compat, "IBM,vtpm20")) {
> +		chip->flags |= TPM_CHIP_FLAG_TPM2;
> +		tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP;

TPM_OPS_AUTO_STARTUP flag isn't set for vTPM 1.2. What is different in 
case of vTPM 2.0 ?

Thanks & Regards,

      - Nayna

Re: [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding

[Nayna]

On 2/4/20 8:27 AM, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Synchronize with the results from the CRQs before continuing with
> the initialization. This avoids trying to send TPM commands while
> the rtce buffer has not been allocated, yet.

Seems like it is not specific to vtpm 2.0.  Shall it be clarified that 
it is a fix for vtpm 1.2 also ?

Thanks & Regards,

     - Nayna

Re: [PATCH 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding

[Stefan Berger]

On 2/13/20 12:53 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Synchronize with the results from the CRQs before continuing with
>> the initialization. This avoids trying to send TPM commands while
>> the rtce buffer has not been allocated, yet.
>
> Seems like it is not specific to vtpm 2.0.  Shall it be clarified that 
> it is a fix for vtpm 1.2 also ?

That's why it says 'TPM commands' and not 'TPM 2.0 commands'.


   Stefan

>
> Thanks & Regards,
>
>     - Nayna
>

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Stefan Berger]

On 2/13/20 12:53 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what
>> version of TPM is connected through the vio_device_id.
>>
>> In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to
>> have properly initialize the TPM and driver.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c 
>> b/drivers/char/tpm/tpm_ibmvtpm.c
>> index eee566eddb35..d479d64a65aa 100644
>> --- a/drivers/char/tpm/tpm_ibmvtpm.c
>> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>> @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = 
>> "tpm_ibmvtpm";
>>
>>   static const struct vio_device_id tpm_ibmvtpm_device_table[] = {
>>       { "IBM,vtpm", "IBM,vtpm"},
>> +    { "IBM,vtpm", "IBM,vtpm20"},
>>       { "", "" }
>>   };
>>   MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table);
>> @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct 
>> tpm_chip *chip, u8 status)
>>       return (status == 0);
>>   }
>>
>> -static const struct tpm_class_ops tpm_ibmvtpm = {
>> +static struct tpm_class_ops tpm_ibmvtpm = {
>>       .recv = tpm_ibmvtpm_recv,
>>       .send = tpm_ibmvtpm_send,
>>       .cancel = tpm_ibmvtpm_cancel,
>> @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev 
>> *vio_dev,
>>       if (rc)
>>           goto init_irq_cleanup;
>>
>> +    if (!strcmp(id->compat, "IBM,vtpm20")) {
>> +        chip->flags |= TPM_CHIP_FLAG_TPM2;
>> +        tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP;
>
> TPM_OPS_AUTO_STARTUP flag isn't set for vTPM 1.2. What is different in 
> case of vTPM 2.0 ?


I don't want side effects for the TPM 1.2 case here, so I am only 
modifying the flag for the case where the new TPM 2 is being used.  
Here's the code where it shows the effect.

int tpm_auto_startup(struct tpm_chip *chip)
{
     int rc;

     if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP))
         return 0;

     if (chip->flags & TPM_CHIP_FLAG_TPM2)
         rc = tpm2_auto_startup(chip);
     else
         rc = tpm1_auto_startup(chip);

     return rc;
}

In the TPM 2 case we then get timeouts, do the TPM self test, send 
TPM2_STARTUP if necessary and get attributes of the TPM 2 command from 
the device. All necessary to start it up.


https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm2-cmd.c#L719

Does this answer your question ?


    Stefan




>
> Thanks & Regards,
>
>      - Nayna
>

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Jason Gunthorpe]

On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:

> I don't want side effects for the TPM 1.2 case here, so I am only modifying
> the flag for the case where the new TPM 2 is being used.  Here's the code
> where it shows the effect.

I'm surprised this driver is using AUTO_STARTUP, it was intended for
embedded cases where their is no firmware to boot the TPM.

Chips using AUTO_STARTUP are basically useless for PCRs/etc.

I'd expect somthing called vtpm to have been started and PCRs working
before Linux is started??

Jason

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Stefan Berger]

On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
>
>> I don't want side effects for the TPM 1.2 case here, so I am only modifying
>> the flag for the case where the new TPM 2 is being used.  Here's the code
>> where it shows the effect.
> I'm surprised this driver is using AUTO_STARTUP, it was intended for
> embedded cases where their is no firmware to boot the TPM.


The TIS is also using it on any device.

static const struct tpm_class_ops tpm_tis = {
     .flags = TPM_OPS_AUTO_STARTUP,
     .status = tpm_tis_status,

https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis_core.c#L917


>
> Chips using AUTO_STARTUP are basically useless for PCRs/etc.
>
> I'd expect somthing called vtpm to have been started and PCRs working
> before Linux is started??

Yes, there's supposed to be firmware.

I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary 
to call. This caller happens to be in tpm2_auto_startup.


    Stefan


>
> Jason

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Jason Gunthorpe]

On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
> On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
> > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
> > 
> > > I don't want side effects for the TPM 1.2 case here, so I am only modifying
> > > the flag for the case where the new TPM 2 is being used.  Here's the code
> > > where it shows the effect.
> > I'm surprised this driver is using AUTO_STARTUP, it was intended for
> > embedded cases where their is no firmware to boot the TPM.
> 
> The TIS is also using it on any device.

TIS is a generic driver, and can run on TPMs without firmware
support. It doesn't know either way

> > Chips using AUTO_STARTUP are basically useless for PCRs/etc.
> > 
> > I'd expect somthing called vtpm to have been started and PCRs working
> > before Linux is started??
> 
> Yes, there's supposed to be firmware.
> 
> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
> call. This caller happens to be in tpm2_auto_startup.

That seems to be a mistake, proper startup of the driver should never
require auto_startup.

Jason

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Stefan Berger]

On 2/13/20 2:11 PM, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
>> On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
>>> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
>>>
>>>> I don't want side effects for the TPM 1.2 case here, so I am only modifying
>>>> the flag for the case where the new TPM 2 is being used.  Here's the code
>>>> where it shows the effect.
>>> I'm surprised this driver is using AUTO_STARTUP, it was intended for
>>> embedded cases where their is no firmware to boot the TPM.
>> The TIS is also using it on any device.
> TIS is a generic driver, and can run on TPMs without firmware
> support. It doesn't know either way

The following drivers are all using it:


drivers/char/tpm/st33zp24/st33zp24.c, line 493
drivers/char/tpm/tpm-interface.c, line 374
drivers/char/tpm/tpm_crb.c, line 421
drivers/char/tpm/tpm_ftpm_tee.c, line 184
drivers/char/tpm/tpm_i2c_atmel.c, line 139
drivers/char/tpm/tpm_i2c_infineon.c, line 602
drivers/char/tpm/tpm_i2c_nuvoton.c, line 465
drivers/char/tpm/tpm_tis_core.c, line 917
drivers/char/tpm/tpm_vtpm_proxy.c, line 435

https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP


>
>>> Chips using AUTO_STARTUP are basically useless for PCRs/etc.
>>>
>>> I'd expect somthing called vtpm to have been started and PCRs working
>>> before Linux is started??
>> Yes, there's supposed to be firmware.
>>
>> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
>> call. This caller happens to be in tpm2_auto_startup.
> That seems to be a mistake, proper startup of the driver should never
> require auto_startup.

Is this IBM vTPM driver special that it should do things differently 
than all those drivers listed above? From looking at the code is seems 
it is to be set for the TPM 2.0 case.


   Stefan

Re: [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters

[Stefan Berger]

On 2/13/20 12:46 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
>> the device tree. Handle it in the same way as 'IBM,vtpm'.
>>
>> The vTPM 2.0's log is written in little endian format so that for this
>> aspect we can rely on existing code.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/eventlog/of.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/eventlog/of.c 
>> b/drivers/char/tpm/eventlog/of.c
>> index af347c190819..a9ce66d09a75 100644
>> --- a/drivers/char/tpm/eventlog/of.c
>> +++ b/drivers/char/tpm/eventlog/of.c
>> @@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>        * endian format. For this reason, vtpm doesn't need conversion
>>        * but physical tpm needs the conversion.
>>        */
>> -    if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
>> +    if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> +        of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>
> How about changing this to use of_device_compatible_match() ?


I can change it.

    Stefan

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Jason Gunthorpe]

On Thu, Feb 13, 2020 at 02:15:03PM -0500, Stefan Berger wrote:
> On 2/13/20 2:11 PM, Jason Gunthorpe wrote:
> > On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
> > > On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
> > > > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
> > > > 
> > > > > I don't want side effects for the TPM 1.2 case here, so I am only modifying
> > > > > the flag for the case where the new TPM 2 is being used.  Here's the code
> > > > > where it shows the effect.
> > > > I'm surprised this driver is using AUTO_STARTUP, it was intended for
> > > > embedded cases where their is no firmware to boot the TPM.
> > > The TIS is also using it on any device.
> > TIS is a generic driver, and can run on TPMs without firmware
> > support. It doesn't know either way
> 
> The following drivers are all using it:
> 
> 
> drivers/char/tpm/st33zp24/st33zp24.c, line 493
> drivers/char/tpm/tpm-interface.c, line 374
> drivers/char/tpm/tpm_crb.c, line 421
> drivers/char/tpm/tpm_ftpm_tee.c, line 184
> drivers/char/tpm/tpm_i2c_atmel.c, line 139
> drivers/char/tpm/tpm_i2c_infineon.c, line 602
> drivers/char/tpm/tpm_i2c_nuvoton.c, line 465
> drivers/char/tpm/tpm_tis_core.c, line 917
> drivers/char/tpm/tpm_vtpm_proxy.c, line 435
> 
> https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP

These are all general purpose drivers.

Though perhaps vtpm_proxy shouldn't include it, not sure.

> > > > Chips using AUTO_STARTUP are basically useless for PCRs/etc.
> > > > 
> > > > I'd expect somthing called vtpm to have been started and PCRs working
> > > > before Linux is started??
> > > Yes, there's supposed to be firmware.
> > > 
> > > I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
> > > call. This caller happens to be in tpm2_auto_startup.
> > That seems to be a mistake, proper startup of the driver should never
> > require auto_startup.
> 
> Is this IBM vTPM driver special that it should do things differently than
> all those drivers listed above? From looking at the code is seems it is to
> be set for the TPM 2.0 case.

Any driver that knows the TPM must be started prior to Linux
booting should not use the flag.  vtpm drivers in general would seem
to be the case where we can make this statement.

If it was mandatory then it would not be a flag the driver has to specify.

Jason

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Stefan Berger]

On 2/13/20 2:39 PM, Jason Gunthorpe wrote:
> On Thu, Feb 13, 2020 at 02:15:03PM -0500, Stefan Berger wrote:
>> On 2/13/20 2:11 PM, Jason Gunthorpe wrote:
>>> On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote:
>>>> On 2/13/20 1:35 PM, Jason Gunthorpe wrote:
>>>>> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote:
>>>>>
>>>>>> I don't want side effects for the TPM 1.2 case here, so I am only modifying
>>>>>> the flag for the case where the new TPM 2 is being used.  Here's the code
>>>>>> where it shows the effect.
>>>>> I'm surprised this driver is using AUTO_STARTUP, it was intended for
>>>>> embedded cases where their is no firmware to boot the TPM.
>>>> The TIS is also using it on any device.
>>> TIS is a generic driver, and can run on TPMs without firmware
>>> support. It doesn't know either way
>> The following drivers are all using it:
>>
>>
>> drivers/char/tpm/st33zp24/st33zp24.c, line 493
>> drivers/char/tpm/tpm-interface.c, line 374
>> drivers/char/tpm/tpm_crb.c, line 421
>> drivers/char/tpm/tpm_ftpm_tee.c, line 184
>> drivers/char/tpm/tpm_i2c_atmel.c, line 139
>> drivers/char/tpm/tpm_i2c_infineon.c, line 602
>> drivers/char/tpm/tpm_i2c_nuvoton.c, line 465
>> drivers/char/tpm/tpm_tis_core.c, line 917
>> drivers/char/tpm/tpm_vtpm_proxy.c, line 435
>>
>> https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP
> These are all general purpose drivers.
>
> Though perhaps vtpm_proxy shouldn't include it, not sure.
>
>>>>> Chips using AUTO_STARTUP are basically useless for PCRs/etc.
>>>>>
>>>>> I'd expect somthing called vtpm to have been started and PCRs working
>>>>> before Linux is started??
>>>> Yes, there's supposed to be firmware.
>>>>
>>>> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to
>>>> call. This caller happens to be in tpm2_auto_startup.
>>> That seems to be a mistake, proper startup of the driver should never
>>> require auto_startup.
>> Is this IBM vTPM driver special that it should do things differently than
>> all those drivers listed above? From looking at the code is seems it is to
>> be set for the TPM 2.0 case.
> Any driver that knows the TPM must be started prior to Linux
> booting should not use the flag.  vtpm drivers in general would seem
> to be the case where we can make this statement.

Wouldn't this statement apply to all systems, including embedded ones?  
Basically all firmwares should implement the CRTM and do the TPM 
initialization.


>
> If it was mandatory then it would not be a flag the driver has to specify.

I'll add a case where we call into  tpm2_get_cc_attrs_tbl(chip) in case 
the auto startup flag is not set. I believe this is the only part that's 
missing for TPM 2 to work if not autostarted.


    Stefan


>
> Jason

Re: [PATCH 3/3] tpm: ibmvtpm: Add support for TPM 2

[Jason Gunthorpe]

On Thu, Feb 13, 2020 at 02:45:49PM -0500, Stefan Berger wrote:
> > Any driver that knows the TPM must be started prior to Linux
> > booting should not use the flag.  vtpm drivers in general would seem
> > to be the case where we can make this statement.
> 
> Wouldn't this statement apply to all systems, including embedded ones? 
> Basically all firmwares should implement the CRTM and do the TPM
> initialization.

It is not mandatory that systems with TPMs start it in the
firmware. That is only required if the TPM PCR feature is going to be
used. A TPM can quite happily be used for key storage without FW
support.

Arguably this is sort of done wrong. eg if the platform has provided
TPM information through uEFI or something then we shouldn't try to
auto start the system TPM. At least for TPM1 detecting a non-started
TPM was harmless, so nobody really cared. I wonder if TPM2 is much
different..

But certainly auto startup should *not* be required to have a working
TPM driver, that is just fundamentally wrong.

Jason

Re: [PATCH 1/3] tpm: of: Handle IBM,vtpm20 case when getting log parameters

[Stefan Berger]

On 2/13/20 12:46 PM, Nayna wrote:
>
> On 2/4/20 8:27 AM, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> A vTPM 2.0 is identified by 'IBM,vtpm20' in the 'compatible' node in
>> the device tree. Handle it in the same way as 'IBM,vtpm'.
>>
>> The vTPM 2.0's log is written in little endian format so that for this
>> aspect we can rely on existing code.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/eventlog/of.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/eventlog/of.c 
>> b/drivers/char/tpm/eventlog/of.c
>> index af347c190819..a9ce66d09a75 100644
>> --- a/drivers/char/tpm/eventlog/of.c
>> +++ b/drivers/char/tpm/eventlog/of.c
>> @@ -51,7 +51,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>        * endian format. For this reason, vtpm doesn't need conversion
>>        * but physical tpm needs the conversion.
>>        */
>> -    if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0) {
>> +    if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> +        of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>
> How about changing this to use of_device_compatible_match() ?


I have to roll back the change to using use_of_device_compatible_match() 
due to a build failure on xtensa. So we'll  be using what I had in this 
patch here.


    Stefan