Paths

Table of Contentst

Fix AccessWidth and BitWidth parsing in SPCR table
ClosedPublic
Actions

Authored by mw on Jun 20 2020, 3:43 PM.

Details

Reviewers

manu
cperciva
val_packett.cool
imp
markm

Commits

rS362574: Fix AccessWidth and BitWidth parsing in SPCR table

Summary

The ACPI Specification defines a Generic Address Structure (GAS),
which is used to describe UART controller register layout in the
SPCR table. The driver responsible for parsing it (uart_cpu_acpi)
wrongly associates the Access Size field to the uart_bas's regshft
and the register BitWidth to the regiowidth - according to
the definitions it should be opposite.

This problem remained hidden most likely because the majority of platforms
use 32-bit registers (BitWidth) which are accessed with the according
size (Dword). However on Marvell Armada 8k / Cn913x platforms,
the 32-bit registers should be accessed with Byte granulity, which
unveiled the issue.

This patch fixes above by proper values assignment and slightly improved
parsing.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mw created this revision.Jun 20 2020, 3:43 PM

Herald added a subscriber: imp. · View Herald TranscriptJun 20 2020, 3:43 PM

mw requested review of this revision.Jun 20 2020, 3:43 PM

Thanks !
I've talked with Greg about this last week, glad you did it before me :)

This revision is now accepted and ready to land.Jun 20 2020, 3:54 PM

Thanks for finding this. We went round and round when it went in. Might not hurt to make sure it still works on other boards
.

sys/dev/uart/uart_cpu_acpi.c
136	This switch statement could just be a shift, but that's not a demand, just a suggestion.

cperciva added inline comments.Jun 20 2020, 4:27 PM

sys/dev/uart/uart_cpu_acpi.c
137	This looks to me like it will break EC2 x86 "bare metal" instances. I added the 0 case because their SPCR table was buggy.

mw added inline comments.Jun 20 2020, 5:10 PM

sys/dev/uart/uart_cpu_acpi.c
136	I'm aware of that (Linux does it this way), but imo the switch-case protects us better from wrong ACPI tables. If you don't mind, I'd leave as-is.
137	I can add 0 case, same as for the BitWidth below. However, since now the AccesWidth has different meaning- what regioshift value should we assign?

@cperciva @greg_unrelenting.technology Hi! Before I do the commit, I'm looking forward to your testing and I'd really appreciate your input.

cperciva added inline comments.Jun 22 2020, 8:07 PM

sys/dev/uart/uart_cpu_acpi.c
137	EC2 x86 "bare metal" has AccessWidth = 0 and BitWidth = 8. The console works with regshft = 0 and regiowidth = 1. So I think it just needs a `case 0: di->bas.regiowidth = 1;` here.

mw updated this revision to Diff 73491.Jun 23 2020, 2:43 AM

mw edited the summary of this revision. (Show Details)

This revision now requires review to proceed.Jun 23 2020, 2:43 AM

mw added inline comments.Jun 23 2020, 2:44 AM

sys/dev/uart/uart_cpu_acpi.c
137	I added case for /* EFI_ACPI_6_0_UNDEFINED */ - now the values should cover the case you mentioned. Please confirm.

cperciva added inline comments.Jun 23 2020, 2:48 AM

sys/dev/uart/uart_cpu_acpi.c
125	Looks like you're missing a level of indentation here? Also I think we usually spell this `/* FALLTHROUGH */` in the FreeBSD src tree.

mw updated this revision to Diff 73497.Jun 23 2020, 7:18 AM

mw added inline comments.

sys/dev/uart/uart_cpu_acpi.c
125	Done

manu accepted this revision.Jun 23 2020, 7:24 AM

This revision is now accepted and ready to land.Jun 23 2020, 7:24 AM

Works on my mcbin with the official SPCR table! :) Might test on other things later.

Interestingly, the unpatched kernel completely failed on the hacked SPCR I used to use.. as in not just no UART output, but the graphical console also wouldn't jump to the kernel's output. I guess the incorrect UART access causes a processor exception super early.

Looks good to me. Might be worth mentioning in the commit message that EFI_ACPI_6_0_UNDEFINED handling is needed to work around a buggy SPCR table on EC2 x86 "bare metal" instances, just in case someone looks at the code a few years from now and wonders why we're doing this.

@cperciva I'm adding such note, thanks.

Closed by commit rS362574: Fix AccessWidth and BitWidth parsing in SPCR table (authored by mw). · Explain WhyJun 24 2020, 12:15 PM

This revision was automatically updated to reflect the committed changes.

mw added a commit: rS362574: Fix AccessWidth and BitWidth parsing in SPCR table.

This change breaks my MacchiattoBin DoubleShot. I've bisected to get to commit r362574, which is this. The symptoms are a hang right at the beginning of the boot, at this point:

:
:
AdmaMap: 0
AdmaPages: 0
Printing EFI_SD_MMC_PASS_THRU_COMMAND_PACKET
Command index: 5, argument: 0
Command type: 1, response type: 4
Response 0: 0, 1: 0, 2: 0, 3: 0
|  ______               ____   _____ _____  
  |  ____|             |  _ \ / ____|  __ \ 
  | |___ _ __ ___  ___ | |_) | (___ | |  | |
  |  ___| '__/ _ \/ _ \|  _ < \___ \| |  | |
  | |   | | |  __/  __/| |_) |____) | |__| |
  | |   | | |    |    ||     |      |      |
  |_|   |_|  \___|\___||____/|_____/|_____/ 
                                                 ```                        `
 +-----------Welcome to FreeBSD------------+    s` `.....---.......--.```   -/
 |                                         |    +o   .--`         /y:`      +.
 |  1. Boot Multi user [Enter]             |     yo`:.            :o      `+-
 |  2. Boot Single user                    |      y/               -/`   -o/
 |  3. Escape to loader prompt             |     .-                  ::/sy+:.
 |  4. Reboot                              |     /                     `--  /
 |                                         |    `:                          :`
 |  Options:                               |    `:                          :`
 |  5. Kernel: default/kernel (1 of 3)     |     /                          /
 |  6. Boot Options                        |     .-                        -.
 |                                         |      --                      -.
 |                                         |       `:`                  `:`
 |                                         |         .--             `--.
 +-----------------------------------------+            .---.....----.
   Autoboot in 9 seconds, hit [Enter] to boot or any other key to stop      

Loading kernel...
/boot/kernel/kernel text=0x83577c data=0x15afd8 data=0x0+0x2ea68e syms=[0x8+0xd6a28+0x8+0xfa244]
Loading configured modules...
/boot/entropy size=0x1000
/etc/hostid size=0x25
/boot/kernel/opensolaris.ko text=0x125f text=0xe40 data=0x450+0x6830 syms=[0x8+0xfd8+0x8+0x89c]
/boot/kernel/zfs.ko text=0x86610 text=0x13f460 data=0x180d0+0x90550 syms=[0x8+0x259e0+0x8+0x1f787]
No valid device tree blob found!

WARNING! Trying to fire up the kernel, but no device tree blob found!
<Hang>

Normally the boot would carry on here. I've checked to see if just the console device was broken and that the machine booted, but no dice; the machine is dead. At the previous commit r362573, boot is normal.

It works fine on mine, are you using an old edk2 build maybe ?
Try https://people.freebsd.org/~manu/flash-image-mcbin.bin
This is mainline edk2 (latest stable version) and Marvell TF-A, ports will land soon.

Yup - my flash is a couple of months old, and a hand-rolled debug-version (not by me). The image you pointed to fails with checksum errors at boot, but it may have something to do with the way I made it. With the same MBR as the old one, I did:

$ sudo dd bs=512 seek=1 if=flash-image-mcbin.bin of=/dev/rdisk3

... on my mac.

Log:

BootROM - 2.03
Starting CP-0 IOROM 1.07
Booting from SD 0 (0x29)
SD - wait_for_sd_interrupt: Error interrupt -  00008000
SD - wait_for_sd_interrupt: Error interrupt status 00000003
SD - sd_get_cmd_response: Get command response failed.

SD - sd_init: Failed - ret = 00000081
Error: Failed initializing interface
Error: Failed boot attempt 01. error = 0x0C1

BootROM - 2.03
Starting CP-0 IOROM 1.07
Booting from SD 0 (0x29)
SD - wait_for_sd_interrupt: Error interrupt -  00008000
SD - wait_for_sd_interrupt: Error interrupt status 00000003
SD - sd_get_cmd_response: Get command response failed.

SD - sd_init: Failed - ret = 00000081
Error: Failed initializing interface
Found valid image at boot postion 0x000
lNOTICE:  Starting binary extension
NOTICE:  SVC: DEV ID: 8040, FREQ Mode: 0x1
NOTICE:  SVC: AVS work point changed from 0x28 to 0x28
mv_ddr: mv_ddr-armada-18.09.2-g99d7725-dirty (Jul 03 2020 - 17:57:41)
mv_ddr: completed successfully
NOTICE:  Cold boot
Error: image checksum verification failed
Error: no valid header till end of media
Error: Failed boot attempt 02. error = 0x041

BootROM - 2.03
Starting CP-0 IOROM 1.07
Booting from SD 0 (0x29)
SD - wait_for_sd_interrupt: Error interrupt -  00008000
SD - wait_for_sd_interrupt: Error interrupt status 00000003
SD - sd_get_cmd_response: Get command response failed.

SD - sd_init: Failed - ret = 00000081
Error: Failed initializing interface
Found valid image at boot postion 0x000
lNOTICE:  Starting binary extension
NOTICE:  SVC: DEV ID: 8040, FREQ Mode: 0x1
NOTICE:  SVC: AVS work point changed from 0x28 to 0x28
mv_ddr: mv_ddr-armada-18.09.2-g99d7725-dirty (Jul 03 2020 - 17:57:41)
mv_ddr: completed successfully
NOTICE:  Cold boot
Error: image checksum verification failed
Error: no valid header till end of media
Error: Failed boot attempt 03. error = 0x041

BootROM - 2.03
Starting CP-0 IOROM 1.07
Booting from SD 0 (0x29)
SD - wait_for_sd_interrupt: Error interrupt -  00008000
SD - wait_for_sd_interrupt: Error interrupt status 00000003
SD - sd_get_cmd_response: Get command response failed.

SD - sd_init: Failed - ret = 00000081
Error: Failed initializing interface
Found valid image at boot postion 0x000
lNOTICE:  Starting binary extension
NOTICE:  SVC: DEV ID: 8040, FREQ Mode: 0x1
NOTICE:  SVC: AVS work point changed from 0x28 to 0x28
mv_ddr: mv_ddr-armada-18.09.2-g99d7725-dirty (Jul 03 2020 - 17:57:41)
mv_ddr: completed successfully
NOTICE:  Cold boot
Error: image checksum verification failed
Error: no valid header till end of media
Error: Failed boot attempt 04. error = 0x041

Trying Uart..

Add conv=sync for dd
If it's the old version that GregV posted a while ago it's normal that this commit "breaks" for you, this image have a patched SPCR table to cope with the old bug that this code fixes.

Trying now. In the meanwhile, is there some place where the Collected Wisdom™ of all current mcbin firmware statusses, hints, binaries etc are kept?

Worse. Previously working console now broken. Current with this change backed out now broken.

Is the console supposed to work without this fix applied?

I'm about to try latest CURRENT with nothing backed out, and latest flash image.

In D25373#565370, @markm wrote:

Worse. Previously working console now broken. Current with this change backed out now broken.

Is the console supposed to work without this fix applied?

Of course not, this fix is needed if you want console.

I'm about to try latest CURRENT with nothing backed out, and latest flash image.

With this patch in, AND the flashe given above, I get my console back!

But I lose my PCI ethernet ports.

In D25373#565376, @markm wrote:

With this patch in, AND the flashe given above, I get my console back!

But I lose my PCI ethernet ports.

I have not included the ECAM hacks yet

ECAM hacks? When are those likely to appear? :-)

In D25373#565380, @markm wrote:

ECAM hacks? When are those likely to appear? :-)

https://unrelentingtech.s3.dualstack.eu-west-1.amazonaws.com/flash-image-2020-07-01.bin try this image, it has the ECAM offset removed

@mw promised to make a toggle for ECAM offset in the setup menu.. :)

That works, thanks! I'm at latest CURRENT, no backouts, my console works and so do my PCI ethernet port.

Thanks for the help!

OK to close again.

This revision is now accepted and ready to land.Jul 5 2020, 3:56 PM

In D25373#565381, @greg_unrelenting.technology wrote:

In D25373#565380, @markm wrote:

ECAM hacks? When are those likely to appear? :-)

https://unrelentingtech.s3.dualstack.eu-west-1.amazonaws.com/flash-image-2020-07-01.bin try this image, it has the ECAM offset removed

@mw promised to make a toggle for ECAM offset in the setup menu.. :)

True - I promised to handle SPCR and ECAM. This patch closes the first part, need to find time for the latter :)

mw closed this revision.Dec 24 2020, 7:26 AM

Revision Contents
Changeset List

Path

Size

sys/

dev/

uart/

uart_cpu_acpi.c

41 lines

Diff 73394

View Options

sys/dev/uart/uart_cpu_acpi.c

Fix AccessWidth and BitWidth parsing in SPCR tableClosedPublicActions