arm64: gicv3: its: Fixes and updates for ThunderX

From: Tirumalesh Chalamarla <[email protected]>

PCI stream ids need to consider pci bridge number to be unique on the system. Using only bus and devfn can't do the trick in systems that have multiple pci bridges.

Signed-off-by Tirumalesh Chalamarla <[email protected]>

Signed-off-by Robert Richter <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-gic-v3-its.c +0 -0

index 9687f8afebff..e30b4de04c6c 100644

drivers/irqchip/irq-gic-v3-its.c static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data) · line 1189

Quoted 2 lines

drivers/irqchip/irq-gic-v3-its.c +1 -1

-	dev_alias->dev_id = alias;
+	dev_alias->dev_id = (pci_domain_nr(pdev->bus) << 16) | alias;

This feels very scary. We're now assuming that the domain number will always be presented to the doorbell. What guarantee do we have that this is always the case, irrespective of the platform?

Also, domains have no PCI reality, they are a Linux thing. And they can be "randomly" assigned, unless you force the domain in DT with a linux,pci-domain property. This looks even more wrong, specially considering ACPI.

It really feels like we need a way to describe how the BDF numbering is augmented. We also need to guarantee that we get the actual bridge number, as opposed to the domain number.

Mark,

thanks for review, also of the other patches of this series.

See below

Quoted 11 lines · Marc Zyngier

drivers/irqchip/irq-gic-v3-its.c static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data) · line 1189

Quoted 2 lines

drivers/irqchip/irq-gic-v3-its.c +1 -1

-	dev_alias->dev_id = alias;
+	dev_alias->dev_id = (pci_domain_nr(pdev->bus) << 16) | alias;

This feels very scary. We're now assuming that the domain number will always be presented to the doorbell. What guarantee do we have that this is always the case, irrespective of the platform?

Also, domains have no PCI reality, they are a Linux thing. And they can be "randomly" assigned, unless you force the domain in DT with a linux,pci-domain property. This looks even more wrong, specially considering ACPI.

The main problem here is that device ids (32 bits) are system specific. Since we have more than one PCI root complex we need the upper 16 bits in the devid for mapping. Using pci_domain_nr for this fits our needs for now and shouldn't affect systems with a single RC only as the domain nr is zero then.

The domain number is incremented during initialization beginnig with zero and the order of it is fixed since it is taken from DT or ACPI tables. So we have full controll of it. I don't see issues here.

Quoted 3 lines

It really feels like we need a way to describe how the BDF numbering is augmented. We also need to guarantee that we get the actual bridge number, as opposed to the domain number.

But true, the obove is just intermediate. In the end we need some sort of handler that is setup during cpu initialization that registers a callback for the gic to determine the device id of that paricular system.

Quoted 25 lines · Robert Richter

Mark,

thanks for review, also of the other patches of this series.

See below

Quoted 11 lines · Marc Zyngier

drivers/irqchip/irq-gic-v3-its.c static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data) · line 1189

Quoted 2 lines

drivers/irqchip/irq-gic-v3-its.c +1 -1

-	dev_alias->dev_id = alias;
+	dev_alias->dev_id = (pci_domain_nr(pdev->bus) << 16) | alias;

This feels very scary. We're now assuming that the domain number will always be presented to the doorbell. What guarantee do we have that this is always the case, irrespective of the platform?

Also, domains have no PCI reality, they are a Linux thing. And they can be "randomly" assigned, unless you force the domain in DT with a linux,pci-domain property. This looks even more wrong, specially considering ACPI.

The main problem here is that device ids (32 bits) are system specific. Since we have more than one PCI root complex we need the upper 16 bits in the devid for mapping. Using pci_domain_nr for this fits our needs for now and shouldn't affect systems with a single RC only as the domain nr is zero then.

Quoted 3 lines

The domain number is incremented during initialization beginnig with zero and the order of it is fixed since it is taken from DT or ACPI tables. So we have full controll of it. I don't see issues here.

This may match what you have on ThunderX (as long as the kernel doesn't adopt another behaviour when allocating the domain number). But other platforms may have a completely different numbering, which will mess them up entirely.

Quoted 8 lines

Quoted 3 lines

It really feels like we need a way to describe how the BDF numbering is augmented. We also need to guarantee that we get the actual bridge number, as opposed to the domain number.

But true, the obove is just intermediate. In the end we need some sort of handler that is setup during cpu initialization that registers a callback for the gic to determine the device id of that paricular system.

I don't really like the idea of a callback from the GIC - I'd prefer it to be standalone, and rely on the topology information to build the DeviceID. Mark Rutland had some ideas for DT (he posted an RFC a while ago), maybe it would be good to get back to that and find out what we can do. ACPI should also have similar information (IORT?).

Thanks,

	M.

Signature · 1 line

Jazz is not dead. It just smells funny...

Quoted 47 lines · Marc Zyngier

Quoted 25 lines · Robert Richter

Mark,

thanks for review, also of the other patches of this series.

See below

Quoted 11 lines · Marc Zyngier

drivers/irqchip/irq-gic-v3-its.c static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data) · line 1189

Quoted 2 lines

drivers/irqchip/irq-gic-v3-its.c +1 -1

-	dev_alias->dev_id = alias;
+	dev_alias->dev_id = (pci_domain_nr(pdev->bus) << 16) | alias;

This feels very scary. We're now assuming that the domain number will always be presented to the doorbell. What guarantee do we have that this is always the case, irrespective of the platform?

Also, domains have no PCI reality, they are a Linux thing. And they can be "randomly" assigned, unless you force the domain in DT with a linux,pci-domain property. This looks even more wrong, specially considering ACPI.

The main problem here is that device ids (32 bits) are system specific. Since we have more than one PCI root complex we need the upper 16 bits in the devid for mapping. Using pci_domain_nr for this fits our needs for now and shouldn't affect systems with a single RC only as the domain nr is zero then.

Quoted 3 lines

The domain number is incremented during initialization beginnig with zero and the order of it is fixed since it is taken from DT or ACPI tables. So we have full controll of it. I don't see issues here.

This may match what you have on ThunderX (as long as the kernel doesn't adopt another behaviour when allocating the domain number). But other platforms may have a completely different numbering, which will mess them up entirely.

Quoted 8 lines

Quoted 3 lines

It really feels like we need a way to describe how the BDF numbering is augmented. We also need to guarantee that we get the actual bridge number, as opposed to the domain number.

But true, the obove is just intermediate. In the end we need some sort of handler that is setup during cpu initialization that registers a callback for the gic to determine the device id of that paricular system.

Quoted 6 lines

I don't really like the idea of a callback from the GIC - I'd prefer it to be standalone, and rely on the topology information to build the DeviceID. Mark Rutland had some ideas for DT (he posted an RFC a while ago), maybe it would be good to get back to that and find out what we can do. ACPI should also have similar information (IORT?).

How can some one pass this from DT, especially in GIC entry. i still think it is bus owner responsibility and call back is better idea. but if some one has a better idea for DT and ACPI, we are fine as long as it works on ThunderX.

From: Robert Richter <[email protected]>

The number of pages for the its table may exceed the maximum of 256. Adding a range check and limitting the number to its maximum.

Based on a patch from Tirumalesh Chalamarla <[email protected]>.

Signed-off-by Robert Richter <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c   | 11 ++++++++++-
 include/linux/irqchip/arm-gic-v3.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

drivers/irqchip/irq-gic-v3-its.c +10 -1

index e30b4de04c6c..a2619a1d5bb3 100644
@@ -810,6 +810,7 @@ static int its_alloc_tables(struct its_node *its)
 		u64 entry_size = GITS_BASER_ENTRY_SIZE(val);
 		int order = get_order(psz);
 		int alloc_size;
+		int alloc_pages;
 		u64 tmp;
 		void *base;
 

@@ -837,6 +838,14 @@ static int its_alloc_tables(struct its_node *its)
 		}
 
 		alloc_size = (1 << order) * PAGE_SIZE;
+		alloc_pages = (alloc_size / psz);
+		if (alloc_pages > GITS_BASER_PAGES_MAX) {
+			alloc_pages = GITS_BASER_PAGES_MAX;
+			order = get_order(GITS_BASER_PAGES_MAX * psz);
+			pr_warn("%s: Device Table too large, reduce its page order to %u (%u pages)\n",
+				its->msi_chip.of_node->full_name, order, alloc_pages);
+		}
+
 		base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
 		if (!base) {
 			err = -ENOMEM;

@@ -865,7 +874,7 @@ static int its_alloc_tables(struct its_node *its)
 			break;
 		}
 
-		val |= (alloc_size / psz) - 1;
+		val |= alloc_pages - 1;
 
 		writeq_relaxed(val, its->base + GITS_BASER + i * 8);
 		tmp = readq_relaxed(its->base + GITS_BASER + i * 8);

include/linux/irqchip/arm-gic-v3.h +1 -0

index ffbc034c8810..f28da189c4aa 100644
@@ -229,6 +229,7 @@
 #define GITS_BASER_PAGE_SIZE_16K	(1UL << GITS_BASER_PAGE_SIZE_SHIFT)
 #define GITS_BASER_PAGE_SIZE_64K	(2UL << GITS_BASER_PAGE_SIZE_SHIFT)
 #define GITS_BASER_PAGE_SIZE_MASK	(3UL << GITS_BASER_PAGE_SIZE_SHIFT)
+#define GITS_BASER_PAGES_MAX		256
 
 #define GITS_BASER_TYPE_NONE		0
 #define GITS_BASER_TYPE_DEVICE		1

From: Robert Richter <[email protected]>

No need to read the typer register in the loop. Values do not change.

Signed-off-by Robert Richter <[email protected]>

---
 drivers/irqchip/irq-gic-v3-its.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/irqchip/irq-gic-v3-its.c +2 -3

index a2619a1d5bb3..916c4724c771 100644
@@ -803,6 +803,8 @@ static int its_alloc_tables(struct its_node *its)
 	int psz = SZ_64K;
 	u64 shr = GITS_BASER_InnerShareable;
 	u64 cache = GITS_BASER_WaWb;
+	u64 typer = readq_relaxed(its->base + GITS_TYPER);
+	u32 ids = GITS_TYPER_DEVBITS(typer);
 
 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
 		u64 val = readq_relaxed(its->base + GITS_BASER + i * 8);

@@ -826,9 +828,6 @@ static int its_alloc_tables(struct its_node *its)
 		 * For other tables, only allocate a single page.
 		 */
 		if (type == GITS_BASER_TYPE_DEVICE) {
-			u64 typer = readq_relaxed(its->base + GITS_TYPER);
-			u32 ids = GITS_TYPER_DEVBITS(typer);
-
 			order = get_order((1UL << ids) * entry_size);
 			if (order >= MAX_ORDER) {
 				order = MAX_ORDER - 1;

From: Radha Mohan Chintakuntla <[email protected]>

In case of ARCH_THUNDER, there is a need to allocate the GICv3 ITS table which is bigger than the allowed max order. So we are forcing it only in case of 4KB page size.

Quoted 5 lines · Robert Richter

From: Radha Mohan Chintakuntla <[email protected]>

In case of ARCH_THUNDER, there is a need to allocate the GICv3 ITS table which is bigger than the allowed max order. So we are forcing it only in case of 4KB page size.

Does this problem disappear if the ITS driver uses dma_alloc_coherent instead? That would also allow us to remove the __flush_dcache_area abuse from the driver.

Quoted 10 lines · Will Deacon

Quoted 5 lines · Robert Richter

From: Radha Mohan Chintakuntla <[email protected]>

In case of ARCH_THUNDER, there is a need to allocate the GICv3 ITS table which is bigger than the allowed max order. So we are forcing it only in case of 4KB page size.

Does this problem disappear if the ITS driver uses dma_alloc_coherent instead? That would also allow us to remove the __flush_dcache_area abuse from the driver.

__get_free_pages() is also used internally in dma_alloc_coherent().

There is another case if the device brings dma mem with it. I am not sure if it would be possible to assign some phys memory via devicetree to the interrupt controller and then assign that range for its table allocation.

Another option would be to allocate a hugepage. This would require setting up hugepages during boottime. I need to figure out whether that could work.

What about on the remaining 3 patches?

Quoted 14 lines · Robert Richter

Quoted 10 lines · Will Deacon

Quoted 5 lines · Robert Richter

From: Radha Mohan Chintakuntla <[email protected]>

In case of ARCH_THUNDER, there is a need to allocate the GICv3 ITS table which is bigger than the allowed max order. So we are forcing it only in case of 4KB page size.

Does this problem disappear if the ITS driver uses dma_alloc_coherent instead? That would also allow us to remove the __flush_dcache_area abuse from the driver.

__get_free_pages() is also used internally in dma_alloc_coherent().

Quoted 8 lines

There is another case if the device brings dma mem with it. I am not sure if it would be possible to assign some phys memory via devicetree to the interrupt controller and then assign that range for its table allocation.

Another option would be to allocate a hugepage. This would require setting up hugepages during boottime. I need to figure out whether that could work.

Quoted 1 line

What about on the remaining 3 patches?

Marc would be the best guy to review those, but he's on holiday for a couple of weeks at the moment.

Quoted 20 lines · Will Deacon

Quoted 19 lines · Robert Richter

Quoted 10 lines · Will Deacon

Quoted 5 lines · Robert Richter

From: Radha Mohan Chintakuntla <[email protected]>

In case of ARCH_THUNDER, there is a need to allocate the GICv3 ITS table which is bigger than the allowed max order. So we are forcing it only in case of 4KB page size.

Does this problem disappear if the ITS driver uses dma_alloc_coherent instead? That would also allow us to remove the __flush_dcache_area abuse from the driver.

__get_free_pages() is also used internally in dma_alloc_coherent().

There is another case if the device brings dma mem with it. I am not sure if it would be possible to assign some phys memory via devicetree to the interrupt controller and then assign that range for its table allocation.

Quoted 3 lines

Quoted 3 lines

Another option would be to allocate a hugepage. This would require setting up hugepages during boottime. I need to figure out whether that could work.

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

* set FORCE_MAX_ZONEORDER to 13 as default,

* set FORCE_MAX_ZONEORDER to 13 if ARM_GIC_V3 is set,

* set FORCE_MAX_ZONEORDER to 13 if ARCH_THUNDER is set (this patch),

* use hugepages if enabled (defconfig has the following options
enable: CGROUP_HUGETLB, TRANSPARENT_HUGEPAGE, HUGETLBFS, this might work with current default kernel without changing defconfig options),

* use devicetree to reserve mem for gicv3 (need to check ACPI).

Do you see any direction?

Quoted 2 lines · Robert Richter

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

Quoted 2 lines

* set FORCE_MAX_ZONEORDER to 13 as default,

Quoted 3 lines

* set FORCE_MAX_ZONEORDER to 13 if ARM_GIC_V3 is set,

* set FORCE_MAX_ZONEORDER to 13 if ARCH_THUNDER is set (this patch),

I'm not hugely fond of these suggestions, as there's still no guarantee that such a huge allocation is going to succeed and we end up bumping MAX_ORDER for all platforms in defconfig if we enable THUNDER there.

Quoted 4 lines

* use hugepages if enabled (defconfig has the following options
enable: CGROUP_HUGETLB, TRANSPARENT_HUGEPAGE, HUGETLBFS, this might work with current default kernel without changing defconfig options),

I don't think hugepages help with DMA.

Quoted 1 line

* use devicetree to reserve mem for gicv3 (need to check ACPI).

Using a carveout like this might be the best bet. I assume the memory used by the ITS can never be reclaimed by the syste (and therefore there's no issue with wastage)?

Quoted 1 line

Do you see any direction?

Dunno, does CMA also require the MAX_ORDER bump?

Quoted 25 lines · Robert Richter

Quoted 23 lines · Will Deacon

Quoted 22 lines · Robert Richter

Quoted 10 lines · Will Deacon

Quoted 5 lines · Robert Richter

From: Radha Mohan Chintakuntla <[email protected]>

In case of ARCH_THUNDER, there is a need to allocate the GICv3 ITS table which is bigger than the allowed max order. So we are forcing it only in case of 4KB page size.

Does this problem disappear if the ITS driver uses dma_alloc_coherent instead? That would also allow us to remove the __flush_dcache_area abuse from the driver.

__get_free_pages() is also used internally in dma_alloc_coherent().

There is another case if the device brings dma mem with it. I am not sure if it would be possible to assign some phys memory via devicetree to the interrupt controller and then assign that range for its table allocation.

Another option would be to allocate a hugepage. This would require setting up hugepages during boottime. I need to figure out whether that could work.

Quoted 2 lines

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

Quoted 6 lines · On Tue, 12 May 2015 18:24:16 +0100 Will Deacon <[email protected]> wrote:

Quoted 4 lines · Robert Richter

On 12.05.15 13:30:57, Will Deacon wrote:

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

Quoted 2 lines

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

That's probably due to the sparseness of the DeviceID space. With some form of bridge number encoded on top of the BFD number, the device table is enormous, and I don't see a nice way to avoid it...

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · On Tue, 12 May 2015 18:24:16 +0100 Will Deacon <[email protected]> wrote:

Quoted 4 lines · Robert Richter

On 12.05.15 13:30:57, Will Deacon wrote:

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

Quoted 6 lines

Quoted 2 lines

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

That's probably due to the sparseness of the DeviceID space. With some form of bridge number encoded on top of the BFD number, the device table is enormous, and I don't see a nice way to avoid it...

Right. At the momement out of 21 bits (16MB) we currently have 2 spare bits, which reduces the actually size used to 4MB. Though, for the current cpu model we can reduce it at least to 8MB total.

I will come up with an additional patch setting this to 8MB.

As said before, I also write on a patch to use CMA.

Quoted 20 lines · Robert Richter

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · On Tue, 12 May 2015 18:24:16 +0100 Will Deacon <[email protected]> wrote:

Quoted 4 lines · Robert Richter

On 12.05.15 13:30:57, Will Deacon wrote:

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

Quoted 6 lines

Quoted 2 lines

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

That's probably due to the sparseness of the DeviceID space. With some form of bridge number encoded on top of the BFD number, the device table is enormous, and I don't see a nice way to avoid it...

Right. At the momement out of 21 bits (16MB) we currently have 2 spare bits, which reduces the actually size used to 4MB. Though, for the current cpu model we can reduce it at least to 8MB total.

Quoted 3 lines

I will come up with an additional patch setting this to 8MB.

As said before, I also write on a patch to use CMA.

Can we not reserve a chunk of memory and pass the information to the kernel via DT (/memreserve/ and a new GIC-specific binding)?

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Robert Richter

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · On Tue, 12 May 2015 18:24:16 +0100 Will Deacon <[email protected]> wrote:

Quoted 4 lines · Robert Richter

On 12.05.15 13:30:57, Will Deacon wrote:

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

Quoted 6 lines

Quoted 2 lines

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

That's probably due to the sparseness of the DeviceID space. With some form of bridge number encoded on top of the BFD number, the device table is enormous, and I don't see a nice way to avoid it...

Right. At the momement out of 21 bits (16MB) we currently have 2 spare bits, which reduces the actually size used to 4MB. Though, for the current cpu model we can reduce it at least to 8MB total.

Quoted 6 lines

Quoted 3 lines

I will come up with an additional patch setting this to 8MB.

As said before, I also write on a patch to use CMA.

Can we not reserve a chunk of memory and pass the information to the kernel via DT (/memreserve/ and a new GIC-specific binding)?

That would have to be done on a per-table basis then. And how would that work with ACPI? I don't think the ACPI ITS table specifies anything in that respect.

We're just facing the horrible reality that linear tables are not very well suited to sparse addressing. Nobody copied the VAX MMU model for a reason... until now.

Quoted 33 lines · Marc Zyngier

Quoted 21 lines · Catalin Marinas

Quoted 20 lines · Robert Richter

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · On Tue, 12 May 2015 18:24:16 +0100 Will Deacon <[email protected]> wrote:

Quoted 4 lines · Robert Richter

On 12.05.15 13:30:57, Will Deacon wrote:

For allocation of 16MB cont. phys mem of a defconfig kernel (4KB default pagesize) I see this different approaches:

Quoted 6 lines

Quoted 2 lines

16MB sounds like an awful lot. Is this because you have tonnes of MSIs or a sparse DeviceID space or both?

That's probably due to the sparseness of the DeviceID space. With some form of bridge number encoded on top of the BFD number, the device table is enormous, and I don't see a nice way to avoid it...

Right. At the momement out of 21 bits (16MB) we currently have 2 spare bits, which reduces the actually size used to 4MB. Though, for the current cpu model we can reduce it at least to 8MB total.

Quoted 6 lines

Quoted 3 lines

I will come up with an additional patch setting this to 8MB.

As said before, I also write on a patch to use CMA.

Can we not reserve a chunk of memory and pass the information to the kernel via DT (/memreserve/ and a new GIC-specific binding)?

That would have to be done on a per-table basis then. And how would that work with ACPI? I don't think the ACPI ITS table specifies anything in that respect.

Quoted 3 lines

We're just facing the horrible reality that linear tables are not very well suited to sparse addressing. Nobody copied the VAX MMU model for a reason... until now.

We could still fall back to mem alloc if the DT or ACPI does not provide a base address for the table.

For know I would prefer to just implement mem allocation with CMA.