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

[Alyssa Ross <hi@alyssa.is>]

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