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: add 9p virtfs support on Darwin #122420

Closed
wants to merge 1 commit into from
Closed

Conversation

mroi
Copy link
Contributor

@mroi mroi commented May 10, 2021

Enable 9p for QEMU’s guest-host file sharing feature on Darwin. With stock QEMU, the 9p functionality is only available on Linux hosts. A third-party patch set was originally written by Keno Fischer to bring this feature to Darwin hosts as well. I adapted this patch set to work with current QEMU 6.0.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 10, 2021
@mroi
Copy link
Contributor Author

mroi commented May 10, 2021

This PR can help #108984.

@mroi
Copy link
Contributor Author

mroi commented May 10, 2021

Note that there is currently an unrelated problem with code signing (#121903) preventing QEMU from building as-is.

@domenkozar
Copy link
Member

@GrahamcOfBorg build qemu

@alyssais
Copy link
Member

Would you consider submitting this patch upstream? It looks like the original developer hasn't pursued it for a couple of years.

@alyssais
Copy link
Member

(I'm a bit nervous about adding eleven third-party patches to QEMU to support this -- it's quite a lot of changes)

@mroi
Copy link
Contributor Author

mroi commented May 17, 2021

I have sent an email to the original author of the patchset in case he still has plans with this. If I don’t hear back within a couple of days, I will try upstreaming it myself.

I fully agree that this should not live in Nixpkgs, but upstream.

@AkihiroSuda
Copy link

AkihiroSuda commented May 22, 2021

It seems that ls /mnt/foo hangs up when the fs was mounted as version=9p2000.L.

  • QEMU arg: -virtfs local,path=/tmp/foo,mount_tag=foo,security_model=mapped-xattr
  • Guest command: mount -t 9p -o trans=virtio,version=9p2000.L foo /mnt/foo
  • Guest OS: Ubuntu 21.04 (x86_64)

version=9p2000.u seemed working fine for small directories, but still seems to have several issues:

  • The :w command of vim hangs up. The C-x C-s command of mg does not hang up, though.
  • When the entire home directory is mounted, ls /Users/foo fails with ls: reading directory '/Users/foo': Timer expired. A workaround seems to be security_model=none.

@tarik02
Copy link

tarik02 commented May 22, 2021

@AkihiroSuda you should try to replace v9fs_co_telldir(pdu, fidp) calls to telldir(fidp->fs.dir.stream). One match in 9p.c and one in codir.c.

@AkihiroSuda
Copy link

AkihiroSuda commented May 22, 2021

@tarik02 Thanks, this seems to fix 9p2000.L 👍
@mroi: Could you include Tarik02's suggestion in the patch set?

ls -l shows some warnings, but probably negligible.

root@lima-default:/tmp/lima# touch foo
root@lima-default:/tmp/lima# ls -l foo
ls: foo: Network dropped connection on reset
-rw-r--r-- 1 root root 0 May 22 08:43 foo

strace: lgetxattr("foo", "security.selinux", 0x5592204c68d0, 255) = -1 ENETRESET (Network dropped connection on reset)

@tarik02
Copy link

tarik02 commented May 22, 2021

@AkihiroSuda Also, I have proof of concept port of virtfs-proxy-helper (it runs in a separate process instead of QEMU). The difference between using virtfs-proxy-helper and just QEMU is that all file operations are running in a separate process which is running by root. This allows doing better with file permissions and things like that.

I will prepare that patch set later, maybe it will be useful.

And there's one more thing I want to about. I think we need to set up UID and GID mapping between host and guest systems, so files will have right owner on host and guest. This is much easier to do in the helper.

@mroi
Copy link
Contributor Author

mroi commented May 22, 2021

@AkihiroSuda you should try to replace v9fs_co_telldir(pdu, fidp) calls to telldir(fidp->fs.dir.stream). One match in 9p.c and one in codir.c.

@tarik02 Is that change needed universally? I thought it’s for Apple Silicon?

@tarik02
Copy link

tarik02 commented May 22, 2021

@AkihiroSuda you should try to replace v9fs_co_telldir(pdu, fidp) calls to telldir(fidp->fs.dir.stream). One match in 9p.c and one in codir.c.

@tarik02 Is that change needed universally? I thought it’s for Apple Silicon?

Universally.

@mroi mroi force-pushed the qemu-darwin-9p branch from a180e45 to 4164883 Compare May 22, 2021 15:23
@mroi
Copy link
Contributor Author

mroi commented May 22, 2021

@tarik02 Thanks for the hint, I updated my branch.

@tarik02
Copy link

tarik02 commented May 23, 2021

@AkihiroSuda Here is virtfs-proxy-helper for Darwin: tarik02/qemu@d0bcdb0

To be honest, it was not enough tested and it has lesser security restrictions than the one on Linux. But for using projects like Lima, this should be enough.

@AkihiroSuda
Copy link

@tarik02 Thanks, could you submit the patch to the upstream? 🙏

@tarik02
Copy link

tarik02 commented May 24, 2021

@AkihiroSuda no, since this patch depends on this pull request. It can't be merged to upstream without merging this 9p virtues support for Darwin.

@AkihiroSuda
Copy link

@mroi
Do you plan to submit the patches to the upstream soon?

@mroi
Copy link
Contributor Author

mroi commented Jun 3, 2021

I did not manage to send the patchset upstream yet. Sorry about that. I am currently “enjoying” an unplanned hospital visit, so if anyone else wants to give this a shot, please go ahead. The original author fully supports us sending this upstream.

@AkihiroSuda
Copy link

Ah, sorry for interrupting, and hope you'll get soon 🙏

@LionsAd
Copy link

LionsAd commented Feb 2, 2022

entry->d_seekoff = telldir(fs->dir.stream);

I bet it’s this line. I think you need to call the special Synth_telldir function instead.

there is a special one IIRC.

@willcohen
Copy link
Contributor

willcohen commented Feb 2, 2022

I tried that too and it doesn't seem to be it -- i've got here as

entry->d_seekoff = synth_telldir(ctx, fs);

I actually also went ahead and changed to local_telldir and proxy_telldir too in the other situations, and still no difference.

Update: changing the line to the following causes it to at least pass one more test:

    if (entry) {
        entry->d_seekoff = synth_telldir(ctx, fs);
    }

...

...
# Start of readdir tests
ok 52 /x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-9p-pci/virtio-9p/virtio-9p-tests/synth/readdir/basic
qemu-system-x86_64: Failed to decode VirtFS request type 40
**
ERROR:../tests/qtest/libqos/virtio.c:228:qvirtio_wait_used_elem: assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)
Bail out! ERROR:../tests/qtest/libqos/virtio.c:228:qvirtio_wait_used_elem: assertion failed: (g_get_monotonic_time() - start_time <= timeout_us)

As additional context (not sure how this plays in here yet): https://patchwork.kernel.org/project/qemu-devel/patch/4dc3706db1f033d922e54af8c74a81211de8b79f.1579567020.git.qemu_oss@crudebyte.com/

@LionsAd
Copy link

LionsAd commented Feb 6, 2022

Nice - idea -- I don't know what is wrong with your patch, but with this patch all tests continue to pass:

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 3a8bdf05d5..5e78557f92 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -563,6 +563,10 @@ again:
         return NULL;
     }
 
+#ifdef DARWIN
+    entry->d_seekoff = telldir(fs->dir.stream);
+#endif
+
     if (ctx->export_flags & V9FS_SM_MAPPED) {
         entry->d_type = DT_UNKNOWN;
     } else if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 09b9c25288..e264a03eef 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -222,7 +222,9 @@ static void synth_direntry(V9fsSynthNode *node,
 {
     strcpy(entry->d_name, node->name);
     entry->d_ino = node->attr->inode;
-#ifndef CONFIG_DARWIN
+#ifdef CONFIG_DARWIN
+    entry->d_seekoff = off + 1;
+#else
     entry->d_off = off + 1;
 #endif
 }
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 70b1d7431e..604288d416 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2233,8 +2233,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
  * Get the seek offset of a dirent. If not available from the structure itself,
  * obtain it by calling telldir.
  */
-static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
-                             struct dirent *dent)
+static int v9fs_dent_telldir(struct dirent *dent)
 {
 #ifdef CONFIG_DARWIN
     /*
@@ -2242,7 +2241,7 @@ static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
      * However, it does not appear to be supported on all file systems,
      * so use telldir for correctness.
      */
-    return v9fs_co_telldir(pdu, fidp);
+    return dent->d_seekoff;
 #else
     return dent->d_off;
 #endif
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index c1b5694f3f..b7f9ed1223 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -168,7 +168,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
 
         size += len;
 #ifdef CONFIG_DARWIN
-        saved_dir_pos = s->ops->telldir(&s->ctx, &fidp->fs);
+        saved_dir_pos = dent->d_seekoff;
 #else
         saved_dir_pos = dent->d_off;
 #endif

If you need the proxy changes as well:

diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index f5aade21b5..8e7804abd6 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -700,7 +700,17 @@ static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 
 static struct dirent *proxy_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return readdir(fs->dir.stream);
+    struct dirent* dentry;
+
+    dentry = readdir(fs->dir.stream);
+
+#ifdef DARWIN
+    if (dentry) {
+        dentry->d_seekoff = telldir(fs->dir.stream);
+    }
+#endif
+
+    return dentry;
 }
 
 static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)

Signed-Off-By: Fabian Franz (email follows)

@LionsAd
Copy link

LionsAd commented Feb 6, 2022

An open question is what we do with the error handling:

https://opensource.apple.com/source/Libc/Libc-167/gen.subproj/telldir.c.auto.html

can fail if malloc() fails.

    1. Is the whole dentry then invalid and we return -1 for readdir() itself?
    1. Do we keep the error checking code when using a dentry->d_seekoff that we already have in place?
  • If we do 2), do we need to add a check to codir.c as well?

Besides that we need to update the comments. Let me know if tests pass for you as well @willcohen.

Thanks,

Fabian

@LionsAd
Copy link

LionsAd commented Feb 6, 2022

I decided to implement the error handling:

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 3a8bdf05d5..4057c8248e 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -563,6 +563,17 @@ again:
         return NULL;
     }
 
+#ifdef DARWIN
+    /* Darwin does not populate d_seekoff, so do it here */
+    entry->d_seekoff = telldir(fs->dir.stream);
+
+    /* It is highly unlikely for this to fail, but fail the
+       whole readdir() call in this case. */
+    if (entry->d_seekoff < 0) {
+        return NULL;
+    }
+#endif
+
     if (ctx->export_flags & V9FS_SM_MAPPED) {
         entry->d_type = DT_UNKNOWN;
     } else if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index f5aade21b5..6a75624e93 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -700,7 +700,26 @@ static off_t proxy_telldir(FsContext *ctx, V9fsFidOpenState *fs)
 
 static struct dirent *proxy_readdir(FsContext *ctx, V9fsFidOpenState *fs)
 {
-    return readdir(fs->dir.stream);
+    struct dirent *entry;
+
+    entry = readdir(fs->dir.stream);
+
+#ifdef DARWIN
+    if (!entry) {
+        return NULL;
+    }
+
+    /* Darwin does not populate d_seekoff, so do it here */
+    entry->d_seekoff = telldir(fs->dir.stream);
+
+    /* It is highly unlikely for this to fail, but fail the
+       whole readdir() call in this case. */
+    if (entry->d_seekoff < 0) {
+        return NULL;
+    }
+#endif
+
+    return entry;
 }
 
 static void proxy_seekdir(FsContext *ctx, V9fsFidOpenState *fs, off_t off)
diff --git a/hw/9pfs/9p-synth.c b/hw/9pfs/9p-synth.c
index 09b9c25288..e264a03eef 100644
--- a/hw/9pfs/9p-synth.c
+++ b/hw/9pfs/9p-synth.c
@@ -222,7 +222,9 @@ static void synth_direntry(V9fsSynthNode *node,
 {
     strcpy(entry->d_name, node->name);
     entry->d_ino = node->attr->inode;
-#ifndef CONFIG_DARWIN
+#ifdef CONFIG_DARWIN
+    entry->d_seekoff = off + 1;
+#else
     entry->d_off = off + 1;
 #endif
 }
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 70b1d7431e..a6bfeb0e92 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -2229,25 +2229,6 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     return offset;
 }
 
-/**
- * Get the seek offset of a dirent. If not available from the structure itself,
- * obtain it by calling telldir.
- */
-static int v9fs_dent_telldir(V9fsPDU *pdu, V9fsFidState *fidp,
-                             struct dirent *dent)
-{
-#ifdef CONFIG_DARWIN
-    /*
-     * Darwin has d_seekoff, which appears to function similarly to d_off.
-     * However, it does not appear to be supported on all file systems,
-     * so use telldir for correctness.
-     */
-    return v9fs_co_telldir(pdu, fidp);
-#else
-    return dent->d_off;
-#endif
-}
-
 static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
                                                   V9fsFidState *fidp,
                                                   uint32_t max_count)
@@ -2311,11 +2292,11 @@ static int coroutine_fn v9fs_do_readdir_with_stat(V9fsPDU *pdu,
         count += len;
         v9fs_stat_free(&v9stat);
         v9fs_path_free(&path);
-        saved_dir_pos = v9fs_dent_telldir(pdu, fidp, dent);
-        if (saved_dir_pos < 0) {
-            err = saved_dir_pos;
-            break;
-        }
+#ifdef CONFIG_DARWIN
+        saved_dir_pos = dent->d_seekoff;
+#else
+        saved_dir_pos = dent->d_off;
+#endif
     }
 
     v9fs_readdir_unlock(&fidp->fs.dir);
@@ -2515,11 +2496,11 @@ static int coroutine_fn v9fs_do_readdir(V9fsPDU *pdu, V9fsFidState *fidp,
             qid.version = 0;
         }
 
-        off = v9fs_dent_telldir(pdu, fidp, dent);
-        if (off < 0) {
-            err = off;
-            break;
-        }
+#ifdef CONFIG_DARWIN
+        off = dent->d_seekoff;
+#else
+        off = dent->d_off;
+#endif
         v9fs_string_init(&name);
         v9fs_string_sprintf(&name, "%s", dent->d_name);
 
diff --git a/hw/9pfs/codir.c b/hw/9pfs/codir.c
index c1b5694f3f..b7f9ed1223 100644
--- a/hw/9pfs/codir.c
+++ b/hw/9pfs/codir.c
@@ -168,7 +168,7 @@ static int do_readdir_many(V9fsPDU *pdu, V9fsFidState *fidp,
 
         size += len;
 #ifdef CONFIG_DARWIN
-        saved_dir_pos = s->ops->telldir(&s->ctx, &fidp->fs);
+        saved_dir_pos = dent->d_seekoff;
 #else
         saved_dir_pos = dent->d_off;
 #endif

Signed-off-by: Fabian Franz <[email protected]>

I am still torn on the proxy d_seekoff changes, but not doing them now might make it really difficult for someone later to do them.

@LionsAd
Copy link

LionsAd commented Feb 6, 2022

@willcohen Patch updated. Tests pass, email added, we should be good to go!

@willcohen
Copy link
Contributor

Will incorporate on Mon or Tues! Many thanks!

@willcohen
Copy link
Contributor

Minor adjustment due to:

../hw/9pfs/9p-proxy.c:717:26: warning: result of comparison of unsigned expression < 0 is always false [-Wtautological-unsigned-zero-compare]
    if (entry->d_seekoff < 0) {
        ~~~~~~~~~~~~~~~~ ^ ~
1 warning generated.

Since d_seekoff is unsigned, I'm putting the telldir in its own int, which can then be compared to see if it's less than zero.

Otherwise, I integrated the changes between your patch and mine to get it to pass, fixed the remaining _sec and _nsec issue that was outstanding, and pushed the latest branch. Tests pass and I'll submit as v4 later today!

@willcohen
Copy link
Contributor

As a quick update here, v4 (https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01223.html) is getting a pretty healthy bit of productive discussion in qemu-devel. There's already some fairly small changes that I know I'll need to roll into a v5, but at the moment they're all minor/housekeeping. Unless anything more serious in terms of functionality comes up during whatever is left in v4, I have a good feeling that we're approaching getting this wrapped up in the near future!

@LionsAd
Copy link

LionsAd commented Feb 10, 2022

Thanks for the on-going back and forth, Will.

i totally misunderstood that they still wanted to use XATTR_SIZE instead of an arbitrary limit.

We are at v6 and so far no Feedback:

https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg02313.html

Which means we are getting really really close to commit now 😊😁👍.

@willcohen
Copy link
Contributor

Just keeping everyone in the loop, looks like we're looking at an April-ish release for 7.0 (https://wiki.qemu.org/Planning/7.0#Release_Schedule). Nothing else to do on this end, but as long as this patch set finishes review by early-ish March, we should be meeting that milestone.

@LionsAd
Copy link

LionsAd commented Feb 17, 2022

@willcohen Maintainer is optimistic for 7.0, but there has been one more request for changes:

https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg03112.html

@willcohen
Copy link
Contributor

Yep! v7 creates the meson check: https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg03347.html

@willcohen
Copy link
Contributor

Since this is closing in on being finalized (https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg04294.html -- it's not literally accepted for merging, but I think we're close), @domenkozar and @alyssais would you all like me to prep a PR based on a 6.2.0 backport branch? Might be good to shake out some issues with it now so that when the release process gets moving for 7.0, at least this will generally be resolved and this won't be an added complication.

@willcohen
Copy link
Contributor

See #162243 for a PR.

Per https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg06130.html, pending no last minute issues, 9p maintainer plans to queue this patchset for merging in the next day. With that, I'd consider v9 (https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg05978.html) good enough for a backport merge. Any remaining changes should be trivial.

@LionsAd
Copy link

LionsAd commented Mar 1, 2022

@willcohen Great work!

@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

v9 is now in the 9p maintainer tree for eventual merge (https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg00232.html). It's not integrated just yet, but I think we can call this feature push complete for 7.0. Many many thanks to all involved.

@dnrce
Copy link

dnrce commented Mar 1, 2022

Bravo! Looking forward to this.

@AkihiroSuda
Copy link

Now merged in the master 🎉

https://github.com/qemu/qemu/tree/f45cc81911adc7726e8a2801986b6998b91b816e
https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg02594.html

@willcohen
Copy link
Contributor

willcohen commented Mar 8, 2022

9p-darwin has been merged upstream. #162243 now pulls from upstream for all commits that apply cleanly, and has a few fixed patch files for the commits that need modification to apply to 6.2.

I don't plan on prepping the PR for homebrew, but whoever would like to should feel free to use the referenced commits and adjusted patch files from #162243 to have a working backport that doesn't rely on a third party fork!

Looks like the podman team is on it here: Homebrew/homebrew-core#95318

@willcohen
Copy link
Contributor

I'm also going to close this PR in favor of #162243, at this point. I think it's okay if any additional final discussion still happens here since this particular issue has been referenced in so many places, but the work is now done!

@willcohen willcohen closed this Mar 8, 2022
@mroi mroi deleted the qemu-darwin-9p branch March 10, 2022 06:24
@afbjorklund
Copy link

Looks like the podman team is on it here: Homebrew/homebrew-core#95318

The above PR was for podman, the PR for qemu is: Homebrew/homebrew-core#96655

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 11-100 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.