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

[Alyssa Ross <hi@alyssa.is>]

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