Re: [Qemu-devel] [PATCH] tpm: Set tpmRegValidSts flag to '1' in device reset

[Stefan Berger]

On 03/20/2018 10:52 AM, Marc-André Lureau wrote:
> Hi
>
> On Mon, Mar 19, 2018 at 5:21 PM, Stefan Berger
> <address@hidden> wrote:
> > Fix the initialization of the tpmRegValidSts flag and set it to '1'
> > during device reset without expecting a write to another register.
> > This seems to also be the default behavior of real hardware.
> >
> > Signed-off-by: Stefan Berger <address@hidden>
> > ---
> >   hw/tpm/tpm_crb.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > index d8917cb..114b66e 100644
> > --- a/hw/tpm/tpm_crb.c
> > +++ b/hw/tpm/tpm_crb.c
> > @@ -145,8 +145,6 @@ static void tpm_crb_mmio_write(void *opaque, hwaddr addr,
> >                                beenSeized, 0);
> >               ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> >                                locAssigned, 1);
> > -            ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> > -                             tpmRegValidSts, 1);
> >               break;
> >           }
> >           break;
> > @@ -210,6 +208,8 @@ static void tpm_crb_reset(void *dev)
> >
> >       tpm_backend_reset(s->tpmbe);
> >
> > +    ARRAY_FIELD_DP32(s->regs, CRB_LOC_STATE,
> > +                     tpmRegValidSts, 1);
> >       ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> >                        InterfaceType, CRB_INTF_TYPE_CRB_ACTIVE);
> >       ARRAY_FIELD_DP32(s->regs, CRB_INTF_ID,
> Hmm, the specification says they default to 0. Except tpmEstablished
> to 1, which we should probably manipulate somehow ("The TPM clears
> this bit to 0 upon receipt of _TPM_Hash_End The TPM sets this bit to a
> 1 when the TPM_LOC_CTRL_x.resetEstablishment field is set to 1.").
> Help welcome!

Right. We would have to set this flag to '1' -- maybe in a separate 
patch. We don't support DRTM, which is initiated by CPU instructions, 
that would then cause the _TPM_Hash_End. So there's no need to 
manipulate it.
> Shouldn't it also set locAssigned to 1 in this case ?

Stephen Douthit did some experiments with a hardware TPM and posted a 
patch to SeaBIOS mailing list:

https://mail.coreboot.org/pipermail/seabios/2018-March/012221.html

A hardware TPM would see the tpmRegValidSts bit set but not the 
locAssigned bit. I think our method of setting the locAssigned bit 
following the requestAccess bit being set is correct.

Also, in the following spec on pdf page 67 it indicates that after 500 
usec the tpmRegValidSts bit is set to a valid value. This means that no 
other register needs to be written to so that this happens. The text 
later on near Table 24 is a little more vague about that I find.

https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_Specification_Family_2.0_Revision_1.3v22.pdf




> I am not very familiar with the locality support, since I didn't
> implement it for CRB. You may have more clues what is expected here.