Skip to contentSkip to conversation

virtio pci: virtio 1.0 support

RFC PATCH In review Michael S. Tsirkin 2 participants 11 messages Dec 15, 2014 – Dec 19, 2014

Subsystems (8)
PCI Library code s390 SuperH +4 more subsystems
lore
Share & tools
lore

Michael S. Tsirkin

author maintainer
#0 lore raw

cover virtio pci: virtio 1.0 support

This is on top of 3.19 master + my bugfix patches, and adds virtio 1.0 support to virtio pci.
This is 3.20 material I think.

Would like to get feedback on s390 change as it's untested.

Michael S Tsirkin (2):
pci: add pci_iomap_range
s390: add pci_iomap_range

Michael S. Tsirkin (2):
virtio_pci: modern driver
virtio_pci: macros for PCI layout offsets.

Rusty Russell (1):
virtio-pci: define layout for virtio 1.0

 arch/s390/include/asm/pci_io.h     |   1 +
 drivers/virtio/virtio_pci_common.h |  29 +-
 include/asm-generic/pci_iomap.h    |  10 +
 include/uapi/linux/virtio_pci.h    |  92 +++++
 tools/virtio/linux/virtio_config.h |  45 +++
 arch/s390/pci/pci.c                |  34 +-
 drivers/virtio/virtio_pci_common.c |  13 +-
 drivers/virtio/virtio_pci_modern.c | 684 +++++++++++++++++++++++++++++++++++++
 lib/pci_iomap.c                    |  35 +-
 tools/virtio/virtio_test.c         |   1 +
 drivers/virtio/Makefile            |   2 +-
 11 files changed, 929 insertions(+), 17 deletions(-)
 create mode 100644 drivers/virtio/virtio_pci_modern.c
Signature · 1 line
MST
author maintainer
#1 lore raw

RFC PATCH 1/5 pci: add pci_iomap_range

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the BAR they need, not necessarily all of it.

Signed-off-by Michael S. Tsirkin <[email protected]>
Signed-off-by Rusty Russell <[email protected]>
---
 include/asm-generic/pci_iomap.h | 10 ++++++++++
 lib/pci_iomap.c                 | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)
include/asm-generic/pci_iomap.h +10 -0
index ce37349..7389c87 100644
@@ -15,6 +15,9 @@ struct pci_dev;
 #ifdef CONFIG_PCI
 /* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+				     unsigned long offset,
+				     unsigned long maxlen);
 /* Create a virtual mapping cookie for a port on a given PCI device.
  * Do not call this directly, it exists to make it easier for architectures
  * to override */

@@ -30,6 +33,13 @@ static inline void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned lon
 {
 	return NULL;
 }
+
+static inline void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
+					    unsigned long offset,
+					    unsigned long maxlen)
+{
+	return NULL;
+}
 #endif
 
 #endif /* __ASM_GENERIC_IO_H */
lib/pci_iomap.c +30 -5
index 0d83ea8..bcce5f1 100644
@@ -10,10 +10,11 @@
 
 #ifdef CONFIG_PCI
 /**
- * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * pci_iomap_range - create a virtual mapping cookie for a PCI BAR
  * @dev: PCI device that owns the BAR
  * @bar: BAR number
- * @maxlen: length of the memory to map
+ * @offset: map memory at the given offset in BAR
+ * @maxlen: max length of the memory to map
  *
  * Using this function you will get a __iomem address to your device BAR.
  * You can access it using ioread*() and iowrite*(). These functions hide

@@ -21,16 +22,21 @@
  * you expect from them in the correct way.
  *
  * @maxlen specifies the maximum length to map. If you want to get access to
- * the complete BAR without checking for its length first, pass %0 here.
+ * the complete BAR from offset to the end, pass %0 here.
  * */
-void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+void __iomem *pci_iomap_range(struct pci_dev *dev,
+			      int bar,
+			      unsigned long offset,
+			      unsigned long maxlen)
 {
 	resource_size_t start = pci_resource_start(dev, bar);
 	resource_size_t len = pci_resource_len(dev, bar);
 	unsigned long flags = pci_resource_flags(dev, bar);
 
-	if (!len || !start)
+	if (len <= offset || !start)
 		return NULL;
+	len -= offset;
+	start += offset;
 	if (maxlen && len > maxlen)
 		len = maxlen;
 	if (flags & IORESOURCE_IO)

@@ -43,6 +49,25 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
 	/* What? */
 	return NULL;
 }
+EXPORT_SYMBOL(pci_iomap_range);
 
+/**
+ * pci_iomap - create a virtual mapping cookie for a PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of the memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using ioread*() and iowrite*(). These functions hide
+ * the details if this is a MMIO or PIO address space and will just do what
+ * you expect from them in the correct way.
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, maxlen);
+}
 EXPORT_SYMBOL(pci_iomap);
 #endif /* CONFIG_PCI */
Signature · 1 line
MST
author maintainer
#2 lore raw

RFC PATCH 2/5 s390: add pci_iomap_range

In reply to Michael S. Tsirkin

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.
To this end, support mapping ranges within BAR on s390. Since multiple ranges can now be mapped within a BAR, we keep track of the number of mappings created, and only clear out the mapping for a BAR when this number reaches 0.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/s390/include/asm/pci_io.h |  1 +
 arch/s390/pci/pci.c            | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)
arch/s390/include/asm/pci_io.h +1 -0
index d194d54..25228b3 100644
@@ -16,6 +16,7 @@
 struct zpci_iomap_entry {
 	u32 fh;
 	u8 bar;
+	u16 count;
 };
 
 extern struct zpci_iomap_entry *zpci_iomap_start;
arch/s390/pci/pci.c +27 -7
index 2fa7b14..51cb653 100644
@@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 }
 
 /* Create a virtual mapping cookie for a PCI BAR */
-void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
+void __iomem *pci_iomap_range(struct pci_dev *pdev,
+			      int bar,
+			      unsigned long offset,
+			      unsigned long max)
 {
 	struct zpci_dev *zdev =	get_zdev(pdev);
 	u64 addr;

@@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
 
 	idx = zdev->bars[bar].map_idx;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = zdev->fh;
-	zpci_iomap_start[idx].bar = bar;
+	if (zpci_iomap_start[idx].count++) {
+		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
+		       zpci_iomap_start[idx].bar != bar);
+	} else {
+		zpci_iomap_start[idx].fh = zdev->fh;
+		zpci_iomap_start[idx].bar = bar;
+	}
+	/* Detect overrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
 	spin_unlock(&zpci_iomap_lock);
 
 	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
-	return (void __iomem *) addr;
+	return (void __iomem *) addr + offset;
 }
-EXPORT_SYMBOL_GPL(pci_iomap);
+EXPORT_SYMBOL_GPL(pci_iomap_range);
+
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, maxlen);
+}
+EXPORT_SYMBOL(pci_iomap);
 
 void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 {

@@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 
 	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >> 48;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = 0;
-	zpci_iomap_start[idx].bar = 0;
+	/* Detect underrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
+	if (!--zpci_iomap_start[idx].count) {
+		zpci_iomap_start[idx].fh = 0;
+		zpci_iomap_start[idx].bar = 0;
+	}
 	spin_unlock(&zpci_iomap_lock);
 }
 EXPORT_SYMBOL_GPL(pci_iounmap);
Signature · 1 line
MST
#3 lore raw

Re: [PATCH RFC 2/5] s390: add pci_iomap_range

Quoted 9 lines · Michael S. Tsirkin

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.
To this end, support mapping ranges within BAR on s390. Since multiple ranges can now be mapped within a BAR, we keep track of the number of mappings created, and only clear out the mapping for a BAR when this number reaches 0.

I can't say much about the users of this interface but in principle I'm OK with such a change.

Quoted 53 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/s390/include/asm/pci_io.h |  1 +
 arch/s390/pci/pci.c            | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)
arch/s390/include/asm/pci_io.h +1 -0
index d194d54..25228b3 100644
@@ -16,6 +16,7 @@
 struct zpci_iomap_entry {
 	u32 fh;
 	u8 bar;
+	u16 count;
 };
 
 extern struct zpci_iomap_entry *zpci_iomap_start;
arch/s390/pci/pci.c +13 -4
index 2fa7b14..51cb653 100644
@@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 }
 
 /* Create a virtual mapping cookie for a PCI BAR */
-void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
+void __iomem *pci_iomap_range(struct pci_dev *pdev,
+			      int bar,
+			      unsigned long offset,
+			      unsigned long max)
 {
 	struct zpci_dev *zdev =	get_zdev(pdev);
 	u64 addr;

@@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
 
 	idx = zdev->bars[bar].map_idx;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = zdev->fh;
-	zpci_iomap_start[idx].bar = bar;
+	if (zpci_iomap_start[idx].count++) {
+		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
+		       zpci_iomap_start[idx].bar != bar);
+	} else {
+		zpci_iomap_start[idx].fh = zdev->fh;
+		zpci_iomap_start[idx].bar = bar;
+	}
+	/* Detect overrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
 	spin_unlock(&zpci_iomap_lock);
 
 	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
-	return (void __iomem *) addr;

arch/s390/pci/pci.c void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max) · line 296

Quoted 10 lines
arch/s390/pci/pci.c +8 -1
+	return (void __iomem *) addr + offset;
 }
-EXPORT_SYMBOL_GPL(pci_iomap);
+EXPORT_SYMBOL_GPL(pci_iomap_range);
+
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, maxlen);
+}
+EXPORT_SYMBOL(pci_iomap);

This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay that way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like we should use that too.

Regards,
Sebastian

Quoted 22 lines
arch/s390/pci/pci.c +6 -2
 void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 {

@@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 
 	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >> 48;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = 0;
-	zpci_iomap_start[idx].bar = 0;
+	/* Detect underrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
+	if (!--zpci_iomap_start[idx].count) {
+		zpci_iomap_start[idx].fh = 0;
+		zpci_iomap_start[idx].bar = 0;
+	}
 	spin_unlock(&zpci_iomap_lock);
 }
 EXPORT_SYMBOL_GPL(pci_iounmap);
Signature · 1 line
MST
author maintainer
#4 lore raw

Re: [PATCH RFC 2/5] s390: add pci_iomap_range

Quoted 11 lines · Sebastian Ott
Quoted 9 lines · Michael S. Tsirkin

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.
To this end, support mapping ranges within BAR on s390. Since multiple ranges can now be mapped within a BAR, we keep track of the number of mappings created, and only clear out the mapping for a BAR when this number reaches 0.

Quoted 2 lines

I can't say much about the users of this interface but in principle I'm OK with such a change.

I don't have an s390 system with pci for testing - could you help me out by testing this and confirming it doesn't break things?

Quoted 68 lines
Quoted 53 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/s390/include/asm/pci_io.h |  1 +
 arch/s390/pci/pci.c            | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)
arch/s390/include/asm/pci_io.h +1 -0
index d194d54..25228b3 100644
@@ -16,6 +16,7 @@
 struct zpci_iomap_entry {
 	u32 fh;
 	u8 bar;
+	u16 count;
 };
 
 extern struct zpci_iomap_entry *zpci_iomap_start;
arch/s390/pci/pci.c +13 -4
index 2fa7b14..51cb653 100644
@@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 }
 
 /* Create a virtual mapping cookie for a PCI BAR */
-void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
+void __iomem *pci_iomap_range(struct pci_dev *pdev,
+			      int bar,
+			      unsigned long offset,
+			      unsigned long max)
 {
 	struct zpci_dev *zdev =	get_zdev(pdev);
 	u64 addr;

@@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
 
 	idx = zdev->bars[bar].map_idx;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = zdev->fh;
-	zpci_iomap_start[idx].bar = bar;
+	if (zpci_iomap_start[idx].count++) {
+		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
+		       zpci_iomap_start[idx].bar != bar);
+	} else {
+		zpci_iomap_start[idx].fh = zdev->fh;
+		zpci_iomap_start[idx].bar = bar;
+	}
+	/* Detect overrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
 	spin_unlock(&zpci_iomap_lock);
 
 	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
-	return (void __iomem *) addr;

arch/s390/pci/pci.c void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max) · line 296

Quoted 10 lines
arch/s390/pci/pci.c +8 -1
+	return (void __iomem *) addr + offset;
 }
-EXPORT_SYMBOL_GPL(pci_iomap);
+EXPORT_SYMBOL_GPL(pci_iomap_range);
+
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, maxlen);
+}
+EXPORT_SYMBOL(pci_iomap);

This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay that way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like we should use that too.

Quoted 2 lines

Regards,
Sebastian

OK, so you want two patches: one with new functionality, one switching to EXPORT_SYMBOL?

Quoted 22 lines
Quoted 22 lines
arch/s390/pci/pci.c +6 -2
 void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 {

@@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 
 	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >> 48;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = 0;
-	zpci_iomap_start[idx].bar = 0;
+	/* Detect underrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
+	if (!--zpci_iomap_start[idx].count) {
+		zpci_iomap_start[idx].fh = 0;
+		zpci_iomap_start[idx].bar = 0;
+	}
 	spin_unlock(&zpci_iomap_lock);
 }
 EXPORT_SYMBOL_GPL(pci_iounmap);
Signature · 1 line
MST
#5 lore raw

Re: [PATCH RFC 2/5] s390: add pci_iomap_range

Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Sebastian Ott
Quoted 9 lines · Michael S. Tsirkin

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.
To this end, support mapping ranges within BAR on s390. Since multiple ranges can now be mapped within a BAR, we keep track of the number of mappings created, and only clear out the mapping for a BAR when this number reaches 0.

Quoted 5 lines
Quoted 2 lines

I can't say much about the users of this interface but in principle I'm OK with such a change.

I don't have an s390 system with pci for testing - could you help me out by testing this and confirming it doesn't break things?

I did some minor tests and nothing broke...

Quoted 69 lines
Quoted 69 lines
Quoted 54 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/s390/include/asm/pci_io.h |  1 +
 arch/s390/pci/pci.c            | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)
arch/s390/include/asm/pci_io.h +1 -0
index d194d54..25228b3 100644
@@ -16,6 +16,7 @@
 struct zpci_iomap_entry {
 	u32 fh;
 	u8 bar;
+	u16 count;
 };
 
 extern struct zpci_iomap_entry *zpci_iomap_start;
arch/s390/pci/pci.c +13 -4
index 2fa7b14..51cb653 100644
@@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 }
 
 /* Create a virtual mapping cookie for a PCI BAR */
-void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
+void __iomem *pci_iomap_range(struct pci_dev *pdev,
+			      int bar,
+			      unsigned long offset,
+			      unsigned long max)
 {
 	struct zpci_dev *zdev =	get_zdev(pdev);
 	u64 addr;

@@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
 
 	idx = zdev->bars[bar].map_idx;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = zdev->fh;
-	zpci_iomap_start[idx].bar = bar;
+	if (zpci_iomap_start[idx].count++) {
+		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
+		       zpci_iomap_start[idx].bar != bar);
+	} else {
+		zpci_iomap_start[idx].fh = zdev->fh;
+		zpci_iomap_start[idx].bar = bar;
+	}
+	/* Detect overrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
 	spin_unlock(&zpci_iomap_lock);
 
 	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
-	return (void __iomem *) addr;

arch/s390/pci/pci.c void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max) · line 296

Quoted 10 lines
arch/s390/pci/pci.c +8 -1
+	return (void __iomem *) addr + offset;
 }
-EXPORT_SYMBOL_GPL(pci_iomap);
+EXPORT_SYMBOL_GPL(pci_iomap_range);
+
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, maxlen);
+}
+EXPORT_SYMBOL(pci_iomap);

This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay that way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like we should use that too.

Quoted 5 lines
Quoted 2 lines

Regards,
Sebastian

OK, so you want two patches: one with new functionality, one switching to EXPORT_SYMBOL?

Either that or at least add a note in the patch description.

Regards,
Sebastian

Quoted 26 lines
Quoted 24 lines
Quoted 24 lines
arch/s390/pci/pci.c +6 -2
 void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 {

@@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 
 	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >> 48;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = 0;
-	zpci_iomap_start[idx].bar = 0;
+	/* Detect underrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
+	if (!--zpci_iomap_start[idx].count) {
+		zpci_iomap_start[idx].fh = 0;
+		zpci_iomap_start[idx].bar = 0;
+	}
 	spin_unlock(&zpci_iomap_lock);
 }
 EXPORT_SYMBOL_GPL(pci_iounmap);
Signature · 1 line
MST
#6 lore raw

Re: [PATCH RFC 2/5] s390: add pci_iomap_range

In reply to Michael S. Tsirkin
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Sebastian Ott
Quoted 9 lines · Michael S. Tsirkin

From: Michael S Tsirkin <[email protected]>

Virtio drivers should map the part of the range they need, not necessarily all of it.
To this end, support mapping ranges within BAR on s390. Since multiple ranges can now be mapped within a BAR, we keep track of the number of mappings created, and only clear out the mapping for a BAR when this number reaches 0.

Quoted 5 lines
Quoted 2 lines

I can't say much about the users of this interface but in principle I'm OK with such a change.

I don't have an s390 system with pci for testing - could you help me out by testing this and confirming it doesn't break things?

I did some minor tests and nothing broke...

Quoted 69 lines
Quoted 69 lines
Quoted 54 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 arch/s390/include/asm/pci_io.h |  1 +
 arch/s390/pci/pci.c            | 34 +++++++++++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)
arch/s390/include/asm/pci_io.h +1 -0
index d194d54..25228b3 100644
@@ -16,6 +16,7 @@
 struct zpci_iomap_entry {
 	u32 fh;
 	u8 bar;
+	u16 count;
 };
 
 extern struct zpci_iomap_entry *zpci_iomap_start;
arch/s390/pci/pci.c +13 -4
index 2fa7b14..51cb653 100644
@@ -259,7 +259,10 @@ void __iowrite64_copy(void __iomem *to, const void *from, size_t count)
 }
 
 /* Create a virtual mapping cookie for a PCI BAR */
-void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
+void __iomem *pci_iomap_range(struct pci_dev *pdev,
+			      int bar,
+			      unsigned long offset,
+			      unsigned long max)
 {
 	struct zpci_dev *zdev =	get_zdev(pdev);
 	u64 addr;

@@ -270,14 +273,27 @@ void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max)
 
 	idx = zdev->bars[bar].map_idx;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = zdev->fh;
-	zpci_iomap_start[idx].bar = bar;
+	if (zpci_iomap_start[idx].count++) {
+		BUG_ON(zpci_iomap_start[idx].fh != zdev->fh ||
+		       zpci_iomap_start[idx].bar != bar);
+	} else {
+		zpci_iomap_start[idx].fh = zdev->fh;
+		zpci_iomap_start[idx].bar = bar;
+	}
+	/* Detect overrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
 	spin_unlock(&zpci_iomap_lock);
 
 	addr = ZPCI_IOMAP_ADDR_BASE | ((u64) idx << 48);
-	return (void __iomem *) addr;

arch/s390/pci/pci.c void __iomem *pci_iomap(struct pci_dev *pdev, int bar, unsigned long max) · line 296

Quoted 10 lines
arch/s390/pci/pci.c +8 -1
+	return (void __iomem *) addr + offset;
 }
-EXPORT_SYMBOL_GPL(pci_iomap);
+EXPORT_SYMBOL_GPL(pci_iomap_range);
+
+void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+	return pci_iomap_range(dev, bar, 0, maxlen);
+}
+EXPORT_SYMBOL(pci_iomap);

This was EXPORT_SYMBOL_GPL. I guess, for this patch, it should stay that way. ...Hm, everyone else has this stuff as EXPORT_SYMBOL looks like we should use that too.

Quoted 5 lines
Quoted 2 lines

Regards,
Sebastian

OK, so you want two patches: one with new functionality, one switching to EXPORT_SYMBOL?

Either that or at least add a note in the patch description.

Regards,
Sebastian

Quoted 26 lines
Quoted 24 lines
Quoted 24 lines
arch/s390/pci/pci.c +6 -2
 void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 {

@@ -285,8 +301,12 @@ void pci_iounmap(struct pci_dev *pdev, void __iomem *addr)
 
 	idx = (((__force u64) addr) & ~ZPCI_IOMAP_ADDR_BASE) >> 48;
 	spin_lock(&zpci_iomap_lock);
-	zpci_iomap_start[idx].fh = 0;
-	zpci_iomap_start[idx].bar = 0;
+	/* Detect underrun */
+	BUG_ON(!zpci_iomap_start[idx].count);
+	if (!--zpci_iomap_start[idx].count) {
+		zpci_iomap_start[idx].fh = 0;
+		zpci_iomap_start[idx].bar = 0;
+	}
 	spin_unlock(&zpci_iomap_lock);
 }
 EXPORT_SYMBOL_GPL(pci_iounmap);
Signature · 1 line
MST
author maintainer
#7 lore raw

RFC PATCH 3/5 virtio-pci: define layout for virtio 1.0

In reply to Michael S. Tsirkin

From: Rusty Russell <[email protected]>

Based on patches by Michael S. Tsirkin <[email protected]>, but I found it hard to follow so changed to use structures which are more self-documenting.

Signed-off-by Rusty Russell <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
 include/uapi/linux/virtio_pci.h | 62 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)
include/uapi/linux/virtio_pci.h +62 -0
index 35b552c..28c2ce0 100644
@@ -99,4 +99,66 @@
 /* Vector value used to disable MSI for queue */
 #define VIRTIO_MSI_NO_VECTOR            0xffff
 
+#ifndef VIRTIO_PCI_NO_MODERN
+
+/* IDs for different capabilities.  Must all exist. */
+
+/* Common configuration */
+#define VIRTIO_PCI_CAP_COMMON_CFG	1
+/* Notifications */
+#define VIRTIO_PCI_CAP_NOTIFY_CFG	2
+/* ISR access */
+#define VIRTIO_PCI_CAP_ISR_CFG		3
+/* Device specific confiuration */
+#define VIRTIO_PCI_CAP_DEVICE_CFG	4
+
+/* This is the PCI capability header: */
+struct virtio_pci_cap {
+	__u8 cap_vndr;		/* Generic PCI field: PCI_CAP_ID_VNDR */
+	__u8 cap_next;		/* Generic PCI field: next ptr. */
+	__u8 cap_len;		/* Generic PCI field: capability length */
+	__u8 type_and_bar;	/* Upper 3 bits: bar.
+				 * Lower 3 is VIRTIO_PCI_CAP_*_CFG. */
+	__le32 offset;		/* Offset within bar. */
+	__le32 length;		/* Length. */
+};
+
+#define VIRTIO_PCI_CAP_BAR_SHIFT	5
+#define VIRTIO_PCI_CAP_BAR_MASK		0x7
+#define VIRTIO_PCI_CAP_TYPE_SHIFT	0
+#define VIRTIO_PCI_CAP_TYPE_MASK	0x7
+
+struct virtio_pci_notify_cap {
+	struct virtio_pci_cap cap;
+	__le32 notify_off_multiplier;	/* Multiplier for queue_notify_off. */
+};
+
+/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
+struct virtio_pci_common_cfg {
+	/* About the whole device. */
+	__le32 device_feature_select;	/* read-write */
+	__le32 device_feature;		/* read-only */
+	__le32 guest_feature_select;	/* read-write */
+	__le32 guest_feature;		/* read-write */
+	__le16 msix_config;		/* read-write */
+	__le16 num_queues;		/* read-only */
+	__u8 device_status;		/* read-write */
+	__u8 config_generation;		/* read-only */
+
+	/* About a specific virtqueue. */
+	__le16 queue_select;		/* read-write */
+	__le16 queue_size;		/* read-write, power of 2. */
+	__le16 queue_msix_vector;	/* read-write */
+	__le16 queue_enable;		/* read-write */
+	__le16 queue_notify_off;	/* read-only */
+	__le32 queue_desc_lo;		/* read-write */
+	__le32 queue_desc_hi;		/* read-write */
+	__le32 queue_avail_lo;		/* read-write */
+	__le32 queue_avail_hi;		/* read-write */
+	__le32 queue_used_lo;		/* read-write */
+	__le32 queue_used_hi;		/* read-write */
+};
+
+#endif /* VIRTIO_PCI_NO_MODERN */
+
 #endif
Signature · 1 line
MST
author maintainer
#8 lore raw

RFC PATCH 4/5 virtio_pci: modern driver

In reply to Michael S. Tsirkin

It compiles! Must be perfect.

One thing *not* implemented here is separate mappings for descriptor/avail/used rings. That's nice to have, will be done later after we have core support.

Signed-off-by Michael S. Tsirkin <[email protected]>
---
 drivers/virtio/virtio_pci_common.h |  29 +-
 tools/virtio/linux/virtio_config.h |  45 +++
 drivers/virtio/virtio_pci_common.c |  13 +-
 drivers/virtio/virtio_pci_modern.c | 621 +++++++++++++++++++++++++++++++++++++
 tools/virtio/virtio_test.c         |   1 +
 drivers/virtio/Makefile            |   2 +-
 6 files changed, 706 insertions(+), 5 deletions(-)
 create mode 100644 drivers/virtio/virtio_pci_modern.c
drivers/virtio/virtio_pci_common.h +26 -3
index adddb64..1584833 100644
@@ -53,12 +53,32 @@ struct virtio_pci_device {
 	struct virtio_device vdev;
 	struct pci_dev *pci_dev;
 
+	/* In legacy mode, these two point to within ->legacy. */
+	/* Where to read and clear interrupt */
+	u8 __iomem *isr;
+
+	/* Modern only fields */
+	/* The IO mapping for the PCI config space (non-legacy mode) */
+	struct virtio_pci_common_cfg __iomem *common;
+	/* Device-specific data (non-legacy mode)  */
+	void __iomem *device;
+	/* Base of vq notifications (non-legacy mode). */
+	void __iomem *notify_base;
+
+	/* So we can sanity-check accesses. */
+	size_t notify_len;
+	size_t device_len;
+
+	/* Capability for when we need to map notifications per-vq. */
+	int notify_map_cap;
+
+	/* Multiply queue_notify_off by this value. (non-legacy mode). */
+	u32 notify_offset_multiplier;
+
+	/* Legacy only field */
 	/* the IO mapping for the PCI config space */
 	void __iomem *ioaddr;
 
-	/* the IO mapping for ISR operation */
-	void __iomem *isr;
-
 	/* a list of queues so we can dispatch IRQs */
 	spinlock_t lock;
 	struct list_head virtqueues;

@@ -131,5 +151,8 @@ void virtio_pci_release_dev(struct device *);
 int virtio_pci_legacy_probe(struct pci_dev *pci_dev,
 			    const struct pci_device_id *id);
 void virtio_pci_legacy_remove(struct pci_dev *pci_dev);
+int virtio_pci_modern_probe(struct pci_dev *pci_dev,
+			    const struct pci_device_id *id);
+void virtio_pci_modern_remove(struct pci_dev *pci_dev);
 
 #endif
tools/virtio/linux/virtio_config.h +45 -0

51 lines not on this page · raw message

drivers/virtio/virtio_pci_common.c +12 -1
index 59d3685..3821703 100644
@@ -475,12 +475,23 @@ MODULE_DEVICE_TABLE(pci, virtio_pci_id_table);
 static int virtio_pci_probe(struct pci_dev *pci_dev,
 			    const struct pci_device_id *id)
 {
+	int rc;
+
+	rc = virtio_pci_modern_probe(pci_dev, id);
+	if (rc != -ENODEV)
+		return rc;
+
 	return virtio_pci_legacy_probe(pci_dev, id);
 }
 
 static void virtio_pci_remove(struct pci_dev *pci_dev)
 {
-     virtio_pci_legacy_remove(pci_dev);
+	struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
+
+	if (vp_dev->ioaddr)
+	     virtio_pci_legacy_remove(pci_dev);
+	else
+	     virtio_pci_modern_remove(pci_dev);
 }
 
 static struct pci_driver virtio_pci_driver = {
drivers/virtio/virtio_pci_modern.c new file +621 -0

621 lines not on this page · raw message

tools/virtio/virtio_test.c +1 -0
index db3437c..67873c3 100644
@@ -11,6 +11,7 @@
 #include <sys/types.h>
 #include <fcntl.h>
 #include <stdbool.h>
+#include <linux/virtio_types.h>
 #include <linux/vhost.h>
 #include <linux/virtio.h>
 #include <linux/virtio_ring.h>
drivers/virtio/Makefile +1 -1
index bf5104b..bd230d1 100644
@@ -1,5 +1,5 @@
 obj-$(CONFIG_VIRTIO) += virtio.o virtio_ring.o
 obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
 obj-$(CONFIG_VIRTIO_PCI) += virtio_pci.o
-virtio_pci-y := virtio_pci_legacy.o virtio_pci_common.o
+virtio_pci-y := virtio_pci_modern.o virtio_pci_legacy.o virtio_pci_common.o
 obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
Signature · 1 line
MST
author maintainer
#9 lore raw

RFC PATCH 5/5 virtio_pci: macros for PCI layout offsets.

In reply to Michael S. Tsirkin

QEMU wants it, so why not? Trust, but verify.

Signed-off-by Rusty Russell <[email protected]>
---
 include/uapi/linux/virtio_pci.h    | 30 ++++++++++++++++++
 drivers/virtio/virtio_pci_modern.c | 63 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
include/uapi/linux/virtio_pci.h +30 -0
index 28c2ce0..5d546c6 100644
@@ -159,6 +159,36 @@ struct virtio_pci_common_cfg {
 	__le32 queue_used_hi;		/* read-write */
 };
 
+/* Macro versions of offsets for the Old Timers! */
+#define VIRTIO_PCI_CAP_VNDR		0
+#define VIRTIO_PCI_CAP_NEXT		1
+#define VIRTIO_PCI_CAP_LEN		2
+#define VIRTIO_PCI_CAP_TYPE_AND_BAR	3
+#define VIRTIO_PCI_CAP_OFFSET		4
+#define VIRTIO_PCI_CAP_LENGTH		8
+
+#define VIRTIO_PCI_NOTIFY_CAP_MULT	12
+
+#define VIRTIO_PCI_COMMON_DFSELECT	0
+#define VIRTIO_PCI_COMMON_DF		4
+#define VIRTIO_PCI_COMMON_GFSELECT	8
+#define VIRTIO_PCI_COMMON_GF		12
+#define VIRTIO_PCI_COMMON_MSIX		16
+#define VIRTIO_PCI_COMMON_NUMQ		18
+#define VIRTIO_PCI_COMMON_STATUS	20
+#define VIRTIO_PCI_COMMON_CFGGENERATION	21
+#define VIRTIO_PCI_COMMON_Q_SELECT	22
+#define VIRTIO_PCI_COMMON_Q_SIZE	24
+#define VIRTIO_PCI_COMMON_Q_MSIX	26
+#define VIRTIO_PCI_COMMON_Q_ENABLE	28
+#define VIRTIO_PCI_COMMON_Q_NOFF	30
+#define VIRTIO_PCI_COMMON_Q_DESCLO	32
+#define VIRTIO_PCI_COMMON_Q_DESCHI	36
+#define VIRTIO_PCI_COMMON_Q_AVAILLO	40
+#define VIRTIO_PCI_COMMON_Q_AVAILHI	44
+#define VIRTIO_PCI_COMMON_Q_USEDLO	48
+#define VIRTIO_PCI_COMMON_Q_USEDHI	52
+
 #endif /* VIRTIO_PCI_NO_MODERN */
 
 #endif
drivers/virtio/virtio_pci_modern.c +63 -0

75 lines not on this page · raw message

Signature · 1 line
MST
author maintainer
#10 lore raw

RFC PATCH 5/5 virtio_pci: macros for PCI layout offsets.

In reply to Michael S. Tsirkin

QEMU wants it, so why not? Trust, but verify.

Signed-off-by Rusty Russell <[email protected]>
---
 include/uapi/linux/virtio_pci.h    | 30 ++++++++++++++++++
 drivers/virtio/virtio_pci_modern.c | 63 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
include/uapi/linux/virtio_pci.h +30 -0
index 28c2ce0..5d546c6 100644
@@ -159,6 +159,36 @@ struct virtio_pci_common_cfg {
 	__le32 queue_used_hi;		/* read-write */
 };
 
+/* Macro versions of offsets for the Old Timers! */
+#define VIRTIO_PCI_CAP_VNDR		0
+#define VIRTIO_PCI_CAP_NEXT		1
+#define VIRTIO_PCI_CAP_LEN		2
+#define VIRTIO_PCI_CAP_TYPE_AND_BAR	3
+#define VIRTIO_PCI_CAP_OFFSET		4
+#define VIRTIO_PCI_CAP_LENGTH		8
+
+#define VIRTIO_PCI_NOTIFY_CAP_MULT	12
+
+#define VIRTIO_PCI_COMMON_DFSELECT	0
+#define VIRTIO_PCI_COMMON_DF		4
+#define VIRTIO_PCI_COMMON_GFSELECT	8
+#define VIRTIO_PCI_COMMON_GF		12
+#define VIRTIO_PCI_COMMON_MSIX		16
+#define VIRTIO_PCI_COMMON_NUMQ		18
+#define VIRTIO_PCI_COMMON_STATUS	20
+#define VIRTIO_PCI_COMMON_CFGGENERATION	21
+#define VIRTIO_PCI_COMMON_Q_SELECT	22
+#define VIRTIO_PCI_COMMON_Q_SIZE	24
+#define VIRTIO_PCI_COMMON_Q_MSIX	26
+#define VIRTIO_PCI_COMMON_Q_ENABLE	28
+#define VIRTIO_PCI_COMMON_Q_NOFF	30
+#define VIRTIO_PCI_COMMON_Q_DESCLO	32
+#define VIRTIO_PCI_COMMON_Q_DESCHI	36
+#define VIRTIO_PCI_COMMON_Q_AVAILLO	40
+#define VIRTIO_PCI_COMMON_Q_AVAILHI	44
+#define VIRTIO_PCI_COMMON_Q_USEDLO	48
+#define VIRTIO_PCI_COMMON_Q_USEDHI	52
+
 #endif /* VIRTIO_PCI_NO_MODERN */
 
 #endif
drivers/virtio/virtio_pci_modern.c +63 -0

75 lines not on this page · raw message

Signature · 1 line
MST