Please enable opengl for acceleration and vfio-MDEV support

Bug #1804766 reported by Christian Ehrhardt  on 2018-11-23
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
libvirt (Ubuntu)
High
Unassigned
Bionic
Wishlist
Unassigned
Cosmic
Wishlist
Unassigned
qemu (Ubuntu)
Undecided
Unassigned
Bionic
Wishlist
Unassigned
Cosmic
Wishlist
Unassigned

Bug Description

[Impact]

 * Newer GPU hardware of Nvidia (4.15) and Intel (4.16) can be split into
   virtual GPUs by their host device driver. It is customers demand to be
   able to utilize that sooner than waiting for 20.04 to happen. While the
   usability prior to very recent libvirt versions (not backporting all of
   that as it is not SRUable) isn't perfect it works well enough as-is.

 * The fix is enabling a build option and some build depends, the code is
   already there.

 * This applies to the safe SRU exception of "For Long Term Support
   releases we sometimes want to introduce new features. They must not
   change the behaviour on existing installations ..." due to the fact
   that gl is disabled by default. So users need to opt-in to use it after
   the change here.

[Test Case]

 * There are various ways to test this but they all depend on certain HW
   to be available as the feature starts with the Hosts GPU device driver
   breaking the card into shareable mediated devices.

 * In theory some Nvidia GPU cards [1] can work as well, but I didn't have
   one of those to test and most others verifying this might not have one
   either. Therefore I'll outline the use of the much more widespread
   Intel i915 chips. Note in Bionic i915 vGPU needs the HWE kernel to
   work.

 * Prepare Host to be able to split the GPU
   # loading modules
   $ printf "kvmgt\nvfio-iommu-type1\nvfio-mdev" | sudo tee /etc/initramfs-tools/modules
   # in /etc/default/grub add kernel opts
       i915.enable_gvt=1 intel_iommu=on drm.debug=0
   # Refresh boot config and reboot
   $ sudo update-initramfs -u
   $ sudo update-grub

 * Create vGPUs in the host, the PCI path depends on your system.
   # Check lspci if you have a different slot than 0000:00:02.0
   $ cd /sys/bus/pci/devices/0000:00:02.0/i915-GVTg_V4_4/
   # you need new UUIDs, you can e.g. run
   $ uuid -n 2
   # and then take these UUIDs into the driver to create the vGPUs
   $ echo 4dd50f26-ec08-11e8-b838-4bc3356865b6 | sudo tee create
   $ echo 4dd511f6-ec08-11e8-b839-2f163ddee3b3 | sudo tee create
   # you should then see soemthing like:
   $ ls -laF /sys/bus/mdev/devices/
lrwxrwxrwx 1 root root 0 Nov 19 15:38 4dd50f26-ec08-11e8-b838-4bc3356865b6 -> ../../../devices/pci0000:00/0000:00:02.0/4dd50f26-ec08-11e8-b838-4bc3356865b6/
lrwxrwxrwx 1 root root 0 Nov 19 15:38 4dd511f6-ec08-11e8-b839-2f163ddee3b3 -> ../../../devices/pci0000:00/0000:00:02.0/4dd511f6-ec08-11e8-b839-2f163ddee3b3/

 * you can start qemu "as usual" with the tweak of adding
   - gl=on to the display statement
   - a vfio-pci device referencing the MDEV uuid path
   # Example qemu:
   /usr/bin/qemu-system-x86_64 \
   -m 2048 \
   -nodefaults \
   -enable-kvm \
   -M graphics=off \
   -serial stdio \
   -cpu host \
   -display gtk,gl=on \
   -usb -usbdevice tablet \
   -net nic -net user \
   -device vfio-pci,sysfsdev=/sys/bus/pci/devices/0000:00:02.0/4dd511f6-ec08-11e8-b839-2f163ddee3b3,display=on,rombar=0 \
   -hda /var/lib/uvtool/libvirt/images/bionic-vgpu.qcow

  This qemu will boot (you could as well use a Ubuntu ISO, I used a image
  I created with uvtool in this case) and when you log in you will see in
  the guest the Intel HD graphics card in e.g. lspci.

[Regression Potential]

 * by being default off the regressions to current users should be near
   zero. We are not changing actual code, but "only" enable a config.
   If the config would enable code that runs in cases that are default
   enabled (like by an ifdef in C code that makes code outside of gl=on
   run the extra code) there could be a regression that we don't see yet.

[Other Info]

 * This is NOT virgl based vGPUs as in bug 1657409
 * Some related links for the MDEV-GPU topic in general:
   https://bugzilla.redhat.com/show_bug.cgi?id=1460804
   https://bugzilla.redhat.com/show_bug.cgi?id=1337290
   https://docs.nvidia.com/grid/latest/grid-vgpu-release-notes-red-hat-el-kvm/index.html
   http://on-demand.gputechconf.com/gtc/2017/presentation/s7572-suneel-marthi-trevor-grant-extending-mahout-samsara.pdf
   https://www.kraxel.org/blog/2018/04/vgpu-display-support-finally-merged-upstream/
   https://github.com/intel/gvt-linux/wiki/GVTg_Setup_Guide
   https://docs.nvidia.com/grid/latest/grid-vgpu-release-notes-red-hat-el-kvm/index.html
   https://www.linux-kvm.org/images/5/59/02x03-Neo_Jia_and_Kirti_Wankhede-vGPU_on_KVM-A_VFIO_based_Framework.pdf
   http://on-demand.gputechconf.com/gtc/2017/presentation/s7572-suneel-marthi-trevor-grant-extending-mahout-samsara.pdf

[1]: https://docs.nvidia.com/grid/latest/grid-vgpu-user-guide/index.html

----

Hi,
for quite a while we had thought enabling opengl to go along the MIR for virglrenderer (bug 1657409), but missed that it is a prereq for using MDEVs as well.
This now become much more important as newer drivers (recent nvidia drivers, as well as kernel >4.16 for intel) made this much more available to users - and it would work just fine if qemu had opengl enabled.

Other than for virgl we don't need additional libraries to be promoted to MAIN.
Those we need for "just" opengl are already in MAIN for Ubuntu Desktop support.
So the change is as easy as adding a few build-deps and setting a config option.

The code is already in place, qemu 2.11 (Bionic) has all it needs for nvidia and 2.12 >=Cosmic has the code for Intel.
That said flipping this build option (no other change) would allow:
- >= 18.10 use vfio MDEVs for gpu's of nvidia and intel
- 18.04 use vfio MDEVs for gpu's of nvidia
- 18.04 + HWE kernel + later Cloud-Archive with qemu >2.12 use vfio MDEVs for intel gpus

I did a bunch of tests and found no related regressions.
And in terms of SRU terms this falls clearly under "enabling new HW/Feature in LTS" exception as the use of vfio-MDEV for gpus just wasn't as clear back then but becomes more and more important now.

Next steps:
- extra testing
- getting it into Disco
- considering SRUs from there

Changed in qemu (Ubuntu Bionic):
status: New → Triaged
Changed in qemu (Ubuntu Cosmic):
status: New → Triaged
Changed in qemu (Ubuntu):
status: New → In Progress

As there are currently so many dev/SRU/CVE things are in flight on these packages tests and builds are combined in https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3520/+packages

I forgot to mention - gl is by default off, you have to enable it on qemu commandline (or through the management software of your choice if it supports setting that already).
But that makes the enabling rather safe in terms of "SRUs should not affect existing users" as it adds a "new, but default-off" feature.

Working PPA version as part of the planned next SRU cycle (post the currently in flight CVE fixes) at [1].

[1]: https://launchpad.net/~ci-train-ppa-service/+archive/ubuntu/3520/+packages

description: updated
description: updated

Since enabling new build-deps sometimes causes unexpected deps up to component mismatches I checked the builds.

59 Dependencies are in main
5 Dependencies are in universe, but from this SRC
0 Dependencies are new (likely introduced by this build)
1 Dependencies are in universe (component mismatch)
0 packages are of unknown state

This is disco, but other releases look more or less the same.
The one universe package is from qemu-user-binfmt which is a binary built by src:qemu but not in main itself, therefore not a problem.

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package qemu - 1:2.12+dfsg-3ubuntu9

---------------
qemu (1:2.12+dfsg-3ubuntu9) disco; urgency=medium

  [ Marc Deslauriers ]
  * SECURITY UPDATE: integer overflow in NE2000 NIC emulation
    - debian/patches/CVE-2018-10839.patch: use proper type in
      hw/net/ne2000.c.
    - CVE-2018-10839
  * SECURITY UPDATE: integer overflow via crafted QMP command
    - debian/patches/CVE-2018-12617.patch: check bytes count read by
      guest-file-read in qga/commands-posix.c.
    - CVE-2018-12617
  * SECURITY UPDATE: OOB heap buffer r/w access in NVM Express Controller
    - debian/patches/CVE-2018-16847.patch: check size in hw/block/nvme.c.
    - CVE-2018-16847
  * SECURITY UPDATE: buffer overflow in rtl8139
    - debian/patches/CVE-2018-17958.patch: use proper type in
      hw/net/rtl8139.c.
    - CVE-2018-17958
  * SECURITY UPDATE: buffer overflow in pcnet
    - debian/patches/CVE-2018-17962.patch: use proper type in
      hw/net/pcnet.c.
    - CVE-2018-17962
  * SECURITY UPDATE: DoS via large packet sizes
    - debian/patches/CVE-2018-17963.patch: check size in net/net.c.
    - CVE-2018-17963
  * SECURITY UPDATE: DoS in lsi53c895a
    - debian/patches/CVE-2018-18849.patch: check message length value is
      valid in hw/scsi/lsi53c895a.c.
    - CVE-2018-18849
  * SECURITY UPDATE: Out-of-bounds r/w stack access in ppc64
    - debian/patches/CVE-2018-18954.patch: check size before data buffer
      access in hw/ppc/pnv_lpc.c.
    - CVE-2018-18954
  * SECURITY UPDATE: race condition in 9p
    - debian/patches/CVE-2018-19364-1.patch: use write lock in
      hw/9pfs/cofile.c.
    - debian/patches/CVE-2018-19364-2.patch: use write lock in
      hw/9pfs/9p.c.
    - CVE-2018-19364

  [ Christian Ehrhardt]
  * debian/patches/ubuntu/lp1787405-*: Support guest dedicated Crypto
    Adapters on s390x (LP: #1787405)
  * enable opengl for vfio-MDEV support (LP: #1804766)
    - d/control-in: set --enable-opengl
    - d/control-in: add gl related build-dependencies

 -- Christian Ehrhardt <email address hidden> Wed, 21 Nov 2018 13:17:01 -0500

Changed in qemu (Ubuntu):
status: In Progress → Fix Released

After the current CVEs passed I verified the precheck PPA matches what we expected.

I did another test with the proposed SRU in regard to the opengl changes on UI based workflows.
Ensuring that by default nothing changes (speed/acceleration) and if tools like e.g. virt-manager would get into trouble once the option is supported.

At least for all my tests:
- UI behaves unchanged by default.
- opengl can be activated in UI and commandline

I realized from my notes to this bug we also need libvirt:

Switching gl on needs the apparmor rules that I mentioned in the description.
# For opengl based display options (LP: #1804766)
/dev/dri/ r,
/dev/dri/* r,

Along that as mentioned libvirt-qemu needs to become member of the video group.
sudo usermod -a -G video libvirt-qemu

I also want to check with the security Team how safe they consider this or if we want to make this an "admin has to opt in" thing.

Hmm, lets unbundle that from the SRU unless strictly required and only continue once better testable (HW availability)

I should add a libvirt task for that to cover it as well.

Changed in qemu (Ubuntu Bionic):
importance: Undecided → Wishlist
Changed in qemu (Ubuntu Cosmic):
importance: Undecided → Wishlist
Changed in libvirt (Ubuntu Bionic):
status: New → Triaged
importance: Undecided → Wishlist
Changed in libvirt (Ubuntu Cosmic):
importance: Undecided → Wishlist
status: New → Triaged
Changed in libvirt (Ubuntu):
status: New → Triaged
importance: Undecided → High

@ubuntu-security:
would you mind checking and letting me know your opinion on adding the following to /etc/apparmor/abstractions/libvirt-qemu
Note: I have not seen a way to detect in virt-aa-helper if gl is enabled to do it dynamically:

# For opengl based display options (LP: #1804766)
/dev/dri/ r,
/dev/dri/* r,

Furthermore I'd want to make the user we run qemu with be part of the video group (if available).
Like this in the postinst after creating the user:
  $ sudo usermod -a -G video libvirt-qemu

Please let me know if you'd consider that ok - needed for sharing vGPUs of cards as MDEV to the guest. Also down the road required for accelerated virtio-vga once we would support virglrender.

tags: added: libvirt-19.04 qemu-19.04

For the security question - amurray pointed out this is in abstractions/X already.
Down the road I might need the EGL line as well.
So if you could also discuss if I should instead of the raw rules use that abstraction that would be nice.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers