[PATCH 0/11] RFC: PCI using capabilitities

[PATCH 0/11] RFC: PCI using capabilitities

[Rusty Russell]

Here's the patch series I ended up with.  I haven't coded up the QEMU
side yet, so no idea if the new driver works.

Questions:
(1) Do we win from separating ISR, NOTIFY and COMMON?
(2) I used a "u8 bar"; should I use a bir and pack it instead?  BIR
    seems a little obscure (noone else in the kernel source seems to
    refer to it).

Cheers,
Rusty.

[RFC 1/11] virtio: use u32, not bitmap for struct virtio_device's features

[Rusty Russell]

It seemed like a good idea, but it's actually a pain when we get more
than 32 feature bits.  Just change it to a u32 for now.
---
 drivers/char/virtio_console.c  |    2 +-
 drivers/lguest/lguest_device.c |    2 +-
 drivers/s390/kvm/kvm_virtio.c  |    2 +-
 drivers/virtio/virtio.c        |   10 +++++-----
 drivers/virtio/virtio_mmio.c   |    8 ++------
 drivers/virtio/virtio_pci.c    |    3 +--
 drivers/virtio/virtio_ring.c   |    2 +-
 include/linux/virtio.h         |    3 +--
 include/linux/virtio_config.h  |    2 +-
 tools/virtio/linux/virtio.h    |   18 ++----------------
 tools/virtio/virtio_test.c     |    5 ++---
 11 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -331,7 +331,7 @@ static inline bool use_multiport(struct 
 	 */
 	if (!portdev->vdev)
 		return 0;
-	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
+	return portdev->vdev->features & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static void free_buf(struct port_buffer *buf)
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -144,7 +144,7 @@ static void lg_finalize_features(struct 
 	memset(out_features, 0, desc->feature_len);
 	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (test_bit(i, vdev->features))
+		if (vdev->features & (1 << i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
 
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -105,7 +105,7 @@ static void kvm_finalize_features(struct
 	memset(out_features, 0, desc->feature_len);
 	bits = min_t(unsigned, desc->feature_len, sizeof(vdev->features)) * 8;
 	for (i = 0; i < bits; i++) {
-		if (test_bit(i, vdev->features))
+		if (vdev->features & (1 << i))
 			out_features[i / 8] |= (1 << (i % 8));
 	}
 }
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -41,9 +41,9 @@ static ssize_t features_show(struct devi
 
 	/* We actually represent this as a bitstring, as it could be
 	 * arbitrary length in future. */
-	for (i = 0; i < ARRAY_SIZE(dev->features)*BITS_PER_LONG; i++)
+	for (i = 0; i < sizeof(dev->features)*8; i++)
 		len += sprintf(buf+len, "%c",
-			       test_bit(i, dev->features) ? '1' : '0');
+			       dev->features & (1ULL << i) ? '1' : '0');
 	len += sprintf(buf+len, "\n");
 	return len;
 }
@@ -122,18 +122,18 @@ static int virtio_dev_probe(struct devic
 	device_features = dev->config->get_features(dev);
 
 	/* Features supported by both device and driver into dev->features. */
-	memset(dev->features, 0, sizeof(dev->features));
+	dev->features = 0;
 	for (i = 0; i < drv->feature_table_size; i++) {
 		unsigned int f = drv->feature_table[i];
 		BUG_ON(f >= 32);
 		if (device_features & (1 << f))
-			set_bit(f, dev->features);
+			dev->features |= (1 << f);
 	}
 
 	/* Transport features always preserved to pass to finalize_features. */
 	for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++)
 		if (device_features & (1 << i))
-			set_bit(i, dev->features);
+			dev->features |= (1 << i);
 
 	dev->config->finalize_features(dev);
 
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -112,16 +112,12 @@ static u32 vm_get_features(struct virtio
 static void vm_finalize_features(struct virtio_device *vdev)
 {
 	struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
-	int i;
 
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	for (i = 0; i < ARRAY_SIZE(vdev->features); i++) {
-		writel(i, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
-		writel(vdev->features[i],
-				vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
-	}
+	writel(0, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES_SEL);
+	writel(vdev->features, vm_dev->base + VIRTIO_MMIO_GUEST_FEATURES);
 }
 
 static void vm_get(struct virtio_device *vdev, unsigned offset,
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -121,8 +121,7 @@ static void vp_finalize_features(struct 
 	vring_transport_features(vdev);
 
 	/* We only support 32 feature bits. */
-	BUILD_BUG_ON(ARRAY_SIZE(vdev->features) != 1);
-	iowrite32(vdev->features[0], vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
+	iowrite32(vdev->features, vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
 }
 
 /* virtio config->get() implementation */
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -685,7 +685,7 @@ void vring_transport_features(struct vir
 			break;
 		default:
 			/* We don't understand this bit. */
-			clear_bit(i, vdev->features);
+			vdev->features &= ~(1 << i);
 		}
 	}
 }
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -66,8 +66,7 @@ struct virtio_device {
 	struct virtio_device_id id;
 	struct virtio_config_ops *config;
 	struct list_head vqs;
-	/* Note that this is a Linux set_bit-style bitmap. */
-	unsigned long features[1];
+	u32 features;
 	void *priv;
 };
 
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -142,7 +142,7 @@ static inline bool virtio_has_feature(co
 	if (fbit < VIRTIO_TRANSPORT_F_START)
 		virtio_check_driver_offered_feature(vdev, fbit);
 
-	return test_bit(fbit, vdev->features);
+	return vdev->features & (1 << fbit);
 }
 
 /**
diff --git a/tools/virtio/linux/virtio.h b/tools/virtio/linux/virtio.h
--- a/tools/virtio/linux/virtio.h
+++ b/tools/virtio/linux/virtio.h
@@ -131,29 +131,15 @@ static inline void kfree(void *p)
 #define BITS_PER_BYTE		8
 #define BITS_PER_LONG (sizeof(long) * BITS_PER_BYTE)
 #define BIT_MASK(nr)		(1UL << ((nr) % BITS_PER_LONG))
-/* TODO: Not atomic as it should be:
- * we don't use this for anything important. */
-static inline void clear_bit(int nr, volatile unsigned long *addr)
-{
-	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
-
-	*p &= ~mask;
-}
-
-static inline int test_bit(int nr, const volatile unsigned long *addr)
-{
-        return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
-}
 
 /* The only feature we care to support */
 #define virtio_has_feature(dev, feature) \
-	test_bit((feature), (dev)->features)
+	((dev)->features & (1 << (feature)))
 /* end of stubs */
 
 struct virtio_device {
 	void *dev;
-	unsigned long features[1];
+	u32 features;
 };
 
 struct virtqueue {
diff --git a/tools/virtio/virtio_test.c b/tools/virtio/virtio_test.c
--- a/tools/virtio/virtio_test.c
+++ b/tools/virtio/virtio_test.c
@@ -55,7 +55,7 @@ void vhost_vq_setup(struct vdev_info *de
 {
 	struct vhost_vring_state state = { .index = info->idx };
 	struct vhost_vring_file file = { .index = info->idx };
-	unsigned long long features = dev->vdev.features[0];
+	unsigned long long features = dev->vdev.features;
 	struct vhost_vring_addr addr = {
 		.index = info->idx,
 		.desc_user_addr = (uint64_t)(unsigned long)info->vring.desc,
@@ -106,8 +106,7 @@ static void vdev_info_init(struct vdev_i
 {
 	int r;
 	memset(dev, 0, sizeof *dev);
-	dev->vdev.features[0] = features;
-	dev->vdev.features[1] = features >> 32;
+	dev->vdev.features = features;
 	dev->buf_size = 1024;
 	dev->buf = malloc(dev->buf_size);
 	assert(dev->buf);

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Sasha Levin]

Rusty, I can't find the actual patches, could you verify that they were
indeed sent?

On Thu, 2011-12-08 at 20:52 +1030, Rusty Russell wrote:
> Here's the patch series I ended up with.  I haven't coded up the QEMU
> side yet, so no idea if the new driver works.
> 
> Questions:
> (1) Do we win from separating ISR, NOTIFY and COMMON?

By separating ISR, NOTIFY and COMMON we can place ISR and NOTIFY in PIO
and COMMON in MMIO. This gives us the benefit of having the small data
path use fast PIO, while big config path can use MMIO.

> (2) I used a "u8 bar"; should I use a bir and pack it instead?  BIR
>     seems a little obscure (noone else in the kernel source seems to
>     refer to it).

BIR is a concept from the PCI spec, but it was only used for MSI-X. I
don't expect to see it all around the kernel source.

-- 

Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Sasha Levin]

On Thu, 2011-12-08 at 20:52 +1030, Rusty Russell wrote:
> Here's the patch series I ended up with.  I haven't coded up the QEMU
> side yet, so no idea if the new driver works.
> 
> Questions:
> (1) Do we win from separating ISR, NOTIFY and COMMON?
> (2) I used a "u8 bar"; should I use a bir and pack it instead?  BIR
>     seems a little obscure (noone else in the kernel source seems to
>     refer to it).

I started implementing it for KVM tools, when I noticed a strange thing:
my vq creating was failing because the driver was reading a value other
than 0 from the address field of a new vq, and failing.

I've added simple prints in the usermode code, and saw the following
ordering:

1. queue select vq 0
2. queue read address (returns 0 - new vq)
3. queue write address (good address of vq)
4. queue read address (returns !=0, fails)
4. queue select vq 1

>From that I understood that the ordering is wrong, the driver was trying
to read address before selecting the correct vq.

At that point, I've added simple prints to the driver. Initially it
looked as follows:

	iowrite16(index, &vp_dev->common->queue_select);

	switch (ioread64(&vp_dev->common->queue_address)) {
		[...]
	};

So I added prints before the iowrite16() and after the ioread64(), and
saw that while the driver prints were ordered, the device ones weren't:

	[    1.264052] before iowrite index=1
	kvmtool: net returning pfn (vq=0): 310706176
	kvmtool: queue selected: 1
	[    1.264890] after ioread index=1

Suspecting that something was wrong with ordering, I've added a print
between the iowrite and the ioread, and it finally started working well.

Which leads me to the question: Are MMIO vs MMIO reads/writes not
ordered?

-- 

Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Rusty Russell]

On Thu, 08 Dec 2011 17:37:37 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> Which leads me to the question: Are MMIO vs MMIO reads/writes not
> ordered?

That seems really odd, especially being repeatable.

BTW, that's an address, not a pfn now.

Cheers,
Rusty.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Sasha Levin]

On Fri, 2011-12-09 at 16:47 +1030, Rusty Russell wrote:
> On Thu, 08 Dec 2011 17:37:37 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> > Which leads me to the question: Are MMIO vs MMIO reads/writes not
> > ordered?
> 
> That seems really odd, especially being repeatable.

Happens every single time. Can't be a coincidence.

I even went into paranoia mode and made sure that both IO requests come
from the same vcpu.

Another weird thing I've noticed is that mb() doesn't fix it, while if I
replace the mb() with a printk() it works well.

> BTW, that's an address, not a pfn now.

Fixed :)

-- 

Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Avi Kivity]

On 12/08/2011 05:37 PM, Sasha Levin wrote:
> On Thu, 2011-12-08 at 20:52 +1030, Rusty Russell wrote:
> > Here's the patch series I ended up with.  I haven't coded up the QEMU
> > side yet, so no idea if the new driver works.
> > 
> > Questions:
> > (1) Do we win from separating ISR, NOTIFY and COMMON?
> > (2) I used a "u8 bar"; should I use a bir and pack it instead?  BIR
> >     seems a little obscure (noone else in the kernel source seems to
> >     refer to it).
>
> I started implementing it for KVM tools, when I noticed a strange thing:
> my vq creating was failing because the driver was reading a value other
> than 0 from the address field of a new vq, and failing.
>
> I've added simple prints in the usermode code, and saw the following
> ordering:
>
> 1. queue select vq 0
> 2. queue read address (returns 0 - new vq)
> 3. queue write address (good address of vq)
> 4. queue read address (returns !=0, fails)
> 4. queue select vq 1
>
> From that I understood that the ordering is wrong, the driver was trying
> to read address before selecting the correct vq.
>
> At that point, I've added simple prints to the driver. Initially it
> looked as follows:
>
> 	iowrite16(index, &vp_dev->common->queue_select);
>
> 	switch (ioread64(&vp_dev->common->queue_address)) {
> 		[...]
> 	};
>
> So I added prints before the iowrite16() and after the ioread64(), and
> saw that while the driver prints were ordered, the device ones weren't:
>
> 	[    1.264052] before iowrite index=1
> 	kvmtool: net returning pfn (vq=0): 310706176
> 	kvmtool: queue selected: 1
> 	[    1.264890] after ioread index=1
>
> Suspecting that something was wrong with ordering, I've added a print
> between the iowrite and the ioread, and it finally started working well.
>
> Which leads me to the question: Are MMIO vs MMIO reads/writes not
> ordered?

mmios are strictly ordered.

Perhaps your printfs are reordered by buffering?  Are they from
different threads?  Are you using coalesced mmio (which is still
strictly ordered, if used correctly)?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Sasha Levin]

On Sun, 2011-12-11 at 11:05 +0200, Avi Kivity wrote:
> mmios are strictly ordered.
> 
> Perhaps your printfs are reordered by buffering?  Are they from
> different threads?  Are you using coalesced mmio (which is still
> strictly ordered, if used correctly)? 

I print the queue_selector and queue_address in the printfs, even if
printfs were reordered they would be printing the data right, unlike
they do now. It's the data in the printfs that matters, not their order.

Same vcpu thread with both accesses.

Not using coalesced mmio.

-- 

Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Michael S. Tsirkin]

On Sun, Dec 11, 2011 at 12:03:52PM +0200, Sasha Levin wrote:
> On Sun, 2011-12-11 at 11:05 +0200, Avi Kivity wrote:
> > mmios are strictly ordered.
> > 
> > Perhaps your printfs are reordered by buffering?  Are they from
> > different threads?  Are you using coalesced mmio (which is still
> > strictly ordered, if used correctly)? 
> 
> I print the queue_selector and queue_address in the printfs, even if
> printfs were reordered they would be printing the data right, unlike
> they do now. It's the data in the printfs that matters, not their order.
> 
> Same vcpu thread with both accesses.
> 
> Not using coalesced mmio.

Not sure why this would matter, but is the BAR a prefetcheable one?
Rusty's patch uses pci_iomap which maps a prefetcheable BAR
as cacheable.


> -- 
> 
> Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Sasha Levin]

On Sun, 2011-12-11 at 14:30 +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 11, 2011 at 12:03:52PM +0200, Sasha Levin wrote:
> > On Sun, 2011-12-11 at 11:05 +0200, Avi Kivity wrote:
> > > mmios are strictly ordered.
> > > 
> > > Perhaps your printfs are reordered by buffering?  Are they from
> > > different threads?  Are you using coalesced mmio (which is still
> > > strictly ordered, if used correctly)? 
> > 
> > I print the queue_selector and queue_address in the printfs, even if
> > printfs were reordered they would be printing the data right, unlike
> > they do now. It's the data in the printfs that matters, not their order.
> > 
> > Same vcpu thread with both accesses.
> > 
> > Not using coalesced mmio.
> 
> Not sure why this would matter, but is the BAR a prefetcheable one?
> Rusty's patch uses pci_iomap which maps a prefetcheable BAR
> as cacheable.

Wasn't defined as prefetchable, but I'm seeing same thing with or
without it.

-- 

Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Michael S. Tsirkin]

On Thu, Dec 08, 2011 at 05:37:37PM +0200, Sasha Levin wrote:
> On Thu, 2011-12-08 at 20:52 +1030, Rusty Russell wrote:
> > Here's the patch series I ended up with.  I haven't coded up the QEMU
> > side yet, so no idea if the new driver works.
> > 
> > Questions:
> > (1) Do we win from separating ISR, NOTIFY and COMMON?
> > (2) I used a "u8 bar"; should I use a bir and pack it instead?  BIR
> >     seems a little obscure (noone else in the kernel source seems to
> >     refer to it).
> 
> I started implementing it for KVM tools, when I noticed a strange thing:
> my vq creating was failing because the driver was reading a value other
> than 0 from the address field of a new vq, and failing.
> 
> I've added simple prints in the usermode code, and saw the following
> ordering:
> 
> 1. queue select vq 0
> 2. queue read address (returns 0 - new vq)
> 3. queue write address (good address of vq)
> 4. queue read address (returns !=0, fails)
> 4. queue select vq 1
> 
> >From that I understood that the ordering is wrong, the driver was trying
> to read address before selecting the correct vq.
> 
> At that point, I've added simple prints to the driver. Initially it
> looked as follows:
> 
> 	iowrite16(index, &vp_dev->common->queue_select);
> 
> 	switch (ioread64(&vp_dev->common->queue_address)) {
> 		[...]
> 	};
> 
> So I added prints before the iowrite16() and after the ioread64(), and
> saw that while the driver prints were ordered, the device ones weren't:
> 
> 	[    1.264052] before iowrite index=1
> 	kvmtool: net returning pfn (vq=0): 310706176
> 	kvmtool: queue selected: 1
> 	[    1.264890] after ioread index=1
> 
> Suspecting that something was wrong with ordering, I've added a print
> between the iowrite and the ioread, and it finally started working well.
> 
> Which leads me to the question: Are MMIO vs MMIO reads/writes not
> ordered?

First, I'd like to answer your questions from the PCI side.
Look for PCI rules in the PCI spec.
You will notices that a write is required to be able to
pass a read request. It might also pass read completion.
A read request will not pass a write request.
There's more or less no ordering between different types of transactions
(memory versus io/configuration).

That's wrt to the question you asked.

But this is not your setup: you have a single vcpu so
you will not initiate a write (select vq) until you get
a read completion.

So what you are really describing is this setup: guest reads a value,
gets the response, then writes out another one, and kvm tool reports the
write before the read.


> -- 
> 
> Sasha.

Re: [PATCH 0/11] RFC: PCI using capabilitities

[Sasha Levin]

On Sun, 2011-12-11 at 14:47 +0200, Michael S. Tsirkin wrote:
> First, I'd like to answer your questions from the PCI side.
> Look for PCI rules in the PCI spec.
> You will notices that a write is required to be able to
> pass a read request. It might also pass read completion.
> A read request will not pass a write request.
> There's more or less no ordering between different types of transactions
> (memory versus io/configuration).
> 
> That's wrt to the question you asked.
> 
> But this is not your setup: you have a single vcpu so
> you will not initiate a write (select vq) until you get
> a read completion.
> 
> So what you are really describing is this setup: guest reads a value,
> gets the response, then writes out another one, and kvm tool reports the
> write before the read. 

No, it's exactly the opposite. Guest writes a value first and then reads
one (writes queue_select and reads queue_address) and kvm tool reporting
the read before the write.

I must add here that the kvm tool doesn't do anything fancy with simple
IO/MMIO. Theres no thread games or anything similar there. The vcpu
thread is doing all the IO/MMIO work.

-- 

Sasha.