Virtualization - Aug 2022 - IOTLB support for vhost/vsock breaks crosvm on Android

Hi Will,

On Fri, Aug 05, 2022 at 07:11:06PM +0100, Will Deacon
wrote:
>Hi folks,
>
>[tl;dr a change from ~18 months ago breaks Android userspace and I don't
> know what to do about it]
>
>As part of the development work for next year's Android, we've
recently
>been bringing up a 5.15 KVM host and have observed that vsock no longer
>works for communicating with a guest because crosvm gets an unexpected
>-EFAULT back from the VHOST_VSOCK_SET_RUNNING ioctl():
>
> | E crosvm : vpipe worker thread exited with error:
VhostVsockStart(IoctlError(Os { code: 14, kind: Uncategorized, message:
"Bad address" }))
>
>The same guest payload works correctly on a 5.10 KVM host kernel.
>
>After some digging, we narrowed this change in behaviour down to
>e13a6915a03f ("vhost/vsock: add IOTLB API support") and further
digging
>reveals that the infamous VIRTIO_F_ACCESS_PLATFORM feature flag is to
>blame. Indeed, our tests once again pass if we revert that patch
(there's
>a trivial conflict with the later addition of VIRTIO_VSOCK_F_SEQPACKET
>but otherwise it reverts cleanly).
>
>On Android, KVM runs in a mode where the host kernel is, by default,
>unable to access guest memory [1] and so memory used for virtio (e.g.
>the rings and descriptor data) needs to be shared explicitly with the
>host using hypercalls issued by the guest. We implement this on top of
>restricted DMA [2], whereby the guest allocates a pool of shared memory
>during boot and bounces all virtio transfers through this window. In
>order to get the guest to use the DMA API for virtio devices (which is
>required for the SWIOTLB code to kick in and do the aforementioned
>bouncing), crosvm sets the VIRTIO_F_ACCESS_PLATFORM feature flag on its
>emulated devices and this is picked up by virtio_has_dma_quirk() in the
>guest kernel. This has been working well for us so far.
>
>With e13a6915a03f, the vhost backend for vsock now advertises
>VIRTIO_F_ACCESS_PLATFORM in its response to the VHOST_GET_FEATURES
>ioctl() and accepts it in the VHOST_SET_FEATURES as a mechanism to
>enable the IOTLB feature (note: this is nothing to do with SWIOTLB!).
>This feature is used for emulation of a virtual IOMMU and requires
>explicit support for issuing IOTLB messages (see VHOST_IOTLB_MSG and
>VHOST_IOTLB_MSG_V2) from userspace to manage address translations for
>the virtio device.
>
>Looking at how crosvm uses these vhost ioctl()s, we can see:
>
>        let avail_features = self
>            .vhost_handle
>            .get_features()
>            .map_err(Error::VhostGetFeatures)?;
>
>        let features: c_ulonglong = self.acked_features &
avail_features;
>        self.vhost_handle
>            .set_features(features)
>            .map_err(Error::VhostSetFeatures)?;
>
>where 'acked_features' is the feature set negotiated between crosvm
and
>the guest, while 'avail_features' is the supported feature set
>advertised by vhost. The intersection of these now includes
>VIRTIO_F_ACCESS_PLATFORM in the 5.15 kernel and so we quietly start
>enabling IOTLB, despite not having any of the necessary support in
>crosvm and therefore the vsock thread effectively grinds to a halt and
>we cannot communicate with the guest.
>
>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?

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.

Indeed that patch was to fix 
https://bugzilla.redhat.com/show_bug.cgi?id=1894101
>features,
>but others here have reasonably pointed out that they didn't expect a
>kernel change to break userspace. On the flip side, the offending commit
>in the kernel isn't exactly new (it's from the end of 2020!) and so
it's
>likely that others (e.g. QEMU) are using this feature.
Yep, QEMU can use it.
>I also strongly
>suspect that vhost net suffers from exactly the same issue, we just
>don't happen to be using that (yet) in Android.
I think so too, the implementation in vsock was done following 
vhost-net.

Thanks,
Stefano

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