2009-12-08 20:57:34

by Ira W. Snyder

[permalink] [raw]
Subject: alacrityvm-next: clearly specify ioq endianness


This is a respin of the previous patch series against alacrityvm.git's
linux-next branch, to facilitate easier merging with the linux-next tree.

The SHM_SIGNAL and IOQ code is meant to be used for communication channels
between systems with different bit widths and operating systems.
Conceivably, this could include systems with different endianness as well,
for example, using QEMU on x86 to emulate a powerpc.

This patch series annotates all shared structures as little endian, and
fixes up all users. This means that there is no overhead in the most common
case, x86-on-x86. The byteswaps compile out.

[RESEND: added LKML to CC per Greg Haskins' request]

drivers/net/vbus-enet.c | 36 ++++++++++++++++++------------------
drivers/vbus/bus-proxy.c | 2 +-
drivers/vbus/pci-bridge.c | 18 +++++++++---------
include/linux/ioq.h | 24 ++++++++++++------------
include/linux/shm_signal.h | 8 ++++----
lib/ioq.c | 28 ++++++++++++++++------------
6 files changed, 60 insertions(+), 56 deletions(-)

Thanks,
Ira


2009-12-08 20:57:42

by Ira W. Snyder

[permalink] [raw]
Subject: [PATCH] vbus: fix lots of sparse "dubious signed bitfield" warnings

The sparse utility gave tons of warnings about signed bitfields. A simple
inspection shows that they are all used as booleans, so convert them to the
correct type.

Signed-off-by: Ira W. Snyder <[email protected]>
---
drivers/vbus/pci-bridge.c | 2 +-
include/linux/ioq.h | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vbus/pci-bridge.c b/drivers/vbus/pci-bridge.c
index 9e37df1..aa683ae 100644
--- a/drivers/vbus/pci-bridge.c
+++ b/drivers/vbus/pci-bridge.c
@@ -43,7 +43,7 @@ struct vbus_pci {
struct vbus_pci_regs *regs;
struct vbus_pci_signals *signals;
int irq;
- int enabled:1;
+ bool enabled;
struct {
struct dentry *fs;
int events;
diff --git a/include/linux/ioq.h b/include/linux/ioq.h
index eba1021..f04dfb4 100644
--- a/include/linux/ioq.h
+++ b/include/linux/ioq.h
@@ -112,9 +112,9 @@ struct ioq_iterator {
struct ioq_ring_idx *idx;
u32 pos;
struct ioq_ring_desc *desc;
- int update:1;
- int dualidx:1;
- int flipowner:1;
+ bool update;
+ bool dualidx;
+ bool flipowner;
};

struct ioq_notifier {
--
1.5.4.3

2009-12-08 20:57:55

by Ira W. Snyder

[permalink] [raw]
Subject: [PATCH] vbus: pci-bridge: fix sparse warnings

The sparse tool caught many missing static declarations. Add them.

There are still many address space errors, but I'm unsure about the best
way to fix these.

Signed-off-by: Ira W. Snyder <[email protected]>
---
drivers/vbus/pci-bridge.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/vbus/pci-bridge.c b/drivers/vbus/pci-bridge.c
index aa683ae..078b8f4 100644
--- a/drivers/vbus/pci-bridge.c
+++ b/drivers/vbus/pci-bridge.c
@@ -66,7 +66,7 @@ struct vbus_pci_device {
struct work_struct drop;
};

-DEFINE_PER_CPU(struct vbus_pci_fastcall_desc, vbus_pci_percpu_fastcall)
+static DEFINE_PER_CPU(struct vbus_pci_fastcall_desc, vbus_pci_percpu_fastcall)
____cacheline_aligned;

/*
@@ -123,7 +123,7 @@ vbus_pci_buscall(unsigned long nr, void *data, unsigned long len)
return ret;
}

-struct vbus_pci_device *
+static struct vbus_pci_device *
to_dev(struct vbus_device_proxy *vdev)
{
return container_of(vdev, struct vbus_pci_device, vdev);
@@ -412,7 +412,7 @@ vbus_pci_device_release(struct vbus_device_proxy *vdev)
kfree(_dev);
}

-struct vbus_device_proxy_ops vbus_pci_device_ops = {
+static struct vbus_device_proxy_ops vbus_pci_device_ops = {
.open = vbus_pci_device_open,
.close = vbus_pci_device_close,
.shm = vbus_pci_device_shm,
@@ -667,7 +667,7 @@ static struct ioq_notifier eventq_notifier = {
};

/* Injected whenever the host issues an ioq_signal() on the eventq */
-irqreturn_t
+static irqreturn_t
eventq_intr(int irq, void *dev)
{
vbus_pci.stats.qnotify++;
@@ -995,7 +995,7 @@ static struct pci_driver vbus_pci_driver = {
.remove = vbus_pci_remove,
};

-int __init
+static int __init
vbus_pci_init(void)
{
memset(&vbus_pci, 0, sizeof(vbus_pci));
--
1.5.4.3

2009-12-08 20:58:02

by Ira W. Snyder

[permalink] [raw]
Subject: [PATCH] vbus-enet: fix sparse warnings

Fix the following sparse warnings:
drivers/net/vbus-enet.c:411:9: warning: Using plain integer as NULL pointer
drivers/net/vbus-enet.c:1250:24: warning: Using plain integer as NULL pointer
drivers/net/vbus-enet.c:1326:24: warning: Using plain integer as NULL pointer

Signed-off-by: Ira W. Snyder <[email protected]>
---
drivers/net/vbus-enet.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vbus-enet.c b/drivers/net/vbus-enet.c
index 4e9ecac..8232215 100644
--- a/drivers/net/vbus-enet.c
+++ b/drivers/net/vbus-enet.c
@@ -408,7 +408,7 @@ tx_setup(struct vbus_enet_priv *priv)
priv->pmtd.pool = pool;

ret = dev->ops->shm(dev, NULL, shmid, 0, pool, poollen,
- 0, NULL, 0);
+ NULL, NULL, 0);
BUG_ON(ret < 0);
}

@@ -1239,7 +1239,7 @@ vbus_enet_evq_negcap(struct vbus_enet_priv *priv, unsigned long count)
priv->evq.pool = pool;

ret = dev->ops->shm(dev, NULL, query.dpid, 0,
- pool, poollen, 0, NULL, 0);
+ pool, poollen, NULL, NULL, 0);
if (ret < 0)
return ret;

@@ -1315,7 +1315,7 @@ vbus_enet_l4ro_negcap(struct vbus_enet_priv *priv, unsigned long count)
* pre-mapped descriptor pool
*/
ret = dev->ops->shm(dev, NULL, query.dpid, 0,
- pool, poollen, 0, NULL, 0);
+ pool, poollen, NULL, NULL, 0);
if (ret < 0) {
printk(KERN_ERR "Error registering L4RO pool: %d\n",
ret);
--
1.5.4.3

2009-12-08 20:57:49

by Ira W. Snyder

[permalink] [raw]
Subject: [PATCH] shm_signal: clearly specify endianness

The shm_signal code uses structures which are designed to be shared between
disparate systems, such as 32-bit and 64-bit, as well as Linux and Windows.

Since shm_signal is primarily intended to be used by qemu/kvm, which
supports virtual guests with a different CPU architecture than the host,
clearly define an endianness for the shared structures.

The endianness is defined to be little-endian, to avoid byte swapping in
the most common case: x86.

Signed-off-by: Ira W. Snyder <[email protected]>
---
include/linux/shm_signal.h | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/shm_signal.h b/include/linux/shm_signal.h
index 21cf750..b2efd72 100644
--- a/include/linux/shm_signal.h
+++ b/include/linux/shm_signal.h
@@ -32,8 +32,8 @@
*-----------
*/

-#define SHM_SIGNAL_MAGIC 0x58fa39df
-#define SHM_SIGNAL_VER 1
+#define SHM_SIGNAL_MAGIC cpu_to_le32(0x58fa39df)
+#define SHM_SIGNAL_VER cpu_to_le32(1)

struct shm_signal_irq {
__u8 enabled;
@@ -47,8 +47,8 @@ enum shm_signal_locality {
};

struct shm_signal_desc {
- __u32 magic;
- __u32 ver;
+ __le32 magic;
+ __le32 ver;
struct shm_signal_irq irq[2];
};

--
1.5.4.3

2009-12-08 20:58:07

by Ira W. Snyder

[permalink] [raw]
Subject: [PATCH] ioq: clearly specify endianness

The IOQ code uses structures which are designed to be shared between
disparate systems, such as 32-bit and 64-bit, as well as Linux and Windows.

Since IOQ is primarily intended to be used by qemu/kvm, which support
virtual guests with a different CPU architecture than the host, clearly
define the endianness for the shared structures.

The endianness is defined to be little-endian, to avoid byte swapping in
the most common case: x86.

Note that the cookie member was not changed to have a fixed endianness.
This is because it is only intended for use by one side of the IOQ.

Signed-off-by: Ira W. Snyder <[email protected]>
---
drivers/net/vbus-enet.c | 30 +++++++++++++++---------------
drivers/vbus/bus-proxy.c | 2 +-
drivers/vbus/pci-bridge.c | 6 +++---
include/linux/ioq.h | 18 +++++++++---------
lib/ioq.c | 28 ++++++++++++++++------------
5 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/net/vbus-enet.c b/drivers/net/vbus-enet.c
index 8232215..94b86d4 100644
--- a/drivers/net/vbus-enet.c
+++ b/drivers/net/vbus-enet.c
@@ -175,8 +175,8 @@ rxdesc_alloc(struct vbus_enet_priv *priv, struct ioq_ring_desc *desc, size_t len
iov->len = len;
} else {
desc->cookie = (u64)(unsigned long)skb;
- desc->ptr = (u64)__pa(skb->data);
- desc->len = len; /* total length */
+ desc->ptr = cpu_to_le64(__pa(skb->data));
+ desc->len = cpu_to_le64(len); /* total length */
}

desc->valid = 1;
@@ -212,8 +212,8 @@ rx_pageq_refill(struct vbus_enet_priv *priv, gfp_t gfp_mask)

added = 1;
iter.desc->cookie = (u64)(unsigned long)page;
- iter.desc->ptr = (u64)__pa(page_address(page));
- iter.desc->len = PAGE_SIZE;
+ iter.desc->ptr = cpu_to_le64(__pa(page_address(page)));
+ iter.desc->len = cpu_to_le64(PAGE_SIZE);

ret = ioq_iter_push(&iter, 0);
BUG_ON(ret < 0);
@@ -257,9 +257,9 @@ rx_setup(struct vbus_enet_priv *priv)
size_t offset = (i * SG_DESC_SIZE);
void *addr = &priv->l4ro.pool[offset];

- iter.desc->ptr = (u64)offset;
+ iter.desc->ptr = cpu_to_le64(offset);
iter.desc->cookie = (u64)(unsigned long)addr;
- iter.desc->len = SG_DESC_SIZE;
+ iter.desc->len = cpu_to_le64(SG_DESC_SIZE);
}

rxdesc_alloc(priv, iter.desc, priv->dev->mtu);
@@ -428,17 +428,17 @@ tx_setup(struct vbus_enet_priv *priv)
size_t offset = (i * SG_DESC_SIZE);

vsg = (struct venet_sg *)&priv->pmtd.pool[offset];
- iter.desc->ptr = (u64)offset;
+ iter.desc->ptr = cpu_to_le64(offset);
} else {
vsg = kzalloc(SG_DESC_SIZE, GFP_KERNEL);
if (!vsg)
return -ENOMEM;

- iter.desc->ptr = (u64)__pa(vsg);
+ iter.desc->ptr = cpu_to_le64(__pa(vsg));
}

iter.desc->cookie = (u64)(unsigned long)vsg;
- iter.desc->len = SG_DESC_SIZE;
+ iter.desc->len = cpu_to_le64(SG_DESC_SIZE);

ret = ioq_iter_seek(&iter, ioq_seek_next, 0, 0);
BUG_ON(ret < 0);
@@ -708,7 +708,7 @@ vbus_enet_flat_import(struct vbus_enet_priv *priv, struct ioq_ring_desc *desc)
return NULL;
}

- skb_put(skb, desc->len);
+ skb_put(skb, le64_to_cpu(desc->len));

return skb;
}
@@ -871,7 +871,7 @@ vbus_enet_tx_start(struct sk_buff *skb, struct net_device *dev)
iov->ptr = (u64)sg_phys(sg);
}

- iter.desc->len = (u64)VSG_DESC_SIZE(vsg->count);
+ iter.desc->len = cpu_to_le64(VSG_DESC_SIZE(vsg->count));

} else {
/*
@@ -879,8 +879,8 @@ vbus_enet_tx_start(struct sk_buff *skb, struct net_device *dev)
* ring.
*/
iter.desc->cookie = (u64)(unsigned long)skb;
- iter.desc->len = (u64)skb->len;
- iter.desc->ptr = (u64)__pa(skb->data);
+ iter.desc->len = cpu_to_le64(skb->len);
+ iter.desc->ptr = cpu_to_le64(__pa(skb->data));
}

iter.desc->valid = 1;
@@ -1258,9 +1258,9 @@ vbus_enet_evq_negcap(struct vbus_enet_priv *priv, unsigned long count)
size_t offset = (i * query.evsize);
void *addr = &priv->evq.pool[offset];

- iter.desc->ptr = (u64)offset;
+ iter.desc->ptr = cpu_to_le64(offset);
iter.desc->cookie = (u64)(unsigned long)addr;
- iter.desc->len = query.evsize;
+ iter.desc->len = cpu_to_le64(query.evsize);

ret = ioq_iter_push(&iter, 0);
BUG_ON(ret < 0);
diff --git a/drivers/vbus/bus-proxy.c b/drivers/vbus/bus-proxy.c
index a318c67..4792842 100644
--- a/drivers/vbus/bus-proxy.c
+++ b/drivers/vbus/bus-proxy.c
@@ -217,7 +217,7 @@ int vbus_driver_ioq_alloc(struct vbus_device_proxy *dev, const char *name,

head->magic = IOQ_RING_MAGIC;
head->ver = IOQ_RING_VER;
- head->count = count;
+ head->count = cpu_to_le32(count);

ret = dev->ops->shm(dev, name, id, prio, head, len,
&head->signal, &signal, 0);
diff --git a/drivers/vbus/pci-bridge.c b/drivers/vbus/pci-bridge.c
index 078b8f4..0d51324 100644
--- a/drivers/vbus/pci-bridge.c
+++ b/drivers/vbus/pci-bridge.c
@@ -579,8 +579,8 @@ eventq_init(int qlen)
BUG_ON(iter.desc->valid);

desc->cookie = (u64)(unsigned long)event;
- desc->ptr = (u64)__pa(event);
- desc->len = len; /* total length */
+ desc->ptr = cpu_to_le64(__pa(event));
+ desc->len = cpu_to_le64(len); /* total length */
desc->valid = 1;

/*
@@ -798,7 +798,7 @@ _ioq_init(size_t ringsize, struct ioq *ioq, struct ioq_ops *ops)

head->magic = IOQ_RING_MAGIC;
head->ver = IOQ_RING_VER;
- head->count = ringsize;
+ head->count = cpu_to_le32(ringsize);

_signal_init(signal, &head->signal, &eventq_signal_ops);

diff --git a/include/linux/ioq.h b/include/linux/ioq.h
index f04dfb4..7c6d6ca 100644
--- a/include/linux/ioq.h
+++ b/include/linux/ioq.h
@@ -52,18 +52,18 @@
*/
struct ioq_ring_desc {
__u64 cookie; /* for arbitrary use by north-side */
- __u64 ptr;
- __u64 len;
+ __le64 ptr;
+ __le64 len;
__u8 valid;
__u8 sown; /* South owned = 1, North owned = 0 */
};

-#define IOQ_RING_MAGIC 0x47fa2fe4
-#define IOQ_RING_VER 4
+#define IOQ_RING_MAGIC cpu_to_le32(0x47fa2fe4)
+#define IOQ_RING_VER cpu_to_le32(4)

struct ioq_ring_idx {
- __u32 head; /* 0 based index to head of ptr array */
- __u32 tail; /* 0 based index to tail of ptr array */
+ __le32 head; /* 0 based index to head of ptr array */
+ __le32 tail; /* 0 based index to tail of ptr array */
__u8 full;
};

@@ -73,11 +73,11 @@ enum ioq_locality {
};

struct ioq_ring_head {
- __u32 magic;
- __u32 ver;
+ __le32 magic;
+ __le32 ver;
struct shm_signal_desc signal;
struct ioq_ring_idx idx[2];
- __u32 count;
+ __le32 count;
struct ioq_ring_desc ring[1]; /* "count" elements will be allocated */
};

diff --git a/lib/ioq.c b/lib/ioq.c
index d5e57be..4027848 100644
--- a/lib/ioq.c
+++ b/lib/ioq.c
@@ -71,10 +71,10 @@ int ioq_iter_seek(struct ioq_iterator *iter, enum ioq_seek_type type,
pos = modulo_inc(iter->pos, iter->ioq->count);
break;
case ioq_seek_tail:
- pos = idx->tail;
+ pos = le32_to_cpu(idx->tail);
break;
case ioq_seek_head:
- pos = idx->head;
+ pos = le32_to_cpu(idx->head);
break;
case ioq_seek_set:
if (offset >= iter->ioq->count)
@@ -91,19 +91,23 @@ EXPORT_SYMBOL_GPL(ioq_iter_seek);

static int ioq_ring_count(struct ioq_ring_idx *idx, int count)
{
- if (idx->full && (idx->head == idx->tail))
+ u32 head = le32_to_cpu(idx->head);
+ u32 tail = le32_to_cpu(idx->tail);
+
+ if (idx->full && (head == tail))
return count;
- else if (idx->tail >= idx->head)
- return idx->tail - idx->head;
+ else if (tail >= head)
+ return tail - head;
else
- return (idx->tail + count) - idx->head;
+ return (tail + count) - head;
}

static void idx_tail_push(struct ioq_ring_idx *idx, int count)
{
- u32 tail = modulo_inc(idx->tail, count);
+ u32 tail = modulo_inc(le32_to_cpu(idx->tail), count);
+ u32 head = le32_to_cpu(idx->head);

- if (idx->head == tail) {
+ if (head == tail) {
rmb();

/*
@@ -116,7 +120,7 @@ static void idx_tail_push(struct ioq_ring_idx *idx, int count)
wmb();
}

- idx->tail = tail;
+ idx->tail = cpu_to_le32(tail);
}

int ioq_iter_push(struct ioq_iterator *iter, int flags)
@@ -128,7 +132,7 @@ int ioq_iter_push(struct ioq_iterator *iter, int flags)
/*
* Its only valid to push if we are currently pointed at the tail
*/
- if (iter->pos != idx->tail || iter->desc->sown != iter->ioq->locale)
+ if (iter->pos != le32_to_cpu(idx->tail) || iter->desc->sown != iter->ioq->locale)
return -EINVAL;

idx_tail_push(idx, iter->ioq->count);
@@ -167,10 +171,10 @@ int ioq_iter_pop(struct ioq_iterator *iter, int flags)
/*
* Its only valid to pop if we are currently pointed at the head
*/
- if (iter->pos != idx->head || iter->desc->sown != iter->ioq->locale)
+ if (iter->pos != le32_to_cpu(idx->head) || iter->desc->sown != iter->ioq->locale)
return -EINVAL;

- idx->head = modulo_inc(idx->head, iter->ioq->count);
+ idx->head = cpu_to_le32(modulo_inc(le32_to_cpu(idx->head), iter->ioq->count));
wmb(); /* head must be visible before full */

if (idx->full) {
--
1.5.4.3

2009-12-09 18:23:15

by Gregory Haskins

[permalink] [raw]
Subject: Re: [Alacrityvm-devel] alacrityvm-next: clearly specify ioq endianness

On 12/8/09 3:57 PM, Ira W. Snyder wrote:
>
> This is a respin of the previous patch series against alacrityvm.git's
> linux-next branch, to facilitate easier merging with the linux-next tree.
>
> The SHM_SIGNAL and IOQ code is meant to be used for communication channels
> between systems with different bit widths and operating systems.
> Conceivably, this could include systems with different endianness as well,
> for example, using QEMU on x86 to emulate a powerpc.
>
> This patch series annotates all shared structures as little endian, and
> fixes up all users. This means that there is no overhead in the most common
> case, x86-on-x86. The byteswaps compile out.
>
> [RESEND: added LKML to CC per Greg Haskins' request]

Applied, thanks!

-Greg


Attachments:
signature.asc (267.00 B)
OpenPGP digital signature