[SeaBIOS] [PATCH v1 0/3] tpm: Support 2.0 TPM devices connected to a TIS host

Stephen Douthit posted 3 patches 6 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/20180227191711.1943-1-stephend@silicom-usa.com
src/hw/tpm_drivers.c | 113 +++++++++++++++++++++++++++------------------------
1 file changed, 60 insertions(+), 53 deletions(-)
[SeaBIOS] [PATCH v1 0/3] tpm: Support 2.0 TPM devices connected to a TIS host
Posted by Stephen Douthit 6 years ago
Changes from v0 -> v1

Patch 1/3
*Rename sts to value

Patch 2/3
*No Change

Patch 3/3
*No longer sending command to the TPM.  Instead check the
InterfaceVersion field in the TPM_INTF_CAPABILITY register if necessary.
*Removed "Tested-by:" since I wasn't able to find 1.2 hardware for
regression test.

Stephen Douthit (3):
  tpm: Refactor duplicated wait code in tis_wait_sts() & crb_wait_reg()
  tpm: Wait for interface startup when probing
  tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()

 src/hw/tpm_drivers.c | 113 +++++++++++++++++++++++++++------------------------
 1 file changed, 60 insertions(+), 53 deletions(-)

-- 
2.14.3


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v1 0/3] tpm: Support 2.0 TPM devices connected to a TIS host
Posted by Kevin O'Connor 6 years ago
On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
> Changes from v0 -> v1
> 
> Patch 1/3
> *Rename sts to value
> 
> Patch 2/3
> *No Change
> 
> Patch 3/3
> *No longer sending command to the TPM.  Instead check the
> InterfaceVersion field in the TPM_INTF_CAPABILITY register if necessary.
> *Removed "Tested-by:" since I wasn't able to find 1.2 hardware for
> regression test.

Thanks.  I committed this series.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v1 0/3] tpm: Support 2.0 TPM devices connected to a TIS host
Posted by Paul Menzel 6 years ago
Dear Stephen, dear Kevin,


On 03/02/18 17:31, Kevin O'Connor wrote:
> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:

[…]

> Thanks.  I committed this series.

The second commit introduced a regression with coreboot on the ASRock 
E350M1. There are a bunch of time-outs, causing the startup to be really 
slow. With no serial console, the user thinks, it’s not working and 
start to debug.

```
[…]
CBFS: Locating 'fallback/payload'
CBFS: Found @ offset 75840 size 10bfc
Loading segment from ROM address 0xffc75a78
   code (compression=1)
   New segment dstaddr 0xe0200 memsize 0x1fe00 srcaddr 0xffc75ab0 
filesize 0x10bc4
Loading segment from ROM address 0xffc75a94
   Entry Point 0x000fd23d
Loading Segment: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 
filesz: 0x0000000000010bc4
lb: [0x00000000c7eb3000, 0x00000000c7fb5730)
Post relocation: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 
filesz: 0x0000000000010bc4
using LZMA
[ 0x000e0200, 00100000, 0x00100000) <- ffc75ab0
dest 000e0200, end 00100000, bouncebuffer ffffffff
Loaded segments
BS: BS_PAYLOAD_LOAD times (us): entry 0 run 95671 exit 0
Jumping to boot code at 000fd23d(c7d77000)
CPU0: stack: c7eef000 - c7ef0000, lowest used address c7eef6ac, stack 
used: 2388 bytes
SeaBIOS (version rel-1.11.0-25-g5adc8bd)
BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 
binutils: (GNU Binutils) 2.29.1
SeaBIOS (version rel-1.11.0-25-g5adc8bd)
BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 
binutils: (GNU Binutils) 2.29.1
Found coreboot cbmem console @ c7fde000
Found mainboard ASROCK E350M1
Relocating init from 0x000e1840 to 0xc7cf32e0 (size 52352)
Found CBFS header at 0xffc00238
multiboot: eax=c7eeab60, ebx=c7eeab14
Found 24 PCI devices (max PCI bus is 03)
Copying SMBIOS entry point from 0xc7d40000 to 0x000f6120
Copying ACPI RSDP from 0xc7d51000 to 0x000f60f0
Copying MPTABLE from 0xc7d75000/c7d75010 to 0x000f5ef0
Copying PIR from 0xc7d76000 to 0x000f5ec0
Using pmtimer, ioport 0x808
WARNING - Timeout at wait_reg8:81!
WARNING - Timeout at wait_reg8:81!
WARNING - Timeout at wait_reg8:81!
Scan for VGA option rom
Running option rom at c000:0003
Turning on vga text mode console
SeaBIOS (version rel-1.11.0-25-g5adc8bd)
EHCI init on dev 00:12.2 (regs=0xf014c020)
EHCI init on dev 00:13.2 (regs=0xf014d020)
OHCI init on dev 00:12.0 (regs=0xf0148000)
OHCI init on dev 00:13.0 (regs=0xf0149000)
OHCI init on dev 00:14.5 (regs=0xf014a000)
AHCI controller at 00:11.0, iobase 0xf014b000, irq 0
Found 0 lpt ports
Found 1 serial ports
Searching bootorder for: /rom@img/memtest
Searching bootorder for: /rom@img/tint
Searching bootorder for: /rom@img/nvramcui
Searching bootorder for: /rom@img/coreinfo
USB mouse initialized
PS2 keyboard initialized
All threads complete.
Scan for option roms

Press ESC for boot menu.

WARNING - Timeout at wait_reg8:81!
```

Please find the config file attached.

Is it possible to add better error handling? Or should this option 
better be disable by default for people running `make olddefconfig`.


Kind regards,

Paul
#
# Automatically generated file; DO NOT EDIT.
# SeaBIOS Configuration
#

#
# General Features
#
CONFIG_COREBOOT=y
# CONFIG_QEMU is not set
# CONFIG_CSM is not set
# CONFIG_QEMU_HARDWARE is not set
CONFIG_THREADS=y
CONFIG_RELOCATE_INIT=y
CONFIG_BOOTMENU=y
CONFIG_BOOTSPLASH=y
CONFIG_BOOTORDER=y
CONFIG_COREBOOT_FLASH=y
CONFIG_LZMA=y
CONFIG_CBFS_LOCATION=0
CONFIG_MULTIBOOT=y
CONFIG_ENTRY_EXTRASTACK=y
CONFIG_MALLOC_UPPERMEMORY=y
CONFIG_ROM_SIZE=0

#
# Hardware support
#
CONFIG_ATA=y
# CONFIG_ATA_DMA is not set
# CONFIG_ATA_PIO32 is not set
CONFIG_AHCI=y
CONFIG_SDCARD=y
CONFIG_MEGASAS=y
CONFIG_FLOPPY=y
CONFIG_FLASH_FLOPPY=y
CONFIG_NVME=y
CONFIG_PS2PORT=y
CONFIG_USB=y
CONFIG_USB_UHCI=y
CONFIG_USB_OHCI=y
CONFIG_USB_EHCI=y
CONFIG_USB_XHCI=y
CONFIG_USB_MSC=y
CONFIG_USB_UAS=y
CONFIG_USB_HUB=y
CONFIG_USB_KEYBOARD=y
CONFIG_USB_MOUSE=y
CONFIG_SERIAL=y
CONFIG_SERCON=y
CONFIG_LPT=y
CONFIG_RTC_TIMER=y
CONFIG_HARDWARE_IRQ=y
CONFIG_PMTIMER=y
CONFIG_TSC_TIMER=y

#
# BIOS interfaces
#
CONFIG_DRIVES=y
CONFIG_CDROM_BOOT=y
CONFIG_CDROM_EMU=y
CONFIG_PCIBIOS=y
CONFIG_APMBIOS=y
CONFIG_PNPBIOS=y
CONFIG_OPTIONROMS=y
CONFIG_PMM=y
CONFIG_BOOT=y
CONFIG_KEYBOARD=y
CONFIG_KBD_CALL_INT15_4F=y
CONFIG_MOUSE=y
CONFIG_S3_RESUME=y
CONFIG_VGAHOOKS=y
# CONFIG_DISABLE_A20 is not set
CONFIG_TCGBIOS=y

#
# VGA ROM
#
CONFIG_NO_VGABIOS=y
# CONFIG_VGA_GEODEGX2 is not set
# CONFIG_VGA_GEODELX is not set
# CONFIG_VGA_COREBOOT is not set
# CONFIG_BUILD_VGABIOS is not set
CONFIG_VGA_EXTRA_STACK_SIZE=512

#
# Debugging
#
CONFIG_DEBUG_LEVEL=1
CONFIG_DEBUG_SERIAL=y
CONFIG_DEBUG_SERIAL_PORT=0x3f8
CONFIG_DEBUG_COREBOOT=y
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH v1 0/3] tpm: Support 2.0 TPM devices connected to a TIS host
Posted by Stephen Douthit 6 years ago
On 03/06/2018 11:04 AM, Paul Menzel wrote:
> Dear Stephen, dear Kevin,
> 
> 
> On 03/02/18 17:31, Kevin O'Connor wrote:
>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
> 
> […]
> 
>> Thanks.  I committed this series.
> 
> The second commit introduced a regression with coreboot on the ASRock E350M1. There are a bunch of time-outs, causing the startup to be really slow. With no serial console, the user thinks, it’s not working and start to debug.

Looking through the the user manual for that board I don't see that it
has a TPM, or even the header for one, so a timeout seems correct.

Multiple 750ms timeouts does seem pretty painful though.  I hadn't
considered that tis_probe() would be called multiple times if no TPM
was present.

What's the preferred way to have a probe function run and bail before
rerunning the timeout?  Just put a static flag in tis_probe()?  The
attached patch takes that approach.  Please let me know if that fixes
the issue for you, or if there's some other preferred pattern I should
use here.

Thanks,
Steve

> ```
> […]
> CBFS: Locating 'fallback/payload'
> CBFS: Found @ offset 75840 size 10bfc
> Loading segment from ROM address 0xffc75a78
>    code (compression=1)
>    New segment dstaddr 0xe0200 memsize 0x1fe00 srcaddr 0xffc75ab0 filesize 0x10bc4
> Loading segment from ROM address 0xffc75a94
>    Entry Point 0x000fd23d
> Loading Segment: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 filesz: 0x0000000000010bc4
> lb: [0x00000000c7eb3000, 0x00000000c7fb5730)
> Post relocation: addr: 0x00000000000e0200 memsz: 0x000000000001fe00 filesz: 0x0000000000010bc4
> using LZMA
> [ 0x000e0200, 00100000, 0x00100000) <- ffc75ab0
> dest 000e0200, end 00100000, bouncebuffer ffffffff
> Loaded segments
> BS: BS_PAYLOAD_LOAD times (us): entry 0 run 95671 exit 0
> Jumping to boot code at 000fd23d(c7d77000)
> CPU0: stack: c7eef000 - c7ef0000, lowest used address c7eef6ac, stack used: 2388 bytes
> SeaBIOS (version rel-1.11.0-25-g5adc8bd)
> BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 binutils: (GNU Binutils) 2.29.1
> SeaBIOS (version rel-1.11.0-25-g5adc8bd)
> BUILD: gcc: (coreboot toolchain v1.50 October 15th, 2017) 6.3.0 binutils: (GNU Binutils) 2.29.1
> Found coreboot cbmem console @ c7fde000
> Found mainboard ASROCK E350M1
> Relocating init from 0x000e1840 to 0xc7cf32e0 (size 52352)
> Found CBFS header at 0xffc00238
> multiboot: eax=c7eeab60, ebx=c7eeab14
> Found 24 PCI devices (max PCI bus is 03)
> Copying SMBIOS entry point from 0xc7d40000 to 0x000f6120
> Copying ACPI RSDP from 0xc7d51000 to 0x000f60f0
> Copying MPTABLE from 0xc7d75000/c7d75010 to 0x000f5ef0
> Copying PIR from 0xc7d76000 to 0x000f5ec0
> Using pmtimer, ioport 0x808
> WARNING - Timeout at wait_reg8:81!
> WARNING - Timeout at wait_reg8:81!
> WARNING - Timeout at wait_reg8:81!
> Scan for VGA option rom
> Running option rom at c000:0003
> Turning on vga text mode console
> SeaBIOS (version rel-1.11.0-25-g5adc8bd)
> EHCI init on dev 00:12.2 (regs=0xf014c020)
> EHCI init on dev 00:13.2 (regs=0xf014d020)
> OHCI init on dev 00:12.0 (regs=0xf0148000)
> OHCI init on dev 00:13.0 (regs=0xf0149000)
> OHCI init on dev 00:14.5 (regs=0xf014a000)
> AHCI controller at 00:11.0, iobase 0xf014b000, irq 0
> Found 0 lpt ports
> Found 1 serial ports
> Searching bootorder for: /rom@img/memtest
> Searching bootorder for: /rom@img/tint
> Searching bootorder for: /rom@img/nvramcui
> Searching bootorder for: /rom@img/coreinfo
> USB mouse initialized
> PS2 keyboard initialized
> All threads complete.
> Scan for option roms
> 
> Press ESC for boot menu.
> 
> WARNING - Timeout at wait_reg8:81!
> ```
> 
> Please find the config file attached.
> 
> Is it possible to add better error handling? Or should this option better be disable by default for people running `make olddefconfig`.
> 
> 
> Kind regards,
> 
> Paul

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
[SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81! (was: [PATCH v1 0/3] tpm: Support 2.0 TPM devices connected to a TIS host)
Posted by Paul Menzel 6 years ago
Dear Stephen,


Thank you for your quick response.

Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
> On 03/06/2018 11:04 AM, Paul Menzel wrote:
> > On 03/02/18 17:31, Kevin O'Connor wrote:
> > > 
> > > On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
> > […]
> > 
> > > 
> > > Thanks.  I committed this series.
> > The second commit introduced a regression with coreboot on the
> > ASRock E350M1. There are a bunch of time-outs, causing the startup
> > to be really slow. With no serial console, the user thinks, it’s
> > not working and start to debug.
>
> Looking through the the user manual for that board I don't see that it
> has a TPM, or even the header for one, so a timeout seems correct.

Indeed, no TPM is present.

> Multiple 750ms timeouts does seem pretty painful though.  I hadn't
> considered that tis_probe() would be called multiple times if no TPM
> was present.
> 
> What's the preferred way to have a probe function run and bail before
> rerunning the timeout?  Just put a static flag in tis_probe()?  The
> attached patch takes that approach.  Please let me know if that fixes
> the issue for you, or if there's some other preferred pattern I should
> use here.

Unfortunately, that didn’t help.

```
$ git log --oneline -2
fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts
5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
```

And the time-outs seem to be around 20 seconds or more. Please find the
log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).


Kind regards,

Paul_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
On 03/07/2018 10:33 AM, Paul Menzel wrote:
> Dear Stephen,
> 
> 
> Thank you for your quick response.
> 
> Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
>> On 03/06/2018 11:04 AM, Paul Menzel wrote:
>>> On 03/02/18 17:31, Kevin O'Connor wrote:
>>>>
>>>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
>>> […]
>>>
>>>>
>>>> Thanks.  I committed this series.
>>> The second commit introduced a regression with coreboot on the
>>> ASRock E350M1. There are a bunch of time-outs, causing the startup
>>> to be really slow. With no serial console, the user thinks, it’s
>>> not working and start to debug.
>>
>> Looking through the the user manual for that board I don't see that it
>> has a TPM, or even the header for one, so a timeout seems correct.
> 
> Indeed, no TPM is present.

Thanks for confirming.

>> Multiple 750ms timeouts does seem pretty painful though.  I hadn't
>> considered that tis_probe() would be called multiple times if no TPM
>> was present.
>>
>> What's the preferred way to have a probe function run and bail before
>> rerunning the timeout?  Just put a static flag in tis_probe()?  The
>> attached patch takes that approach.  Please let me know if that fixes
>> the issue for you, or if there's some other preferred pattern I should
>> use here.
> 
> Unfortunately, that didn’t help.
> 
> ```
> $ git log --oneline -2
> fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts
> 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
> ```
> 
> And the time-outs seem to be around 20 seconds or more. Please find the
> log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).

Yikes, 20 seconds is the medium duration timeout, not the default A
timeout of 750ms.  I was poking the wrong area with the last patch.
It looks like tis_probe() is propagating the return from
tis_wait_access() in the no device present case.

Please try the attached patch instead of the 'tpm: Save tis_probe...'
and see if that works better.

Thanks,
Steve


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Kevin O'Connor 6 years ago
On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
> On 03/07/2018 10:33 AM, Paul Menzel wrote:
> > Dear Stephen,
> > 
> > 
> > Thank you for your quick response.
> > 
> > Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
> > > On 03/06/2018 11:04 AM, Paul Menzel wrote:
> > > > On 03/02/18 17:31, Kevin O'Connor wrote:
> > > > > 
> > > > > On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
> > > > […]
> > > > 
> > > > > 
> > > > > Thanks.  I committed this series.
> > > > The second commit introduced a regression with coreboot on the
> > > > ASRock E350M1. There are a bunch of time-outs, causing the startup
> > > > to be really slow. With no serial console, the user thinks, it’s
> > > > not working and start to debug.
> > > 
> > > Looking through the the user manual for that board I don't see that it
> > > has a TPM, or even the header for one, so a timeout seems correct.
> > 
> > Indeed, no TPM is present.
> 
> Thanks for confirming.
> 
> > > Multiple 750ms timeouts does seem pretty painful though.  I hadn't
> > > considered that tis_probe() would be called multiple times if no TPM
> > > was present.
> > > 
> > > What's the preferred way to have a probe function run and bail before
> > > rerunning the timeout?  Just put a static flag in tis_probe()?  The
> > > attached patch takes that approach.  Please let me know if that fixes
> > > the issue for you, or if there's some other preferred pattern I should
> > > use here.
> > 
> > Unfortunately, that didn’t help.
> > 
> > ```
> > $ git log --oneline -2
> > fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts
> > 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
> > ```
> > 
> > And the time-outs seem to be around 20 seconds or more. Please find the
> > log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
> 
> Yikes, 20 seconds is the medium duration timeout, not the default A
> timeout of 750ms.  I was poking the wrong area with the last patch.
> It looks like tis_probe() is propagating the return from
> tis_wait_access() in the no device present case.

FYI, even adding 5ms to the boot time is unacceptable.  Is there
anyway to verify the hardware exists before waiting for it to be
ready?

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
> On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
>> On 03/07/2018 10:33 AM, Paul Menzel wrote:
>>> Dear Stephen,
>>>
>>>
>>> Thank you for your quick response.
>>>
>>> Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
>>>> On 03/06/2018 11:04 AM, Paul Menzel wrote:
>>>>> On 03/02/18 17:31, Kevin O'Connor wrote:
>>>>>>
>>>>>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
>>>>> […]
>>>>>
>>>>>>
>>>>>> Thanks.  I committed this series.
>>>>> The second commit introduced a regression with coreboot on the
>>>>> ASRock E350M1. There are a bunch of time-outs, causing the startup
>>>>> to be really slow. With no serial console, the user thinks, it’s
>>>>> not working and start to debug.
>>>>
>>>> Looking through the the user manual for that board I don't see that it
>>>> has a TPM, or even the header for one, so a timeout seems correct.
>>>
>>> Indeed, no TPM is present.
>>
>> Thanks for confirming.
>>
>>>> Multiple 750ms timeouts does seem pretty painful though.  I hadn't
>>>> considered that tis_probe() would be called multiple times if no TPM
>>>> was present.
>>>>
>>>> What's the preferred way to have a probe function run and bail before
>>>> rerunning the timeout?  Just put a static flag in tis_probe()?  The
>>>> attached patch takes that approach.  Please let me know if that fixes
>>>> the issue for you, or if there's some other preferred pattern I should
>>>> use here.
>>>
>>> Unfortunately, that didn’t help.
>>>
>>> ```
>>> $ git log --oneline -2
>>> fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts
>>> 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
>>> ```
>>>
>>> And the time-outs seem to be around 20 seconds or more. Please find the
>>> log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
>>
>> Yikes, 20 seconds is the medium duration timeout, not the default A
>> timeout of 750ms.  I was poking the wrong area with the last patch.
>> It looks like tis_probe() is propagating the return from
>> tis_wait_access() in the no device present case.
> 
> FYI, even adding 5ms to the boot time is unacceptable.  Is there
> anyway to verify the hardware exists before waiting for it to be
> ready?

The only way I know of would be to check if we have TCPA or TPM2 ACPI
tables, and only attempt to probe for a TPM if those are present.

Attached patch should do that, and it's probably a good idea
independent of any of my other patches.
_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Paul Menzel 6 years ago
Dear Stephen,


On 03/07/2018 07:24 PM, Stephen Douthit wrote:
> On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
>> On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
>>> On 03/07/2018 10:33 AM, Paul Menzel wrote:

>>>> Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
>>>>> On 03/06/2018 11:04 AM, Paul Menzel wrote:
>>>>>> On 03/02/18 17:31, Kevin O'Connor wrote:
>>>>>>>
>>>>>>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
>>>>>> […]
>>>>>>
>>>>>>>
>>>>>>> Thanks.  I committed this series.
>>>>>> The second commit introduced a regression with coreboot on the
>>>>>> ASRock E350M1. There are a bunch of time-outs, causing the startup
>>>>>> to be really slow. With no serial console, the user thinks, it’s
>>>>>> not working and start to debug.
>>>>>
>>>>> Looking through the the user manual for that board I don't see that it
>>>>> has a TPM, or even the header for one, so a timeout seems correct.
>>>>
>>>> Indeed, no TPM is present.
>>>
>>> Thanks for confirming.
>>>
>>>>> Multiple 750ms timeouts does seem pretty painful though.  I hadn't
>>>>> considered that tis_probe() would be called multiple times if no TPM
>>>>> was present.
>>>>>
>>>>> What's the preferred way to have a probe function run and bail before
>>>>> rerunning the timeout?  Just put a static flag in tis_probe()?  The
>>>>> attached patch takes that approach.  Please let me know if that fixes
>>>>> the issue for you, or if there's some other preferred pattern I should
>>>>> use here.
>>>>
>>>> Unfortunately, that didn’t help.
>>>>
>>>> ```
>>>> $ git log --oneline -2
>>>> fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save 
>>>> tis_probe() result to avoid a reun of lengthy timeouts
>>>> 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in 
>>>> tis_get_tpm_version()
>>>> ```
>>>>
>>>> And the time-outs seem to be around 20 seconds or more. Please find the
>>>> log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
>>>
>>> Yikes, 20 seconds is the medium duration timeout, not the default A
>>> timeout of 750ms.  I was poking the wrong area with the last patch.
>>> It looks like tis_probe() is propagating the return from
>>> tis_wait_access() in the no device present case.
>>
>> FYI, even adding 5ms to the boot time is unacceptable.  Is there
>> anyway to verify the hardware exists before waiting for it to be
>> ready?
> 
> The only way I know of would be to check if we have TCPA or TPM2 ACPI
> tables, and only attempt to probe for a TPM if those are present.
> 
> Attached patch should do that, and it's probably a good idea
> independent of any of my other patches.

I applied both the latest commits, and quickly testing that, I believe 
the long delay is still there. I won’t be able to get to until next 
week, and make the ACPI tables available. Maybe there is a way to test 
this with QEMU? Kevin also owns the ASRock E350M1 to my knowledge.


Kind regards,

Paul

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
On 03/09/2018 06:05 AM, Paul Menzel wrote:
> Dear Stephen,
> 
> 
> On 03/07/2018 07:24 PM, Stephen Douthit wrote:
>> On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
>>> On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
>>>> On 03/07/2018 10:33 AM, Paul Menzel wrote:
> 
>>>>> Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
>>>>>> On 03/06/2018 11:04 AM, Paul Menzel wrote:
>>>>>>> On 03/02/18 17:31, Kevin O'Connor wrote:
>>>>>>>>
>>>>>>>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
>>>>>>> […]
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks.  I committed this series.
>>>>>>> The second commit introduced a regression with coreboot on the
>>>>>>> ASRock E350M1. There are a bunch of time-outs, causing the startup
>>>>>>> to be really slow. With no serial console, the user thinks, it’s
>>>>>>> not working and start to debug.
>>>>>>
>>>>>> Looking through the the user manual for that board I don't see that it
>>>>>> has a TPM, or even the header for one, so a timeout seems correct.
>>>>>
>>>>> Indeed, no TPM is present.
>>>>
>>>> Thanks for confirming.
>>>>
>>>>>> Multiple 750ms timeouts does seem pretty painful though.  I hadn't
>>>>>> considered that tis_probe() would be called multiple times if no TPM
>>>>>> was present.
>>>>>>
>>>>>> What's the preferred way to have a probe function run and bail before
>>>>>> rerunning the timeout?  Just put a static flag in tis_probe()?  The
>>>>>> attached patch takes that approach.  Please let me know if that fixes
>>>>>> the issue for you, or if there's some other preferred pattern I should
>>>>>> use here.
>>>>>
>>>>> Unfortunately, that didn’t help.
>>>>>
>>>>> ```
>>>>> $ git log --oneline -2
>>>>> fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts
>>>>> 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
>>>>> ```
>>>>>
>>>>> And the time-outs seem to be around 20 seconds or more. Please find the
>>>>> log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
>>>>
>>>> Yikes, 20 seconds is the medium duration timeout, not the default A
>>>> timeout of 750ms.  I was poking the wrong area with the last patch.
>>>> It looks like tis_probe() is propagating the return from
>>>> tis_wait_access() in the no device present case.
>>>
>>> FYI, even adding 5ms to the boot time is unacceptable.  Is there
>>> anyway to verify the hardware exists before waiting for it to be
>>> ready?
>>
>> The only way I know of would be to check if we have TCPA or TPM2 ACPI
>> tables, and only attempt to probe for a TPM if those are present.
>>
>> Attached patch should do that, and it's probably a good idea
>> independent of any of my other patches.
> 
> I applied both the latest commits, and quickly testing that, I believe the long delay is still there. I won’t be able to get to until next week, and make the ACPI tables available. Maybe there is a way to test this with QEMU? Kevin also owns the ASRock E350M1 to my knowledge.

Thanks for the continued testing.  I don't have a good theory for
what's going on at the moment.

It looks like there's a series resistor I can depop to isolate the TPM
reset on the board I was testing on.  I should be able to jumper that
so I can test the TPM and no-TPM cases on the same hardware.
Hopefully I can reproduce the timeout that way.

Kevin - Do you want to revert the second patch for now?  I'll get a
board modded today, and hopefully get a patch out early next week
assuming this reproduces on my system.

Apologies for the thrash here.

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
On 03/09/2018 09:49 AM, Stephen Douthit wrote:
> [This sender failed our fraud detection checks and may not be who they appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
> 
> On 03/09/2018 06:05 AM, Paul Menzel wrote:
>> Dear Stephen,
>>
>>
>> On 03/07/2018 07:24 PM, Stephen Douthit wrote:
>>> On 03/07/2018 12:41 PM, Kevin O'Connor wrote:
>>>> On Wed, Mar 07, 2018 at 12:33:36PM -0500, Stephen Douthit wrote:
>>>>> On 03/07/2018 10:33 AM, Paul Menzel wrote:
>>
>>>>>> Am Dienstag, den 06.03.2018, 11:57 -0500 schrieb Stephen Douthit:
>>>>>>> On 03/06/2018 11:04 AM, Paul Menzel wrote:
>>>>>>>> On 03/02/18 17:31, Kevin O'Connor wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Feb 27, 2018 at 02:17:08PM -0500, Stephen Douthit wrote:
>>>>>>>> […]
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks.  I committed this series.
>>>>>>>> The second commit introduced a regression with coreboot on the
>>>>>>>> ASRock E350M1. There are a bunch of time-outs, causing the startup
>>>>>>>> to be really slow. With no serial console, the user thinks, it’s
>>>>>>>> not working and start to debug.
>>>>>>>
>>>>>>> Looking through the the user manual for that board I don't see that it
>>>>>>> has a TPM, or even the header for one, so a timeout seems correct.
>>>>>>
>>>>>> Indeed, no TPM is present.
>>>>>
>>>>> Thanks for confirming.
>>>>>
>>>>>>> Multiple 750ms timeouts does seem pretty painful though.  I hadn't
>>>>>>> considered that tis_probe() would be called multiple times if no TPM
>>>>>>> was present.
>>>>>>>
>>>>>>> What's the preferred way to have a probe function run and bail before
>>>>>>> rerunning the timeout?  Just put a static flag in tis_probe()?  The
>>>>>>> attached patch takes that approach.  Please let me know if that fixes
>>>>>>> the issue for you, or if there's some other preferred pattern I should
>>>>>>> use here.
>>>>>>
>>>>>> Unfortunately, that didn’t help.
>>>>>>
>>>>>> ```
>>>>>> $ git log --oneline -2
>>>>>> fd1cbb4 (HEAD -> master, origin/master, origin/HEAD) tpm: Save tis_probe() result to avoid a reun of lengthy timeouts
>>>>>> 5adc8bd tpm: Handle unimplemented TIS_REG_IFACE_ID in tis_get_tpm_version()
>>>>>> ```
>>>>>>
>>>>>> And the time-outs seem to be around 20 seconds or more. Please find the
>>>>>> log with time stamps attached (`sudo ./readserial.py /dev/ttyUSB0`).
>>>>>
>>>>> Yikes, 20 seconds is the medium duration timeout, not the default A
>>>>> timeout of 750ms.  I was poking the wrong area with the last patch.
>>>>> It looks like tis_probe() is propagating the return from
>>>>> tis_wait_access() in the no device present case.
>>>>
>>>> FYI, even adding 5ms to the boot time is unacceptable.  Is there
>>>> anyway to verify the hardware exists before waiting for it to be
>>>> ready?
>>>
>>> The only way I know of would be to check if we have TCPA or TPM2 ACPI
>>> tables, and only attempt to probe for a TPM if those are present.
>>>
>>> Attached patch should do that, and it's probably a good idea
>>> independent of any of my other patches.
>>
>> I applied both the latest commits, and quickly testing that, I believe the long delay is still there. I won’t be able to get to until next week, and make the ACPI tables available. Maybe there is a way to test this with QEMU? Kevin also owns the ASRock E350M1 to my knowledge.
> 
> Thanks for the continued testing.  I don't have a good theory for
> what's going on at the moment.
> 
> It looks like there's a series resistor I can depop to isolate the TPM
> reset on the board I was testing on.  I should be able to jumper that
> so I can test the TPM and no-TPM cases on the same hardware.
> Hopefully I can reproduce the timeout that way.

I've got a board modded so I can jumper the TPM in and out.

What I found in the no-TPM case was that both tis_probe() and
crb_probe() incorrectly return 1 for device present if all Fs are read.

For tis_probe() that was because rc wasn't updated to 0 if didvid was
0xffffffff.  For crb_probe() the last three return statements are
inverted from what they should be, and the first 64bit address check
returned the wrong value.  Fixing both probe functions got rid of the
timeout for me when the TPM was disconnected.

It looks like there's a bit in the ACCESS register called Seize that
must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
check that instead of didvid in tis_probe to handle the aborted read all
0s/Fs case.

I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
what's in tis_probe() to avoid potential races on real hardware.
There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could
use as a sanity check against the no device all Fs case.

Let me know if that sounds like a better way to catch the no device
case, or if there's is some other check that would be better.

Thanks,
Steve

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Kevin O'Connor 6 years ago
On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
> I've got a board modded so I can jumper the TPM in and out.
> 
> What I found in the no-TPM case was that both tis_probe() and
> crb_probe() incorrectly return 1 for device present if all Fs are read.
> 
> For tis_probe() that was because rc wasn't updated to 0 if didvid was
> 0xffffffff.  For crb_probe() the last three return statements are
> inverted from what they should be, and the first 64bit address check
> returned the wrong value.  Fixing both probe functions got rid of the
> timeout for me when the TPM was disconnected.
> 
> It looks like there's a bit in the ACCESS register called Seize that
> must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
> check that instead of didvid in tis_probe to handle the aborted read all
> 0s/Fs case.
> 
> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
> what's in tis_probe() to avoid potential races on real hardware.
> There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could
> use as a sanity check against the no device all Fs case.
> 
> Let me know if that sounds like a better way to catch the no device
> case, or if there's is some other check that would be better.

Thanks for looking at this.  It is common on x86 for invalid memory
accesses to return 0xff.  I don't know enough about the TPM hardware
to make a judgement call on the best way to test for presence.  I'd
like to hear what Stefan's thoughts are on this.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stefan Berger 6 years ago
On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
> On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
>> I've got a board modded so I can jumper the TPM in and out.
>>
>> What I found in the no-TPM case was that both tis_probe() and
>> crb_probe() incorrectly return 1 for device present if all Fs are read.
>>
>> For tis_probe() that was because rc wasn't updated to 0 if didvid was
>> 0xffffffff.  For crb_probe() the last three return statements are

static u32 tis_probe(void)
{
     if (!CONFIG_TCGBIOS)
         return 0;

     /* Wait for the interface to report it's ready */
     u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
                              TIS_ACCESS_TPM_REG_VALID_STS,
                              TIS_ACCESS_TPM_REG_VALID_STS);
     if (rc)
         return 0;

     u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));

     if ((didvid != 0) && (didvid != 0xffffffff))
         rc = 1;

     /* TPM 2 has an interface register */
     u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));

     if ((ifaceid & 0xf) != 0xf) {
         if ((ifaceid & 0xf) == 1) {
             /* CRB is active; no TIS */
             return 0;
         }
         if ((ifaceid & (1 << 13)) == 0) {
             /* TIS cannot be selected */
             return 0;
         }
         /* write of 0 to bits 17-18 selects TIS */
         writel(TIS_REG(0, TIS_REG_IFACE_ID), 0);
         /* since we only support TIS, we lock it */
         writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19));
     }

     return rc;
}

When tis_probe() returns '1', it means the interface was detected.
If all registers return 0xffffffff in the no-TPM case we should return a 
'0' from tis_probe since rc was set to 0 from tis_wait_access() and we 
will not get into the ifaceid test case. If they return 0 then we 
shouldn't get past the tis_wait_access() because the expected pattern 
would not show up. I think tis_probe() is correct.

static u32 crb_probe(void)
{
     if (!CONFIG_TCGBIOS)
         return 0;

     u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));

     if ((ifaceid & 0xf) != 0xf) {
         if ((ifaceid & 0xf) == 1) {
             /* CRB is active */
             return 1;
         }
         if ((ifaceid & (1 << 14)) == 0) {
             /* CRB cannot be selected */
             return 0;
         }
         /* write of 1 to bits 17-18 selects CRB */
         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
         /* lock it */
         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
     }

     /* no support for 64 bit addressing yet */
     if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
         return 1;

     u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
     if (addr > 0xffffffff)
         return 1;

     return 0;
}

if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. 
So probably we should test for ifaceid = ~0 after it is read and return 0.



>> inverted from what they should be, and the first 64bit address check
>> returned the wrong value.  Fixing both probe functions got rid of the
>> timeout for me when the TPM was disconnected.

I see a problem with the crb_probe function but not the tis_probe.

>>
>> It looks like there's a bit in the ACCESS register called Seize that
>> must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
>> check that instead of didvid in tis_probe to handle the aborted read all
>> 0s/Fs case.

Why would there be a 'aborted read all'? I think the didvid registers 
should not return either 0 or ~0 for there to be a valid device. Why 
would that not be a good first test to see whether anything is there?

>>
>> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
>> what's in tis_probe() to avoid potential races on real hardware.

That may be something necessary on real hardware, though in case nothing 
is there we all need to wait... I am not sure whether the checking for 
these bits has impact on the didvid registers. ValidSts presumably means 
that the STS register has valid bits. I don't have real hardware anymore 
-- my Acer died a while ago, so I cannot tell but its hardware didn't 
need the wait.

>> There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could
>> use as a sanity check against the no device all Fs case.
>>
>> Let me know if that sounds like a better way to catch the no device
>> case, or if there's is some other check that would be better.
> Thanks for looking at this.  It is common on x86 for invalid memory
> accesses to return 0xff.  I don't know enough about the TPM hardware
> to make a judgement call on the best way to test for presence.  I'd
> like to hear what Stefan's thoughts are on this.
>
> -Kevin
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stefan Berger 6 years ago
On 03/13/2018 07:31 AM, Stefan Berger wrote:
> On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
>> On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
>>> I've got a board modded so I can jumper the TPM in and out.
>>>
>>> What I found in the no-TPM case was that both tis_probe() and
>>> crb_probe() incorrectly return 1 for device present if all Fs are read.
>>>
>>> For tis_probe() that was because rc wasn't updated to 0 if didvid was
>>> 0xffffffff.  For crb_probe() the last three return statements are
>
> static u32 tis_probe(void)
> {
>     if (!CONFIG_TCGBIOS)
>         return 0;
>
>     /* Wait for the interface to report it's ready */
>     u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
>                              TIS_ACCESS_TPM_REG_VALID_STS,
>                              TIS_ACCESS_TPM_REG_VALID_STS);
>     if (rc)
>         return 0;

Can we do without this tis_wait_access on real hardware? Presumably it 
only affects the STS register. If so...

>
>     u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
>
>     if ((didvid != 0) && (didvid != 0xffffffff))
>         rc = 1;

... then we could have an else branch here and 'return 0'.

>
>     /* TPM 2 has an interface register */
>     u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
>
>     if ((ifaceid & 0xf) != 0xf) {
>         if ((ifaceid & 0xf) == 1) {
>             /* CRB is active; no TIS */
>             return 0;
>         }
>         if ((ifaceid & (1 << 13)) == 0) {
>             /* TIS cannot be selected */
>             return 0;
>         }
>         /* write of 0 to bits 17-18 selects TIS */
>         writel(TIS_REG(0, TIS_REG_IFACE_ID), 0);
>         /* since we only support TIS, we lock it */
>         writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19));
>     }
>
>     return rc;
> }
>
> When tis_probe() returns '1', it means the interface was detected.
> If all registers return 0xffffffff in the no-TPM case we should return 
> a '0' from tis_probe since rc was set to 0 from tis_wait_access() and 
> we will not get into the ifaceid test case. If they return 0 then we 
> shouldn't get past the tis_wait_access() because the expected pattern 
> would not show up. I think tis_probe() is correct.
>
> static u32 crb_probe(void)
> {
>     if (!CONFIG_TCGBIOS)
>         return 0;
>
>     u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>
>     if ((ifaceid & 0xf) != 0xf) {
>         if ((ifaceid & 0xf) == 1) {
>             /* CRB is active */
>             return 1;
>         }
>         if ((ifaceid & (1 << 14)) == 0) {
>             /* CRB cannot be selected */
>             return 0;
>         }
>         /* write of 1 to bits 17-18 selects CRB */
>         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>         /* lock it */
>         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>     }
>
>     /* no support for 64 bit addressing yet */
>     if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>         return 1;
>
>     u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>     if (addr > 0xffffffff)
>         return 1;
>
>     return 0;
> }
>
> if ifaceid returns ~0 we may return when reading 
> CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 
> after it is read and return 0.
>
>
>
>>> inverted from what they should be, and the first 64bit address check
>>> returned the wrong value.  Fixing both probe functions got rid of the
>>> timeout for me when the TPM was disconnected.
>
> I see a problem with the crb_probe function but not the tis_probe.
>
>>>
>>> It looks like there's a bit in the ACCESS register called Seize that
>>> must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
>>> check that instead of didvid in tis_probe to handle the aborted read 
>>> all
>>> 0s/Fs case.
>
> Why would there be a 'aborted read all'? I think the didvid registers 
> should not return either 0 or ~0 for there to be a valid device. Why 
> would that not be a good first test to see whether anything is there?
>
>>>
>>> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
>>> what's in tis_probe() to avoid potential races on real hardware.
>
> That may be something necessary on real hardware, though in case 
> nothing is there we all need to wait... I am not sure whether the 
> checking for these bits has impact on the didvid registers. ValidSts 
> presumably means that the STS register has valid bits. I don't have 
> real hardware anymore -- my Acer died a while ago, so I cannot tell 
> but its hardware didn't need the wait.
>
>>> There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we 
>>> could
>>> use as a sanity check against the no device all Fs case.
>>>
>>> Let me know if that sounds like a better way to catch the no device
>>> case, or if there's is some other check that would be better.
>> Thanks for looking at this.  It is common on x86 for invalid memory
>> accesses to return 0xff.  I don't know enough about the TPM hardware
>> to make a judgement call on the best way to test for presence. I'd
>> like to hear what Stefan's thoughts are on this.
>>
>> -Kevin
>>
>
>
> _______________________________________________
> SeaBIOS mailing list
> SeaBIOS@seabios.org
> https://mail.coreboot.org/mailman/listinfo/seabios
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
On 03/13/2018 07:39 AM, Stefan Berger wrote:
> On 03/13/2018 07:31 AM, Stefan Berger wrote:
>> On 03/12/2018 06:11 PM, Kevin O'Connor wrote:
>>> On Mon, Mar 12, 2018 at 01:38:52PM -0400, Stephen Douthit wrote:
>>>> I've got a board modded so I can jumper the TPM in and out.
>>>>
>>>> What I found in the no-TPM case was that both tis_probe() and
>>>> crb_probe() incorrectly return 1 for device present if all Fs are read.
>>>>
>>>> For tis_probe() that was because rc wasn't updated to 0 if didvid was
>>>> 0xffffffff.  For crb_probe() the last three return statements are
>>
>> static u32 tis_probe(void)
>> {
>>     if (!CONFIG_TCGBIOS)
>>         return 0;
>>
>>     /* Wait for the interface to report it's ready */
>>     u32 rc = tis_wait_access(0, TIS_DEFAULT_TIMEOUT_A,
>>                              TIS_ACCESS_TPM_REG_VALID_STS,
>>                              TIS_ACCESS_TPM_REG_VALID_STS);
>>     if (rc)
>>         return 0;
> 
> Can we do without this tis_wait_access on real hardware? Presumably it only affects the STS register. If so...

The reset timing chapter of the spec implies that you need to check this
bit to know when the TPM has completed it's initialization after reset
is released.

>>
>>     u32 didvid = readl(TIS_REG(0, TIS_REG_DID_VID));
>>
>>     if ((didvid != 0) && (didvid != 0xffffffff))
>>         rc = 1;
> 
> ... then we could have an else branch here and 'return 0'.
> 
>>
>>     /* TPM 2 has an interface register */
>>     u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
>>
>>     if ((ifaceid & 0xf) != 0xf) {
>>         if ((ifaceid & 0xf) == 1) {
>>             /* CRB is active; no TIS */
>>             return 0;
>>         }
>>         if ((ifaceid & (1 << 13)) == 0) {
>>             /* TIS cannot be selected */
>>             return 0;
>>         }
>>         /* write of 0 to bits 17-18 selects TIS */
>>         writel(TIS_REG(0, TIS_REG_IFACE_ID), 0);
>>         /* since we only support TIS, we lock it */
>>         writel(TIS_REG(0, TIS_REG_IFACE_ID), (1 << 19));
>>     }
>>
>>     return rc;
>> }
>>
>> When tis_probe() returns '1', it means the interface was detected.
>> If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
>>
>> static u32 crb_probe(void)
>> {
>>     if (!CONFIG_TCGBIOS)
>>         return 0;
>>
>>     u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>
>>     if ((ifaceid & 0xf) != 0xf) {
>>         if ((ifaceid & 0xf) == 1) {
>>             /* CRB is active */
>>             return 1;
>>         }
>>         if ((ifaceid & (1 << 14)) == 0) {
>>             /* CRB cannot be selected */
>>             return 0;
>>         }
>>         /* write of 1 to bits 17-18 selects CRB */
>>         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>         /* lock it */
>>         writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>     }
>>
>>     /* no support for 64 bit addressing yet */
>>     if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>         return 1;
>>
>>     u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>     if (addr > 0xffffffff)
>>         return 1;
>>
>>     return 0;
>> }
>>
>> if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
>>
>>
>>
>>>> inverted from what they should be, and the first 64bit address check
>>>> returned the wrong value.  Fixing both probe functions got rid of the
>>>> timeout for me when the TPM was disconnected.
>>
>> I see a problem with the crb_probe function but not the tis_probe.
>>
>>>>
>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>> must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
>>>> check that instead of didvid in tis_probe to handle the aborted read all
>>>> 0s/Fs case.
>>
>> Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
>>
>>>>
>>>> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
>>>> what's in tis_probe() to avoid potential races on real hardware.
>>
>> That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.
>>
>>>> There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could
>>>> use as a sanity check against the no device all Fs case.
>>>>
>>>> Let me know if that sounds like a better way to catch the no device
>>>> case, or if there's is some other check that would be better.
>>> Thanks for looking at this.  It is common on x86 for invalid memory
>>> accesses to return 0xff.  I don't know enough about the TPM hardware
>>> to make a judgement call on the best way to test for presence. I'd
>>> like to hear what Stefan's thoughts are on this.
>>>
>>> -Kevin
>>>
>>
>>
>> _______________________________________________
>> SeaBIOS mailing list
>> SeaBIOS@seabios.org
>> https://mail.coreboot.org/mailman/listinfo/seabios
>>
> 


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
> When tis_probe() returns '1', it means the interface was detected.
> If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.

Sounds good, I assumed I had screwed this up based on Paul's original
bug report, and was still carrying the patch for my 'fix' to this
function when I was doing my testing.
  
> static u32 crb_probe(void)
> {
>      if (!CONFIG_TCGBIOS)
>          return 0;
> 
>      u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
> 
>      if ((ifaceid & 0xf) != 0xf) {
>          if ((ifaceid & 0xf) == 1) {
>              /* CRB is active */

We also return here without doing the 64 bit address check.

>              return 1;
>          }
>          if ((ifaceid & (1 << 14)) == 0) {
>              /* CRB cannot be selected */
>              return 0;
>          }
>          /* write of 1 to bits 17-18 selects CRB */
>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>          /* lock it */
>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>      }
> 
>      /* no support for 64 bit addressing yet */
>      if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>          return 1;
> 
>      u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>      if (addr > 0xffffffff)
>          return 1;
> 
>      return 0;
> }
> 
> if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
> 
> 
> 
>>> inverted from what they should be, and the first 64bit address check
>>> returned the wrong value.  Fixing both probe functions got rid of the
>>> timeout for me when the TPM was disconnected.
> 
> I see a problem with the crb_probe function but not the tis_probe.
> 
>>>
>>> It looks like there's a bit in the ACCESS register called Seize that
>>> must always read '0' for the version 1.2/1.3 interfaces.  I'd like to
>>> check that instead of didvid in tis_probe to handle the aborted read all
>>> 0s/Fs case.
> 
> Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?

The old parallel PCI bus referred to the termination of any access to a
bad address where no device responded as 'Master Abort'.  That's where I
got the aborted read term from.  As Kevin mentioned, on x86 this case
normally results in a read of all Fs.

The reason I want to move to Seize (or something like it) instead of
didvid is that the spec actually says that _must_ always return '0', I
don't see any explicit restrictions on didvid.

>>>
>>> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
>>> what's in tis_probe() to avoid potential races on real hardware.
> 
> That may be something necessary on real hardware, though in case nothing is there we all need to wait... I am not sure whether the checking for these bits has impact on the didvid registers. ValidSts presumably means that the STS register has valid bits. I don't have real hardware anymore -- my Acer died a while ago, so I cannot tell but its hardware didn't need the wait.

For x86 if there's no device then there is no wait.  The valid flag is
still set in what I was calling the abort case.  In this case it's not
returned by a device, but by the bus read logic.

With the ACPI check reorder we only have to wait in the following
scenario.

1) CONFIG_TPM is set
2) There's a bogus TCPA/TPM2 table for a device that doesn't exist.
3) There's no TPM
4) We're on a platform that returns 0 for aborted reads, so we poll
for a tpmRegValidSts that will never be set.

>>> There's a Seize bit in TPM_LOC_CTRL_x which always reads 0 that we could
>>> use as a sanity check against the no device all Fs case.
>>>
>>> Let me know if that sounds like a better way to catch the no device
>>> case, or if there's is some other check that would be better.
>> Thanks for looking at this.  It is common on x86 for invalid memory
>> accesses to return 0xff.  I don't know enough about the TPM hardware
>> to make a judgement call on the best way to test for presence.  I'd
>> like to hear what Stefan's thoughts are on this.
>>
>> -Kevin
>>
> 


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stefan Berger 6 years ago
On 03/13/2018 10:15 AM, Stephen Douthit wrote:
>> When tis_probe() returns '1', it means the interface was detected.
>> If all registers return 0xffffffff in the no-TPM case we should 
>> return a '0' from tis_probe since rc was set to 0 from 
>> tis_wait_access() and we will not get into the ifaceid test case. If 
>> they return 0 then we shouldn't get past the tis_wait_access() 
>> because the expected pattern would not show up. I think tis_probe() 
>> is correct.
>
> Sounds good, I assumed I had screwed this up based on Paul's original
> bug report, and was still carrying the patch for my 'fix' to this
> function when I was doing my testing.
>
>> static u32 crb_probe(void)
>> {
>>      if (!CONFIG_TCGBIOS)
>>          return 0;
>>
>>      u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>
>>      if ((ifaceid & 0xf) != 0xf) {
>>          if ((ifaceid & 0xf) == 1) {
>>              /* CRB is active */
>
> We also return here without doing the 64 bit address check.

Right...

>
>>              return 1;
>>          }
>>          if ((ifaceid & (1 << 14)) == 0) {
>>              /* CRB cannot be selected */
>>              return 0;
>>          }
>>          /* write of 1 to bits 17-18 selects CRB */
>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>          /* lock it */
>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>      }
>>
>>      /* no support for 64 bit addressing yet */
>>      if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>          return 1;
>>
>>      u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>      if (addr > 0xffffffff)
>>          return 1;
>>
>>      return 0;
>> }
>>
>> if ifaceid returns ~0 we may return when reading 
>> CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 
>> after it is read and return 0.
>>
>>
>>
>>>> inverted from what they should be, and the first 64bit address check
>>>> returned the wrong value.  Fixing both probe functions got rid of the
>>>> timeout for me when the TPM was disconnected.
>>
>> I see a problem with the crb_probe function but not the tis_probe.
>>
>>>>
>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>> must always read '0' for the version 1.2/1.3 interfaces. I'd like to
>>>> check that instead of didvid in tis_probe to handle the aborted 
>>>> read all
>>>> 0s/Fs case.
>>
>> Why would there be a 'aborted read all'? I think the didvid registers 
>> should not return either 0 or ~0 for there to be a valid device. Why 
>> would that not be a good first test to see whether anything is there?
>
> The old parallel PCI bus referred to the termination of any access to a
> bad address where no device responded as 'Master Abort'.  That's where I
> got the aborted read term from.  As Kevin mentioned, on x86 this case
> normally results in a read of all Fs.
>
> The reason I want to move to Seize (or something like it) instead of
> didvid is that the spec actually says that _must_ always return '0', I
> don't see any explicit restrictions on didvid.

We could just look at the vendor id part of didvid and follow this 
document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming 
the list is complete.

https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Registry-Version-1.01-Revision-1.00.pdf

So, following that logic, it cannot return 32bit ~0 (or 0), either. So 
the existing test is fine from what I can see.

I think that should be a first test, maybe also for CRB.

     Stefan


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
On 03/13/2018 10:40 AM, Stefan Berger wrote:
> On 03/13/2018 10:15 AM, Stephen Douthit wrote:
>>> When tis_probe() returns '1', it means the interface was detected.
>>> If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
>>
>> Sounds good, I assumed I had screwed this up based on Paul's original
>> bug report, and was still carrying the patch for my 'fix' to this
>> function when I was doing my testing.
>>
>>> static u32 crb_probe(void)
>>> {
>>>      if (!CONFIG_TCGBIOS)
>>>          return 0;
>>>
>>>      u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>>
>>>      if ((ifaceid & 0xf) != 0xf) {
>>>          if ((ifaceid & 0xf) == 1) {
>>>              /* CRB is active */
>>
>> We also return here without doing the 64 bit address check.
> 
> Right...
> 
>>
>>>              return 1;
>>>          }
>>>          if ((ifaceid & (1 << 14)) == 0) {
>>>              /* CRB cannot be selected */
>>>              return 0;
>>>          }
>>>          /* write of 1 to bits 17-18 selects CRB */
>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>>          /* lock it */
>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>>      }
>>>
>>>      /* no support for 64 bit addressing yet */
>>>      if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>>          return 1;
>>>
>>>      u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>>      if (addr > 0xffffffff)
>>>          return 1;
>>>
>>>      return 0;
>>> }
>>>
>>> if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
>>>
>>>
>>>
>>>>> inverted from what they should be, and the first 64bit address check
>>>>> returned the wrong value.  Fixing both probe functions got rid of the
>>>>> timeout for me when the TPM was disconnected.
>>>
>>> I see a problem with the crb_probe function but not the tis_probe.
>>>
>>>>>
>>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>>> must always read '0' for the version 1.2/1.3 interfaces. I'd like to
>>>>> check that instead of didvid in tis_probe to handle the aborted read all
>>>>> 0s/Fs case.
>>>
>>> Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
>>
>> The old parallel PCI bus referred to the termination of any access to a
>> bad address where no device responded as 'Master Abort'.  That's where I
>> got the aborted read term from.  As Kevin mentioned, on x86 this case
>> normally results in a read of all Fs.
>>
>> The reason I want to move to Seize (or something like it) instead of
>> didvid is that the spec actually says that _must_ always return '0', I
>> don't see any explicit restrictions on didvid.
> 
> We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
> 
> https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Registry-Version-1.01-Revision-1.00.pdf
> 
> So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.

It's fine with the caveat that no vendor in the future is ever assigned
0xffff or 0x0000.  It's a low risk, but not a no risk scenario.

> I think that should be a first test, maybe also for CRB.

I don't think we can make that the first test.  If we don't wait for
tpmRegValidSts (qualified by some known zero bit), then we can't tell
the difference between no-TPM, and reading before the device is ready.

Note 2 in section 6.6 of the TIS 1.3 spec:

2. Within 30 milliseconds of the completion of TPM_Init:
   a. All fields within the access register and all other registers MUST
      return with the state of all their fields valid (i.e.
      TPM_ACCESS_x.tpmRegValidSts is set to ‘1’).
   b. The TPM MUST be ready to receive a command

Maybe the best thing to do is try to skip probing entirely based if we
don't see a related ACPI table.  If we do probe, and have to wait in the
corrected wait code where we qualify tpmRegValidSts against vendor ID or
Seize, and we still find no device, we should probably print a message
to the user.

I don't see a way around some potential wait if we want to support real
hardware devices reliably.

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stephen Douthit 6 years ago
> On 03/13/2018 10:40 AM, Stefan Berger wrote:
>> On 03/13/2018 10:15 AM, Stephen Douthit wrote:
>>>> When tis_probe() returns '1', it means the interface was detected.
>>>> If all registers return 0xffffffff in the no-TPM case we should return a '0' from tis_probe since rc was set to 0 from tis_wait_access() and we will not get into the ifaceid test case. If they return 0 then we shouldn't get past the tis_wait_access() because the expected pattern would not show up. I think tis_probe() is correct.
>>>
>>> Sounds good, I assumed I had screwed this up based on Paul's original
>>> bug report, and was still carrying the patch for my 'fix' to this
>>> function when I was doing my testing.
>>>
>>>> static u32 crb_probe(void)
>>>> {
>>>>      if (!CONFIG_TCGBIOS)
>>>>          return 0;
>>>>
>>>>      u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>>>
>>>>      if ((ifaceid & 0xf) != 0xf) {
>>>>          if ((ifaceid & 0xf) == 1) {
>>>>              /* CRB is active */
>>>
>>> We also return here without doing the 64 bit address check.
>>
>> Right...
>>
>>>
>>>>              return 1;
>>>>          }
>>>>          if ((ifaceid & (1 << 14)) == 0) {
>>>>              /* CRB cannot be selected */
>>>>              return 0;
>>>>          }
>>>>          /* write of 1 to bits 17-18 selects CRB */
>>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>>>          /* lock it */
>>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>>>      }
>>>>
>>>>      /* no support for 64 bit addressing yet */
>>>>      if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>>>          return 1;
>>>>
>>>>      u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>>>      if (addr > 0xffffffff)
>>>>          return 1;
>>>>
>>>>      return 0;
>>>> }
>>>>
>>>> if ifaceid returns ~0 we may return when reading CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 after it is read and return 0.
>>>>
>>>>
>>>>
>>>>>> inverted from what they should be, and the first 64bit address check
>>>>>> returned the wrong value.  Fixing both probe functions got rid of the
>>>>>> timeout for me when the TPM was disconnected.
>>>>
>>>> I see a problem with the crb_probe function but not the tis_probe.
>>>>
>>>>>>
>>>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>>>> must always read '0' for the version 1.2/1.3 interfaces. I'd like to
>>>>>> check that instead of didvid in tis_probe to handle the aborted read all
>>>>>> 0s/Fs case.
>>>>
>>>> Why would there be a 'aborted read all'? I think the didvid registers should not return either 0 or ~0 for there to be a valid device. Why would that not be a good first test to see whether anything is there?
>>>
>>> The old parallel PCI bus referred to the termination of any access to a
>>> bad address where no device responded as 'Master Abort'.  That's where I
>>> got the aborted read term from.  As Kevin mentioned, on x86 this case
>>> normally results in a read of all Fs.
>>>
>>> The reason I want to move to Seize (or something like it) instead of
>>> didvid is that the spec actually says that _must_ always return '0', I
>>> don't see any explicit restrictions on didvid.
>>
>> We could just look at the vendor id part of didvid and follow this document here. There's neither a 0x0000 nor a 0xffff vendor -- assuming the list is complete.
>>
>> https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Registry-Version-1.01-Revision-1.00.pdf
>>
>> So, following that logic, it cannot return 32bit ~0 (or 0), either. So the existing test is fine from what I can see.
> 
> It's fine with the caveat that no vendor in the future is ever assigned
> 0xffff or 0x0000.  It's a low risk, but not a no risk scenario.
> 
>> I think that should be a first test, maybe also for CRB.
> 
> I don't think we can make that the first test.  If we don't wait for
> tpmRegValidSts (qualified by some known zero bit), then we can't tell
> the difference between no-TPM, and reading before the device is ready.
> 
> Note 2 in section 6.6 of the TIS 1.3 spec:
> 
> 2. Within 30 milliseconds of the completion of TPM_Init:
>    a. All fields within the access register and all other registers MUST
>       return with the state of all their fields valid (i.e.
>       TPM_ACCESS_x.tpmRegValidSts is set to ‘1’).
>    b. The TPM MUST be ready to receive a command
> 
> Maybe the best thing to do is try to skip probing entirely based if we
> don't see a related ACPI table.  If we do probe, and have to wait in the
> corrected wait code where we qualify tpmRegValidSts against vendor ID or
> Seize, and we still find no device, we should probably print a message
> to the user.
> 
> I don't see a way around some potential wait if we want to support real
> hardware devices reliably.

Or maybe we could skip polling on QEMU based on CONFIG_QEMU?

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Kevin O'Connor 6 years ago
On Tue, Mar 13, 2018 at 11:36:25AM -0400, Stephen Douthit wrote:
> On 03/13/2018 10:40 AM, Stefan Berger wrote:
> > I think that should be a first test, maybe also for CRB.
> 
> I don't think we can make that the first test.  If we don't wait for
> tpmRegValidSts (qualified by some known zero bit), then we can't tell
> the difference between no-TPM, and reading before the device is ready.
> 
> Note 2 in section 6.6 of the TIS 1.3 spec:
> 
> 2. Within 30 milliseconds of the completion of TPM_Init:
>   a. All fields within the access register and all other registers MUST
>      return with the state of all their fields valid (i.e.
>      TPM_ACCESS_x.tpmRegValidSts is set to ‘1’).
>   b. The TPM MUST be ready to receive a command

I'm not sure of the specifics with the TPM.  But, as a general rule of
thumb, the SeaBIOS code can assume it's been over 30ms since power was
turned on to the machine.

-Kevin

_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stefan Berger 6 years ago
On 03/13/2018 11:36 AM, Stephen Douthit wrote:
> On 03/13/2018 10:40 AM, Stefan Berger wrote:
>> On 03/13/2018 10:15 AM, Stephen Douthit wrote:
>>>> When tis_probe() returns '1', it means the interface was detected.
>>>> If all registers return 0xffffffff in the no-TPM case we should 
>>>> return a '0' from tis_probe since rc was set to 0 from 
>>>> tis_wait_access() and we will not get into the ifaceid test case. 
>>>> If they return 0 then we shouldn't get past the tis_wait_access() 
>>>> because the expected pattern would not show up. I think tis_probe() 
>>>> is correct.
>>>
>>> Sounds good, I assumed I had screwed this up based on Paul's original
>>> bug report, and was still carrying the patch for my 'fix' to this
>>> function when I was doing my testing.
>>>
>>>> static u32 crb_probe(void)
>>>> {
>>>>      if (!CONFIG_TCGBIOS)
>>>>          return 0;
>>>>
>>>>      u32 ifaceid = readl(CRB_REG(0, CRB_REG_INTF_ID));
>>>>
>>>>      if ((ifaceid & 0xf) != 0xf) {
>>>>          if ((ifaceid & 0xf) == 1) {
>>>>              /* CRB is active */
>>>
>>> We also return here without doing the 64 bit address check.
>>
>> Right...
>>
>>>
>>>>              return 1;
>>>>          }
>>>>          if ((ifaceid & (1 << 14)) == 0) {
>>>>              /* CRB cannot be selected */
>>>>              return 0;
>>>>          }
>>>>          /* write of 1 to bits 17-18 selects CRB */
>>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 17));
>>>>          /* lock it */
>>>>          writel(CRB_REG(0, CRB_REG_INTF_ID), (1 << 19));
>>>>      }
>>>>
>>>>      /* no support for 64 bit addressing yet */
>>>>      if (readl(CRB_REG(0, CRB_REG_CTRL_CMD_HADDR)))
>>>>          return 1;
>>>>
>>>>      u64 addr = readq(CRB_REG(0, CRB_REG_CTRL_RSP_ADDR));
>>>>      if (addr > 0xffffffff)
>>>>          return 1;
>>>>
>>>>      return 0;
>>>> }
>>>>
>>>> if ifaceid returns ~0 we may return when reading 
>>>> CRB_REG_CTRL_CMD_HADDR. So probably we should test for ifaceid = ~0 
>>>> after it is read and return 0.
>>>>
>>>>
>>>>
>>>>>> inverted from what they should be, and the first 64bit address check
>>>>>> returned the wrong value.  Fixing both probe functions got rid of 
>>>>>> the
>>>>>> timeout for me when the TPM was disconnected.
>>>>
>>>> I see a problem with the crb_probe function but not the tis_probe.
>>>>
>>>>>>
>>>>>> It looks like there's a bit in the ACCESS register called Seize that
>>>>>> must always read '0' for the version 1.2/1.3 interfaces. I'd like to
>>>>>> check that instead of didvid in tis_probe to handle the aborted 
>>>>>> read all
>>>>>> 0s/Fs case.
>>>>
>>>> Why would there be a 'aborted read all'? I think the didvid 
>>>> registers should not return either 0 or ~0 for there to be a valid 
>>>> device. Why would that not be a good first test to see whether 
>>>> anything is there?
>>>
>>> The old parallel PCI bus referred to the termination of any access to a
>>> bad address where no device responded as 'Master Abort'. That's where I
>>> got the aborted read term from.  As Kevin mentioned, on x86 this case
>>> normally results in a read of all Fs.
>>>
>>> The reason I want to move to Seize (or something like it) instead of
>>> didvid is that the spec actually says that _must_ always return '0', I
>>> don't see any explicit restrictions on didvid.
>>
>> We could just look at the vendor id part of didvid and follow this 
>> document here. There's neither a 0x0000 nor a 0xffff vendor -- 
>> assuming the list is complete.
>>
>> https://trustedcomputinggroup.org/wp-content/uploads/TCG-TPM-Vendor-ID-Registry-Version-1.01-Revision-1.00.pdf 
>>
>>
>> So, following that logic, it cannot return 32bit ~0 (or 0), either. 
>> So the existing test is fine from what I can see.
>
> It's fine with the caveat that no vendor in the future is ever assigned
> 0xffff or 0x0000.  It's a low risk, but not a no risk scenario.
>
>> I think that should be a first test, maybe also for CRB.
>
> I don't think we can make that the first test.  If we don't wait for
> tpmRegValidSts (qualified by some known zero bit), then we can't tell
> the difference between no-TPM, and reading before the device is ready.

Fine, then we add the wait for valid STS register check before that.

>
> Note 2 in section 6.6 of the TIS 1.3 spec:
>
> 2. Within 30 milliseconds of the completion of TPM_Init:
>   a. All fields within the access register and all other registers MUST
>      return with the state of all their fields valid (i.e.
>      TPM_ACCESS_x.tpmRegValidSts is set to ‘1’).
>   b. The TPM MUST be ready to receive a command
>
> Maybe the best thing to do is try to skip probing entirely based if we
> don't see a related ACPI table.  If we do probe, and have to wait in the
> corrected wait code where we qualify tpmRegValidSts against vendor ID or
> Seize, and we still find no device, we should probably print a message
> to the user.

We cannot get rid of the probing entirely since we do have failure 
handling in QEMU that disables the interface (by returning 0 on all 
registers).


>
> I don't see a way around some potential wait if we want to support real
> hardware devices reliably.
>


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios
Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!
Posted by Stefan Berger 6 years ago
On 03/12/2018 01:38 PM, Stephen Douthit wrote:
>
>
> I'd like to add a poll for tpmRegValidSts to crb_probe() similar to
> what's in tis_probe() to avoid potential races on real hardware.

The QEMU CRB currently does not set this bit until access is requested 
by writing bit 0 to TPM_LOC_CTRL_x.

The spec is a bit ambiguous about this bit and says 'The TPM SHALL NOT 
set TPM_LOC_STATE_x.tpmRegValidSts to 1 unless all other fields are 
valid' (description near Table 24 of TCG PC Client Platform TPM Profile 
(PTP) Specification). What makes all the other fields valid? Do you know 
whether your hardware has this bit set to '1' at this point ? I can add 
an initialization to QEMU that sets this bit to '1' as well, but if your 
hardware doesn't have it to '1' I'd rather not do it but do the following:

- request access by writing '1' to TPM_LOC_CTRL
- checking whether this bit is now '1'

The code may be a bit confusing and driven by our QEMU implementation.

     Stefan


_______________________________________________
SeaBIOS mailing list
SeaBIOS@seabios.org
https://mail.coreboot.org/mailman/listinfo/seabios