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
