Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO

[André Silva]

Hi Michael!
Thanks for reviewing the patch!

> we always get LE values from memory subsystem,
> not target endian values:

I see. So do you think the patch is correct in eliminating the extra
swap (as virtio_config_readw for example already makes a swap)?

Thanks,
andré

On Thu, Jan 9, 2020 at 7:50 AM Michael S. Tsirkin <address@hidden> wrote:
>
> On Wed, Jan 08, 2020 at 01:16:18PM -0300, Andre Silva wrote:
> > Remove the bswap function calls after reading and before writing
> > memory bytes in virtio_pci_config_read and virtio_pci_config_write
> > because they are reverting back an already swapped bytes.
> >
> > Consider the table below in the context of virtio_pci_config_read
> > function.
> >
> > Host   Target  virtio-config-read[wl]
> >                swap?                   virtio-is-big-endian?   extra bswap? 
> >   Should be   Final result   Final result ok?
> > ----- ------- ------------------------ ----------------------- 
> > -------------- ----------- -------------- ------------------
> > LE     BE      s(x)                    true                    s(s(x))      
> >   s(x)        x              No
> > LE     LE      x                       false                   -            
> >   x           x              Yes
> > BE     LE      s(x)                    false                   -            
> >   s(x)        s(x)           Yes
> > BE     BE      x                       true                    s(x)         
> >   x           s(x)           No
>
> we always get LE values from memory subsystem,
> not target endian values:
>
> static const MemoryRegionOps virtio_pci_config_ops = {
>     .read = virtio_pci_config_read,
>     .write = virtio_pci_config_write,
>     .impl = {
>         .min_access_size = 1,
>         .max_access_size = 4,
>     },
>     .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
>
> This triggers another swap in address_space_ldl_internal
> (memory_ldst.inc.c).
>
>
> > In table above, when target is big endian and VirtIO is pre 1.0,
> > function virtio_is_big_endian would return true and the extra
> > swap would be executed, reverting the previous swap made by
> > virtio_config_read[wl].
> >
> > The 's(x)' means that a swap function was applied at
> > address x. 'LE' is little endian and 'BE' is big endian. The
> > 'Final result' column is the returned value from
> > virtio_pci_config_read, considering a target Virtio pre 1.0.
> > 'x' means that target's value was not swapped in Qemu, 's(x)' means
> > that Qemu will use a swapped value.
> >
> > If we remove the extra swap made in virtio_pci_config_read we will
> > have the correct result in any host/target combination, both for
> > VirtIO pre 1.0 or later versions.
> >
> > The same reasoning applies to virtio_pci_config_write.
> >
> > Signed-off-by: Andre Silva <address@hidden>
> > ---
> >  hw/virtio/virtio-pci.c | 12 ------------
> >  1 file changed, 12 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index c6b47a9c73..4ba9e847f3 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -431,15 +431,9 @@ static uint64_t virtio_pci_config_read(void *opaque, 
> > hwaddr addr,
> >          break;
> >      case 2:
> >          val = virtio_config_readw(vdev, addr);
> > -        if (virtio_is_big_endian(vdev)) {
> > -            val = bswap16(val);
> > -        }
> >          break;
> >      case 4:
> >          val = virtio_config_readl(vdev, addr);
> > -        if (virtio_is_big_endian(vdev)) {
> > -            val = bswap32(val);
> > -        }
> >          break;
> >      }
> >      return val;
> > @@ -465,15 +459,9 @@ static void virtio_pci_config_write(void *opaque, 
> > hwaddr addr,
> >          virtio_config_writeb(vdev, addr, val);
> >          break;
> >      case 2:
> > -        if (virtio_is_big_endian(vdev)) {
> > -            val = bswap16(val);
> > -        }
> >          virtio_config_writew(vdev, addr, val);
> >          break;
> >      case 4:
> > -        if (virtio_is_big_endian(vdev)) {
> > -            val = bswap32(val);
> > -        }
> >          virtio_config_writel(vdev, addr, val);
> >          break;
> >      }
> > --
> > 2.24.1
>