On 2/14/25 6:03 AM, Andrea Bolognani wrote:
On Thu, Feb 13, 2025 at 01:19:53PM -0500, Laine Stump wrote:The result is that you can now have: <interface type='vhostuser'> <backend type='passt'/> ... </interface> Then as long as you also have the following as a subelement of <domain>: <memoryBacking> <access mode='shared'/> </memoryBacking> your passt interfaces will benefit from the greatly improved efficiency of a vhost-user data pathThe presence of this element is not validated anywhere though, so if you leave it out the network interface will just silently be non-functional. I don't think that makes for a good user experience.
Keeping in mind that this is a pre-existing requirement for a pre-existing functionality that's been there since 2014...
My assumption has been that whatever type='vhostuser' already does is "good enough", and at some point during testing of uncompleted code, one of the unit tests produced an error saying something about "you've selected a feature that requires shared memory but don't have shared memory turned on; your configuration may not work correctly" or something like that.
Because I saw that during a failed unit test (which was later fixed) I figured that must be reported when a vhost-user config doesn't have shared memory enabled (and didn't think to actually try it), but just now I tried making a vhost-user config without shared memory enabled, and it didn't produce any error in libvirt at virsh edit time, nor did either QEMU or passt log any error (that I can see, althouygh maybe I'm not looking in all the right places), but it did produce a non-working interface for the guest!
I agree with you that it doesn't "make for a good user experience" and something should be done about it. I had considered having shared memory turned on automatically for type='vhostuser', but anything about "shared memory" sets security red flags ringing (or some other mixed metaphor) so I've assumed that's why they didn't already do this for traditional vhost-user interfaces. (I did ask about this online, but sbrivio was the only one who answered (saying a variation of the above)) (admittedly it was late in the day for me, so anybody in a further east timezone was probably already offline (except sbrivio, who seems to not sleep until after it is sunrise in Australia for some strange reason ;-)).
As a followup, I'd be happy to make a patch that causes a config with a vhost-user interface but no shared mem enabled to fail validation. That shouldn't hold up what's here though - as I said above, it's a preexisting condition (I don't know if you were actually suggesting that, but just want to "head it off at the pass" if you were :-))
(Now *there's* a nice American colloquialism (straight out of old western movies) - do people outside the U.S. every use it?)
How does passt know where the backing file is located in the first place? It's not passed on its command line, and I didn't spot any logic to hand the information over. This part clearly works, I just don't understand how :)
me too - double :-)
+++ b/src/conf/domain_validate.c @@ -2163,7 +2163,8 @@ virDomainNetDefValidate(const virDomainNetDef *net) return -1; } - if (net->type != VIR_DOMAIN_NET_TYPE_USER) { + if (net->type != VIR_DOMAIN_NET_TYPE_USER && + net->type != VIR_DOMAIN_NET_TYPE_VHOSTUSER) { if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("The 'passt' backend can only be used with interface type='user'"));This error message needs to be updated.
Oops, missed that one. I'll fix it before I push anything.
+++ b/tests/qemuxmlconfdata/net-vhostuser-passt.x86_64-latest.args @@ -0,0 +1,42 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \ +/usr/bin/qemu-system-x86_64 \ +-name guest=QEMUGuest1,debug-threads=on \ +-S \ +-object '{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}' \ +-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \ +-accel tcg \ +-cpu qemu64 \ +-m size=219136k \ +-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \ +-overcommit mem-lock=off \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-boot strict=on \ +-blockdev '{"driver":"host_device","filename":"/dev/HostVG/QEMUGuest1","node-name":"libvirt-1-storage","read-only":false}' \ +-device '{"driver":"ide-hd","bus":"ide.0","unit":0,"drive":"libvirt-1-storage","id":"ide0-0-0","bootindex":1}' \ +-chardev socket,id=charnet0,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net0.socket \ +-netdev '{"type":"vhost-user","chardev":"charnet0","id":"hostnet0"}' \ +-device '{"driver":"virtio-net-pci","netdev":"hostnet0","id":"net0","mac":"00:11:22:33:44:55","bus":"pci.0","addr":"0x2"}' \ +-chardev socket,id=charnet1,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net1.socket \ +-netdev '{"type":"vhost-user","chardev":"charnet1","id":"hostnet1"}' \ +-device '{"driver":"virtio-net-pci","netdev":"hostnet1","id":"net1","mac":"00:11:22:33:44:11","bus":"pci.0","addr":"0x3"}' \ +-chardev socket,id=charnet2,path=/var/run/libvirt/qemu/passt/-1-QEMUGuest1-net2.socket \ +-netdev '{"type":"vhost-user","chardev":"charnet2","id":"hostnet2"}' \ +-device '{"driver":"virtio-net-pci","netdev":"hostnet2","id":"net2","mac":"00:11:22:33:44:11","bus":"pci.0","addr":"0x4"}' \ +-audiodev '{"id":"audio1","driver":"none"}' \ +-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \ +-msg timestamp=onIt's a real shame that we don't have a way to include the command line for external programs (swtpm, passt) in the corresponding test case...
I was thinking that myself as I was writing this code! We really should be grabbing *all* external commands and putting them somewhere to compare (I guess they could all just go into the same file, and then the unit test would be able to verify the commandline was correct, and that the commands had all been executed in the proper order).
