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

[Daniel P. Berrange]

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. You're also tieing the code
into the QEMU release cycle, again for no tangible 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.

> 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.

> 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/ :|