Re: [PATCH 2/2] virtio: use virtio accessor to access packed event

[Jason Wang]

On Thu, Nov 11, 2021 at 3:51 PM Philippe Mathieu-Daudé
 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-sta...@nongnu.org
> > Signed-off-by: Jason Wang 
> > ---
> >  hw/virtio/virtio.c | 13 -
> >  1 file changed, 4 insertions(+), 9 deletions(-)
>
> Reviewed-by: Philippe Mathieu-Daudé 
>
> 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

>

Re: [PATCH 2/2] virtio: use virtio accessor to access packed event

[Philippe Mathieu-Daudé]

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-sta...@nongnu.org
> Signed-off-by: Jason Wang 
> ---
>  hw/virtio/virtio.c | 13 -
>  1 file changed, 4 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

No cover so asking here, what about vring_packed_desc_read()?

[PATCH 2/2] virtio: use virtio accessor to access packed event

[Jason Wang]

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-sta...@nongnu.org
Signed-off-by: Jason Wang 
---
 hw/virtio/virtio.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

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, >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, >off_wrap,
-  sizeof(e->off_wrap));
-virtio_tswap16s(vdev, >off_wrap);
+e->off_wrap = virtio_lduw_phys_cached(vdev, cache, off_off);
 virtio_tswap16s(vdev, >flags);
 }
 
@@ -263,8 +260,7 @@ static void vring_packed_off_wrap_write(VirtIODevice *vdev,
 {
 hwaddr off = offsetof(VRingPackedDescEvent, off_wrap);
 
-virtio_tswap16s(vdev, _wrap);
-address_space_write_cached(cache, 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, );
-address_space_write_cached(cache, off, , sizeof(flags));
+virtio_stw_phys_cached(vdev, cache, off, flags);
 address_space_cache_invalidate(cache, off, sizeof(flags));
 }
 
-- 
2.25.1