| Message ID | 20140528112450.GH3693@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Russell, Thanks a lot for working on a new proposal for this topic. On Wed, 28 May 2014 12:24:50 +0100, Russell King - ARM Linux wrote: > But the way you go about this is totally silly - you effectively end > up enabling all the SMP stuff even though you don't need it (which means > that on a SMP kernel, you end up with a bunch of extra stuff on those > platforms which aren't SMP, just because you want maybe one or two > is_smp() sites to return true. Again, have you looked at the PATCH RFCv1 for this patch series? My first version was doing *exactly* what you suggest: only make the few call sites of is_smp() I was interested in to return true for the specific platforms that needed it. PATCH RFCv2 was generated after a discussion with Catalin and Rob, who suggested that we might want to simply always use the SMP capabilities on SMP-capable processors. > How about this patch for a start - which incidentally fixes two minor > bugs where specifying cachepolicy=<anything> on ARMv6 provokes a warning, > and sets the policy to writeback read-allocate, only to have it overriden > later. The second bug is that we "hoped" that is_smp() reflects the asm > code's page table setup - this patch makes it more explicit by reading > the PMD flags, finding the appropriate entry in the table, and setting > the cache policy to that. I've left the is_smp() check in because we > really do want to detect if something goes awry there. > > However, the effect of this patch is that the C code now follows how > the assembly code sets up the page tables, which means that this is now > controllable via the PMD MMU flags in the assembly procinfo structure. > This means that for the Armada devices which need write-alloc for > coherency, you can specify that in the proc-*.S files - yes, it means > that you need a separate entry. While that would work for Armada 370 and Armada XP, it does not work for Armada 375 and Armada 38x, as far as I can see. Armada 375 and Armada 38x are SMP-capable, but we want to support hardware I/O coherency in a CONFIG_SMP=n configuration (mainly because Armada 380 is single-core). This means that in proc-v7.S, we would have to define that the page tables should use the write-allocate cache policy, and the shareable attribute, and this for *all* Cortex-A9, because from a MIDR point of view, there is no difference between the Cortex-A9 in Armada 375/38x and other Cortex-A9. And there are Cortex-A9, like the Aegis thing from TI that are *not* SMP-capable, so enabling shareable attributes will break. So unless I'm missing something, I don't see how your approach solves the problem for the Cortex-A9 platforms. What would be your suggestion to solve this issue? Thanks, Thomas
On Fri, May 30, 2014 at 10:08:19AM +0200, Thomas Petazzoni wrote: > Russell, > > Thanks a lot for working on a new proposal for this topic. > > On Wed, 28 May 2014 12:24:50 +0100, Russell King - ARM Linux wrote: > > > But the way you go about this is totally silly - you effectively end > > up enabling all the SMP stuff even though you don't need it (which means > > that on a SMP kernel, you end up with a bunch of extra stuff on those > > platforms which aren't SMP, just because you want maybe one or two > > is_smp() sites to return true. > > Again, have you looked at the PATCH RFCv1 for this patch series? My > first version was doing *exactly* what you suggest: only make the few > call sites of is_smp() I was interested in to return true for the > specific platforms that needed it. No, and I don't intend to, because it's probably well buried. > PATCH RFCv2 was generated after a discussion with Catalin and Rob, who > suggested that we might want to simply always use the SMP capabilities > on SMP-capable processors. Frankly, they are both wrong if they suggested the approach in your v2 patch for all the reasons I've raised as objections against v2. > > How about this patch for a start - which incidentally fixes two minor > > bugs where specifying cachepolicy=<anything> on ARMv6 provokes a warning, > > and sets the policy to writeback read-allocate, only to have it overriden > > later. The second bug is that we "hoped" that is_smp() reflects the asm > > code's page table setup - this patch makes it more explicit by reading > > the PMD flags, finding the appropriate entry in the table, and setting > > the cache policy to that. I've left the is_smp() check in because we > > really do want to detect if something goes awry there. > > > > However, the effect of this patch is that the C code now follows how > > the assembly code sets up the page tables, which means that this is now > > controllable via the PMD MMU flags in the assembly procinfo structure. > > This means that for the Armada devices which need write-alloc for > > coherency, you can specify that in the proc-*.S files - yes, it means > > that you need a separate entry. > > While that would work for Armada 370 and Armada XP, it does not work > for Armada 375 and Armada 38x, as far as I can see. Armada 375 and > Armada 38x are SMP-capable, I'm *not* trying to solve everything here, I'm only solving the problem of the C setup code matching what the assembly setup code is doing - which is a long standing minor bug for ARMv6+. > but we want to support hardware I/O > coherency in a CONFIG_SMP=n configuration (mainly because Armada 380 is > single-core). This means that in proc-v7.S, we would have to define > that the page tables should use the write-allocate cache policy, and > the shareable attribute, and this for *all* Cortex-A9, because from a > MIDR point of view, there is no difference between the Cortex-A9 in > Armada 375/38x and other Cortex-A9. And there are Cortex-A9, like the > Aegis thing from TI that are *not* SMP-capable, so enabling shareable > attributes will break. If that's the case, we can't then support IO coherency at all - because to have the asm code setup the page tables without the S bit set, and then have the C code overwrite them with the S bit set is also a violation of the architecture, as Catalin said when he patched the kernel adding this restriction: /* * This restriction is partly to do with the way we boot; it is * unpredictable to have memory mapped using two different sets of * memory attributes (shared, type, and cache attribs). We can not * change these attributes once the initial assembly has setup the * page tables. */ Whatever the assembly code does in respect to these attributes, the C code has to follow. So, my approach is entirely correct and right. What this means is if we can't distinguish Armada's IO coherency at assembly time, then we can't setup the page tables without the sharable bit set in the asm code, and then set them up with the sharable bit set in the C code.
Russell, Thanks again for your feedback, and sorry for the delay in replying, I was busy with other things. On Fri, 30 May 2014 10:20:44 +0100, Russell King - ARM Linux wrote: > > Again, have you looked at the PATCH RFCv1 for this patch series? My > > first version was doing *exactly* what you suggest: only make the few > > call sites of is_smp() I was interested in to return true for the > > specific platforms that needed it. > > No, and I don't intend to, because it's probably well buried. Well, it's not that buried since it only dates back from last month and is available at http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/256258.html. I think looking at the prior versions of a patch series is kind of important to understand the various solutions that have been discussed/explored, especially when comments made on later versions reflect back on things that where already proposed in earlier versions. > > While that would work for Armada 370 and Armada XP, it does not work > > for Armada 375 and Armada 38x, as far as I can see. Armada 375 and > > Armada 38x are SMP-capable, > > I'm *not* trying to solve everything here, I'm only solving the problem > of the C setup code matching what the assembly setup code is doing - > which is a long standing minor bug for ARMv6+. Right, understood. Since we have a special proc_info entry for Armada 370/XP (as they are PJ4B and not Cortex-A based), this means we can ensure the cache policy is write-allocate for both UP and SMP for those processors. > > but we want to support hardware I/O > > coherency in a CONFIG_SMP=n configuration (mainly because Armada 380 is > > single-core). This means that in proc-v7.S, we would have to define > > that the page tables should use the write-allocate cache policy, and > > the shareable attribute, and this for *all* Cortex-A9, because from a > > MIDR point of view, there is no difference between the Cortex-A9 in > > Armada 375/38x and other Cortex-A9. And there are Cortex-A9, like the > > Aegis thing from TI that are *not* SMP-capable, so enabling shareable > > attributes will break. > > If that's the case, we can't then support IO coherency at all - because > to have the asm code setup the page tables without the S bit set, and > then have the C code overwrite them with the S bit set is also a violation > of the architecture, as Catalin said when he patched the kernel adding > this restriction: > > /* > * This restriction is partly to do with the way we boot; it is > * unpredictable to have memory mapped using two different sets of > * memory attributes (shared, type, and cache attribs). We can not > * change these attributes once the initial assembly has setup the > * page tables. > */ > > Whatever the assembly code does in respect to these attributes, the C > code has to follow. So, my approach is entirely correct and right. > > What this means is if we can't distinguish Armada's IO coherency at > assembly time, then we can't setup the page tables without the sharable > bit set in the asm code, and then set them up with the sharable bit set > in the C code. I spoke briefly about this with Albin Tonnerre from ARM, and his feedback is that it should not cause any problem to set the SMP bit and the shareable attribute even on non-SMP Cortex-A9 processors such as the Aegis. If that's correct, then we could do it for all Cortex-A9, without having to know whether what we have is a SoC from Marvell with I/O coherency or any other Cortex-A9 based SoC. Would this be an acceptable solution for you? Thanks! Thomas
On Fri, Jun 13, 2014 at 12:11:21PM +0200, Thomas Petazzoni wrote: > On Fri, 30 May 2014 10:20:44 +0100, Russell King - ARM Linux wrote: > > > Again, have you looked at the PATCH RFCv1 for this patch series? My > > > first version was doing *exactly* what you suggest: only make the few > > > call sites of is_smp() I was interested in to return true for the > > > specific platforms that needed it. > > > > No, and I don't intend to, because it's probably well buried. > > Well, it's not that buried since it only dates back from last month and > is available at > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/256258.html. > I think looking at the prior versions of a patch series is kind of > important to understand the various solutions that have been > discussed/explored, especially when comments made on later versions > reflect back on things that where already proposed in earlier versions. For me, "last month" is as good as buried. With already 2300 messages this month alone, I'm not going to be looking back through last months 8297 messages. If it wasn't sent during the last week, it's buried. > Right, understood. Since we have a special proc_info entry for Armada > 370/XP (as they are PJ4B and not Cortex-A based), this means we can > ensure the cache policy is write-allocate for both UP and SMP for those > processors. Yep. > I spoke briefly about this with Albin Tonnerre from ARM, and his > feedback is that it should not cause any problem to set the SMP bit and > the shareable attribute even on non-SMP Cortex-A9 processors such as > the Aegis. If that's correct, then we could do it for all Cortex-A9, > without having to know whether what we have is a SoC from Marvell with > I/O coherency or any other Cortex-A9 based SoC. Setting the sharable attribute is not really a good idea - implementations are permitted to treat a writeback sharable mapping as uncacheable, which would be a severe performance degredation to single core CPUs. The patches I sent during this thread are now merged into mainline. The setting of the shared bit, and the memory cache policy are now both derived from the proc_info structures, specifically the __cpu_mm_mmu_flags member.
Russell, On Fri, 13 Jun 2014 11:20:19 +0100, Russell King - ARM Linux wrote: > > I spoke briefly about this with Albin Tonnerre from ARM, and his > > feedback is that it should not cause any problem to set the SMP bit and > > the shareable attribute even on non-SMP Cortex-A9 processors such as > > the Aegis. If that's correct, then we could do it for all Cortex-A9, > > without having to know whether what we have is a SoC from Marvell with > > I/O coherency or any other Cortex-A9 based SoC. > > Setting the sharable attribute is not really a good idea - implementations > are permitted to treat a writeback sharable mapping as uncacheable, which > would be a severe performance degredation to single core CPUs. Then maybe we could set the shareable attribute on SMP-capable processors even if CONFIG_SMP is disabled? This way, those single core CPUs that are really not SMP capable continue to run without the shareable attribute, while all SMP-capable processors run with the shareable attribute, regardless of whether the kernel is configured CONFIG_SMP or not. > The patches I sent during this thread are now merged into mainline. The > setting of the shared bit, and the memory cache policy are now both > derived from the proc_info structures, specifically the __cpu_mm_mmu_flags > member. Right. So for Armada 370/XP, I'll update the proc_info structure. We still have to find the right solution/compromise for the Armada 375/38x though. Thanks! Thomas
Hello Russell, On Fri, 13 Jun 2014 13:34:37 +0200, Thomas Petazzoni wrote: > > The patches I sent during this thread are now merged into mainline. The > > setting of the shared bit, and the memory cache policy are now both > > derived from the proc_info structures, specifically the __cpu_mm_mmu_flags > > member. > > Right. So for Armada 370/XP, I'll update the proc_info structure. We > still have to find the right solution/compromise for the Armada 375/38x > though. So, I had a look at that, and there are still I believe a few issues to solve: * Your change that takes the PMD flags set by the assembly code as the reference can easily be used by specifying mm_mmuflags = PMD_SECT_WBWA for Armada 370 and PMD_SECT_WBWA|PMD_SECT_S for Armada XP to get write-allocate on Armada 370 and write-allocate + shareable on Armada XP. However, this adjusts only the PMD flags, and does not adjust the TTB flags: they are still governed only by the ALT_UP/ALT_SMP logic: #define TTB_FLAGS_UP TTB_IRGN_WB|TTB_RGN_OC_WB #define TTB_FLAGS_SMP TTB_IRGN_WBWA|TTB_S|TTB_NOS|TTB_RGN_OC_WBWA [...] ALT_SMP(orr r0, r0, #TTB_FLAGS_SMP) ALT_UP(orr r0, r0, #TTB_FLAGS_UP) Do you have a suggestion on how to handle this? * Regarding setting the SMP bit and TLB broadcast bit for Cortex-A9, I've moved the __v7_ca9mp_setup code outside of the common Cortex-A code so that it does set both bits as soon as it's a SMP-capable Cortex-A9. Something like (untested): /* * Cortex-A9 is handled as a special case, as we want the SMP bit to * be set regardless of whether CONFIG_SMP is enabled or not: as soon * as the processor is SMP capable, we want to enable the SMP bit so * that Cortex-A9 processors that support hardware I/O coherency will * operate properly. */ __v7_ca9mp_setup: mcr p15, 0, r0, c0, c0, 5 @ read MPIDR and r0, r0, #0xc0000000 @ multiprocessing extensions and teq r0, #0x80000000 @ not part of a uniprocessor system? bne __v7_setup mrc p15, 0, r0, c1, c0, 1 tst r0, #(1 << 6) orreq r0, r0, #((1 << 6) | (1 << 10)) mcreq p15, 0, r0, c1, c0, 1 b __v7_setup Would this be acceptable? * Regarding setting the write allocate and shareable attribute in the PMD bits for SMP-capable Cortex-A9, I was hoping to be able to do that from the __v7_ca9mp_setup function above, but in fact this per-processor 'initfunc' is called *after* the page tables are created. So I'm not sure where to put the logic that detects we have an SMP-capable Cortex-A9 and tweak the PMD flags accordingly. Any suggestion? Thanks! Thomas
Russell, Do you have some input/feedback about the below proposals/questions? Thanks a lot! Thomas On Fri, 13 Jun 2014 15:51:39 +0200, Thomas Petazzoni wrote: > Hello Russell, > > On Fri, 13 Jun 2014 13:34:37 +0200, Thomas Petazzoni wrote: > > > > The patches I sent during this thread are now merged into mainline. The > > > setting of the shared bit, and the memory cache policy are now both > > > derived from the proc_info structures, specifically the __cpu_mm_mmu_flags > > > member. > > > > Right. So for Armada 370/XP, I'll update the proc_info structure. We > > still have to find the right solution/compromise for the Armada 375/38x > > though. > > So, I had a look at that, and there are still I believe a few issues to > solve: > > * Your change that takes the PMD flags set by the assembly code as the > reference can easily be used by specifying mm_mmuflags = > PMD_SECT_WBWA for Armada 370 and PMD_SECT_WBWA|PMD_SECT_S for Armada > XP to get write-allocate on Armada 370 and write-allocate + > shareable on Armada XP. > > However, this adjusts only the PMD flags, and does not adjust > the TTB flags: they are still governed only by the ALT_UP/ALT_SMP > logic: > > #define TTB_FLAGS_UP TTB_IRGN_WB|TTB_RGN_OC_WB > #define TTB_FLAGS_SMP TTB_IRGN_WBWA|TTB_S|TTB_NOS|TTB_RGN_OC_WBWA > [...] > ALT_SMP(orr r0, r0, #TTB_FLAGS_SMP) > ALT_UP(orr r0, r0, #TTB_FLAGS_UP) > > Do you have a suggestion on how to handle this? > > * Regarding setting the SMP bit and TLB broadcast bit for Cortex-A9, > I've moved the __v7_ca9mp_setup code outside of the common Cortex-A > code so that it does set both bits as soon as it's a SMP-capable > Cortex-A9. Something like (untested): > > /* > * Cortex-A9 is handled as a special case, as we want the SMP bit to > * be set regardless of whether CONFIG_SMP is enabled or not: as soon > * as the processor is SMP capable, we want to enable the SMP bit so > * that Cortex-A9 processors that support hardware I/O coherency will > * operate properly. > */ > __v7_ca9mp_setup: > mcr p15, 0, r0, c0, c0, 5 @ read MPIDR > and r0, r0, #0xc0000000 @ multiprocessing extensions and > teq r0, #0x80000000 @ not part of a uniprocessor system? > bne __v7_setup > > mrc p15, 0, r0, c1, c0, 1 > tst r0, #(1 << 6) > orreq r0, r0, #((1 << 6) | (1 << 10)) > mcreq p15, 0, r0, c1, c0, 1 > b __v7_setup > > Would this be acceptable? > > * Regarding setting the write allocate and shareable attribute in the > PMD bits for SMP-capable Cortex-A9, I was hoping to be able to do > that from the __v7_ca9mp_setup function above, but in fact this > per-processor 'initfunc' is called *after* the page tables are > created. So I'm not sure where to put the logic that detects we have > an SMP-capable Cortex-A9 and tweak the PMD flags accordingly. Any > suggestion? > > Thanks! > > Thomas
On Tue, Jun 17, 2014 at 10:48:54AM +0200, Thomas Petazzoni wrote: > Russell, > > Do you have some input/feedback about the below proposals/questions? Not at the moment, I'm busy sorting through the 150-odd months-old patches which remain in my tree post-merge window, working through regressions, and trying to sort them out so I can (re-)post them and hopefully get rid of them. Of course, if it didn't take many months to get patches into mainline, then I wouldn't be doing this right now, and I could spend more time thinking more about these sorts of issues, rather than (eg) thinking about how to generate DT binding documentation for drivers which have been merged without that required documentation, which I then need to modify, and then promptly get asked to write that required documentation for.
Hello Russell, On Tue, 17 Jun 2014 12:26:24 +0100, Russell King - ARM Linux wrote: > On Tue, Jun 17, 2014 at 10:48:54AM +0200, Thomas Petazzoni wrote: > > Russell, > > > > Do you have some input/feedback about the below proposals/questions? > > Not at the moment, I'm busy sorting through the 150-odd months-old > patches which remain in my tree post-merge window, working through > regressions, and trying to sort them out so I can (re-)post them and > hopefully get rid of them. > > Of course, if it didn't take many months to get patches into mainline, > then I wouldn't be doing this right now, and I could spend more time > thinking more about these sorts of issues, rather than (eg) thinking > about how to generate DT binding documentation for drivers which have > been merged without that required documentation, which I then need to > modify, and then promptly get asked to write that required documentation > for. I surely understand that it sometimes takes a lot of time to get some feedback from maintainers, I also had similar troubles with the maintainers of certain subsystems. However, I'd really like to see the hardware I/O coherency issue make some progress: it's currently completely broken on Armada 370, causing random corruptions, and the only solution is to be able to properly enable write-allocate. Your patch series makes that partially possible, but there are some remaining problems, which I summarized at http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/263524.html. Thanks a lot for your help and suggestions, Thomas
On Sat, Jun 28, 2014 at 05:39:55PM +0200, Thomas Petazzoni wrote: > Hello Russell, > > On Tue, 17 Jun 2014 12:26:24 +0100, Russell King - ARM Linux wrote: > > On Tue, Jun 17, 2014 at 10:48:54AM +0200, Thomas Petazzoni wrote: > > > Russell, > > > > > > Do you have some input/feedback about the below proposals/questions? > > > > Not at the moment, I'm busy sorting through the 150-odd months-old > > patches which remain in my tree post-merge window, working through > > regressions, and trying to sort them out so I can (re-)post them and > > hopefully get rid of them. > > > > Of course, if it didn't take many months to get patches into mainline, > > then I wouldn't be doing this right now, and I could spend more time > > thinking more about these sorts of issues, rather than (eg) thinking > > about how to generate DT binding documentation for drivers which have > > been merged without that required documentation, which I then need to > > modify, and then promptly get asked to write that required documentation > > for. > > I surely understand that it sometimes takes a lot of time to get some > feedback from maintainers, I also had similar troubles with the > maintainers of certain subsystems. Yes, it takes /months/. The typical way things work is you send a patch set of any size. You get a number of minor comments on it. You address those comments. You get another set of minor comments on a different selection of patches. You address those. So the loop continues, wasting lots of time updating the patches and re-posting them. That means that the maintainers who are trying to push those patches are consumed with this process trying to deal with these other people rather than working on the area that they're supposed to be maintaining. It's a vicious circle where eventually no one is doing any useful work - one which I see kernel development heading headlong towards. I still have a significant quantity of patches (slightly more than when I sent my reply on the 17th of June) with only a relatively small amount of movement towards getting some of them accepted by maintainers. I'm not talking about patches which I've only just created - I'm talking about patches which are more than /six/ months old. The more time this takes, the less time I have to look at core ARM stuff. Then you have problems like the fscked regulator code, which seems to be the only code in the kernel which doesn't allow GPIO number 0 to be specified, not even via DT. It just ignores it. It seems that I'm having to work to fix that code, rather than being able to report it as a bug and have the bug fixed. There is very little to no sharing of these issues, so consequently I'm the one who ends up with _loads_ of crap on my plate to try and solve. > However, I'd really like to see the hardware I/O coherency issue make > some progress: it's currently completely broken on Armada 370, causing > random corruptions, and the only solution is to be able to properly > enable write-allocate. Your patch series makes that partially possible, > but there are some remaining problems, which I summarized at > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/263524.html. Yes, I'm aware that there are still some remaining problems, but I don't have a solution for it yet. What we really need for your case is someway to detect your SoCs in the early assembly code - and right now I don't have any idea how to do that. The reason we need to do that is because we must have the write-allocate cache mode and the shared bits set appropriately from the initial page table setup in the early assembly code, because we can't just overwrite the existing page tables with differing cache attributes without polluting the TLB with those differing cache attributes (which can lead to unpredictable behaviour.) The Armada 37x/38x problem is a very difficult one to solve... really I wish that those spins of the SoC had been chucked in the trash, because working around this properly is going to take a lot of time and effort. That would've been /far/ better for us. I'll also note that it would've been a _lot_ easier to work around had we not had this DT stuff, and kept the machine IDs being passed to the kernel. Such is life though, while DT makes some stuff easier, it makes other stuff absolute hell.
Russell, On Sat, 28 Jun 2014 17:34:13 +0100, Russell King - ARM Linux wrote: > > I surely understand that it sometimes takes a lot of time to get some > > feedback from maintainers, I also had similar troubles with the > > maintainers of certain subsystems. > > Yes, it takes /months/. The typical way things work is you send a > patch set of any size. You get a number of minor comments on it. You > address those comments. You get another set of minor comments on a > different selection of patches. You address those. So the loop > continues, wasting lots of time updating the patches and re-posting > them. Right, but that's the process we all go through. It indeed takes time, but I believe it's the price to pay for a project that big with so many people involved. On the other hand, I agree that some maintainers that no longer have enough time to maintain their subsystem/area should probably try harder to pass the role to some other developers. > That means that the maintainers who are trying to push those patches > are consumed with this process trying to deal with these other people > rather than working on the area that they're supposed to be maintaining. True, but as I said earlier, the process you're going through is the one every kernel developer is going through, and I'm not sure why the process should be different for a kernel maintainer or a random kernel developer. > I still have a significant quantity of patches (slightly more than > when I sent my reply on the 17th of June) with only a relatively small > amount of movement towards getting some of them accepted by maintainers. > > I'm not talking about patches which I've only just created - I'm talking > about patches which are more than /six/ months old. I'd say that after a few months without any useful feedback or answer, patches should get merged by the maintainer. Inaction is terrible, as it de-motivates developers. But I'm sure not everybody would agree with such a proposal :) > > However, I'd really like to see the hardware I/O coherency issue make > > some progress: it's currently completely broken on Armada 370, causing > > random corruptions, and the only solution is to be able to properly > > enable write-allocate. Your patch series makes that partially possible, > > but there are some remaining problems, which I summarized at > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/263524.html. > > Yes, I'm aware that there are still some remaining problems, but I don't > have a solution for it yet. > > What we really need for your case is someway to detect your SoCs in the > early assembly code - and right now I don't have any idea how to do that. Well, I'd like to *first* solve the Armada 370 issue, and the Armada 370 is using a specific ARM core, so it can definitely be detected early on in the assembly code. So for the Armada 370 most of the problems are solved thanks to your previous patch, the only remaining problem is the TTB_FLAGS problem I highlighted in http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/263524.html. It's entirely solvable in the early assembly code, I just need your input on what you think is the right direction to solve this: currently TTB_FLAGS are defined globally, with one value for UP and one value for SMP. I would need to make those TTB_FLAGS part of the proc_info structure, so that Armada 370 and Armada XP (which use special ARM cores and therefore have separate proc_info structures). Would you agree with such a change? Of course this is only needed if the TTB_FLAGS need to match the cache policy used in the PMD_FLAGS, but I believe Catalin once said that it was the case. Maybe Catalin, Will or Albin from ARM could confirm/infirm? > The reason we need to do that is because we must have the write-allocate > cache mode and the shared bits set appropriately from the initial page > table setup in the early assembly code, because we can't just overwrite > the existing page tables with differing cache attributes without polluting > the TLB with those differing cache attributes (which can lead to > unpredictable behaviour.) > > The Armada 37x/38x problem is a very difficult one to solve... really > I wish that those spins of the SoC had been chucked in the trash, because > working around this properly is going to take a lot of time and effort. > That would've been /far/ better for us. What about simply making write-allocate + shared attribute the default on all SMP-capable Cortex-A9 ? Thanks, Thomas
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index df21f9f98945..aa516bc4ca30 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -72,6 +72,7 @@ static int __init fpe_setup(char *line) __setup("fpe=", fpe_setup); #endif +extern void init_default_cache_policy(unsigned long); extern void paging_init(const struct machine_desc *desc); extern void early_paging_init(const struct machine_desc *, struct proc_info_list *); @@ -603,7 +604,9 @@ static void __init setup_processor(void) #ifndef CONFIG_ARM_THUMB elf_hwcap &= ~(HWCAP_THUMB | HWCAP_IDIVT); #endif - +#ifdef CONFIG_CPU_CP15 + init_default_cache_policy(list->__cpu_mm_mmu_flags); +#endif erratum_a15_798181_init(); feat_v6_fixup(); diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index a476051c0567..704ff018e67b 100644 --- a/arch/arm/mm/mmu.c +++ b/arch/arm/mm/mmu.c @@ -118,6 +118,29 @@ static struct cachepolicy cache_policies[] __initdata = { }; #ifdef CONFIG_CPU_CP15 +/* + * Initialise the cache_policy variable with the initial state specified + * via the "pmd" value. This is used to ensure that on ARMv6 and later, + * the C code sets the page tables up with the same policy as the head + * assembly code, which avoids an illegal state where the TLBs can get + * confused. See comments in early_cachepolicy() for more information. + */ +void __init init_default_cache_policy(unsigned long pmd) +{ + int i; + + pmd &= PMD_SECT_TEX(1) | PMD_SECT_BUFFERABLE | PMD_SECT_CACHEABLE; + + for (i = 0; i < ARRAY_SIZE(cache_policies); i++) + if (cache_policies[i].pmd == pmd) { + cachepolicy = i; + break; + } + + if (i == ARRAY_SIZE(cache_policies)) + pr_err("ERROR: could not find cache policy\n"); +} + unsigned long __init __clear_cr(unsigned long mask) { cr_alignment = cr_alignment & ~mask; @@ -125,27 +148,26 @@ unsigned long __init __clear_cr(unsigned long mask) } /* - * These are useful for identifying cache coherency - * problems by allowing the cache or the cache and - * writebuffer to be turned off. (Note: the write - * buffer should not be on and the cache off). + * These are useful for identifying cache coherency problems by allowing + * the cache or the cache and writebuffer to be turned off. (Note: the + * write buffer should not be on and the cache off). */ static int __init early_cachepolicy(char *p) { - unsigned long cr = get_cr(); - int i; + int i, selected = -1; for (i = 0; i < ARRAY_SIZE(cache_policies); i++) { int len = strlen(cache_policies[i].policy); if (memcmp(p, cache_policies[i].policy, len) == 0) { - cachepolicy = i; - cr = __clear_cr(cache_policies[i].cr_mask); + selected = i; break; } } - if (i == ARRAY_SIZE(cache_policies)) - printk(KERN_ERR "ERROR: unknown or unsupported cache policy\n"); + + if (selected == -1) + pr_err("ERROR: unknown or unsupported cache policy\n"); + /* * This restriction is partly to do with the way we boot; it is * unpredictable to have memory mapped using two different sets of @@ -153,12 +175,18 @@ static int __init early_cachepolicy(char *p) * change these attributes once the initial assembly has setup the * page tables. */ - if (cpu_architecture() >= CPU_ARCH_ARMv6) { - printk(KERN_WARNING "Only cachepolicy=writeback supported on ARMv6 and later\n"); - cachepolicy = CPOLICY_WRITEBACK; + if (cpu_architecture() >= CPU_ARCH_ARMv6 && selected != cachepolicy) { + pr_warn("Only cachepolicy=%s supported on ARMv6 and later\n", + cache_policies[cachepolicy].policy); + return 0; + } + + if (selected != cachepolicy) { + unsigned long cr = __clear_cr(cache_policies[selected].cr_mask); + cachepolicy = selected; + flush_cache_all(); + set_cr(cr); } - flush_cache_all(); - set_cr(cr); return 0; } early_param("cachepolicy", early_cachepolicy); @@ -392,8 +420,11 @@ static void __init build_mem_type_table(void) cachepolicy = CPOLICY_WRITEBACK; ecc_mask = 0; } - if (is_smp()) + + if (is_smp() && cachepolicy != CPOLICY_WRITEALLOC) { + pr_warn("Forcing write-allocate cache policy for SMP\n"); cachepolicy = CPOLICY_WRITEALLOC; + } /* * Strip out features not present on earlier architectures.