linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Tolnay <dtolnay@gmail.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>,
	Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	linux-integrity@vger.kernel.org, Jason Wang <jasowang@redhat.com>,
	virtualization@lists.linux-foundation.org, dgreid@chromium.org,
	apronin@chromium.org
Subject: Re: [PATCH] tpm: Add driver for TPM over virtio
Date: Fri, 22 Feb 2019 14:24:55 -0800	[thread overview]
Message-ID: <f16bd565-0a7e-d7bb-a5e8-eda48ac8de80@gmail.com> (raw)
In-Reply-To: <20190222215001.GA21427@linux.intel.com>

On 2/22/19 1:50 PM, Jarkko Sakkinen wrote:
> On Fri, Feb 22, 2019 at 04:25:08PM -0500, Michael S. Tsirkin wrote:
>> On Fri, Feb 22, 2019 at 09:33:05PM +0200, Jarkko Sakkinen wrote:
>>> On Fri, Feb 22, 2019 at 09:31:56PM +0200, Jarkko Sakkinen wrote:
>>>> On Fri, Feb 22, 2019 at 10:23:02AM -0500, Michael S. Tsirkin wrote:
>>>>> On Fri, Feb 22, 2019 at 12:26:10PM +0200, Jarkko Sakkinen 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
>>>>>>
>>>>>> These files/links do not make sense for kernel testing. Please remove
>>>>>> them from the next version.
>>>>>
>>>>> To clarify generally for a virtio device we want
>>>>> - guest support
>>>>> - device support
>>>>> - spec
>>>>>
>>>>> If the device is implemented in qemu and guest in linux kernel,
>>>>> then there are lots of people familiar with these
>>>>> programming environments, so sometimes we merge
>>>>> guest and host code even if spec isn't written up at all.
>>>>>
>>>>> If you don't want to do that there's a small number of people who can
>>>>> properly review code, e.g. I don't think lots of people on this list are
>>>>> familiar with crosvm.  One way to address this would be to build a QEMU
>>>>> implementation. Another would be to write up a spec.  You can do both
>>>>> too :)
>>>>
>>>> I don't really understand your arguments.
>>>
>>> ... and I did your response total three times and did not find any
>>> causality of any sort from anything.
>>>
>>> /Jarkko
>>
>> Thanks for spending the time reading my response.  What was included in
>> it was a general suggestion for a virtio based driver to be acceptable
>> in upstream Linux.
>>
>> You pointed out that a pointer to a prototype implementation in Rust
>> isn't relevant. However, FYI just posting guest code and asking for it
>> to be merged alone won't work for a virtio driver either. I am merely
>> trying to speed things up instead of having the contributor repost with
>> a tweaked commit log just to immediately get another set of nacks.
> 
> I did not say anything about relevance of any implementation. I tried to
> mainly point out that looking at random source files implemented with a
> relatively alien language to most does not really make a case.
> 
> Things that would help to move this forward:
> 
> - Documentation of the stack, nothing spectacular but more like how it
>   works in practical terms.
> - Sufficiently easy ways to test the code.
> - Explain in the commit message why we want this.
> 
> /Jarkko


Thanks Jarkko, I respect all of the points you've raised and you are absolutely
correct to ask for a spec and/or spec-quality documentation, detailed testing
steps for exercising this driver in qemu or crosvm, and strong justification for
the new driver.

- Regarding spec, at this point we'll follow up with OASIS to claim a device
  number and pin down the protocol in sufficient detail to make it clear when an
  implementation is conformant. As I noted in the patch message, at the very
  least the virtio device number will need to be resolved before this driver
  could be accepted.

  On the Chrome OS side we're comfortable pursuing review of the spec and review
  of the driver in parallel. At what point in the spec process the driver can be
  accepted is up to you folks.

- I will make sure to include detailed testing steps with the next patch.

- I'm glad we are hashing out why this driver is necessary (or why it is not
  necessary, if it turns out that way) in this discussion. You are right that it
  may have been better originally sent as an RFC. Once I understand there to be
  some agreement, I will write that up for the next patch.

Thanks,
David

  reply	other threads:[~2019-02-22 22:25 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-22  2:14 [PATCH] tpm: Add driver for TPM over virtio David Tolnay
2019-02-22  5:51 ` Michael S. Tsirkin
2019-02-22 21:40   ` David Tolnay
2019-02-22 22:24     ` Michael S. Tsirkin
2019-02-23  1:23       ` David Tolnay
2019-02-25  9:58   ` Jarkko Sakkinen
2019-02-22 10:26 ` Jarkko Sakkinen
2019-02-22 15:23   ` Michael S. Tsirkin
2019-02-22 19:31     ` Jarkko Sakkinen
2019-02-22 19:33       ` Jarkko Sakkinen
2019-02-22 21:25         ` Michael S. Tsirkin
2019-02-22 21:50           ` Jarkko Sakkinen
2019-02-22 22:24             ` David Tolnay [this message]
2019-02-22 22:36               ` Jarkko Sakkinen
2019-02-22 23:05                 ` Michael S. Tsirkin
2019-02-24  9:33                   ` Jarkko Sakkinen
2019-02-22 20:55       ` Michael S. Tsirkin
2019-02-22 21:30         ` Jarkko Sakkinen
2019-02-22 10:30 ` Jarkko Sakkinen
2019-02-22 15:30 ` James Bottomley
2019-02-22 21:16   ` Michael S. Tsirkin
2019-02-22 21:31     ` Jason Gunthorpe
2019-02-22 21:59       ` Jarkko Sakkinen
2019-02-22 22:07         ` Michael S. Tsirkin
2019-02-22 22:14           ` Jarkko Sakkinen
2019-02-22 22:00   ` David Tolnay
2019-02-22 22:18     ` James Bottomley
2019-02-23  0:45       ` David Tolnay
2019-02-23  1:34         ` James Bottomley
2019-02-23  2:41           ` David Tolnay
2019-02-24 16:30             ` James Bottomley
2019-02-24 17:51               ` Jarkko Sakkinen
2019-02-24 22:12               ` David Tolnay
2019-02-25  9:55                 ` Jarkko Sakkinen
2019-02-25 15:36                 ` James Bottomley
2019-02-25 19:17                   ` Matthew Garrett
2019-02-25 19:54                     ` Jarkko Sakkinen
2019-02-25 20:20                     ` James Bottomley
2019-02-25 21:00                       ` Matthew Garrett
2019-02-25 21:02                         ` Matthew Garrett
2019-02-25 22:14                         ` James Bottomley
2019-02-25 22:24                           ` Matthew Garrett
2019-02-25 22:32                             ` James Bottomley
2019-02-25 22:43                               ` Matthew Garrett
2019-02-25 22:51                                 ` James Bottomley
2019-02-25 23:02                                   ` Matthew Garrett
2019-02-25 23:09                                     ` James Bottomley
2019-02-25 21:05                       ` Jarkko Sakkinen
2019-02-25 22:24                         ` James Bottomley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f16bd565-0a7e-d7bb-a5e8-eda48ac8de80@gmail.com \
    --to=dtolnay@gmail.com \
    --cc=apronin@chromium.org \
    --cc=dgreid@chromium.org \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jasowang@redhat.com \
    --cc=jgg@ziepe.ca \
    --cc=linux-integrity@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=peterhuewe@gmx.de \
    --cc=virtualization@lists.linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).