PCI bus support

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 38 ++++++++++++++++++++------------------
 lib/pci.h |  2 +-
 2 files changed, 21 insertions(+), 19 deletions(-)

lib/pci.c +13 -14

index 0058d70c888d..bfdaebac862e 100644
@@ -10,34 +10,36 @@
 /* Scan bus look for a specific device. Only bus 0 scanned for now. */
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 {
-    unsigned dev;
-    for (dev = 0; dev < 256; ++dev) {
-    uint32_t id = pci_config_read(dev, 0);
-    if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) {
-        return dev;
-    }
-    }
-    return PCIDEVADDR_INVALID;
+	pcidevaddr_t dev;
+
+	for (dev = 0; dev < 256; ++dev) {
+		uint32_t id = pci_config_read(dev, 0);
+
+		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+			return dev;
+	}
+
+	return PCIDEVADDR_INVALID;
 }
 
 unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-    uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
-    if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
-        return bar & PCI_BASE_ADDRESS_IO_MASK;
-    } else {
-        return bar & PCI_BASE_ADDRESS_MEM_MASK;
-    }
+	uint32_t bar = pci_config_read(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+
+	if (bar & PCI_BASE_ADDRESS_SPACE_IO)

lib/pci.c +0 -0

 }

lib/pci.h +1 -1

index 9160cfb5950d..88dc47c1f48d 100644
@@ -12,7 +12,7 @@
 
 typedef uint16_t pcidevaddr_t;
 enum {
-    PCIDEVADDR_INVALID = 0xffff,
+	PCIDEVADDR_INVALID = 0xffff,
 };
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num);

Signature · 1 line

1.8.3.1

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c         |  5 ++---
 lib/x86/asm/pci.h | 23 +++++++++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

lib/pci.c +2 -3

index 7ddaac639006..b0d89bf98067 100644
@@ -13,9 +13,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	pcidevaddr_t dev;
 
 	for (dev = 0; dev < 256; ++dev) {
-		uint32_t id = pci_config_readl(dev, 0);
-
-		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+		if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
+		    pci_config_readw(dev, PCI_DEVICE_ID) == device_id)
 			return dev;
 	}
 

lib/x86/asm/pci.h +15 -2

index d00438fe91e4..821a2c1e180a 100644
@@ -9,11 +9,30 @@
 #include "pci.h"
 #include "x86/asm/io.h"
 
+#define PCI_CONF1_ADDRESS(dev, reg)	((0x1 << 31) | (dev << 8) | reg)
+
+static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inb(0xCFC);
+}
+
+static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inw(0xCFC);
+}
+
 static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg)
 {
-    uint32_t index = reg | (dev << 8) | (0x1 << 31);
-    outl(index, 0xCF8);
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);

Quoted 55 lines · Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c         |  5 ++---
 lib/x86/asm/pci.h | 23 +++++++++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

lib/pci.c +2 -3

index 7ddaac639006..b0d89bf98067 100644
@@ -13,9 +13,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	pcidevaddr_t dev;
 
 	for (dev = 0; dev < 256; ++dev) {
-		uint32_t id = pci_config_readl(dev, 0);
-
-		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+		if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
+		    pci_config_readw(dev, PCI_DEVICE_ID) == device_id)
 			return dev;
 	}
 

lib/x86/asm/pci.h +15 -2

index d00438fe91e4..821a2c1e180a 100644
@@ -9,11 +9,30 @@
 #include "pci.h"
 #include "x86/asm/io.h"
 
+#define PCI_CONF1_ADDRESS(dev, reg)	((0x1 << 31) | (dev << 8) | reg)
+
+static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inb(0xCFC);
+}
+
+static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inw(0xCFC);
+}
+
 static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg)
 {
-    uint32_t index = reg | (dev << 8) | (0x1 << 31);
-    outl(index, 0xCF8);
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
     return inl(0xCFC);
 }
 

lib/x86/asm/pci.h line 38

Quoted 7 lines

lib/x86/asm/pci.h +6 -0

+static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    outl(val, 0xCFC);
+}
+
 #endif

Maybe add a #define for that magic value 0xCF8, now that you use it multiple times?

Anyway,

Reviewed-by Thomas Huth <[email protected]>

Quoted 56 lines · Thomas Huth

Quoted 55 lines · Alexander Gordeev

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Reviewed-by Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c         |  5 ++---
 lib/x86/asm/pci.h | 23 +++++++++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

lib/pci.c +2 -3

index 7ddaac639006..b0d89bf98067 100644
@@ -13,9 +13,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	pcidevaddr_t dev;
 
 	for (dev = 0; dev < 256; ++dev) {
-		uint32_t id = pci_config_readl(dev, 0);
-
-		if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id)
+		if (pci_config_readw(dev, PCI_VENDOR_ID) == vendor_id &&
+		    pci_config_readw(dev, PCI_DEVICE_ID) == device_id)
 			return dev;
 	}
 

lib/x86/asm/pci.h +15 -2

index d00438fe91e4..821a2c1e180a 100644
@@ -9,11 +9,30 @@
 #include "pci.h"
 #include "x86/asm/io.h"
 
+#define PCI_CONF1_ADDRESS(dev, reg)	((0x1 << 31) | (dev << 8) | reg)
+
+static inline uint8_t pci_config_readb(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inb(0xCFC);
+}
+
+static inline uint16_t pci_config_readw(pcidevaddr_t dev, uint8_t reg)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    return inw(0xCFC);
+}
+
 static inline uint32_t pci_config_readl(pcidevaddr_t dev, uint8_t reg)
 {
-    uint32_t index = reg | (dev << 8) | (0x1 << 31);
-    outl(index, 0xCF8);
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
     return inl(0xCFC);
 }
 

Quoted 10 lines

lib/x86/asm/pci.h line 38

Quoted 7 lines

lib/x86/asm/pci.h +6 -0

+static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val)
+{
+    outl(PCI_CONF1_ADDRESS(dev, reg), 0xCF8);
+    outl(val, 0xCFC);
+}
+
 #endif

Maybe add a #define for that magic value 0xCF8, now that you use it multiple times?

I used to an idea 0xCF8/0xCFC in x86 world are one of those well- known constant that people perceive better their values rather than any names :)

Andrew?

Quoted 5 lines

Anyway,

Reviewed-by Thomas Huth <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This update makes pci_bar_addr() interface 64 bit BARs aware and introduces a concept of PCI address translation.

An architecutre should implement pci_translate_addr() interface in order to provide mapping between PCI bus address and CPU physical address.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c         | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 lib/pci.h         | 16 +++++++++++++-
 lib/x86/asm/pci.h |  6 +++++
 3 files changed, 82 insertions(+), 6 deletions(-)

lib/pci.c +1 -1

index b0d89bf98067..07c911820e20 100644
@@ -21,14 +21,59 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
-unsigned long pci_bar_addr(pcidevaddr_t dev, int bar_num)
+static uint32_t pci_bar_mask(uint32_t bar)

lib/pci.c +33 -0

+
+	if (pci_bar_is64(dev, bar_num))
+		bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
+
+	return pci_translate_addr(dev, bar & mask);
+}
+
+/*
+ * To determine the amount of address space needed by a PCI device,
+ * one must save the original value of the BAR, write a value of
+ * all 1's to the register, then read it back. The amount of memory
+ * can then be then determined by masking the information bits,
+ * performing a bitwise NOT and incrementing the value by 1.
+ *
+ * The following pci_bar_size32() and pci_bar_size() functions do
+ * the described algorithm.
+ */
+static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
+{
+	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+	uint32_t bar, size;
+
+	bar = pci_config_readl(dev, off);
+	pci_config_writel(dev, off, ~0u);
+	size = pci_config_readl(dev, off);
+	pci_config_writel(dev, off, bar);
+
+	return size;
+}
+
+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 

lib/pci.c +11 -0

 }
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)

@@ -42,3 +87,14 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 	return bar;
 }
+
+bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
+{
+	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+
+	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
+		return false;
+
+	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
+		      PCI_BASE_ADDRESS_MEM_TYPE_64;
+}

lib/pci.h +0 -0

index 88dc47c1f48d..dbfe7918da37 100644
@@ -15,7 +15,21 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };

lib/pci.h +7 -0

+ *
+ * It is expected the caller is aware of the device BAR layout and never
+ * tries to address in the middle of a 64-bit register.
+ */
+phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);
 

lib/x86/asm/pci.h +0 -0

index 821a2c1e180a..813dc28b9232 100644

lib/x86/asm/pci.h +2 -0

+}
+
 #endif

Signature · 1 line

1.8.3.1

Quoted 6 lines

lib/pci.c +3 -0

+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 76

Quoted 10 lines

lib/pci.c +6 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;

All this casting of size and mask is pointless. Please rework it similar to what I did above.

Thanks,
drew

Quoted 6 lines · Andrew Jones

Quoted 6 lines

lib/pci.c +3 -0

+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

Quoted 13 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 76

Quoted 10 lines

lib/pci.c +6 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;

All this casting of size and mask is pointless. Please rework it similar to what I did above.

This is the most compact and straight variant I was able to come up with:

	uint32_t bar = pci_bar_get(dev, bar_num);
	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);

	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

	if (pci_bar_is64(dev, bar_num))
		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;

	return (~(size & mask)) + 1;

The casting is needed to avoid putting explicitly all 1s into higher bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would not bring correct results. I really struggle to make something better readable.

Quoted 21 lines · Alexander Gordeev

Quoted 19 lines · Andrew Jones

Quoted 6 lines

lib/pci.c +3 -0

+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 76

Quoted 10 lines

lib/pci.c +6 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;

All this casting of size and mask is pointless. Please rework it similar to what I did above.

Quoted 4 lines

This is the most compact and straight variant I was able to come up with:

	uint32_t bar = pci_bar_get(dev, bar_num);
	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);

But this is wrong. If your 32-bit size was 0x80000000, then you now say it's 0xffffffff80000000.

Quoted 1 line

	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

It might be OK to do that here, on a mask, but even if it is, then I don't like it, because it's too subtle (like I said for the casting in pci_bar_addr in my last reply). I don't like that it requires us to know that masking bit 31 in a 32-bit mask means we also want to mask 63..32. That should at least be in a comment somewhere.

Quoted 9 lines

	if (pci_bar_is64(dev, bar_num))
		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;

	return (~(size & mask)) + 1;

The casting is needed to avoid putting explicitly all 1s into higher bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would not bring correct results. I really struggle to make something better readable.

It's not just about readability, it's about correctness. You shouldn't use 32-bit sizes/masks with 64-bit data this way. Like I said in the last reply, you should rework it like I showed you, operate on the 32-bit data with the 32-bit mask/size, and then eventually construct 64-bit data.

Thanks,
drew

Quoted 7 lines

Quoted 1 line

	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

It might be OK to do that here, on a mask, but even if it is, then I don't like it, because it's too subtle (like I said for the casting in pci_bar_addr in my last reply). I don't like that it requires us to know that masking bit 31 in a 32-bit mask means we also want to mask 63..32. That should at least be in a comment somewhere.

Yes, but it is not an arbitrary mask, it is an alignment mask. We unconditionally want to mask 63..32, 31th and even lower.

Quoted 10 lines

Quoted 9 lines

	if (pci_bar_is64(dev, bar_num))
		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;

	return (~(size & mask)) + 1;

The casting is needed to avoid putting explicitly all 1s into higher bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would not bring correct results. I really struggle to make something better readable.

Quoted 5 lines

It's not just about readability, it's about correctness. You shouldn't use 32-bit sizes/masks with 64-bit data this way. Like I said in the last reply, you should rework it like I showed you, operate on the 32-bit data with the 32-bit mask/size, and then eventually construct 64-bit data.

It is all about those last +1 and shifting bits from lower to upper word of 64-bit value. My hope was to off-load it to the compiler :)

Quoted 2 lines

Thanks,
drew

Quoted 8 lines · Alexander Gordeev

Quoted 7 lines

Quoted 1 line

	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

It might be OK to do that here, on a mask, but even if it is, then I don't like it, because it's too subtle (like I said for the casting in pci_bar_addr in my last reply). I don't like that it requires us to know that masking bit 31 in a 32-bit mask means we also want to mask 63..32. That should at least be in a comment somewhere.

Quoted 2 lines

Yes, but it is not an arbitrary mask, it is an alignment mask. We unconditionally want to mask 63..32, 31th and even lower.

Seems like making pci_bar_mask() return phys_addr_t rather than uint32_t would make more sense.

Quoted 24 lines · Andrew Jones

Quoted 23 lines · Alexander Gordeev

Quoted 6 lines · Andrew Jones

Quoted 6 lines

lib/pci.c +3 -0

+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

Quoted 13 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 76

Quoted 10 lines

lib/pci.c +6 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;

All this casting of size and mask is pointless. Please rework it similar to what I did above.

This is the most compact and straight variant I was able to come up with:

Quoted 5 lines

Quoted 2 lines

	uint32_t bar = pci_bar_get(dev, bar_num);
	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);

But this is wrong. If your 32-bit size was 0x80000000, then you now say it's 0xffffffff80000000.

Hmm.. I am either terribly missing the point or we are on different pages.

So if pci_bar_size32() returned 0x80000000 the size sign-extension gives 0xffffffff80000000 and the mask sign extension gives (i.e. mmio) 0xfffffffffffffff0. The AND gives 0xffffffff80000000 and the NOT gives 0x7fffffff. Finally, 0x7fffffff + 1 gives 0x80000000.

If we do not sign-extend (or explicitly OR with 0xffffffff00000000) size and/or mask then the return(~(size & mask)) + 1 gives a wrong 0xffffffff80000000.

Quoted 2 lines

Quoted 1 line

	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

Quoted 5 lines

It might be OK to do that here, on a mask, but even if it is, then I don't like it, because it's too subtle (like I said for the casting in pci_bar_addr in my last reply). I don't like that it requires us to know that masking bit 31 in a 32-bit mask means we also want to mask 63..32. That should at least be in a comment somewhere.

Yes, but it is not an arbitrary mask, it is an alignment mask. We unconditionally want to mask 63..32, 31th and even lower.

Quoted 11 lines

Quoted 10 lines

	if (pci_bar_is64(dev, bar_num))
		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;

	return (~(size & mask)) + 1;

The casting is needed to avoid putting explicitly all 1s into higher bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would not bring correct results. I really struggle to make something better readable.

Quoted 5 lines

It's not just about readability, it's about correctness. You shouldn't use 32-bit sizes/masks with 64-bit data this way. Like I said in the last reply, you should rework it like I showed you, operate on the 32-bit data with the 32-bit mask/size, and then eventually construct 64-bit data.

It is all about those last +1 and shifting bits from lower to upper word of 64-bit value. My hope was to off-load it to the compiler :)

Quoted 31 lines · Alexander Gordeev

Quoted 29 lines · Andrew Jones

Quoted 21 lines · Alexander Gordeev

Quoted 6 lines · Andrew Jones

Quoted 6 lines

lib/pci.c +3 -0

+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

Quoted 13 lines

lib/pci.c pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id) · line 76

Quoted 10 lines

lib/pci.c +6 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;

All this casting of size and mask is pointless. Please rework it similar to what I did above.

Quoted 4 lines

This is the most compact and straight variant I was able to come up with:

	uint32_t bar = pci_bar_get(dev, bar_num);
	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);

But this is wrong. If your 32-bit size was 0x80000000, then you now say it's 0xffffffff80000000.

Quoted 2 lines

Hmm.. I am either terribly missing the point or we are on different pages.

Let's make sure I know what the function is supposed to do.

Based on your comment explaining how to get the PCI bar size, it sounds like we should do these steps; let's assume a device needs size=0x200000, and is 32-bit

read-write1s-dance barsz=0x00000000
readl barsz=0xffe0000c
mask barsz=0xffe00000

barsz = ~barsz + 1 barsz=0x00200000

Now let's say it's 64-bit

read-write1s-dance barsz_lo=0x00000000
readl barsz_lo=0xffe0000c
mask barsz_lo=0xffe00000
read-write1s-dance barsz_hi=0x00000000
readl barsz_hi=0xffffffff
<upper 32-bit mask is 0xffffffff, so don't bother masking at all>

barsz = ~((phys_addr_t)barsz_hi << 32 | barsz_lo) + 1 (barsz=0x200000)

So you need 5 functions

uint32_t get_mask(int bar)
{
...
}

uint32_t read_dance(int bar)
{
read-write1s-dance
return readl
}

uint32_t get_size32(int bar)
{
uint32_t size = read_dance(bar) & get_mask(bar); return ~size + 1;
}

uint64_t get_size64(int bar)
{
uint64_t size = read_dance(bar) & get_mask(bar); uint64_t size_hi = read_dance(bar + 1);

size |= size_hi << 32;
return ~size + 1;
}

uint64_t get_size(int bar)
{
return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
}

Quoted 4 lines

So if pci_bar_size32() returned 0x80000000 the size sign-extension gives 0xffffffff80000000 and the mask sign extension gives (i.e. mmio) 0xfffffffffffffff0. The AND gives 0xffffffff80000000 and the NOT gives 0x7fffffff. Finally, 0x7fffffff + 1 gives 0x80000000.

I don't see the point of using the upper 32-bits to calculate a 32-bit value from 32-bit inputs.

Quoted 6 lines

If we do not sign-extend (or explicitly OR with 0xffffffff00000000) size and/or mask then the return(~(size & mask)) + 1 gives a wrong 0xffffffff80000000.

Quoted 2 lines

Quoted 1 line

	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

Quoted 8 lines

Quoted 5 lines

It might be OK to do that here, on a mask, but even if it is, then I don't like it, because it's too subtle (like I said for the casting in pci_bar_addr in my last reply). I don't like that it requires us to know that masking bit 31 in a 32-bit mask means we also want to mask 63..32. That should at least be in a comment somewhere.

Yes, but it is not an arbitrary mask, it is an alignment mask. We unconditionally want to mask 63..32, 31th and even lower.

If you use sign-extension to generate the alignment mask, then it needs a comment. But if we can avoid the headache of 64-bit data computing 32-bit results then let's avoid it. (Even though you described to me how it's supposed to work, I'm still not sure it's safe for all cases :-)

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

	if (pci_bar_is64(dev, bar_num))
		size |= (phys_addr_t)pci_bar_size32(dev, bar_num + 1) << 32;

	return (~(size & mask)) + 1;

The casting is needed to avoid putting explicitly all 1s into higher bits of size and/or mask. Otherwise (~(size & mask)) + 1 expression would not bring correct results. I really struggle to make something better readable.

Quoted 8 lines

Quoted 5 lines

It's not just about readability, it's about correctness. You shouldn't use 32-bit sizes/masks with 64-bit data this way. Like I said in the last reply, you should rework it like I showed you, operate on the 32-bit data with the 32-bit mask/size, and then eventually construct 64-bit data.

It is all about those last +1 and shifting bits from lower to upper word of 64-bit value. My hope was to off-load it to the compiler :)

I think the problem comes from trying to apply a mask to the upper bits, even though it doesn't need one. Off-loading to the compiler is a good idea, but we can avoid being compilers ourselves by writing short, simple to understand functions, and then letting the compiler inline and optimize them for us :-)

Thanks,
drew

Quoted 35 lines · Andrew Jones

Quoted 33 lines · Alexander Gordeev

Quoted 22 lines · Andrew Jones

Quoted 21 lines · Alexander Gordeev

Quoted 19 lines · Andrew Jones

Quoted 6 lines

(quoted diff) +3 -0

+phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
 	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
+	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 

Review

Quoted 10 lines

(quoted diff) +6 -4

-	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
-		return bar & PCI_BASE_ADDRESS_IO_MASK;
-	else
-		return bar & PCI_BASE_ADDRESS_MEM_MASK;
+	if (pci_bar_is64(dev, bar_num)) {
+		uint32_t size_high = pci_bar_size32(dev, bar_num + 1);
+		size = ((phys_addr_t)size_high << 32) | (uint32_t)size;
+	}
+
+	return (~(size & mask)) + 1;

All this casting of size and mask is pointless. Please rework it similar to what I did above.

Quoted 7 lines

Quoted 4 lines

This is the most compact and straight variant I was able to come up with:

	uint32_t bar = pci_bar_get(dev, bar_num);
	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);

But this is wrong. If your 32-bit size was 0x80000000, then you now say it's 0xffffffff80000000.

Hmm.. I am either terribly missing the point or we are on different pages.

Quoted 5 lines

Let's make sure I know what the function is supposed to do.

Based on your comment explaining how to get the PCI bar size, it sounds like we should do these steps; let's assume a device needs size=0x200000, and is 32-bit

I am not quite sure what barsz=0x00000000 at read-write1s-dance

phase is, but otherwise it seems correct to me (well may be a nit - the 32-bit variant denoted 64-bit mmio).

Quoted 46 lines

read-write1s-dance barsz=0x00000000
readl barsz=0xffe0000c
mask barsz=0xffe00000

barsz = ~barsz + 1 barsz=0x00200000

Now let's say it's 64-bit

read-write1s-dance barsz_lo=0x00000000
readl barsz_lo=0xffe0000c
mask barsz_lo=0xffe00000
read-write1s-dance barsz_hi=0x00000000
readl barsz_hi=0xffffffff
<upper 32-bit mask is 0xffffffff, so don't bother masking at all>

barsz = ~((phys_addr_t)barsz_hi << 32 | barsz_lo) + 1 (barsz=0x200000)

So you need 5 functions

uint32_t get_mask(int bar)
{
...
}

uint32_t read_dance(int bar)
{
read-write1s-dance
return readl
}

uint32_t get_size32(int bar)
{
uint32_t size = read_dance(bar) & get_mask(bar); return ~size + 1;
}

uint64_t get_size64(int bar)
{
uint64_t size = read_dance(bar) & get_mask(bar); uint64_t size_hi = read_dance(bar + 1);

size |= size_hi << 32;
return ~size + 1;
}

Quoted 4 lines

uint64_t get_size(int bar)
{
return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
}

If the above is a pseudo code or you want me to rework using these functions?

If not, below is how it could look like.

Also, looking at your get_size() I noticed a bug in my version :-) when read_dance() is followed by readl() returns zero - that means no (more) BAR(s).

phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)

{
	uint32_t size;
	uint32_t bar;

	size = pci_bar_size32(dev, bar_num);
	if (!size)
		return 0;

	bar = pci_bar_get(dev, bar_num);
	size &= pci_bar_mask(bar);

	if (pci_bar_is64(dev, bar_num)) {
		phys_addr_t size64 = pci_bar_size32(dev, bar_num + 1);
		size64 = (size64 << 32) | size;

		return ~size64 + 1;
	} else {
		return ~size + 1;
	}
}

Quoted 33 lines

Quoted 32 lines

So you need 5 functions

uint32_t get_mask(int bar)
{
...
}

uint32_t read_dance(int bar)
{
read-write1s-dance
return readl
}

uint32_t get_size32(int bar)
{
uint32_t size = read_dance(bar) & get_mask(bar); return ~size + 1;
}

uint64_t get_size64(int bar)
{
uint64_t size = read_dance(bar) & get_mask(bar); uint64_t size_hi = read_dance(bar + 1);

size |= size_hi << 32;
return ~size + 1;
}

uint64_t get_size(int bar)
{
return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
}

Quoted 2 lines

If the above is a pseudo code or you want me to rework using these functions?

Yes and no. You can do what you like, but pci_bar_size32 should return a size (like my get_size32), not a 2's complement of size, otherwise the name is wrong.

Quoted 11 lines

If not, below is how it could look like.

Also, looking at your get_size() I noticed a bug in my version :-) when read_dance() is followed by readl() returns zero - that means no (more) BAR(s).

phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)

{
	uint32_t size;
	uint32_t bar;

Quoted 3 lines

	size = pci_bar_size32(dev, bar_num);
	if (!size)
		return 0;

Should callers ever call pci_bar_size on a bar_num that will result in zero, "no more BARs"? If not, then you can assert here. Otherwise will all callers know what zero means, and check for it?

Thanks,
drew

Quoted 36 lines · Andrew Jones

Quoted 35 lines

Quoted 24 lines

So you need 5 functions

uint32_t get_mask(int bar)
{
...
}

uint32_t read_dance(int bar)
{
read-write1s-dance
return readl
}

uint32_t get_size32(int bar)
{
uint32_t size = read_dance(bar) & get_mask(bar); return ~size + 1;
}

uint64_t get_size64(int bar)
{
uint64_t size = read_dance(bar) & get_mask(bar); uint64_t size_hi = read_dance(bar + 1);

Quoted 8 lines

size |= size_hi << 32;
return ~size + 1;
}

uint64_t get_size(int bar)
{
return bar_size(bar) == BAR64 ? get_size64(bar) : get_size32(bar);
}

If the above is a pseudo code or you want me to rework using these functions?

Quoted 3 lines

Yes and no. You can do what you like, but pci_bar_size32 should return a size (like my get_size32), not a 2's complement of size, otherwise the name is wrong.

Right. So my pci_bar_size32 is your read_dance. Want to use that name or i.e. pci_bar_size_complement?

Quoted 12 lines

Quoted 12 lines

If not, below is how it could look like.

Also, looking at your get_size() I noticed a bug in my version :-) when read_dance() is followed by readl() returns zero - that means no (more) BAR(s).

phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)

{
	uint32_t size;
	uint32_t bar;

Quoted 7 lines

Quoted 3 lines

	size = pci_bar_size32(dev, bar_num);
	if (!size)
		return 0;

Should callers ever call pci_bar_size on a bar_num that will result in zero, "no more BARs"? If not, then you can assert here. Otherwise will all callers know what zero means, and check for it?

Yes, it is an indication a BAR is implemented in HW. But I misled you :/ I remember some code stopped on a zero-sized BAR, but I do not recall what code it was. There is nothing in specs about stopping and both Seabios and Linux just continue probing. So I will update the breaks to continues.

Quoted 2 lines

Thanks,
drew

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines

+/*
+ * BAR number in all BAR access functions below is a number of 32-bit
+ * register starting from PCI_BASE_ADDRESS_0 offset.
+ *
+ * In cases BAR size is 64-bit a caller should still provide BAR number
+ * in terms of 32-bit words. For example, if a device has 64-bit BAR#0
+ * and 32-bit BAR#1 the caller should provide 2 to address BAR#1, not 1.

Is this how people label bars? I.e. they call a bar following a 64-bit bar BAR#N+1? Or do they skip numbers with the labels to match the offsets? I.e. in this example you'd say BAR#0 (64-bit), there is no BAR#1, and then BAR#2 (32-bit) when labeling?

Thanks,
drew

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 39

Quoted 10 lines

lib/x86/asm/pci.h +2 -0

index 821a2c1e180a..813dc28b9232 100644
@@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
     outl(val, 0xCFC);
 }
 
+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)

I put the __unused after the argument name.

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 41

Quoted 2 lines

lib/x86/asm/pci.h +2 -0

+{
+    return addr;

Need tab here.

Quoted 6 lines

lib/x86/asm/pci.h +2 -0

+}
+
 #endif

Signature · 1 line

1.8.3.1

Thanks,
drew

Quoted 13 lines · Andrew Jones

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 39

Quoted 10 lines

lib/x86/asm/pci.h +2 -0

index 821a2c1e180a..813dc28b9232 100644
@@ -35,4 +35,10 @@ static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val
     outl(val, 0xCFC);
 }
 
+static inline
+phys_addr_t pci_translate_addr(pcidevaddr_t __unused dev, uint64_t addr)

I put the __unused after the argument name.

Quoted 4 lines

lib/x86/asm/pci.h static inline void pci_config_writel(pcidevaddr_t dev, uint8_t reg, uint32_t val · line 41

Quoted 2 lines

lib/x86/asm/pci.h +2 -0

+{
+    return addr;

Need tab here.

It will differ then from the coding style of the other functions in this header (and elsewhere in x86).

Quoted 10 lines

Quoted 7 lines

lib/x86/asm/pci.h +2 -0

+}
+
 #endif

Signature · 1 line

1.8.3.1

Thanks,
drew

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

lib/pci.c +9 -5

index 07c911820e20..d092e22b8804 100644
@@ -27,14 +27,18 @@ static uint32_t pci_bar_mask(uint32_t bar)
 		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
 }
 
+static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
+{
+	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+}
+
 phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 {
-	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
-	phys_addr_t bar = pci_config_readl(dev, off);
+	phys_addr_t bar = pci_bar_get(dev, bar_num);
 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 
 	if (pci_bar_is64(dev, bar_num))
-		bar |= (phys_addr_t)pci_config_readl(dev, off + 4) << 32;
+		bar |= (phys_addr_t)pci_bar_get(dev, bar_num + 1) << 32;
 
 	return pci_translate_addr(dev, bar & mask);
 }

@@ -64,7 +68,7 @@ static uint32_t pci_bar_size32(pcidevaddr_t dev, int bar_num)
 
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_bar_get(dev, bar_num);
 	phys_addr_t size = (int32_t)pci_bar_size32(dev, bar_num);
 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);
 

@@ -78,19 +82,19 @@ phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num)
 
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num)
 {
-	uint32_t bar = pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
+	uint32_t bar = pci_bar_get(dev, bar_num);
 	return !(bar & PCI_BASE_ADDRESS_SPACE_IO);
 }
 
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num)

lib/pci.c +0 -0

 	if (bar & PCI_BASE_ADDRESS_SPACE_IO)
 		return false;

Signature · 1 line

1.8.3.1

Because the counterpart to pci_bar_set_addr() setter is pci_bar_addr() getter, these names become inconsistent. Rename pci_bar_addr() to pci_bar_get_addr() also to make the resulting names conform to each other.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c    | 12 +++++++++++-
 lib/pci.h    |  3 ++-
 x86/vmexit.c |  4 ++--
 3 files changed, 15 insertions(+), 4 deletions(-)

lib/pci.c +11 -1

index d092e22b8804..c9b2daa6a4ce 100644
@@ -32,7 +32,7 @@ static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }
 
-phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
+phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num)
 {
 	phys_addr_t bar = pci_bar_get(dev, bar_num);
 	phys_addr_t mask = (int32_t)pci_bar_mask(bar);

@@ -43,6 +43,16 @@ phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num)
 	return pci_translate_addr(dev, bar & mask);
 }
 
+void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr)
+{
+	int off = PCI_BASE_ADDRESS_0 + bar_num * 4;
+
+	pci_config_writel(dev, off, (uint32_t)addr);
+
+	if (pci_bar_is64(dev, bar_num))
+		pci_config_writel(dev, off + 4, (uint32_t)(addr >> 32));
+}
+
 /*
  * To determine the amount of address space needed by a PCI device,
  * one must save the original value of the BAR, write a value of

lib/pci.h +2 -1

index dbfe7918da37..a51d2ff52cec 100644
@@ -27,7 +27,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
  * It is expected the caller is aware of the device BAR layout and never
  * tries to address in the middle of a 64-bit register.
  */
-phys_addr_t pci_bar_addr(pcidevaddr_t dev, int bar_num);
+phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
+void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);

x86/vmexit.c +2 -2

index c2e1e496918d..2d99d5fdf1c2 100644
@@ -392,10 +392,10 @@ int main(int ac, char **av)
 				continue;
 			}
 			if (pci_bar_is_memory(pcidev, i)) {
-				membar = pci_bar_addr(pcidev, i);
+				membar = pci_bar_get_addr(pcidev, i);
 				pci_test.memaddr = ioremap(membar, PAGE_SIZE);
 			} else {
-				pci_test.iobar = pci_bar_addr(pcidev, i);
+				pci_test.iobar = pci_bar_get_addr(pcidev, i);
 			}
 		}
 		printf("pci-testdev at 0x%x membar %lx iobar %x\n",

Signature · 1 line

1.8.3.1

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 6 ++++++
 lib/pci.h | 1 +
 2 files changed, 7 insertions(+)

lib/pci.c +0 -0

index c9b2daa6a4ce..5c107e7b4f4e 100644

lib/pci.h +1 -0

+bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 
 /*

Signature · 1 line

1.8.3.1

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci.h |  3 +++
 2 files changed, 82 insertions(+)

lib/pci.c +65 -0

index 5c107e7b4f4e..e715a3e51cc4 100644
@@ -118,3 +118,82 @@ bool pci_bar_is64(pcidevaddr_t dev, int bar_num)
 	return (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
 		      PCI_BASE_ADDRESS_MEM_TYPE_64;
 }
+
+static void pci_dev_print(pcidevaddr_t dev)
+{
+	uint16_t vendor_id = pci_config_readw(dev, PCI_VENDOR_ID);
+	uint16_t device_id = pci_config_readw(dev, PCI_DEVICE_ID);
+	uint8_t header = pci_config_readb(dev, PCI_HEADER_TYPE);
+	uint8_t progif = pci_config_readb(dev, PCI_CLASS_PROG);
+	uint8_t subclass = pci_config_readb(dev, PCI_CLASS_DEVICE);
+	uint8_t class = pci_config_readb(dev, PCI_CLASS_DEVICE + 1);
+	int i;
+
+	printf("dev %2d fn %d vendor_id %04x device_id %04x type %02x "
+	       "progif %02x class %02x subclass %02x\n",
+	       dev / 8, dev % 8, vendor_id, device_id, header,
+	       progif, class, subclass);
+
+	if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
+		return;
+
+	for (i = 0; i < 6; i++) {
+		phys_addr_t start, end;
+		uint32_t bar;
+
+		if (!pci_bar_is_valid(dev, i))
+			break;
+
+		start = pci_bar_get_addr(dev, i);
+		end = start + pci_bar_size(dev, i) - 1;
+
+		if (pci_bar_is64(dev, i)) {
+			printf("\tBAR#%d,%d [%" PRIx64 "-%" PRIx64 " ",
+			       i, i + 1, start, end);
+			i++;
+		} else {
+			printf("\tBAR#%d    [%02x-%02x ",
+			       i, (uint32_t)start, (uint32_t)end);
+		}
+
+		bar = pci_bar_get(dev, i);
+
+		if (bar & PCI_BASE_ADDRESS_SPACE_IO) {
+			printf("PIO]\n");
+			continue;
+		}
+
+		printf("MEM");
+
+		switch (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) {
+		case PCI_BASE_ADDRESS_MEM_TYPE_32:
+			printf("32");
+			break;
+		case PCI_BASE_ADDRESS_MEM_TYPE_1M:
+			printf("1M");
+			break;
+		case PCI_BASE_ADDRESS_MEM_TYPE_64:
+			printf("64");
+			break;
+		default:
+			assert(0);
+		}
+
+		if (bar & PCI_BASE_ADDRESS_MEM_PREFETCH)
+			printf("/p");
+
+		printf("]\n");

lib/pci.c +4 -0

+			pci_dev_print(dev);
+		}
+	}
+}

lib/pci.h +3 -0

index db8296b2dfa7..e9911db92320 100644
@@ -14,6 +14,7 @@ typedef uint16_t pcidevaddr_t;
 enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
+void pci_print(void);
 bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 

@@ -55,4 +56,6 @@ struct pci_test_dev_hdr {
 	uint8_t  name[];
 };
 
+#define  PCI_HEADER_TYPE_MASK		0x7f
+
 #endif /* PCI_H */

Signature · 1 line

1.8.3.1

Unlike x86, other architectures using generic ECAM PCI host do not have a luxury of PCI bus initialized by a BIOS and ready to use at start. Thus, we need allocate and assign resources to all devices, much like an architecture's firmware would do.

There is no any sort of resource management for memory and io spaces, since only ones-per-BAR allocations are expected and no deallocations at all.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h |  46 ++++++++
 lib/pci.c              |   4 +-
 lib/pci.h              |   3 +
 4 files changed, 345 insertions(+), 2 deletions(-)
 create mode 100644 lib/pci-host-generic.c
 create mode 100644 lib/pci-host-generic.h

lib/pci-host-generic.c new file +98 -0

new file mode 100644
index 000000000000..f01913fca053
@@ -0,0 +1,294 @@
+/*
+ * Generic PCI host controller as described in PCI Bus Binding to Open Firmware
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+#include "devicetree.h"
+#include "alloc.h"
+#include "pci.h"
+#include "asm/pci.h"
+#include "asm/io.h"
+#include "pci-host-generic.h"
+#include <linux/pci_regs.h>
+
+static struct pci_host_bridge *pci_host_bridge;
+
+static int of_flags_to_pci_type(u32 of_flags)
+{
+	static int type_map[] = {
+		[1] = PCI_BASE_ADDRESS_SPACE_IO,
+		[2] = PCI_BASE_ADDRESS_MEM_TYPE_32,
+		[3] = PCI_BASE_ADDRESS_MEM_TYPE_64
+	};
+	int idx = (of_flags >> 24) & 0x03;
+	int res;
+
+	assert(idx > 0);
+	res = type_map[idx];
+
+	if (of_flags & 0x40000000)
+		res |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+
+	return res;
+}
+
+static int pci_bar_type(u32 bar)
+{
+	if (bar & PCI_BASE_ADDRESS_SPACE)
+		return PCI_BASE_ADDRESS_SPACE_IO;
+	else
+		return bar & (PCI_BASE_ADDRESS_MEM_TYPE_MASK |
+			      PCI_BASE_ADDRESS_MEM_PREFETCH);
+}
+
+/*
+ * Probe DT for a generic PCI host controller
+ * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
+ * and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c
+ */
+static struct pci_host_bridge *pci_dt_probe(void)
+{
+	struct pci_host_bridge *host;
+	const void *fdt = dt_fdt();
+	const struct fdt_property *prop;
+	struct dt_pbus_reg base;
+	struct dt_device dt_dev;
+	struct dt_bus dt_bus;
+	struct pci_addr_space *as;
+	fdt32_t *data;
+	u32 bus, bus_max;
+	u32 nac, nsc, nac_root, nsc_root;
+	int nr_range_cells, nr_addr_spaces;
+	int ret, node, len, i;
+
+	if (!dt_available()) {
+		printf("No device tree found\n");
+		return NULL;
+	}
+
+	dt_bus_init_defaults(&dt_bus);
+	dt_device_init(&dt_dev, &dt_bus, NULL);
+
+	node = fdt_path_offset(fdt, "/");
+	assert(node >= 0);
+
+	ret = dt_get_nr_cells(node, &nac_root, &nsc_root);
+	assert(ret == 0);
+	assert(nac_root == 1 || nac_root == 2);
+
+	node = fdt_node_offset_by_compatible(fdt, node,
+					     "pci-host-ecam-generic");
+	if (node == -FDT_ERR_NOTFOUND) {
+		printf("No PCIe ECAM compatible controller found\n");
+		return NULL;
+	}
+	assert(node >= 0);
+
+	prop = fdt_get_property(fdt, node, "device_type", &len);
+	assert(prop && len == 4 && !strcmp((char *)prop->data, "pci"));
+
+	dt_device_bind_node(&dt_dev, node);
+	ret = dt_pbus_get_base(&dt_dev, &base);
+	assert(ret == 0);
+
+	prop = fdt_get_property(fdt, node, "bus-range", &len);
+	if (prop == NULL) {

lib/pci-host-generic.c new file +19 -0

+
+	ret = dt_get_nr_cells(node, &nac, &nsc);
+	assert(ret == 0);
+	assert(nac == 3 && nsc == 2);
+
+	prop = fdt_get_property(fdt, node, "ranges", &len);
+	assert(prop != NULL);
+
+	nr_range_cells = nac + nsc + nac_root;
+	nr_addr_spaces = (len / 4) / nr_range_cells;
+	assert(nr_addr_spaces);
+
+	host = malloc(sizeof(*host) +
+		      sizeof(host->addr_space[0]) * nr_addr_spaces);
+	assert(host != NULL);
+
+	host->start		= base.addr;
+	host->size		= base.size;
+	host->bus		= bus;

lib/pci-host-generic.c new file +32 -0

+		 *
+		 * phys.hi  cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
+		 * phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
+		 * phys.lo  cell: llllllll llllllll llllllll llllllll
+		 *
+		 * PCI device bus address and flags are encoded into phys.high
+		 * PCI 64 bit address is encoded into phys.mid and phys.low
+		 */
+		as->type = of_flags_to_pci_type(fdt32_to_cpu(data[0]));
+		as->pci_start = ((u64)fdt32_to_cpu(data[1]) << 32) |
+				fdt32_to_cpu(data[2]);
+
+		if (nr_range_cells == 6) {
+			as->start = fdt32_to_cpu(data[3]);
+			as->size  = ((u64)fdt32_to_cpu(data[4]) << 32) |
+				    fdt32_to_cpu(data[5]);
+		} else {
+			as->start = ((u64)fdt32_to_cpu(data[3]) << 32) |
+				    fdt32_to_cpu(data[4]);
+			as->size  = ((u64)fdt32_to_cpu(data[5]) << 32) |
+				    fdt32_to_cpu(data[6]);
+		}
+
+		data += nr_range_cells;
+		as++;
+	}
+
+	return host;
+}
+
+static u64 pci_alloc_resource(u32 bar, u64 size)
+{

lib/pci-host-generic.c new file +115 -0

+	}
+	if (i >= host->nr_addr_spaces) {
+		printf("No PCI resource found for a device\n");
+		assert(0);
+	}
+
+	mask = pci_bar_mask(bar);
+	size = ALIGN(size, ~mask + 1);
+	assert(as->allocated + size <= as->size);
+
+	addr = as->pci_start + as->allocated;
+	as->allocated += size;
+
+	return addr;
+}
+
+bool pci_probe(void)
+{
+	pcidevaddr_t dev;
+	u8 header;
+	u32 cmd;
+	int i;
+
+	assert(!pci_host_bridge);
+	pci_host_bridge = pci_dt_probe();
+	if (!pci_host_bridge)
+		return false;
+
+	for (dev = 0; dev < 256; dev++) {
+		if (!pci_dev_exists(dev))
+			continue;
+
+		/* We are only interested in normal PCI devices */
+		header = pci_config_readb(dev, PCI_HEADER_TYPE);
+		if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
+			continue;
+
+		cmd = PCI_COMMAND_SERR;
+
+		for (i = 0; i < 6; i++) {
+			u64 addr, size;
+			u32 bar;
+
+			size = pci_bar_size(dev, i);
+			if (!size)
+				break;
+
+			bar = pci_bar_get(dev, i);
+			addr = pci_alloc_resource(bar, size);
+			pci_bar_set_addr(dev, i, addr);
+
+			if (pci_bar_is_memory(dev, i))
+				cmd |= PCI_COMMAND_MEMORY;
+			else
+				cmd |= PCI_COMMAND_IO;
+
+			if (pci_bar_is64(dev, i))
+				i++;
+		}
+
+		pci_config_writel(dev, PCI_COMMAND, cmd);
+	}
+
+	return true;
+}
+
+/*
+ * This function is to be called from pci_translate_addr() to provide
+ * mapping between this host bridge's PCI busses address and CPU physical
+ * address.
+ */
+phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr)
+{
+	struct pci_host_bridge *host = pci_host_bridge;
+	struct pci_addr_space *as = &host->addr_space[0];
+	int i;
+
+	for (i = 0; i < host->nr_addr_spaces; i++, as++) {
+		if (pci_addr >= as->pci_start &&
+		    pci_addr < as->pci_start + as->size)
+			return as->start + (pci_addr - as->pci_start);
+	}
+
+	return 0;
+}
+
+static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn)
+{
+	return (void __iomem *)(unsigned long)
+		host->start + (devfn << PCI_ECAM_DEVFN_SHIFT);
+}
+
+u8 pci_config_readb(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readb(conf + off);
+}
+
+u16 pci_config_readw(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readw(conf + off);
+}
+
+u32 pci_config_readl(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readl(conf + off);
+}
+
+void pci_config_writel(pcidevaddr_t dev, u8 off, u32 val)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	writel(val, conf + off);
+}

lib/pci-host-generic.h new file +46 -0

new file mode 100644
index 000000000000..fd30e7c74ed8
@@ -0,0 +1,46 @@
+#ifndef PCI_HOST_GENERIC_H
+#define PCI_HOST_GENERIC_H
+/*
+ * PCI host bridge supporting structures and constants
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+
+struct pci_addr_space {
+	phys_addr_t		pci_start;
+	phys_addr_t		start;
+	phys_addr_t		size;
+	phys_addr_t		allocated;
+	int			type;
+};
+
+struct pci_host_bridge {
+	phys_addr_t		start;
+	phys_addr_t		size;
+	int			bus;
+	int			bus_max;
+	int			nr_addr_spaces;
+	struct pci_addr_space	addr_space[];
+};
+
+/*
+ * The following constants are derived from Linux, see this source:
+ *
+ *         drivers/pci/host/pci-host-generic.c
+ *                 struct gen_pci_cfg_bus_ops::bus_shift
+ *                 int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
+ *
+ * Documentation/devicetree/bindings/pci/host-generic-pci.txt describes
+ * ECAM Configuration Space is be memory-mapped by concatenating the various
+ * components to form an offset:
+ *
+ *	cfg_offset(bus, device, function, register) =
+ *		   bus << 20 | device << 15 | function << 12 | register
+ */
+#define PCI_ECAM_BUS_SHIFT	20
+#define PCI_ECAM_DEVFN_SHIFT	12
+
+#endif

lib/pci.c +2 -2

index e715a3e51cc4..41a44d04d475 100644
@@ -27,13 +27,13 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
-static uint32_t pci_bar_mask(uint32_t bar)
+uint32_t pci_bar_mask(uint32_t bar)
 {
 	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
 		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
 }
 
-static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
+uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 {
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }

lib/pci.h +3 -0

index e9911db92320..37b282169a4e 100644
@@ -15,6 +15,7 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 void pci_print(void);
+bool pci_probe(void);
 bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 

@@ -32,6 +33,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
+uint32_t pci_bar_mask(uint32_t bar);
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);

Signature · 1 line

1.8.3.1

Quoted 131 lines · Alexander Gordeev

Unlike x86, other architectures using generic ECAM PCI host do not have a luxury of PCI bus initialized by a BIOS and ready to use at start. Thus, we need allocate and assign resources to all devices, much like an architecture's firmware would do.

There is no any sort of resource management for memory and io spaces, since only ones-per-BAR allocations are expected and no deallocations at all.

Cc Thomas Huth <[email protected]>

Cc Andrew Jones <[email protected]>

Signed-off-by Alexander Gordeev <[email protected]>

---
 lib/pci-host-generic.c | 294 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/pci-host-generic.h |  46 ++++++++
 lib/pci.c              |   4 +-
 lib/pci.h              |   3 +
 4 files changed, 345 insertions(+), 2 deletions(-)
 create mode 100644 lib/pci-host-generic.c
 create mode 100644 lib/pci-host-generic.h

lib/pci-host-generic.c new file +103 -0

new file mode 100644
index 000000000000..f01913fca053
@@ -0,0 +1,294 @@
+/*
+ * Generic PCI host controller as described in PCI Bus Binding to Open Firmware
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+#include "devicetree.h"
+#include "alloc.h"
+#include "pci.h"
+#include "asm/pci.h"
+#include "asm/io.h"
+#include "pci-host-generic.h"
+#include <linux/pci_regs.h>
+
+static struct pci_host_bridge *pci_host_bridge;
+
+static int of_flags_to_pci_type(u32 of_flags)
+{
+	static int type_map[] = {
+		[1] = PCI_BASE_ADDRESS_SPACE_IO,
+		[2] = PCI_BASE_ADDRESS_MEM_TYPE_32,
+		[3] = PCI_BASE_ADDRESS_MEM_TYPE_64
+	};
+	int idx = (of_flags >> 24) & 0x03;
+	int res;
+
+	assert(idx > 0);
+	res = type_map[idx];
+
+	if (of_flags & 0x40000000)
+		res |= PCI_BASE_ADDRESS_MEM_PREFETCH;
+
+	return res;
+}
+
+static int pci_bar_type(u32 bar)
+{
+	if (bar & PCI_BASE_ADDRESS_SPACE)
+		return PCI_BASE_ADDRESS_SPACE_IO;
+	else
+		return bar & (PCI_BASE_ADDRESS_MEM_TYPE_MASK |
+			      PCI_BASE_ADDRESS_MEM_PREFETCH);
+}
+
+/*
+ * Probe DT for a generic PCI host controller
+ * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
+ * and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c
+ */
+static struct pci_host_bridge *pci_dt_probe(void)
+{
+	struct pci_host_bridge *host;
+	const void *fdt = dt_fdt();
+	const struct fdt_property *prop;
+	struct dt_pbus_reg base;
+	struct dt_device dt_dev;
+	struct dt_bus dt_bus;
+	struct pci_addr_space *as;
+	fdt32_t *data;
+	u32 bus, bus_max;
+	u32 nac, nsc, nac_root, nsc_root;
+	int nr_range_cells, nr_addr_spaces;
+	int ret, node, len, i;
+
+	if (!dt_available()) {
+		printf("No device tree found\n");
+		return NULL;
+	}
+
+	dt_bus_init_defaults(&dt_bus);
+	dt_device_init(&dt_dev, &dt_bus, NULL);
+
+	node = fdt_path_offset(fdt, "/");
+	assert(node >= 0);
+
+	ret = dt_get_nr_cells(node, &nac_root, &nsc_root);
+	assert(ret == 0);
+	assert(nac_root == 1 || nac_root == 2);
+
+	node = fdt_node_offset_by_compatible(fdt, node,
+					     "pci-host-ecam-generic");
+	if (node == -FDT_ERR_NOTFOUND) {
+		printf("No PCIe ECAM compatible controller found\n");
+		return NULL;
+	}
+	assert(node >= 0);
+
+	prop = fdt_get_property(fdt, node, "device_type", &len);
+	assert(prop && len == 4 && !strcmp((char *)prop->data, "pci"));
+
+	dt_device_bind_node(&dt_dev, node);
+	ret = dt_pbus_get_base(&dt_dev, &base);
+	assert(ret == 0);
+
+	prop = fdt_get_property(fdt, node, "bus-range", &len);
+	if (prop == NULL) {
+		assert(len == -FDT_ERR_NOTFOUND);
+		bus		= 0x00;
+		bus_max		= 0xff;
+	} else {
+		data		= (fdt32_t *)prop->data;

lib/pci-host-generic.c line 108

Quoted 5 lines

lib/pci-host-generic.c new file +5 -0

+		bus		= fdt32_to_cpu(data[0]);
+		bus_max		= fdt32_to_cpu(data[1]);
+		assert(bus <= bus_max);
+	}
+	assert(bus_max < base.size / (1 << PCI_ECAM_BUS_SHIFT));

Maybe assert(bus_max + 1 == base.size / (1 << PCI_ECAM_BUS_SHIFT)) ? Or is there a reason to have a larger address space than necessary?

Quoted 18 lines

lib/pci-host-generic.c new file +18 -0

+	ret = dt_get_nr_cells(node, &nac, &nsc);
+	assert(ret == 0);
+	assert(nac == 3 && nsc == 2);
+
+	prop = fdt_get_property(fdt, node, "ranges", &len);
+	assert(prop != NULL);
+
+	nr_range_cells = nac + nsc + nac_root;
+	nr_addr_spaces = (len / 4) / nr_range_cells;
+	assert(nr_addr_spaces);
+
+	host = malloc(sizeof(*host) +
+		      sizeof(host->addr_space[0]) * nr_addr_spaces);
+	assert(host != NULL);
+
+	host->start		= base.addr;
+	host->size		= base.size;
+	host->bus		= bus;

lib/pci-host-generic.c line 137

Quoted 10 lines

lib/pci-host-generic.c new file +10 -0

+	host->bus_max		= bus_max;
+	host->nr_addr_spaces	= nr_addr_spaces;
+
+	data = (fdt32_t *)prop->data;
+	as = &host->addr_space[0];
+
+	for (i = 0; i < nr_addr_spaces; i++) {
+		/*
+		 * The PCI binding claims the numerical representation of a PCI
+		 * address consists of three cells, encoded as follows:

nit: No need to say 'claims', "The PCI binding encodes the PCI address with three cells as follows:"

Quoted 32 lines

lib/pci-host-generic.c new file +32 -0

+		 *
+		 * phys.hi  cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
+		 * phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
+		 * phys.lo  cell: llllllll llllllll llllllll llllllll
+		 *
+		 * PCI device bus address and flags are encoded into phys.high
+		 * PCI 64 bit address is encoded into phys.mid and phys.low
+		 */
+		as->type = of_flags_to_pci_type(fdt32_to_cpu(data[0]));
+		as->pci_start = ((u64)fdt32_to_cpu(data[1]) << 32) |
+				fdt32_to_cpu(data[2]);
+
+		if (nr_range_cells == 6) {
+			as->start = fdt32_to_cpu(data[3]);
+			as->size  = ((u64)fdt32_to_cpu(data[4]) << 32) |
+				    fdt32_to_cpu(data[5]);
+		} else {
+			as->start = ((u64)fdt32_to_cpu(data[3]) << 32) |
+				    fdt32_to_cpu(data[4]);
+			as->size  = ((u64)fdt32_to_cpu(data[5]) << 32) |
+				    fdt32_to_cpu(data[6]);
+		}
+
+		data += nr_range_cells;
+		as++;
+	}
+
+	return host;
+}
+
+static u64 pci_alloc_resource(u32 bar, u64 size)
+{

lib/pci-host-generic.c line 179

Quoted 10 lines

lib/pci-host-generic.c new file +10 -0

+	struct pci_host_bridge *host = pci_host_bridge;
+	struct pci_addr_space *as = &host->addr_space[0];
+	phys_addr_t addr;
+	u32 mask;
+	int i;
+
+	for (i = 0; i < host->nr_addr_spaces; i++) {
+		if (as->type == pci_bar_type(bar))
+			break;
+		as++;

nit: could have put this as++ up with the i++ like you do below

Quoted 211 lines

lib/pci-host-generic.c new file +115 -0

+	}
+	if (i >= host->nr_addr_spaces) {
+		printf("No PCI resource found for a device\n");
+		assert(0);
+	}
+
+	mask = pci_bar_mask(bar);
+	size = ALIGN(size, ~mask + 1);
+	assert(as->allocated + size <= as->size);
+
+	addr = as->pci_start + as->allocated;
+	as->allocated += size;
+
+	return addr;
+}
+
+bool pci_probe(void)
+{
+	pcidevaddr_t dev;
+	u8 header;
+	u32 cmd;
+	int i;
+
+	assert(!pci_host_bridge);
+	pci_host_bridge = pci_dt_probe();
+	if (!pci_host_bridge)
+		return false;
+
+	for (dev = 0; dev < 256; dev++) {
+		if (!pci_dev_exists(dev))
+			continue;
+
+		/* We are only interested in normal PCI devices */
+		header = pci_config_readb(dev, PCI_HEADER_TYPE);
+		if ((header & PCI_HEADER_TYPE_MASK) != PCI_HEADER_TYPE_NORMAL)
+			continue;
+
+		cmd = PCI_COMMAND_SERR;
+
+		for (i = 0; i < 6; i++) {
+			u64 addr, size;
+			u32 bar;
+
+			size = pci_bar_size(dev, i);
+			if (!size)
+				break;
+
+			bar = pci_bar_get(dev, i);
+			addr = pci_alloc_resource(bar, size);
+			pci_bar_set_addr(dev, i, addr);
+
+			if (pci_bar_is_memory(dev, i))
+				cmd |= PCI_COMMAND_MEMORY;
+			else
+				cmd |= PCI_COMMAND_IO;
+
+			if (pci_bar_is64(dev, i))
+				i++;
+		}
+
+		pci_config_writel(dev, PCI_COMMAND, cmd);
+	}
+
+	return true;
+}
+
+/*
+ * This function is to be called from pci_translate_addr() to provide
+ * mapping between this host bridge's PCI busses address and CPU physical
+ * address.
+ */
+phys_addr_t pci_host_bridge_get_paddr(u64 pci_addr)
+{
+	struct pci_host_bridge *host = pci_host_bridge;
+	struct pci_addr_space *as = &host->addr_space[0];
+	int i;
+
+	for (i = 0; i < host->nr_addr_spaces; i++, as++) {
+		if (pci_addr >= as->pci_start &&
+		    pci_addr < as->pci_start + as->size)
+			return as->start + (pci_addr - as->pci_start);
+	}
+
+	return 0;
+}
+
+static void __iomem *pci_get_dev_conf(struct pci_host_bridge *host, int devfn)
+{
+	return (void __iomem *)(unsigned long)
+		host->start + (devfn << PCI_ECAM_DEVFN_SHIFT);
+}
+
+u8 pci_config_readb(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readb(conf + off);
+}
+
+u16 pci_config_readw(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readw(conf + off);
+}
+
+u32 pci_config_readl(pcidevaddr_t dev, u8 off)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	return readl(conf + off);
+}
+
+void pci_config_writel(pcidevaddr_t dev, u8 off, u32 val)
+{
+	void __iomem *conf = pci_get_dev_conf(pci_host_bridge, dev);
+	writel(val, conf + off);
+}

lib/pci-host-generic.h new file +46 -0

new file mode 100644
index 000000000000..fd30e7c74ed8
@@ -0,0 +1,46 @@
+#ifndef PCI_HOST_GENERIC_H
+#define PCI_HOST_GENERIC_H
+/*
+ * PCI host bridge supporting structures and constants
+ *
+ * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <[email protected]>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include "libcflat.h"
+
+struct pci_addr_space {
+	phys_addr_t		pci_start;
+	phys_addr_t		start;
+	phys_addr_t		size;
+	phys_addr_t		allocated;
+	int			type;
+};
+
+struct pci_host_bridge {
+	phys_addr_t		start;
+	phys_addr_t		size;
+	int			bus;
+	int			bus_max;
+	int			nr_addr_spaces;
+	struct pci_addr_space	addr_space[];
+};
+
+/*
+ * The following constants are derived from Linux, see this source:
+ *
+ *         drivers/pci/host/pci-host-generic.c
+ *                 struct gen_pci_cfg_bus_ops::bus_shift
+ *                 int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
+ *
+ * Documentation/devicetree/bindings/pci/host-generic-pci.txt describes
+ * ECAM Configuration Space is be memory-mapped by concatenating the various
+ * components to form an offset:
+ *
+ *	cfg_offset(bus, device, function, register) =
+ *		   bus << 20 | device << 15 | function << 12 | register
+ */
+#define PCI_ECAM_BUS_SHIFT	20
+#define PCI_ECAM_DEVFN_SHIFT	12
+
+#endif

lib/pci.c +2 -2

index e715a3e51cc4..41a44d04d475 100644
@@ -27,13 +27,13 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id)
 	return PCIDEVADDR_INVALID;
 }
 
-static uint32_t pci_bar_mask(uint32_t bar)
+uint32_t pci_bar_mask(uint32_t bar)
 {
 	return (bar & PCI_BASE_ADDRESS_SPACE_IO) ?
 		PCI_BASE_ADDRESS_IO_MASK : PCI_BASE_ADDRESS_MEM_MASK;
 }
 
-static uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
+uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num)
 {
 	return pci_config_readl(dev, PCI_BASE_ADDRESS_0 + bar_num * 4);
 }

lib/pci.h +3 -0

index e9911db92320..37b282169a4e 100644
@@ -15,6 +15,7 @@ enum {
 	PCIDEVADDR_INVALID = 0xffff,
 };
 void pci_print(void);
+bool pci_probe(void);
 bool pci_dev_exists(pcidevaddr_t dev);
 pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 

@@ -32,6 +33,8 @@ pcidevaddr_t pci_find_dev(uint16_t vendor_id, uint16_t device_id);
 phys_addr_t pci_bar_get_addr(pcidevaddr_t dev, int bar_num);
 void pci_bar_set_addr(pcidevaddr_t dev, int bar_num, phys_addr_t addr);
 phys_addr_t pci_bar_size(pcidevaddr_t dev, int bar_num);
+uint32_t pci_bar_get(pcidevaddr_t dev, int bar_num);
+uint32_t pci_bar_mask(uint32_t bar);
 bool pci_bar_is64(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_memory(pcidevaddr_t dev, int bar_num);
 bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num);

Signature · 1 line

1.8.3.1

Looks great! Only a couple nits, so

Reviewed-by Andrew Jones <[email protected]>