Re: [Qemu-devel] [PATCH v5 1/4] Provide support for the CUSE TPM

[Michael S. Tsirkin]

On Wed, Mar 01, 2017 at 06:11:17PM +0000, Daniel P. Berrange wrote:
> On Wed, Mar 01, 2017 at 07:58:36PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Mar 01, 2017 at 05:38:23PM +0000, Daniel P. Berrange wrote:
> > > On Wed, Mar 01, 2017 at 12:25:46PM -0500, Stefan Berger wrote:
> > > > On 03/01/2017 12:16 PM, Michael S. Tsirkin wrote:
> > > > > On Wed, Mar 01, 2017 at 12:12:34PM -0500, Stefan Berger wrote:
> > > > > > On 03/01/2017 12:02 PM, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Mar 01, 2017 at 04:31:04PM +0000, Daniel P. Berrange 
> > > > > > > wrote:
> > > > > > > > On Wed, Mar 01, 2017 at 06:22:45PM +0200, Michael S. Tsirkin 
> > > > > > > > wrote:
> > > > > > > > > On Wed, Mar 01, 2017 at 09:50:38AM -0500, Stefan Berger wrote:
> > > > > > > > > > I had already proposed a linked-in version before I went to 
> > > > > > > > > > the out-of-process
> > > > > > > > > > design. Anthony's concerns back then were related to the 
> > > > > > > > > > code not being trusted
> > > > > > > > > > and a segfault in the code could bring down all of QEMU. 
> > > > > > > > > > That we have test
> > > > > > > > > > suites running over it didn't work as an argument. Some of 
> > > > > > > > > > the test suite are
> > > > > > > > > > private, though.
> > > > > > > > > Given how bad the alternative is maybe we should go back to 
> > > > > > > > > that one.
> > > > > > > > > Same argument can be made for any device and we aren't making
> > > > > > > > > them out of process right now.
> > > > > > > > > 
> > > > > > > > > IIMO it's less the in-process question (modularization
> > > > > > > > > of QEMU has been on the agenda since years and I don't
> > > > > > > > > think anyone is against it) it's more a code 
> > > > > > > > > control/community question.
> > > > > > > > I rather disagree. Modularization of QEMU has seen few results
> > > > > > > > because it is generally a hard problem to solve when you have a
> > > > > > > > complex pre-existing codebase.  I don't think code control has
> > > > > > > > been a factor in this - as long as QEMU can clearly define its
> > > > > > > > ABI/API between core & the modular pieces, it doesn't matter
> > > > > > > > who owns the module. We've seen this with vhost-user which is
> > > > > > > > essentially outsourcing network device backend impls to a 3rd
> > > > > > > > party project.
> > > > > > > And it was done precisely for community reasons.  dpdk/VPP 
> > > > > > > community is
> > > > > > > quite large and fell funded but they just can't all grok QEMU.  
> > > > > > > They
> > > > > > > work for hardware vendors and do baremetal things.  With the 
> > > > > > > split we
> > > > > > > can focus on virtualization and they can focus on moving packets 
> > > > > > > around.
> > > > > > > 
> > > > > > > 
> > > > > > > > QEMU's defined the vhost-user ABI/API and delegated
> > > > > > > > impl to something else.
> > > > > > > The vhost ABI isn't easy to maintain at all though. So I would not
> > > > > > > commit to that lightly without a good reason.
> > > > > > > 
> > > > > > > It will be way more painful if the ABI is dictated by a 3rd party
> > > > > > > library.
> > > > > > Who should define it?
> > > > > > 
> > > > > No one. Put it in same source tree with QEMU and forget ABI stability
> > > > > issues.
> > > > 
> > > > You mean put the code implementing TPM 1.2 and/or TPM 2 into the QEMU 
> > > > tree?
> > > > These are multiple thousands of lines of code each and we'll break them
> > > > apart into logical chunks and review them?
> > > 
> > > No, lets not make that mistake again - we only just got rid of the
> > > libcacard smartcard library code from QEMU git. 
> > 
> > I don't mean that as an external library. As an integral part of QEMU
> > adhering to our coding style etc - why not?
> 
> Changing swtpm to the QEMU coding style is a pointless exercise - just
> busy work for no functional end benefit.

I'm not sure what you are saying here, I don't appreciate extra hurdles
to review, it's hard enough as it is. If others don't care, good for
them.

> You're also tieing the code
> into the QEMU release cycle, again for no tangible benefit.

No need for ABI stability would be the benefit.

> Conceptually
> swtpm does not depend on, or require, QEMU to be useful - it can have
> other non-QEMU consumers - bundling with QEMU is not helpful there.

Maybe it could but it isn't.

> 
> > I don't know what are the other options.  How is depending on an ABI
> > with a utility with no other users and not packaged by most distros
> > good? You are calling out to a CUSE device but who's reviewing that
> > code?
> 
> If anyone is motivated enough to review the code, they can do it whether
> it is in QEMU git or its own git. Pulling entire of swtpm into QEMU GIT
> isn't magically going to get useful reviews done on the code. The QEMU
> maintainers already have far more code to review than available review
> bandwidth, and lack domain knowledge in TPM concepts.

I was the only one merging TPM code so far. I don't call myself an
expert.  If someone steps up to do the work, is trusted by Peter to
maintain it for X years and doesn't care about the extra hurdles, more
power to them.

> > Anyway, it all boils down to lack of reviewers. I know I am not merging
> > the current implementation because I could not figure out what do qemu
> > bits do without looking at the implementation. I don't want to jump
> > between so many trees and coding styles. bios/qemu/linux/dpdk are
> > painful enough to manage. If some other maintainer volunteers, or if
> > Peter wants to merge it directly from Stefan, I won't object.
> 
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|