diff mbox series

firmware: coreboot: framebuffer: Avoid invalid zero physical address

Message ID 20231108182625.46563-1-alpernebiyasak@gmail.com
State New
Headers
Series firmware: coreboot: framebuffer: Avoid invalid zero physical address |

Commit Message

Alper Nebi Yasak Nov. 8, 2023, 6:25 p.m. UTC 
  On ARM64 systems coreboot defers framebuffer allocation to its payload,
to be done by a libpayload function call. In this case, coreboot tables
still include a framebuffer entry with display format details, but the
physical address field is set to zero (as in [1], for example).

Unfortunately, this field is not automatically updated when the
framebuffer is initialized through libpayload, citing that doing so
would invalidate checksums over the entire coreboot table [2].

This can be observed on ARM64 Chromebooks with stock firmware. On a
Google Kevin (RK3399), trying to use coreboot framebuffer driver as
built-in to the kernel results in a benign error. But on Google Hana
(MT8173) and Google Cozmo (MT8183) it causes a hang.

When the framebuffer physical address field in the coreboot table is
zero, we have no idea where coreboot initialized a framebuffer, or even
if it did. Instead of trying to set up a framebuffer located at zero,
return ENODEV to indicate that there isn't one.

[1] https://review.coreboot.org/c/coreboot/+/17109
[2] https://review.coreboot.org/c/coreboot/+/8797

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
(I was previously told on #coreboot IRC that I could add coreboot
mailing list to CC for kernel patches related to coreboot.)

 drivers/firmware/google/framebuffer-coreboot.c | 3 +++
 1 file changed, 3 insertions(+)


base-commit: 2220f68f4504aa1ccce0fac721ccdb301e9da32f

Comments

Julius Werner Nov. 8, 2023, 11:53 p.m. UTC | #1 
Yeah, if the kernel wanted to make use of coreboot display init on
those boards, it would have to reserve its own framebuffer space and
need to have at least enough of a driver for the display controller to
be able to tell it which address it picked. Until someone implements
that, erroring out for those cases makes sense.

Reviewed-by: Julius Werner <jwerner@chromium.org>
Tzung-Bi Shih Dec. 28, 2023, 3:17 a.m. UTC | #2 
On Wed, Nov 08, 2023 at 09:25:13PM +0300, Alper Nebi Yasak wrote:
> On ARM64 systems coreboot defers framebuffer allocation to its payload,
> to be done by a libpayload function call. In this case, coreboot tables
> still include a framebuffer entry with display format details, but the
> physical address field is set to zero (as in [1], for example).
> 
> Unfortunately, this field is not automatically updated when the
> framebuffer is initialized through libpayload, citing that doing so
> would invalidate checksums over the entire coreboot table [2].
> 
> [...]

Applied, thanks!

[1/1] firmware: coreboot: framebuffer: Avoid invalid zero physical address
        commit: ecea08916418a94f99f89c543303877cb6e08a11

Just realized the bot didn't send the mail for the patch.
diff mbox series

Patch

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index c323a818805c..5c84bbebfef8 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -36,6 +36,9 @@  static int framebuffer_probe(struct coreboot_device *dev)
 		.format = NULL,
 	};
 
+	if (!fb->physical_address)
+		return -ENODEV;
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&