Re: [PATCH] tpm: Add driver for TPM over virtio

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Feb 22, 2019 at 01:40:25PM -0800, David Tolnay wrote:
> On 2/21/19 9:51 PM, Michael S. Tsirkin wrote:
> > On Thu, Feb 21, 2019 at 06:14:02PM -0800, David Tolnay wrote:
> >> Add a config TCG_VIRTIO_VTPM which enables a driver providing the guest
> >> kernel side of TPM over virtio.
> >>
> >> Use case: TPM support is needed for performing trusted work from within
> >> a virtual machine launched by Chrome OS.
> >>
> >> Tested inside crosvm, the Chrome OS virtual machine monitor. Crosvm's
> >> implementation of the virtio TPM device can be found in these two source
> >> files:
> >>
> >> - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/devices/src/virtio/tpm.rs
> >> - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/tpm2/src/lib.rs
> >>
> >> and is currently backed by the libtpm2 TPM simulator:
> >>
> >> - https://chromium.googlesource.com/chromiumos/third_party/tpm2/
> >>
> >> Reviewed-on: https://chromium-review.googlesource.com/1387655
> >> Reviewed-by: Andrey Pronin <apronin@chromium.org>
> >> Tested-by: David Tolnay <dtolnay@gmail.com>
> >> Signed-off-by: David Tolnay <dtolnay@gmail.com>
> >> ---
> >> UNRESOLVED:
> >> The driver assumes virtio device number VIRTIO_ID_TPM=31. If there is
> >> interest in accepting a virtio TPM driver into the kernel, the Chrome OS
> >> team will coordinate with the OASIS virtio technical committee to secure
> >> an agreed upon device number and include it in a later patchset.
> > 
> > I am not a tpm expert but I don't see why not.
> > 
> > 
> >>  drivers/char/tpm/Kconfig      |   9 +
> >>  drivers/char/tpm/Makefile     |   1 +
> >>  drivers/char/tpm/tpm_virtio.c | 460 ++++++++++++++++++++++++++++++++++
> > 
> > Maintainer entry?
> 
> Good call, I will add one.
> 
> Could you let me know what workflow would work best for you? I can direct
> patches toward Chrome OS's kernel tree and only a Chrome OS list, then send them
> your way only once they have been reviewed and accepted there. Or I can list one
> or both of the linux-integrity@v.k.o and virtualization@l.l-f.o lists along with
> a list for Chrome OS reviewers.
> 
> Either way, we'll want eyes from people who know virtio and from people who know
> TPM on most changes.

Well if you are changing a host/guest interface then you also need to copy
virtio-dev. That one is subscriber only so that would
imply sending there after it's reviewed in chrome.
As an extra bonus reviewed code is hopefully higher quality
so less review work for me ;)
so the first option sounds like a better plan.

But I hope accepted above just means it goes on some branch,
such that we won't end up with production code that
is set in stone and needs to be maintained forever?

> 
> > 
> >>  3 files changed, 470 insertions(+)
> >>  create mode 100644 drivers/char/tpm/tpm_virtio.c
> >>
> >> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> >> index 536e55d3919f..8997060e248e 100644
> >> --- a/drivers/char/tpm/Kconfig
> >> +++ b/drivers/char/tpm/Kconfig
> >> @@ -164,6 +164,15 @@ config TCG_VTPM_PROXY
> >>  	  /dev/vtpmX and a server-side file descriptor on which the vTPM
> >>  	  can receive commands.
> >>  
> >> +config TCG_VIRTIO_VTPM
> >> +	tristate "Virtio vTPM"
> >> +	depends on TCG_TPM
> >> +	help
> >> +	  This driver provides the guest kernel side of TPM over Virtio. If
> >> +	  you are building Linux to run inside of a hypervisor that supports
> >> +	  TPM over Virtio, say Yes and the virtualized TPM will be
> >> +	  accessible from the guest.
> >> +
> >>  
> >>  source "drivers/char/tpm/st33zp24/Kconfig"
> >>  endif # TCG_TPM
> >> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> >> index a01c4cab902a..4f5d1071257a 100644
> >> --- a/drivers/char/tpm/Makefile
> >> +++ b/drivers/char/tpm/Makefile
> >> @@ -33,3 +33,4 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> >>  obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
> >>  obj-$(CONFIG_TCG_CRB) += tpm_crb.o
> >>  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> >> +obj-$(CONFIG_TCG_VIRTIO_VTPM) += tpm_virtio.o
> >> diff --git a/drivers/char/tpm/tpm_virtio.c b/drivers/char/tpm/tpm_virtio.c
> >> new file mode 100644
> >> index 000000000000..f3239d983f18
> >> --- /dev/null
> >> +++ b/drivers/char/tpm/tpm_virtio.c
> >> @@ -0,0 +1,460 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright 2019 Google Inc.
> >> + *
> >> + * Author: David Tolnay <dtolnay@gmail.com>
> >> + *
> >> + * ---
> >> + *
> >> + * Device driver for TPM over virtio.
> >> + *
> >> + * This driver employs a single virtio queue to handle both send and recv. TPM
> >> + * commands are sent over virtio to the hypervisor during a TPM send operation
> >> + * and responses are received over the same queue during a recv operation.
> >> + *
> >> + * The driver contains a single buffer that is the only buffer we ever place on
> >> + * the virtio queue. Commands are copied from the caller's command buffer into
> >> + * the driver's buffer before handing off to virtio, and responses are received
> >> + * into the driver's buffer then copied into the caller's response buffer. This
> >> + * allows us to be resilient to timeouts. When a send or recv operation times
> >> + * out, the caller is free to destroy their buffer; we don't want the hypervisor
> >> + * continuing to perform reads or writes against that destroyed buffer.
> >> + *
> >> + * This driver does not support concurrent send and recv operations. Mutual
> >> + * exclusion is upheld by the tpm_mutex lock held in tpm-interface.c around the
> >> + * calls to chip->ops->send and chip->ops->recv.
> >> + *
> >> + * The intended hypervisor-side implementation is as follows.
> >> + *
> >> + *     while true:
> >> + *         await next virtio buffer.
> >> + *         expect first descriptor in chain to be guest-to-host.
> >> + *         read tpm command from that buffer.
> >> + *         synchronously perform TPM work determined by the command.
> >> + *         expect second descriptor in chain to be host-to-guest.
> >> + *         write TPM response into that buffer.
> >> + *         place buffer on virtio used queue indicating how many bytes written.
> > 
> > That's fine I think except generally it should be legal for guest
> > to split each buffer to several segments.
> 
> Acknowledged. I will adjust this comment.
> 
> To clarify, this means the hypervisor will need to accept a single descriptor
> chain consisting of one or more guest-to-host descriptors which together form
> the command, followed by one or more host-to-guest descriptors into which the
> response may be written. Is that what you had in mind?

Exactly.

> TPM commands and responses are both capped at a moderate size (4096 bytes). With
> that in mind, is it still necessary to allow split buffers? We won't be dealing
> with multi-megabyte transfers.

This has been a general virtio rule, yes. See for example "2.6.4 Message
Framing" in v1.1 draft.

It's generally not hard to implement and generally easier than argue about
whether it's necessary. If you think it's a big problem, it's a good
idea to take it up with the virtio tc. Address this to virtio-comment
list.

> 
> >> + */
> >> +
> >> +#include <linux/virtio_config.h>
> >> +
> >> +#include "tpm.h"
> >> +
> >> +/*
> >> + * Timeout duration when waiting on the hypervisor to complete its end of the
> >> + * TPM operation. This timeout is relatively high because certain TPM operations
> >> + * can take dozens of seconds.
> >> + */
> >> +#define TPM_VIRTIO_TIMEOUT (120 * HZ)
> > 
> > Should we read this from device? E.g. a busy hypervisor might
> > take longer to respond ...
> 
> That's true. Do you have an example of an existing virtio driver that reads
> timeout from the hypervisor?
> 
> To understand your perspective, do you see one of the following as being the
> bigger advantage? -- either that we can use a timeout tighter than 120s for
> hypervisors that believe they can respond quickly to any command, or that we can
> relax the timeout beyond 120s for an especially overburdened hypervisor. The
> first is nice because the caller in the guest will receive their error code
> sooner in certain failure modes, but maybe that would be better addressed by
> some other way of poking the host to find out whether it is still alive and
> working. For the second, 120s is pretty generous and in our use case we would
> just want to give up and retry much later at that point; I don't know how much
> further it would be reasonable to grow a timeout.

I think the whole idea around timeout handling needs a bit more
thought. What kind of reasons for the timeout do you envision
that require the extra kicks?


> 
> >> +
> >> +struct vtpm_device {
> >> +	/*
> >> +	 * Data structure for integration with the common code of the TPM driver
> >> +	 * in tpm-chip.c.
> >> +	 */
> >> +	struct tpm_chip *chip;
> >> +
> >> +	/*
> >> +	 * Virtio queue for sending TPM commands out of the virtual machine and
> >> +	 * receiving TPM responses back from the hypervisor.
> >> +	 */
> >> +	struct virtqueue *vq;
> >> +
> >> +	/*
> >> +	 * Completion that is notified when a virtio operation has been
> >> +	 * fulfilled by the hypervisor.
> >> +	 */
> >> +	struct completion complete;
> >> +
> >> +	/*
> >> +	 * Whether driver currently holds ownership of the virtqueue buffer.
> >> +	 * When false, the hypervisor is in the process of reading or writing
> >> +	 * the buffer and the driver must not touch it.
> >> +	 */
> >> +	bool driver_has_buffer;
> >> +
> >> +	/*
> >> +	 * Whether during the most recent TPM operation, a virtqueue_kick failed
> >> +	 * or a wait timed out.
> >> +	 *
> >> +	 * The next send or recv operation will attempt a kick upon seeing this
> >> +	 * status. That should clear up the queue in the case that the
> >> +	 * hypervisor became temporarily nonresponsive, such as by resource
> >> +	 * exhaustion on the host. The extra kick enables recovery from kicks
> >> +	 * going unnoticed by the hypervisor as well as recovery from virtio
> >> +	 * callbacks going unnoticed by the guest kernel.
> > 
> > Well not necessarily. E.g. virtqueue_kick does not
> > kick if hypervisor didn't enable notifications
> > (e.g. with event idx they get disabled automatically).
> > So it won't recover if hypervisor can discard
> > kicks.
> > 
> > I think this comment needs clarification.
> 
> Makes sense. I didn't think to consider "hypervisor discards kicks" as a failure
> mode. :) Would virtqueue_kick return false in that case? If so, needs_kick will
> stay true and the kick will keep being retried on subsequent send/recv
> operations until the hypervisor has enabled notifications again.
> 
> I will clarify the comment to touch on that situation.

IIRC virtqueue_kick returns false when device is broken.

If you want to handle that you generally need to reset
the device. Kicking it won't fix it :)

> 
> >> +	 */
> >> +	bool needs_kick;
> >> +
> >> +	/* Number of bytes available to read from the virtqueue buffer. */
> >> +	unsigned int readable;
> >> +
> >> +	/*
> >> +	 * Buffer in which all virtio transfers take place. Buffer size is the
> >> +	 * maximum legal TPM command or response message size.
> >> +	 */
> >> +	u8 virtqueue_buffer[TPM_BUFSIZE];
> >> +};
> >> +
> >> +/*
> >> + * Wait for ownership of the virtqueue buffer.
> >> + *
> >> + * The why-string should begin with "waiting to..." or "waiting for..." with no
> >> + * trailing newline. It will appear in log output.
> >> + *
> >> + * Returns zero for success, otherwise negative error.
> >> + */
> >> +static int vtpm_wait_for_buffer(struct vtpm_device *dev, const char *why)
> >> +{
> >> +	int ret;
> >> +	bool did_kick;
> >> +	struct tpm_chip *chip = dev->chip;
> >> +	unsigned long deadline = jiffies + TPM_VIRTIO_TIMEOUT;
> >> +
> >> +	/* Kick queue if needed. */
> >> +	if (dev->needs_kick) {
> >> +		did_kick = virtqueue_kick(dev->vq);
> >> +		if (!did_kick) {
> >> +			dev_notice(&chip->dev, "kick failed; will retry\n");
> >> +			return -EBUSY;
> >> +		}
> >> +		dev->needs_kick = false;
> >> +	}
> >> +
> >> +	while (!dev->driver_has_buffer) {
> >> +		unsigned long now = jiffies;
> >> +
> >> +		/* Check timeout, otherwise `deadline - now` may underflow. */
> >> +		if time_after_eq(now, deadline) {
> >> +			dev_warn(&chip->dev, "timed out %s\n", why);
> >> +			dev->needs_kick = true;
> >> +			return -ETIMEDOUT;
> >> +		}
> >> +
> >> +		/*
> >> +		 * Wait to be signaled by virtio callback.
> >> +		 *
> >> +		 * Positive ret is jiffies remaining until timeout when the
> >> +		 * completion occurred, which means successful completion. Zero
> >> +		 * ret is timeout. Negative ret is error.
> >> +		 */
> >> +		ret = wait_for_completion_killable_timeout(
> >> +				&dev->complete, deadline - now);
> >> +
> >> +		/* Log if completion did not occur. */
> >> +		if (ret == -ERESTARTSYS) {
> >> +			/* Not a warning if it was simply interrupted. */
> >> +			dev_dbg(&chip->dev, "interrupted %s\n", why);
> >> +		} else if (ret == 0) {
> >> +			dev_warn(&chip->dev, "timed out %s\n", why);
> >> +			ret = -ETIMEDOUT;
> > 
> > Should we check NEEDS_RESET bit and try to reset the device?
> > Or is that too drastic?
> 
> I'll let you make the call on whether we need a reset implemented. This driver
> was initially based on the simple virtio-rng driver which doesn't reset (but nor
> does it detect timeout). Let me know and I can give it a shot.

I think yes - generally NEEDS_RESET is the only mechanism we have for recovering from
device errors. And yes no one implemented it yet :)



> 
> > 
> >> +		} else if (ret < 0) {
> >> +			dev_warn(&chip->dev, "failed while %s: error %d\n",
> >> +					why, -ret);
> >> +		}
> >> +
> >> +		/*
> >> +		 * Return error if completion did not occur. Schedule kick to be
> >> +		 * retried at the start of the next send/recv to help unblock
> >> +		 * the queue.
> >> +		 */
> >> +		if (ret < 0) {
> >> +			dev->needs_kick = true;
> >> +			return ret;
> >> +		}
> >> +
> >> +		/* Completion occurred. Expect response buffer back. */
> >> +		if (virtqueue_get_buf(dev->vq, &dev->readable)) {
> >> +			dev->driver_has_buffer = true;
> >> +
> >> +			if (dev->readable > TPM_BUFSIZE) {
> >> +				dev_crit(&chip->dev,
> >> +						"hypervisor bug: response exceeds max size, %u > %u\n",
> >> +						dev->readable,
> >> +						(unsigned int) TPM_BUFSIZE);
> >> +				dev->readable = TPM_BUFSIZE;
> >> +				return -EPROTO;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int vtpm_op_send(struct tpm_chip *chip, u8 *caller_buf, size_t len)
> >> +{
> >> +	int ret;
> >> +	bool did_kick;
> >> +	struct scatterlist sg_outbuf, sg_inbuf;
> >> +	struct scatterlist *sgs[2] = { &sg_outbuf, &sg_inbuf };
> >> +	struct vtpm_device *dev = dev_get_drvdata(&chip->dev);
> >> +	u8 *virtqueue_buf = dev->virtqueue_buffer;
> >> +
> >> +	dev_dbg(&chip->dev, __func__ " %zu bytes\n", len);
> >> +
> >> +	if (len > TPM_BUFSIZE) {
> >> +		dev_err(&chip->dev,
> >> +				"command is too long, %zu > %zu\n",
> >> +				len, (size_t) TPM_BUFSIZE);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Wait until hypervisor relinquishes ownership of the virtqueue buffer.
> >> +	 *
> >> +	 * This may block if the previous recv operation timed out in the guest
> >> +	 * kernel but is still being processed by the hypervisor. Also may block
> >> +	 * if send operations are performed back-to-back, such as if something
> >> +	 * in the caller failed in between a send and recv.
> >> +	 *
> >> +	 * During normal operation absent of any errors or timeouts, this does
> >> +	 * not block.
> >> +	 */
> >> +	ret = vtpm_wait_for_buffer(dev, "waiting to begin send");
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* Driver owns virtqueue buffer and may now write into it. */
> >> +	memcpy(virtqueue_buf, caller_buf, len);
> >> +
> >> +	/*
> >> +	 * Enqueue the virtqueue buffer once as outgoing virtio data (written by
> >> +	 * the virtual machine and read by the hypervisor) and again as incoming
> >> +	 * data (written by the hypervisor and read by the virtual machine).
> >> +	 * This step moves ownership of the virtqueue buffer from driver to
> >> +	 * hypervisor.
> >> +	 *
> >> +	 * Note that we don't know here how big of a buffer the caller will use
> >> +	 * with their later call to recv. We allow the hypervisor to write up to
> >> +	 * the TPM max message size. If the caller ends up using a smaller
> >> +	 * buffer with recv that is too small to hold the entire response, the
> >> +	 * recv will return an error. This conceivably breaks TPM
> >> +	 * implementations that want to produce a different verbosity of
> >> +	 * response depending on the receiver's buffer size.
> >> +	 */
> >> +	sg_init_one(&sg_outbuf, virtqueue_buf, len);
> >> +	sg_init_one(&sg_inbuf, virtqueue_buf, TPM_BUFSIZE);
> >> +	ret = virtqueue_add_sgs(dev->vq, sgs, 1, 1, virtqueue_buf, GFP_KERNEL);
> >> +	if (ret) {
> >> +		dev_err(&chip->dev, "failed virtqueue_add_sgs\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	/* Kick the other end of the virtqueue after having added a buffer. */
> >> +	did_kick = virtqueue_kick(dev->vq);
> >> +	if (!did_kick) {
> >> +		dev->needs_kick = true;
> >> +		dev_notice(&chip->dev, "kick failed; will retry\n");
> >> +
> >> +		/*
> >> +		 * We return 0 anyway because what the caller doesn't know can't
> >> +		 * hurt them. They can call recv and it will retry the kick. If
> >> +		 * that works, everything is fine.
> >> +		 *
> >> +		 * If the retry in recv fails too, they will get -EBUSY from
> >> +		 * recv.
> >> +		 */
> >> +	}
> >> +
> >> +	/*
> >> +	 * Hypervisor is now processing the TPM command asynchronously. It will
> >> +	 * read the command from the output buffer and write the response into
> >> +	 * the input buffer (which are the same buffer). When done, it will send
> >> +	 * back the buffers over virtio and the driver's virtio callback will
> >> +	 * complete dev->complete so that we know the response is ready to be
> >> +	 * read.
> >> +	 *
> >> +	 * It is important to have copied data out of the caller's buffer into
> >> +	 * the driver's virtqueue buffer because the caller is free to destroy
> >> +	 * their buffer when this call returns. We can't avoid copying by
> >> +	 * waiting here for the hypervisor to finish reading, because if that
> >> +	 * wait times out, we return and the caller may destroy their buffer
> >> +	 * while the hypervisor is continuing to read from it.
> >> +	 */
> >> +	dev->driver_has_buffer = false;
> >> +	return 0;
> >> +}
> >> +
> >> +static int vtpm_op_recv(struct tpm_chip *chip, u8 *caller_buf, size_t len)
> >> +{
> >> +	int ret;
> >> +	struct vtpm_device *dev = dev_get_drvdata(&chip->dev);
> >> +	u8 *virtqueue_buf = dev->virtqueue_buffer;
> >> +
> >> +	dev_dbg(&chip->dev, __func__ "\n");
> >> +
> >> +	/*
> >> +	 * Wait until the virtqueue buffer is owned by the driver.
> >> +	 *
> >> +	 * This will usually block while the hypervisor finishes processing the
> >> +	 * most recent TPM command.
> >> +	 */
> >> +	ret = vtpm_wait_for_buffer(dev, "waiting for TPM response");
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	dev_dbg(&chip->dev, "received %u bytes\n", dev->readable);
> >> +
> >> +	if (dev->readable > len) {
> >> +		dev_notice(&chip->dev,
> >> +				"TPM response is bigger than receiver's buffer: %u > %zu\n",
> >> +				dev->readable, len);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* Copy response over to the caller. */
> >> +	memcpy(caller_buf, virtqueue_buf, dev->readable);
> >> +
> >> +	return dev->readable;
> >> +}
> >> +
> >> +static void vtpm_op_cancel(struct tpm_chip *chip)
> >> +{
> >> +	/*
> >> +	 * Cancel is not called in this driver's use of tpm-interface.c. It may
> >> +	 * be triggered through tpm-sysfs but that can be implemented as needed.
> >> +	 * Be aware that tpm-sysfs performs cancellation without holding the
> >> +	 * tpm_mutex that protects our send and recv operations, so a future
> >> +	 * implementation will need to consider thread safety of concurrent
> >> +	 * send/recv and cancel.
> >> +	 */
> >> +	dev_notice(&chip->dev, "cancellation is not implemented\n");
> >> +}
> >> +
> >> +static u8 vtpm_op_status(struct tpm_chip *chip)
> >> +{
> >> +	/*
> >> +	 * Status is for TPM drivers that want tpm-interface.c to poll for
> >> +	 * completion before calling recv. Usually this is when the hardware
> >> +	 * needs to be polled i.e. there is no other way for recv to block on
> >> +	 * the TPM command completion.
> >> +	 *
> >> +	 * Polling goes until `(status & complete_mask) == complete_val`. This
> >> +	 * driver defines both complete_mask and complete_val as 0 and blocks on
> >> +	 * our own completion object in recv instead.
> >> +	 */
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct tpm_class_ops vtpm_ops = {
> >> +	.flags = TPM_OPS_AUTO_STARTUP,
> >> +	.send = vtpm_op_send,
> >> +	.recv = vtpm_op_recv,
> >> +	.cancel = vtpm_op_cancel,
> >> +	.status = vtpm_op_status,
> >> +	.req_complete_mask = 0,
> >> +	.req_complete_val = 0,
> >> +};
> >> +
> >> +static void vtpm_virtio_complete(struct virtqueue *vq)
> >> +{
> >> +	struct virtio_device *vdev = vq->vdev;
> >> +	struct vtpm_device *dev = vdev->priv;
> >> +
> >> +	complete(&dev->complete);
> >> +}
> >> +
> >> +static int vtpm_probe(struct virtio_device *vdev)
> >> +{
> >> +	int err;
> >> +	struct vtpm_device *dev;
> >> +	struct virtqueue *vq;
> >> +	struct tpm_chip *chip;
> >> +
> >> +	dev_dbg(&vdev->dev, __func__ "\n");
> >> +
> >> +	dev = kzalloc(sizeof(struct vtpm_device), GFP_KERNEL);
> >> +	if (!dev) {
> >> +		err = -ENOMEM;
> >> +		dev_err(&vdev->dev, "failed kzalloc\n");
> >> +		goto err_dev_alloc;
> >> +	}
> >> +	vdev->priv = dev;
> >> +
> >> +	vq = virtio_find_single_vq(vdev, vtpm_virtio_complete, "vtpm");
> >> +	if (IS_ERR(vq)) {
> >> +		err = PTR_ERR(vq);
> >> +		dev_err(&vdev->dev, "failed virtio_find_single_vq\n");
> >> +		goto err_virtio_find;
> >> +	}
> >> +	dev->vq = vq;
> >> +
> >> +	chip = tpm_chip_alloc(&vdev->dev, &vtpm_ops);
> >> +	if (IS_ERR(chip)) {
> >> +		err = PTR_ERR(chip);
> >> +		dev_err(&vdev->dev, "failed tpm_chip_alloc\n");
> >> +		goto err_chip_alloc;
> >> +	}
> >> +	dev_set_drvdata(&chip->dev, dev);
> >> +	chip->flags |= TPM_CHIP_FLAG_TPM2;
> >> +	dev->chip = chip;
> >> +
> >> +	init_completion(&dev->complete);
> >> +	dev->driver_has_buffer = true;
> >> +	dev->needs_kick = false;
> >> +	dev->readable = 0;
> >> +
> >> +	/*
> >> +	 * Required in order to enable vq use in probe function for auto
> >> +	 * startup.
> >> +	 */
> >> +	virtio_device_ready(vdev);
> >> +
> >> +	err = tpm_chip_register(dev->chip);
> >> +	if (err) {
> >> +		dev_err(&vdev->dev, "failed tpm_chip_register\n");
> >> +		goto err_chip_register;
> >> +	}
> >> +
> >> +	return 0;
> >> +
> >> +err_chip_register:
> >> +	put_device(&dev->chip->dev);
> >> +err_chip_alloc:
> >> +	vdev->config->del_vqs(vdev);
> >> +err_virtio_find:
> >> +	kfree(dev);
> >> +err_dev_alloc:
> >> +	return err;
> >> +}
> >> +
> >> +static void vtpm_remove(struct virtio_device *vdev)
> >> +{
> >> +	struct vtpm_device *dev = vdev->priv;
> >> +
> >> +	/* Undo tpm_chip_register. */
> >> +	tpm_chip_unregister(dev->chip);
> >> +
> >> +	/* Undo tpm_chip_alloc. */
> >> +	put_device(&dev->chip->dev);
> >> +
> >> +	vdev->config->reset(vdev);
> >> +	vdev->config->del_vqs(vdev);
> >> +
> >> +	kfree(dev);
> >> +}
> >> +
> >> +#define VIRTIO_ID_TPM 31
> >> +
> >> +static struct virtio_device_id id_table[] = {
> >> +	{
> >> +		.device = VIRTIO_ID_TPM,
> >> +		.vendor = VIRTIO_DEV_ANY_ID,
> >> +	},
> >> +	{},
> >> +};
> > 
> > Let's write
> > 
> > static struct virtio_device_id id_table[] = {
> >         { VIRTIO_ID_TPM, VIRTIO_DEV_ANY_ID },
> >         { 0 },
> > };
> > 
> > for consistency with other virtio devices.
> 
> Acknowledged, will do.
> 
> 
> > 
> >> +
> >> +static struct virtio_driver vtpm_driver = {
> >> +	.driver.name = KBUILD_MODNAME,
> >> +	.driver.owner = THIS_MODULE,
> >> +	.id_table = id_table,
> >> +	.probe = vtpm_probe,
> >> +	.remove = vtpm_remove,
> > 
> > Freeze/restore?
> 
> Good call. We don't need this for Chrome OS but I agree it should be in the
> driver. I will provide a basic freeze/restore in line with what I see in
> virtio-rng.
> 
> Thanks!
> 
> 
> > 
> > 
> >> +};
> >> +
> >> +module_virtio_driver(vtpm_driver);
> >> +
> >> +MODULE_AUTHOR("David Tolnay (dtolnay@gmail.com)");
> >> +MODULE_DESCRIPTION("Virtio vTPM Driver");
> >> +MODULE_VERSION("1.0");
> >> +MODULE_LICENSE("GPL");
> >> -- 
> >> 2.20.1
> >