Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 6 Aug 2022 10:42:40 +0100
From:   Will Deacon <will@kernel.org>
To:     Stefano Garzarella <sgarzare@redhat.com>
Cc:     mst@redhat.com, stefanha@redhat.com, jasowang@redhat.com,
        torvalds@linux-foundation.org, ascull@google.com, maz@kernel.org,
        keirf@google.com, jiyong@google.com, kernel-team@android.com,
        linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org, kvm@vger.kernel.org
Subject: Re: IOTLB support for vhost/vsock breaks crosvm on Android
Message-ID: <20220806094239.GA30268@willie-the-truck>
References: <20220805181105.GA29848@willie-the-truck>
 <20220806074828.zwzgn5gj47gjx5og@sgarzare-redhat>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220806074828.zwzgn5gj47gjx5og@sgarzare-redhat>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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