Re: [PATCH 9/9] qemu: complete vhostuser + passt support

On Fri, Feb 14, 2025 at 08:49:29AM -0500, Laine Stump wrote: > 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 path >> >> The 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 :-)) I was pretty much requesting that change be made, actually :) I have no idea about how things work in the pre-existing vhost-user scenarios, but vhost-user passt is obviously broken when shared memory is disabled, so we should simply not allow it. I see no reason to introduce a new feature with known flaws, especially when adding a check for this should be completely trivial.

You can restrict the check to just the vhost-user passt case, leaving other vhost-user cases alone, so that we don't have to worry about accidentally breaking existing configurations.

Let's not even consider enabling shared memory automatically for now, because as you say that's potentially opening a big can of worm.

You need to respin to add the documentation anyway, don't you?