* Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-15 23:19 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-15 23:19 UTC (permalink / raw) To: linux-arm-kernel Hi, we are having a problem with atomic accesses in pstore on some ARM CPUs (specifically rk3288 and rk3399). With those chips, atomic accesses fail with both pgprot_noncached and pgprot_writecombine memory. Atomic accesses do work when selecting PAGE_KERNEL protection. Debugging on rk3399 shows the following crash. [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 [ 0.926838] Hardware name: Google Kevin (DT) [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: ffffffc0edf 7c000 [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c [ 0.945811] LR is at 0x1 The "solution" for this problem in various Chrome OS releases is to disable atomic accesses in pstore entirely, which seems to be a bit brute-force. Question is what a proper upstream-acceptable solution might be. Introduce another memory type to select PAGE_KERNEL ? Is there some means to determine if atomic operations are supported with a given protection mask, maybe ? Anything else ? Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-15 23:19 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-15 23:19 UTC (permalink / raw) To: Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson Cc: linux-kernel, linux-arm-kernel Hi, we are having a problem with atomic accesses in pstore on some ARM CPUs (specifically rk3288 and rk3399). With those chips, atomic accesses fail with both pgprot_noncached and pgprot_writecombine memory. Atomic accesses do work when selecting PAGE_KERNEL protection. Debugging on rk3399 shows the following crash. [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 [ 0.926838] Hardware name: Google Kevin (DT) [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: ffffffc0edf 7c000 [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c [ 0.945811] LR is at 0x1 The "solution" for this problem in various Chrome OS releases is to disable atomic accesses in pstore entirely, which seems to be a bit brute-force. Question is what a proper upstream-acceptable solution might be. Introduce another memory type to select PAGE_KERNEL ? Is there some means to determine if atomic operations are supported with a given protection mask, maybe ? Anything else ? Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-15 23:19 ` Guenter Roeck @ 2016-08-16 10:32 ` Robin Murphy -1 siblings, 0 replies; 31+ messages in thread From: Robin Murphy @ 2016-08-16 10:32 UTC (permalink / raw) To: linux-arm-kernel Hi Guenter, On 16/08/16 00:19, Guenter Roeck wrote: > Hi, > > we are having a problem with atomic accesses in pstore on some ARM > CPUs (specifically rk3288 and rk3399). With those chips, atomic > accesses fail with both pgprot_noncached and pgprot_writecombine > memory. Atomic accesses do work when selecting PAGE_KERNEL protection. What's the pstore backed by? I'm guessing it's not normal DRAM. > Debugging on rk3399 shows the following crash. > > [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError > [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 > [ 0.926838] Hardware name: Google Kevin (DT) > [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: > ffffffc0edf 7c000 > [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c > [ 0.945811] LR is at 0x1 > > The "solution" for this problem in various Chrome OS releases is to > disable atomic accesses in pstore entirely, which seems to be a bit > brute-force. Question is what a proper upstream-acceptable solution >From that, it sounds like the endpoint doesn't support exclusive accesses. I imagine you get away with it through a PAGE_KERNEL mapping by virtue of being write-back cacheable, such that the exclusives end up being handled by the local monitor at L1 and don't go out to memory, but I'm not sure even that's necessarily reliable. In general terms, not doing atomic accesses is probably the only 100% safe solution. Robin. > might be. Introduce another memory type to select PAGE_KERNEL ? Is > there some means to determine if atomic operations are supported with > a given protection mask, maybe ? Anything else ? > > Thanks, > Guenter > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 10:32 ` Robin Murphy 0 siblings, 0 replies; 31+ messages in thread From: Robin Murphy @ 2016-08-16 10:32 UTC (permalink / raw) To: Guenter Roeck, Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson Cc: linux-kernel, linux-arm-kernel, Will Deacon Hi Guenter, On 16/08/16 00:19, Guenter Roeck wrote: > Hi, > > we are having a problem with atomic accesses in pstore on some ARM > CPUs (specifically rk3288 and rk3399). With those chips, atomic > accesses fail with both pgprot_noncached and pgprot_writecombine > memory. Atomic accesses do work when selecting PAGE_KERNEL protection. What's the pstore backed by? I'm guessing it's not normal DRAM. > Debugging on rk3399 shows the following crash. > > [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError > [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 > [ 0.926838] Hardware name: Google Kevin (DT) > [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: > ffffffc0edf 7c000 > [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c > [ 0.945811] LR is at 0x1 > > The "solution" for this problem in various Chrome OS releases is to > disable atomic accesses in pstore entirely, which seems to be a bit > brute-force. Question is what a proper upstream-acceptable solution >From that, it sounds like the endpoint doesn't support exclusive accesses. I imagine you get away with it through a PAGE_KERNEL mapping by virtue of being write-back cacheable, such that the exclusives end up being handled by the local monitor at L1 and don't go out to memory, but I'm not sure even that's necessarily reliable. In general terms, not doing atomic accesses is probably the only 100% safe solution. Robin. > might be. Introduce another memory type to select PAGE_KERNEL ? Is > there some means to determine if atomic operations are supported with > a given protection mask, maybe ? Anything else ? > > Thanks, > Guenter > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 10:32 ` Robin Murphy @ 2016-08-16 10:45 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-08-16 10:45 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 11:32:04AM +0100, Robin Murphy wrote: > On 16/08/16 00:19, Guenter Roeck wrote: > > we are having a problem with atomic accesses in pstore on some ARM > > CPUs (specifically rk3288 and rk3399). With those chips, atomic > > accesses fail with both pgprot_noncached and pgprot_writecombine > > memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > What's the pstore backed by? I'm guessing it's not normal DRAM. Regardless, pgprot_noncached and pgprot_writecombine map to Device-nGnRnE and Normal-non-cacheable respectively, and so it's IMP DEP whether or not exclusives will work there. > > Debugging on rk3399 shows the following crash. > > > > [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError > > [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 > > [ 0.926838] Hardware name: Google Kevin (DT) > > [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: > > ffffffc0edf 7c000 > > [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c > > [ 0.945811] LR is at 0x1 > > > > The "solution" for this problem in various Chrome OS releases is to > > disable atomic accesses in pstore entirely, which seems to be a bit > > brute-force. Question is what a proper upstream-acceptable solution Why do you require atomics to the pstore? If you need to serialise updates from coherent observers (e.g. CPUs), is it acceptable to use a lock in normal memory instead? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 10:45 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-08-16 10:45 UTC (permalink / raw) To: Robin Murphy Cc: Guenter Roeck, Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson, linux-kernel, linux-arm-kernel On Tue, Aug 16, 2016 at 11:32:04AM +0100, Robin Murphy wrote: > On 16/08/16 00:19, Guenter Roeck wrote: > > we are having a problem with atomic accesses in pstore on some ARM > > CPUs (specifically rk3288 and rk3399). With those chips, atomic > > accesses fail with both pgprot_noncached and pgprot_writecombine > > memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > What's the pstore backed by? I'm guessing it's not normal DRAM. Regardless, pgprot_noncached and pgprot_writecombine map to Device-nGnRnE and Normal-non-cacheable respectively, and so it's IMP DEP whether or not exclusives will work there. > > Debugging on rk3399 shows the following crash. > > > > [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError > > [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 > > [ 0.926838] Hardware name: Google Kevin (DT) > > [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: > > ffffffc0edf 7c000 > > [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c > > [ 0.945811] LR is at 0x1 > > > > The "solution" for this problem in various Chrome OS releases is to > > disable atomic accesses in pstore entirely, which seems to be a bit > > brute-force. Question is what a proper upstream-acceptable solution Why do you require atomics to the pstore? If you need to serialise updates from coherent observers (e.g. CPUs), is it acceptable to use a lock in normal memory instead? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 10:45 ` Will Deacon @ 2016-08-16 13:21 ` Guenter Roeck -1 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 13:21 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 3:45 AM, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Aug 16, 2016 at 11:32:04AM +0100, Robin Murphy wrote: >> On 16/08/16 00:19, Guenter Roeck wrote: >> > we are having a problem with atomic accesses in pstore on some ARM >> > CPUs (specifically rk3288 and rk3399). With those chips, atomic >> > accesses fail with both pgprot_noncached and pgprot_writecombine >> > memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >> >> What's the pstore backed by? I'm guessing it's not normal DRAM. > > Regardless, pgprot_noncached and pgprot_writecombine map to Device-nGnRnE > and Normal-non-cacheable respectively, and so it's IMP DEP whether or not > exclusives will work there. > >> > Debugging on rk3399 shows the following crash. >> > >> > [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError >> > [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 >> > [ 0.926838] Hardware name: Google Kevin (DT) >> > [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: >> > ffffffc0edf 7c000 >> > [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c >> > [ 0.945811] LR is at 0x1 >> > >> > The "solution" for this problem in various Chrome OS releases is to >> > disable atomic accesses in pstore entirely, which seems to be a bit >> > brute-force. Question is what a proper upstream-acceptable solution > > Why do you require atomics to the pstore? If you need to serialise updates > from coherent observers (e.g. CPUs), is it acceptable to use a lock in > normal memory instead? > Sure, this is just how pstore works today, and I would prefer not having to hack upstream code. This is normal DRAM. The initialization code does the following. if (pfn_valid(start >> PAGE_SHIFT)) prz->vaddr = persistent_ram_vmap(start, size, memtype); else prz->vaddr = persistent_ram_iomap(start, size, memtype); persistent_ram_vmap() uses atomics, persistent_ram_iomap() uses spinlocks. For normal DRAM, pfn_valid() returns true. Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 13:21 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 13:21 UTC (permalink / raw) To: Will Deacon Cc: Robin Murphy, Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson, linux-kernel, linux-arm-kernel On Tue, Aug 16, 2016 at 3:45 AM, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Aug 16, 2016 at 11:32:04AM +0100, Robin Murphy wrote: >> On 16/08/16 00:19, Guenter Roeck wrote: >> > we are having a problem with atomic accesses in pstore on some ARM >> > CPUs (specifically rk3288 and rk3399). With those chips, atomic >> > accesses fail with both pgprot_noncached and pgprot_writecombine >> > memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >> >> What's the pstore backed by? I'm guessing it's not normal DRAM. > > Regardless, pgprot_noncached and pgprot_writecombine map to Device-nGnRnE > and Normal-non-cacheable respectively, and so it's IMP DEP whether or not > exclusives will work there. > >> > Debugging on rk3399 shows the following crash. >> > >> > [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError >> > [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 >> > [ 0.926838] Hardware name: Google Kevin (DT) >> > [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: >> > ffffffc0edf 7c000 >> > [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c >> > [ 0.945811] LR is at 0x1 >> > >> > The "solution" for this problem in various Chrome OS releases is to >> > disable atomic accesses in pstore entirely, which seems to be a bit >> > brute-force. Question is what a proper upstream-acceptable solution > > Why do you require atomics to the pstore? If you need to serialise updates > from coherent observers (e.g. CPUs), is it acceptable to use a lock in > normal memory instead? > Sure, this is just how pstore works today, and I would prefer not having to hack upstream code. This is normal DRAM. The initialization code does the following. if (pfn_valid(start >> PAGE_SHIFT)) prz->vaddr = persistent_ram_vmap(start, size, memtype); else prz->vaddr = persistent_ram_iomap(start, size, memtype); persistent_ram_vmap() uses atomics, persistent_ram_iomap() uses spinlocks. For normal DRAM, pfn_valid() returns true. Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 10:32 ` Robin Murphy @ 2016-08-16 13:14 ` Guenter Roeck -1 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 13:14 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > Hi Guenter, > > On 16/08/16 00:19, Guenter Roeck wrote: >> Hi, >> >> we are having a problem with atomic accesses in pstore on some ARM >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >> accesses fail with both pgprot_noncached and pgprot_writecombine >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > What's the pstore backed by? I'm guessing it's not normal DRAM. > it is normal DRAM. >> Debugging on rk3399 shows the following crash. >> >> [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError >> [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 >> [ 0.926838] Hardware name: Google Kevin (DT) >> [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: >> ffffffc0edf 7c000 >> [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c >> [ 0.945811] LR is at 0x1 >> >> The "solution" for this problem in various Chrome OS releases is to >> disable atomic accesses in pstore entirely, which seems to be a bit >> brute-force. Question is what a proper upstream-acceptable solution > > From that, it sounds like the endpoint doesn't support exclusive > accesses. I imagine you get away with it through a PAGE_KERNEL mapping > by virtue of being write-back cacheable, such that the exclusives end up > being handled by the local monitor at L1 and don't go out to memory, but > I'm not sure even that's necessarily reliable. > > In general terms, not doing atomic accesses is probably the only 100% > safe solution. > Not sure I understand. Atomics work for the rest of the DRAM. Can't I just use the same access protection ? Thanks, Guenter > Robin. > >> might be. Introduce another memory type to select PAGE_KERNEL ? Is >> there some means to determine if atomic operations are supported with >> a given protection mask, maybe ? Anything else ? >> >> Thanks, >> Guenter >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 13:14 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 13:14 UTC (permalink / raw) To: Robin Murphy Cc: Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson, linux-kernel, linux-arm-kernel, Will Deacon On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > Hi Guenter, > > On 16/08/16 00:19, Guenter Roeck wrote: >> Hi, >> >> we are having a problem with atomic accesses in pstore on some ARM >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >> accesses fail with both pgprot_noncached and pgprot_writecombine >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > What's the pstore backed by? I'm guessing it's not normal DRAM. > it is normal DRAM. >> Debugging on rk3399 shows the following crash. >> >> [ 0.912669] Bad mode in Error handler detected, code 0xbf000002 -- SError >> [ 0.920140] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.4.14 #389 >> [ 0.926838] Hardware name: Google Kevin (DT) >> [ 0.931533] task: ffffffc0edfe0000 ti: ffffffc0edf7c000 task.ti: >> ffffffc0edf 7c000 >> [ 0.939780] PC is at __ll_sc___cmpxchg_case_mb_4+0x2c/0x5c >> [ 0.945811] LR is at 0x1 >> >> The "solution" for this problem in various Chrome OS releases is to >> disable atomic accesses in pstore entirely, which seems to be a bit >> brute-force. Question is what a proper upstream-acceptable solution > > From that, it sounds like the endpoint doesn't support exclusive > accesses. I imagine you get away with it through a PAGE_KERNEL mapping > by virtue of being write-back cacheable, such that the exclusives end up > being handled by the local monitor at L1 and don't go out to memory, but > I'm not sure even that's necessarily reliable. > > In general terms, not doing atomic accesses is probably the only 100% > safe solution. > Not sure I understand. Atomics work for the rest of the DRAM. Can't I just use the same access protection ? Thanks, Guenter > Robin. > >> might be. Introduce another memory type to select PAGE_KERNEL ? Is >> there some means to determine if atomic operations are supported with >> a given protection mask, maybe ? Anything else ? >> >> Thanks, >> Guenter >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 13:14 ` Guenter Roeck @ 2016-08-16 13:21 ` Will Deacon -1 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-08-16 13:21 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: > On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > > On 16/08/16 00:19, Guenter Roeck wrote: > >> we are having a problem with atomic accesses in pstore on some ARM > >> CPUs (specifically rk3288 and rk3399). With those chips, atomic > >> accesses fail with both pgprot_noncached and pgprot_writecombine > >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > > > What's the pstore backed by? I'm guessing it's not normal DRAM. > > > > it is normal DRAM. In which case, why does it need to be mapped with weird attributes? Is there an alias in the linear map you can use? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 13:21 ` Will Deacon 0 siblings, 0 replies; 31+ messages in thread From: Will Deacon @ 2016-08-16 13:21 UTC (permalink / raw) To: Guenter Roeck Cc: Robin Murphy, Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson, linux-kernel, linux-arm-kernel On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: > On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > > On 16/08/16 00:19, Guenter Roeck wrote: > >> we are having a problem with atomic accesses in pstore on some ARM > >> CPUs (specifically rk3288 and rk3399). With those chips, atomic > >> accesses fail with both pgprot_noncached and pgprot_writecombine > >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > > > What's the pstore backed by? I'm guessing it's not normal DRAM. > > > > it is normal DRAM. In which case, why does it need to be mapped with weird attributes? Is there an alias in the linear map you can use? Will ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 13:21 ` Will Deacon @ 2016-08-16 15:02 ` Guenter Roeck -1 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 15:02 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >> > On 16/08/16 00:19, Guenter Roeck wrote: >> >> we are having a problem with atomic accesses in pstore on some ARM >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >> > >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >> > >> >> it is normal DRAM. > > In which case, why does it need to be mapped with weird attributes? > Is there an alias in the linear map you can use? > I don't really _want_ to do anything besides using pstore as-is, or, in other words, to have the upstream kernel work with the affected systems. The current pstore code runs the following code for memory with pfn_valid() = true. if (memtype) prot = pgprot_noncached(PAGE_KERNEL); else prot = pgprot_writecombine(PAGE_KERNEL); ... vaddr = vmap(pages, page_count, VM_MAP, prot); It then uses the memory pointed to by vaddr for atomic operations. In my case, both protection options don't work. Everything works fine (or at least doesn't create an exception) if I use vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); instead. Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 15:02 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 15:02 UTC (permalink / raw) To: Will Deacon Cc: Robin Murphy, Kees Cook, Jeffy Chen, Colin Cross, Tony Luck, Douglas Anderson, linux-kernel, linux-arm-kernel On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >> > On 16/08/16 00:19, Guenter Roeck wrote: >> >> we are having a problem with atomic accesses in pstore on some ARM >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >> > >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >> > >> >> it is normal DRAM. > > In which case, why does it need to be mapped with weird attributes? > Is there an alias in the linear map you can use? > I don't really _want_ to do anything besides using pstore as-is, or, in other words, to have the upstream kernel work with the affected systems. The current pstore code runs the following code for memory with pfn_valid() = true. if (memtype) prot = pgprot_noncached(PAGE_KERNEL); else prot = pgprot_writecombine(PAGE_KERNEL); ... vaddr = vmap(pages, page_count, VM_MAP, prot); It then uses the memory pointed to by vaddr for atomic operations. In my case, both protection options don't work. Everything works fine (or at least doesn't create an exception) if I use vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); instead. Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 15:02 ` Guenter Roeck @ 2016-08-15 22:15 ` Mark Rutland -1 siblings, 0 replies; 31+ messages in thread From: Mark Rutland @ 2016-08-15 22:15 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: > On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: > >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > >> > On 16/08/16 00:19, Guenter Roeck wrote: > >> >> we are having a problem with atomic accesses in pstore on some ARM > >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic > >> >> accesses fail with both pgprot_noncached and pgprot_writecombine > >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > >> > > >> > What's the pstore backed by? I'm guessing it's not normal DRAM. > >> > > >> > >> it is normal DRAM. > > > > In which case, why does it need to be mapped with weird attributes? > > Is there an alias in the linear map you can use? > > > > I don't really _want_ to do anything besides using pstore as-is, or, > in other words, to have the upstream kernel work with the affected > systems. > > The current pstore code runs the following code for memory with > pfn_valid() = true. > > if (memtype) > prot = pgprot_noncached(PAGE_KERNEL); > else > prot = pgprot_writecombine(PAGE_KERNEL); > ... > vaddr = vmap(pages, page_count, VM_MAP, prot); > > It then uses the memory pointed to by vaddr for atomic operations. This means that the generic ramoops / pstore code is making non-portable assumptions about memory types. So _something_ has to happen to that code. > In my case, both protection options don't work. Everything works fine > (or at least doesn't create an exception) if I use > vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); > instead. Architecturally, that will give you a memory type to which we can safely use atomics on. It would be nice to know why the ramoops/pstore code must use atomics, and exactly what it's trying to achieve (i.e. is this just for serialisation, or an attempt to ensure persistence). Depending on that, it may be possible to fix things more generically by using memremap by default, for example, and only allowing uncached mappings on those architectures which support them. Thanks, Mark. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-15 22:15 ` Mark Rutland 0 siblings, 0 replies; 31+ messages in thread From: Mark Rutland @ 2016-08-15 22:15 UTC (permalink / raw) To: Guenter Roeck Cc: Will Deacon, Tony Luck, Kees Cook, Jeffy Chen, Douglas Anderson, linux-kernel, Colin Cross, Robin Murphy, linux-arm-kernel On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: > On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: > > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: > >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > >> > On 16/08/16 00:19, Guenter Roeck wrote: > >> >> we are having a problem with atomic accesses in pstore on some ARM > >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic > >> >> accesses fail with both pgprot_noncached and pgprot_writecombine > >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > >> > > >> > What's the pstore backed by? I'm guessing it's not normal DRAM. > >> > > >> > >> it is normal DRAM. > > > > In which case, why does it need to be mapped with weird attributes? > > Is there an alias in the linear map you can use? > > > > I don't really _want_ to do anything besides using pstore as-is, or, > in other words, to have the upstream kernel work with the affected > systems. > > The current pstore code runs the following code for memory with > pfn_valid() = true. > > if (memtype) > prot = pgprot_noncached(PAGE_KERNEL); > else > prot = pgprot_writecombine(PAGE_KERNEL); > ... > vaddr = vmap(pages, page_count, VM_MAP, prot); > > It then uses the memory pointed to by vaddr for atomic operations. This means that the generic ramoops / pstore code is making non-portable assumptions about memory types. So _something_ has to happen to that code. > In my case, both protection options don't work. Everything works fine > (or at least doesn't create an exception) if I use > vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); > instead. Architecturally, that will give you a memory type to which we can safely use atomics on. It would be nice to know why the ramoops/pstore code must use atomics, and exactly what it's trying to achieve (i.e. is this just for serialisation, or an attempt to ensure persistence). Depending on that, it may be possible to fix things more generically by using memremap by default, for example, and only allowing uncached mappings on those architectures which support them. Thanks, Mark. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-15 22:15 ` Mark Rutland @ 2016-08-16 17:35 ` Colin Cross -1 siblings, 0 replies; 31+ messages in thread From: Colin Cross @ 2016-08-16 17:35 UTC (permalink / raw) To: linux-arm-kernel On Mon, Aug 15, 2016 at 3:15 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: >> On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: >> > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >> >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >> >> > On 16/08/16 00:19, Guenter Roeck wrote: >> >> >> we are having a problem with atomic accesses in pstore on some ARM >> >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >> >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >> >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >> >> > >> >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >> >> > >> >> >> >> it is normal DRAM. >> > >> > In which case, why does it need to be mapped with weird attributes? >> > Is there an alias in the linear map you can use? >> > >> >> I don't really _want_ to do anything besides using pstore as-is, or, >> in other words, to have the upstream kernel work with the affected >> systems. >> >> The current pstore code runs the following code for memory with >> pfn_valid() = true. >> >> if (memtype) >> prot = pgprot_noncached(PAGE_KERNEL); >> else >> prot = pgprot_writecombine(PAGE_KERNEL); >> ... >> vaddr = vmap(pages, page_count, VM_MAP, prot); >> >> It then uses the memory pointed to by vaddr for atomic operations. > > This means that the generic ramoops / pstore code is making non-portable > assumptions about memory types. > > So _something_ has to happen to that code. > >> In my case, both protection options don't work. Everything works fine >> (or at least doesn't create an exception) if I use >> vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); >> instead. > > Architecturally, that will give you a memory type to which we can safely use > atomics on. > > It would be nice to know why the ramoops/pstore code must use atomics, and > exactly what it's trying to achieve (i.e. is this just for serialisation, or an > attempt to ensure persistence). > > Depending on that, it may be possible to fix things more generically by using > memremap by default, for example, and only allowing uncached mappings on those > architectures which support them. persistent_ram uses atomic ops in uncached memory to store the start and end positions in the ringbuffer so that the state of the ringbuffer will be valid if the kernel crashes at any time. This was inherited from Android's ram_console implementation, and worked through armv7. It has been causing more and more problems recently, see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: Allow optional mapping with pgprot_noncached) and 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by using write-combine mappings). Maybe it should be replaced with a spinlock in normal ram protecting writes to the uncached region. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 17:35 ` Colin Cross 0 siblings, 0 replies; 31+ messages in thread From: Colin Cross @ 2016-08-16 17:35 UTC (permalink / raw) To: Mark Rutland Cc: Guenter Roeck, Will Deacon, Tony Luck, Kees Cook, Jeffy Chen, Douglas Anderson, linux-kernel, Robin Murphy, linux-arm-kernel@lists.infradead.org On Mon, Aug 15, 2016 at 3:15 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: >> On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: >> > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >> >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >> >> > On 16/08/16 00:19, Guenter Roeck wrote: >> >> >> we are having a problem with atomic accesses in pstore on some ARM >> >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >> >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >> >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >> >> > >> >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >> >> > >> >> >> >> it is normal DRAM. >> > >> > In which case, why does it need to be mapped with weird attributes? >> > Is there an alias in the linear map you can use? >> > >> >> I don't really _want_ to do anything besides using pstore as-is, or, >> in other words, to have the upstream kernel work with the affected >> systems. >> >> The current pstore code runs the following code for memory with >> pfn_valid() = true. >> >> if (memtype) >> prot = pgprot_noncached(PAGE_KERNEL); >> else >> prot = pgprot_writecombine(PAGE_KERNEL); >> ... >> vaddr = vmap(pages, page_count, VM_MAP, prot); >> >> It then uses the memory pointed to by vaddr for atomic operations. > > This means that the generic ramoops / pstore code is making non-portable > assumptions about memory types. > > So _something_ has to happen to that code. > >> In my case, both protection options don't work. Everything works fine >> (or at least doesn't create an exception) if I use >> vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); >> instead. > > Architecturally, that will give you a memory type to which we can safely use > atomics on. > > It would be nice to know why the ramoops/pstore code must use atomics, and > exactly what it's trying to achieve (i.e. is this just for serialisation, or an > attempt to ensure persistence). > > Depending on that, it may be possible to fix things more generically by using > memremap by default, for example, and only allowing uncached mappings on those > architectures which support them. persistent_ram uses atomic ops in uncached memory to store the start and end positions in the ringbuffer so that the state of the ringbuffer will be valid if the kernel crashes at any time. This was inherited from Android's ram_console implementation, and worked through armv7. It has been causing more and more problems recently, see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: Allow optional mapping with pgprot_noncached) and 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by using write-combine mappings). Maybe it should be replaced with a spinlock in normal ram protecting writes to the uncached region. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 17:35 ` Colin Cross @ 2016-08-16 20:26 ` Guenter Roeck -1 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 20:26 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 10:35 AM, Colin Cross <ccross@android.com> wrote: > On Mon, Aug 15, 2016 at 3:15 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: >>> On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: >>> > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >>> >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>> >> > On 16/08/16 00:19, Guenter Roeck wrote: >>> >> >> we are having a problem with atomic accesses in pstore on some ARM >>> >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >>> >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >>> >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >>> >> > >>> >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >>> >> > >>> >> >>> >> it is normal DRAM. >>> > >>> > In which case, why does it need to be mapped with weird attributes? >>> > Is there an alias in the linear map you can use? >>> > >>> >>> I don't really _want_ to do anything besides using pstore as-is, or, >>> in other words, to have the upstream kernel work with the affected >>> systems. >>> >>> The current pstore code runs the following code for memory with >>> pfn_valid() = true. >>> >>> if (memtype) >>> prot = pgprot_noncached(PAGE_KERNEL); >>> else >>> prot = pgprot_writecombine(PAGE_KERNEL); >>> ... >>> vaddr = vmap(pages, page_count, VM_MAP, prot); >>> >>> It then uses the memory pointed to by vaddr for atomic operations. >> >> This means that the generic ramoops / pstore code is making non-portable >> assumptions about memory types. >> >> So _something_ has to happen to that code. >> >>> In my case, both protection options don't work. Everything works fine >>> (or at least doesn't create an exception) if I use >>> vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); >>> instead. >> >> Architecturally, that will give you a memory type to which we can safely use >> atomics on. >> >> It would be nice to know why the ramoops/pstore code must use atomics, and >> exactly what it's trying to achieve (i.e. is this just for serialisation, or an >> attempt to ensure persistence). >> >> Depending on that, it may be possible to fix things more generically by using >> memremap by default, for example, and only allowing uncached mappings on those >> architectures which support them. > > persistent_ram uses atomic ops in uncached memory to store the start > and end positions in the ringbuffer so that the state of the > ringbuffer will be valid if the kernel crashes at any time. This was > inherited from Android's ram_console implementation, and worked > through armv7. It has been causing more and more problems recently, > see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: > Allow optional mapping with pgprot_noncached) and > 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by > using write-combine mappings). > > Maybe it should be replaced with a spinlock in normal ram protecting > writes to the uncached region. The necessary functions already exist, and are used for memory mapped with ioremap() / ioremap_wc(). They were introduced with commit 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped regions"), and the description in that patch sounds quite similar to the current problem. Given that, would it be acceptable to remove buffer_start_add_atomic() and buffer_size_add_atomic(), and always use buffer_start_add_locked() and buffer_size_add_locked() instead ? Those functions still use atomic_set() and atomic_read(), which works fine in my tests. The only difference is that a spinlock in main memory is used instead of atomic_cmpxchg(). Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 20:26 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-16 20:26 UTC (permalink / raw) To: Colin Cross Cc: Mark Rutland, Will Deacon, Tony Luck, Kees Cook, Jeffy Chen, Douglas Anderson, linux-kernel, Robin Murphy, linux-arm-kernel@lists.infradead.org On Tue, Aug 16, 2016 at 10:35 AM, Colin Cross <ccross@android.com> wrote: > On Mon, Aug 15, 2016 at 3:15 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: >>> On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: >>> > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >>> >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>> >> > On 16/08/16 00:19, Guenter Roeck wrote: >>> >> >> we are having a problem with atomic accesses in pstore on some ARM >>> >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >>> >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >>> >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >>> >> > >>> >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >>> >> > >>> >> >>> >> it is normal DRAM. >>> > >>> > In which case, why does it need to be mapped with weird attributes? >>> > Is there an alias in the linear map you can use? >>> > >>> >>> I don't really _want_ to do anything besides using pstore as-is, or, >>> in other words, to have the upstream kernel work with the affected >>> systems. >>> >>> The current pstore code runs the following code for memory with >>> pfn_valid() = true. >>> >>> if (memtype) >>> prot = pgprot_noncached(PAGE_KERNEL); >>> else >>> prot = pgprot_writecombine(PAGE_KERNEL); >>> ... >>> vaddr = vmap(pages, page_count, VM_MAP, prot); >>> >>> It then uses the memory pointed to by vaddr for atomic operations. >> >> This means that the generic ramoops / pstore code is making non-portable >> assumptions about memory types. >> >> So _something_ has to happen to that code. >> >>> In my case, both protection options don't work. Everything works fine >>> (or at least doesn't create an exception) if I use >>> vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); >>> instead. >> >> Architecturally, that will give you a memory type to which we can safely use >> atomics on. >> >> It would be nice to know why the ramoops/pstore code must use atomics, and >> exactly what it's trying to achieve (i.e. is this just for serialisation, or an >> attempt to ensure persistence). >> >> Depending on that, it may be possible to fix things more generically by using >> memremap by default, for example, and only allowing uncached mappings on those >> architectures which support them. > > persistent_ram uses atomic ops in uncached memory to store the start > and end positions in the ringbuffer so that the state of the > ringbuffer will be valid if the kernel crashes at any time. This was > inherited from Android's ram_console implementation, and worked > through armv7. It has been causing more and more problems recently, > see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: > Allow optional mapping with pgprot_noncached) and > 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by > using write-combine mappings). > > Maybe it should be replaced with a spinlock in normal ram protecting > writes to the uncached region. The necessary functions already exist, and are used for memory mapped with ioremap() / ioremap_wc(). They were introduced with commit 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped regions"), and the description in that patch sounds quite similar to the current problem. Given that, would it be acceptable to remove buffer_start_add_atomic() and buffer_size_add_atomic(), and always use buffer_start_add_locked() and buffer_size_add_locked() instead ? Those functions still use atomic_set() and atomic_read(), which works fine in my tests. The only difference is that a spinlock in main memory is used instead of atomic_cmpxchg(). Thanks, Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 20:26 ` Guenter Roeck @ 2016-08-16 20:50 ` Kees Cook -1 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2016-08-16 20:50 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 1:26 PM, Guenter Roeck <groeck@google.com> wrote: > On Tue, Aug 16, 2016 at 10:35 AM, Colin Cross <ccross@android.com> wrote: >> On Mon, Aug 15, 2016 at 3:15 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: >>>> On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: >>>> > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >>>> >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>>> >> > On 16/08/16 00:19, Guenter Roeck wrote: >>>> >> >> we are having a problem with atomic accesses in pstore on some ARM >>>> >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >>>> >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >>>> >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >>>> >> > >>>> >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >>>> >> > >>>> >> >>>> >> it is normal DRAM. >>>> > >>>> > In which case, why does it need to be mapped with weird attributes? >>>> > Is there an alias in the linear map you can use? >>>> > >>>> >>>> I don't really _want_ to do anything besides using pstore as-is, or, >>>> in other words, to have the upstream kernel work with the affected >>>> systems. >>>> >>>> The current pstore code runs the following code for memory with >>>> pfn_valid() = true. >>>> >>>> if (memtype) >>>> prot = pgprot_noncached(PAGE_KERNEL); >>>> else >>>> prot = pgprot_writecombine(PAGE_KERNEL); >>>> ... >>>> vaddr = vmap(pages, page_count, VM_MAP, prot); >>>> >>>> It then uses the memory pointed to by vaddr for atomic operations. >>> >>> This means that the generic ramoops / pstore code is making non-portable >>> assumptions about memory types. >>> >>> So _something_ has to happen to that code. >>> >>>> In my case, both protection options don't work. Everything works fine >>>> (or at least doesn't create an exception) if I use >>>> vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); >>>> instead. >>> >>> Architecturally, that will give you a memory type to which we can safely use >>> atomics on. >>> >>> It would be nice to know why the ramoops/pstore code must use atomics, and >>> exactly what it's trying to achieve (i.e. is this just for serialisation, or an >>> attempt to ensure persistence). >>> >>> Depending on that, it may be possible to fix things more generically by using >>> memremap by default, for example, and only allowing uncached mappings on those >>> architectures which support them. >> >> persistent_ram uses atomic ops in uncached memory to store the start >> and end positions in the ringbuffer so that the state of the >> ringbuffer will be valid if the kernel crashes at any time. This was >> inherited from Android's ram_console implementation, and worked >> through armv7. It has been causing more and more problems recently, >> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: >> Allow optional mapping with pgprot_noncached) and >> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by >> using write-combine mappings). >> >> Maybe it should be replaced with a spinlock in normal ram protecting >> writes to the uncached region. > > The necessary functions already exist, and are used for memory mapped > with ioremap() / ioremap_wc(). They were introduced with commit > 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped > regions"), and the description in that patch sounds quite similar to > the current problem. Given that, would it be acceptable to remove > buffer_start_add_atomic() and buffer_size_add_atomic(), and always use > buffer_start_add_locked() and buffer_size_add_locked() instead ? Those > functions still use atomic_set() and atomic_read(), which works fine > in my tests. The only difference is that a spinlock in main memory is > used instead of atomic_cmpxchg(). I don't see much of a down side to this. ramoops isn't expected to be high-bandwidth so trading for a single global lock doesn't really bother me. (I've added other folks to CC that have touched this more recently...) -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-16 20:50 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2016-08-16 20:50 UTC (permalink / raw) To: Guenter Roeck Cc: Colin Cross, Mark Rutland, Will Deacon, Tony Luck, Jeffy Chen, Douglas Anderson, linux-kernel, Robin Murphy, linux-arm-kernel@lists.infradead.org, Tony Lindgren, Rob Herring, Anton Vorontsov On Tue, Aug 16, 2016 at 1:26 PM, Guenter Roeck <groeck@google.com> wrote: > On Tue, Aug 16, 2016 at 10:35 AM, Colin Cross <ccross@android.com> wrote: >> On Mon, Aug 15, 2016 at 3:15 PM, Mark Rutland <mark.rutland@arm.com> wrote: >>> On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: >>>> On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: >>>> > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: >>>> >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>>> >> > On 16/08/16 00:19, Guenter Roeck wrote: >>>> >> >> we are having a problem with atomic accesses in pstore on some ARM >>>> >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic >>>> >> >> accesses fail with both pgprot_noncached and pgprot_writecombine >>>> >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. >>>> >> > >>>> >> > What's the pstore backed by? I'm guessing it's not normal DRAM. >>>> >> > >>>> >> >>>> >> it is normal DRAM. >>>> > >>>> > In which case, why does it need to be mapped with weird attributes? >>>> > Is there an alias in the linear map you can use? >>>> > >>>> >>>> I don't really _want_ to do anything besides using pstore as-is, or, >>>> in other words, to have the upstream kernel work with the affected >>>> systems. >>>> >>>> The current pstore code runs the following code for memory with >>>> pfn_valid() = true. >>>> >>>> if (memtype) >>>> prot = pgprot_noncached(PAGE_KERNEL); >>>> else >>>> prot = pgprot_writecombine(PAGE_KERNEL); >>>> ... >>>> vaddr = vmap(pages, page_count, VM_MAP, prot); >>>> >>>> It then uses the memory pointed to by vaddr for atomic operations. >>> >>> This means that the generic ramoops / pstore code is making non-portable >>> assumptions about memory types. >>> >>> So _something_ has to happen to that code. >>> >>>> In my case, both protection options don't work. Everything works fine >>>> (or at least doesn't create an exception) if I use >>>> vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL); >>>> instead. >>> >>> Architecturally, that will give you a memory type to which we can safely use >>> atomics on. >>> >>> It would be nice to know why the ramoops/pstore code must use atomics, and >>> exactly what it's trying to achieve (i.e. is this just for serialisation, or an >>> attempt to ensure persistence). >>> >>> Depending on that, it may be possible to fix things more generically by using >>> memremap by default, for example, and only allowing uncached mappings on those >>> architectures which support them. >> >> persistent_ram uses atomic ops in uncached memory to store the start >> and end positions in the ringbuffer so that the state of the >> ringbuffer will be valid if the kernel crashes at any time. This was >> inherited from Android's ram_console implementation, and worked >> through armv7. It has been causing more and more problems recently, >> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: >> Allow optional mapping with pgprot_noncached) and >> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by >> using write-combine mappings). >> >> Maybe it should be replaced with a spinlock in normal ram protecting >> writes to the uncached region. > > The necessary functions already exist, and are used for memory mapped > with ioremap() / ioremap_wc(). They were introduced with commit > 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped > regions"), and the description in that patch sounds quite similar to > the current problem. Given that, would it be acceptable to remove > buffer_start_add_atomic() and buffer_size_add_atomic(), and always use > buffer_start_add_locked() and buffer_size_add_locked() instead ? Those > functions still use atomic_set() and atomic_read(), which works fine > in my tests. The only difference is that a spinlock in main memory is > used instead of atomic_cmpxchg(). I don't see much of a down side to this. ramoops isn't expected to be high-bandwidth so trading for a single global lock doesn't really bother me. (I've added other folks to CC that have touched this more recently...) -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 20:50 ` Kees Cook @ 2016-08-17 0:26 ` Guenter Roeck -1 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-17 0:26 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 1:50 PM, Kees Cook <keescook@chromium.org> wrote: [ ... ] >>> persistent_ram uses atomic ops in uncached memory to store the start >>> and end positions in the ringbuffer so that the state of the >>> ringbuffer will be valid if the kernel crashes at any time. This was >>> inherited from Android's ram_console implementation, and worked >>> through armv7. It has been causing more and more problems recently, >>> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: >>> Allow optional mapping with pgprot_noncached) and >>> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by >>> using write-combine mappings). >>> >>> Maybe it should be replaced with a spinlock in normal ram protecting >>> writes to the uncached region. >> >> The necessary functions already exist, and are used for memory mapped >> with ioremap() / ioremap_wc(). They were introduced with commit >> 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped >> regions"), and the description in that patch sounds quite similar to >> the current problem. Given that, would it be acceptable to remove >> buffer_start_add_atomic() and buffer_size_add_atomic(), and always use >> buffer_start_add_locked() and buffer_size_add_locked() instead ? Those >> functions still use atomic_set() and atomic_read(), which works fine >> in my tests. The only difference is that a spinlock in main memory is >> used instead of atomic_cmpxchg(). > > I don't see much of a down side to this. ramoops isn't expected to be > high-bandwidth so trading for a single global lock doesn't really > bother me. > Sounds good. I'll submit a patch to address the problem as suggested above. Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-17 0:26 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-17 0:26 UTC (permalink / raw) To: Kees Cook Cc: Colin Cross, Mark Rutland, Will Deacon, Tony Luck, Jeffy Chen, Douglas Anderson, linux-kernel, Robin Murphy, linux-arm-kernel@lists.infradead.org, Tony Lindgren, Rob Herring, Anton Vorontsov On Tue, Aug 16, 2016 at 1:50 PM, Kees Cook <keescook@chromium.org> wrote: [ ... ] >>> persistent_ram uses atomic ops in uncached memory to store the start >>> and end positions in the ringbuffer so that the state of the >>> ringbuffer will be valid if the kernel crashes at any time. This was >>> inherited from Android's ram_console implementation, and worked >>> through armv7. It has been causing more and more problems recently, >>> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: >>> Allow optional mapping with pgprot_noncached) and >>> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by >>> using write-combine mappings). >>> >>> Maybe it should be replaced with a spinlock in normal ram protecting >>> writes to the uncached region. >> >> The necessary functions already exist, and are used for memory mapped >> with ioremap() / ioremap_wc(). They were introduced with commit >> 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped >> regions"), and the description in that patch sounds quite similar to >> the current problem. Given that, would it be acceptable to remove >> buffer_start_add_atomic() and buffer_size_add_atomic(), and always use >> buffer_start_add_locked() and buffer_size_add_locked() instead ? Those >> functions still use atomic_set() and atomic_read(), which works fine >> in my tests. The only difference is that a spinlock in main memory is >> used instead of atomic_cmpxchg(). > > I don't see much of a down side to this. ramoops isn't expected to be > high-bandwidth so trading for a single global lock doesn't really > bother me. > Sounds good. I'll submit a patch to address the problem as suggested above. Guenter ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-17 0:26 ` Guenter Roeck (?) @ 2016-08-18 14:02 ` Tony Lindgren -1 siblings, 0 replies; 31+ messages in thread From: Tony Lindgren @ 2016-08-18 14:02 UTC (permalink / raw) To: linux-arm-kernel * Guenter Roeck <groeck@google.com> [160816 17:27]: > On Tue, Aug 16, 2016 at 1:50 PM, Kees Cook <keescook@chromium.org> wrote: > > [ ... ] > > >>> persistent_ram uses atomic ops in uncached memory to store the start > >>> and end positions in the ringbuffer so that the state of the > >>> ringbuffer will be valid if the kernel crashes at any time. This was > >>> inherited from Android's ram_console implementation, and worked > >>> through armv7. It has been causing more and more problems recently, > >>> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram: > >>> Allow optional mapping with pgprot_noncached) and > >>> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by > >>> using write-combine mappings). > >>> > >>> Maybe it should be replaced with a spinlock in normal ram protecting > >>> writes to the uncached region. > >> > >> The necessary functions already exist, and are used for memory mapped > >> with ioremap() / ioremap_wc(). They were introduced with commit > >> 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped > >> regions"), and the description in that patch sounds quite similar to > >> the current problem. Given that, would it be acceptable to remove > >> buffer_start_add_atomic() and buffer_size_add_atomic(), and always use > >> buffer_start_add_locked() and buffer_size_add_locked() instead ? Those > >> functions still use atomic_set() and atomic_read(), which works fine > >> in my tests. The only difference is that a spinlock in main memory is > >> used instead of atomic_cmpxchg(). > > > > I don't see much of a down side to this. ramoops isn't expected to be > > high-bandwidth so trading for a single global lock doesn't really > > bother me. > > > > Sounds good. I'll submit a patch to address the problem as suggested above. Yeah seems like it simplifies things quite a bit for us. Tony ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-16 17:35 ` Colin Cross @ 2016-08-19 9:35 ` Russell King - ARM Linux -1 siblings, 0 replies; 31+ messages in thread From: Russell King - ARM Linux @ 2016-08-19 9:35 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 16, 2016 at 10:35:52AM -0700, Colin Cross wrote: > persistent_ram uses atomic ops in uncached memory to store the start > and end positions in the ringbuffer so that the state of the > ringbuffer will be valid if the kernel crashes at any time. This was > inherited from Android's ram_console implementation, and worked > through armv7. That statement is actually inaccurate. It may have worked on _some_ ARMv7 implementations, but it's not architecturally compliant. The exclusive access instructions are not portable to anything but "normal memory": It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be performed to a memory region with the Device or Strongly-ordered memory attribute. Unless the implementation documentation explicitly states that LDREX and STREX operations to a memory region with the Device or Strongly-ordered attribute are permitted, the effect of such operations is UNPREDICTABLE. pgprot_noncached() gives strongly-ordered memory, and so is unsuitable to place semaphores in for all ARMv7 implementations. Also: + if (memtype) + va = ioremap(start, size); this returns *device* memory, which is also unsuitable for all ARMv7 implementations. It seems that pstore is playing in areas of the architecture which are implementation defined, so it's no surprise that folk are seeing different behaviours with different implementations. The code isn't architecturally wrong, it just isn't portable to all ARMv7 implementations. So, saying that it works on some ARMv7 implementations is irrelevent. Note that LDREX and STREX are used for all operations that require atomicity - iow, atomics and locks. pgprot_writecombine() and ioremap_wc() will return memory which is suitable for these exclusive accesses - it maps to the architectures' "normal memory, uncacheable". So, I suspect the OP should not be using mem_type=1 or using the "unbuffered" DT attribute, but should leave it was the default (mem_type=0). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-19 9:35 ` Russell King - ARM Linux 0 siblings, 0 replies; 31+ messages in thread From: Russell King - ARM Linux @ 2016-08-19 9:35 UTC (permalink / raw) To: Colin Cross Cc: Mark Rutland, Tony Luck, Kees Cook, Guenter Roeck, Jeffy Chen, Will Deacon, Douglas Anderson, linux-kernel, Robin Murphy, linux-arm-kernel@lists.infradead.org On Tue, Aug 16, 2016 at 10:35:52AM -0700, Colin Cross wrote: > persistent_ram uses atomic ops in uncached memory to store the start > and end positions in the ringbuffer so that the state of the > ringbuffer will be valid if the kernel crashes at any time. This was > inherited from Android's ram_console implementation, and worked > through armv7. That statement is actually inaccurate. It may have worked on _some_ ARMv7 implementations, but it's not architecturally compliant. The exclusive access instructions are not portable to anything but "normal memory": It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be performed to a memory region with the Device or Strongly-ordered memory attribute. Unless the implementation documentation explicitly states that LDREX and STREX operations to a memory region with the Device or Strongly-ordered attribute are permitted, the effect of such operations is UNPREDICTABLE. pgprot_noncached() gives strongly-ordered memory, and so is unsuitable to place semaphores in for all ARMv7 implementations. Also: + if (memtype) + va = ioremap(start, size); this returns *device* memory, which is also unsuitable for all ARMv7 implementations. It seems that pstore is playing in areas of the architecture which are implementation defined, so it's no surprise that folk are seeing different behaviours with different implementations. The code isn't architecturally wrong, it just isn't portable to all ARMv7 implementations. So, saying that it works on some ARMv7 implementations is irrelevent. Note that LDREX and STREX are used for all operations that require atomicity - iow, atomics and locks. pgprot_writecombine() and ioremap_wc() will return memory which is suitable for these exclusive accesses - it maps to the architectures' "normal memory, uncacheable". So, I suspect the OP should not be using mem_type=1 or using the "unbuffered" DT attribute, but should leave it was the default (mem_type=0). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-19 9:35 ` Russell King - ARM Linux @ 2016-08-19 12:47 ` Guenter Roeck -1 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-19 12:47 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 19, 2016 at 2:35 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Tue, Aug 16, 2016 at 10:35:52AM -0700, Colin Cross wrote: >> persistent_ram uses atomic ops in uncached memory to store the start >> and end positions in the ringbuffer so that the state of the >> ringbuffer will be valid if the kernel crashes at any time. This was >> inherited from Android's ram_console implementation, and worked >> through armv7. > > That statement is actually inaccurate. It may have worked on _some_ > ARMv7 implementations, but it's not architecturally compliant. > > The exclusive access instructions are not portable to anything but > "normal memory": > > It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can > be performed to a memory region with the Device or Strongly-ordered > memory attribute. Unless the implementation documentation explicitly > states that LDREX and STREX operations to a memory region with the > Device or Strongly-ordered attribute are permitted, the effect of > such operations is UNPREDICTABLE. > > pgprot_noncached() gives strongly-ordered memory, and so is unsuitable > to place semaphores in for all ARMv7 implementations. > > Also: > > + if (memtype) > + va = ioremap(start, size); > > this returns *device* memory, which is also unsuitable for all ARMv7 > implementations. > > It seems that pstore is playing in areas of the architecture which are > implementation defined, so it's no surprise that folk are seeing > different behaviours with different implementations. > > The code isn't architecturally wrong, it just isn't portable to all ARMv7 > implementations. > > So, saying that it works on some ARMv7 implementations is irrelevent. > > Note that LDREX and STREX are used for all operations that require > atomicity - iow, atomics and locks. > > pgprot_writecombine() and ioremap_wc() will return memory which is > suitable for these exclusive accesses - it maps to the architectures' > "normal memory, uncacheable". > Unfortunately, pgprot_writecombine() doesn't work in my case (for rk3288 and rk3399). It was also reported not to work on Tegra Logan by Nvidia. As mentioned earlier, PAGE_KERNEL works, at least for rk3288 and rk3399. Guenter > So, I suspect the OP should not be using mem_type=1 or using the > "unbuffered" DT attribute, but should leave it was the default > (mem_type=0). > > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-19 12:47 ` Guenter Roeck 0 siblings, 0 replies; 31+ messages in thread From: Guenter Roeck @ 2016-08-19 12:47 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Colin Cross, Mark Rutland, Tony Luck, Kees Cook, Jeffy Chen, Will Deacon, Douglas Anderson, linux-kernel, Robin Murphy, linux-arm-kernel@lists.infradead.org On Fri, Aug 19, 2016 at 2:35 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Tue, Aug 16, 2016 at 10:35:52AM -0700, Colin Cross wrote: >> persistent_ram uses atomic ops in uncached memory to store the start >> and end positions in the ringbuffer so that the state of the >> ringbuffer will be valid if the kernel crashes at any time. This was >> inherited from Android's ram_console implementation, and worked >> through armv7. > > That statement is actually inaccurate. It may have worked on _some_ > ARMv7 implementations, but it's not architecturally compliant. > > The exclusive access instructions are not portable to anything but > "normal memory": > > It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can > be performed to a memory region with the Device or Strongly-ordered > memory attribute. Unless the implementation documentation explicitly > states that LDREX and STREX operations to a memory region with the > Device or Strongly-ordered attribute are permitted, the effect of > such operations is UNPREDICTABLE. > > pgprot_noncached() gives strongly-ordered memory, and so is unsuitable > to place semaphores in for all ARMv7 implementations. > > Also: > > + if (memtype) > + va = ioremap(start, size); > > this returns *device* memory, which is also unsuitable for all ARMv7 > implementations. > > It seems that pstore is playing in areas of the architecture which are > implementation defined, so it's no surprise that folk are seeing > different behaviours with different implementations. > > The code isn't architecturally wrong, it just isn't portable to all ARMv7 > implementations. > > So, saying that it works on some ARMv7 implementations is irrelevent. > > Note that LDREX and STREX are used for all operations that require > atomicity - iow, atomics and locks. > > pgprot_writecombine() and ioremap_wc() will return memory which is > suitable for these exclusive accesses - it maps to the architectures' > "normal memory, uncacheable". > Unfortunately, pgprot_writecombine() doesn't work in my case (for rk3288 and rk3399). It was also reported not to work on Tegra Logan by Nvidia. As mentioned earlier, PAGE_KERNEL works, at least for rk3288 and rk3399. Guenter > So, I suspect the OP should not be using mem_type=1 or using the > "unbuffered" DT attribute, but should leave it was the default > (mem_type=0). > > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Problem with atomic accesses in pstore on some ARM CPUs 2016-08-15 22:15 ` Mark Rutland @ 2016-08-22 21:03 ` Arnd Bergmann -1 siblings, 0 replies; 31+ messages in thread From: Arnd Bergmann @ 2016-08-22 21:03 UTC (permalink / raw) To: linux-arm-kernel On Monday, August 15, 2016 11:15:18 PM CEST Mark Rutland wrote: > On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: > > On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: > > > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: > > >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > > >> > On 16/08/16 00:19, Guenter Roeck wrote: > > >> >> we are having a problem with atomic accesses in pstore on some ARM > > >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic > > >> >> accesses fail with both pgprot_noncached and pgprot_writecombine > > >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > >> > > > >> > What's the pstore backed by? I'm guessing it's not normal DRAM. > > >> > > > >> > > >> it is normal DRAM. > > > > > > In which case, why does it need to be mapped with weird attributes? > > > Is there an alias in the linear map you can use? > > > > > > > I don't really _want_ to do anything besides using pstore as-is, or, > > in other words, to have the upstream kernel work with the affected > > systems. > > > > The current pstore code runs the following code for memory with > > pfn_valid() = true. > > > > if (memtype) > > prot = pgprot_noncached(PAGE_KERNEL); > > else > > prot = pgprot_writecombine(PAGE_KERNEL); > > ... > > vaddr = vmap(pages, page_count, VM_MAP, prot); > > > > It then uses the memory pointed to by vaddr for atomic operations. > > This means that the generic ramoops / pstore code is making non-portable > assumptions about memory types. > > So _something_ has to happen to that code. If we have both a cacheable and a noncacheable mapping for the same DRAM area, things get even worse across many architectures. IIRC PowerPC will trigger a checkstop if it encounters a valid cache line for a noncacheable mapping. If there is only one mapping, this is not a problem, but we probably want to avoid having a write-back cache here, in case something serious goes wrong and all the cache is invalidated but the pstore is used for post-mortem analysis. Arnd ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: Problem with atomic accesses in pstore on some ARM CPUs @ 2016-08-22 21:03 ` Arnd Bergmann 0 siblings, 0 replies; 31+ messages in thread From: Arnd Bergmann @ 2016-08-22 21:03 UTC (permalink / raw) To: linux-arm-kernel Cc: Mark Rutland, Guenter Roeck, Tony Luck, Kees Cook, Jeffy Chen, Will Deacon, linux-kernel, Douglas Anderson, Colin Cross, Robin Murphy On Monday, August 15, 2016 11:15:18 PM CEST Mark Rutland wrote: > On Tue, Aug 16, 2016 at 08:02:53AM -0700, Guenter Roeck wrote: > > On Tue, Aug 16, 2016 at 6:21 AM, Will Deacon <will.deacon@arm.com> wrote: > > > On Tue, Aug 16, 2016 at 06:14:53AM -0700, Guenter Roeck wrote: > > >> On Tue, Aug 16, 2016 at 3:32 AM, Robin Murphy <robin.murphy@arm.com> wrote: > > >> > On 16/08/16 00:19, Guenter Roeck wrote: > > >> >> we are having a problem with atomic accesses in pstore on some ARM > > >> >> CPUs (specifically rk3288 and rk3399). With those chips, atomic > > >> >> accesses fail with both pgprot_noncached and pgprot_writecombine > > >> >> memory. Atomic accesses do work when selecting PAGE_KERNEL protection. > > >> > > > >> > What's the pstore backed by? I'm guessing it's not normal DRAM. > > >> > > > >> > > >> it is normal DRAM. > > > > > > In which case, why does it need to be mapped with weird attributes? > > > Is there an alias in the linear map you can use? > > > > > > > I don't really _want_ to do anything besides using pstore as-is, or, > > in other words, to have the upstream kernel work with the affected > > systems. > > > > The current pstore code runs the following code for memory with > > pfn_valid() = true. > > > > if (memtype) > > prot = pgprot_noncached(PAGE_KERNEL); > > else > > prot = pgprot_writecombine(PAGE_KERNEL); > > ... > > vaddr = vmap(pages, page_count, VM_MAP, prot); > > > > It then uses the memory pointed to by vaddr for atomic operations. > > This means that the generic ramoops / pstore code is making non-portable > assumptions about memory types. > > So _something_ has to happen to that code. If we have both a cacheable and a noncacheable mapping for the same DRAM area, things get even worse across many architectures. IIRC PowerPC will trigger a checkstop if it encounters a valid cache line for a noncacheable mapping. If there is only one mapping, this is not a problem, but we probably want to avoid having a write-back cache here, in case something serious goes wrong and all the cache is invalidated but the pstore is used for post-mortem analysis. Arnd ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2016-08-22 21:04 UTC | newest] Thread overview: 31+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-15 23:19 Problem with atomic accesses in pstore on some ARM CPUs Guenter Roeck 2016-08-15 23:19 ` Guenter Roeck 2016-08-16 10:32 ` Robin Murphy 2016-08-16 10:32 ` Robin Murphy 2016-08-16 10:45 ` Will Deacon 2016-08-16 10:45 ` Will Deacon 2016-08-16 13:21 ` Guenter Roeck 2016-08-16 13:21 ` Guenter Roeck 2016-08-16 13:14 ` Guenter Roeck 2016-08-16 13:14 ` Guenter Roeck 2016-08-16 13:21 ` Will Deacon 2016-08-16 13:21 ` Will Deacon 2016-08-16 15:02 ` Guenter Roeck 2016-08-16 15:02 ` Guenter Roeck 2016-08-15 22:15 ` Mark Rutland 2016-08-15 22:15 ` Mark Rutland 2016-08-16 17:35 ` Colin Cross 2016-08-16 17:35 ` Colin Cross 2016-08-16 20:26 ` Guenter Roeck 2016-08-16 20:26 ` Guenter Roeck 2016-08-16 20:50 ` Kees Cook 2016-08-16 20:50 ` Kees Cook 2016-08-17 0:26 ` Guenter Roeck 2016-08-17 0:26 ` Guenter Roeck 2016-08-18 14:02 ` Tony Lindgren 2016-08-19 9:35 ` Russell King - ARM Linux 2016-08-19 9:35 ` Russell King - ARM Linux 2016-08-19 12:47 ` Guenter Roeck 2016-08-19 12:47 ` Guenter Roeck 2016-08-22 21:03 ` Arnd Bergmann 2016-08-22 21:03 ` Arnd Bergmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.