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

TBD: Add partially Intel GVT-g support (xengt device) #125

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

jevank
Copy link
Contributor

@jevank jevank commented Feb 2, 2022

This requires to add minimal support Intel GVT-g mediated passtrough
to Qubes OS. This patches based on Intel XenGT preview, XCP-NG
Project.

This requires to add minimal support Intel GVT-g mediated passtrough
to Qubes OS. This patches based on Intel XenGT preview, XCP-NG
Project.
@jevank
Copy link
Contributor Author

jevank commented Feb 2, 2022

These are the most controversial upstream changes to support GVT-g (additional hypercall mostly). If adopted, an alternative 'kernel-gvt' [1] hosted in the qubes-contrib repository would be sufficient to use the GVT-g device in Linux HVMs. Otherwise, a fully alternative set of xen-gvt, libvirt-gvt (binary compatibility with xen-gvt is required) and kernel-gvt must be created.

Similar changes have been made in XCP-NG and Citrix Hypervisor as far as I understand.

[1] https://github.com/jevank/qubes-linux-kernel-gvt

@marmarek
Copy link
Member

It doesn't look too intrusive, there are two new hypercalls:

  • one is dom0-only
  • one just returns a number of saved pages (a global counter, it isn't clear to me why would GVT-g need it)

hvmloader is running inside a guest already, so it isn't security critical. And libxl changes looks sane (creating vgt device for domains that request that in the config explicitly).

What is interesting to me, where actually guest is granted access to that device - is it mediated through (patched?) qemu? In that case, I guess it requires qemu running in dom0 - otherwise it wouldn't have access to the GPU. Or maybe I'm missing something here?

@jevank
Copy link
Contributor Author

jevank commented Feb 15, 2022

What is interesting to me, where actually guest is granted access to that device - is it mediated through (patched?) qemu? In that case, I guess it requires qemu running in dom0 - otherwise it wouldn't have access to the GPU. Or maybe I'm missing something here?

With minimal support (w/o emulated virtual display) no needs to significantly patch qemu. As I understand it is some kind of dom0 kernel emulated device with ioreq_server.

https://github.com/jevank/qubes-linux-kernel-gvt/blob/bc702750a2127be3be91a80ad2104075270776ce/0002-arch-x86-xen-add-infrastruction-in-xen-to-support-gv.patch#L1433-L1450

@marmarek
Copy link
Member

Okay, that makes sense. Implementing IOREQ server in dom0 kernel is a bit ... unusual, but technically it should work. What is worrying here, that it adds another interface were guest can talk to dom0 kernel (the other one is xen-blkback), which is extra attack surface. I'm not sure how resilient for attacks is that part, so until proven otherwise, I assume "not much". For this reason - as discussed before - I don't want the kernel patch included in default package. But this PR is fine.

BTW, we really need some more visibility of the risky settings/configurations state - something explored in QubesOS/qubes-issues#6 (comment), but that's independent of this PR.

@jevank
Copy link
Contributor Author

jevank commented Feb 15, 2022

Okay, that makes sense. Implementing IOREQ server in dom0 kernel is a bit ... unusual, but technically it should work.

I guess this is the main reason why it wasn't accepted by Xen upstream...

For this reason - as discussed before - I don't want the kernel patch included in default package.

This is absolutely reasonable. Our position on supporting this solution is to give at least some ability to work with GPU to users who take such risks. It might be GVT-g now and Intel vGPU based on SR-IOV later.

But this PR is fine.

Great! Than I will prepare PRs with libvirt, stubdom support and qubes-gui-agent for review.

BTW, we really need some more visibility of the risky settings/configurations state - something explored in QubesOS/qubes-issues#6 (comment), but that's independent of this PR.

That would be a great feature.

@jevank jevank marked this pull request as ready for review February 15, 2022 13:51
@marmarek
Copy link
Member

@HW42 can you confirm my above assessment about including this PR in the default build?

@jevank
Copy link
Contributor Author

jevank commented Aug 5, 2022

Any chance of making progress here?

@marmarek
Copy link
Member

PipelineRefresh

@marmarek
Copy link
Member

For the record, the hypercall introduced here on its own isn't a security risk (as commented earlier), but it is really easy to misuse. And as far as I can tell, the Linux kernel GVT patches do misuse it. So, this is yet another reason why using GVT-g with untrusted guest is unsafe from security point of view, and in its current shape is not acceptable in Qubes by default. But, there are workflows where it can be useful for trusted guests - but user user needs to know and accept the risks. Having to install 3rd-party dom0 kernel should be a barrier high enough to ensure it isn't enabled by accident.

It gives physical address of some VM's memory page, but it doesn't guarantee that page will keep be used by that VM. When a VM is destroyed, or balloon down happen (or other similar action), that memory page might be re-assigned to another guest. Now, it seems GVT uses it for instructing the actual device to perform DMA to/from that address. Without proper reference counting, that page might be released by its original owner and used for something else (like, another guest) - then the GPU might read from/write to another guest. I suppose all this is properly coordinated with well behaving guests, but it seems to not be safe against misbehaving guests. Furthermore, this approach looks to be incompatible with IOMMU, and I guess it requires `iommu=no-igfx` Xen option. Using (non-existing yet) vIOMMU API might solve both issues, but that isn't an option right now.

@marmarek marmarek merged commit 4286cef into QubesOS:xen-4.14 Aug 16, 2022
@jevank
Copy link
Contributor Author

jevank commented Aug 16, 2022

I just had a thought, shouldn't I have made mutual dependencies with libvirt to prevent partial upgrade and libxl incompatibility?..

It gives physical address of some VM's memory page, but it doesn't guarantee that page will keep be used by that VM. When a VM is destroyed, or balloon down happen (or other similar action), that memory page might be re-assigned to another guest.

It is requires to disable memory balloning with GVT-g Linux kernel to be stable enough.

Furthermore, this approach looks to be incompatible with IOMMU, and I guess it requires iommu=no-igfx Xen option.

This is optional by the way, but might be useful to disable IOMMU in case of troubleshooting. Practically it helps to get rid of intrusive messages in dom0 kernel logs AFAIK unrelated to the of GVT-g using (page flipping or similar).

@marmarek
Copy link
Member

I just had a thought, shouldn't I have made mutual dependencies with libvirt to prevent partial upgrade and libxl incompatibility?..

Yes, that is an issue. I'll handle that when uploading new package to current-testing.

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.

2 participants