Re: [RFC 4/6] tests: tpm-emu: Remove assert on TPM2_ST_NO_SESSIONS

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 5 Jun 2020 17:47:08 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Stefan,
> 
> On 6/5/20 5:25 PM, Stefan Berger wrote:
> > On 6/5/20 5:35 AM, Auger Eric wrote:  
> >> Hi Stefan,
> >>
> >> On 6/2/20 6:17 PM, Stefan Berger wrote:  
> >>> On 6/2/20 12:13 PM, Auger Eric wrote:  
> >>>> Hi Stefan,
> >>>>
> >>>> On 6/2/20 3:39 PM, Stefan Berger wrote:  
> >>>>> On 6/1/20 6:21 AM, Eric Auger wrote:  
> >>>>>> While writing tests for checking the content of TPM2 and DSDT
> >>>>>> along with TPM-TIS instantiation I attempted to reuse the
> >>>>>> framework used for TPM-TIS tests. However While dumping the
> >>>>>> ACPI tables I get an assert on TPM2_ST_NO_SESSIONS. My assumption
> >>>>>> is maybe the other tests did not execute long enough to encounter
> >>>>>> this. So I tentatively propose to remove the assert as it
> >>>>>> does not seem to break other tests and enable the new ones.
> >>>>>>
> >>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>>>> ---
> >>>>>>     tests/qtest/tpm-emu.c | 1 -
> >>>>>>     1 file changed, 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/tests/qtest/tpm-emu.c b/tests/qtest/tpm-emu.c
> >>>>>> index c43ac4aef8..298d0eec74 100644
> >>>>>> --- a/tests/qtest/tpm-emu.c
> >>>>>> +++ b/tests/qtest/tpm-emu.c
> >>>>>> @@ -49,7 +49,6 @@ static void *tpm_emu_tpm_thread(void *data)
> >>>>>>             s->tpm_msg->tag = be16_to_cpu(s->tpm_msg->tag);
> >>>>>>             s->tpm_msg->len = be32_to_cpu(s->tpm_msg->len);
> >>>>>>             g_assert_cmpint(s->tpm_msg->len, >=, minhlen);
> >>>>>> -        g_assert_cmpint(s->tpm_msg->tag, ==, TPM2_ST_NO_SESSIONS);  
> >>>>> You should not have to remove this. The tests are skipped if swtpm
> >>>>> does
> >>>>> not support TPM 2 via --tpm2 option. This would be a very old swtpm
> >>>>> version, though. So, all tests are run with --tpm2 option and any
> >>>>> response received from the TPM would be a TPM 2 response that should
> >>>>> have TPM2_ST_NO_SESSIONS as the tag. I'd be curious what other
> >>>>> value you
> >>>>> are seeing there.  
> >>>> If I revert this patch I am getting TPM2_ST_SESSIONS on my end.  
> >>> Is firmware/BIOS active? There's no TPM2_ST_SESSIONS coming out of QEMU.  
> >> So it looks SeaBIOS is in use (bios-256k.bin loaded).
> >>
> >> I can see MMIO accesses to the TPM and the following commands are
> >> observable:
> >> tpm_emu_tpm_thread code=0x181 tag=0x8001 len=0xa
> >> tpm_emu_tpm_thread code=0x144 tag=0x8001 len=0xc
> >> tpm_emu_tpm_thread code=0x121 tag=0x8002 len=0x20
> >> This last one causes the assert (TPM2_CC_HierarchyControl)
> >>
> >> I checked in Seabios and effectively tpm20_hierarchycontrol() tags the
> >> TPM2_CC_HierarchyControl command with TPM2_ST_SESSIONS
> >>
> >> Due to our emulation, maybe tpm_set_failure() gets called, inducing
> >> tpm20_hierarchycontrol() call.
> >>
> >> That being said, what do you recommend? Remove the assert, improve the
> >> emulation, other?  
> > 
> > So this is an ACPI test. What role does the firmware play for success of
> > the test? If the test relies on the firmware showing some sort of
> > expected result, then I would recommend only running this test with an
> > attached swtpm, like we run some other tests. If we don't need the
> > firmware to succeed then I would just get rid of the assert. Probably no
> > other test we have implemented so far was running the firmware...  
> FWIU The goal of this test is to compare the acpi tables generated by
> qemu against reference ones. I dont think we expect from the FW any
> specific result but I would prefer Igor or Michael to confirm.

Firmware is needed to fetch tables from QEMU and place them in guest RAM,
it will also patch cross table pointers accordingly.

So bios-tables-test checks both QEMU and firmware at the same time,
and reference tables are in form guest OS will see them.

> 
> In that case, removing the assert() allows to compare the specific DSDT
> and TPM2 tables and that's our expectation here I think.
> 
> Thanks
> 
> Eric
> > 
> > 
> >    Stefan
> > 
> >   
> >>
> >> Thank you in advance
> >>
> >> Best Regards
> >>
> >> Eric
> >>  
> >>>     Stefan
> >>>
> >>>
> >>>  
> > 
> >