| Message ID | 20211111063854.29060-2-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] virtio: use virtio accessor to access packed descriptor flags | expand |
On 11/11/21 07:38, Jason Wang wrote: > We used to access packed descriptor event and off_wrap via > address_space_{write|read}_cached(). When we hit the cache, memcpy() > is used which is not atomic which may lead a wrong value to be read or > wrote. > > This patch fixes this by switching to use > virito_{stw|lduw}_phys_cached() to make sure the access is atomic. > > Fixes: 683f7665679c1 ("virtio: event suppression support for packed ring") > Cc: qemu-stable@nongnu.org > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/virtio/virtio.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> No cover so asking here, what about vring_packed_desc_read()?
On Thu, Nov 11, 2021 at 3:51 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 11/11/21 07:38, Jason Wang wrote: > > We used to access packed descriptor event and off_wrap via > > address_space_{write|read}_cached(). When we hit the cache, memcpy() > > is used which is not atomic which may lead a wrong value to be read or > > wrote. > > > > This patch fixes this by switching to use > > virito_{stw|lduw}_phys_cached() to make sure the access is atomic. > > > > Fixes: 683f7665679c1 ("virtio: event suppression support for packed ring") > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > --- > > hw/virtio/virtio.c | 13 ++++--------- > > 1 file changed, 4 insertions(+), 9 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > No cover so asking here, what about vring_packed_desc_read()? In that function, the vring_packed_desc_read_flags() used for reading the flags atomically. If the flags told us the buffer is available, there's no need read the rest of descriptor in atomic operation since the driver guarantee that the changes of flags are visible after the rest of the descriptor is setup. Thanks >
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 939bcbfeb9..ea7c079fb0 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -247,13 +247,10 @@ static void vring_packed_event_read(VirtIODevice *vdev, hwaddr off_off = offsetof(VRingPackedDescEvent, off_wrap); hwaddr off_flags = offsetof(VRingPackedDescEvent, flags); - address_space_read_cached(cache, off_flags, &e->flags, - sizeof(e->flags)); + e->flags = virtio_lduw_phys_cached(vdev, cache, off_flags); /* Make sure flags is seen before off_wrap */ smp_rmb(); - address_space_read_cached(cache, off_off, &e->off_wrap, - sizeof(e->off_wrap)); - virtio_tswap16s(vdev, &e->off_wrap); + e->off_wrap = virtio_lduw_phys_cached(vdev, cache, off_off); virtio_tswap16s(vdev, &e->flags); } @@ -263,8 +260,7 @@ static void vring_packed_off_wrap_write(VirtIODevice *vdev, { hwaddr off = offsetof(VRingPackedDescEvent, off_wrap); - virtio_tswap16s(vdev, &off_wrap); - address_space_write_cached(cache, off, &off_wrap, sizeof(off_wrap)); + virtio_stw_phys_cached(vdev, cache, off, off_wrap); address_space_cache_invalidate(cache, off, sizeof(off_wrap)); } @@ -273,8 +269,7 @@ static void vring_packed_flags_write(VirtIODevice *vdev, { hwaddr off = offsetof(VRingPackedDescEvent, flags); - virtio_tswap16s(vdev, &flags); - address_space_write_cached(cache, off, &flags, sizeof(flags)); + virtio_stw_phys_cached(vdev, cache, off, flags); address_space_cache_invalidate(cache, off, sizeof(flags)); }
We used to access packed descriptor event and off_wrap via address_space_{write|read}_cached(). When we hit the cache, memcpy() is used which is not atomic which may lead a wrong value to be read or wrote. This patch fixes this by switching to use virito_{stw|lduw}_phys_cached() to make sure the access is atomic. Fixes: 683f7665679c1 ("virtio: event suppression support for packed ring") Cc: qemu-stable@nongnu.org Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio/virtio.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)