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

feat: add kvmfr module (looking-glass) and vfio support #1226

Merged
merged 6 commits into from
Apr 28, 2024
Merged

Conversation

HikariKnight
Copy link
Member

@HikariKnight HikariKnight commented Apr 28, 2024

This adds support for the kvmfr module used in LookingGlass and vfio support for VMs.
I have been testing this in bazzite since we got it added with Bazzite3.0 (Fedora40) and i have tried my best to make the module crash (and failed) in order to test it's stability.

This is used for GPU passthrough to windows VMs for those with supported systems that want a more seamless, no compromises solution for compatibility with some software (or for some games).
The LookingGlass team is looking to deprecate (and potentially remove) the support for the old shm method which atomic desktops relied upon before to have this work without a kernel module, hence the addition of this module.
image

This only enables the modules and does not fully configure everything for the user (no way we will do that), the user has to bind the card to the vfio-pci driver, configure the VM and compile looking-glass-client themselves (we in bazzite provides a guide for the latter that is linked to in the just recipe)

The LookingGlass team were ok with us including the kvmfr module as long as we at least took responsibility for our module package (as they had no interest in supporting "distro specific packaging") and filter out the issues that could be caused by our kernel module package (essentially, if user gets an image from the VM, the issue is upstream, otherwise it might be the module package, in which case it is my job to fix it. Which i am fine with as i made the akmods package the same way that dkms would. So they are 1:1)

# Needs clarification before merge
I am unsure if you want this in base or just dx so the PR right now have this added to base (since i figured some people might end up layering just virt-manager and qemu instead of using dx?), but i have no problems with moving it into dx only and i have the code ready for it.

@HikariKnight HikariKnight added enhancement New feature or request question labels Apr 28, 2024
@HikariKnight HikariKnight self-assigned this Apr 28, 2024
@HikariKnight HikariKnight requested a review from castrojo as a code owner April 28, 2024 13:49
@HikariKnight HikariKnight marked this pull request as draft April 28, 2024 14:11
@m2Giles
Copy link
Member

m2Giles commented Apr 28, 2024

Only for -dx would be preferable since we have libvirt and qemu enabled there. I would not want to encourage a user to layer all of the qemu when we have the images. That said, containerized kvm could take advantage of this module.

Additionally, with the reorganization of akmods, we should move kvmfr from extras to common.

@HikariKnight
Copy link
Member Author

will switch to dx @m2Giles
also i might be doing something wrong with the inclusion since i am getting an error about missing one of the dependencies for kvmfr akmod when building that we do not get in bazzite 🤔 so i am investigating that too

@m2Giles
Copy link
Member

m2Giles commented Apr 28, 2024

I would also prefer this to be a script and not inlined in just.

For the script check if we're on -dx otherwise recommend the user to rebase to -dx for virtualization features.

Additionally we can add the vfio drivers to the initramfs. I'm fairly certain they won't cause any issues being there. Then this should all be configurable with only kargs

@HikariKnight
Copy link
Member Author

HikariKnight commented Apr 28, 2024

I would also prefer this to be a script and not inlined in just.

For the script check if we're on -dx otherwise recommend the user to rebase to -dx for virtualization features.

already adding a devmode check to the just, i can split the selinux and kvmfr configuration part into a script since that is where most of the "mess" is

Additionally we can add the vfio drivers to the initramfs. I'm fairly certain they won't cause any issues being there. Then this should all be configurable with only kargs

The only reason we have kvmfr not configured in bazzite is due to handhelds. Plus upstream everywhere has vfio disabled by default.

@m2Giles
Copy link
Member

m2Giles commented Apr 28, 2024

The only reason we have kvmfr not configured in bazzite is due to handhelds. Plus upstream everywhere has vfio disabled by default.

Is there an issue with this? Arch and Proxmox have a few the modules in kernel and not as modules. Just wondering what the negatives would be.

@HikariKnight
Copy link
Member Author

The only reason we have kvmfr not configured in bazzite is due to handhelds. Plus upstream everywhere has vfio disabled by default.

Is there an issue with this? Arch and Proxmox have a few the modules in kernel and not as modules. Just wondering what the negatives would be.

I am guessing there is a chance that some oddball hardware might not like it. 🤷
for me it does not matter, we can decide once i push the commit that splits the kvmfr part into a script

@HikariKnight
Copy link
Member Author

HikariKnight commented Apr 28, 2024

first time i have actually added an akmods so i just copied what kyle did in bazzite, got it working now

@HikariKnight HikariKnight added the dx Developer Experience Image specific label Apr 28, 2024
@HikariKnight HikariKnight marked this pull request as ready for review April 28, 2024 16:03
@m2Giles m2Giles enabled auto-merge April 28, 2024 16:43
@m2Giles m2Giles added this pull request to the merge queue Apr 28, 2024
Merged via the queue into main with commit 49e7d04 Apr 28, 2024
62 checks passed
@m2Giles m2Giles deleted the add-kvmfr branch April 28, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx Developer Experience Image specific enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants