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

[Will Deacon <will@kernel.org>]

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".

> 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.

Will