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

qemu: backport 9p-darwin to v6.2.0 #162243

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Conversation

willcohen
Copy link
Contributor

@willcohen willcohen commented Feb 28, 2022

Motivation for this change

Backports 9p-darwin functionality from upstream v9 patchset (https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05978.html) to v6.2.0.

Please note that the final commit in the backported patchset includes a utimensat fallback, which will be needed until macOS SDK reaches 10.13 (#101229). I'll be more than happy to rebase/edit that particular final patch as needed once 7.0-rc hits, to ease that upgrade.

Addresses changes proposed by #122420.

@domenkozar @alyssais

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@willcohen
Copy link
Contributor Author

nixpkgs-review is giving me all kinds of noisy errors because of other upstream stuff, but:

macOS:

$ nix-build $NIXPKGS -A qemu
/nix/store/d8whkgmadif7yx3qrjdsc661scnkax57-qemu-6.2.0

NixOS:

$ nix-build $NIXPKGS -A qemu
/nix/store/87myhvvhyj9nl4djcwif2bl0b9l69qyw-qemu-6.2.0

And from the qemu source directory on macOS, the 9pfs synth tests appear to pass with the new build too.

$ export QTEST_QEMU_BINARY=/nix/store/d8whkgmadif7yx3qrjdsc661scnkax57-qemu-6.2.0/bin/qemu-system-x86_64
$ cd build
$ tests/qtest/qos-test -m slow 
....
# End of x86_64 tests

@willcohen
Copy link
Contributor Author

Result of nixpkgs-review pr 162243 run on x86_64-darwin 1

11 packages marked as broken and skipped:
  • alpine-make-vm-image
  • cloud-init
  • cloud-utils
  • multibootusb
  • python310Packages.guestfs
  • python39Packages.guestfs
  • qemu_full
  • qemu_xen
  • qemu_xen-light
  • qemu_xen_4_10
  • qemu_xen_4_10-light
1 package failed to build:
  • python310Packages.cot
8 packages built:
  • colima
  • cot (python39Packages.cot)
  • lima
  • out-of-tree
  • qemu
  • qemu-utils
  • qemu_kvm
  • qemu_test

@willcohen
Copy link
Contributor Author

Result of nixpkgs-review pr 162243 run on x86_64-linux 1

8 packages marked as broken and skipped:
  • aqemu
  • linuxKernel.packages.hardkernel_4_14.virtualbox
  • linuxPackages_hardkernel_latest.virtualbox
  • multibootusb
  • qemu_xen
  • qemu_xen-light
  • qemu_xen_4_10
  • qemu_xen_4_10-light
1 package blacklisted:
  • tests.nixos-functions.nixosTest-test
48 packages built:
  • alpine-make-vm-image
  • cloud-init
  • cloud-utils
  • colima
  • cot (python39Packages.cot)
  • diffoscope
  • gnome.gnome-boxes
  • libguestfs
  • lima
  • linuxKernel.packages.linux_4_14.virtualbox
  • linuxPackages_4_14_hardened.virtualbox (linuxKernel.packages.linux_4_14_hardened.virtualbox)
  • linuxKernel.packages.linux_4_19.virtualbox
  • linuxPackages_4_19_hardened.virtualbox (linuxKernel.packages.linux_4_19_hardened.virtualbox)
  • linuxKernel.packages.linux_4_4.virtualbox
  • linuxKernel.packages.linux_4_9.virtualbox
  • linuxKernel.packages.linux_5_10.virtualbox
  • linuxPackages_5_10_hardened.virtualbox (linuxKernel.packages.linux_5_10_hardened.virtualbox)
  • linuxPackages.virtualbox (linuxKernel.packages.linux_5_15.virtualbox)
  • linuxPackages_hardened.virtualbox (linuxPackages_5_15_hardened.virtualbox)
  • linuxPackages_latest.virtualbox (linuxKernel.packages.linux_5_16.virtualbox)
  • linuxKernel.packages.linux_5_4.virtualbox
  • linuxPackages_5_4_hardened.virtualbox (linuxKernel.packages.linux_5_4_hardened.virtualbox)
  • linuxPackages_latest-libre.virtualbox (linuxKernel.packages.linux_latest_libre.virtualbox)
  • linuxPackages-libre.virtualbox (linuxKernel.packages.linux_libre.virtualbox)
  • linuxPackages_lqx.virtualbox (linuxKernel.packages.linux_lqx.virtualbox)
  • linuxPackages_testing_bcachefs.virtualbox (linuxKernel.packages.linux_testing_bcachefs.virtualbox)
  • linuxPackages_xanmod.virtualbox (linuxKernel.packages.linux_xanmod.virtualbox)
  • linuxPackages_zen.virtualbox (linuxKernel.packages.linux_zen.virtualbox)
  • open-watcom-bin
  • open-watcom-bin-unwrapped
  • out-of-tree
  • python310Packages.cot
  • python310Packages.guestfs
  • python39Packages.guestfs
  • qemu
  • qemu-utils
  • qemu_full
  • qemu_kvm
  • qemu_test
  • qtemu
  • quickemu
  • solo5
  • tests.trivial-builders.references
  • vagrant
  • virtualbox
  • virtualboxHardened
  • virtualboxHeadless
  • virtualboxWithExtpack

@risicle
Copy link
Contributor

risicle commented Mar 1, 2022

Builds for me, macos 10.15, along with the passthru test. Don't really know how to test this feature though. Any suggestions?

@willcohen
Copy link
Contributor Author

This is one piece of a many-part puzzle to solve various other issues, so you're right to ask how to test it.

For nix users, #108984 is the most relevant set of use cases, but this only solves one of multiple set of issues (finally merging all the random QEMU patches and forks mentioned there into nixpkgs).

To test a smaller use case of whether this version of QEMU integrates 9p correctly, containers/podman#13256 is probably your best bet. Unfortunately, it still requires fixing podman on nixpkgs. You can probably test it for now if you use homebrew's podman and nix's QEMU.

@willcohen
Copy link
Contributor Author

@risicle if you rebase qowoz's podman4-gvproxy from #161130 on top of my qemu branch from this PR and build that version of podman on top of this qemu:

$ podman machine init --volume /Users/yourusername:/mnt/yourusername
$ podman machine start
$ podman machine ssh
$ cd /mnt/yourusername
$ ls

You should be able to see your home directory from there!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/performant-graphical-vm-via-qemu/17895/3

@willcohen
Copy link
Contributor Author

With #122420 (comment), I think we can consider this feature-complete enough to safely merge v9 as a backport. @alyssais and @domenkozar let me know what you both think!

@risicle
Copy link
Contributor

risicle commented Mar 1, 2022

Hah.. Thanks for your detailed instructions @willcohen but after half an hour of messing around with it I realized of course it's not going to work on the mac I'm currently working on - it's virtualized itself. Anyway, builds & unit tests still pass as far as I can test 👍

@zowoq
Copy link
Contributor

zowoq commented Mar 1, 2022

Seems to work fine, previously podman/qemu would error trying to mount a volume.

podman machine init --now --volume /Users/zowoq:/mnt/zowoq
Extracting compressed file
Image resized.
Machine init complete
INFO[0000] waiting for clients...
INFO[0000] new connection from  to /tmp/podman/qemu_podman-machine-default.sock
Waiting for VM ...
qemu-system-x86_64: -virtfs local,path=/Users/zowoq,mount_tag=vol0,security_model=mapped-xattr: There is no option group 'virtfs'
qemu-system-x86_64: -virtfs local,path=/Users/zowoq,mount_tag=vol0,security_model=mapped-xattr: virtfs support is disabled
Error: dial unix /tmp/podman/podman-machine-default_ready.sock: connect: no such file or directory
ERRO[0003] cannot receive packets from , disconnecting: cannot read size from socket: EOF
ERRO[0003] cannot read size from socket: EOF

@zowoq
Copy link
Contributor

zowoq commented Mar 1, 2022

@ofborg build qemu

@nicknovitski
Copy link
Contributor

You should be able to see your home directory from there!

This worked for me, on macos 12.2.1, with this branch's qemu and the podman on master (thanks to #161130).

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Excellent work, thank you so much. I've been following it on the mailing list, and I've been hugely impressed with your dedication getting this through 9 rounds of review. As promised, I'm happy for this to be backported now that it's queued upstream. Thanks so much!

pkgs/applications/virtualization/qemu/default.nix Outdated Show resolved Hide resolved
pkgs/applications/virtualization/qemu/default.nix Outdated Show resolved Hide resolved
@tricktron
Copy link
Member

I could successfully mount volumes natively using this backport and the Podman branch from containers/podman#13409 on my m1 mac.

Amazing stuff:+1:.

@willcohen
Copy link
Contributor Author

@alyssais

Just force-pushed some changes.

  • Each of the commits is broken out now (for easier picking-and-choosing, as well as a cleaner potential switchover to the per-commit URL fetchpatch style of gitlab whenever the PR gets merged.)
  • All the changes which don't apply cleanly have been broken out into patch files.
  • Because the second commit (which also stopped applying cleanly) is now a patch file, the sed changes are no longer needed before building.

This can either be merged as is (and then I can prepare a smaller subsequent PR to point the nine patches to QEMU upstream), or we can wait till whenever it gets merged and I'll force push one more time. I'm not sure on timeline for when that merge would happen. The 9p maintainer submitted this latest pull request incorporating these changes this morning.

@willcohen
Copy link
Contributor Author

@alyssais 9p-darwin has been merged upstream. I've updated the PR to pull from the official qemu-project/qemu repo. I think this PR is finalized now.

@willcohen
Copy link
Contributor Author

Result of nixpkgs-review pr 162243 run on x86_64-darwin 1

1 package marked as broken and skipped:
  • alpine-make-vm-image
1 package failed to build:
  • python310Packages.cot
8 packages built:
  • colima
  • cot (python39Packages.cot)
  • lima
  • out-of-tree
  • qemu
  • qemu-utils
  • qemu_kvm
  • qemu_test

@alyssais
Copy link
Member

alyssais commented Mar 9, 2022

@willcohen sorry to put you through another round of this, but could we apply the patches unconditionally? It makes maintenance easier, because it means I'll notice e.g. patches that fail to apply immediately when updating from Linux.

@willcohen
Copy link
Contributor Author

Of course. Will revise.

@willcohen
Copy link
Contributor Author

@alyssais oh wait, I remember why I did this now. If I try to apply these patches on top of 9p-ignore-noatime, they fail:

unpacking source archive /nix/store/1rnwb2yy7n64y86hakkb0acrkp65a9pf-qemu-6.2.0.tar.xz
source root is qemu-6.2.0
setting SOURCE_DATE_EPOCH to timestamp 1639514589 of file qemu-6.2.0/roms/edk2/UnitTestFrameworkPkg/Library/CmockaLib/cmocka/tests/test_wildcard.c
patching sources
applying patch /nix/store/9acf6z50qqingjpn8i6bw9r5d85nxzhg-fix-qemu-ga.patch
patching file qga/commands-posix.c
Hunk #1 succeeded at 109 with fuzz 2.
applying patch /nix/store/k5ncmjr0jf0rkkgb8ndml1f36knv3njv-9p-ignore-noatime.patch
patching file hw/9pfs/9p.c
Hunk #1 succeeded at 137 (offset 10 lines).
applying patch /nix/store/ra37ssglh8dds19fbr0ymq2qnizr5bxl-7e3e20d89129614f4a7b2451fe321cc6ccca3b76.diff
patching file include/ui/clipboard.h
Hunk #1 succeeded at 218 with fuzz 1 (offset 31 lines).
patching file ui/clipboard.c
Hunk #1 succeeded at 114 (offset 41 lines).
patching file ui/cocoa.m
applying patch /nix/store/0zbyxfcpbw93380jzfky5gkpayfzamdq-e0bd743bb2dd4985791d4de880446bdbb4e04fed.patch
patching file fsdev/file-op-9p.h
patching file hw/9pfs/9p-local.c
patching file hw/9pfs/9p.c
patching file include/qemu/xattr.h
applying patch /nix/store/nm69lx0icsnx7s0ad18yz8l38gml9mss-rename-9p-util.patch
patching file hw/9pfs/9p-util-linux.c (renamed from hw/9pfs/9p-util.c)
patching file hw/9pfs/meson.build
applying patch /nix/store/pqcdyn6gvkwndnsm5i9m43ykb0ahnpji-f41db099c71151291c269bf48ad006de9cbd9ca6.patch
patching file hw/9pfs/9p-proxy.c
patching file hw/9pfs/9p-synth.c
Hunk #1 succeeded at 427 (offset -12 lines).
patching file hw/9pfs/9p.c
Hunk #1 succeeded at 1312 (offset -1 lines).
Hunk #2 succeeded at 3524 (offset -1 lines).
applying patch /nix/store/05l0gl7zfpk9xin41srpcv8x8cdd5ja9-6b3b279bd670c6a2fa23c9049820c814f0e2c846.patch
patching file hw/9pfs/9p-local.c
patching file hw/9pfs/9p-proxy.c
patching file hw/9pfs/9p-synth.c
Hunk #1 succeeded at 222 with fuzz 2 (offset -12 lines).
patching file hw/9pfs/9p-util.h
patching file hw/9pfs/9p.c
Hunk #2 succeeded at 2281 (offset -1 lines).
Hunk #3 succeeded at 2420 (offset -1 lines).
Hunk #4 succeeded at 2481 (offset -1 lines).
patching file hw/9pfs/codir.c
Hunk #2 succeeded at 169 (offset 1 line).
applying patch /nix/store/2zi4jkf6apny80mjvsnysf9y39hkvcjh-67a71e3b71a2834d028031a92e76eb9444e423c6.patch
patching file hw/9pfs/9p-util.h
patching file hw/9pfs/9p.c
Hunk #1 FAILED at 138.
Hunk #2 succeeded at 170 (offset -1 lines).
1 out of 2 hunks FAILED -- saving rejects to file hw/9pfs/9p.c.rej
error: builder for '/nix/store/prrnyklwp6cbymighad5d8w6fk79q0qi-qemu-6.2.0.drv' failed with exit code 1;
       last 10 log lines:
       > Hunk #3 succeeded at 2420 (offset -1 lines).
       > Hunk #4 succeeded at 2481 (offset -1 lines).
       > patching file hw/9pfs/codir.c
       > Hunk #2 succeeded at 169 (offset 1 line).
       > applying patch /nix/store/2zi4jkf6apny80mjvsnysf9y39hkvcjh-67a71e3b71a2834d028031a92e76eb9444e423c6.patch
       > patching file hw/9pfs/9p-util.h
       > patching file hw/9pfs/9p.c
       > Hunk #1 FAILED at 138.
       > Hunk #2 succeeded at 170 (offset -1 lines).
       > 1 out of 2 hunks FAILED -- saving rejects to file hw/9pfs/9p.c.rej
       For full logs, run 'nix log /nix/store/prrnyklwp6cbymighad5d8w6fk79q0qi-qemu-6.2.0.drv'.

Is that patch going to keep being needed? If so, it's going to break with 7.0 too based on 9p-darwin. If so, I can also update it to apply AFTER these ones, so you'll be able to retain it with the version bump.

@alyssais
Copy link
Member

alyssais commented Mar 9, 2022

Is that patch going to keep being needed?

Let me have a look into it. I'll try to get you an answer tonight.

@willcohen
Copy link
Contributor Author

More specifically, this is why it conflicts: https://gitlab.com/qemu-project/qemu/-/commit/67a71e3b71a2834d028031a92e76eb9444e423c6

9p-darwin patch set moves the deleted code to a new location, so the patch will need to take that into account if it's still needed on Linux. I don't know enough about what it's doing to assess whether it's still important, though, but if so, I'm happy to modify the file for this PR.

@willcohen
Copy link
Contributor Author

Upon a non-exhaustive search, I'm not currently seeing that that patch ever got finalized into the kernel, though it's extremely possible that I missed it. That said, as I'm not familiar with the NixOS tests in question, I can't tell if it's still applicable anymore either.

@willcohen
Copy link
Contributor Author

@alyssais I can't figure out why the patch WOULDN'T still be needed, so I went ahead and updated it to work on a post 9p-darwin setup, and removed the conditional application of the patches

@willcohen
Copy link
Contributor Author

Result of nixpkgs-review pr 162243 run on x86_64-darwin 1

1 package marked as broken and skipped:
  • alpine-make-vm-image
1 package failed to build:
  • python310Packages.cot
8 packages built:
  • colima
  • cot (python39Packages.cot)
  • lima
  • out-of-tree
  • qemu
  • qemu-utils
  • qemu_kvm
  • qemu_test

@alyssais
Copy link
Member

My initial testing implies that the patch isn't needed any more, but I need to investigate to be sure. But since you've done the work anyway to rebase it, there's no reason to make you wait while I investigate. Thanks so much!

@willcohen
Copy link
Contributor Author

Thanks!

@willcohen willcohen deleted the qemu-9p-darwin branch March 10, 2022 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants