Re: [Qemu-devel] [PATCH V15 1/7] Support for TPM command line options

[Stefan Berger]

On 03/27/2012 05:35 PM, Anthony Liguori wrote:
> On 03/27/2012 03:24 PM, Stefan Berger wrote:
> > This patch adds support for TPM command line options.
> > The command line options supported here are
[...]
> > Monitor support for 'info tpm' has been added. It for example prints the
> > following:
> >
> > (qemu) info tpm
> > TPM devices:
> >   tpm0: model=tpm-tis
> >    \ tpm0: type=passthrough,path=/dev/tpm0
> >
> > Signed-off-by: Stefan Berger<address@hidden>
> > ---
[...]
> > --- /dev/null
> > +++ b/hw/tpm_tis.h
> > @@ -0,0 +1,81 @@
> > +/*
> > + * tpm_tis.c - QEMU's TPM TIS interface emulator
> > + *
> > + * Copyright (C) 2006,2010,2011 IBM Corporation
> > + *
> > + * Authors:
> > + *  Stefan Berger<address@hidden>
> > + *  David Safford<address@hidden>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation, version 2 of the
> > + * License.
>
> Or later please.  We're sticking with GPLv2 as our effective license 
> but asking that all new code is GPLv2 or later.

Forgot that one. Fixed.
> > + *
> > + *
> > + * Implementation of the TIS interface according to specs found at
> > + * http://www.trustedcomputiggroup.org
> > + *
> > + */
> > +#ifndef HW_TPM_TIS_H
> > +#define HW_TPM_TIS_H
> > +
> > +#include "isa.h"
> > +#include "qemu-thread.h"
>
> This shouldn't be needed anymore.

Indeed. Removed.+
> > +typedef struct TPMTISState {
> > +    uint32_t offset;
> > +    uint8_t buf[TPM_TIS_BUFFER_MAX];
> > +
> > +    uint8_t active_locty;
> > +    uint8_t aborting_locty;
> > +    uint8_t next_locty;
> > +
> > +    TPMLocality loc[TPM_TIS_NUM_LOCALITIES];
> > +
> > +    qemu_irq irq;
> > +    uint32_t irq_num;
>
> I'm a bit confused here.  If TPMTISState has an irq, shouldn't it be a 
> DeviceState?

So I guess you would expect the irq to be part of TPMState?diff --git 
a/qapi-schema.json b/qapi-schema.json

> > index 0d11d6e..4ad6d29 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -1701,3 +1701,32 @@
> >   # Since: 1.1
> >   ##
> >   { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
> > +
> > +##
> > +# @TPMInfo:
> > +#
> > +# Information about the TPM
> > +#
> > +# @model: The TPM frontend model, i.e., tpm-tis
> > +#
> > +# @id: The ID of the TPM
> > +#
> > +# @type: The type of TPM backend, i.e., passthrough
> > +#
> > +# @parameters: Additional parameters of the TPM backend device
>
> This is in some sort of key=value format?  Why not specify those 
> parameters properly in the schema as optional items?

Yes, it's in key=value format and it is optional. So, I fixed this now 
with #optional and '*parameters'. Should it make
a difference in the code, except for the auto-generated code?
+
> > +/* overall state of the TPM interface */
> > +typedef struct TPMState {
> > +    ISADevice busdev;
> > +    MemoryRegion mmio;
> > +
> > +    union {
> > +        TPMTISState tis;
> > +    } s;
> > +
> > +    uint8_t     command_locty;
> > +    TPMLocality *cmd_locty;
> > +
> > +    char *backend;
> > +    TPMBackend *be_driver;
> > +} TPMState;
>
> I'm a bit confused at what the relationship between TPMTISState and 
> TPMSTate.

The rational is that TPMState could accomodate different types of 
front-ends in the union with the part in the union being private to the 
front-end. Right now there is only a TIS emulator, there could be a 
virtio (with the restriction that it would have to support TPMLocality). 
Obviously there is a backend driver necessary for this split between 
front- and backend to work, so this is what the 'char *backend' and 
TPMBackend *be_driver are good for. cmd_locty is shared between front 
and backend and is set by the frontend and read by the backend.

Regards,
   Stefan