On 09.11.2024 22:46, Alyssa Ross wrote: >Dom RODRIGUEZ writes: > >> On 28.09.2024 16:27, Alyssa Ross wrote: >>>Dom Rodriguez writes: >>> >>>> On 07.09.2024 18:40, Alyssa Ross wrote: >>>>>Dom Rodriguez 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 >>>>>> --- >>>>>> 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