Re: [SeaBIOS] New build warnings from GCC 8

[Kevin O'Connor]

On Thu, Mar 08, 2018 at 03:16:30PM +0100, Paul Menzel wrote:
> Dear SeaBIOS folks,
> 
> Building SeaBIOS with GCC 8, the warnings below are shown.
[...]
> In file included from src/fw/shadow.c:17:
> src/fw/shadow.c: In function 'qemu_reboot':
> src/string.h:23:16: warning: '__builtin_memcpy' offset -1048576 is out of the 
> bounds [0, 1] of object 'code32flat_start' with type 'char' [-Warray-bounds]
>  #define memcpy __builtin_memcpy
> src/fw/shadow.c:190:9: note: in expansion of macro 'memcpy'
>  memcpy(cstart, cstart + BIOS_SRC_OFFSET, hrp - cstart);
>  ^~

Well, technically the above code isn't valid C.  It's always worked in
practice though.  The code needs to perform some memory copies based
on physical addresses in memory - and we use the ELF symbol table to
get at those memory addresses.

I'm surprsied that gcc still compains after the addresses have been
cast to an integer.  I'm not sure what format would make gcc happy.

-Kevin

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

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Kevin O'Connor]

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
> 0x.  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] preparing 1.11.1 stable release

[Kevin O'Connor]

On Wed, Mar 07, 2018 at 11:49:59AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> Time to prepare a 1.11-stable release, so we can pick up bugfixes for the
> upcoming qemu release.  Looking at the commits I think these should go in:
> 
> a3c93bd81d build: Use git describe --always
> 42812e062a shadow: Don't invoke a shutdown on reboot unless in a reboot loop
> 14d91c353e pci: fix 'io hints' capability for RedHat PCI bridges
> 0541f2f0f2 paravirt: Only enable sercon in NOGRAPHIC mode if no other console 
> specified
> 
> Comments?  Other suggestions?

Sounds good to me.

Thanks,
-Kevin

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

Re: [SeaBIOS] Long delay: WARNING - Timeout at wait_reg8:81!

[Stephen Douthit]

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
0x.  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