[PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`. In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket. Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \ diff --git a/release/checks/wayland/default.nix b/release/checks/wayland/default.nix index d05aa88..a36dbab 100644 --- a/release/checks/wayland/default.nix +++ b/release/checks/wayland/default.nix @@ -29,7 +29,7 @@ nixosTest ({ lib, pkgs, ... }: { systemd.services.crosvm = { after = [ "crosvm-gpu.service" "weston.service" ]; requires = [ "crosvm-gpu.service" "weston.service" ]; - serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user-gpu /run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; + serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user gpu,socket=/run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; serviceConfig.ExecStop = "${lib.getExe pkgs.crosvm} stop /run/crosvm"; }; -- 2.44.1
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid. When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error: [2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0 Probably the tag should also be a comma-separated key=value option? (This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
diff --git a/release/checks/wayland/default.nix b/release/checks/wayland/default.nix index d05aa88..a36dbab 100644 --- a/release/checks/wayland/default.nix +++ b/release/checks/wayland/default.nix @@ -29,7 +29,7 @@ nixosTest ({ lib, pkgs, ... }: { systemd.services.crosvm = { after = [ "crosvm-gpu.service" "weston.service" ]; requires = [ "crosvm-gpu.service" "weston.service" ]; - serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user-gpu /run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; + serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user gpu,socket=/run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; serviceConfig.ExecStop = "${lib.getExe pkgs.crosvm} stop /run/crosvm"; };
This part works now though, thanks!
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option. I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now. Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
diff --git a/release/checks/wayland/default.nix b/release/checks/wayland/default.nix index d05aa88..a36dbab 100644 --- a/release/checks/wayland/default.nix +++ b/release/checks/wayland/default.nix @@ -29,7 +29,7 @@ nixosTest ({ lib, pkgs, ... }: { systemd.services.crosvm = { after = [ "crosvm-gpu.service" "weston.service" ]; requires = [ "crosvm-gpu.service" "weston.service" ]; - serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user-gpu /run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; + serviceConfig.ExecStart = "${lib.getExe pkgs.crosvm} run -s /run/crosvm --disk ${appvm}/img/appvm/blk/root.img --disk ${run}/blk/run.img -p \"console=ttyS0 root=PARTLABEL=root\" --vhost-user gpu,socket=/run/crosvm-gpu.sock --vsock cid=3 --serial type=stdout,hardware=virtio-console,stdin=true ${appvm}/img/appvm/vmlinux"; serviceConfig.ExecStop = "${lib.getExe pkgs.crosvm} stop /run/crosvm"; };
This part works now though, thanks!
Awesome. I'll leave that as-is for the next patch revision. Best wishes, -- Dom Rodriguez GPG Fingerprint: EB0D 45E6 D0DC 1BA1 A2B5 FC24 72DC F123 1E54 BD43
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option.
I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now.
Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out. In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag. If that's too much work, I'd also be happy to apply just the GPU part of this patch. Just let me know. :) [1]: https://chromium.googlesource.com/crosvm/crosvm/+/b3f46c8fcbb0ce1047c4a5ca2c...
Hey, Sorry for the super late reply. I was flying back home on the 28th, and then I had to prepare for a new job - room organising, laptop configuring, onboarding, etc :-/ On 28.09.2024 16:27, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option.
I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now.
Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Oh, that explains *a lot*! I agree, a generic approach would be preferable.
If that's too much work, I'd also be happy to apply just the GPU part of this patch. Just let me know. :)
I've started a new job, and so I've only really got weekends free - and even then, I have Repair Cafes to volunteer at. If I send over the GPU patch only, can you apply that, and then we can discuss on Matrix/IRC the best approach for `vhost-user-fs`? Feel free to tag me - I'm mostly using IRC rightn ow. Thanks.
[1]: https://chromium.googlesource.com/crosvm/crosvm/+/b3f46c8fcbb0ce1047c4a5ca2c...
Best wishes, -- Dom Rodriguez
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 28.09.2024 16:27, Alyssa Ross wrote:
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Oh, that explains *a lot*! I agree, a generic approach would be preferable.
If that's too much work, I'd also be happy to apply just the GPU part of this patch. Just let me know. :)
I've started a new job, and so I've only really got weekends free - and even then, I have Repair Cafes to volunteer at.
If I send over the GPU patch only, can you apply that, and then we can discuss on Matrix/IRC the best approach for `vhost-user-fs`? Feel free to tag me - I'm mostly using IRC rightn ow.
SGTM.
Alyssa Ross <hi@alyssa.is> writes:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 28.09.2024 16:27, Alyssa Ross wrote:
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Oh, that explains *a lot*! I agree, a generic approach would be preferable.
If that's too much work, I'd also be happy to apply just the GPU part of this patch. Just let me know. :)
I've started a new job, and so I've only really got weekends free - and even then, I have Repair Cafes to volunteer at.
If I send over the GPU patch only, can you apply that, and then we can discuss on Matrix/IRC the best approach for `vhost-user-fs`? Feel free to tag me - I'm mostly using IRC rightn ow.
SGTM.
BTW: I've just seen this when building release/checks/wayland after a recent Nixpkgs update: machine # [ 18.830327] crosvm[987]: [src/crosvm/cmdline.rs:2829] Deprecated disk flags such as --[rw]disk or --[rw]root are passed. Use --block instead. Just in case you're interested. :)
On 28.09.2024 16:27, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option.
I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now.
Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Finally got round to this. By the looks of it, currently `cloud-hypervisor` and QEMU do not support a backend-provided tag. We could introduce an abstraction [function] in the Makefile over `vhost-user-fs` arguments for each hypervisor, which we could use whilst waiting for QEMU and cloud-hypervisor to support backend-provided tags with `vhost-user-fs`, and in the meantime use the abstraction for crosvm. I envision the abstraction as taking a few arguments: hypervisor, virtiofsd socket path, and tag. The abstraction would then construct & return the correct command-line argument for the hypervisor. We would call the abstractive function in the call to the `run` Make target. That way, we're prepared for the future. What do you think? Best wishes, -- Dom Rodriguez
Dom RODRIGUEZ <dominic.rodriguez@rodriguez.org.uk> writes:
On 28.09.2024 16:27, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option.
I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now.
Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Finally got round to this.
By the looks of it, currently `cloud-hypervisor` and QEMU do not support a backend-provided tag.
We could introduce an abstraction [function] in the Makefile over `vhost-user-fs` arguments for each hypervisor, which we could use whilst waiting for QEMU and cloud-hypervisor to support backend-provided tags with `vhost-user-fs`, and in the meantime use the abstraction for crosvm.
I envision the abstraction as taking a few arguments: hypervisor, virtiofsd socket path, and tag. The abstraction would then construct & return the correct command-line argument for the hypervisor. We would call the abstractive function in the call to the `run` Make target.
That way, we're prepared for the future.
What do you think?
Given that they don't support the optional protocol feature, I'd expect cloud-hypervisor and QEMU to just ignore the tag given to virtiofsd — can we not just set the tag on the virtiofsd invocation for crosvm, and keep setting it in the cloud-hypervisor and QEMU command lines for them?
On 09.11.2024 22:46, Alyssa Ross wrote:
Dom RODRIGUEZ <dominic.rodriguez@rodriguez.org.uk> writes:
On 28.09.2024 16:27, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
crosvm was producing warnings when using `--vhost-user-gpu` and `--vhost-user-fs`.
In this commit, I have adjusted the `crosvm` invocations to look something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, `gpu` or `fs`, and `$PATH` is the path to the Unix socket.
Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> --- img/app/Makefile | 4 ++-- release/checks/wayland/default.nix | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/img/app/Makefile b/img/app/Makefile index 3adf8c4..11ef6e1 100644 --- a/img/app/Makefile +++ b/img/app/Makefile @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd --disk $(RUN_IMG) \ -p "console=ttyS0 root=PARTLABEL=root" \ --net tap-name=tap0 \ - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ - --vhost-user-gpu build/vhost-user-gpu.sock \ + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ --vsock cid=3 \ --serial type=file,hardware=serial,path=build/serial.log \ --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option.
I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now.
Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Finally got round to this.
By the looks of it, currently `cloud-hypervisor` and QEMU do not support a backend-provided tag.
We could introduce an abstraction [function] in the Makefile over `vhost-user-fs` arguments for each hypervisor, which we could use whilst waiting for QEMU and cloud-hypervisor to support backend-provided tags with `vhost-user-fs`, and in the meantime use the abstraction for crosvm.
I envision the abstraction as taking a few arguments: hypervisor, virtiofsd socket path, and tag. The abstraction would then construct & return the correct command-line argument for the hypervisor. We would call the abstractive function in the call to the `run` Make target.
That way, we're prepared for the future.
What do you think?
Given that they don't support the optional protocol feature, I'd expect cloud-hypervisor and QEMU to just ignore the tag given to virtiofsd — can we not just set the tag on the virtiofsd invocation for crosvm, and keep setting it in the cloud-hypervisor and QEMU command lines for them?
That's a fair conclusion. Are you happy for me to submit a patch just for crosvm for now? Best wishes, -- Dom Rodriguez
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 09.11.2024 22:46, Alyssa Ross wrote:
Dom RODRIGUEZ <dominic.rodriguez@rodriguez.org.uk> writes:
On 28.09.2024 16:27, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
On 07.09.2024 18:40, Alyssa Ross wrote:
Dom Rodriguez <shymega@shymega.org.uk> writes:
> crosvm was producing warnings when using `--vhost-user-gpu` and > `--vhost-user-fs`. > > In this commit, I have adjusted the `crosvm` invocations to look > something like `--vhost-user $DEVICE,socket=$PATH`, where `$DEVICE` is, in this case, > `gpu` or `fs`, and `$PATH` is the path to the Unix socket. > > Signed-off-by: Dom Rodriguez <shymega@shymega.org.uk> > --- > img/app/Makefile | 4 ++-- > release/checks/wayland/default.nix | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/img/app/Makefile b/img/app/Makefile > index 3adf8c4..11ef6e1 100644 > --- a/img/app/Makefile > +++ b/img/app/Makefile > @@ -152,8 +152,8 @@ run-crosvm: $(imgdir)/appvm/blk/root.img start-vhost-user-gpu start-virtiofsd > --disk $(RUN_IMG) \ > -p "console=ttyS0 root=PARTLABEL=root" \ > --net tap-name=tap0 \ > - --vhost-user-fs build/virtiofsd.sock:virtiofs0 \ > - --vhost-user-gpu build/vhost-user-gpu.sock \ > + --vhost-user fs,socket=build/virtiofsd.sock:virtiofs0 \ > + --vhost-user gpu,socket=build/vhost-user-gpu.sock \ > --vsock cid=3 \ > --serial type=file,hardware=serial,path=build/serial.log \ > --serial type=stdout,hardware=virtio-console,stdin=true \
This is still not right, I'm afraid.
When I run nix-shell --run 'make clean && make run' VMM=crosvm in img/app, I get this error:
[2024-09-07T16:36:32.592274891+00:00 ERROR crosvm] exiting with error 1: failed to connect to vhost-user socket path build/virtiofsd.sock:virtiofs0
Probably the tag should also be a comma-separated key=value option?
(This doesn't have any automated test, because it's just part of the development environment, and setting up a test environment to resemble a development machine isn't trivial. Should be possible though.)
It's bizarre. I've done some more testing, and it seems that `--vhost-user` isn't *quite* there yet on feature parity with `--vhost-user-fs`. Maybe I'm going wrong here, but it doesn't recognise the tag as a k/v option.
I have managed to get the tests running with `--vhost-user-fs`, but it does look mismatched now.
Would you prefer I revert the changes to `--vhost-user fs` => `--vhost-user-fs`?
You're right! I've just spent the last little while looking around trying to figure out what's up here, and I think I've figured it out.
In this commit[1] they made the vhost-user-fs tag optional, because it can now be set with virtiosfd instead of the VMM. This is probably good, because the more generic vhost-user becomes, the more hope Spectrum has of one day not needing Cloud Hypervisor patches. :) So we could try using --vhost-user fs for crosvm, and then passing --tag to virtiofsd, but it'd need to be tested with VMM=qemu and VMM=cloud-hypervisor as well, because I'm not sure whether they support a backend-provided tag.
Finally got round to this.
By the looks of it, currently `cloud-hypervisor` and QEMU do not support a backend-provided tag.
We could introduce an abstraction [function] in the Makefile over `vhost-user-fs` arguments for each hypervisor, which we could use whilst waiting for QEMU and cloud-hypervisor to support backend-provided tags with `vhost-user-fs`, and in the meantime use the abstraction for crosvm.
I envision the abstraction as taking a few arguments: hypervisor, virtiofsd socket path, and tag. The abstraction would then construct & return the correct command-line argument for the hypervisor. We would call the abstractive function in the call to the `run` Make target.
That way, we're prepared for the future.
What do you think?
Given that they don't support the optional protocol feature, I'd expect cloud-hypervisor and QEMU to just ignore the tag given to virtiofsd — can we not just set the tag on the virtiofsd invocation for crosvm, and keep setting it in the cloud-hypervisor and QEMU command lines for them?
That's a fair conclusion. Are you happy for me to submit a patch just for crosvm for now?
Yeah. Add the tag to the virtiofsd invocation, update the crosvm command line, then check that "make run" in a nix-shell still works with each of VMM=cloud-hypervisor, VMM=qemu, and VMM=crosvm, and let me know if you need help with anything. :) (I think QEMU might not be able to open application windows at the moment for upstream reasons, so if that's the case rather than checking an application window appears, you could instead just check that /run/virtiofs/virtiofs0 is non-empty in the QEMU serial console.)
participants (3)
-
Alyssa Ross -
Dom Rodriguez -
Dom RODRIGUEZ