December 2015 - SeaBIOS - mail.coreboot.org

Re: [SeaBIOS] SeaBIOS Digest, Vol 72, Issue 33
by Kevin O'Connor Jan. 8, 2016

Jan. 8, 2016

On Mon, Dec 21, 2015 at 11:50:07AM -0500, Stefan Berger wrote: > "Kevin O'Connor" <kevin(a)koconnor.net> wrote on 12/17/2015 05:22:56 PM: > > > > > On Mon, Nov 30, 2015 at 11:32:05AM +0000, Wim Vervoorn wrote: > > > Hello, > > > > > > I noticed that a lot of work is going on for the TPM support in > SeaBIOS. > > > > > > All of this work is TPM 1.2 based. I was wondering if there are any > > > plans to support TPM 2.0 in the future. > > > > I'm not aware of any plans. > > We're working on it... > > > So maybe you have some comments to the following: > > There will be a patch for probing the TPM TIS hardware interface for > whether there's a TPM 1.2 or a TPM 2. > We then have a patch for prefixing all TPM 1.2 functions with tpm12_ and > then introduce functions like these ones here: > > static ... tpm12_foo() { ... } > static ... tpm2_foo() { ... } > > tpm_foo() > { > [...] > > switch (tpmversion) { > case TPM_VERSION_1_2: > tpm12_foo() > break; > case TPM_VERSION_2: > tpm2_foo(); > break; > } > > [...] > } Is the difference between 1.2 and 2.0 so large that the above is needed? -Kevin

4 6

[PATCH 0/8] TPM return code simplifications
by Kevin O'Connor Jan. 5, 2016

Jan. 5, 2016

Hi Stefan, I cleaned up the additional patches I mentioned yesterday. Some time back, you posted a series of patches that removed the use of TCG 16bit BIOS structs from internal functions. Many of those functions were still using return codes from the spec though. I found using these return codes in internal functions to be annoying because they conflict with the TIS return codes. This series mostly just separates out the return codes so that only the TCG 16bit BIOS functions deal with the TCG 16bit BIOS return codes. This series is on top of patches 1-9 of the previous series (it replaces patch 10 of the previous series). Both series are also available at: https://github.com/KevinOConnor/seabios/tree/testing I'm not currently planning any other TPM changes after these. -Kevin Kevin O'Connor (8): tpm: Don't return a status from external bios measurement functions tpm: No need to check the return status of measurements tpm: Don't call tpm_set_failure() from tpm_log_extend_event() tpm: Don't use 16bit BIOS return codes in build_and_send_cmd() tpm: Don't use 16bit BIOS return codes in tpm_log_event() tpm: Don't use 16bit BIOS return codes in tpmhw_* functions tpm: Don't use 16bit BIOS return codes in TPM menu functions tpm: Replace build_and_send_cmd with tpm_send_cmd and tpm_send_check_cmd src/hw/tpm_drivers.c | 20 +- src/hw/tpm_drivers.h | 2 +- src/tcgbios.c | 718 +++++++++++++++++++-------------------------------- src/tcgbios.h | 8 +- 4 files changed, 287 insertions(+), 461 deletions(-) -- 2.5.0

3 12

[PATCH 00/10] Some TPM simplifications
by Kevin O'Connor Jan. 5, 2016

Jan. 5, 2016

The following series involves some code reorganization in the TPM code that I found useful in understanding the code. Patches 3-5 simplify the hardware interface by only exporting three commands to the underlying TIS hardware (tpmhw_probe, tpmhw_transmit, tpmhw_set_timeouts). Patches 8-10 simplify the parameters to the build_and_send_cmd() function. The remaining patches are mostly just code reorg. I have only compile tested these changes. -Kevin Kevin O'Connor (10): tpm: Add banner separating the TCG bios interface code from TCG menu code tpm: Avoid macro expansion of tpm request / response structs tpm: Simplify hardware probe and detection checks tpm: Add wrapper function tpm_set_timeouts() tpm: Move TPM hardware functions from tcgbios.c to hw/tpm_drivers.c tpm: Rework TPM interface shutdown support tpm: Simplify tcpa probe tpm: Introduce tpm_get_capability() helper function tpm: Eliminate response buffer parameter from build_and_send_cmd() tpm: Return returnCode from build_and_send_cmd() instead of via pointer param src/hw/tpm_drivers.c | 84 ++++++ src/hw/tpm_drivers.h | 28 +- src/std/tcg.h | 55 ++-- src/tcgbios.c | 704 +++++++++++++++------------------------------------ 4 files changed, 317 insertions(+), 554 deletions(-) -- 2.5.0

3 19

Re: [SeaBIOS] SeaBIOS crashes on CBFS without legacy header
by Kevin O'Connor Dec. 29, 2015

Dec. 29, 2015

On Thu, Dec 24, 2015 at 03:45:03PM -0800, Alex Gagniuc wrote: > When SeaBIOS is run on an FMAP-based CBFS (AKA, without a CBFS master > header, it crashes). The problem is in coreboot_cbfs_init. A > disassembly with -Mintel shows the following: > > 00000000 <coreboot_cbfs_init>: > ... > 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc > d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f > > > 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc > > Move the contents of memory address 0xfffffffc to esi. Since there's > no legacy CBFS pointer, the region is all 1's. esi now contains > 0xffffffff > > > d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f > > 32-bit dereference esi (0xffffffff) -- causes general protection > fault. The dereference is not because of unaligned access, but because > the access goes over the 32-bit address space in flat mode. The way > around this iss to sanitize the value at [0xffffffff] before > dereferencing it. A sane check is: > > if(address_of_hdr > (0xfffffff0 - sizeof(*hdr))) > fail(); > This is sane check because because the x86 reset vector is resides at > 0xfffffff0 (there are cases where this isn't true, but that's besides > the point here). Thanks. Can you put together a patch, or do you want me to? > There's also an unrelated problem with the C code that generates this assembly: > > > struct cbfs_header *hdr = *(void **)(CONFIG_CBFS_LOCATION - 4); > > With CONFIG_CBFS_LOCATION = 0, this places a negative value (-4) in an > unsigned type (pointer). The behavior of this is undefined according > to the C standard. Are you suggesting that an (unsigned) cast should be added? I doubt it would matter. -Kevin

2 2

[PATCH] build: fix typo in buildversion.py
by Roger Pau Monne Dec. 28, 2015

Dec. 28, 2015

Fixes the following build error: Building ld scripts Traceback (most recent call last): File "./scripts/buildversion.py", line 134, in <module> main() File "./scripts/buildversion.py", line 114, in main cleanbuild, toolstr = tool_versions(options.tools) File "./scripts/buildversion.py", line 90, in tool_versions vers[isbinutils] = "mixed" NameError: global name 'vers' is not defined Makefile:160: recipe for target 'out/romlayout16.lds' failed Signed-off-by: Roger Pau Monné <roger.pau(a)citrix.com> --- Cc: Kevin O'Connor <kevin(a)koconnor.net> --- scripts/buildversion.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/buildversion.py b/scripts/buildversion.py index ad47b90..4692898 100755 --- a/scripts/buildversion.py +++ b/scripts/buildversion.py @@ -87,7 +87,7 @@ def tool_versions(tools): if versions[isbinutils] and versions[isbinutils] != ver: logging.debug("Mixed version %s vs %s" % ( repr(versions[isbinutils]), repr(ver))) - vers[isbinutils] = "mixed" + versions[isbinutils] = "mixed" continue versions[isbinutils] = ver success += 1 -- 1.9.5 (Apple Git-50.3)

3 2

[PATCH 1/2] scsi: Do not call printf() from scsi_is_ready()
by Kevin O'Connor Dec. 28, 2015

Dec. 28, 2015

The scsi_is_ready() function may be called from a thread, and it is not valid to call printf() from a thread. Convert printf() to dprintf() to avoid this possibility. This does mean that cdrom detection (from cdrom_boot() ) may not give notification of slow cdrom drives to a user. However, the extra medium detection time is unlikely to be large anyway. Reported-by: Tobias Diedrich <tobiasdiedrich(a)gmail.com> Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/hw/blockcmd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hw/blockcmd.c b/src/hw/blockcmd.c index e20e3fc..0725b46 100644 --- a/src/hw/blockcmd.c +++ b/src/hw/blockcmd.c @@ -168,7 +168,7 @@ scsi_is_ready(struct disk_op_s *op) if (sense.asc == 0x04 && sense.ascq == 0x01 && !in_progress) { /* IN PROGRESS OF BECOMING READY */ - printf("Waiting for device to detect medium... "); + dprintf(1, "Waiting for device to detect medium... "); /* Allow 30 seconds more */ end = timer_calc(30000); in_progress = 1; -- 2.5.0

1 2

[PATCH] nmi: Don't try to switch onto extra stack in NMI handler
by Kevin O'Connor Dec. 28, 2015

Dec. 28, 2015

The NMI could occur when already on the extra stack, which would corrupt it. Always use the current stack on an NMI to avoid this. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/romlayout.S | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/romlayout.S b/src/romlayout.S index fedadfe..53cc0f5 100644 --- a/src/romlayout.S +++ b/src/romlayout.S @@ -548,7 +548,10 @@ entry_post: ENTRY_INTO32 _cfunc32flat_handle_post // Normal entry point ORG 0xe2c3 - IRQ_ENTRY 02 + .global entry_02 +entry_02: + ENTRY handle_02 // NMI handler does not switch onto extra stack + iretw ORG 0xe3fe .global entry_13_official -- 2.5.0

1 1

SeaBIOS crashes on CBFS without legacy header
by Alex Gagniuc Dec. 25, 2015

Dec. 25, 2015

When SeaBIOS is run on an FMAP-based CBFS (AKA, without a CBFS master header, it crashes). The problem is in coreboot_cbfs_init. A disassembly with -Mintel shows the following: 00000000 <coreboot_cbfs_init>: ... 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f > 7: 8b 35 fc ff ff ff mov esi,DWORD PTR ds:0xfffffffc Move the contents of memory address 0xfffffffc to esi. Since there's no legacy CBFS pointer, the region is all 1's. esi now contains 0xffffffff > d: 81 3e 4f 52 42 43 cmp DWORD PTR [esi],0x4342524f 32-bit dereference esi (0xffffffff) -- causes general protection fault. The dereference is not because of unaligned access, but because the access goes over the 32-bit address space in flat mode. The way around this iss to sanitize the value at [0xffffffff] before dereferencing it. A sane check is: if(address_of_hdr > (0xfffffff0 - sizeof(*hdr))) fail(); This is sane check because because the x86 reset vector is resides at 0xfffffff0 (there are cases where this isn't true, but that's besides the point here). There's also an unrelated problem with the C code that generates this assembly: > struct cbfs_header *hdr = *(void **)(CONFIG_CBFS_LOCATION - 4); With CONFIG_CBFS_LOCATION = 0, this places a negative value (-4) in an unsigned type (pointer). The behavior of this is undefined according to the C standard. Hope this helps, Alex

1 0

[PATCH] xhci: Wait for port enable even for USB3 devices
by Kevin O'Connor Dec. 23, 2015

Dec. 23, 2015

Some USB3 controllers (and/or devices) need additional time after the device is detected to place the port in an enabled state. Wait for the controller to report enabled before proceeding. This fixes several reports of devices that showed a "stall" error (cc 4) during set address. Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net> --- src/hw/usb-xhci.c | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/hw/usb-xhci.c b/src/hw/usb-xhci.c index 945b462..654feba 100644 --- a/src/hw/usb-xhci.c +++ b/src/hw/usb-xhci.c @@ -350,42 +350,41 @@ xhci_hub_reset(struct usbhub_s *hub, u32 port) { struct usb_xhci_s *xhci = container_of(hub->cntl, struct usb_xhci_s, usb); u32 portsc = readl(&xhci->pr[port].portsc); - int rc; if (!(portsc & XHCI_PORTSC_CCS)) // Device no longer connected?! return -1; switch (xhci_get_field(portsc, XHCI_PORTSC_PLS)) { case PLS_U0: - // A USB3 port - no reset necessary. - rc = speed_from_xhci[xhci_get_field(portsc, XHCI_PORTSC_SPEED)]; + // A USB3 port - controller automatically performs reset break; case PLS_POLLING: - // A USB2 port - perform device reset and wait for completion + // A USB2 port - perform device reset xhci_print_port_state(3, __func__, port, portsc); writel(&xhci->pr[port].portsc, portsc | XHCI_PORTSC_PR); - u32 end = timer_calc(100); - for (;;) { - portsc = readl(&xhci->pr[port].portsc); - if (!(portsc & XHCI_PORTSC_CCS)) - // Device disconnected during reset - return -1; - if (portsc & XHCI_PORTSC_PED) - // Reset complete - break; - if (timer_check(end)) { - warn_timeout(); - return -1; - } - yield(); - } - rc = speed_from_xhci[xhci_get_field(portsc, XHCI_PORTSC_SPEED)]; break; default: - rc = -1; - break; + return -1; + } + + // Wait for device to complete reset and be enabled + u32 end = timer_calc(100); + for (;;) { + portsc = readl(&xhci->pr[port].portsc); + if (!(portsc & XHCI_PORTSC_CCS)) + // Device disconnected during reset + return -1; + if (portsc & XHCI_PORTSC_PED) + // Reset complete + break; + if (timer_check(end)) { + warn_timeout(); + return -1; + } + yield(); } + int rc = speed_from_xhci[xhci_get_field(portsc, XHCI_PORTSC_SPEED)]; xhci_print_port_state(1, "XHCI", port, portsc); return rc; } -- 2.5.0

1 1

Re: [SeaBIOS] [Qemu-devel] [PATCH] SeaBios: Fix reset procedure reentrancy problem on qemu-kvm platform
by Kevin O'Connor Dec. 23, 2015

Dec. 23, 2015

On Wed, Dec 23, 2015 at 06:40:12AM +0000, Gonglei (Arei) wrote: > > From: Kevin O'Connor [mailto:kevin@koconnor.net] > > On Tue, Dec 22, 2015 at 02:14:12AM +0000, Gonglei (Arei) wrote: > > > Sorry, it doesn't work. What's worse is we cannot stop SeaBIOS stuck by > > > Setting "CONFIG_ENTRY_EXTRASTACK=n" after applying this patch. > > > > Oops, can you try with the patch below instead? > > > > It works now. Thanks! > > But do we need to check other possible situations > that maybe cause *extra stack* broken or overridden? I believe the issue is that an NMI could occur while SeaBIOS is already using its extra stack. The code is not prepared to switch into the extra stack while already on the extra stack. SeaBIOS is careful to always disable IRQs while running C code to prevent this issue, but disabling normal IRQs does not disable NMIs. So, I believe this issue is specific to the nature of NMIs. -Kevin

2 2