diff mbox

[1/8] virtio_get_byteswap: function for endian-ambivalent targets using virtio.

Message ID 1376294363-4650-2-git-send-email-rusty@rustcorp.com.au
State New
Headers show

Commit Message

Rusty Russell Aug. 12, 2013, 7:59 a.m. UTC 
virtio data structures are defined as "target endian", which assumes
that's a fixed value.  In fact, that actually means it's
platform-specific.

Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
a hook for little endian ppc.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 include/hw/virtio/virtio-access.h | 130 ++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h        |   2 +
 stubs/Makefile.objs               |   1 +
 stubs/virtio_get_byteswap.c       |   6 ++
 4 files changed, 139 insertions(+)
 create mode 100644 include/hw/virtio/virtio-access.h
 create mode 100644 stubs/virtio_get_byteswap.c

Comments

Benjamin Herrenschmidt Aug. 12, 2013, 9:28 a.m. UTC | #1 
On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's
> platform-specific.
> 
> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
> a hook for little endian ppc.

Ok, sorry if I missed a previous debate on that one but why do you do a
call-out to architecture specific stuff (that is not even inline) on
every access ?

If we consider that virtio byte order is global, can't you make it a
global that is *set* by the architecture rather than *polled* by
virtio ?

We have explicit knowledge of when our endianness change (we get the
hcall or a write to some SPR), we can call virtio *then* to adjust the
endianness rather than having a call-out to the platform on every
access.

Ben.
Peter Maydell Aug. 12, 2013, 9:39 a.m. UTC | #2 
On 12 August 2013 10:28, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> We have explicit knowledge of when our endianness change (we get the
> hcall or a write to some SPR), we can call virtio *then* to adjust the
> endianness rather than having a call-out to the platform on every
> access.

ARM doesn't -- I wouldn't expect changing the endianness of
exceptions via writing to the SCTLR to trap to the hypervisor
(and in any case it certainly won't result in a return to
userspace).

-- PMM
Benjamin Herrenschmidt Aug. 12, 2013, 9:43 a.m. UTC | #3 
On Mon, 2013-08-12 at 10:39 +0100, Peter Maydell wrote:
> On 12 August 2013 10:28, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > We have explicit knowledge of when our endianness change (we get the
> > hcall or a write to some SPR), we can call virtio *then* to adjust the
> > endianness rather than having a call-out to the platform on every
> > access.
> 
> ARM doesn't -- I wouldn't expect changing the endianness of
> exceptions via writing to the SCTLR to trap to the hypervisor
> (and in any case it certainly won't result in a return to
> userspace).

But don't you need to reconfigure the bridge (as per our previous
discussion) ? In that case you do need to call out to qemu ...

Cheers,
Ben.
Peter Maydell Aug. 12, 2013, 9:45 a.m. UTC | #4 
On 12 August 2013 10:43, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2013-08-12 at 10:39 +0100, Peter Maydell wrote:
>> ARM doesn't -- I wouldn't expect changing the endianness of
>> exceptions via writing to the SCTLR to trap to the hypervisor
>> (and in any case it certainly won't result in a return to
>> userspace).
>
> But don't you need to reconfigure the bridge (as per our previous
> discussion) ? In that case you do need to call out to qemu ...

Bridge? You've lost me, I'm afraid.

-- PMM
Benjamin Herrenschmidt Aug. 12, 2013, 9:50 a.m. UTC | #5 
On Mon, 2013-08-12 at 10:45 +0100, Peter Maydell wrote:
> On 12 August 2013 10:43, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Mon, 2013-08-12 at 10:39 +0100, Peter Maydell wrote:
> >> ARM doesn't -- I wouldn't expect changing the endianness of
> >> exceptions via writing to the SCTLR to trap to the hypervisor
> >> (and in any case it certainly won't result in a return to
> >> userspace).
> >
> > But don't you need to reconfigure the bridge (as per our previous
> > discussion) ? In that case you do need to call out to qemu ...
> 
> Bridge? You've lost me, I'm afraid.

I must be confused ... you mentioned in a previous discussion around
endianness that on some ARM cores at least, when changing the OS
endianness, you had to configure a different lane swapping in the bridge
to the the IO devices (AXI ?)

But I might be confusing with something else.

Ben.
Peter Maydell Aug. 12, 2013, 9:52 a.m. UTC | #6 
On 12 August 2013 10:50, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> I must be confused ... you mentioned in a previous discussion around
> endianness that on some ARM cores at least, when changing the OS
> endianness, you had to configure a different lane swapping in the bridge
> to the the IO devices (AXI ?)

No, that's just the implementation -- the bit in the control
register is effectively controlling whether there is byte lane
swapping in the part of the CPU which is the data path between
it and its bus to the outside world.

-- PMM
Benjamin Herrenschmidt Aug. 12, 2013, 9:56 a.m. UTC | #7 
On Mon, 2013-08-12 at 10:52 +0100, Peter Maydell wrote:
> On 12 August 2013 10:50, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > I must be confused ... you mentioned in a previous discussion around
> > endianness that on some ARM cores at least, when changing the OS
> > endianness, you had to configure a different lane swapping in the bridge
> > to the the IO devices (AXI ?)
> 
> No, that's just the implementation -- the bit in the control
> register is effectively controlling whether there is byte lane
> swapping in the part of the CPU which is the data path between
> it and its bus to the outside world.

I find it amazing that an OS can touch that without hitting the
hypervisor :-) Anyway, ok, we do need to poll from virtio then, but we
probably need to cache as well, no ?

When do you sample it in qemu ?

Cheers,
Ben.
Peter Maydell Aug. 12, 2013, 10:36 a.m. UTC | #8 
On 12 August 2013 10:56, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2013-08-12 at 10:52 +0100, Peter Maydell wrote:
>> On 12 August 2013 10:50, Benjamin Herrenschmidt
>> <benh@kernel.crashing.org> wrote:
>> > I must be confused ... you mentioned in a previous discussion around
>> > endianness that on some ARM cores at least, when changing the OS
>> > endianness, you had to configure a different lane swapping in the bridge
>> > to the the IO devices (AXI ?)
>>
>> No, that's just the implementation -- the bit in the control
>> register is effectively controlling whether there is byte lane
>> swapping in the part of the CPU which is the data path between
>> it and its bus to the outside world.
>
> I find it amazing that an OS can touch that without hitting the
> hypervisor :-)

It's no different to having a userspace process able to have
a different setting from the OS, really. (There is an equivalent
bit in another register that controls what endianness we
use if we trap to hyp mode.)

> Anyway, ok, we do need to poll from virtio then, but we
> probably need to cache as well, no ?
>
> When do you sample it in qemu ?

It's a bit theoretical at the moment since QEMU's ARM code
kind of assumes little endian. I would expect that at the
point when virtio was in an MMIO callback the CPUState
struct would have been updated via the usual sync process in
kvm_arch_get_registers().

-- PMM
Anthony Liguori Aug. 12, 2013, 12:56 p.m. UTC | #9 
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
>> virtio data structures are defined as "target endian", which assumes
>> that's a fixed value.  In fact, that actually means it's
>> platform-specific.
>> 
>> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
>> a hook for little endian ppc.
>
> Ok, sorry if I missed a previous debate on that one but why do you do a
> call-out to architecture specific stuff (that is not even inline) on
> every access ?

Let's focus on getting something merged.  Then we can muck around with
it down the road.

Having target-ppc call into virtio is a layering violation.  This
approach keeps the dependencies cleaner.

Regards,

Anthony Liguori

>
> If we consider that virtio byte order is global, can't you make it a
> global that is *set* by the architecture rather than *polled* by
> virtio ?
>
> We have explicit knowledge of when our endianness change (we get the
> hcall or a write to some SPR), we can call virtio *then* to adjust the
> endianness rather than having a call-out to the platform on every
> access.
>
> Ben.
Rusty Russell Aug. 13, 2013, 4:20 a.m. UTC | #10 
Anthony Liguori <anthony@codemonkey.ws> writes:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>
>> On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
>>> virtio data structures are defined as "target endian", which assumes
>>> that's a fixed value.  In fact, that actually means it's
>>> platform-specific.
>>> 
>>> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
>>> a hook for little endian ppc.
>>
>> Ok, sorry if I missed a previous debate on that one but why do you do a
>> call-out to architecture specific stuff (that is not even inline) on
>> every access ?

Anthony said he wanted it that way: my initial patch was more optimized.

> Let's focus on getting something merged.  Then we can muck around with
> it down the road.
>
> Having target-ppc call into virtio is a layering violation.  This
> approach keeps the dependencies cleaner.

We can have it call once (eg. when the first and storing the status
word) and store the result.

Cheers,
Rusty.
Benjamin Herrenschmidt Aug. 13, 2013, 5:30 a.m. UTC | #11 
On Tue, 2013-08-13 at 13:50 +0930, Rusty Russell wrote:
> We can have it call once (eg. when the first and storing the status
> word) and store the result.

And fail with kexec of a different endian kernel :-) Let's not bother
yet. Merge it and then we see if we can optimize.

Cheers,
Ben.
Rusty Russell Aug. 14, 2013, 12:03 a.m. UTC | #12 
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Tue, 2013-08-13 at 13:50 +0930, Rusty Russell wrote:
>> We can have it call once (eg. when the first and storing the status
>> word) and store the result.
>
> And fail with kexec of a different endian kernel :-) Let's not bother
> yet. Merge it and then we see if we can optimize.

Yeah, my code actually did it every status bye write, which
unintentionally solved this.

But agreed, let's let these patches stand...

Cheers,
Rusty.
Rusty Russell Sept. 6, 2013, 2:27 a.m. UTC | #13 
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Mon, 2013-08-12 at 17:29 +0930, Rusty Russell wrote:
>> virtio data structures are defined as "target endian", which assumes
>> that's a fixed value.  In fact, that actually means it's
>> platform-specific.
>> 
>> Hopefully the OASIS virtio 1.0 spec will fix this.  Meanwhile, create
>> a hook for little endian ppc.
>
> Ok, sorry if I missed a previous debate on that one but why do you do a
> call-out to architecture specific stuff (that is not even inline) on
> every access ?
>
> If we consider that virtio byte order is global, can't you make it a
> global that is *set* by the architecture rather than *polled* by
> virtio ?

OK, so after some more offline discussion it turns out these patches
won't really work reliably, since powerpc kvm doesn't reflect the
register into qemu.

Mikey N has promised me a KVM_GET_ONE_REG to get the register, and I'll
rework on top of that: we will query that whenever a device is reset
(which Linux does on every device init, so it captures the kexec case
too).

Cheers,
Rusty.
diff mbox

Patch

diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..01d7dd6
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,130 @@ 
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Rusty Russell   <rusty@au.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+#include "hw/virtio/virtio.h"
+
+static inline uint16_t virtio_lduw_phys(hwaddr pa)
+{
+    if (virtio_get_byteswap()) {
+        return bswap16(lduw_phys(pa));
+    }
+    return lduw_phys(pa);
+    
+}
+
+static inline uint32_t virtio_ldl_phys(hwaddr pa)
+{
+    if (virtio_get_byteswap()) {
+        return bswap32(ldl_phys(pa));
+    }
+    return ldl_phys(pa);
+}
+
+static inline uint64_t virtio_ldq_phys(hwaddr pa)
+{
+    if (virtio_get_byteswap()) {
+        return bswap64(ldq_phys(pa));
+    }
+    return ldq_phys(pa);
+}
+
+static inline void virtio_stw_phys(hwaddr pa, uint16_t value)
+{
+    if (virtio_get_byteswap()) {
+        stw_phys(pa, bswap16(value));
+    } else {
+        stw_phys(pa, value);
+    }
+}
+
+static inline void virtio_stl_phys(hwaddr pa, uint32_t value)
+{
+    if (virtio_get_byteswap()) {
+        stl_phys(pa, bswap32(value));
+    } else {
+        stl_phys(pa, value);
+    }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v)
+{
+    if (virtio_get_byteswap()) {
+        stw_p(ptr, bswap16(v));
+    } else {
+        stw_p(ptr, v);
+    }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v)
+{
+    if (virtio_get_byteswap()) {
+        stl_p(ptr, bswap32(v));
+    } else {
+        stl_p(ptr, v);
+    }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v)
+{
+    if (virtio_get_byteswap()) {
+        stq_p(ptr, bswap64(v));
+    } else {
+        stq_p(ptr, v);
+    }
+}
+
+static inline int virtio_lduw_p(const void *ptr)
+{
+    if (virtio_get_byteswap()) {
+        return bswap16(lduw_p(ptr));
+    } else {
+        return lduw_p(ptr);
+    }
+}
+
+static inline int virtio_ldl_p(const void *ptr)
+{
+    if (virtio_get_byteswap()) {
+        return bswap32(ldl_p(ptr));
+    } else {
+        return ldl_p(ptr);
+    }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr)
+{
+    if (virtio_get_byteswap()) {
+        return bswap64(ldq_p(ptr));
+    } else {
+        return ldq_p(ptr);
+    }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s)
+{
+    if (virtio_get_byteswap()) {
+        return bswap32(tswap32(s));
+    } else {
+        return tswap32(s);
+    }
+}
+
+static inline void virtio_tswap32s(uint32_t *s)
+{
+    tswap32s(s);
+    if (virtio_get_byteswap()) {
+        *s = bswap32(*s);
+    }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d7e9e0f..18ca725 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -248,4 +248,6 @@  void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
+
+extern bool virtio_get_byteswap(void);
 #endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f306cba..05f7bab 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -26,3 +26,4 @@  stub-obj-y += vm-stop.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += cpus.o
+stub-obj-y += virtio_get_byteswap.o
diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c
new file mode 100644
index 0000000..7cf764d
--- /dev/null
+++ b/stubs/virtio_get_byteswap.c
@@ -0,0 +1,6 @@ 
+#include "hw/virtio/virtio.h"
+
+bool virtio_get_byteswap(void)
+{
+    return false;
+}