Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Virtual Consoles and Framebuffer drivers when enabling/disabling VGA #4648

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rene
Copy link
Contributor

@rene rene commented Feb 27, 2025

Description

When VGA is enabled/disabled, the PCIe VGA device is assigned/unassigned to/from the host through the vfio-pci driver mechanism. However, this might not be enough for integrated graphics adapters (iGPUs). This PR adds additional procedures that detach all framebuffer drivers and Virtual Terminals when VGA access is disabled and attach them back when VGA is enabled.

Non-PCIe graphics cards

On devices with integrated graphics card, such as those present on many arm64 SoCs, the vfio-pci mechanism cannot be applied. Thus, on these devices video remains active even though debug.enable.vga is disabled. The changes introduced by this PR will also address these devices, so the expected behavior will now be applied.

iGPU passthrough

The procedure introduced by this PR might be required to passthrough some iGPUs, so the passthrough of such devices can now work if the VGA is disabled. However, this is not in the scope of this PR, so only the debug.enable.vga enable/disable it's being handled for now.

Limitations

The re-attach of framebuffer and VT terminals (once they were removed) might not work during runtime on some devices. This is a limitation observed with the device itself and not related to EVE. On such devices, reboot is required to get the video back. A small disclaimer is introduced in the documentation about this possible situation.

Tests

x86_64:

  • QEMU
  • Real PC device

arm64

  • QEMU
  • Raspberry Pi
  • Jetson Xavier NX devkit

@rene rene requested review from rucoder and eriknordmark February 27, 2025 14:02
@rene rene marked this pull request as draft February 27, 2025 14:03
@rene rene force-pushed the handle-fb-vt-vga branch from 3b0b7bd to b2fc482 Compare March 3, 2025 14:04
rene added 5 commits March 4, 2025 10:14
Add function description to make revive plugin pass.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
The log message "updateVgaAccess(%t)" was wrongly showing the value of
ctx.usbAccess.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
This commit adds a new file with helper functions to handle Virtual
Terminals (VTs) and Framebuffer drivers. They will be required to
detach/attach VTs and Framebuffers when VGA access is disabled/enabled.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
When VGA is enabled/disabled, the PCIe VGA device is assigned/unassigned
to/from the host through the vfio-pci driver mechanism. However, this might
not be enough for integrated graphics adapters. This commit adds additional
procedures that detach all framebuffer drivers and Virtual Terminals when
VGA access is disabled and attach them back when VGA is enabled.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
Add a short notice that on some devices a reboot might be required in
order to get VGA console back after access is enabled.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
@rene rene force-pushed the handle-fb-vt-vga branch from b2fc482 to 5ef8278 Compare March 4, 2025 11:29
@rene rene changed the title [WIP]: Handle Virtual Consoles and Framebuffer drivers when enabling/disabling VGA Handle Virtual Consoles and Framebuffer drivers when enabling/disabling VGA Mar 4, 2025
Fix typo: controler -> controller

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
@rene rene marked this pull request as ready for review March 4, 2025 11:39
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.07%. Comparing base (77e0e79) to head (87a2335).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4648   +/-   ##
=======================================
  Coverage   21.07%   21.07%           
=======================================
  Files           8        8           
  Lines        1167     1167           
=======================================
  Hits          246      246           
  Misses        861      861           
  Partials       60       60           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking to @rucoder to review the details.

Even though this doesn't promise to make iGPU passthrough work on x86, what behavior should we see when debug.enable.vga is enabled and disabled on x86 with an iGPU? And have we tested that we see that behavior?

@rene
Copy link
Contributor Author

rene commented Mar 4, 2025

Looking to @rucoder to review the details.

Even though this doesn't promise to make iGPU passthrough work on x86, what behavior should we see when debug.enable.vga is enabled and disabled on x86 with an iGPU? And have we tested that we see that behavior?

Good question @eriknordmark , yes, I've tested on a real x86 device with an iGPU. Depending on the device, the behavior can be one of the following:

  1. Enable/Disable video just works without issues
  2. Disable video: it works and iGPU passthrough will also work (with Seabios, no UEFI). However, enabling video back might require reboot, this was observed on the real x86 device that I tested with an iGPU. There is no issues after bringing PCIe VGA card back to the host, but the video will only appear again after reboot. This behavior is also described on several forums by people doing passthrough with Proxmox and other hypervisors. So in a nutshell: Disabling VGA will disable video for sure and Enabling VGA after disabling it might require reboot to bring video back.

Why I'm not fully covering iGPU passthrough on this PR?

Because although iGPU passthrough might work with Seabios, there is one potential issue observed on this real device: trying to passthrough iGPU with UEFI OVMF BIOS can freeze the device, so in order to fully support iGPU passthrough, we also need to do some work on our OVMF BIOS... No other issues were observed if we don't passthrough the iGPU, so I think this PR is safe...

checkIoBundleAll(ctx)

// Nothing to do if VGA is already enabled
if vgaSwitch {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the correlation between vgaSwitch and gcp.GlobalValueBool(types.VgaAccess) != ctx.vgaAccess ?

IMO introducing this global variable makes it a lot harder to understand when what is happening.

vt, err := unix.IoctlGetInt(fd, VT_OPENQRY)
if err != nil {
if err := unix.Close(fd); err != nil {
log.Errorf("Cannot close file descriptor: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here you log if closing the filedescriptor fails, but you don't log if ioctl fails, although IMO that is the more important information

Comment on lines +193 to +194
// Switch virtual terminal console
func chvt(vt int) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Switch virtual terminal console
func chvt(vt int) error {
func switchVirtualTerminalConsole(vt int) error {

Comment on lines +195 to +196
// Open default console file device
fd, err := unix.Open("/dev/tty0", unix.O_RDWR, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Open default console file device
fd, err := unix.Open("/dev/tty0", unix.O_RDWR, 0)
consoleFileDevice, err := unix.Open("/dev/tty0", unix.O_RDWR, 0)

for _, vt := range deviceVTs {
if vt.bound {
if err := vt.bind(); err != nil {
deviceVTs = nil // Clear the global list before return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know for sure that these functions are always called in the same goroutine and we don't run into a race condition?

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants