ARM: mvebu: disable I/O coherency on !SMP

Some independent testing/review of this patch series would definitely be welcome.

Jason, Andrew, Gregory, Sebastian,

I believe you have seen the on-going discussion with Russell on how to ensure that the relevant requirements for hardware I/O coherency are meant in non-SMP situations. Since it appears it will take quite a bit of time to find a proper solution to this problem, this patch series proposes to simply disable hardware I/O coherency in problematic configurations.

Quoted 6 lines

I believe you have seen the on-going discussion with Russell on how to ensure that the relevant requirements for hardware I/O coherency are meant in non-SMP situations. Since it appears it will take quite a bit of time to find a proper solution to this problem, this patch series proposes to simply disable hardware I/O coherency in problematic configurations.

It is unfortunate, but I do feel that there's been something of an excessive amount of pressure here - it seems to me that the I/O coherency as merged and enabled, without thinking about the consequences. When it was then realised that some fundamental core changes were needed, it's somehow been turned into my fault that this stuff doesn't work, and my problem to solve.

While it /is/ my problem to solve (or rather, ensure that it does get solved in a way which is compliant with the architecture) I'm also juggling several other things which leaves me not enough time to look at this right now.

It seems that people think that I have oodles of spare time to be able to jump onto their problem at a moments notice. I don't.

I've already said that device tree and single zImage makes solving your problem /much/ harder than it otherwise was. Right now, I don't have any solution to it in mind that would both satisfy the requirements of the architecture _and_ allow the coherency stuff to operate as Marvell wants it to.

Before device tree, we had the atags and the machine ID, with the machine_desc discovered in the early boot code. This would have allowed us to add a hook there which identified your coherent platform and adjusted the page tables appropriately to ensure that the coherency stuff worked as intended.

We don't have that anymore (not even in non-device tree mode), and device tree doesn't offer any replacement for it.

As I've frequently said, device tree is great for some things, but it makes solving other problems insanely more difficult. This is one of those which is insanely more difficult because it just doesn't fit the device tree model.

While it's regrettable to have to disable the coherency support, I think that's the best way to proceed at the moment until we're able to find some kind of solution.

The most important first step in that is working out how, in the early assembly code, we can identify these Armada SoCs. That's a task I can't undertake because I know next to nothing about the SoCs you're dealing with. Yes, I know that Marvell released a TRM a short while back, which is great, but not everyone has time to go around reading every TRM which every silicon vendor releases into the public domain.

If it /is/ possible to detect the Armada SoCs in the early assembly, then we can start to solve this.

1) The Armada 370 and Armada XP are perfectly detectable in the early assembly, since they have different values in the main identification register (they use PJ4B and PJ4B-MP cores).

You say that, but Dove is PJ4B too (I forget whether it's PJ4B or not.) The only way to be sure is to compare the ID numbers. The AP510 in Dove I have here has an ID register value of 0x560f5815. This ties up with the PJ4B entry in proc-v7.S:

        .type   __v7_pj4b_proc_info, #object
__v7_pj4b_proc_info:
        .long   0x560f5800
        .long   0xff0fff00
        __v7_proc __v7_pj4b_setup, proc_fns = pj4b_processor_functions
        .size   __v7_pj4b_proc_info, . - __v7_pj4b_proc_info

How does that compare to the ID you see on Armada 370 and XP? (I've no idea what IDs you get there...) If the ID is different, then we should be able to solve this pretty easily for these CPUs by creating a new proc_info record as I think I have previously suggested.

With a separate record, we can set the S bit if we need to, and we can also set the write-allocate mode too, now that the patch I sent you is in mainline (which is something I definitely indicated along with that patch.)

I've not really said anything new in this email which I haven't said before, with the exception of stating what the ID is for the Dove SoC I have. That's the only ID I know, and I assume that those working on mvebu have a better idea what ID numbers are found across all the families.

Russell,

Quoted 12 lines · Russell King - ARM Linux

You say that, but Dove is PJ4B too (I forget whether it's PJ4B or not.) The only way to be sure is to compare the ID numbers. The AP510 in Dove I have here has an ID register value of 0x560f5815. This ties up with the PJ4B entry in proc-v7.S:

        .type   __v7_pj4b_proc_info, #object
__v7_pj4b_proc_info:
        .long   0x560f5800
        .long   0xff0fff00
        __v7_proc __v7_pj4b_setup, proc_fns = pj4b_processor_functions
        .size   __v7_pj4b_proc_info, . - __v7_pj4b_proc_info

Quoted 4 lines

How does that compare to the ID you see on Armada 370 and XP? (I've no idea what IDs you get there...) If the ID is different, then we should be able to solve this pretty easily for these CPUs by creating a new proc_info record as I think I have previously suggested.

The ID for Armada 370 is 0x560f5811, but the last digit is the "Product Revision Number", so I'm not sure it can be used to distinguish Dove from Armada 370. However, I believe we could simply run Dove in a Write-Allocate cache policy.

Quoted 10 lines

With a separate record, we can set the S bit if we need to, and we can also set the write-allocate mode too, now that the patch I sent you is in mainline (which is something I definitely indicated along with that patch.)

I've not really said anything new in this email which I haven't said before, with the exception of stating what the ID is for the Dove SoC I have. That's the only ID I know, and I assume that those working on mvebu have a better idea what ID numbers are found across all the families.

Russell, please again what I've asked numerous times. I already wrote the patches that creates specific proc_info structures for Armada 370 and Armada XP to set the cache policy and S bit, as you suggested. But this is only setting the PMD flags, but doesn't touch the TTB flags, which remain only defined by ALT_UP/ALT_SMP conditional. And I remember Catalin saying that having PMD flags varying from the TTB flags could be a problem.

Sorry to be a bit harsh here but you seem to assume that I'm an idiot who didn't read what you said, and you simply repeat again and again that your patch already solves the problem by allowing the proc_info structure to define the PMD flags. But you never answered my question about the TTB flags, which is the question I've been asking since several e-mails already.

arch/arm/mm/proc-v7.S +1 -1

index 6e1b2da..310834e 100644
@@ -483,14 +483,14 @@ __v7_ca9mp_proc_info:
 __v7_pj4b_proc_info:
 	.long	0x560f5810
 	.long	0xff0ffff0
-	__v7_proc __v7_pj4b_setup, proc_fns = pj4b_processor_functions
+	__v7_proc __v7_pj4b_setup, proc_fns = pj4b_processor_functions, mm_mmuflags = PMD_SECT_WBWA

That's where having a separate processor struct comes in (which we already have for the pj4b) and having a separate switch_mm which knows to set the TTB flags to write allocate.

You're assuming that I remember all the details each time... I know that you have sent me several times all the details about what each Armada device requires, and each time I've briefly read it and thought "yes, I must get to that" and then never have done. That's why we're not making progress - I haven't had the time to read and digest your messages properly. For example, if you asked me right now what Armada XP requires, whether it's SMP or not, I really couldn't tell you.

Right, so you're using pj4b_processor_functions here.

#ifdef CONFIG_CPU_PJ4B

        globl_equ       cpu_pj4b_switch_mm,     cpu_v7_switch_mm
        globl_equ       cpu_pj4b_set_pte_ext,   cpu_v7_set_pte_ext
        globl_equ       cpu_pj4b_proc_init,     cpu_v7_proc_init

So we need to delete the first (switch_mm) so we can replace it. I think we may need a second set of processor functions to deal with the MP version as well. Most of these would need to be aliased to the cpu_pj4b versions, except for the switch_mm one too.

You may wish to consider changing switch_mm to look something like the following. I'm assuming that for pj4b you just need WBWA for both the inner and the outer, but not the shared bit - you'll need to change that if it needs something else:

ENTRY(cpu_pj4b_switch_mm)

	orr	r0, r0, #TTB_IRGN_WBWA | TTB_RGN_OC_WBWA
	b	1f
ENTRY(cpu_pj4b_mp_switch_mm)
	orr	r0, r0, #TTB_FLAGS_SMP
	b	1f

ENTRY(cpu_v7_switch_mm)

#ifdef CONFIG_MMU
        ALT_SMP(orr     r0, r0, #TTB_FLAGS_SMP)
        ALT_UP(orr      r0, r0, #TTB_FLAGS_UP)
1:      mmid    r1, r1                          @ get mm->context.id
        mov     r2, #0

#ifdef CONFIG_ARM_ERRATA_430973
mcr p15, 0, r2, c7, c5, 6 @ flush BTAC/BTB
#endif
...
ENDPROC(cpu_v7_switch_mm)
ENDPROC(cpu_pj4b_switch_mm)
ENDPROC(cpu_pj4b_mp_switch_mm)

That gets rid of the one in the switch_mm path. The setup function is much more icky.

I think __v7_setup needs splitting up - especially due to all the errata in there. I think we ought to add a whole load of processor proc_info's - for ARM Cortex-A8, Cortex-A9, Cortex-A15 etc. Each one calls a separate setup function which runs only the appropriate errata fixups for that core (which eliminates a load of testing in __v7_setup).

This also means we can split the v7_ttb_setup macro and change it to be per CPU. (I'm not entirely convinced right now that TTB1 isn't initialised with an undefined value - I can't see where r8 is setup amongst the mess that this has turned into.

I'm not sure what this means for the 3level v7 code - that's something I've never looked at except a brief glance when it went in. The v7_ttb_setup there looks rather complicated and might make the changes I outlined above much harder. That also isn't something I can play around with because I've never been able to get Linux running on the Versatile Express with the CPU card that would support that code. We may need to enlist Catalin's help to modify that code.

That's about the best guidance I can give this evening, and I hope it helps you.

The most important first step in that is working out how, in the early assembly code, we can identify these Armada SoCs. That's a task I can't undertake because I know next to nothing about the SoCs you're dealing with. Yes, I know that Marvell released a TRM a short while back, which is great, but not everyone has time to go around reading every TRM which every silicon vendor releases into the public domain.

If it /is/ possible to detect the Armada SoCs in the early assembly, then we can start to solve this.

If I understand correctly, we are already changing the page tables anyway, to switch certain pages to be mapped uncached, to do DMA coherent allocations, no?

So wouldn't it be possible to keep the early
assembly code as it is today, and then later, in C code, once we have identified the platform on which we're running, modify the page tables to switch to the appropriate page attributes (write allocate cache policy, shareable attribute, etc.) ?

Quoted 5 lines

So wouldn't it be possible to keep the early
assembly code as it is today, and then later, in C code, once we have identified the platform on which we're running, modify the page tables to switch to the appropriate page attributes (write allocate cache policy, shareable attribute, etc.) ?

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

Russell,

Quoted 13 lines · Russell King - ARM Linux

Quoted 5 lines

So wouldn't it be possible to keep the early
assembly code as it is today, and then later, in C code, once we have identified the platform on which we're running, modify the page tables to switch to the appropriate page attributes (write allocate cache policy, shareable attribute, etc.) ?

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

Quoted 4 lines

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

That's indeed tricky. But then, if we don't find solutions to support in mainline the features of those SoCs, why are we asking all those SoC vendors to push their code mainline?

Quoted 3 lines · Thomas Petazzoni

If I understand correctly, we are already changing the page tables anyway, to switch certain pages to be mapped uncached, to do DMA coherent allocations, no?

I've no idea, I never looked at that code. I hope that Marek has considered the requirements of the architecture when creating that code...

Quoted 3 lines · Thomas Petazzoni

If I understand correctly, we are already changing the page tables anyway, to switch certain pages to be mapped uncached, to do DMA coherent allocations, no?

I've no idea, I never looked at that code. I hope that Marek has considered the requirements of the architecture when creating that code...

Quoted 5 lines

So wouldn't it be possible to keep the early
assembly code as it is today, and then later, in C code, once we have identified the platform on which we're running, modify the page tables to switch to the appropriate page attributes (write allocate cache policy, shareable attribute, etc.) ?

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

Quoted 11 lines · On Thursday 17 July 2014 09:33:42 Russell King - ARM Linux wrote:

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

Is this different from what we do in the LPAE version of early_paging_init()? That code already adjusts all the page table entries on a per-platform setting and should be very easy to extend for a modified procinfo->__cpu_mm_mmu_flags, and possibly able to extend for traditional (non-LPAE) page tables without a lot of duplication.

From a glance over that function, I think that is unsafe for all the

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Thursday 17 July 2014 09:33:42 Russell King - ARM Linux wrote:

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

Quoted 6 lines

Is this different from what we do in the LPAE version of early_paging_init()? That code already adjusts all the page table entries on a per-platform setting and should be very easy to extend for a modified procinfo->__cpu_mm_mmu_flags, and possibly able to extend for traditional (non-LPAE) page tables without a lot of duplication.

I'm going to have to profess no knowledge of how the LPAE stuff works. LPAE is something I can't run, and something that I was not involved in its creation. Therefore, I know very little about it and zero experience thereof.

From a glance over that function, I think that is unsafe for all the reasons I've stated. However, I'll pass this over to Will Deacon to comment further on.

Quoted 26 lines · Russell King - ARM Linux

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Thursday 17 July 2014 09:33:42 Russell King - ARM Linux wrote:

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

Quoted 6 lines

Is this different from what we do in the LPAE version of early_paging_init()? That code already adjusts all the page table entries on a per-platform setting and should be very easy to extend for a modified procinfo->__cpu_mm_mmu_flags, and possibly able to extend for traditional (non-LPAE) page tables without a lot of duplication.

I'm going to have to profess no knowledge of how the LPAE stuff works. LPAE is something I can't run, and something that I was not involved in its creation. Therefore, I know very little about it and zero experience thereof.

Quoted 3 lines

From a glance over that function, I think that is unsafe for all the reasons I've stated. However, I'll pass this over to Will Deacon to comment further on.

Just an FYI, but I've had to check internally to clarify the behaviour around TLB conflict aborts. We're changing live page tables here, but the *only* thing to differ is the output address, which I would hope is ok but need to check to be sure.

Quoted 4 lines · On Thursday 17 July 2014 15:27:20 Will Deacon wrote:

Just an FYI, but I've had to check internally to clarify the behaviour around TLB conflict aborts. We're changing live page tables here, but the *only* thing to differ is the output address, which I would hope is ok but need to check to be sure.

The two cases are slightly different:

- The existing keystone code changes the output address but not the flags
- The hypothetical mvebu code needs to change the flags but not the address.

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · On Thursday 17 July 2014 15:27:20 Will Deacon wrote:

Just an FYI, but I've had to check internally to clarify the behaviour around TLB conflict aborts. We're changing live page tables here, but the *only* thing to differ is the output address, which I would hope is ok but need to check to be sure.

Quoted 4 lines

The two cases are slightly different:

- The existing keystone code changes the output address but not the flags
- The hypothetical mvebu code needs to change the flags but not the address.

Don't base too much confidence in the keystone code:

        map_start = init_mm.start_code;
        map_end   = init_mm.brk;

        phys = __pa(map_start) & PMD_MASK;
        do {
...
                phys += PMD_SIZE;
        } while (phys < map_end);

Consider what happens when map_end is not PMD aligned - it misses the PMD covering the end of the kernel BSS, which will be quite a common case. Obviously something that needs fixing...

Quoted 7 lines · On Thursday 17 July 2014 16:34:01 Russell King - ARM Linux wrote:

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · On Thursday 17 July 2014 15:27:20 Will Deacon wrote:

Just an FYI, but I've had to check internally to clarify the behaviour around TLB conflict aborts. We're changing live page tables here, but the *only* thing to differ is the output address, which I would hope is ok but need to check to be sure.

Quoted 6 lines

Quoted 4 lines

The two cases are slightly different:

- The existing keystone code changes the output address but not the flags
- The hypothetical mvebu code needs to change the flags but not the address.

Don't base too much confidence in the keystone code:

What I meant was that verifying the correctness of one of the two above with the architecture team doesn't mean that the other one is okay too.

Quoted 9 lines

        map_start = init_mm.start_code;
        map_end   = init_mm.brk;

        phys = __pa(map_start) & PMD_MASK;
        do {
...
                phys += PMD_SIZE;
        } while (phys < map_end);

Quoted 3 lines

Consider what happens when map_end is not PMD aligned - it misses the PMD covering the end of the kernel BSS, which will be quite a common case. Obviously something that needs fixing...

Ouch.

Thanks for spotting it RMK. Will have a loot at it.

Quoted 1 line · Santosh Shilimkar

Thanks for spotting it RMK. Will have a loot at it.

I already have this for it - which includes a lot more comments on the code (some in anticipation of the reply from the architecture people.) The first hunk alone should fix the spotted problem.

arch/arm/mm/mmu.c +42 -5

index ab14b79b03f0..917aabd6b2dc 100644
@@ -1406,8 +1406,8 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 		return;
 
 	/* remap kernel code and data */
-	map_start = init_mm.start_code;
-	map_end   = init_mm.brk;
+	map_start = init_mm.start_code & PMD_MASK;
+	map_end   = ALIGN(init_mm.brk, PMD_SIZE);
 
 	/* get a handle on things... */
 	pgd0 = pgd_offset_k(0);

@@ -1434,23 +1434,60 @@ void __init early_paging_init(const struct machine_desc *mdesc,
 	dsb(ishst);
 	isb();
 
-	/* remap level 1 table */
+	/*
+	 * WARNING: This code is not architecturally compliant: we modify
+	 * the mappings in-place, indeed while they are in use by this
+	 * very same code.
+	 *
+	 * Even modifying the mappings in a separate page table does
+	 * not resolve this.
+	 *
+	 * The architecture strongly recommends that when a mapping is
+	 * changed, that it is changed by first going via an invalid
+	 * mapping and back to the new mapping.  This is to ensure
+	 * that no TLB conflicts (caused by the TLB having more than
+	 * one TLB entry match a translation) can occur.
+	 */
+
+	/*
+	 * Remap level 1 table.  This changes the physical addresses
+	 * used to refer to the level 2 page tables to the high
+	 * physical address alias, leaving everything else the same.
+	 */
 	for (i = 0; i < PTRS_PER_PGD; pud0++, i++) {
 		set_pud(pud0,
 			__pud(__pa(pmd0) | PMD_TYPE_TABLE | L_PGD_SWAPPER));
 		pmd0 += PTRS_PER_PMD;
 	}
 
-	/* remap pmds for kernel mapping */
-	phys = __pa(map_start) & PMD_MASK;
+	/*
+	 * Remap the level 2 table, pointing the mappings at the high
+	 * physical address alias of these pages.
+	 */
+	phys = __pa(map_start);
 	do {
 		*pmdk++ = __pmd(phys | pmdprot);
 		phys += PMD_SIZE;
 	} while (phys < map_end);
 
+	/*
+	 * Ensure that the above updates are flushed out of the cache.
+	 * This is not strictly correct; on a system where the caches
+	 * are coherent with each other, but the MMU page table walks
+	 * may not be coherent, flush_cache_all() may be a no-op, and
+	 * this will fail.
+	 */
 	flush_cache_all();
+
+	/*
+	 * Re-write the TTBR values to point them at the high physical
+	 * alias of the page tables.  We expect __va() will work on
+	 * cpu_get_pgd(), which returns the value of TTBR0.
+	 */
 	cpu_switch_mm(pgd0, &init_mm);
 	cpu_set_ttbr(1, __pa(pgd0) + TTBR1_OFFSET);
+
+	/* Finally flush any stale TLB values. */
 	local_flush_bp_all();
 	local_flush_tlb_all();
 }

Quoted 21 lines · Russell King - ARM Linux

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Thursday 17 July 2014 09:33:42 Russell King - ARM Linux wrote:

No - the problem is that we're running from the page table in question with global mappings, and we need to switch all these mappings, including the ones we're currently using to execute from.

We can't even create a new page table and switch to it because the mappings in question are global mappings.

The only way to do that safely from an architectural point of view would be to turn the MMU off, and drop back to assembly code to change the page tables, and re-enable the MMU. For something as obscure as Marvell's coherency stuff, that's not something I want to see in core code.

Quoted 6 lines

Is this different from what we do in the LPAE version of early_paging_init()? That code already adjusts all the page table entries on a per-platform setting and should be very easy to extend for a modified procinfo->__cpu_mm_mmu_flags, and possibly able to extend for traditional (non-LPAE) page tables without a lot of duplication.

Quoted 8 lines

I'm going to have to profess no knowledge of how the LPAE stuff works. LPAE is something I can't run, and something that I was not involved in its creation. Therefore, I know very little about it and zero experience thereof.

From a glance over that function, I think that is unsafe for all the reasons I've stated. However, I'll pass this over to Will Deacon to comment further on.

Just an FYI, but I've had to check internally to clarify the behaviour around TLB conflict aborts. We're changing live page tables here, but the *only* thing to differ is the output address, which I would hope is ok but need to check to be sure.

The ARM ARM recommends always going via an invalid mapping, but that's not always the most practical thing to do.