Cornelia Huck
2021-Oct-04 15:50 UTC
[RFC PATCH 1/1] virtio: write back features before verify
On Mon, Oct 04 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote:> On Mon, Oct 04, 2021 at 04:33:21PM +0200, Cornelia Huck wrote: >> On Mon, Oct 04 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote: >> >> > On Mon, Oct 04, 2021 at 02:19:55PM +0200, Cornelia Huck wrote: >> >> >> >> [cc:qemu-devel] >> >> >> >> On Sat, Oct 02 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote: >> >> >> >> > On Fri, Oct 01, 2021 at 09:21:25AM +0200, Halil Pasic wrote: >> >> >> On Thu, 30 Sep 2021 07:12:21 -0400 >> >> >> "Michael S. Tsirkin" <mst at redhat.com> wrote: >> >> >> >> >> >> > On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote: >> >> >> > > This patch fixes a regression introduced by commit 82e89ea077b9 >> >> >> > > ("virtio-blk: Add validation for block size in config space") and >> >> >> > > enables similar checks in verify() on big endian platforms. >> >> >> > > >> >> >> > > The problem with checking multi-byte config fields in the verify >> >> >> > > callback, on big endian platforms, and with a possibly transitional >> >> >> > > device is the following. The verify() callback is called between >> >> >> > > config->get_features() and virtio_finalize_features(). That we have a >> >> >> > > device that offered F_VERSION_1 then we have the following options >> >> >> > > either the device is transitional, and then it has to present the legacy >> >> >> > > interface, i.e. a big endian config space until F_VERSION_1 is >> >> >> > > negotiated, or we have a non-transitional device, which makes >> >> >> > > F_VERSION_1 mandatory, and only implements the non-legacy interface and >> >> >> > > thus presents a little endian config space. Because at this point we >> >> >> > > can't know if the device is transitional or non-transitional, we can't >> >> >> > > know do we need to byte swap or not. >> >> >> > >> >> >> > Hmm which transport does this refer to? >> >> >> >> >> >> It is the same with virtio-ccw and virtio-pci. I see the same problem >> >> >> with both on s390x. I didn't try with virtio-blk-pci-non-transitional >> >> >> yet (have to figure out how to do that with libvirt) for pci I used >> >> >> virtio-blk-pci. >> >> >> >> >> >> > Distinguishing between legacy and modern drivers is transport >> >> >> > specific. PCI presents >> >> >> > legacy and modern at separate addresses so distinguishing >> >> >> > between these two should be no trouble. >> >> >> >> >> >> You mean the device id? Yes that is bolted down in the spec, but >> >> >> currently we don't exploit that information. Furthermore there >> >> >> is a fat chance that with QEMU even the allegedly non-transitional >> >> >> devices only present a little endian config space after VERSION_1 >> >> >> was negotiated. Namely get_config for virtio-blk is implemented in >> >> >> virtio_blk_update_config() which does virtio_stl_p(vdev, >> >> >> &blkcfg.blk_size, blk_size) and in there we don't care >> >> >> about transitional or not: >> >> >> >> >> >> static inline bool virtio_access_is_big_endian(VirtIODevice *vdev) >> >> >> { >> >> >> #if defined(LEGACY_VIRTIO_IS_BIENDIAN) >> >> >> return virtio_is_big_endian(vdev); >> >> >> #elif defined(TARGET_WORDS_BIGENDIAN) >> >> >> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { >> >> >> /* Devices conforming to VIRTIO 1.0 or later are always LE. */ >> >> >> return false; >> >> >> } >> >> >> return true; >> >> >> #else >> >> >> return false; >> >> >> #endif >> >> >> } >> >> >> >> >> > >> >> > ok so that's a QEMU bug. Any virtio 1.0 and up >> >> > compatible device must use LE. >> >> > It can also present a legacy config space where the >> >> > endian depends on the guest. >> >> >> >> So, how is the virtio core supposed to determine this? A >> >> transport-specific callback? >> > >> > I'd say a field in VirtIODevice is easiest. >> >> The transport needs to set this as soon as it has figured out whether >> we're using legacy or not. > > Basically on each device config access?Prior to the first one, I think. It should not change again, should it?> >> I guess we also need to fence off any >> accesses respectively error out the device if the driver tries any >> read/write operations that would depend on that knowledge? >> >> And using a field in VirtIODevice would probably need some care when >> migrating. Hm... > > It's just a shorthand to minimize changes. No need to migrate I think.If we migrate in from an older QEMU, we don't know whether we are dealing with legacy or not, until feature negotiation is already done... don't we have to ask the transport?
Michael S. Tsirkin
2021-Oct-04 19:17 UTC
[RFC PATCH 1/1] virtio: write back features before verify
On Mon, Oct 04, 2021 at 05:50:44PM +0200, Cornelia Huck wrote:> On Mon, Oct 04 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > On Mon, Oct 04, 2021 at 04:33:21PM +0200, Cornelia Huck wrote: > >> On Mon, Oct 04 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote: > >> > >> > On Mon, Oct 04, 2021 at 02:19:55PM +0200, Cornelia Huck wrote: > >> >> > >> >> [cc:qemu-devel] > >> >> > >> >> On Sat, Oct 02 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote: > >> >> > >> >> > On Fri, Oct 01, 2021 at 09:21:25AM +0200, Halil Pasic wrote: > >> >> >> On Thu, 30 Sep 2021 07:12:21 -0400 > >> >> >> "Michael S. Tsirkin" <mst at redhat.com> wrote: > >> >> >> > >> >> >> > On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote: > >> >> >> > > This patch fixes a regression introduced by commit 82e89ea077b9 > >> >> >> > > ("virtio-blk: Add validation for block size in config space") and > >> >> >> > > enables similar checks in verify() on big endian platforms. > >> >> >> > > > >> >> >> > > The problem with checking multi-byte config fields in the verify > >> >> >> > > callback, on big endian platforms, and with a possibly transitional > >> >> >> > > device is the following. The verify() callback is called between > >> >> >> > > config->get_features() and virtio_finalize_features(). That we have a > >> >> >> > > device that offered F_VERSION_1 then we have the following options > >> >> >> > > either the device is transitional, and then it has to present the legacy > >> >> >> > > interface, i.e. a big endian config space until F_VERSION_1 is > >> >> >> > > negotiated, or we have a non-transitional device, which makes > >> >> >> > > F_VERSION_1 mandatory, and only implements the non-legacy interface and > >> >> >> > > thus presents a little endian config space. Because at this point we > >> >> >> > > can't know if the device is transitional or non-transitional, we can't > >> >> >> > > know do we need to byte swap or not. > >> >> >> > > >> >> >> > Hmm which transport does this refer to? > >> >> >> > >> >> >> It is the same with virtio-ccw and virtio-pci. I see the same problem > >> >> >> with both on s390x. I didn't try with virtio-blk-pci-non-transitional > >> >> >> yet (have to figure out how to do that with libvirt) for pci I used > >> >> >> virtio-blk-pci. > >> >> >> > >> >> >> > Distinguishing between legacy and modern drivers is transport > >> >> >> > specific. PCI presents > >> >> >> > legacy and modern at separate addresses so distinguishing > >> >> >> > between these two should be no trouble. > >> >> >> > >> >> >> You mean the device id? Yes that is bolted down in the spec, but > >> >> >> currently we don't exploit that information. Furthermore there > >> >> >> is a fat chance that with QEMU even the allegedly non-transitional > >> >> >> devices only present a little endian config space after VERSION_1 > >> >> >> was negotiated. Namely get_config for virtio-blk is implemented in > >> >> >> virtio_blk_update_config() which does virtio_stl_p(vdev, > >> >> >> &blkcfg.blk_size, blk_size) and in there we don't care > >> >> >> about transitional or not: > >> >> >> > >> >> >> static inline bool virtio_access_is_big_endian(VirtIODevice *vdev) > >> >> >> { > >> >> >> #if defined(LEGACY_VIRTIO_IS_BIENDIAN) > >> >> >> return virtio_is_big_endian(vdev); > >> >> >> #elif defined(TARGET_WORDS_BIGENDIAN) > >> >> >> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { > >> >> >> /* Devices conforming to VIRTIO 1.0 or later are always LE. */ > >> >> >> return false; > >> >> >> } > >> >> >> return true; > >> >> >> #else > >> >> >> return false; > >> >> >> #endif > >> >> >> } > >> >> >> > >> >> > > >> >> > ok so that's a QEMU bug. Any virtio 1.0 and up > >> >> > compatible device must use LE. > >> >> > It can also present a legacy config space where the > >> >> > endian depends on the guest. > >> >> > >> >> So, how is the virtio core supposed to determine this? A > >> >> transport-specific callback? > >> > > >> > I'd say a field in VirtIODevice is easiest. > >> > >> The transport needs to set this as soon as it has figured out whether > >> we're using legacy or not. > > > > Basically on each device config access? > > Prior to the first one, I think. It should not change again, should it?Well yes but we never prohibited someone from poking at both .. Doing it on each access means we don't have state to migrate.> > > >> I guess we also need to fence off any > >> accesses respectively error out the device if the driver tries any > >> read/write operations that would depend on that knowledge? > >> > >> And using a field in VirtIODevice would probably need some care when > >> migrating. Hm... > > > > It's just a shorthand to minimize changes. No need to migrate I think. > > If we migrate in from an older QEMU, we don't know whether we are > dealing with legacy or not, until feature negotiation is already > done... don't we have to ask the transport?Right but the only thing that can happen is config access. Well and for legacy a kick I guess. -- MST