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

[Dom Rodriguez <shymega@shymega.org.uk>]

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