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

[PATCH v5 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

- v4->v5:
  - Added error path in case tpm2_get_cc_attrs_tbl() fails

- v3->v4:
  - Dropped patch 3; getting command code attributes table in IBM driver

- v2->v3:
  - Added fixes tag to patch 2/4; the race seems to have existed
    since the driver was first added
  - Renamed tpm2_init to tpm2_init_commands in 3/4

- v1->v2:
  - Addressed comments to v1; added patch 3 to handle case when
    TPM_OPS_AUTO_STARTUP is not set





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 |  8 +++++++-
 drivers/char/tpm/tpm.h         |  1 +
 drivers/char/tpm/tpm2-cmd.c    |  2 +-
 drivers/char/tpm/tpm_ibmvtpm.c | 17 +++++++++++++++++
 drivers/char/tpm/tpm_ibmvtpm.h |  1 +
 5 files changed, 27 insertions(+), 2 deletions(-)

-- 
2.23.0

[PATCH v5 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>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/eventlog/of.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index af347c190819..a31a625ad44e 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -17,6 +17,12 @@
 #include "../tpm.h"
 #include "common.h"
 
+static const char * const compatibles[] = {
+	"IBM,vtpm",
+	"IBM,vtpm20",
+	NULL
+};
+
 int tpm_read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
@@ -51,7 +57,7 @@ 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_device_compatible_match(np, compatibles)) {
 		size = be32_to_cpup((__force __be32 *)sizep);
 		base = be64_to_cpup((__force __be64 *)basep);
 	} else {
-- 
2.23.0

[PATCH v5 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.

This patch fixes an existing race condition that may occurr if the
hypervisor does not quickly respond to the VTPM_GET_RTCE_BUFFER_SIZE
request sent during initialization and therefore the ibmvtpm->rtce_buf
has not been allocated at the time the first TPM command is sent.

Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
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 v5 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_CHIP_FLAG_TPM2 flag
and get the command codes attributes table. The driver does
not need the timeouts and durations, though.

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

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 5620747da0cf..ad55c9824338 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -226,6 +226,7 @@ int tpm2_auto_startup(struct tpm_chip *chip);
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
 int tpm2_probe(struct tpm_chip *chip);
+int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
 int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 int tpm2_init_space(struct tpm_space *space);
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 13696deceae8..b6a0ee6bb03a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -613,7 +613,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	return rc;
 }
 
-static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
+int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
 {
 	struct tpm_buf buf;
 	u32 nr_commands;
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index eee566eddb35..676a65148f82 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);
@@ -672,6 +673,13 @@ 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;
+		rc = tpm2_get_cc_attrs_tbl(chip);
+		if (rc)
+			goto init_irq_cleanup;
+	}
+
 	if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
 				ibmvtpm->rtce_buf != NULL,
 				HZ)) {
-- 
2.23.0

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

[Jarkko Sakkinen]

On Thu, Feb 27, 2020 at 10:03:29PM -0500, 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.
> 
> This patch fixes an existing race condition that may occurr if the
> hypervisor does not quickly respond to the VTPM_GET_RTCE_BUFFER_SIZE
> request sent during initialization and therefore the ibmvtpm->rtce_buf
> has not been allocated at the time the first TPM command is sent.
> 
> Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
> 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");

I'd change this something more descriptive "CRQ response timed out".

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Feb 27, 2020 at 10:03:30PM -0500, 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.

I'd prefer "TPM2" over "TPM 2".

> In case a TPM 2 is found, we set the TPM_CHIP_FLAG_TPM2 flag
> and get the command codes attributes table. The driver does
> not need the timeouts and durations, though.

A TPM2 what? TPM2 is not a thing.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  drivers/char/tpm/tpm.h         | 1 +
>  drivers/char/tpm/tpm2-cmd.c    | 2 +-
>  drivers/char/tpm/tpm_ibmvtpm.c | 8 ++++++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 5620747da0cf..ad55c9824338 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -226,6 +226,7 @@ int tpm2_auto_startup(struct tpm_chip *chip);
>  void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
>  unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>  int tpm2_probe(struct tpm_chip *chip);
> +int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip);
>  int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
>  int tpm2_init_space(struct tpm_space *space);
>  void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 13696deceae8..b6a0ee6bb03a 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -613,7 +613,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	return rc;
>  }
>  
> -static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
> +int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
>  {
>  	struct tpm_buf buf;
>  	u32 nr_commands;
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index eee566eddb35..676a65148f82 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);
> @@ -672,6 +673,13 @@ 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;
> +		rc = tpm2_get_cc_attrs_tbl(chip);
> +		if (rc)
> +			goto init_irq_cleanup;
> +	}
> +
>  	if (!wait_event_timeout(ibmvtpm->crq_queue.wq,
>  				ibmvtpm->rtce_buf != NULL,
>  				HZ)) {
> -- 
> 2.23.0
> 

The code change looks fine.

/Jarkko

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

[Stefan Berger]

On 3/2/20 6:15 AM, Jarkko Sakkinen wrote:
> On Thu, Feb 27, 2020 at 10:03:30PM -0500, 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.
> I'd prefer "TPM2" over "TPM 2".
Fixed.
>
>> In case a TPM 2 is found, we set the TPM_CHIP_FLAG_TPM2 flag
>> and get the command codes attributes table. The driver does
>> not need the timeouts and durations, though.
> A TPM2 what? TPM2 is not a thing.


I don't know what you mean? Is it the word 'found' and it should be 
'present' ? Otherwise a TPM2 is a 'thing' / object / device, at least to me.


    Stefan

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

[Nayna]

On 2/27/20 10:03 PM, 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.
>
> This patch fixes an existing race condition that may occurr if the
> hypervisor does not quickly respond to the VTPM_GET_RTCE_BUFFER_SIZE
> request sent during initialization and therefore the ibmvtpm->rtce_buf
> has not been allocated at the time the first TPM command is sent.
>
> Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
> 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 {

Acked-by: Nayna Jain <nayna@linux.ibm.com>

Thanks & Regards,

      - Nayna

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

[Jarkko Sakkinen]

On Mon, Mar 02, 2020 at 11:21:27AM -0500, Stefan Berger wrote:
> On 3/2/20 6:15 AM, Jarkko Sakkinen wrote:
> > On Thu, Feb 27, 2020 at 10:03:30PM -0500, 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.
> > I'd prefer "TPM2" over "TPM 2".
> Fixed.
> > 
> > > In case a TPM 2 is found, we set the TPM_CHIP_FLAG_TPM2 flag
> > > and get the command codes attributes table. The driver does
> > > not need the timeouts and durations, though.
> > A TPM2 what? TPM2 is not a thing.
> 
> 
> I don't know what you mean? Is it the word 'found' and it should be
> 'present' ? Otherwise a TPM2 is a 'thing' / object / device, at least to me.

TPM2 chip would be better. TPM2 can refer either to the protocol or to a
chip.

/Jarkko

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

[Stefan Berger]

On 3/3/20 3:11 PM, Jarkko Sakkinen wrote:
> On Mon, Mar 02, 2020 at 11:21:27AM -0500, Stefan Berger wrote:
>> On 3/2/20 6:15 AM, Jarkko Sakkinen wrote:
>>> On Thu, Feb 27, 2020 at 10:03:30PM -0500, 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.
>>> I'd prefer "TPM2" over "TPM 2".
>> Fixed.
>>>> In case a TPM 2 is found, we set the TPM_CHIP_FLAG_TPM2 flag
>>>> and get the command codes attributes table. The driver does
>>>> not need the timeouts and durations, though.
>>> A TPM2 what? TPM2 is not a thing.
>>
>> I don't know what you mean? Is it the word 'found' and it should be
>> 'present' ? Otherwise a TPM2 is a 'thing' / object / device, at least to me.
> TPM2 chip would be better. TPM2 can refer either to the protocol or to a
> chip.

Let's call it 'device', which in this case is a virtual device rather 
than a 'virtual chip.'


    Stefan

>
> /Jarkko