* [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes
@ 2014-03-28 10:57 Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
` (8 more replies)
0 siblings, 9 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
Hi,
This serie addresses the comments made on v5. The main goal is to share
most of the code to support both the current legacy virtio and the yet
to come 1.0 implementations.
The changes since the last post are:
- introduce a per-device property to supersede the
evil virtio_byteswap global
- pass VirtIODevice to virtio memory accessors so they can use
the per-device property
- migration support
- fix SCSI event endianness issue in virtio-scsi
Thank you for your comments.
---
Greg
Greg Kurz (1):
virtio-9p: use virtio wrappers to access headers
Rusty Russell (7):
virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
virtio: allow byte swapping for vring and config access
virtio-net: use virtio wrappers to access headers
virtio-balloon: use virtio wrappers to access page frame numbers
virtio-blk: use virtio wrappers to access headers
virtio-scsi: use virtio wrappers to access headers
virtio-serial-bus: use virtio wrappers to access headers
hw/9pfs/virtio-9p-device.c | 3 +
hw/block/virtio-blk.c | 40 ++++++-----
hw/char/virtio-serial-bus.c | 39 ++++++----
hw/net/virtio-net.c | 17 +++--
hw/scsi/virtio-scsi.c | 38 +++++-----
hw/virtio/virtio-balloon.c | 4 +
hw/virtio/virtio.c | 98 +++++++++++++++-----------
include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++
include/hw/virtio/virtio.h | 3 +
stubs/Makefile.objs | 1
stubs/virtio_get_byteswap.c | 6 ++
11 files changed, 288 insertions(+), 100 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
create mode 100644 stubs/virtio_get_byteswap.c
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
@ 2014-03-28 10:57 ` Greg Kurz
2014-03-28 14:15 ` Thomas Huth
` (2 more replies)
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access Greg Kurz
` (7 subsequent siblings)
8 siblings, 3 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
virtio data structures are defined as "target endian", which assumes
that's a fixed value. In fact, that actually means it's platform-specific.
The OASIS virtio 1.0 spec will fix this, by making all little endian.
We need to support both implementations and we want to share as much code
as possible.
A good way to do it is to introduce a per-device boolean property to tell
memory accessors whether they should swap bytes or not. This flag should
be set at device reset time, because:
- endianness won't change while the device is in use, and if we reboot
into a different endianness, a new device reset will occur
- as suggested by Alexander Graf, we can keep all the logic to set the
property in a single place and share all the virtio memory accessors
between the two implementations
For legacy devices, we rely on a per-platform hook to set the flag. The
virtio 1.0 implementation will just have to add some more logic in
virtio_reset() instead of calling the hook:
if (vdev->legacy) {
vdev->needs_byteswap = virtio_legacy_get_byteswap();
} else {
#ifdef HOST_WORDS_BIGENDIAN
vdev->needs_byteswap = true;
#else
vdev->needs_byteswap = false;
#endif
}
The needs_byteswap flag is preserved accross migrations.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ fixed checkpatch.pl error with the virtio_byteswap initialisation,
ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device needs_byteswap flag,
add VirtIODevice * arg to virtio helpers,
rename virtio_get_byteswap to virtio_legacy_get_byteswap,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/virtio/virtio.c | 5 +
include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++
include/hw/virtio/virtio.h | 3 +
stubs/Makefile.objs | 1
stubs/virtio_get_byteswap.c | 6 ++
5 files changed, 154 insertions(+)
create mode 100644 include/hw/virtio/virtio-access.h
create mode 100644 stubs/virtio_get_byteswap.c
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index aeabf3a..24b565f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -19,6 +19,7 @@
#include "hw/virtio/virtio.h"
#include "qemu/atomic.h"
#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
/*
* The alignment to use between consumer and producer parts of vring.
@@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
virtio_set_status(vdev, 0);
+ vdev->needs_byteswap = virtio_legacy_get_byteswap();
+
if (k->reset) {
k->reset(vdev);
}
@@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
qemu_put_8s(f, &vdev->status);
qemu_put_8s(f, &vdev->isr);
+ qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
qemu_put_be16s(f, &vdev->queue_sel);
qemu_put_be32s(f, &vdev->guest_features);
qemu_put_be32(f, vdev->config_len);
@@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
qemu_get_8s(f, &vdev->status);
qemu_get_8s(f, &vdev->isr);
+ qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
qemu_get_be16s(f, &vdev->queue_sel);
qemu_get_be32s(f, &features);
diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
new file mode 100644
index 0000000..70dd1e2
--- /dev/null
+++ b/include/hw/virtio/virtio-access.h
@@ -0,0 +1,139 @@
+/*
+ * Virtio Accessor Support: In case your target can change endian.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ * Rusty Russell <rusty@au.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+#ifndef _QEMU_VIRTIO_ACCESS_H
+#define _QEMU_VIRTIO_ACCESS_H
+#include "hw/virtio/virtio.h"
+
+static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap16(lduw_phys(as, pa));
+ }
+ return lduw_phys(as, pa);
+}
+
+static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap32(ldl_phys(as, pa));
+ }
+ return ldl_phys(as, pa);
+}
+
+static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap64(ldq_phys(as, pa));
+ }
+ return ldq_phys(as, pa);
+}
+
+static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ stw_phys(as, pa, bswap16(value));
+ } else {
+ stw_phys(as, pa, value);
+ }
+}
+
+static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ stl_phys(as, pa, bswap32(value));
+ } else {
+ stl_phys(as, pa, value);
+ }
+}
+
+static inline void virtio_stw_p(void *ptr, uint16_t v,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ stw_p(ptr, bswap16(v));
+ } else {
+ stw_p(ptr, v);
+ }
+}
+
+static inline void virtio_stl_p(void *ptr, uint32_t v,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ stl_p(ptr, bswap32(v));
+ } else {
+ stl_p(ptr, v);
+ }
+}
+
+static inline void virtio_stq_p(void *ptr, uint64_t v,
+ struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ stq_p(ptr, bswap64(v));
+ } else {
+ stq_p(ptr, v);
+ }
+}
+
+static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap16(lduw_p(ptr));
+ } else {
+ return lduw_p(ptr);
+ }
+}
+
+static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap32(ldl_p(ptr));
+ } else {
+ return ldl_p(ptr);
+ }
+}
+
+static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap64(ldq_p(ptr));
+ } else {
+ return ldq_p(ptr);
+ }
+}
+
+static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev)
+{
+ if (vdev->needs_byteswap) {
+ return bswap32(tswap32(s));
+ } else {
+ return tswap32(s);
+ }
+}
+
+static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev)
+{
+ tswap32s(s);
+ if (vdev->needs_byteswap) {
+ *s = bswap32(*s);
+ }
+}
+#endif /* _QEMU_VIRTIO_ACCESS_H */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 3e54e90..3595da5 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -121,6 +121,7 @@ struct VirtIODevice
bool vm_running;
VMChangeStateEntry *vmstate;
char *bus_name;
+ bool needs_byteswap;
};
typedef struct VirtioDeviceClass {
@@ -253,4 +254,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_legacy_get_byteswap(void);
#endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 5ed1d38..a13381a 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -30,3 +30,4 @@ stub-obj-y += vmstate.o
stub-obj-$(CONFIG_WIN32) += fd-register.o
stub-obj-y += cpus.o
stub-obj-y += kvm.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..28af5e3
--- /dev/null
+++ b/stubs/virtio_get_byteswap.c
@@ -0,0 +1,6 @@
+#include "hw/virtio/virtio.h"
+
+bool virtio_legacy_get_byteswap(void)
+{
+ return false;
+}
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
@ 2014-03-28 10:57 ` Greg Kurz
2014-03-28 16:07 ` Thomas Huth
2014-03-31 16:24 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers Greg Kurz
` (6 subsequent siblings)
8 siblings, 2 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
This is based on a simpler patch by Anthony Liguouri, which only handled
the vring accesses. We also need some drivers to access these helpers,
eg. for data which contains headers.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ ldq_phys() API change,
use per-device needs_byteswap flag,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/virtio/virtio.c | 93 ++++++++++++++++++++++++++++++----------------------
1 file changed, 53 insertions(+), 40 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 24b565f..1877b46 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
vq->vring.align);
}
-static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
+static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
+ struct VirtIODevice *vdev)
{
hwaddr pa;
pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
- return ldq_phys(&address_space_memory, pa);
+ return virtio_ldq_phys(&address_space_memory, pa, vdev);
}
-static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
+static inline uint32_t vring_desc_len(hwaddr desc_pa, int i,
+ struct VirtIODevice *vdev)
{
hwaddr pa;
pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
- return ldl_phys(&address_space_memory, pa);
+ return virtio_ldl_phys(&address_space_memory, pa, vdev);
}
-static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
+static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i,
+ struct VirtIODevice *vdev)
{
hwaddr pa;
pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
- return lduw_phys(&address_space_memory, pa);
+ return virtio_lduw_phys(&address_space_memory, pa, vdev);
}
-static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
+static inline uint16_t vring_desc_next(hwaddr desc_pa, int i,
+ struct VirtIODevice *vdev)
{
hwaddr pa;
pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
- return lduw_phys(&address_space_memory, pa);
+ return virtio_lduw_phys(&address_space_memory, pa, vdev);
}
static inline uint16_t vring_avail_flags(VirtQueue *vq)
{
hwaddr pa;
pa = vq->vring.avail + offsetof(VRingAvail, flags);
- return lduw_phys(&address_space_memory, pa);
+ return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
}
static inline uint16_t vring_avail_idx(VirtQueue *vq)
{
hwaddr pa;
pa = vq->vring.avail + offsetof(VRingAvail, idx);
- return lduw_phys(&address_space_memory, pa);
+ return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
}
static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
{
hwaddr pa;
pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
- return lduw_phys(&address_space_memory, pa);
+ return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
}
static inline uint16_t vring_used_event(VirtQueue *vq)
@@ -160,44 +164,46 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
{
hwaddr pa;
pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
- stl_phys(&address_space_memory, pa, val);
+ virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
}
static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
{
hwaddr pa;
pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
- stl_phys(&address_space_memory, pa, val);
+ virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
}
static uint16_t vring_used_idx(VirtQueue *vq)
{
hwaddr pa;
pa = vq->vring.used + offsetof(VRingUsed, idx);
- return lduw_phys(&address_space_memory, pa);
+ return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
}
static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
{
hwaddr pa;
pa = vq->vring.used + offsetof(VRingUsed, idx);
- stw_phys(&address_space_memory, pa, val);
+ virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
}
static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
{
hwaddr pa;
pa = vq->vring.used + offsetof(VRingUsed, flags);
- stw_phys(&address_space_memory,
- pa, lduw_phys(&address_space_memory, pa) | mask);
+ virtio_stw_phys(&address_space_memory, pa,
+ virtio_lduw_phys(&address_space_memory, pa,
+ vq->vdev) | mask, vq->vdev);
}
static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
{
hwaddr pa;
pa = vq->vring.used + offsetof(VRingUsed, flags);
- stw_phys(&address_space_memory,
- pa, lduw_phys(&address_space_memory, pa) & ~mask);
+ virtio_stw_phys(&address_space_memory, pa,
+ virtio_lduw_phys(&address_space_memory, pa,
+ vq->vdev) & ~mask, vq->vdev);
}
static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
@@ -207,7 +213,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
return;
}
pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
- stw_phys(&address_space_memory, pa, val);
+ virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
}
void virtio_queue_set_notification(VirtQueue *vq, int enable)
@@ -325,16 +331,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
}
static unsigned virtqueue_next_desc(hwaddr desc_pa,
- unsigned int i, unsigned int max)
+ unsigned int i, unsigned int max,
+ struct VirtIODevice *vdev)
{
unsigned int next;
/* If this descriptor says it doesn't chain, we're done. */
- if (!(vring_desc_flags(desc_pa, i) & VRING_DESC_F_NEXT))
+ if (!(vring_desc_flags(desc_pa, i, vdev) & VRING_DESC_F_NEXT)) {
return max;
+ }
/* Check they're not leading us off end of descriptors. */
- next = vring_desc_next(desc_pa, i);
+ next = vring_desc_next(desc_pa, i, vdev);
/* Make sure compiler knows to grab that: we don't want it changing! */
smp_wmb();
@@ -366,8 +374,9 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
i = virtqueue_get_head(vq, idx++);
desc_pa = vq->vring.desc;
- if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
- if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
+ if (vring_desc_flags(desc_pa, i, vq->vdev)
+ & VRING_DESC_F_INDIRECT) {
+ if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
error_report("Invalid size for indirect buffer table");
exit(1);
}
@@ -380,8 +389,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
/* loop over the indirect descriptor table */
indirect = 1;
- max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
- desc_pa = vring_desc_addr(desc_pa, i);
+ max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
+ desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
num_bufs = i = 0;
}
@@ -392,15 +401,17 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
exit(1);
}
- if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
- in_total += vring_desc_len(desc_pa, i);
+ if (vring_desc_flags(desc_pa, i, vq->vdev)
+ & VRING_DESC_F_WRITE) {
+ in_total += vring_desc_len(desc_pa, i, vq->vdev);
} else {
- out_total += vring_desc_len(desc_pa, i);
+ out_total += vring_desc_len(desc_pa, i, vq->vdev);
}
if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
goto done;
}
- } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
+ } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev))
+ != max);
if (!indirect)
total_bufs = num_bufs;
@@ -459,15 +470,15 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
vring_avail_event(vq, vring_avail_idx(vq));
}
- if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
- if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
+ if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_INDIRECT) {
+ if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
error_report("Invalid size for indirect buffer table");
exit(1);
}
/* loop over the indirect descriptor table */
- max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
- desc_pa = vring_desc_addr(desc_pa, i);
+ max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
+ desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
i = 0;
}
@@ -475,30 +486,32 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
do {
struct iovec *sg;
- if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
+ if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_WRITE) {
if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
error_report("Too many write descriptors in indirect table");
exit(1);
}
- elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
+ elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i,
+ vq->vdev);
sg = &elem->in_sg[elem->in_num++];
} else {
if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
error_report("Too many read descriptors in indirect table");
exit(1);
}
- elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
+ elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i,
+ vq->vdev);
sg = &elem->out_sg[elem->out_num++];
}
- sg->iov_len = vring_desc_len(desc_pa, i);
+ sg->iov_len = vring_desc_len(desc_pa, i, vq->vdev);
/* If we've got too many, that implies a descriptor loop. */
if ((elem->in_num + elem->out_num) > max) {
error_report("Looped descriptor");
exit(1);
}
- } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
+ } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev)) != max);
/* Now map what we have collected */
virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access Greg Kurz
@ 2014-03-28 10:57 ` Greg Kurz
2014-03-31 16:28 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers Greg Kurz
` (5 subsequent siblings)
8 siblings, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/net/virtio-net.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 439477b..eb89f48 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -23,6 +23,7 @@
#include "hw/virtio/virtio-bus.h"
#include "qapi/qmp/qjson.h"
#include "monitor/monitor.h"
+#include "hw/virtio/virtio-access.h"
#define VIRTIO_NET_VM_VERSION 11
@@ -72,8 +73,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
VirtIONet *n = VIRTIO_NET(vdev);
struct virtio_net_config netcfg;
- stw_p(&netcfg.status, n->status);
- stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
+ virtio_stw_p(&netcfg.status, n->status, vdev);
+ virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues, vdev);
memcpy(netcfg.mac, n->mac, ETH_ALEN);
memcpy(config, &netcfg, n->config_size);
}
@@ -614,6 +615,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
struct virtio_net_ctrl_mac mac_data;
size_t s;
NetClientState *nc = qemu_get_queue(n->nic);
+ VirtIODevice *vdev = VIRTIO_DEVICE(n);
if (cmd == VIRTIO_NET_CTRL_MAC_ADDR_SET) {
if (iov_size(iov, iov_cnt) != sizeof(n->mac)) {
@@ -639,7 +641,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
sizeof(mac_data.entries));
- mac_data.entries = ldl_p(&mac_data.entries);
+ mac_data.entries = virtio_ldl_p(&mac_data.entries, vdev);
if (s != sizeof(mac_data.entries)) {
goto error;
}
@@ -666,7 +668,7 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t cmd,
s = iov_to_buf(iov, iov_cnt, 0, &mac_data.entries,
sizeof(mac_data.entries));
- mac_data.entries = ldl_p(&mac_data.entries);
+ mac_data.entries = virtio_ldl_p(&mac_data.entries, vdev);
if (s != sizeof(mac_data.entries)) {
goto error;
}
@@ -709,9 +711,10 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
uint16_t vid;
size_t s;
NetClientState *nc = qemu_get_queue(n->nic);
+ VirtIODevice *vdev = VIRTIO_DEVICE(n);
s = iov_to_buf(iov, iov_cnt, 0, &vid, sizeof(vid));
- vid = lduw_p(&vid);
+ vid = virtio_lduw_p(&vid, vdev);
if (s != sizeof(vid)) {
return VIRTIO_NET_ERR;
}
@@ -748,7 +751,7 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
return VIRTIO_NET_ERR;
}
- queues = lduw_p(&mq.virtqueue_pairs);
+ queues = virtio_lduw_p(&mq.virtqueue_pairs, vdev);
if (queues < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
queues > VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX ||
@@ -1047,7 +1050,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
}
if (mhdr_cnt) {
- stw_p(&mhdr.num_buffers, i);
+ virtio_stw_p(&mhdr.num_buffers, i, vdev);
iov_from_buf(mhdr_sg, mhdr_cnt,
0,
&mhdr.num_buffers, sizeof mhdr.num_buffers);
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
` (2 preceding siblings ...)
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers Greg Kurz
@ 2014-03-28 10:57 ` Greg Kurz
2014-03-31 16:30 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers Greg Kurz
` (4 subsequent siblings)
8 siblings, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/virtio/virtio-balloon.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index a470a0b..e0b31ba 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -30,6 +30,7 @@
#endif
#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
static void balloon_page(void *addr, int deflate)
{
@@ -192,7 +193,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
ram_addr_t pa;
ram_addr_t addr;
- pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
+ pa = (ram_addr_t)virtio_ldl_p(&pfn,
+ vdev) << VIRTIO_BALLOON_PFN_SHIFT;
offset += 4;
/* FIXME: remove get_system_memory(), but how? */
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
` (3 preceding siblings ...)
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers Greg Kurz
@ 2014-03-28 10:57 ` Greg Kurz
2014-03-31 16:31 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 6/8] virtio-scsi: " Greg Kurz
` (3 subsequent siblings)
8 siblings, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/block/virtio-blk.c | 40 +++++++++++++++++++++++-----------------
1 file changed, 23 insertions(+), 17 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8a568e5..c652ff2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -26,6 +26,7 @@
# include <scsi/sg.h>
#endif
#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
typedef struct VirtIOBlockReq
{
@@ -77,7 +78,9 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
trace_virtio_blk_rw_complete(req, ret);
if (ret) {
- bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
+ bool is_read =
+ !(virtio_ldl_p(&req->out->type,
+ VIRTIO_DEVICE(req->dev)) & VIRTIO_BLK_T_OUT);
if (virtio_blk_handle_rw_error(req, -ret, is_read))
return;
}
@@ -224,12 +227,13 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
hdr.status = CHECK_CONDITION;
}
- stl_p(&req->scsi->errors,
- hdr.status | (hdr.msg_status << 8) |
- (hdr.host_status << 16) | (hdr.driver_status << 24));
- stl_p(&req->scsi->residual, hdr.resid);
- stl_p(&req->scsi->sense_len, hdr.sb_len_wr);
- stl_p(&req->scsi->data_len, hdr.dxfer_len);
+ virtio_stl_p(&req->scsi->errors,
+ hdr.status | (hdr.msg_status << 8) |
+ (hdr.host_status << 16) | (hdr.driver_status << 24),
+ VIRTIO_DEVICE(req->dev));
+ virtio_stl_p(&req->scsi->residual, hdr.resid, VIRTIO_DEVICE(req->dev));
+ virtio_stl_p(&req->scsi->sense_len, hdr.sb_len_wr, VIRTIO_DEVICE(req->dev));
+ virtio_stl_p(&req->scsi->data_len, hdr.dxfer_len, VIRTIO_DEVICE(req->dev));
virtio_blk_req_complete(req, status);
g_free(req);
@@ -240,7 +244,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
fail:
/* Just put anything nonzero so that the ioctl fails in the guest. */
- stl_p(&req->scsi->errors, 255);
+ virtio_stl_p(&req->scsi->errors, 255, VIRTIO_DEVICE(req->dev));
virtio_blk_req_complete(req, status);
g_free(req);
}
@@ -286,7 +290,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
BlockRequest *blkreq;
uint64_t sector;
- sector = ldq_p(&req->out->sector);
+ sector = virtio_ldq_p(&req->out->sector, VIRTIO_DEVICE(req->dev));
bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
@@ -320,7 +324,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
{
uint64_t sector;
- sector = ldq_p(&req->out->sector);
+ sector = virtio_ldq_p(&req->out->sector, VIRTIO_DEVICE(req->dev));
bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
@@ -358,7 +362,7 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
req->out = (void *)req->elem.out_sg[0].iov_base;
req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
- type = ldl_p(&req->out->type);
+ type = virtio_ldl_p(&req->out->type, VIRTIO_DEVICE(req->dev));
if (type & VIRTIO_BLK_T_FLUSH) {
virtio_blk_handle_flush(req, mrb);
@@ -487,12 +491,14 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
bdrv_get_geometry(s->bs, &capacity);
memset(&blkcfg, 0, sizeof(blkcfg));
- stq_raw(&blkcfg.capacity, capacity);
- stl_raw(&blkcfg.seg_max, 128 - 2);
- stw_raw(&blkcfg.cylinders, s->conf->cyls);
- stl_raw(&blkcfg.blk_size, blk_size);
- stw_raw(&blkcfg.min_io_size, s->conf->min_io_size / blk_size);
- stw_raw(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size);
+ virtio_stq_p(&blkcfg.capacity, capacity, vdev);
+ virtio_stl_p(&blkcfg.seg_max, 128 - 2, vdev);
+ virtio_stw_p(&blkcfg.cylinders, s->conf->cyls, vdev);
+ virtio_stl_p(&blkcfg.blk_size, blk_size, vdev);
+ virtio_stw_p(&blkcfg.min_io_size, s->conf->min_io_size / blk_size,
+ vdev);
+ virtio_stw_p(&blkcfg.opt_io_size, s->conf->opt_io_size / blk_size,
+ vdev);
blkcfg.heads = s->conf->heads;
/*
* We must ensure that the block device capacity is a multiple of
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
` (4 preceding siblings ...)
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers Greg Kurz
@ 2014-03-28 10:57 ` Greg Kurz
2014-03-28 17:13 ` Greg Kurz
2014-03-31 16:34 ` Alexander Graf
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: " Greg Kurz
` (2 subsequent siblings)
8 siblings, 2 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:57 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
fix missing tswap32 in virtio_scsi_push_event(),
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/scsi/virtio-scsi.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b0d7517..20d326e 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -18,6 +18,7 @@
#include <hw/scsi/scsi.h>
#include <block/scsi.h>
#include <hw/virtio/virtio-bus.h>
+#include "hw/virtio/virtio-access.h"
typedef struct VirtIOSCSIReq {
VirtIOSCSI *dev;
@@ -315,12 +316,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
req->resp.cmd->response = VIRTIO_SCSI_S_OK;
req->resp.cmd->status = status;
if (req->resp.cmd->status == GOOD) {
- req->resp.cmd->resid = tswap32(resid);
+ req->resp.cmd->resid = virtio_tswap32(resid, VIRTIO_DEVICE(s));
} else {
req->resp.cmd->resid = 0;
sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
vs->sense_size);
- req->resp.cmd->sense_len = tswap32(sense_len);
+ req->resp.cmd->sense_len = virtio_tswap32(sense_len, VIRTIO_DEVICE(s));
}
virtio_scsi_complete_req(req);
}
@@ -416,16 +417,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
- stl_raw(&scsiconf->num_queues, s->conf.num_queues);
- stl_raw(&scsiconf->seg_max, 128 - 2);
- stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
- stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
- stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
- stl_raw(&scsiconf->sense_size, s->sense_size);
- stl_raw(&scsiconf->cdb_size, s->cdb_size);
- stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
- stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
- stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
+ virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues, vdev);
+ virtio_stl_p(&scsiconf->seg_max, 128 - 2, vdev);
+ virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors, vdev);
+ virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun, vdev);
+ virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent), vdev);
+ virtio_stl_p(&scsiconf->sense_size, s->sense_size, vdev);
+ virtio_stl_p(&scsiconf->cdb_size, s->cdb_size, vdev);
+ virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL, vdev);
+ virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET, vdev);
+ virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN, vdev);
}
static void virtio_scsi_set_config(VirtIODevice *vdev,
@@ -434,14 +435,15 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
- if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
- (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
+ if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size,
+ vdev) >= 65536 ||
+ (uint32_t) virtio_ldl_p(&scsiconf->cdb_size, vdev) >= 256) {
error_report("bad data written to virtio-scsi configuration space");
exit(1);
}
- vs->sense_size = ldl_raw(&scsiconf->sense_size);
- vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
+ vs->sense_size = virtio_ldl_p(&scsiconf->sense_size, vdev);
+ vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size, vdev);
}
static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
@@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
evt = req->resp.event;
memset(evt, 0, sizeof(VirtIOSCSIEvent));
- evt->event = event;
- evt->reason = reason;
+ evt->event = virtio_tswap32(event);
+ evt->reason = virtio_tswap32(reason);
if (!dev) {
assert(event == VIRTIO_SCSI_T_EVENTS_MISSED);
} else {
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: use virtio wrappers to access headers
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
` (5 preceding siblings ...)
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 6/8] virtio-scsi: " Greg Kurz
@ 2014-03-28 10:58 ` Greg Kurz
2014-03-31 17:01 ` Alexander Graf
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 8/8] virtio-9p: " Greg Kurz
2014-03-28 11:22 ` [Qemu-devel] [PATCH v4] target-ppc: ppc64 target's virtio can be either endian Greg Kurz
8 siblings, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:58 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
From: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
[ use per-device needs_byteswap flag,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/char/virtio-serial-bus.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..7a84b2a 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -24,6 +24,7 @@
#include "hw/sysbus.h"
#include "trace.h"
#include "hw/virtio/virtio-serial.h"
+#include "hw/virtio/virtio-access.h"
static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
{
@@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
{
struct virtio_console_control cpkt;
- stl_p(&cpkt.id, port_id);
- stw_p(&cpkt.event, event);
- stw_p(&cpkt.value, value);
+ virtio_stl_p(&cpkt.id, port_id, VIRTIO_DEVICE(vser));
+ virtio_stw_p(&cpkt.event, event, VIRTIO_DEVICE(vser));
+ virtio_stw_p(&cpkt.value, value, VIRTIO_DEVICE(vser));
trace_virtio_serial_send_control_event(port_id, event, value);
return send_control_msg(vser, &cpkt, sizeof(cpkt));
@@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
return;
}
- cpkt.event = lduw_p(&gcpkt->event);
- cpkt.value = lduw_p(&gcpkt->value);
+ cpkt.event = virtio_lduw_p(&gcpkt->event, VIRTIO_DEVICE(vser));
+ cpkt.value = virtio_lduw_p(&gcpkt->value, VIRTIO_DEVICE(vser));
trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
@@ -312,10 +313,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
return;
}
- port = find_port_by_id(vser, ldl_p(&gcpkt->id));
+ port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id, VIRTIO_DEVICE(vser)));
if (!port) {
error_report("virtio-serial-bus: Unexpected port id %u for device %s",
- ldl_p(&gcpkt->id), vser->bus.qbus.name);
+ virtio_ldl_p(&gcpkt->id, VIRTIO_DEVICE(vser)),
+ vser->bus.qbus.name);
return;
}
@@ -342,9 +344,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
}
if (port->name) {
- stl_p(&cpkt.id, port->id);
- stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
- stw_p(&cpkt.value, 1);
+ virtio_stl_p(&cpkt.id, port->id, VIRTIO_DEVICE(vser));
+ virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME,
+ VIRTIO_DEVICE(vser));
+ virtio_stw_p(&cpkt.value, 1, VIRTIO_DEVICE(vser));
buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
buffer = g_malloc(buffer_len);
@@ -536,7 +539,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
qemu_put_be32s(f, &s->config.max_nr_ports);
/* The ports map */
- max_nr_ports = tswap32(s->config.max_nr_ports);
+ max_nr_ports = virtio_tswap32(s->config.max_nr_ports, VIRTIO_DEVICE(s));
for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
qemu_put_be32s(f, &s->ports_map[i]);
}
@@ -690,8 +693,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
qemu_get_be16s(f, &s->config.rows);
qemu_get_be32s(f, &max_nr_ports);
- tswap32s(&max_nr_ports);
- if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
+ virtio_tswap32s(&max_nr_ports, VIRTIO_DEVICE(s));
+ if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports,
+ VIRTIO_DEVICE(s))) {
/* Source could have had more ports than us. Fail migration. */
return -EINVAL;
}
@@ -760,7 +764,8 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
{
unsigned int i, max_nr_ports;
- max_nr_ports = tswap32(vser->config.max_nr_ports);
+ max_nr_ports = virtio_tswap32(vser->config.max_nr_ports,
+ VIRTIO_DEVICE(vser));
for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
uint32_t map, bit;
@@ -848,7 +853,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
}
}
- max_nr_ports = tswap32(port->vser->config.max_nr_ports);
+ max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports,
+ VIRTIO_DEVICE(port->vser));
if (port->id >= max_nr_ports) {
error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
"max. allowed: %u", max_nr_ports - 1);
@@ -949,7 +955,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
}
- vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
+ vser->config.max_nr_ports =
+ virtio_tswap32(vser->serial.max_virtserial_ports, VIRTIO_DEVICE(vser));
vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
* sizeof(vser->ports_map[0]));
/*
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v6 8/8] virtio-9p: use virtio wrappers to access headers
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
` (6 preceding siblings ...)
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: " Greg Kurz
@ 2014-03-28 10:58 ` Greg Kurz
2014-03-28 11:22 ` [Qemu-devel] [PATCH v4] target-ppc: ppc64 target's virtio can be either endian Greg Kurz
8 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 10:58 UTC (permalink / raw)
To: afaerber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
hw/9pfs/virtio-9p-device.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index 15a4983..c758500 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -19,6 +19,7 @@
#include "fsdev/qemu-fsdev.h"
#include "virtio-9p-xattr.h"
#include "virtio-9p-coth.h"
+#include "hw/virtio/virtio-access.h"
static uint32_t virtio_9p_get_features(VirtIODevice *vdev, uint32_t features)
{
@@ -34,7 +35,7 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)
len = strlen(s->tag);
cfg = g_malloc0(sizeof(struct virtio_9p_config) + len);
- stw_raw(&cfg->tag_len, len);
+ virtio_stw_p(&cfg->tag_len, len, vdev);
/* We don't copy the terminating null to config space */
memcpy(cfg->tag, s->tag, len);
memcpy(config, cfg, s->config_size);
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v4] target-ppc: ppc64 target's virtio can be either endian
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
` (7 preceding siblings ...)
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 8/8] virtio-9p: " Greg Kurz
@ 2014-03-28 11:22 ` Greg Kurz
8 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 11:22 UTC (permalink / raw)
To: afaerber, agraf
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, amit.shah, qemu-ppc, aneesh.kumar, stefanha,
cornelia.huck, pbonzini, anthony
We base it on the OS endian, as reflected by the endianness of the
interrupt vectors (handled through the ILE bit in the LPCR register).
Using first_cpu to fetch the registers from KVM may look arbitrary
and awkward, but it is okay because KVM sets/unsets the ILE bit on
all CPUs.
Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
[ rename virtio_get_byteswap to virtio_legacy_get_byteswap,
support both LE and BE host,
Greg Kurz <gkurz@linux.vnet.ibm.com> ]
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
---
Alex,
Welcome back ! :)
Changes since v3:
- rename the helper according to v6 of the main LE virtio patchset
- prepare to support LE ppc64 host
- tentatively fixed the list of changes embeded in SoB lines
Cheers.
--
Greg
target-ppc/misc_helper.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/target-ppc/misc_helper.c b/target-ppc/misc_helper.c
index 2eb2fa6..4dbb274 100644
--- a/target-ppc/misc_helper.c
+++ b/target-ppc/misc_helper.c
@@ -20,6 +20,8 @@
#include "helper.h"
#include "helper_regs.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"
/*****************************************************************************/
/* SPR accesses */
@@ -120,3 +122,15 @@ void ppc_store_msr(CPUPPCState *env, target_ulong value)
{
hreg_store_msr(env, value, 0);
}
+
+bool virtio_legacy_get_byteswap(void)
+{
+ PowerPCCPU *cp = POWERPC_CPU(first_cpu);
+ CPUPPCState *env = &cp->env;
+ bool ile = env->spr[SPR_LPCR] & LPCR_ILE;
+#ifdef HOST_WORDS_BIGENDIAN
+ return ile;
+#else
+ return !ile;
+#endif
+}
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
@ 2014-03-28 14:15 ` Thomas Huth
2014-03-28 15:40 ` Greg Kurz
2014-03-28 17:59 ` Andreas Färber
2014-03-31 14:50 ` Alexander Graf
2 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2014-03-28 14:15 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, anthony, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, afaerber
On Fri, 28 Mar 2014 11:57:17 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value. In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
>
> We need to support both implementations and we want to share as much code
> as possible.
>
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
> into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
> property in a single place and share all the virtio memory accessors
> between the two implementations
>
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
>
> if (vdev->legacy) {
> vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
> vdev->needs_byteswap = true;
> #else
> vdev->needs_byteswap = false;
> #endif
> }
>
> The needs_byteswap flag is preserved accross migrations.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> ldq_phys() API change,
> relicensed virtio-access.h to GPLv2+ on Rusty's request,
> introduce a per-device needs_byteswap flag,
> add VirtIODevice * arg to virtio helpers,
> rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/virtio/virtio.c | 5 +
> include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++
> include/hw/virtio/virtio.h | 3 +
> stubs/Makefile.objs | 1
> stubs/virtio_get_byteswap.c | 6 ++
> 5 files changed, 154 insertions(+)
> create mode 100644 include/hw/virtio/virtio-access.h
> create mode 100644 stubs/virtio_get_byteswap.c
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index aeabf3a..24b565f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -19,6 +19,7 @@
> #include "hw/virtio/virtio.h"
> #include "qemu/atomic.h"
> #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>
> /*
> * The alignment to use between consumer and producer parts of vring.
> @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
>
> virtio_set_status(vdev, 0);
>
> + vdev->needs_byteswap = virtio_legacy_get_byteswap();
> +
> if (k->reset) {
> k->reset(vdev);
> }
> @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>
> qemu_put_8s(f, &vdev->status);
> qemu_put_8s(f, &vdev->isr);
> + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
> qemu_put_be16s(f, &vdev->queue_sel);
> qemu_put_be32s(f, &vdev->guest_features);
> qemu_put_be32(f, vdev->config_len);
> @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>
> qemu_get_8s(f, &vdev->status);
> qemu_get_8s(f, &vdev->isr);
> + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
> qemu_get_be16s(f, &vdev->queue_sel);
> qemu_get_be32s(f, &features);
Two remarks here:
1) You've declared needs_byteswap as "bool", but in above code you
assume that the "bool" type is implemented as a single byte. That's
most likely true on most system, but AFAIK it's specific to the
compiler. So for portable code, I think you should either change the
type of needs_byteswap or you should use a temporary uint8_t variable
here instead.
2) You're changing the layout of the saved data ... don't you also have
to increase the version numbers in that case, too? (e.g. change the
version id for the register_savevm call in virtio-blk.c, virtio-net.c,
etc.)?
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-28 14:15 ` Thomas Huth
@ 2014-03-28 15:40 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 15:40 UTC (permalink / raw)
To: Thomas Huth
Cc: kwolf, peter.maydell, stefanha, mst, marc.zyngier, rusty, agraf,
qemu-devel, anthony, cornelia.huck, pbonzini, afaerber
On Fri, 28 Mar 2014 15:15:46 +0100
Thomas Huth <thuth@linux.vnet.ibm.com> wrote:
> On Fri, 28 Mar 2014 11:57:17 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > virtio data structures are defined as "target endian", which assumes
> > that's a fixed value. In fact, that actually means it's platform-specific.
> > The OASIS virtio 1.0 spec will fix this, by making all little endian.
> >
> > We need to support both implementations and we want to share as much code
> > as possible.
> >
> > A good way to do it is to introduce a per-device boolean property to tell
> > memory accessors whether they should swap bytes or not. This flag should
> > be set at device reset time, because:
> > - endianness won't change while the device is in use, and if we reboot
> > into a different endianness, a new device reset will occur
> > - as suggested by Alexander Graf, we can keep all the logic to set the
> > property in a single place and share all the virtio memory accessors
> > between the two implementations
> >
> > For legacy devices, we rely on a per-platform hook to set the flag. The
> > virtio 1.0 implementation will just have to add some more logic in
> > virtio_reset() instead of calling the hook:
> >
> > if (vdev->legacy) {
> > vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > } else {
> > #ifdef HOST_WORDS_BIGENDIAN
> > vdev->needs_byteswap = true;
> > #else
> > vdev->needs_byteswap = false;
> > #endif
> > }
> >
> > The needs_byteswap flag is preserved accross migrations.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device needs_byteswap flag,
> > add VirtIODevice * arg to virtio helpers,
> > rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> > Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > hw/virtio/virtio.c | 5 +
> > include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++
> > include/hw/virtio/virtio.h | 3 +
> > stubs/Makefile.objs | 1
> > stubs/virtio_get_byteswap.c | 6 ++
> > 5 files changed, 154 insertions(+)
> > create mode 100644 include/hw/virtio/virtio-access.h
> > create mode 100644 stubs/virtio_get_byteswap.c
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index aeabf3a..24b565f 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -19,6 +19,7 @@
> > #include "hw/virtio/virtio.h"
> > #include "qemu/atomic.h"
> > #include "hw/virtio/virtio-bus.h"
> > +#include "hw/virtio/virtio-access.h"
> >
> > /*
> > * The alignment to use between consumer and producer parts of vring.
> > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
> >
> > virtio_set_status(vdev, 0);
> >
> > + vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > +
> > if (k->reset) {
> > k->reset(vdev);
> > }
> > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >
> > qemu_put_8s(f, &vdev->status);
> > qemu_put_8s(f, &vdev->isr);
> > + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
> > qemu_put_be16s(f, &vdev->queue_sel);
> > qemu_put_be32s(f, &vdev->guest_features);
> > qemu_put_be32(f, vdev->config_len);
> > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> >
> > qemu_get_8s(f, &vdev->status);
> > qemu_get_8s(f, &vdev->isr);
> > + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
> > qemu_get_be16s(f, &vdev->queue_sel);
> > qemu_get_be32s(f, &features);
>
> Two remarks here:
>
> 1) You've declared needs_byteswap as "bool", but in above code you
> assume that the "bool" type is implemented as a single byte. That's
> most likely true on most system, but AFAIK it's specific to the
> compiler. So for portable code, I think you should either change the
> type of needs_byteswap or you should use a temporary uint8_t variable
> here instead.
>
Thomas,
I guess turning needs_byteswap into a uint8_t is ok and less intrusive
for the code.
> 2) You're changing the layout of the saved data ... don't you also have
> to increase the version numbers in that case, too? (e.g. change the
> version id for the register_savevm call in virtio-blk.c, virtio-net.c,
> etc.)?
>
Oops, my bad ! :-\
> Thomas
Thanks for your time.
Cheers.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access Greg Kurz
@ 2014-03-28 16:07 ` Thomas Huth
2014-03-28 17:02 ` Greg Kurz
2014-03-31 16:24 ` Alexander Graf
1 sibling, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2014-03-28 16:07 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, anthony, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, afaerber
On Fri, 28 Mar 2014 11:57:25 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> This is based on a simpler patch by Anthony Liguouri, which only handled
> the vring accesses. We also need some drivers to access these helpers,
> eg. for data which contains headers.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ ldq_phys() API change,
> use per-device needs_byteswap flag,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/virtio/virtio.c | 93 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 40 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 24b565f..1877b46 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
> vq->vring.align);
> }
>
> -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> + struct VirtIODevice *vdev)
> {
> hwaddr pa;
> pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
> - return ldq_phys(&address_space_memory, pa);
> + return virtio_ldq_phys(&address_space_memory, pa, vdev);
> }
>
> -static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
> +static inline uint32_t vring_desc_len(hwaddr desc_pa, int i,
> + struct VirtIODevice *vdev)
> {
> hwaddr pa;
> pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
> - return ldl_phys(&address_space_memory, pa);
> + return virtio_ldl_phys(&address_space_memory, pa, vdev);
> }
>
> -static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
> +static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i,
> + struct VirtIODevice *vdev)
> {
> hwaddr pa;
> pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
> - return lduw_phys(&address_space_memory, pa);
> + return virtio_lduw_phys(&address_space_memory, pa, vdev);
> }
>
> -static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
> +static inline uint16_t vring_desc_next(hwaddr desc_pa, int i,
> + struct VirtIODevice *vdev)
> {
> hwaddr pa;
> pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
> - return lduw_phys(&address_space_memory, pa);
> + return virtio_lduw_phys(&address_space_memory, pa, vdev);
> }
>
> static inline uint16_t vring_avail_flags(VirtQueue *vq)
> {
> hwaddr pa;
> pa = vq->vring.avail + offsetof(VRingAvail, flags);
> - return lduw_phys(&address_space_memory, pa);
> + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> }
>
> static inline uint16_t vring_avail_idx(VirtQueue *vq)
> {
> hwaddr pa;
> pa = vq->vring.avail + offsetof(VRingAvail, idx);
> - return lduw_phys(&address_space_memory, pa);
> + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> }
>
> static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
> {
> hwaddr pa;
> pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
> - return lduw_phys(&address_space_memory, pa);
> + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> }
>
> static inline uint16_t vring_used_event(VirtQueue *vq)
> @@ -160,44 +164,46 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
> {
> hwaddr pa;
> pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
> - stl_phys(&address_space_memory, pa, val);
> + virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
> }
>
> static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
> {
> hwaddr pa;
> pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
> - stl_phys(&address_space_memory, pa, val);
> + virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
> }
>
> static uint16_t vring_used_idx(VirtQueue *vq)
> {
> hwaddr pa;
> pa = vq->vring.used + offsetof(VRingUsed, idx);
> - return lduw_phys(&address_space_memory, pa);
> + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> }
>
> static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
> {
> hwaddr pa;
> pa = vq->vring.used + offsetof(VRingUsed, idx);
> - stw_phys(&address_space_memory, pa, val);
> + virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
> }
>
> static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
> {
> hwaddr pa;
> pa = vq->vring.used + offsetof(VRingUsed, flags);
> - stw_phys(&address_space_memory,
> - pa, lduw_phys(&address_space_memory, pa) | mask);
> + virtio_stw_phys(&address_space_memory, pa,
> + virtio_lduw_phys(&address_space_memory, pa,
> + vq->vdev) | mask, vq->vdev);
> }
>
> static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
> {
> hwaddr pa;
> pa = vq->vring.used + offsetof(VRingUsed, flags);
> - stw_phys(&address_space_memory,
> - pa, lduw_phys(&address_space_memory, pa) & ~mask);
> + virtio_stw_phys(&address_space_memory, pa,
> + virtio_lduw_phys(&address_space_memory, pa,
> + vq->vdev) & ~mask, vq->vdev);
> }
>
> static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> @@ -207,7 +213,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> return;
> }
> pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
> - stw_phys(&address_space_memory, pa, val);
> + virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
> }
>
> void virtio_queue_set_notification(VirtQueue *vq, int enable)
> @@ -325,16 +331,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
> }
>
> static unsigned virtqueue_next_desc(hwaddr desc_pa,
> - unsigned int i, unsigned int max)
> + unsigned int i, unsigned int max,
> + struct VirtIODevice *vdev)
> {
> unsigned int next;
>
> /* If this descriptor says it doesn't chain, we're done. */
> - if (!(vring_desc_flags(desc_pa, i) & VRING_DESC_F_NEXT))
> + if (!(vring_desc_flags(desc_pa, i, vdev) & VRING_DESC_F_NEXT)) {
> return max;
> + }
>
> /* Check they're not leading us off end of descriptors. */
> - next = vring_desc_next(desc_pa, i);
> + next = vring_desc_next(desc_pa, i, vdev);
> /* Make sure compiler knows to grab that: we don't want it changing! */
> smp_wmb();
>
> @@ -366,8 +374,9 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> i = virtqueue_get_head(vq, idx++);
> desc_pa = vq->vring.desc;
>
> - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> - if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> + if (vring_desc_flags(desc_pa, i, vq->vdev)
> + & VRING_DESC_F_INDIRECT) {
I think the above if-statement would still fit into one line with the
80-columns limit.
> + if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
> error_report("Invalid size for indirect buffer table");
> exit(1);
> }
> @@ -380,8 +389,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>
> /* loop over the indirect descriptor table */
> indirect = 1;
> - max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> - desc_pa = vring_desc_addr(desc_pa, i);
> + max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> + desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
> num_bufs = i = 0;
> }
>
> @@ -392,15 +401,17 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> exit(1);
> }
>
> - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> - in_total += vring_desc_len(desc_pa, i);
> + if (vring_desc_flags(desc_pa, i, vq->vdev)
> + & VRING_DESC_F_WRITE) {
Again, no need for two lines?
> + in_total += vring_desc_len(desc_pa, i, vq->vdev);
> } else {
> - out_total += vring_desc_len(desc_pa, i);
> + out_total += vring_desc_len(desc_pa, i, vq->vdev);
> }
> if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
> goto done;
> }
> - } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> + } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev))
> + != max);
Doesn't that while statement also still fit into 80 columns?
> if (!indirect)
> total_bufs = num_bufs;
> @@ -459,15 +470,15 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> vring_avail_event(vq, vring_avail_idx(vq));
> }
>
> - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> - if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> + if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_INDIRECT) {
> + if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
> error_report("Invalid size for indirect buffer table");
> exit(1);
> }
>
> /* loop over the indirect descriptor table */
> - max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> - desc_pa = vring_desc_addr(desc_pa, i);
> + max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> + desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
> i = 0;
> }
>
> @@ -475,30 +486,32 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> do {
> struct iovec *sg;
>
> - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> + if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_WRITE) {
> if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
> error_report("Too many write descriptors in indirect table");
> exit(1);
> }
> - elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
> + elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i,
> + vq->vdev);
That one seems to be pretty close, but could also still fit into 80
columns?
> sg = &elem->in_sg[elem->in_num++];
> } else {
> if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
> error_report("Too many read descriptors in indirect table");
> exit(1);
> }
> - elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
> + elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i,
> + vq->vdev);
> sg = &elem->out_sg[elem->out_num++];
> }
>
> - sg->iov_len = vring_desc_len(desc_pa, i);
> + sg->iov_len = vring_desc_len(desc_pa, i, vq->vdev);
>
> /* If we've got too many, that implies a descriptor loop. */
> if ((elem->in_num + elem->out_num) > max) {
> error_report("Looped descriptor");
> exit(1);
> }
> - } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> + } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev)) != max);
>
> /* Now map what we have collected */
> virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
Apart from the cosmetic nits, patch looks fine to me.
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access
2014-03-28 16:07 ` Thomas Huth
@ 2014-03-28 17:02 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 17:02 UTC (permalink / raw)
To: Thomas Huth
Cc: kwolf, peter.maydell, stefanha, mst, marc.zyngier, rusty, agraf,
qemu-devel, anthony, cornelia.huck, pbonzini, afaerber
On Fri, 28 Mar 2014 17:07:31 +0100
Thomas Huth <thuth@linux.vnet.ibm.com> wrote:
> On Fri, 28 Mar 2014 11:57:25 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > This is based on a simpler patch by Anthony Liguouri, which only handled
> > the vring accesses. We also need some drivers to access these helpers,
> > eg. for data which contains headers.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ ldq_phys() API change,
> > use per-device needs_byteswap flag,
> > Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > hw/virtio/virtio.c | 93 ++++++++++++++++++++++++++++++----------------------
> > 1 file changed, 53 insertions(+), 40 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 24b565f..1877b46 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
> > vq->vring.align);
> > }
> >
> > -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> > +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> > + struct VirtIODevice *vdev)
> > {
> > hwaddr pa;
> > pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, addr);
> > - return ldq_phys(&address_space_memory, pa);
> > + return virtio_ldq_phys(&address_space_memory, pa, vdev);
> > }
> >
> > -static inline uint32_t vring_desc_len(hwaddr desc_pa, int i)
> > +static inline uint32_t vring_desc_len(hwaddr desc_pa, int i,
> > + struct VirtIODevice *vdev)
> > {
> > hwaddr pa;
> > pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, len);
> > - return ldl_phys(&address_space_memory, pa);
> > + return virtio_ldl_phys(&address_space_memory, pa, vdev);
> > }
> >
> > -static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i)
> > +static inline uint16_t vring_desc_flags(hwaddr desc_pa, int i,
> > + struct VirtIODevice *vdev)
> > {
> > hwaddr pa;
> > pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, flags);
> > - return lduw_phys(&address_space_memory, pa);
> > + return virtio_lduw_phys(&address_space_memory, pa, vdev);
> > }
> >
> > -static inline uint16_t vring_desc_next(hwaddr desc_pa, int i)
> > +static inline uint16_t vring_desc_next(hwaddr desc_pa, int i,
> > + struct VirtIODevice *vdev)
> > {
> > hwaddr pa;
> > pa = desc_pa + sizeof(VRingDesc) * i + offsetof(VRingDesc, next);
> > - return lduw_phys(&address_space_memory, pa);
> > + return virtio_lduw_phys(&address_space_memory, pa, vdev);
> > }
> >
> > static inline uint16_t vring_avail_flags(VirtQueue *vq)
> > {
> > hwaddr pa;
> > pa = vq->vring.avail + offsetof(VRingAvail, flags);
> > - return lduw_phys(&address_space_memory, pa);
> > + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> > }
> >
> > static inline uint16_t vring_avail_idx(VirtQueue *vq)
> > {
> > hwaddr pa;
> > pa = vq->vring.avail + offsetof(VRingAvail, idx);
> > - return lduw_phys(&address_space_memory, pa);
> > + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> > }
> >
> > static inline uint16_t vring_avail_ring(VirtQueue *vq, int i)
> > {
> > hwaddr pa;
> > pa = vq->vring.avail + offsetof(VRingAvail, ring[i]);
> > - return lduw_phys(&address_space_memory, pa);
> > + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> > }
> >
> > static inline uint16_t vring_used_event(VirtQueue *vq)
> > @@ -160,44 +164,46 @@ static inline void vring_used_ring_id(VirtQueue *vq, int i, uint32_t val)
> > {
> > hwaddr pa;
> > pa = vq->vring.used + offsetof(VRingUsed, ring[i].id);
> > - stl_phys(&address_space_memory, pa, val);
> > + virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
> > }
> >
> > static inline void vring_used_ring_len(VirtQueue *vq, int i, uint32_t val)
> > {
> > hwaddr pa;
> > pa = vq->vring.used + offsetof(VRingUsed, ring[i].len);
> > - stl_phys(&address_space_memory, pa, val);
> > + virtio_stl_phys(&address_space_memory, pa, val, vq->vdev);
> > }
> >
> > static uint16_t vring_used_idx(VirtQueue *vq)
> > {
> > hwaddr pa;
> > pa = vq->vring.used + offsetof(VRingUsed, idx);
> > - return lduw_phys(&address_space_memory, pa);
> > + return virtio_lduw_phys(&address_space_memory, pa, vq->vdev);
> > }
> >
> > static inline void vring_used_idx_set(VirtQueue *vq, uint16_t val)
> > {
> > hwaddr pa;
> > pa = vq->vring.used + offsetof(VRingUsed, idx);
> > - stw_phys(&address_space_memory, pa, val);
> > + virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
> > }
> >
> > static inline void vring_used_flags_set_bit(VirtQueue *vq, int mask)
> > {
> > hwaddr pa;
> > pa = vq->vring.used + offsetof(VRingUsed, flags);
> > - stw_phys(&address_space_memory,
> > - pa, lduw_phys(&address_space_memory, pa) | mask);
> > + virtio_stw_phys(&address_space_memory, pa,
> > + virtio_lduw_phys(&address_space_memory, pa,
> > + vq->vdev) | mask, vq->vdev);
> > }
> >
> > static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
> > {
> > hwaddr pa;
> > pa = vq->vring.used + offsetof(VRingUsed, flags);
> > - stw_phys(&address_space_memory,
> > - pa, lduw_phys(&address_space_memory, pa) & ~mask);
> > + virtio_stw_phys(&address_space_memory, pa,
> > + virtio_lduw_phys(&address_space_memory, pa,
> > + vq->vdev) & ~mask, vq->vdev);
> > }
> >
> > static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> > @@ -207,7 +213,7 @@ static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
> > return;
> > }
> > pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
> > - stw_phys(&address_space_memory, pa, val);
> > + virtio_stw_phys(&address_space_memory, pa, val, vq->vdev);
> > }
> >
> > void virtio_queue_set_notification(VirtQueue *vq, int enable)
> > @@ -325,16 +331,18 @@ static unsigned int virtqueue_get_head(VirtQueue *vq, unsigned int idx)
> > }
> >
> > static unsigned virtqueue_next_desc(hwaddr desc_pa,
> > - unsigned int i, unsigned int max)
> > + unsigned int i, unsigned int max,
> > + struct VirtIODevice *vdev)
> > {
> > unsigned int next;
> >
> > /* If this descriptor says it doesn't chain, we're done. */
> > - if (!(vring_desc_flags(desc_pa, i) & VRING_DESC_F_NEXT))
> > + if (!(vring_desc_flags(desc_pa, i, vdev) & VRING_DESC_F_NEXT)) {
> > return max;
> > + }
> >
> > /* Check they're not leading us off end of descriptors. */
> > - next = vring_desc_next(desc_pa, i);
> > + next = vring_desc_next(desc_pa, i, vdev);
> > /* Make sure compiler knows to grab that: we don't want it changing! */
> > smp_wmb();
> >
> > @@ -366,8 +374,9 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> > i = virtqueue_get_head(vq, idx++);
> > desc_pa = vq->vring.desc;
> >
> > - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> > - if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> > + if (vring_desc_flags(desc_pa, i, vq->vdev)
> > + & VRING_DESC_F_INDIRECT) {
>
> I think the above if-statement would still fit into one line with the
> 80-columns limit.
>
> > + if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
> > error_report("Invalid size for indirect buffer table");
> > exit(1);
> > }
> > @@ -380,8 +389,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> >
> > /* loop over the indirect descriptor table */
> > indirect = 1;
> > - max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> > - desc_pa = vring_desc_addr(desc_pa, i);
> > + max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> > + desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
> > num_bufs = i = 0;
> > }
> >
> > @@ -392,15 +401,17 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> > exit(1);
> > }
> >
> > - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> > - in_total += vring_desc_len(desc_pa, i);
> > + if (vring_desc_flags(desc_pa, i, vq->vdev)
> > + & VRING_DESC_F_WRITE) {
>
> Again, no need for two lines?
>
> > + in_total += vring_desc_len(desc_pa, i, vq->vdev);
> > } else {
> > - out_total += vring_desc_len(desc_pa, i);
> > + out_total += vring_desc_len(desc_pa, i, vq->vdev);
> > }
> > if (in_total >= max_in_bytes && out_total >= max_out_bytes) {
> > goto done;
> > }
> > - } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> > + } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev))
> > + != max);
>
> Doesn't that while statement also still fit into 80 columns?
>
> > if (!indirect)
> > total_bufs = num_bufs;
> > @@ -459,15 +470,15 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> > vring_avail_event(vq, vring_avail_idx(vq));
> > }
> >
> > - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
> > - if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
> > + if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_INDIRECT) {
> > + if (vring_desc_len(desc_pa, i, vq->vdev) % sizeof(VRingDesc)) {
> > error_report("Invalid size for indirect buffer table");
> > exit(1);
> > }
> >
> > /* loop over the indirect descriptor table */
> > - max = vring_desc_len(desc_pa, i) / sizeof(VRingDesc);
> > - desc_pa = vring_desc_addr(desc_pa, i);
> > + max = vring_desc_len(desc_pa, i, vq->vdev) / sizeof(VRingDesc);
> > + desc_pa = vring_desc_addr(desc_pa, i, vq->vdev);
> > i = 0;
> > }
> >
> > @@ -475,30 +486,32 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> > do {
> > struct iovec *sg;
> >
> > - if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_WRITE) {
> > + if (vring_desc_flags(desc_pa, i, vq->vdev) & VRING_DESC_F_WRITE) {
> > if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
> > error_report("Too many write descriptors in indirect table");
> > exit(1);
> > }
> > - elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i);
> > + elem->in_addr[elem->in_num] = vring_desc_addr(desc_pa, i,
> > + vq->vdev);
>
> That one seems to be pretty close, but could also still fit into 80
> columns?
>
> > sg = &elem->in_sg[elem->in_num++];
> > } else {
> > if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
> > error_report("Too many read descriptors in indirect table");
> > exit(1);
> > }
> > - elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i);
> > + elem->out_addr[elem->out_num] = vring_desc_addr(desc_pa, i,
> > + vq->vdev);
> > sg = &elem->out_sg[elem->out_num++];
> > }
> >
> > - sg->iov_len = vring_desc_len(desc_pa, i);
> > + sg->iov_len = vring_desc_len(desc_pa, i, vq->vdev);
> >
> > /* If we've got too many, that implies a descriptor loop. */
> > if ((elem->in_num + elem->out_num) > max) {
> > error_report("Looped descriptor");
> > exit(1);
> > }
> > - } while ((i = virtqueue_next_desc(desc_pa, i, max)) != max);
> > + } while ((i = virtqueue_next_desc(desc_pa, i, max, vq->vdev)) != max);
> >
> > /* Now map what we have collected */
> > virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
>
> Apart from the cosmetic nits, patch looks fine to me.
>
Heh... remains of a first shot where I was passing vdev->needs_byteswap
and overpassed the limit. :)
> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
>
>
>
Thanks !
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 6/8] virtio-scsi: " Greg Kurz
@ 2014-03-28 17:13 ` Greg Kurz
2014-03-28 17:21 ` Andreas Färber
2014-03-31 16:34 ` Alexander Graf
1 sibling, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 17:13 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On Fri, 28 Mar 2014 11:57:56 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
> fix missing tswap32 in virtio_scsi_push_event(),
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/scsi/virtio-scsi.c | 38 ++++++++++++++++++++------------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index b0d7517..20d326e 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -18,6 +18,7 @@
> #include <hw/scsi/scsi.h>
> #include <block/scsi.h>
> #include <hw/virtio/virtio-bus.h>
> +#include "hw/virtio/virtio-access.h"
>
> typedef struct VirtIOSCSIReq {
> VirtIOSCSI *dev;
> @@ -315,12 +316,12 @@ static void virtio_scsi_command_complete(SCSIRequest *r, uint32_t status,
> req->resp.cmd->response = VIRTIO_SCSI_S_OK;
> req->resp.cmd->status = status;
> if (req->resp.cmd->status == GOOD) {
> - req->resp.cmd->resid = tswap32(resid);
> + req->resp.cmd->resid = virtio_tswap32(resid, VIRTIO_DEVICE(s));
> } else {
> req->resp.cmd->resid = 0;
> sense_len = scsi_req_get_sense(r, req->resp.cmd->sense,
> vs->sense_size);
> - req->resp.cmd->sense_len = tswap32(sense_len);
> + req->resp.cmd->sense_len = virtio_tswap32(sense_len, VIRTIO_DEVICE(s));
> }
> virtio_scsi_complete_req(req);
> }
> @@ -416,16 +417,16 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
> VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
> VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
>
> - stl_raw(&scsiconf->num_queues, s->conf.num_queues);
> - stl_raw(&scsiconf->seg_max, 128 - 2);
> - stl_raw(&scsiconf->max_sectors, s->conf.max_sectors);
> - stl_raw(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> - stl_raw(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent));
> - stl_raw(&scsiconf->sense_size, s->sense_size);
> - stl_raw(&scsiconf->cdb_size, s->cdb_size);
> - stw_raw(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
> - stw_raw(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
> - stl_raw(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
> + virtio_stl_p(&scsiconf->num_queues, s->conf.num_queues, vdev);
> + virtio_stl_p(&scsiconf->seg_max, 128 - 2, vdev);
> + virtio_stl_p(&scsiconf->max_sectors, s->conf.max_sectors, vdev);
> + virtio_stl_p(&scsiconf->cmd_per_lun, s->conf.cmd_per_lun, vdev);
> + virtio_stl_p(&scsiconf->event_info_size, sizeof(VirtIOSCSIEvent), vdev);
> + virtio_stl_p(&scsiconf->sense_size, s->sense_size, vdev);
> + virtio_stl_p(&scsiconf->cdb_size, s->cdb_size, vdev);
> + virtio_stw_p(&scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL, vdev);
> + virtio_stw_p(&scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET, vdev);
> + virtio_stl_p(&scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN, vdev);
> }
>
> static void virtio_scsi_set_config(VirtIODevice *vdev,
> @@ -434,14 +435,15 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
> VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(vdev);
>
> - if ((uint32_t) ldl_raw(&scsiconf->sense_size) >= 65536 ||
> - (uint32_t) ldl_raw(&scsiconf->cdb_size) >= 256) {
> + if ((uint32_t) virtio_ldl_p(&scsiconf->sense_size,
> + vdev) >= 65536 ||
> + (uint32_t) virtio_ldl_p(&scsiconf->cdb_size, vdev) >= 256) {
> error_report("bad data written to virtio-scsi configuration space");
> exit(1);
> }
>
> - vs->sense_size = ldl_raw(&scsiconf->sense_size);
> - vs->cdb_size = ldl_raw(&scsiconf->cdb_size);
> + vs->sense_size = virtio_ldl_p(&scsiconf->sense_size, vdev);
> + vs->cdb_size = virtio_ldl_p(&scsiconf->cdb_size, vdev);
> }
>
> static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
>
> evt = req->resp.event;
> memset(evt, 0, sizeof(VirtIOSCSIEvent));
> - evt->event = event;
> - evt->reason = reason;
> + evt->event = virtio_tswap32(event);
> + evt->reason = virtio_tswap32(reason);
My bad again, it should be:
+ evt->event = virtio_tswap32(event, vdev);
+ evt->reason = virtio_tswap32(reason, vdev);
> if (!dev) {
> assert(event == VIRTIO_SCSI_T_EVENTS_MISSED);
> } else {
>
>
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 17:13 ` Greg Kurz
@ 2014-03-28 17:21 ` Andreas Färber
2014-03-28 17:37 ` Greg Kurz
0 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2014-03-28 17:21 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
Am 28.03.2014 18:13, schrieb Greg Kurz:
> On Fri, 28 Mar 2014 11:57:56 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
>>
>> evt = req->resp.event;
>> memset(evt, 0, sizeof(VirtIOSCSIEvent));
>> - evt->event = event;
>> - evt->reason = reason;
>> + evt->event = virtio_tswap32(event);
>> + evt->reason = virtio_tswap32(reason);
>
> My bad again, it should be:
>
> + evt->event = virtio_tswap32(event, vdev);
> + evt->reason = virtio_tswap32(reason, vdev);
Pure bikeshedding, but wouldn't it make sense to have the vdev as first
argument?
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 17:21 ` Andreas Färber
@ 2014-03-28 17:37 ` Greg Kurz
2014-03-28 17:43 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 17:37 UTC (permalink / raw)
To: Andreas Färber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony
On Fri, 28 Mar 2014 18:21:43 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Am 28.03.2014 18:13, schrieb Greg Kurz:
> > On Fri, 28 Mar 2014 11:57:56 +0100
> > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> >> @@ -519,8 +521,8 @@ static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> >>
> >> evt = req->resp.event;
> >> memset(evt, 0, sizeof(VirtIOSCSIEvent));
> >> - evt->event = event;
> >> - evt->reason = reason;
> >> + evt->event = virtio_tswap32(event);
> >> + evt->reason = virtio_tswap32(reason);
> >
> > My bad again, it should be:
> >
> > + evt->event = virtio_tswap32(event, vdev);
> > + evt->reason = virtio_tswap32(reason, vdev);
>
> Pure bikeshedding, but wouldn't it make sense to have the vdev as first
> argument?
>
I have thought about it also... it would make sense to do the same with
all the other helpers in virtio-access.h then.
And while we are at it, since we pass &address_space_memory to all occurences
of virtio_*_phys() and I don't see why we would change that, maybe we
can also move that into the helpers. Thoughts ?
> Regards,
> Andreas
>
Thanks.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 17:37 ` Greg Kurz
@ 2014-03-28 17:43 ` Peter Maydell
2014-03-28 18:04 ` Greg Kurz
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2014-03-28 17:43 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Thomas Huth, Michael S. Tsirkin, Marc Zyngier,
Rusty Russell, Alexander Graf, QEMU Developers, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini, Anthony Liguori,
Andreas Färber
On 28 March 2014 17:37, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> And while we are at it, since we pass &address_space_memory to all
> occurences of virtio_*_phys() and I don't see why we would change
> that, maybe we can also move that into the helpers. Thoughts ?
In the longer term I'm hoping that references to
&address_space_memory go away -- we should be modelling
separate address spaces per CPU and per every other
thing that can act as a DMA master (ie issue memory
transactions). I'm not sure exactly how virtio ought to
work since these accesses directly to memory are a total
hack, but probably we will end up setting the virtio
device up and handing it an AddressSpace* that it should use.
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
2014-03-28 14:15 ` Thomas Huth
@ 2014-03-28 17:59 ` Andreas Färber
2014-03-28 19:00 ` Greg Kurz
2014-03-31 14:50 ` Alexander Graf
2 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2014-03-28 17:59 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, Juan Quintela, stefanha, cornelia.huck, pbonzini,
anthony
Am 28.03.2014 11:57, schrieb Greg Kurz:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value. In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
>
> We need to support both implementations and we want to share as much code
> as possible.
>
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
> into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
> property in a single place and share all the virtio memory accessors
> between the two implementations
>
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
>
> if (vdev->legacy) {
> vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
> vdev->needs_byteswap = true;
> #else
> vdev->needs_byteswap = false;
> #endif
> }
>
> The needs_byteswap flag is preserved accross migrations.
"across"
Why? :) For all targets except ppc this field does not change during
runtime. Do you intend to support ppc64 <-> ppc64le migration, i.e.
protection against changing HOST_WORDS_BIGENDIAN?
Since you're setting the field on reset rather than in instance_init or
realize, resetting the device on one host with changing ILE may lead to
weird endianness settings: Devices are initially reset before the VM
starts. ILE will always be Big Endian then IIUC, since all PowerPCCPU
models default to Big Endian and SLOF runs Big Endian. SLOF might use a
virtio-blk device to boot from. We then boot into SLES12 ppc64le - ILE
indicates Little Endian now. As soon as the guest triggers a reset of
the device, such as by resetting the whole PCI bus, endianness changes
to Little Endian. Now you indeed have an endianness on the source that
is different from that of the newly Big Endian reset device on the
destination. Is this desired, or did you accidentally initialize the
flag in the wrong place?
If we do need it, maybe you can place the field into a subsection to
avoid imposing it on x86?
Regards,
Andreas
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> ldq_phys() API change,
> relicensed virtio-access.h to GPLv2+ on Rusty's request,
> introduce a per-device needs_byteswap flag,
> add VirtIODevice * arg to virtio helpers,
> rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 17:43 ` Peter Maydell
@ 2014-03-28 18:04 ` Greg Kurz
2014-03-28 18:14 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 18:04 UTC (permalink / raw)
To: Peter Maydell
Cc: Kevin Wolf, Thomas Huth, Michael S. Tsirkin, Marc Zyngier,
Rusty Russell, Alexander Graf, QEMU Developers, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini, Anthony Liguori,
Andreas Färber
On Fri, 28 Mar 2014 17:43:07 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 March 2014 17:37, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > And while we are at it, since we pass &address_space_memory to all
> > occurences of virtio_*_phys() and I don't see why we would change
> > that, maybe we can also move that into the helpers. Thoughts ?
>
> In the longer term I'm hoping that references to
> &address_space_memory go away -- we should be modelling
> separate address spaces per CPU and per every other
> thing that can act as a DMA master (ie issue memory
> transactions). I'm not sure exactly how virtio ought to
> work since these accesses directly to memory are a total
> hack, but probably we will end up setting the virtio
> device up and handing it an AddressSpace* that it should use.
>
Ok, I am now convinced. Let's make struct VirtIODevice* be the
first argument for all helpers and kill the AddressSpace* one.
Unless you envision we could end up with different address spaces
accross multiple virtio devices, I would then do as proposed
above... Even if we add an AddressSpace* to devices, the API
will remain the same.
> thanks
> -- PMM
>
Cheers.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 18:04 ` Greg Kurz
@ 2014-03-28 18:14 ` Peter Maydell
2014-03-28 18:58 ` Greg Kurz
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2014-03-28 18:14 UTC (permalink / raw)
To: Greg Kurz
Cc: Kevin Wolf, Thomas Huth, Michael S. Tsirkin, Marc Zyngier,
Rusty Russell, Alexander Graf, QEMU Developers, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini, Anthony Liguori,
Andreas Färber
On 28 March 2014 18:04, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> Ok, I am now convinced. Let's make struct VirtIODevice* be the
> first argument for all helpers and kill the AddressSpace* one.
> Unless you envision we could end up with different address spaces
> accross multiple virtio devices
Well, any one virtio device would always use the same AddressSpace,
but consider a system model with two separate models of different
PCs in it, each of which has its own self-contained memory, PCI
bus and virtio devices...
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 18:14 ` Peter Maydell
@ 2014-03-28 18:58 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 18:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Kevin Wolf, Thomas Huth, Michael S. Tsirkin, Marc Zyngier,
Rusty Russell, Alexander Graf, QEMU Developers, Stefan Hajnoczi,
Cornelia Huck, Paolo Bonzini, Anthony Liguori,
Andreas Färber
On Fri, 28 Mar 2014 18:14:55 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 March 2014 18:04, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> > Ok, I am now convinced. Let's make struct VirtIODevice* be the
> > first argument for all helpers and kill the AddressSpace* one.
> > Unless you envision we could end up with different address spaces
> > accross multiple virtio devices
>
> Well, any one virtio device would always use the same AddressSpace,
> but consider a system model with two separate models of different
> PCs in it, each of which has its own self-contained memory, PCI
> bus and virtio devices...
>
A per-device AddressSpace* property would be more appropriate then. Correct ?
> thanks
> -- PMM
>
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-28 17:59 ` Andreas Färber
@ 2014-03-28 19:00 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-03-28 19:00 UTC (permalink / raw)
To: Andreas Färber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty, agraf,
qemu-devel, Juan Quintela, stefanha, cornelia.huck, pbonzini,
anthony
On Fri, 28 Mar 2014 18:59:22 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Am 28.03.2014 11:57, schrieb Greg Kurz:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > virtio data structures are defined as "target endian", which assumes
> > that's a fixed value. In fact, that actually means it's platform-specific.
> > The OASIS virtio 1.0 spec will fix this, by making all little endian.
> >
> > We need to support both implementations and we want to share as much code
> > as possible.
> >
> > A good way to do it is to introduce a per-device boolean property to tell
> > memory accessors whether they should swap bytes or not. This flag should
> > be set at device reset time, because:
> > - endianness won't change while the device is in use, and if we reboot
> > into a different endianness, a new device reset will occur
> > - as suggested by Alexander Graf, we can keep all the logic to set the
> > property in a single place and share all the virtio memory accessors
> > between the two implementations
> >
> > For legacy devices, we rely on a per-platform hook to set the flag. The
> > virtio 1.0 implementation will just have to add some more logic in
> > virtio_reset() instead of calling the hook:
> >
> > if (vdev->legacy) {
> > vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > } else {
> > #ifdef HOST_WORDS_BIGENDIAN
> > vdev->needs_byteswap = true;
> > #else
> > vdev->needs_byteswap = false;
> > #endif
> > }
> >
> > The needs_byteswap flag is preserved accross migrations.
>
> "across"
>
> Why? :) For all targets except ppc this field does not change during
> runtime. Do you intend to support ppc64 <-> ppc64le migration, i.e.
> protection against changing HOST_WORDS_BIGENDIAN?
>
Because we only set this property at virtio_reset() time... how can
we ensure it still has the correct value after a migration then ?
And no, I don't intend to support cross-endian migration... The
only endianness change that we can support is rebooting into a
different endianness.
> Since you're setting the field on reset rather than in instance_init or
> realize, resetting the device on one host with changing ILE may lead to
> weird endianness settings: Devices are initially reset before the VM
> starts. ILE will always be Big Endian then IIUC, since all PowerPCCPU
> models default to Big Endian and SLOF runs Big Endian. SLOF might use a
> virtio-blk device to boot from. We then boot into SLES12 ppc64le - ILE
> indicates Little Endian now. As soon as the guest triggers a reset of
> the device, such as by resetting the whole PCI bus, endianness changes
> to Little Endian. Now you indeed have an endianness on the source that
> is different from that of the newly Big Endian reset device on the
> destination. Is this desired, or did you accidentally initialize the
> flag in the wrong place?
>
Hmmm... the assumption is that ALL virtio devices get reset after the guest
kernel switches to LE. Are you saying this is not the case if SLOF uses
a virtio-blk device to boot from ? This device would be handed over to
the guest kernel to be used as is ? If yes, then I don't see how we can
cope with that... The way legacy virtio is implemented, we cannot
reasonably support an endianness change without fully reseting the device.
I guess this is the main motivation behind virtio 1.0 :)
> If we do need it, maybe you can place the field into a subsection to
> avoid imposing it on x86?
>
I think it is needed anyway as long as we want to support a ppc64 guest
that can change endianness and uses legacy virtio devices, even with
a x86 host.
> Regards,
> Andreas
>
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device needs_byteswap flag,
> > add VirtIODevice * arg to virtio helpers,
> > rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> > Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
Cheers.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
2014-03-28 14:15 ` Thomas Huth
2014-03-28 17:59 ` Andreas Färber
@ 2014-03-31 14:50 ` Alexander Graf
2014-04-01 11:54 ` Greg Kurz
2 siblings, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 14:50 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value. In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
>
> We need to support both implementations and we want to share as much code
> as possible.
>
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
> into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
> property in a single place and share all the virtio memory accessors
> between the two implementations
>
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
>
> if (vdev->legacy) {
> vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
> vdev->needs_byteswap = true;
> #else
> vdev->needs_byteswap = false;
> #endif
> }
>
> The needs_byteswap flag is preserved accross migrations.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> ldq_phys() API change,
> relicensed virtio-access.h to GPLv2+ on Rusty's request,
> introduce a per-device needs_byteswap flag,
> add VirtIODevice * arg to virtio helpers,
> rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/virtio/virtio.c | 5 +
> include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++
> include/hw/virtio/virtio.h | 3 +
> stubs/Makefile.objs | 1
> stubs/virtio_get_byteswap.c | 6 ++
> 5 files changed, 154 insertions(+)
> create mode 100644 include/hw/virtio/virtio-access.h
> create mode 100644 stubs/virtio_get_byteswap.c
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index aeabf3a..24b565f 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -19,6 +19,7 @@
> #include "hw/virtio/virtio.h"
> #include "qemu/atomic.h"
> #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>
> /*
> * The alignment to use between consumer and producer parts of vring.
> @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
>
> virtio_set_status(vdev, 0);
>
> + vdev->needs_byteswap = virtio_legacy_get_byteswap();
> +
> if (k->reset) {
> k->reset(vdev);
> }
> @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
>
> qemu_put_8s(f, &vdev->status);
> qemu_put_8s(f, &vdev->isr);
> + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
> qemu_put_be16s(f, &vdev->queue_sel);
> qemu_put_be32s(f, &vdev->guest_features);
> qemu_put_be32(f, vdev->config_len);
> @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
>
> qemu_get_8s(f, &vdev->status);
> qemu_get_8s(f, &vdev->isr);
> + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
> qemu_get_be16s(f, &vdev->queue_sel);
> qemu_get_be32s(f, &features);
>
> diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> new file mode 100644
> index 0000000..70dd1e2
> --- /dev/null
> +++ b/include/hw/virtio/virtio-access.h
> @@ -0,0 +1,139 @@
> +/*
> + * Virtio Accessor Support: In case your target can change endian.
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + * Rusty Russell <rusty@au.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +#ifndef _QEMU_VIRTIO_ACCESS_H
> +#define _QEMU_VIRTIO_ACCESS_H
> +#include "hw/virtio/virtio.h"
> +
> +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap16(lduw_phys(as, pa));
> + }
> + return lduw_phys(as, pa);
> +}
> +
> +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap32(ldl_phys(as, pa));
> + }
> + return ldl_phys(as, pa);
> +}
> +
> +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap64(ldq_phys(as, pa));
> + }
> + return ldq_phys(as, pa);
> +}
> +
> +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + stw_phys(as, pa, bswap16(value));
> + } else {
> + stw_phys(as, pa, value);
> + }
> +}
> +
> +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + stl_phys(as, pa, bswap32(value));
> + } else {
> + stl_phys(as, pa, value);
> + }
> +}
> +
> +static inline void virtio_stw_p(void *ptr, uint16_t v,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + stw_p(ptr, bswap16(v));
> + } else {
> + stw_p(ptr, v);
> + }
> +}
> +
> +static inline void virtio_stl_p(void *ptr, uint32_t v,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + stl_p(ptr, bswap32(v));
> + } else {
> + stl_p(ptr, v);
> + }
> +}
> +
> +static inline void virtio_stq_p(void *ptr, uint64_t v,
> + struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + stq_p(ptr, bswap64(v));
> + } else {
> + stq_p(ptr, v);
> + }
> +}
> +
> +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap16(lduw_p(ptr));
> + } else {
> + return lduw_p(ptr);
> + }
> +}
> +
> +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap32(ldl_p(ptr));
> + } else {
> + return ldl_p(ptr);
> + }
> +}
> +
> +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap64(ldq_p(ptr));
> + } else {
> + return ldq_p(ptr);
> + }
> +}
> +
> +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev)
> +{
> + if (vdev->needs_byteswap) {
> + return bswap32(tswap32(s));
> + } else {
> + return tswap32(s);
> + }
> +}
> +
> +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev)
> +{
> + tswap32s(s);
> + if (vdev->needs_byteswap) {
> + *s = bswap32(*s);
> + }
Couldn't you just reuse virtio_tswap32() here?
Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't
have any access to CPU endian specific RAM accessors.
How about we introduce a flag like "is_bigendian" that we set depending
on the CPU default endianness for legacy virtio. We can then change that
flag accordingly. For the wrappers here, we could just use accessors
like ldl_be_phys() or ldl_le_phys().
That should make things a lot easier to understand - and for virtio 1.0
we only need to set is_bigendian = false :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access Greg Kurz
2014-03-28 16:07 ` Thomas Huth
@ 2014-03-31 16:24 ` Alexander Graf
2014-03-31 16:26 ` Andreas Färber
1 sibling, 1 reply; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 16:24 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> This is based on a simpler patch by Anthony Liguouri, which only handled
> the vring accesses. We also need some drivers to access these helpers,
> eg. for data which contains headers.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> [ ldq_phys() API change,
> use per-device needs_byteswap flag,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/virtio/virtio.c | 93 ++++++++++++++++++++++++++++++----------------------
> 1 file changed, 53 insertions(+), 40 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 24b565f..1877b46 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
> vq->vring.align);
> }
>
> -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> + struct VirtIODevice *vdev)
The logical ordering for helper is usually to have the device as the
first parameter (it's basically the self object). Could you please order
it accordingly?
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access
2014-03-31 16:24 ` Alexander Graf
@ 2014-03-31 16:26 ` Andreas Färber
2014-04-01 12:03 ` Greg Kurz
0 siblings, 1 reply; 33+ messages in thread
From: Andreas Färber @ 2014-03-31 16:26 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
Alexander Graf, qemu-devel, stefanha, cornelia.huck, pbonzini,
anthony
Am 31.03.2014 18:24, schrieb Alexander Graf:
> On 03/28/2014 11:57 AM, Greg Kurz wrote:
>> From: Rusty Russell <rusty@rustcorp.com.au>
>>
>> This is based on a simpler patch by Anthony Liguouri, which only handled
>> the vring accesses. We also need some drivers to access these helpers,
>> eg. for data which contains headers.
>>
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> [ ldq_phys() API change,
>> use per-device needs_byteswap flag,
>> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> ---
>> hw/virtio/virtio.c | 93
>> ++++++++++++++++++++++++++++++----------------------
>> 1 file changed, 53 insertions(+), 40 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 24b565f..1877b46 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
>> vq->vring.align);
>> }
>> -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
>> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
>> + struct VirtIODevice *vdev)
>
> The logical ordering for helper is usually to have the device as the
> first parameter (it's basically the self object). Could you please order
> it accordingly?
Also please note that QEMU Coding Style requests the use of typedefs,
i.e. drop the "struct".
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers Greg Kurz
@ 2014-03-31 16:28 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 16:28 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/net/virtio-net.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 439477b..eb89f48 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -23,6 +23,7 @@
> #include "hw/virtio/virtio-bus.h"
> #include "qapi/qmp/qjson.h"
> #include "monitor/monitor.h"
> +#include "hw/virtio/virtio-access.h"
>
> #define VIRTIO_NET_VM_VERSION 11
>
> @@ -72,8 +73,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> VirtIONet *n = VIRTIO_NET(vdev);
> struct virtio_net_config netcfg;
>
> - stw_p(&netcfg.status, n->status);
> - stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
> + virtio_stw_p(&netcfg.status, n->status, vdev);
> + virtio_stw_p(&netcfg.max_virtqueue_pairs, n->max_queues, vdev);
If you make virtio_stw_p() only depend on endian specific operations
this file could be built as part of obj rather than common-obj again,
right? So it doesn't depend on the target to operate. So as part of this
change you should also have a Makefile.obj change.
The same obviously applies to all the other files as well :)
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers Greg Kurz
@ 2014-03-31 16:30 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 16:30 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/virtio/virtio-balloon.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index a470a0b..e0b31ba 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -30,6 +30,7 @@
> #endif
>
> #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>
> static void balloon_page(void *addr, int deflate)
> {
> @@ -192,7 +193,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> ram_addr_t pa;
> ram_addr_t addr;
>
> - pa = (ram_addr_t)ldl_p(&pfn) << VIRTIO_BALLOON_PFN_SHIFT;
> + pa = (ram_addr_t)virtio_ldl_p(&pfn,
> + vdev) << VIRTIO_BALLOON_PFN_SHIFT;
This is quite unreadable. Just create a new small local variable for the
load result :).
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers Greg Kurz
@ 2014-03-31 16:31 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 16:31 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/block/virtio-blk.c | 40 +++++++++++++++++++++++-----------------
> 1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8a568e5..c652ff2 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -26,6 +26,7 @@
> # include <scsi/sg.h>
> #endif
> #include "hw/virtio/virtio-bus.h"
> +#include "hw/virtio/virtio-access.h"
>
> typedef struct VirtIOBlockReq
> {
> @@ -77,7 +78,9 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
> trace_virtio_blk_rw_complete(req, ret);
>
> if (ret) {
> - bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
> + bool is_read =
> + !(virtio_ldl_p(&req->out->type,
> + VIRTIO_DEVICE(req->dev)) & VIRTIO_BLK_T_OUT);
Any way you could fit this into the line limit by extracting part of it
into a separate variable?
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 6/8] virtio-scsi: use virtio wrappers to access headers
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 6/8] virtio-scsi: " Greg Kurz
2014-03-28 17:13 ` Greg Kurz
@ 2014-03-31 16:34 ` Alexander Graf
1 sibling, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 16:34 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:57 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Note that st*_raw and ld*_raw are effectively replaced by st*_p and ld*_p.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
> fix missing tswap32 in virtio_scsi_push_event(),
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/scsi/virtio-scsi.c | 38 ++++++++++++++++++++------------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index b0d7517..20d326e 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -18,6 +18,7 @@
> #include <hw/scsi/scsi.h>
> #include <block/scsi.h>
> #include <hw/virtio/virtio-bus.h>
> +#include "hw/virtio/virtio-access.h"
Just a small nit, but I spot some inconsitency in < and " usage here :)
Alex
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: use virtio wrappers to access headers
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: " Greg Kurz
@ 2014-03-31 17:01 ` Alexander Graf
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Graf @ 2014-03-31 17:01 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On 03/28/2014 11:58 AM, Greg Kurz wrote:
> From: Rusty Russell <rusty@rustcorp.com.au>
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> [ use per-device needs_byteswap flag,
> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> hw/char/virtio-serial-bus.c | 39 +++++++++++++++++++++++----------------
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 2b647b6..7a84b2a 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -24,6 +24,7 @@
> #include "hw/sysbus.h"
> #include "trace.h"
> #include "hw/virtio/virtio-serial.h"
> +#include "hw/virtio/virtio-access.h"
>
> static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
> {
> @@ -185,9 +186,9 @@ static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
> {
> struct virtio_console_control cpkt;
>
> - stl_p(&cpkt.id, port_id);
> - stw_p(&cpkt.event, event);
> - stw_p(&cpkt.value, value);
> + virtio_stl_p(&cpkt.id, port_id, VIRTIO_DEVICE(vser));
These are dynamic casts. You at least want to do these only once,
probably even use some faster form of casting.
Alex
> + virtio_stw_p(&cpkt.event, event, VIRTIO_DEVICE(vser));
> + virtio_stw_p(&cpkt.value, value, VIRTIO_DEVICE(vser));
>
> trace_virtio_serial_send_control_event(port_id, event, value);
> return send_control_msg(vser, &cpkt, sizeof(cpkt));
> @@ -291,8 +292,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> return;
> }
>
> - cpkt.event = lduw_p(&gcpkt->event);
> - cpkt.value = lduw_p(&gcpkt->value);
> + cpkt.event = virtio_lduw_p(&gcpkt->event, VIRTIO_DEVICE(vser));
> + cpkt.value = virtio_lduw_p(&gcpkt->value, VIRTIO_DEVICE(vser));
>
> trace_virtio_serial_handle_control_message(cpkt.event, cpkt.value);
>
> @@ -312,10 +313,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> return;
> }
>
> - port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> + port = find_port_by_id(vser, virtio_ldl_p(&gcpkt->id, VIRTIO_DEVICE(vser)));
> if (!port) {
> error_report("virtio-serial-bus: Unexpected port id %u for device %s",
> - ldl_p(&gcpkt->id), vser->bus.qbus.name);
> + virtio_ldl_p(&gcpkt->id, VIRTIO_DEVICE(vser)),
> + vser->bus.qbus.name);
> return;
> }
>
> @@ -342,9 +344,10 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> }
>
> if (port->name) {
> - stl_p(&cpkt.id, port->id);
> - stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
> - stw_p(&cpkt.value, 1);
> + virtio_stl_p(&cpkt.id, port->id, VIRTIO_DEVICE(vser));
> + virtio_stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME,
> + VIRTIO_DEVICE(vser));
> + virtio_stw_p(&cpkt.value, 1, VIRTIO_DEVICE(vser));
>
> buffer_len = sizeof(cpkt) + strlen(port->name) + 1;
> buffer = g_malloc(buffer_len);
> @@ -536,7 +539,7 @@ static void virtio_serial_save(QEMUFile *f, void *opaque)
> qemu_put_be32s(f, &s->config.max_nr_ports);
>
> /* The ports map */
> - max_nr_ports = tswap32(s->config.max_nr_ports);
> + max_nr_ports = virtio_tswap32(s->config.max_nr_ports, VIRTIO_DEVICE(s));
> for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> qemu_put_be32s(f, &s->ports_map[i]);
> }
> @@ -690,8 +693,9 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
> qemu_get_be16s(f, &s->config.rows);
>
> qemu_get_be32s(f, &max_nr_ports);
> - tswap32s(&max_nr_ports);
> - if (max_nr_ports > tswap32(s->config.max_nr_ports)) {
> + virtio_tswap32s(&max_nr_ports, VIRTIO_DEVICE(s));
> + if (max_nr_ports > virtio_tswap32(s->config.max_nr_ports,
> + VIRTIO_DEVICE(s))) {
> /* Source could have had more ports than us. Fail migration. */
> return -EINVAL;
> }
> @@ -760,7 +764,8 @@ static uint32_t find_free_port_id(VirtIOSerial *vser)
> {
> unsigned int i, max_nr_ports;
>
> - max_nr_ports = tswap32(vser->config.max_nr_ports);
> + max_nr_ports = virtio_tswap32(vser->config.max_nr_ports,
> + VIRTIO_DEVICE(vser));
> for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
> uint32_t map, bit;
>
> @@ -848,7 +853,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
> }
> }
>
> - max_nr_ports = tswap32(port->vser->config.max_nr_ports);
> + max_nr_ports = virtio_tswap32(port->vser->config.max_nr_ports,
> + VIRTIO_DEVICE(port->vser));
> if (port->id >= max_nr_ports) {
> error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
> "max. allowed: %u", max_nr_ports - 1);
> @@ -949,7 +955,8 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
> vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
> }
>
> - vser->config.max_nr_ports = tswap32(vser->serial.max_virtserial_ports);
> + vser->config.max_nr_ports =
> + virtio_tswap32(vser->serial.max_virtserial_ports, VIRTIO_DEVICE(vser));
> vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
> * sizeof(vser->ports_map[0]));
> /*
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio
2014-03-31 14:50 ` Alexander Graf
@ 2014-04-01 11:54 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-04-01 11:54 UTC (permalink / raw)
To: Alexander Graf
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
qemu-devel, stefanha, cornelia.huck, pbonzini, anthony, afaerber
On Mon, 31 Mar 2014 16:50:55 +0200
Alexander Graf <agraf@suse.de> wrote:
> On 03/28/2014 11:57 AM, Greg Kurz wrote:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> >
> > virtio data structures are defined as "target endian", which assumes
> > that's a fixed value. In fact, that actually means it's platform-specific.
> > The OASIS virtio 1.0 spec will fix this, by making all little endian.
> >
> > We need to support both implementations and we want to share as much code
> > as possible.
> >
> > A good way to do it is to introduce a per-device boolean property to tell
> > memory accessors whether they should swap bytes or not. This flag should
> > be set at device reset time, because:
> > - endianness won't change while the device is in use, and if we reboot
> > into a different endianness, a new device reset will occur
> > - as suggested by Alexander Graf, we can keep all the logic to set the
> > property in a single place and share all the virtio memory accessors
> > between the two implementations
> >
> > For legacy devices, we rely on a per-platform hook to set the flag. The
> > virtio 1.0 implementation will just have to add some more logic in
> > virtio_reset() instead of calling the hook:
> >
> > if (vdev->legacy) {
> > vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > } else {
> > #ifdef HOST_WORDS_BIGENDIAN
> > vdev->needs_byteswap = true;
> > #else
> > vdev->needs_byteswap = false;
> > #endif
> > }
> >
> > The needs_byteswap flag is preserved accross migrations.
> >
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
> > ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device needs_byteswap flag,
> > add VirtIODevice * arg to virtio helpers,
> > rename virtio_get_byteswap to virtio_legacy_get_byteswap,
> > Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> > hw/virtio/virtio.c | 5 +
> > include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++
> > include/hw/virtio/virtio.h | 3 +
> > stubs/Makefile.objs | 1
> > stubs/virtio_get_byteswap.c | 6 ++
> > 5 files changed, 154 insertions(+)
> > create mode 100644 include/hw/virtio/virtio-access.h
> > create mode 100644 stubs/virtio_get_byteswap.c
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index aeabf3a..24b565f 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -19,6 +19,7 @@
> > #include "hw/virtio/virtio.h"
> > #include "qemu/atomic.h"
> > #include "hw/virtio/virtio-bus.h"
> > +#include "hw/virtio/virtio-access.h"
> >
> > /*
> > * The alignment to use between consumer and producer parts of vring.
> > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque)
> >
> > virtio_set_status(vdev, 0);
> >
> > + vdev->needs_byteswap = virtio_legacy_get_byteswap();
> > +
> > if (k->reset) {
> > k->reset(vdev);
> > }
> > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> >
> > qemu_put_8s(f, &vdev->status);
> > qemu_put_8s(f, &vdev->isr);
> > + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap);
> > qemu_put_be16s(f, &vdev->queue_sel);
> > qemu_put_be32s(f, &vdev->guest_features);
> > qemu_put_be32(f, vdev->config_len);
> > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> >
> > qemu_get_8s(f, &vdev->status);
> > qemu_get_8s(f, &vdev->isr);
> > + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap);
> > qemu_get_be16s(f, &vdev->queue_sel);
> > qemu_get_be32s(f, &features);
> >
> > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h
> > new file mode 100644
> > index 0000000..70dd1e2
> > --- /dev/null
> > +++ b/include/hw/virtio/virtio-access.h
> > @@ -0,0 +1,139 @@
> > +/*
> > + * Virtio Accessor Support: In case your target can change endian.
> > + *
> > + * Copyright IBM, Corp. 2013
> > + *
> > + * Authors:
> > + * Rusty Russell <rusty@au.ibm.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + */
> > +#ifndef _QEMU_VIRTIO_ACCESS_H
> > +#define _QEMU_VIRTIO_ACCESS_H
> > +#include "hw/virtio/virtio.h"
> > +
> > +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap16(lduw_phys(as, pa));
> > + }
> > + return lduw_phys(as, pa);
> > +}
> > +
> > +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap32(ldl_phys(as, pa));
> > + }
> > + return ldl_phys(as, pa);
> > +}
> > +
> > +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap64(ldq_phys(as, pa));
> > + }
> > + return ldq_phys(as, pa);
> > +}
> > +
> > +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + stw_phys(as, pa, bswap16(value));
> > + } else {
> > + stw_phys(as, pa, value);
> > + }
> > +}
> > +
> > +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + stl_phys(as, pa, bswap32(value));
> > + } else {
> > + stl_phys(as, pa, value);
> > + }
> > +}
> > +
> > +static inline void virtio_stw_p(void *ptr, uint16_t v,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + stw_p(ptr, bswap16(v));
> > + } else {
> > + stw_p(ptr, v);
> > + }
> > +}
> > +
> > +static inline void virtio_stl_p(void *ptr, uint32_t v,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + stl_p(ptr, bswap32(v));
> > + } else {
> > + stl_p(ptr, v);
> > + }
> > +}
> > +
> > +static inline void virtio_stq_p(void *ptr, uint64_t v,
> > + struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + stq_p(ptr, bswap64(v));
> > + } else {
> > + stq_p(ptr, v);
> > + }
> > +}
> > +
> > +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap16(lduw_p(ptr));
> > + } else {
> > + return lduw_p(ptr);
> > + }
> > +}
> > +
> > +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap32(ldl_p(ptr));
> > + } else {
> > + return ldl_p(ptr);
> > + }
> > +}
> > +
> > +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap64(ldq_p(ptr));
> > + } else {
> > + return ldq_p(ptr);
> > + }
> > +}
> > +
> > +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev)
> > +{
> > + if (vdev->needs_byteswap) {
> > + return bswap32(tswap32(s));
> > + } else {
> > + return tswap32(s);
> > + }
> > +}
> > +
> > +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev)
> > +{
> > + tswap32s(s);
> > + if (vdev->needs_byteswap) {
> > + *s = bswap32(*s);
> > + }
>
> Couldn't you just reuse virtio_tswap32() here?
>
Yes. Good catch !
> Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't
> have any access to CPU endian specific RAM accessors.
>
> How about we introduce a flag like "is_bigendian" that we set depending
> on the CPU default endianness for legacy virtio. We can then change that
> flag accordingly. For the wrappers here, we could just use accessors
> like ldl_be_phys() or ldl_le_phys().
>
> That should make things a lot easier to understand - and for virtio 1.0
> we only need to set is_bigendian = false :).
>
Yeah definitely !
>
> Alex
>
>
Thanks.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access
2014-03-31 16:26 ` Andreas Färber
@ 2014-04-01 12:03 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2014-04-01 12:03 UTC (permalink / raw)
To: Andreas Färber
Cc: kwolf, peter.maydell, thuth, mst, marc.zyngier, rusty,
Alexander Graf, qemu-devel, stefanha, cornelia.huck, pbonzini,
anthony
On Mon, 31 Mar 2014 18:26:54 +0200
Andreas Färber <afaerber@suse.de> wrote:
> Am 31.03.2014 18:24, schrieb Alexander Graf:
> > On 03/28/2014 11:57 AM, Greg Kurz wrote:
> >> From: Rusty Russell <rusty@rustcorp.com.au>
> >>
> >> This is based on a simpler patch by Anthony Liguouri, which only handled
> >> the vring accesses. We also need some drivers to access these helpers,
> >> eg. for data which contains headers.
> >>
> >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> >> [ ldq_phys() API change,
> >> use per-device needs_byteswap flag,
> >> Greg Kurz <gkurz@linux.vnet.ibm.com> ]
> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> >> ---
> >> hw/virtio/virtio.c | 93
> >> ++++++++++++++++++++++++++++++----------------------
> >> 1 file changed, 53 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index 24b565f..1877b46 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -102,53 +102,57 @@ static void virtqueue_init(VirtQueue *vq)
> >> vq->vring.align);
> >> }
> >> -static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i)
> >> +static inline uint64_t vring_desc_addr(hwaddr desc_pa, int i,
> >> + struct VirtIODevice *vdev)
> >
> > The logical ordering for helper is usually to have the device as the
> > first parameter (it's basically the self object). Could you please order
> > it accordingly?
>
> Also please note that QEMU Coding Style requests the use of typedefs,
> i.e. drop the "struct".
>
> Regards,
> Andreas
>
>
Oops my bad... I did not know about the HACKING file until your mail. :)
I will fixe that.
Thanks.
--
Gregory Kurz kurzgreg@fr.ibm.com
gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2014-04-01 12:03 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-28 10:57 [Qemu-devel] [PATCH v6 0/8] virtio endian-ambivalent target fixes Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio Greg Kurz
2014-03-28 14:15 ` Thomas Huth
2014-03-28 15:40 ` Greg Kurz
2014-03-28 17:59 ` Andreas Färber
2014-03-28 19:00 ` Greg Kurz
2014-03-31 14:50 ` Alexander Graf
2014-04-01 11:54 ` Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 2/8] virtio: allow byte swapping for vring and config access Greg Kurz
2014-03-28 16:07 ` Thomas Huth
2014-03-28 17:02 ` Greg Kurz
2014-03-31 16:24 ` Alexander Graf
2014-03-31 16:26 ` Andreas Färber
2014-04-01 12:03 ` Greg Kurz
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 3/8] virtio-net: use virtio wrappers to access headers Greg Kurz
2014-03-31 16:28 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 4/8] virtio-balloon: use virtio wrappers to access page frame numbers Greg Kurz
2014-03-31 16:30 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 5/8] virtio-blk: use virtio wrappers to access headers Greg Kurz
2014-03-31 16:31 ` Alexander Graf
2014-03-28 10:57 ` [Qemu-devel] [PATCH v6 6/8] virtio-scsi: " Greg Kurz
2014-03-28 17:13 ` Greg Kurz
2014-03-28 17:21 ` Andreas Färber
2014-03-28 17:37 ` Greg Kurz
2014-03-28 17:43 ` Peter Maydell
2014-03-28 18:04 ` Greg Kurz
2014-03-28 18:14 ` Peter Maydell
2014-03-28 18:58 ` Greg Kurz
2014-03-31 16:34 ` Alexander Graf
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 7/8] virtio-serial-bus: " Greg Kurz
2014-03-31 17:01 ` Alexander Graf
2014-03-28 10:58 ` [Qemu-devel] [PATCH v6 8/8] virtio-9p: " Greg Kurz
2014-03-28 11:22 ` [Qemu-devel] [PATCH v4] target-ppc: ppc64 target's virtio can be either endian Greg Kurz
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.