* [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
@ 2020-01-08 16:16 Andre Silva
2020-01-08 17:51 ` Peter Maydell
2020-01-09 10:50 ` Michael S. Tsirkin
0 siblings, 2 replies; 15+ messages in thread
From: Andre Silva @ 2020-01-08 16:16 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
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
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 <afscoelho@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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-08 16:16 [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO Andre Silva
@ 2020-01-08 17:51 ` Peter Maydell
2020-01-08 19:37 ` André Silva
2020-01-09 10:50 ` Michael S. Tsirkin
1 sibling, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2020-01-08 17:51 UTC (permalink / raw)
To: Andre Silva; +Cc: QEMU Developers, Michael S. Tsirkin
On Wed, 8 Jan 2020 at 16:20, Andre Silva <afscoelho@gmail.com> 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
Is "because we wrote it via virtio_pci_config_write" really
the only way that the data read by virtio_pci_config_read
is ever set or updated ?
thanks
-- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-08 17:51 ` Peter Maydell
@ 2020-01-08 19:37 ` André Silva
0 siblings, 0 replies; 15+ messages in thread
From: André Silva @ 2020-01-08 19:37 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Michael S. Tsirkin
Yes, it looks like. virtio_pci_config_read is called via a pointer
from memory_region_read_accessor and memory_region_write_accessor
calls virtio_pci_config_write.
I tested the patch in a linux/ppc64 host with a FreeBSD/ppc64 guest
and VirtIO 0.9 (legacy) driver and from what I saw the config area is
accessed only in these functions.
Thanks!
andré
On Wed, Jan 8, 2020 at 2:51 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 8 Jan 2020 at 16:20, Andre Silva <afscoelho@gmail.com> 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
>
> Is "because we wrote it via virtio_pci_config_write" really
> the only way that the data read by virtio_pci_config_read
> is ever set or updated ?
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-08 16:16 [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO Andre Silva
2020-01-08 17:51 ` Peter Maydell
@ 2020-01-09 10:50 ` Michael S. Tsirkin
2020-01-09 12:25 ` André Silva
1 sibling, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2020-01-09 10:50 UTC (permalink / raw)
To: Andre Silva; +Cc: qemu-devel
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 <afscoelho@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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-09 10:50 ` Michael S. Tsirkin
@ 2020-01-09 12:25 ` André Silva
2020-01-09 12:39 ` Michael S. Tsirkin
0 siblings, 1 reply; 15+ messages in thread
From: André Silva @ 2020-01-09 12:25 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: QEMU Developers
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 <mst@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 <afscoelho@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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-09 12:25 ` André Silva
@ 2020-01-09 12:39 ` Michael S. Tsirkin
2020-01-09 16:06 ` Greg Kurz
0 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2020-01-09 12:39 UTC (permalink / raw)
To: André Silva; +Cc: QEMU Developers
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 <mst@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 <afscoelho@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
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-09 12:39 ` Michael S. Tsirkin
@ 2020-01-09 16:06 ` Greg Kurz
2020-01-09 21:18 ` André Silva
0 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2020-01-09 16:06 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: André Silva, QEMU Developers
On Thu, 9 Jan 2020 07:39:17 -0500
"Michael S. Tsirkin" <mst@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 <benh@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 <mst@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 <afscoelho@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
> > >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-09 16:06 ` Greg Kurz
@ 2020-01-09 21:18 ` André Silva
2020-01-10 8:55 ` Greg Kurz
0 siblings, 1 reply; 15+ messages in thread
From: André Silva @ 2020-01-09 21:18 UTC (permalink / raw)
To: Greg Kurz; +Cc: QEMU Developers, Michael S. Tsirkin
Hi Greg,
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
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
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 <groug@kaod.org> wrote:
>
> On Thu, 9 Jan 2020 07:39:17 -0500
> "Michael S. Tsirkin" <mst@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 <benh@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 <mst@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 <afscoelho@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
> > > >
> >
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-09 21:18 ` André Silva
@ 2020-01-10 8:55 ` Greg Kurz
2020-01-10 12:00 ` André Silva
0 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2020-01-10 8:55 UTC (permalink / raw)
To: André Silva; +Cc: QEMU Developers, Michael S. Tsirkin
On Thu, 9 Jan 2020 18:18:57 -0300
André Silva <afscoelho@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 <groug@kaod.org> wrote:
> >
> > On Thu, 9 Jan 2020 07:39:17 -0500
> > "Michael S. Tsirkin" <mst@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 <benh@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 <mst@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 <afscoelho@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
> > > > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-10 8:55 ` Greg Kurz
@ 2020-01-10 12:00 ` André Silva
2020-01-10 14:50 ` Greg Kurz
0 siblings, 1 reply; 15+ messages in thread
From: André Silva @ 2020-01-10 12:00 UTC (permalink / raw)
To: Greg Kurz; +Cc: QEMU Developers, Michael S. Tsirkin
> 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
pseries,accel=kvm,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-hpt-max-page-size=16M
-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
Thanks,
andré
On Fri, Jan 10, 2020 at 5:55 AM Greg Kurz <groug@kaod.org> wrote:
>
> On Thu, 9 Jan 2020 18:18:57 -0300
> André Silva <afscoelho@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 <groug@kaod.org> wrote:
> > >
> > > On Thu, 9 Jan 2020 07:39:17 -0500
> > > "Michael S. Tsirkin" <mst@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 <benh@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 <mst@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 <afscoelho@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
> > > > > >
> > > >
> > > >
> > >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-10 12:00 ` André Silva
@ 2020-01-10 14:50 ` Greg Kurz
2020-01-10 17:09 ` André Silva
0 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2020-01-10 14:50 UTC (permalink / raw)
To: André Silva; +Cc: QEMU Developers, Michael S. Tsirkin
On Fri, 10 Jan 2020 09:00:50 -0300
André Silva <afscoelho@gmail.com> 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 <groug@kaod.org> wrote:
> >
> > On Thu, 9 Jan 2020 18:18:57 -0300
> > André Silva <afscoelho@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 <groug@kaod.org> wrote:
> > > >
> > > > On Thu, 9 Jan 2020 07:39:17 -0500
> > > > "Michael S. Tsirkin" <mst@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 <benh@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 <mst@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 <afscoelho@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
> > > > > > >
> > > > >
> > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-10 14:50 ` Greg Kurz
@ 2020-01-10 17:09 ` André Silva
0 siblings, 0 replies; 15+ messages in thread
From: André Silva @ 2020-01-10 17:09 UTC (permalink / raw)
To: Greg Kurz; +Cc: QEMU Developers, Michael S. Tsirkin
> Unrelated, -enable-kvm isn't needed since...
Thanks.
> Could you try your patch against master with if=virtio and the
> -global above ?
There you go! I got the same error as you!
This looks like the only case that makes slof complain. If I use
virtio-blk without the -global option slof works fine.
Virtio-net (with if=scsi) and Virtio-scsi plus my patch plus the
-global option work fine too, no slof error.
Thanks Greg, I will investigate this virtio-blk side in qemu/slof on
how to make it work with the patch.
andré
On Fri, Jan 10, 2020 at 11:50 AM Greg Kurz <groug@kaod.org> wrote:
>
> On Fri, 10 Jan 2020 09:00:50 -0300
> André Silva <afscoelho@gmail.com> 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 <groug@kaod.org> wrote:
> > >
> > > On Thu, 9 Jan 2020 18:18:57 -0300
> > > André Silva <afscoelho@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 <groug@kaod.org> wrote:
> > > > >
> > > > > On Thu, 9 Jan 2020 07:39:17 -0500
> > > > > "Michael S. Tsirkin" <mst@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 <benh@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 <mst@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 <afscoelho@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
> > > > > > > >
> > > > > >
> > > > > >
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-08 12:56 ` Andre Silva
@ 2020-01-08 15:49 ` no-reply
0 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2020-01-08 15:49 UTC (permalink / raw)
To: afscoelho; +Cc: qemu-devel, mst
Patchew URL: https://patchew.org/QEMU/20200108125658.208480-2-afscoelho@gmail.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
Type: series
Message-id: 20200108125658.208480-2-afscoelho@gmail.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/642f73c78a99258dc134e3879a0287db8ef176c0.1578497245.git.tgolembi@redhat.com -> patchew/642f73c78a99258dc134e3879a0287db8ef176c0.1578497245.git.tgolembi@redhat.com
Switched to a new branch 'test'
1d3cb2b virtio: Prevent double swap due to target pre 1.0 VirtIO
=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)
total: 1 errors, 0 warnings, 30 lines checked
Commit 1d3cb2b436f0 (virtio: Prevent double swap due to target pre 1.0 VirtIO) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200108125658.208480-2-afscoelho@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
2020-01-08 12:56 Andre Silva
@ 2020-01-08 12:56 ` Andre Silva
2020-01-08 15:49 ` no-reply
0 siblings, 1 reply; 15+ messages in thread
From: Andre Silva @ 2020-01-08 12:56 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
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
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.
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO
@ 2020-01-08 12:56 Andre Silva
2020-01-08 12:56 ` Andre Silva
0 siblings, 1 reply; 15+ messages in thread
From: Andre Silva @ 2020-01-08 12:56 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
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
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 <afscoelho@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
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-01-10 17:10 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-08 16:16 [PATCH] virtio: Prevent double swap due to target pre 1.0 VirtIO Andre Silva
2020-01-08 17:51 ` Peter Maydell
2020-01-08 19:37 ` André Silva
2020-01-09 10:50 ` Michael S. Tsirkin
2020-01-09 12:25 ` André Silva
2020-01-09 12:39 ` Michael S. Tsirkin
2020-01-09 16:06 ` Greg Kurz
2020-01-09 21:18 ` André Silva
2020-01-10 8:55 ` Greg Kurz
2020-01-10 12:00 ` André Silva
2020-01-10 14:50 ` Greg Kurz
2020-01-10 17:09 ` André Silva
-- strict thread matches above, loose matches on Subject: below --
2020-01-08 12:56 Andre Silva
2020-01-08 12:56 ` Andre Silva
2020-01-08 15:49 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.