loader should consult with ACPI SPCR table for serial console

[tsoome]

Summary

Check for SPCR table, if exists, set console=comconsole.

[cem]

Thanks! I found some trivial issues but it mostly looks great, thanks for doing the work!

stand/i386/libi386/biosacpi.c

59–60

Usually in FreeBSD we declare variables at top of function. (style)

65

Perhaps sizeof(xsdt->TableOffsetEntry[0]), as that is where the uint64 comes from?

Similarly below for rdst fallback.

69–70

ACPI_SIG_SPCR is defined as the literal string "SPCR", which has sizeof() 5, not the probably expected 4.

Semantically, it might make be more logical to type the pointer as ACPI_TABLE_SPCR only after doing the signature check. That'd avoid the need to NULL it out on mismatches but is otherwise just a subjective and non-functional change.

E.g.,

void *tmp;
for ( ... ) {
        tmp = (void *)PTOV(xsdt->TableOffsetEntry[i];
        if (memcmp(tmp, ACPI_SIG_SPCR, strlen(ACPI_SIG_SPCR)) != 0)
                continue;
        return (tmp); // implicit cast to ACPI_TABLE_SPCR* here
}

102

Address is typically small (0x3f8), but the value is technically uint64 and we need 21 chars to store the maximal possible decimal value (18446744073709551615), plus trailing \0.

130–131

I think we want "%ju" and (uintmax_t)spcr->SerialPort.Address, as we define ACPI_USE_SYSTEM_INTTYPES in at least some stand targets and use the FreeBSD uint64 type, which is not always long long. (Clang complains about type mismatch even when long and long long are both 64 bits.)

[tsoome]

feedback from cem.

[val_packett.cool]

hmm.. The kernel already reads from SPCR (dev/uart/uart_cpu_acpi.c), do we need to feed it duplicate info via comconsole_speed etc.? Or does the i386 legacy bios loader use this info for itself somehow? (I thought env is for the kernel mostly..)

Also, does the setenv("console", "comconsole", 1) mean it's not going to probe vidconsole? (The existence of SPCR should never mean there's no vidconsole ­— I have amd64 and arm64 devices that boot with both an SPCR-defined serial port and a graphical (EFI framebuffer) console at the same time.. )

Wait, that setenv would be overwritten by the ones just after the new biosacpi_detect() call, where the initial_howto & RB_MULTIPLE stuff is.. so is it meant for the case where none of these conditions are true?

[cem]

LGTM, thanks!

@greg_unrelenting.technology I believe the intent is to improve console performance in loader itself, before kernel starts.

Edit: Yes, looks like RB_foo are allowed to override the "console" env var itself, which seems reasonable. console_port and console_baud are not overwritten, and after the RB_ section we invoke cons_probe(). This is mostly about getting the right baud rate (and possibly secondarily, the right IO port) so that the serial isn't dog slow in loader.

The comconsole probe implementation respects the values in "console", "console_port", and "console_speed," if present and enabled. So this should improve serial console throughput on pretty much all devices that support better than 9600 or BIOS default (which is all these days, unless the BIOS already graciously configured 115200 for us).

[tsoome]

In D24041#528666, @greg_unrelenting.technology wrote:

hmm.. The kernel already reads from SPCR (dev/uart/uart_cpu_acpi.c), do we need to feed it duplicate info via comconsole_speed etc.? Or does the i386 legacy bios loader use this info for itself somehow? (I thought env is for the kernel mostly..)

Also, does the setenv("console", "comconsole", 1) mean it's not going to probe vidconsole? (The existence of SPCR should never mean there's no vidconsole ­— I have amd64 and arm64 devices that boot with both an SPCR-defined serial port and a graphical (EFI framebuffer) console at the same time.. )

Wait, that setenv would be overwritten by the ones just after the new biosacpi_detect() call, where the initial_howto & RB_MULTIPLE stuff is.. so is it meant for the case where none of these conditions are true?

Our task here is to improve what is happening before kernel; The comconsole is configured by device and speed (and pcidevice, not implemented by this patch because I do not have anything to test with). The check in kernel is good to have, that will help with third party bootloader(s). And no, SPCR does not hint about video because it is created to support console redirection to serial port.

[tsoome]

In D24041#528666, @greg_unrelenting.technology wrote:

hmm.. The kernel already reads from SPCR (dev/uart/uart_cpu_acpi.c), do we need to feed it duplicate info via comconsole_speed etc.? Or does the i386 legacy bios loader use this info for itself somehow? (I thought env is for the kernel mostly..)

Also, does the setenv("console", "comconsole", 1) mean it's not going to probe vidconsole? (The existence of SPCR should never mean there's no vidconsole ­— I have amd64 and arm64 devices that boot with both an SPCR-defined serial port and a graphical (EFI framebuffer) console at the same time.. )

Wait, that setenv would be overwritten by the ones just after the new biosacpi_detect() call, where the initial_howto & RB_MULTIPLE stuff is.. so is it meant for the case where none of these conditions are true?

Sorry, I did miss the last bit; yes, we allow boot1 to override what we get from SPCR, because there may be /boot/config or user input on boot: prompt.

[imp]

I need to review this carefully. Please do not commit until I've had a chance to do so.
We have a number of issues with the teken changes and they remain unresolved wrt to API changes, I don't want to see more so I want to look at it closely.

[tsoome]

feedback from cem.

[tsoome]

update zfsboot (gptzfsboot) to check SPCR.

Tested with bhyve on illumos.

[imp]

stand/i386/libi386/biosacpi.c

125

I think it's wrong to unconditionally set this. It's a POLA violation and change in behavior that people will not like.

128

Likewise. I know the port I want if I've set it. Overriding it is uncool.

137

Likewise with speed.

[imp]

My big concern with this is that there is no override provision. It is good we set it early, but it makes it harder to say you want a video console instead...

[tsoome]

In D24041#635435, @imp wrote:

My big concern with this is that there is no override provision. It is good we set it early, but it makes it harder to say you want a video console instead...

There are few things to consider; SPCR is only helping to get usable console when built in defaults are set to something different (like LOM console on ttya, not ttyb as we have built in). As we process it early, there still is /boot/config. We can set dual console, but at this time, at least bhyve setup will hung when boot1 is attempting to output on (non-existing) VGA. Of course, on another hand, it means, we should stop assuming there is vga and test if it actually is there:D

And then again, poking on ttya when the console actually is on ttyb is not exactly good when you only do have standard image from freebsd.org:) Anyhow, this is exactly the reason why we have review, so we can collect ideas and opinions. It can just be that whole idea is just not worth it:)