[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO |
Date: |
Thu, 9 Jan 2020 07:39:17 -0500 |
On Thu, Jan 09, 2020 at 09:25:42AM -0300, André Silva wrote:
> 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é
I don't think it is, I think we do need an extra swap
in some cases. It's possible that some cross-endian
setups are broken now, if so pls include testing
result not just theoretical analysis.
> 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
> >
- [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Andre Silva, 2020/01/08
- [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Andre Silva, 2020/01/08
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Peter Maydell, 2020/01/08
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Michael S. Tsirkin, 2020/01/09
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, André Silva, 2020/01/09
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO,
Michael S. Tsirkin <=
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Greg Kurz, 2020/01/09
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, André Silva, 2020/01/09
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Greg Kurz, 2020/01/10
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, André Silva, 2020/01/10
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, Greg Kurz, 2020/01/10
- Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO, André Silva, 2020/01/10