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 :)
> > 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
>
> A non-standard flag. Should be removed. Also
>
> > Reviewed-by: Andrey Pronin <apronin at chromium.org>
> > Tested-by: David Tolnay <dtolnay at gmail.com>
> > Signed-off-by: David Tolnay <dtolnay at gmail.com>
>
> Your SOB should first and you cannot peer test your own patches. Please
> remove tested-by.
>
> The whole thing looks like an early draft. Why the patch does not have
> an RFC tag? You should use it for early drafts. Now it is like saying
> "please merge this".
>
> I don't have much knowledge of virtio. The commit message should at
> least give rough overview what is meant by "kernel side" in this
> context.
>
> Since one cannot use standard Linux environment to test this I'm not
too
> optimistic about this getting merged any time soon. And since even the
> commit message is broken I don't think it makes sense to review the
code
> in detail at this point.
>
> /Jarkko
