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

[Alyssa Ross <hi@alyssa.is>]

[-- 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 --]