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

vglass: replace surfman/input-server with vglass #1382

Merged
merged 38 commits into from
May 28, 2021
Merged

Conversation

eric-ch
Copy link
Contributor

@eric-ch eric-ch commented Dec 15, 2020

In this changeset:

  • Retire Surfman related projects (surfman, libsusrfman, surfman-plugins, input_server, libdmbus, shutdown-screen, xenfb2, gem-foreign/i915-foreign patches, libedid);
  • Retire the SELinux definitions associated with the aforementioned projects;
  • Retire the IDL exclusively associated with the aforementioned projects;
  • Include the vglass layer: https://gitlab.com/vglass/meta-vglass as a dependency, note this inherits meta-qt5 as a dependency;
  • Amend the necessary configuration to use the required vGlass PV drivers where necessary (ivc, pv-display-helper, openxtfb in UIVM)

General notes:

  • dom0 memory is bumped to 768M. This has been changed by recommandation, yet it is suspected keeping 512M should be enough with the backported patch for gntdev (9b24f64).

vGlass notes:

  • This submission currently relies on openxtfb as a PV framebuffer driver in UIVM. An alternative is available (glassdrm), and will be the object of later submissions.
  • IVC (libivc2) in dom0 is not the same component as IVC (libivc) in service-VMs (stubdomains included).
  • HVM w/o stubdomain guests are not supported by the current vGlass stack. There is no graphic support for this use-case due to IVC2/IVC limitations at the moment.
  • Monitor hotplug issues have been reported and are being investigted, they can and should be reported in the vGlass gitlab group: gitlab.com/groups/vglass
  • There is a known limitation with Windows PV Tools when using the emulated mouse. This changeset includes the work-around to let Windows HVM w/ stubdomain guests create and use a PV device.

Windows PV tools:

  • vGlass PV tools interface with Xen Windows PV drivers 9.0, the builder provides a built binary: https://openxt.ainfosec.com/OpenXT/auto/windows/9.0.0/;
  • The bundled installer also includes a port of OpenXT Windows PV USB drivers on Xen Windows PV drivers 9.0;
  • There is no PV audio drivers available at the moment;

Builds and test:

Depends on:

@eric-ch
Copy link
Contributor Author

eric-ch commented Dec 18, 2020

v2: Rebase on top of master. No change.

Copy link
Contributor

@jandryuk jandryuk left a comment

Choose a reason for hiding this comment

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

Noticed two avcs when a usb-c dock triggered display and usb hotplug. This was a local build - are the public ones running in permissive?

vusb.te should gain
glass_dbus_chat(vusbd_t)
It used to have input_server_dbus_chat(vusbd_t).

dbus-daemon[1142]: avc:  denied  { send_msg } for msgtype=method_call interface=com.citrix.xenclient.input member=get_focus_domid dest=com.citrix.xenclient.input spid=1299 tpid=1388 scontext=system_u:system_r:vusbd_t:s0 tcontext=system_u:system_r:glass_t:s0 tclass=dbus permissive=0

disman.te should gain
disman_dbus_chat(udev_t)
So that udev -> 99-disman-hotplug.rules -> /usr/bin/disman-hotplug.sh -> dbus-send can work. Or disman_script_t needs to be fully implemented as I've indicated elsewhere.

dbus-daemon[1142]: avc:  denied  { send_msg } for msgtype=method_call interface=mil.af.secureview.disman member=hotplug dest=mil.af.secureview.disman spid=3726 tpid=1395 scontext=system_u:system_r:udev_t:s0-s0:c0.c1023 tcontext=system_u:system_r:disman_t:s0 tclass=dbus permissive=0

conf/machine/xenclient-uivm.conf Show resolved Hide resolved
/etc/init\.d/disman -- gen_context(system_u:object_r:disman_initrc_exec_t,s0)

/usr/bin/disman -- gen_context(system_u:object_r:disman_exec_t,s0)
/usr/bin/disman-hotplug.sh -- gen_context(system_u:object_r:disman_exec_t,s0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Swith this to disman_script_exec_t:
/usr/bin/disman-hotplug.sh -- gen_context(system_u:object_r:disman_script_exec_t,s0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and also \ that ..

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Good self-review 🙂

Copy link
Contributor

@jandryuk jandryuk left a comment

Choose a reason for hiding this comment

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

These can go in separately and immediately, I think:
d9eac76
e12d717

conf/machine/xenclient-dom0.conf Show resolved Hide resolved
@@ -96,6 +96,7 @@ IMAGE_INSTALL += "\
anthy \
matchbox-keyboard \
matchbox-keyboard-im \
kernel-module-openxtfb \
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it's in the wrong commit 761aed3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indeed.

@@ -21,3 +19,5 @@ KERNEL_MODULE_AUTOLOAD += " \
psmouse \
hid-multitouch \
"

PREFERRED_PROVIDER_virtual/libivc = "libivc2"
Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping that dependencies could handle this, but libpvglass depends on virtual/libivc, so we can't just make vglass depend on libvc2 and qemu depend on libivc, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is why I ended up defining a prefered per-machine provider, which in turns comes from libpvbackendhelper/libpvdisplayhelper, aka recipe libpvglass, exporting the same public headers, OE rightfully reports one can overwrite the other if they are provided by two different recipes.

recipes-extended/qemu-dm/qemu-dm.inc Show resolved Hide resolved

do_configure_append_openxt-installer(){
# vglass supports radeon and nouveau DRM.
kernel_conf_variable DRM_RADEON m
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just update all the relevant defconfigs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. It was somewhat convenient to isolate the changes initially.
Just for documentation: With #1383 it would become configuration fragments.

{
libxl__device_console console;
libxl__device device;
- libxl_device_vkb vkb;
+
+ for (i = 0; i < d_config->num_vkbs; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding this here, this hunk can be dropped to re-instate the original code

- libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);

That is assuming 1 vkb is acceptable, but I think only one is added with vglass.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a long time ago and I'm trying to remember why the vkbs ended up like this. libxl-xenmgr-support.patch split apart the vfb and vkb creation in xl_parse.c because I think we wanted 2 vkbs (one for keyboard and one for mouse). I think you're right about vglass not needing 2 vkbs so we could drop the loop in xl_parse.c, but the UIVM still wants vfb=0, and vkb=1, so the initialization needs to remain split.

The d_config->vkbs[] array already has initialized vkbs from the code in xl_parse.c, so we're just calling the libxl__device_add() in this hunk instead of initializing another new vkb, like line 78 in libxl-xenmgr-support.patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, input server used two vkbs because a single one can only send either relative or absolute events.

UIVM is PVH, so it doesn't go through this code path and vkbs can be added separately anyway. My suggested change would mean every HVM gets a vkb xenstore entry regardless of config.vkbd setting. vglass actually providing a backend seems to be conditioned on the config.vglass-enabled setting.

The previously mentioned xenmgr change to use vkb=[] (https://github.com/jandryuk/manager/commits/vkb-fixup) may mean I'm missing something for the case without it.

Copy link
Contributor Author

@eric-ch eric-ch Jan 7, 2021

Choose a reason for hiding this comment

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

vkb=[0|1] and vfb=[0|1] are colliding with xl.cfg(5) definitions for vkb=[ "VKB_SPEC_STRING", "VKB_SPEC_STRING", ...] and vfb=[ "VFB_SPEC_STRING", "VFB_SPEC_STRING", ...] which, it sounds like, we could use directly without removing the other PV display support like @jandryuk points out.

Windows HVM guests and UIVM by default boot with vfb=0 and vkb=1 when using vglass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With @jandryuk changes the changes from libxl-xenmgr-support.patch related to vkb and vfb are dropped. Are there still concerns regarding this?

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 7, 2021

v2

  • Address refpolicy comments and typos (e,g \.);
  • xen-fbfront is no longer in the modprobe configuration, it will be loaded depending on the xenbus probe. xenbus.wait_nonessentials is removed since no longer needed;
  • vkb and vfb flags in xenmgr are deprecated, amend the libxl patch in consequence (see relevant commits in vglass: introduce RPCs used by vglass and remove deprecated ones idl#41 and vglass: deprecate surfman logics and wire up vglass manager#180). An upgrade script is still needed;
  • Prune the kernel defconfigs using bitbake -c savedefconfig to remove auto-selected configurations;
  • Transfer kernel configuration from the linux-openxt recipe to the defconfig themselves;
  • sort out cross-commit changes

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 8, 2021

Build: $mcv-9

@jandryuk
Copy link
Contributor

jandryuk commented Jan 8, 2021

This looks good.
a43a418 got re-introduced?
c77f7d5 should maybe be its own PR?

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 8, 2021

@jandryuk a43a418 is a mistake on my part, I must have rebased before the other PR went in. Agreed for c77f7d5.

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 8, 2021

v3

  • Rebase on master
  • Removed c77f7d5 for separate submission.

@jandryuk
Copy link
Contributor

I've seen vglass segfault 4 times.
This is Eric's build. Looks like it was from hot unplugging a USB-C Dock with an external display.

Dec 16 16:37:25.486943 kernel:[ 3267.244797] glass[1433]: segfault at 0 ip 00007ff88ba1c10f sp 00007ffd47b71280 error 4 in libfs_wm.so.1.0.0[7ff88ba0b000+1f000]

The December segfault is Eric's build and the January ones are my own builds:

Dec 18 13:54:31.235419 kernel:[ 4169.988263] glass[1295]: segfault at a0 ip 00007f3b6a02c570 sp 00007ffea13b7528 error 4 in libpthread-2.31.so[7f3b6a028000+f000]
Jan  8 20:22:00.205688 kernel:[ 1019.965024] glass[1408]: segfault at a0 ip 00007f32e6a95570 sp 00007fffab7649e8 error 4 in libpthread-2.31.so[7f32e6a91000+f000]
Jan 13 19:27:04.764638 kernel:[16805.510069] glass[1373]: segfault at a0 ip 00007f6d7dcbb570 sp 00007fff73002aa8 error 4 in libpthread-2.31.so[7f6d7dcb7000+f000]

These occur randomly when shutting down a Windows 10 HVM without PV graphics drivers.

If you ssh into dom0, you can manual restart vglass, but uivm doesn't display - it doesn't reconnect its graphics. X, surf and xterm windows are still running seemingly none the wiser. Restarting uivm restores the display. The inittab line makes this tricky, but If you sshargo into uivm, you can kill X, rmmod and modprobe openxtfb, and then restart X to get the display restored.

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 13, 2021

I have also had glass crash when plugging an external display (DVI port, but likely irrelevant) while a windows guest with PV tools is running, but UIVM is in focus:

[75046.121497] glass[1717]: segfault at 0 ip 0000000000000000 sp 00007faaf3013418 error 14 in glass[400000+6000]
[75046.121537] Code: Bad RIP value.

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 22, 2021

OpenXT: Build: $mcqv-11
Windows tools: Build: $9.0.0-5

@eric-ch
Copy link
Contributor Author

eric-ch commented Jan 26, 2021

V4

  • Rebase on master
  • Add new dependency to xenmgr to support the new RPC

@eric-ch
Copy link
Contributor Author

eric-ch commented Apr 15, 2021

V5
  • Rebase on master

@eric-ch
Copy link
Contributor Author

eric-ch commented Apr 26, 2021

OpenXT: Build: $master-custom-vglass-18
WinTools: Build: $windows-tools-9-0-0-13

Copy link
Member

@dpsmith dpsmith left a comment

Choose a reason for hiding this comment

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

No Rb since I haven't honestly reviewed the latest but willing to ACK merging

Eric Chanudet added 6 commits May 20, 2021 17:24
replaced by openxtfb or xenfb.

Signed-off-by: Eric Chanudet <[email protected]>
Deprecated, vGlass can handle splash-screen without help from the
kernel.

Signed-off-by: Eric Chanudet <[email protected]>
Replaced with vglass.
So long Surface Manager.

Signed-off-by: Eric Chanudet <[email protected]>
Signed-off-by: Eric Chanudet <[email protected]>
Replaced by vglass.

Signed-off-by: Eric Chanudet <[email protected]>
Eric Chanudet added 25 commits May 20, 2021 17:24
https://www.openembedded.org/wiki/Styleguide
Add quotes and use OE variables.
Remove deprecated Rpc.

Signed-off-by: Eric Chanudet <[email protected]>
The only consumers of dmbus were surfman and input-server. Both are
deprecated, dmbus can follow.

Signed-off-by: Eric Chanudet <[email protected]>
remove switcher.patch
remove xenmou.patch

switcher.patch also added the configuration for libargo, this is now
done in a new patch (argo-configure.patch).

Fix conflicts and refresh relevant patches on top of the above.

Signed-off-by: Eric Chanudet <[email protected]>
xen-translate.patch is no longer required and was already a hack as
surfman should have dealt with gpfn instead of mfns.

workaround-nehalem-igd-vtd.patch is a work-around with security
implications for platform 10+ years old. vglass no longer rely on the
gem-foreign which is the root cause of this.

Signed-off-by: Eric Chanudet <[email protected]>
libivc2/ivcdaemon is only suited for dom0.
The Linux stubdomain should not add QT or C++ runtime dependencies to
keep the footprint manageable, hence use the ivc module through libivc.

Signed-off-by: Eric Chanudet <[email protected]>
Refresh the patch-queue for fuzz introduced by vglass changes in it.

Signed-off-by: Eric Chanudet <[email protected]>
Apparently a wrapper script around depmod.
Unused and forgotten.

Signed-off-by: Eric Chanudet <[email protected]>
bitbake -c savedefconfig, which runs make savedefconfig, which in turns
takes into account automatically selected configurations.

No functional changes, this only removes kernel configuration statement
that would be selected anyway by the remaining configuration or
generated by the kernel build-system.

Signed-off-by: Eric Chanudet <[email protected]>
Add vglass components to relevant images and configurations.

- enable meta-vglass layer
- Add vglass and disman to dom0
- Pass kernel modules recommended for vglass.

Note, some of the kernel configuration only handles drivers in images
that do not leverage vglass, but are now supported with vglass.

Signed-off-by: Eric Chanudet <[email protected]>
inherit the module-signing bbclass for out-of-tree modules.

Signed-off-by: Eric Chanudet <[email protected]>
xenfb2 is now deprecated. Remove xf86 fbdev driver customization
supporting randr1.2 interfaces for modesetting to a higher resolution
framebuffer.

vGlass provides the same feature in openxtfb associated xf86 driver:
xf86-video-openxtfb.

Signed-off-by: Eric Chanudet <[email protected]>
Add openxtfb to UIVM.
vGlass provides an xf86 Xorg driver for the PV framebuffer openxtfb.

Signed-off-by: Eric Chanudet <[email protected]>
openxtfb is the default framebuffer driver in UIVM with vGlass.
Autoload the driver.

Signed-off-by: Eric Chanudet <[email protected]>
allow-pat-cacheattrs-for-all-domains is only used by xenfb2/surfman that
would foreign map pages in the guest and have the GPU scan them, hence
required the guest to set the cache policies to WT or WC. PV guests
were prevented from doing this. Note that since UIVM was PVH, the cache
policy was changed in surfman anyway.

allow-stubdoms-cacheattr-control.patch handles the same issue from the
stubdomain perspective. Since the i915-foreign map is no more, this is
no longer required.

foreign-batch-cacheattr.patch is no longer required since the cache
policy is no longer modified by the display stack.

Signed-off-by: Eric Chanudet <[email protected]>
Surfman used VBE and other BIOS call extensions to handle early boot
resolution. This is no longer required.

QEMU:
- vbe-xt-extensions.patch: Added VBE calls to configure resolution taken
  by Surfman from the host EDID while being compatible with the GPU
  scan and make sure this was used from early compatible resolutions
  (set by vgabios). Used by vgabios/vbe-extensions.path and
  vgabios/vbe-edid-interface.patch
- vga-spinlock: Work-around for vgabios/vbe-xenvesa.patch presumably.
  int10h was used after bootstrap to pass up modsetting information to
  the defunct xenvesa driver. It is unclear how this could have raced.
  Used by vgabios/vga-spinlock.patch, vgabios/vga-shadown-bda.patch,
  vgabios/vbe-xenvesa.patch.
- vga-shadow-bda.patch: implement IO-ports to store VGABIOS data
  otherwise lost after early bootstrap as it resides in BDA.

VGABIOS:
- vbe-edid-interface.path: Use VBE extensions implemented in
  qemu/vbe-xt-extensions.patch to handle compatible resolutions for
  Surfman.
- vbe-extensions.patch: In the line of vbe-edid-interface.patch, except
  these calls are not defined in the VBE interface.
- vbe-xenvesa.patch: Pass a Surfman provided EDID through an int10h
  IO-port. The EDID being stored in BDA, this depends on
  vga-shadow-bda.patch as it would be used after the BDA is reclaimed by
  the guest OS.
- vga-shadow-bda.patch: Allow some BDA stored info to still be reachable
  post bootstrap.
- vga-spinlock.patch: spinlock to avoid presumed int10h races in
  emulated guests.

Signed-off-by: Eric Chanudet <[email protected]>
openxtfb depends on:
- FB_SYS_FILLRECT
- FB_SYS_COPYAREA
- FB_SYS_IMAGEBLIT
- FB_SYS_FOPS
- FB_DEFERRED_IO
- FB_MODE_HELPERS

In order to select the above configuration, UIVM is built with
XEN_FBDEV_FRONTEND as it shares and selects most of these. Both drivers
will therefor load creating /dev/fb0 and /dev/fb1 by default. This is
undesirable as it makes the XOrg configuration more complicated, so
avoid loading xen-fbfront.

This could be handled differently, e.g, in the xf86 driver.

Signed-off-by: Eric Chanudet <[email protected]>
modules.dep is present, modprobe can be used.
Add ivc and use modprobe for xen-argo.

Signed-off-by: Eric Chanudet <[email protected]>
libedid was used by Surfman to emulate EDID values.

Signed-off-by: Eric Chanudet <[email protected]>
vGlass has higher memory requirements.
This increase is motivated as vGlass would otherwise fail to drive UIVM
with ENOMEM when opening the grant-refs used by the channels with UIVM
and other guests.

Signed-off-by: Eric Chanudet <[email protected]>
Backport upstream commit:
b3f7931f5c61 xen/gntdev: switch from kcalloc() to kvcalloc()

When mapping guest framebuffer, gntdev can reach order 5 allocation
sizes (1920x1080 display only). Using non physically contiguous buffers
makes it easier on dom0's memory.

Signed-off-by: Eric Chanudet <[email protected]>
SELinux module policies for ivcdaemon, glass and disman components of
vglass.

Signed-off-by: Eric Chanudet <[email protected]>
Remove hunks related to display_depth and display_res passed via
XenStore.

No components were using these initially. These might be an artefact
from before libdmbus.

Signed-off-by: Eric Chanudet <[email protected]>
vfb=[0|1] and vkb=[0|1] only really create the base object for each
structure in order to get xl to create the base xenstore nodes.

This is redundant with vkb=[ "VKB_SPEC_STRING", "VKB_SPEC_STRING", ...]
and vfb=[ "VFB_SPEC_STRING", "VFB_SPEC_STRING", ...] which should be
used instead.

Note: vfb is constantly set to 0 in xenmgr with vglass, should '1' be useful
somehow it would need to be handled via a new VFB_SPEC_STRING entry to
have the base vfb structure initialized correctly.

Signed-off-by: Eric Chanudet <[email protected]>
Changes to Xenmgr require the split from Hackage.

Signed-off-by: Eric Chanudet <[email protected]>
@eric-ch
Copy link
Contributor Author

eric-ch commented May 24, 2021

V6

  • Rebase on master

@eric-ch
Copy link
Contributor Author

eric-ch commented May 25, 2021

@jandryuk
Copy link
Contributor

Merging soon

@jandryuk jandryuk merged commit 8522fd8 into OpenXT:master May 28, 2021
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.

4 participants