Re: [Qemu-devel] [PATCH 7/7] Added support for TPM emulator

[Eric Blake]

On 03/31/2017 08:10 AM, Amarnath Valluri wrote:
> This change introduces a new TPM backend driver that can communicates with
> swtpm(software TPM emulator) using unix domain socket interface.
> 
> Swtpm uses two unix sockets, one for plain TPM commands and responses, and one
> for out-of-band control messages.
> 
> The swtpm and associated tools can be found here:
>     https://github.com/stefanberger/swtpm
> 
> Usage:
>     # setup TPM state directory
>     mkdir /tmp/mytpm
>     chown -R tss:root /tmp/mytpm
>     /usr/bin/swtpm_setup --tpm-state /tmp/mytpm --createek
> 
>     # Ask qeum to use TPM emulator with given tpm state directory
>     qemu-system-x86_64 \
>         [...] \
>         -tpmdev 
> emulator,id=tpm0,tpmstatedir=/tmp/mytpm,logfile=/tmp/swtpm.log \
>         -device tpm-tis,tpmdev=tpm0 \
>         [...]
> 
> Signed-off-by: Amarnath Valluri <address@hidden>
> ---

Just an interface review for now:


> +++ b/qapi-schema.json
> @@ -5117,10 +5117,11 @@
>  # An enumeration of TPM types
>  #
>  # @passthrough: TPM passthrough type
> +# @emulator: Software Emulator TPM type

Missing a '(since 2.10)' designator on @emulator

>  #
>  # Since: 1.5
>  ##
> -{ 'enum': 'TpmType', 'data': [ 'passthrough' ] }
> +{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ] }
>  
>  ##
>  # @query-tpm-types:
> @@ -5163,6 +5164,22 @@
>    'data': { '*path' : 'str', '*cancel-path' : 'str'} }
>  
>  ##
> +# @TPMEmulatorOptions:
> +#
> +# Information about the TPM emulator
> +#
> +# @tpmstatedir: TPM emilator state dir

s/emilator/emulator/

> +# @path: TPM emulator binary path to use
> +# @logfile: file to use to place TPM emulator logs

What's the default when logfile is omitted?

> +# @loglevel: log level number

What's the default, or even the valid range of values? Is a larger
number noisier?

> +#
> +# Since: 2.6

You've missed 2.6 by a long shot. We are now working on 2.10 interfaces.

> +##
> +{ 'struct': 'TPMEmulatorOptions', 'base': 'TPMOptions',

Okay, the base class you added in 5/7 makes a bit more sense now, even
if it remains empty.  But then that means you need to update the commit
message to call it out as intentional that it is empty and a second
derived class will be added later.

> +  'data': { 'tpmstatedir' : 'str', '*path': 'str',
> +            '*logfile' : 'str', '*loglevel' : 'int' } }
> +
> +##
>  # @TpmTypeOptions:
>  #
>  # A union referencing different TPM backend types' configuration options
> @@ -5172,7 +5189,8 @@
>  # Since: 1.5
>  ##
>  { 'union': 'TpmTypeOptions',
> -   'data': { 'passthrough' : 'TPMPassthroughOptions' } }
> +  'data': { 'passthrough' : 'TPMPassthroughOptions',
> +            'emulator' : 'TPMEmulatorOptions' } }
>  
>  ##
>  # @TPMInfo:

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature