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@xxxxxxxxxx> 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); 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. Will
