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;
MIME-Version: 1.0
References: <20220805181105.GA29848@willie-the-truck> <20220806074828.zwzgn5gj47gjx5og@sgarzare-redhat>
 <20220806094239.GA30268@willie-the-truck> <CAD60JZMbbkwFHqCm_iCrOrKgRLBUMkDQfuJ=Q1T-sZt59eTBrw@mail.gmail.com>
 <20220806143443.GA30658@willie-the-truck>
In-Reply-To: <20220806143443.GA30658@willie-the-truck>
From:   Stefan Hajnoczi <shajnocz@redhat.com>
Date:   Sat, 6 Aug 2022 17:47:50 -0400
Message-ID: <CAD60JZOwho3D-_gXZTT3aYSgo04Pd=VrM7QyJbgfp8PYqzOvgw@mail.gmail.com>
Subject: Re: IOTLB support for vhost/vsock breaks crosvm on Android
To:     Will Deacon <will@kernel.org>
Cc:     Stefano Garzarella <sgarzare@redhat.com>,
        Michael Tsirkin <mst@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Jason Wang <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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Aug 6, 2022 at 10:35 AM Will Deacon <will@kernel.org> wrote:
>
> On Sat, Aug 06, 2022 at 06:52:15AM -0400, Stefan Hajnoczi wrote:
> > On Sat, Aug 6, 2022 at 5:50 AM Will Deacon <will@kernel.org> wrote:
> > > 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:
> > > > 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.
> >
> > Unconditionally exposing all vhost feature bits to the guest is
> > incorrect. The emulator must filter out only the feature bits that it
> > supports.
>
> I've evidently done a bad job of explaining this, sorry.
>
> crosvm _does_ filter the feature bits which it passes to vhost. It takes the
> feature set which it has negotiated with the guest and then takes the
> intersection of this set with the set of features which vhost advertises.
> The result is what is passed to VHOST_SET_FEATURES. I included the rust
> for this in my initial mail, but in C it might look something like:
>
>         u64 features = negotiate_features_with_guest(dev);
>
>         ioctl(vhost_fd, VHOST_GET_FEATURES, &vhost_features);
>         vhost_features &= features;     /* Mask out unsupported features */
>         ioctl(vhost_fd, VHOST_SET_FEATURES, &vhost_features);

This is unrelated to the current issue, but this code looks wrong.
VHOST_GET_FEATURES must be called before negotiating with the guest.
The device features must be restricted by vhost before advertising
them to the guest. For example, if a new crosvm binary runs on an old
kernel then feature bits crosvm negotiated with the guest may not be
supported by the vhost kernel module and the device is broken.

> The problem is that crosvm has negotiated VIRTIO_F_ACCESS_PLATFORM with
> the guest so that restricted DMA is used for the virtio devices. With
> e13a6915a03f, VIRTIO_F_ACCESS_PLATFORM is now advertised by
> VHOST_GET_FEATURES and so IOTLB is enabled by the sequence above.
>
> > For example, see QEMU's vhost-net device's vhost feature bit allowlist:
> > https://gitlab.com/qemu-project/qemu/-/blob/master/hw/net/vhost_net.c#L40
>
> I agree that changing crosvm to use an allowlist would fix the problem,
> I'm just questioning whether we should be changing userspace at all to
> resolve this issue.
>
> > The reason why the emulator (crosvm/QEMU/etc) must filter out feature
> > bits is that vhost devices are full VIRTIO devices. They are a subset
> > of a VIRTIO device and the emulator is responsible for the rest of the
> > device. Some features will require both vhost and emulator support.
> > Therefore it is incorrect to expect the device to work correctly if
> > the vhost feature bits are passed through to the guest.
>
> I think crosvm is trying to cater for this by masking out the features
> it doesn't know about.

Can you point to the guest driver code for restricted DMA? It's
unclear to me what the guest drivers are doing and whether that is
VIRTIO spec compliant. Is the driver compliant with VIRTIO 1.2 "6.1
Driver Requirements: Reserved Feature Bits":

  A driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered,
and it MUST then either disable the IOMMU or configure the IOMMU to
translate bus addresses passed to the device into physical addresses
in memory. If VIRTIO_F_ACCESS_PLATFORM is not offered, then a driver
MUST pass only physical addresses to the device.

Stefan