[PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

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

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 3708 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]

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/+/b3f46c8fcbb0ce1047c4a5ca2c26c66e29e8eb44

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 4101 bytes --]

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/+/b3f46c8fcbb0ce1047c4a5ca2c26c66e29e8eb44


Best wishes,
--
Dom Rodriguez

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

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. :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom RODRIGUEZ]

[-- Attachment #1: Type: text/plain, Size: 4069 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 4496 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 4762 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 5409 bytes --]

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.)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]