On Thu, 9 Jan 2020 18:18:57 -0300 André Silva <afscoe...@gmail.com> 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 <gr...@kaod.org> wrote: > > > > On Thu, 9 Jan 2020 07:39:17 -0500 > > "Michael S. Tsirkin" <m...@redhat.com> 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 <b...@kernel.crashing.org> > > 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 <m...@redhat.com> > > > > 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 <afscoe...@gmail.com> > > > > > > --- > > > > > > 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 > > > > > > > > > > > > >
