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

[Greg Kurz]

On Fri, 10 Jan 2020 09:00:50 -0300
André Silva <address@hidden> wrote:

> > What are the symptoms without your patch ? What's the QEMU version ?
> 
> If using virtio for networking, guest vtnet0 interface appears with
> 'status: no carrier'. Applying the patch the interface appears as
> 'status: active' and works normally.
> I tested with branches stable-4.1 and master.
> 
> > Do you hit the issue with upstream QEMU ?
> 
> No, I tested it with master and got the same fw as you, 'FW Version =
> git-9546892a80d5a4c7' and had no problems...
> Not sure if there are some parameter to quemu that may your side work,
> but I'm invoking qemu like this:
> 
> $ sudo ./qemu/build/release/ppc64-softmmu/qemu-system-ppc64 -drive
> file=disc1.qcow2,if=scsi,format=qcow2 -enable-kvm -machine

Hmm if I have to pass if=virtio to end up with a virtio-blk device.
Unrelated, -enable-kvm isn't needed since...

> pseries,accel=kvm,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-hpt-max-page-size=16M

... accel=kvm is passed to the machine.

> -mem-prealloc -mem-path /dev/hugepages -vnc :74 -nographic -vga none
> -smp 20 -m 4G -net tap -netdev tap,id=n1 -device
> virtio-net-pci,netdev=n1
> 

I enforce the use of legacy virtio by adding:

    -global virtio-pci.disable-modern=on

Could you try your patch against master with if=virtio and the
-global above ?

> Thanks,
> andré
> 
> On Fri, Jan 10, 2020 at 5:55 AM Greg Kurz <address@hidden> wrote:
> >
> > On Thu, 9 Jan 2020 18:18:57 -0300
> > André Silva <address@hidden> wrote:
> >
> > > Hi Greg,
> > >
> >
> > Hi André,
> >
> > > Thanks for the commit info.
> > > But I'm testing in this scenario, that is, a ppc64le host with a ppc64
> > > BE guest, and without my patch I can't get virtio to work. The patch
> >
> > What are the symptoms without your patch ? What's the QEMU version ?
> >
> > > makes virtio 0.95 (legacy) net, scsi, blk work. I don't get the
> > > firmware error. I also tested with a ppc64le guest and had no problems
> > > either. Maybe we have different firmware versions?
> > >
> > > My firmware output:
> > >
> > > SLOF 
> > > **********************************************************************
> > > QEMU Starting
> > >  Build Date = Jul  3 2019 12:26:14
> > >  FW Version = git-ba1ab360eebe6338
> >
> > I'm using the latest SLOF from the QEMU tree (pc-bios/slof.bin):
> >
> > SLOF **********************************************************************
> > QEMU Starting
> >  Build Date = Dec 17 2019 11:31:13
> >  FW Version = git-9546892a80d5a4c7
> >
> > Do you hit the issue with upstream QEMU ?
> >
> > >  Press "s" to enter Open Firmware.
> > >
> > > Populating /vdevice methods
> > > Populating /vdevice/vty@71000000
> > > Populating /vdevice/nvram@71000001
> > > Populating /vdevice/v-scsi@71000002
> > >        SCSI: Looking for devices
> > >           8200000000000000 CD-ROM   : "QEMU     QEMU CD-ROM      2.5+"
> > > Populating /pci@800000020000000
> > >                      00 0000 (D) : 1af4 1000    virtio [ net ]
> > >                      00 0800 (D) : 1af4 1001    virtio [ block ]
> > > No NVRAM common partition, re-initializing...
> > > Scanning USB
> > > Using default console: /vdevice/vty@71000000
> > >
> > >   Welcome to Open Firmware
> > >
> > >   Copyright (c) 2004, 2017 IBM Corporation All rights reserved.
> > >   This program and the accompanying materials are made available
> > >   under the terms of the BSD License available at
> > >   http://www.opensource.org/licenses/bsd-license.php
> > >
> > >
> > > Trying to load:  from: /pci@800000020000000/scsi@1 ...   Successfully 
> > > loaded
> > >
> > > >> FreeBSD/powerpc Open Firmware boot block
> > >    Boot path:   /pci@800000020000000/scsi@1
> > >    Boot loader: /boot/loader
> > >    Boot volume:   /pci@800000020000000/scsi@1:2
> > > Consoles: Open Firmware console
> > >
> > > FreeBSD/powerpc64 Open Firmware loader, Revision 0.1
> > > (Mon Nov 11 22:33:43 -02 2019 jenkins@FreeBSD_x86)
> > > Memory: 4194304KB
> > > Booted from: /pci@800000020000000/scsi@1
> > >
> > > Loading /boot/defaults/loader.conf
> > > /boot/kernel/kernel data=0x129f658+0x4aaa88 
> > > syms=[0x8+0x105120+0x8+0x125429]
> > > ...
> > >
> > > Until now, I was able to test the patch and see virtio working on the
> > > following systems:
> > >
> > >   Qemu     Host             Guest                           Guest VirtIO
> > >   -------- ---------------- ------------------------------- --------------
> > >   master   Ubuntu ppc64le   FreeBSD 13.0-current ppc64 BE   legacy
> > >   master   Ubuntu ppc64le   debian 4.19.0-6-powerpc64le     modern
> > >   master   Ubuntu ppc64le   debian 4.19.0-6-powerpc64le     legacy
> > >   master   arch x86_64      FreeBSD 13.0-current ppc64 BE   legacy
> > >
> > > Thanks,
> > > andré
> > >
> > > On Thu, Jan 9, 2020 at 1:06 PM Greg Kurz <address@hidden> wrote:
> > > >
> > > > On Thu, 9 Jan 2020 07:39:17 -0500
> > > > "Michael S. Tsirkin" <address@hidden> wrote:
> > > >
> > > > > 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.
> > > > >
> > > >
> > > > I confirm that we must keep the extra swap otherwise
> > > > read/write in cross-endian setups will have wrong
> > > > endian. Please read this commit for a more detailed
> > > > explanation:
> > > >
> > > > commit 82afa58641b0e67abbaf4da6c325ebd7c2513262
> > > > Author: Benjamin Herrenschmidt <address@hidden>
> > > > Date:   Tue Jan 10 01:35:11 2012 +0000
> > > >
> > > >     virtio-pci: Fix endianness of virtio config
> > > >
> > > > https://git.qemu.org/?p=qemu.git;a=commitdiff;h=82afa58641b0e67abbaf4da6c325ebd7c2513262
> > > >
> > > > This is especially critical on ppc64 since _all_ hosts are now LE
> > > > but the first piece of code in the guest that is likely to drive
> > > > the device is the SLOF firmware which is BE.
> > > >
> > > > This is what we get with this patch when trying to run a pseries guest 
> > > > on a
> > > > ppc64le host:
> > > >
> > > > Trying to load:  from: /pci@800000020000000/scsi@0 ... 
> > > > virtioblk_transfer: Access beyond end of device!
> > > >
> > > > Cheers,
> > > >
> > > > --
> > > > Greg
> > > >
> > > > > > 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
> > > > > > >
> > > > >
> > > > >
> > > >
> >