Re: IOTLB support for vhost/vsock breaks crosvm on Android

[Stefano Garzarella]

On Sat, Aug 06, 2022 at 10:42:40AM +0100, Will Deacon wrote:
> Hi Stefano,
>
> On Sat, Aug 06, 2022 at 09:48:28AM +0200, Stefano Garzarella wrote:
> > On Fri, Aug 05, 2022 at 07:11:06PM +0100, Will Deacon wrote:
> > > The fundamental issue is, I think, that VIRTIO_F_ACCESS_PLATFORM is
> > > being used for two very different things within the same device; for the
> > > guest it basically means "use the DMA API, it knows what to do" but for
> > > vhost it very specifically means "enable IOTLB". We've recently had
> > > other problems with this flag [3] but in this case it used to work
> > > reliably and now it doesn't anymore.
> > >
> > > So how should we fix this? One possibility is for us to hack crosvm to
> > > clear the VIRTIO_F_ACCESS_PLATFORM flag when setting the vhost
> >
> > Why do you consider this a hack?
>
> I think it's a hack for two reasons:
>
>  (1) We're changing userspace to avoid a breaking change in kernel behaviour
>  (2) I think that crosvm's approach is actually pretty reasonable
>
> To elaborate on (2), crosvm has a set of device features that it has
> negotiated with the guest. It then takes the intersection of these features
> with those advertised by VHOST_GET_FEATURES and calls VHOST_SET_FEATURES
> with the result. If there was a common interpretation of what these features
> do, then this would work and would mean we wouldn't have to opt-in on a
> per-flag basis for vhost. Since VIRTIO_F_ACCESS_PLATFORM is being overloaded
> to mean two completely different things, then it breaks and I think masking
> out that specific flag is a hack because it's basically crosvm saying "yeah,
> I may have negotiated this with the driver but vhost _actually_ means
> 'IOTLB' when it says it supports this flag so I'll mask it out because I
> know better".

Thanks for elaborating, now I think I get your point!

If I understand you correctly, what you would like is that GET_FEATURES 
should return only the data path features (thus exposed to the guest) 
and not the features for the VMM, right?

In that case, since we also negotiate backend features (with
SET|GET_BACKEND_FEATURES ioctls) for IOTLB messages to work, maybe we 
could only expose that feature if VHOST_BACKEND_F_IOTLB_MSG_V2 has been 
negotiated

@Michael, @Jason, do you think this could be doable?

> > If the VMM implements the translation feature, it is right in my opinion
> > that it does not enable the feature for the vhost device. Otherwise, if it
> > wants the vhost device to do the translation, enable the feature and send
> > the IOTLB messages to set the translation.
> >
> > QEMU for example masks features when not required or supported.
> > crosvm should negotiate only the features it supports.
> >
> > @Michael and @Jason can correct me, but if a vhost device negotiates
> > VIRTIO_F_ACCESS_PLATFORM, then it expects the VMM to send IOTLB messages to
> > set the translation.
>
> As above, the issue is that vhost now unconditionally advertises this in
> VHOST_GET_FEATURES and so a VMM with no knowledge of IOTLB can end up
> enabling it by accident.

I honestly don't know what the initial design was, though, from what
I've seen in QEMU, it only enables the known features, in fact for
example when we added F_SEQPACKET for vhost-vsock, we had to update QEMU
to pass the feature to the guest, so I think the initial idea was to not
unconditionally accept all the features exposed by the vhost device.

Maybe this part should be clarified.

Thanks,
Stefano