-
Notifications
You must be signed in to change notification settings - Fork 168
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
base: master
Are you sure you want to change the base?
Conversation
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>
Fix typo: controler -> controller Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
There was a problem hiding this 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?
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:
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 { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
// Switch virtual terminal console | ||
func chvt(vt int) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Switch virtual terminal console | |
func chvt(vt int) error { | |
func switchVirtualTerminalConsole(vt int) error { |
// Open default console file device | ||
fd, err := unix.Open("/dev/tty0", unix.O_RDWR, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run tests
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:
arm64