Page MenuHomeFreeBSD

ofwfb: fix vga/hdmi console during boot on powerpc64
ClosedPublic

Authored by alfredo on Jun 3 2021, 7:00 AM.

Details

Summary

On recent OpenBMC firmware, the onboard ASMEDIA video card framebuffer address was removed from device tree for security purposes (value is set to zero to avoid leaking the address).
This patch works around the problem by taking framebuffer base address from "ranges" property of a parent node.

Tested on Raptor Blackbird and Talos II

MFC After: 2 weeks

Diff Detail

Repository
rG FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alfredo retitled this revision from WIP: powerpc64: ofwfb - fix vga/hdmi boot to ofwfb: fix vga/hdmi console during boot on powerpc64.
alfredo edited the summary of this revision. (Show Details)
alfredo added a project: PowerPC.

This approach of auto detecting the correct frame buffer physical address based on Vendor ID looks good.
It would help to make FreeBSD graphics work out of the box for Blackbird and Talos II machines, instead of forcing the user to figure out the physical address of its VGA adapter and pass it to the kernel manually, or else the system just hangs.
Ideally, Petitboot should fix it on their side, but I don't know if it will happen anytime soon, since Linux isn't affected, because it just talks directly to ASPEED video device, instead of relying on the framebuffer info exposed in the device tree.

sys/dev/vt/hw/ofwfb/ofwfb.c
305

Isn't it possible to use or adapt any of the following functions that already parse "ranges" instead?

  • ofw_reg_to_paddr - this may not be generic enough for this purpose
  • ofw_pcib_nranges
  • ofw_pcib_fill_ranges - this one looks promising and similar to this implementation
  • fdt_get_range
  • simple_bus_fill_ranges - this one depends on simplebus_softc, so it may be harder to adapt
374

style: need a newline here

384

style: returned values must be inside parentheses. This comment is valid for all returns added by this change.

598

I think "expose" would describe it better than "leak", maybe also adding that this is done "for security reasons".

605–608

nitpick: for consistency, it's better to either remove the braces or add it to this "else if" too.

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

addresses commiter's suggestions

alfredo marked 5 inline comments as done.

address reviewer comments

sys/dev/vt/hw/ofwfb/ofwfb.c
305

ofw_pcib_fill_ranges is the one that best fits the need but it allocates memory dynamically. This is not possible here since system hangs due to facility not being available.
I added a comment to the code explaining the reasons

luporl added inline comments.
sys/dev/vt/hw/ofwfb/ofwfb.c
305

There are some issues with this line, maybe replace it with "memory allocation is not possible at the point this function is used"?

306

s/there also/there are also/

This revision is now accepted and ready to land.Nov 1 2021, 12:29 PM
This revision now requires review to proceed.Nov 1 2021, 2:33 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 3 2021, 1:50 PM
This revision was automatically updated to reflect the committed changes.