* [RFC PATCH 08/11] powerpc/tm: Do not reclaim on ptrace
From: Breno Leitao @ 2018-09-12 19:40 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, paulus, gromero, mpe, ldufour, Breno Leitao
In-Reply-To: <1536781219-13938-1-git-send-email-leitao@debian.org>
Make sure that we are not suspended on ptrace and that the registers were
already reclaimed.
Since the data was already reclaimed, there is nothing to be done here
except to restore the SPRs.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
arch/powerpc/kernel/ptrace.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 9667666eb18e..cf6ee9154b11 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -136,12 +136,10 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
return;
- if (MSR_TM_SUSPENDED(mfmsr())) {
- tm_reclaim_current(TM_CAUSE_SIGNAL);
- } else {
- tm_enable();
- tm_save_sprs(&(tsk->thread));
- }
+ WARN_ON(MSR_TM_SUSPENDED(mfmsr()));
+
+ tm_enable();
+ tm_save_sprs(&(tsk->thread));
}
#else
static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
--
2.19.0
^ permalink raw reply related
* [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR
From: Breno Leitao @ 2018-09-12 19:40 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, paulus, gromero, mpe, ldufour, Breno Leitao
In-Reply-To: <1536781219-13938-1-git-send-email-leitao@debian.org>
In the previous TM code, trecheckpoint was being executed in the middle of
an exception, thus, DSCR was being restored to default kernel DSCR value
after trecheckpoint was done.
With this current patchset, trecheckpoint is executed just before getting
to userspace, at ret_from_except_lite, for example. Thus, we do not need to
set default kernel DSCR value anymore, as we are leaving kernel space. It
is OK to keep the checkpointed DSCR value into the live SPR, mainly because
the transaction is doomed and it will fail soon (after RFID), so,
continuing with the pre-checkpointed DSCR value is what seems correct.
That said, we must set the DSCR value that will be used in userspace now.
Current trecheckpoint() function sets it to the pre-checkpointed value
prior to lines being changed in this patch, so, removing these lines would
keep the pre-checkpointed values.
Important to say that we do not need to do the same thing with tm_reclaim,
since it already set the DSCR to the default value, after TRECLAIM is
called, in the following lines:
/* Load CPU's default DSCR */
ld r0, PACA_DSCR_DEFAULT(r13)
mtspr SPRN_DSCR, r0
Signed-off-by: Breno Leitao <leitao@debian.org>
---
arch/powerpc/kernel/tm.S | 4 ----
1 file changed, 4 deletions(-)
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 6bffbc5affe7..5427eda69846 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -493,10 +493,6 @@ restore_gprs:
mtlr r0
ld r2, STK_GOT(r1)
- /* Load CPU's default DSCR */
- ld r0, PACA_DSCR_DEFAULT(r13)
- mtspr SPRN_DSCR, r0
-
blr
/* ****************************************************************** */
--
2.19.0
^ permalink raw reply related
* [RFC PATCH 10/11] powerpc/tm: Set failure summary
From: Breno Leitao @ 2018-09-12 19:40 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, paulus, gromero, mpe, ldufour, Breno Leitao
In-Reply-To: <1536781219-13938-1-git-send-email-leitao@debian.org>
Since the transaction will be doomed with treckpt., the TEXASR[FS]
should be set, to reflect that the transaction is a failure. This patch
ensures it before recheckpointing, and remove changes from other places
that were calling recheckpoint.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
arch/powerpc/kernel/process.c | 6 ++++++
arch/powerpc/kernel/signal_32.c | 2 --
arch/powerpc/kernel/signal_64.c | 2 --
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5cace1b744b1..77725b2e4dc1 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -937,6 +937,12 @@ void tm_recheckpoint(struct thread_struct *thread)
local_irq_save(flags);
hard_irq_disable();
+ /*
+ * Make sure the failure summary is set, since the transaction will be
+ * doomed.
+ */
+ thread->tm_texasr |= TEXASR_FS;
+
/* The TM SPRs are restored here, so that TEXASR.FS can be set
* before the trecheckpoint and no explosion occurs.
*/
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 4a1b17409bf3..96956d50538e 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -851,8 +851,6 @@ static long restore_tm_user_regs(struct pt_regs *regs,
/* Pull in the MSR TM bits from the user context */
regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
- /* Make sure the transaction is marked as failed */
- current->thread.tm_texasr |= TEXASR_FS;
/* Make sure restore_tm_state will be called */
set_thread_flag(TIF_RESTORE_TM);
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 32402aa23a5e..c84501711b14 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -569,8 +569,6 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
}
}
#endif
- /* Make sure the transaction is marked as failed */
- tsk->thread.tm_texasr |= TEXASR_FS;
/* Guarantee that restore_tm_state() will be called */
set_thread_flag(TIF_RESTORE_TM);
--
2.19.0
^ permalink raw reply related
* [RFC PATCH 11/11] selftests/powerpc: Adapt the test
From: Breno Leitao @ 2018-09-12 19:40 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, paulus, gromero, mpe, ldufour, Breno Leitao
In-Reply-To: <1536781219-13938-1-git-send-email-leitao@debian.org>
The Documentation/powerpc/transactional_memory.txt says:
"Syscalls made from within a suspended transaction are performed as normal
and the transaction is not explicitly doomed by the kernel. However,
what the kernel does to perform the syscall may result in the transaction
being doomed by the hardware."
With this new TM mechanism, the syscall will continue to be executed if the
syscall happens on a suspended syscall, but, the syscall will *fail* if the
transaction is still active during the syscall invocation.
On the syscall path, if the transaction is active and not suspended, it
will call TM_KERNEL_ENTRY which will reclaim and recheckpoint the
transaction, thus, dooming the transaction on userspace return, with
failure code TM_CAUSE_SYSCALL.
This new model will break part of this test, but I understand that that the
documentation above didn't guarantee that the syscall would succeed, and it
will never succeed anymore now on.
In fact, glibc is calling 'tabort' before every syscalls, thus, any syscall
called through glibc from inside a transaction will be doomed anyhow.
This patch updates the test case to not assume that a syscall inside a
active transaction will succeed, because it will not anymore.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
tools/testing/selftests/powerpc/tm/tm-syscall.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c
index 454b965a2db3..1439a87eba3a 100644
--- a/tools/testing/selftests/powerpc/tm/tm-syscall.c
+++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c
@@ -78,12 +78,6 @@ int tm_syscall(void)
timeradd(&end, &now, &end);
for (count = 0; timercmp(&now, &end, <); count++) {
- /*
- * Test a syscall within a suspended transaction and verify
- * that it succeeds.
- */
- FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */
-
/*
* Test a syscall within an active transaction and verify that
* it fails with the correct failure code.
--
2.19.0
^ permalink raw reply related
* [PATCH v2] selftests/powerpc: Do not fail with reschedule
From: Breno Leitao @ 2018-09-12 20:31 UTC (permalink / raw)
To: linuxppc-dev; +Cc: gromero, Breno Leitao
In-Reply-To: <3404db1f-cd5a-5ca1-0559-26d2ad08f410@linux.vnet.ibm.com>
There are cases where the test is not expecting to have the transaction
aborted, but, the test process might have been rescheduled, either in the
OS level or by KVM (if it is running on a KVM guest machine). The process
reschedule will cause a treclaim/recheckpoint which will cause the
transaction to doom, aborting the transaction as soon as the process is
rescheduled back to the CPU. This might cause the test to fail, but this is
not a failure in essence.
If that is the case, TEXASR[FC] is indicated with either
TM_CAUSE_RESCHEDULE or TM_CAUSE_KVM_RESCHEDULE for KVM interruptions.
In this scenario, ignore these two failures and avoid the whole test to
return failure.
Signed-off-by: Breno Leitao <leitao@debian.org>
Reviewed-by: Gustavo Romero <gromero@linux.ibm.com>
---
tools/testing/selftests/powerpc/tm/tm-unavailable.c | 9 ++++++---
tools/testing/selftests/powerpc/tm/tm.h | 9 +++++++++
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index 156c8e750259..09894f4ff62e 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -236,7 +236,8 @@ void *tm_una_ping(void *input)
}
/* Check if we were not expecting a failure and a it occurred. */
- if (!expecting_failure() && is_failure(cr_)) {
+ if (!expecting_failure() && is_failure(cr_) &&
+ !failure_is_reschedule()) {
printf("\n\tUnexpected transaction failure 0x%02lx\n\t",
failure_code());
return (void *) -1;
@@ -244,9 +245,11 @@ void *tm_una_ping(void *input)
/*
* Check if TM failed due to the cause we were expecting. 0xda is a
- * TM_CAUSE_FAC_UNAV cause, otherwise it's an unexpected cause.
+ * TM_CAUSE_FAC_UNAV cause, otherwise it's an unexpected cause, unless
+ * it was caused by a reschedule.
*/
- if (is_failure(cr_) && !failure_is_unavailable()) {
+ if (is_failure(cr_) && !failure_is_unavailable() &&
+ !failure_is_reschedule()) {
printf("\n\tUnexpected failure cause 0x%02lx\n\t",
failure_code());
return (void *) -1;
diff --git a/tools/testing/selftests/powerpc/tm/tm.h b/tools/testing/selftests/powerpc/tm/tm.h
index df4204247d45..5518b1d4ef8b 100644
--- a/tools/testing/selftests/powerpc/tm/tm.h
+++ b/tools/testing/selftests/powerpc/tm/tm.h
@@ -52,6 +52,15 @@ static inline bool failure_is_unavailable(void)
return (failure_code() & TM_CAUSE_FAC_UNAV) == TM_CAUSE_FAC_UNAV;
}
+static inline bool failure_is_reschedule(void)
+{
+ if ((failure_code() & TM_CAUSE_RESCHED) == TM_CAUSE_RESCHED ||
+ (failure_code() & TM_CAUSE_KVM_RESCHED) == TM_CAUSE_KVM_RESCHED)
+ return true;
+
+ return false;
+}
+
static inline bool failure_is_nesting(void)
{
return (__builtin_get_texasru() & 0x400000);
--
2.19.0
^ permalink raw reply related
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
From: Michael Neuling @ 2018-09-13 0:36 UTC (permalink / raw)
To: Christophe LEROY, mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni,
Michal Suchánek
In-Reply-To: <0922624b-6c6f-1afd-a9e2-cde5a9a8a1e4@c-s.fr>
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -23,11 +23,33 @@
> > #include <asm/code-patching.h>
> > #include <asm/setup.h>
> > =20
> > +
>=20
> This blank line is not needed
Ack
>=20
> > +static inline bool in_init_section(unsigned int *patch_addr)
> > +{
> > + if (patch_addr < (unsigned int *)__init_begin)
> > + return false;
> > + if (patch_addr >=3D (unsigned int *)__init_end)
> > + return false;
> > + return true;
> > +}
>=20
> Can we use the existing function init_section_contains() instead of this=
=20
> new function ?
Nice, I was looking for something like that...=20
> > +
> > +static inline bool init_freed(void)
> > +{
> > + return (system_state >=3D SYSTEM_RUNNING);
> > +}
> > +
>=20
> I would call this function differently, for instance init_is_finished(),=
=20
> because as you mentionned it doesn't exactly mean that init memory is fre=
ed.
Talking to Nick and mpe offline I think we are going to have to add a flag =
when
we free init mem rather than doing what we have now since what we have now =
has a
potential race. That change will eliminate the function entirely.
> > static int __patch_instruction(unsigned int *exec_addr, unsigned int
> > instr,
> > unsigned int *patch_addr)
> > {
> > int err;
> > =20
> > + /* Make sure we aren't patching a freed init section */
> > + if (in_init_section(patch_addr) && init_freed()) {
>=20
> The test must be done on exec_addr, not on patch_addr, as patch_addr is=
=20
> the address where the instruction as been remapped RW for allowing its=
=20
> modification.
Thanks for the catch
> Also I think it should be tested the other way round, because the=20
> init_freed() is a simpler test which will be false most of the time once=
=20
> the system is running so it should be checked first.
ok, I'll change.
> > + printk(KERN_DEBUG "Skipping init section patching addr:
> > 0x%lx\n",
>=20
> Maybe use pr_debug() instead.
Sure.
>=20
> > + (unsigned long)patch_addr);
>=20
> Please align second line as per Codying style.
Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell=
it
this out for me...
>=20
> > + return 0;
> > + }
> > +
> > __put_user_size(instr, patch_addr, 4, err);
> > if (err)
> > return err;
> >=20
>=20
> I think it would be better to put this verification in=20
> patch_instruction() instead, to avoid RW mapping/unmapping the=20
> instruction to patch when we are not going to do the patching.
If we do it there then we miss the raw_patch_intruction case.
IMHO I don't think we need to optimise this rare and non-critical path.=20
Mikey
^ permalink raw reply
* Re: [PATCH v2 03/17] compat_ioctl: use correct compat_ptr() translation in drivers
From: Andrew Donnellan @ 2018-09-13 0:48 UTC (permalink / raw)
To: Arnd Bergmann, viro
Cc: linux-fsdevel, Frederic Barrat, Greg Kroah-Hartman,
Frank Haverkamp, Guilherme G. Piccoli, Kashyap Desai,
Sumit Saxena, Shivasharan S, James E.J. Bottomley,
Martin K. Petersen, Felipe Balbi, linuxppc-dev, linux-kernel,
megaraidlinux.pdl, linux-scsi, linux-usb
In-Reply-To: <20180912150142.157913-3-arnd@arndb.de>
On 13/09/18 01:01, Arnd Bergmann wrote:
> A handful of drivers all have a trivial wrapper around their ioctl
> handler, but don't call the compat_ptr() conversion function at the
> moment. In practice this does not matter, since none of them are used
> on the s390 architecture and for all other architectures, compat_ptr()
> does not do anything, but using the new generic_compat_ioctl_ptrarg
> helper makes it more correct in theory, and simplifies the code.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
For cxl:
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
From: Tyrel Datwyler @ 2018-09-13 1:21 UTC (permalink / raw)
To: Michael Neuling, Christophe LEROY, mpe
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
In-Reply-To: <29d3467a9314f5b80f93d241ae2566c48b546bfe.camel@neuling.org>
On 09/12/2018 05:36 PM, Michael Neuling wrote:
>
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -23,11 +23,33 @@
>>> #include <asm/code-patching.h>
>>> #include <asm/setup.h>
>>>
>>> +
>>
>> This blank line is not needed
>
> Ack
>
>>
>>> +static inline bool in_init_section(unsigned int *patch_addr)
>>> +{
>>> + if (patch_addr < (unsigned int *)__init_begin)
>>> + return false;
>>> + if (patch_addr >= (unsigned int *)__init_end)
>>> + return false;
>>> + return true;
>>> +}
>>
>> Can we use the existing function init_section_contains() instead of this
>> new function ?
>
> Nice, I was looking for something like that...
>
>>> +
>>> +static inline bool init_freed(void)
>>> +{
>>> + return (system_state >= SYSTEM_RUNNING);
>>> +}
>>> +
>>
>> I would call this function differently, for instance init_is_finished(),
>> because as you mentionned it doesn't exactly mean that init memory is freed.
>
> Talking to Nick and mpe offline I think we are going to have to add a flag when
> we free init mem rather than doing what we have now since what we have now has a
> potential race. That change will eliminate the function entirely.
>
>>> static int __patch_instruction(unsigned int *exec_addr, unsigned int
>>> instr,
>>> unsigned int *patch_addr)
>>> {
>>> int err;
>>>
>>> + /* Make sure we aren't patching a freed init section */
>>> + if (in_init_section(patch_addr) && init_freed()) {
>>
>> The test must be done on exec_addr, not on patch_addr, as patch_addr is
>> the address where the instruction as been remapped RW for allowing its
>> modification.
>
> Thanks for the catch
>
>> Also I think it should be tested the other way round, because the
>> init_freed() is a simpler test which will be false most of the time once
>> the system is running so it should be checked first.
>
> ok, I'll change.
>
>>> + printk(KERN_DEBUG "Skipping init section patching addr:
>>> 0x%lx\n",
>>
>> Maybe use pr_debug() instead.
>
> Sure.
>
>>
>>> + (unsigned long)patch_addr);
>>
>> Please align second line as per Codying style.
>
> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
> this out for me...
I suspect that the suggestion is the opening parenthesis of "(unsigned long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty sure Documentation/process/coding-style.rst is very adamant that all identation is always 8 characters and spaces should never be used, but there still seems to be a lot of places/suggestions that argument lists that spill over multiple lines should be space indented to align with the very first argument at the top level. So, I guess I'm not sure what the desire is here. Although moving to pr_debug might fit it to a single line anyways. ;)
-Tyrel
>
>>
>>> + return 0;
>>> + }
>>> +
>>> __put_user_size(instr, patch_addr, 4, err);
>>> if (err)
>>> return err;
>>>
>>
>> I think it would be better to put this verification in
>> patch_instruction() instead, to avoid RW mapping/unmapping the
>> instruction to patch when we are not going to do the patching.
>
> If we do it there then we miss the raw_patch_intruction case.
>
> IMHO I don't think we need to optimise this rare and non-critical path.
>
> Mikey
>
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: Add PPC contacts for PCI core error handling
From: Russell Currey @ 2018-09-13 2:26 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci, linuxppc-dev, sbobroff
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
linux-kernel, oohall
In-Reply-To: <20180912165846.GH118330@bhelgaas-glaptop.roam.corp.google.com>
On Wed, 2018-09-12 at 11:58 -0500, Bjorn Helgaas wrote:
> On Wed, Sep 12, 2018 at 11:55:26AM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > The original PCI error recovery functionality was for the powerpc-specific
> > IBM EEH feature. PCIe subsequently added some similar features, including
> > AER and DPC, that can be used on any architecture.
> >
> > We want the generic PCI core error handling support to work with all of
> > these features. Driver error recovery callbacks should be independent of
> > which feature the platform provides.
> >
> > Add the generic PCI core error recovery files to the powerpc EEH
> > MAINTAINERS entry so the powerpc folks will be copied on changes to the
> > generic PCI error handling strategy.
>
> I really want to make sure the powerpc folks are plugged into any PCI core
> error handling discussions. Please let me know if there's a better way
> than this patch, or if there are other people who should be added.
I think this patch is definitely a good idea.
Alongside this (whether you want to include it in your patch or not, Bjorn),
Sam Bobroff should replace myself as maintainer. He's been very active in fixing,
reworking and reviewing in EEH land and will probably do a better job of keeping
his finger on the pulse of the generic PCI error handling code than I did.
- Russell
>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> > MAINTAINERS | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7e10ba65bfe4..d6699597fd89 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11202,6 +11202,10 @@ PCI ENHANCED ERROR HANDLING (EEH) FOR POWERPC
> > M: Russell Currey <ruscur@russell.cc>
> > L: linuxppc-dev@lists.ozlabs.org
> > S: Supported
> > +F: Documentation/PCI/pci-error-recovery.txt
> > +F: drivers/pci/pcie/aer.c
> > +F: drivers/pci/pcie/dpc.c
> > +F: drivers/pci/pcie/err.c
> > F: Documentation/powerpc/eeh-pci-error-recovery.txt
> > F: arch/powerpc/kernel/eeh*.c
> > F: arch/powerpc/platforms/*/eeh*.c
> >
^ permalink raw reply
* [PATCH v3] powerpc: Avoid code patching freed init sections
From: Michael Neuling @ 2018-09-13 3:03 UTC (permalink / raw)
To: mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni, mikey,
Michal Suchánek, Christophe LEROY
This stops us from doing code patching in init sections after they've
been freed.
In this chain:
kvm_guest_init() ->
kvm_use_magic_page() ->
fault_in_pages_readable() ->
__get_user() ->
__get_user_nocheck() ->
barrier_nospec();
We have a code patching location at barrier_nospec() and
kvm_guest_init() is an init function. This whole chain gets inlined,
so when we free the init section (hence kvm_guest_init()), this code
goes away and hence should no longer be patched.
We seen this as userspace memory corruption when using a memory
checker while doing partition migration testing on powervm (this
starts the code patching post migration via
/sys/kernel/mobility/migration). In theory, it could also happen when
using /sys/kernel/debug/powerpc/barrier_nospec.
cc: stable@vger.kernel.org # 4.13+
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
For stable I've marked this as v4.13+ since that's when we refactored
code-patching.c but it could go back even further than that. In
reality though, I think we can only hit this since the first
spectre/meltdown changes.
v3:
Add init_mem_free flag to avoid potential race.
Feedback from Christophe Leroy:
- use init_section_contains()
- change order of init test for performance
- use pr_debug()
- remove blank line
v2:
Print when we skip an address
---
arch/powerpc/include/asm/setup.h | 1 +
arch/powerpc/lib/code-patching.c | 7 +++++++
arch/powerpc/mm/mem.c | 2 ++
3 files changed, 10 insertions(+)
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 1a951b0046..4b4522b738 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -9,6 +9,7 @@ extern void ppc_printk_progress(char *s, unsigned short hex);
extern unsigned int rtas_data;
extern unsigned long long memory_limit;
+extern bool init_mem_free;
extern unsigned long klimit;
extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 850f3b8f4d..dacd6cef92 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -28,6 +28,13 @@ static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
{
int err;
+ /* Make sure we aren't patching a freed init section */
+ if (init_mem_free && init_section_contains(exec_addr, 4)) {
+ pr_debug("Skipping init section patching addr: 0x%lx\n",
+ (unsigned long)exec_addr);
+ return 0;
+ }
+
__put_user_size(instr, patch_addr, 4, err);
if (err)
return err;
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 5c8530d0c6..b9d59e1a83 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -63,6 +63,7 @@
#endif
unsigned long long memory_limit;
+bool init_mem_free;
#ifdef CONFIG_HIGHMEM
pte_t *kmap_pte;
@@ -396,6 +397,7 @@ void free_initmem(void)
{
ppc_md.progress = ppc_printk_progress;
mark_initmem_nx();
+ init_mem_free = true;
free_initmem_default(POISON_FREE_INITMEM);
}
--
2.17.1
^ permalink raw reply related
* RE: [PATCH 0/7 v7] Support for fsl-mc bus and its devices in SMMU
From: Nipun Gupta @ 2018-09-13 5:01 UTC (permalink / raw)
To: Will Deacon
Cc: joro@8bytes.org, robin.murphy@arm.com, robh+dt@kernel.org,
robh@kernel.org, mark.rutland@arm.com, catalin.marinas@arm.com,
gregkh@linuxfoundation.org, Laurentiu Tudor, bhelgaas@google.com,
hch@lst.de, m.szyprowski@samsung.com, shawnguo@kernel.org,
frowand.list@gmail.com, iommu@lists.linux-foundation.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linuxppc-dev@lists.ozlabs.org, linux-pci@vger.kernel.org,
Bharat Bhushan, stuyoder@gmail.com, Leo Li
In-Reply-To: <20180912165845.GD16071@arm.com>
> -----Original Message-----
> From: Will Deacon [mailto:will.deacon@arm.com]
> Sent: Wednesday, September 12, 2018 10:29 PM
> To: Nipun Gupta <nipun.gupta@nxp.com>
> Cc: joro@8bytes.org; robin.murphy@arm.com; robh+dt@kernel.org;
> robh@kernel.org; mark.rutland@arm.com; catalin.marinas@arm.com;
> gregkh@linuxfoundation.org; Laurentiu Tudor <laurentiu.tudor@nxp.com>;
> bhelgaas@google.com; hch@lst.de; m.szyprowski@samsung.com;
> shawnguo@kernel.org; frowand.list@gmail.com; iommu@lists.linux-
> foundation.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org; linu=
x-
> pci@vger.kernel.org; Bharat Bhushan <bharat.bhushan@nxp.com>;
> stuyoder@gmail.com; Leo Li <leoyang.li@nxp.com>
> Subject: Re: [PATCH 0/7 v7] Support for fsl-mc bus and its devices in SMM=
U
>=20
> Hi Nipun,
>=20
> On Mon, Sep 10, 2018 at 07:19:14PM +0530, Nipun Gupta wrote:
> > This patchset defines IOMMU DT binding for fsl-mc bus and adds
> > support in SMMU for fsl-mc bus.
> >
> > These patches
> > - Define property 'iommu-map' for fsl-mc bus (patch 1)
> > - Integrates the fsl-mc bus with the SMMU using this
> > IOMMU binding (patch 2,3,4)
> > - Adds the dma configuration support for fsl-mc bus (patch 5, 6)
> > - Updates the fsl-mc device node with iommu/dma related changes (patc=
h 7)
>=20
> It looks like you have all the Acks in place for this series now, so I
> assume it's going to go via Joerg directly. Is that right?
This is what I am expecting :)
=20
> Will
>=20
^ permalink raw reply
* [PATCH] KVM: PPC: Book3S HV: Fix guest r11 corruption with POWER9 TM workarounds
From: Michael Neuling @ 2018-09-13 5:33 UTC (permalink / raw)
To: mpe; +Cc: linuxppc-dev, kvm-ppc, paulus, sjitindarsingh, mikey
When we come into the softpatch handler (0x1500), we use r11 to store
the HSRR0 for later use by the denorm handler.
We also use the softpatch handler for the TM workarounds for
POWER9. Unfortunately, in kvmppc_interrupt_hv we later store r11 out
to the vcpu assuming it's still what we got from userspace.
This causes r11 to be corrupted in the VCPU and hence when we restore
the guest, we get a corrupted r11. We've seen this when running TM
tests inside guests on P9.
This fixes the problem by only touching r11 in the denorm case.
Fixes: 4bb3c7a020 ("KVM: PPC: Book3S HV: Work around transactional memory bugs in POWER9")
Cc: <stable@vger.kernel.org> # 4.17+
Test-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
arch/powerpc/kernel/exceptions-64s.S | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index ea04dfb8c0..2d8fc8c9da 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -1314,9 +1314,7 @@ EXC_REAL_BEGIN(denorm_exception_hv, 0x1500, 0x100)
#ifdef CONFIG_PPC_DENORMALISATION
mfspr r10,SPRN_HSRR1
- mfspr r11,SPRN_HSRR0 /* save HSRR0 */
andis. r10,r10,(HSRR1_DENORM)@h /* denorm? */
- addi r11,r11,-4 /* HSRR0 is next instruction */
bne+ denorm_assist
#endif
@@ -1382,6 +1380,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
*/
XVCPSGNDP32(32)
denorm_done:
+ mfspr r11,SPRN_HSRR0
+ subi r11,r11,4
mtspr SPRN_HSRR0,r11
mtcrf 0x80,r9
ld r9,PACA_EXGEN+EX_R9(r13)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
From: Christophe LEROY @ 2018-09-13 5:38 UTC (permalink / raw)
To: Tyrel Datwyler, Michael Neuling, mpe
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
In-Reply-To: <e8a68da4-1c58-b30b-ccd0-f37a9e7c330c@linux.vnet.ibm.com>
Le 13/09/2018 à 03:21, Tyrel Datwyler a écrit :
> On 09/12/2018 05:36 PM, Michael Neuling wrote:
>>
>>>
>>>> + (unsigned long)patch_addr);
>>>
>>> Please align second line as per Codying style.
>>
>> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
>> this out for me...
>
> I suspect that the suggestion is the opening parenthesis of "(unsigned long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty sure Documentation/process/coding-style.rst is very adamant that all identation is always 8 characters and spaces should never be used, but there still seems to be a lot of places/suggestions that argument lists that spill over multiple lines should be space indented to align with the very first argument at the top level. So, I guess I'm not sure what the desire is here. Although moving to pr_debug might fit it to a single line anyways. ;)
It is exactly that, as reported by checkpatch, look at
https://patchwork.ozlabs.org/patch/968850/
Christophe
^ permalink raw reply
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
From: Christophe LEROY @ 2018-09-13 5:45 UTC (permalink / raw)
To: Michael Neuling, mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni,
Michal Suchánek
In-Reply-To: <29d3467a9314f5b80f93d241ae2566c48b546bfe.camel@neuling.org>
Le 13/09/2018 à 02:36, Michael Neuling a écrit :
>
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -23,11 +23,33 @@
>>> #include <asm/code-patching.h>
>>> #include <asm/setup.h>
>>>
>>> +
>>
>> This blank line is not needed
>
> Ack
>
>>
>>> +static inline bool in_init_section(unsigned int *patch_addr)
>>> +{
>>> + if (patch_addr < (unsigned int *)__init_begin)
>>> + return false;
>>> + if (patch_addr >= (unsigned int *)__init_end)
>>> + return false;
>>> + return true;
>>> +}
>>
>> Can we use the existing function init_section_contains() instead of this
>> new function ?
>
> Nice, I was looking for something like that...
>
>>> +
>>> +static inline bool init_freed(void)
>>> +{
>>> + return (system_state >= SYSTEM_RUNNING);
>>> +}
>>> +
>>
>> I would call this function differently, for instance init_is_finished(),
>> because as you mentionned it doesn't exactly mean that init memory is freed.
>
> Talking to Nick and mpe offline I think we are going to have to add a flag when
> we free init mem rather than doing what we have now since what we have now has a
> potential race. That change will eliminate the function entirely.
>
>>> static int __patch_instruction(unsigned int *exec_addr, unsigned int
>>> instr,
>>> unsigned int *patch_addr)
>>> {
>>> int err;
>>>
>>> + /* Make sure we aren't patching a freed init section */
>>> + if (in_init_section(patch_addr) && init_freed()) {
>>
>> The test must be done on exec_addr, not on patch_addr, as patch_addr is
>> the address where the instruction as been remapped RW for allowing its
>> modification.
>
> Thanks for the catch
>
>> Also I think it should be tested the other way round, because the
>> init_freed() is a simpler test which will be false most of the time once
>> the system is running so it should be checked first.
>
> ok, I'll change.
>
>>> + printk(KERN_DEBUG "Skipping init section patching addr:
>>> 0x%lx\n",
>>
>> Maybe use pr_debug() instead.
>
> Sure.
>
>>
>>> + (unsigned long)patch_addr);
>>
>> Please align second line as per Codying style.
>
> Sorry I can't see what's wrong. You're (or Cody :-P) going to have to spell it
> this out for me...
>
>>
>>> + return 0;
>>> + }
>>> +
>>> __put_user_size(instr, patch_addr, 4, err);
>>> if (err)
>>> return err;
>>>
>>
>> I think it would be better to put this verification in
>> patch_instruction() instead, to avoid RW mapping/unmapping the
>> instruction to patch when we are not going to do the patching.
>
> If we do it there then we miss the raw_patch_intruction case.
raw_patch_instruction() can only be used during init. Once kernel memory
has been marked readonly, raw_patch_instruction() cannot be used
anymore. And mark_readonly() is called immediately after free_initmem()
Christophe
>
> IMHO I don't think we need to optimise this rare and non-critical path.
>
> Mikey
>
^ permalink raw reply
* Re: [PATCH v2] powerpc: Avoid code patching freed init sections
From: Michael Neuling @ 2018-09-13 5:48 UTC (permalink / raw)
To: Christophe LEROY, Tyrel Datwyler, mpe
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
In-Reply-To: <7d32de2e-b390-527d-cd5c-2c5eacc19a2c@c-s.fr>
On Thu, 2018-09-13 at 07:38 +0200, Christophe LEROY wrote:
>=20
> Le 13/09/2018 =C3=A0 03:21, Tyrel Datwyler a =C3=A9crit :
> > On 09/12/2018 05:36 PM, Michael Neuling wrote:
> > >=20
> > > >=20
> > > > > + (unsigned long)patch_addr);
> > > >=20
> > > > Please align second line as per Codying style.
> > >=20
> > > Sorry I can't see what's wrong. You're (or Cody :-P) going to have to
> > > spell it
> > > this out for me...
> >=20
> > I suspect that the suggestion is the opening parenthesis of "(unsigned
> > long)" should sit directly under the "K" of "KERN_DEBUG". I'm pretty su=
re
> > Documentation/process/coding-style.rst is very adamant that all identat=
ion
> > is always 8 characters and spaces should never be used, but there still
> > seems to be a lot of places/suggestions that argument lists that spill =
over
> > multiple lines should be space indented to align with the very first
> > argument at the top level. So, I guess I'm not sure what the desire is =
here.
> > Although moving to pr_debug might fit it to a single line anyways. ;)
>=20
> It is exactly that, as reported by checkpatch, look at=20
> https://patchwork.ozlabs.org/patch/968850/
Sweet... looks like v3 is clean
https://patchwork.ozlabs.org/patch/969241/
Mikey
^ permalink raw reply
* Re: [PATCH v3] powerpc: Avoid code patching freed init sections
From: Christophe LEROY @ 2018-09-13 5:51 UTC (permalink / raw)
To: Michael Neuling, mpe
Cc: linuxppc-dev, Nicholas Piggin, paulus, Haren Myneni,
Michal Suchánek
In-Reply-To: <20180913030329.12052-1-mikey@neuling.org>
Le 13/09/2018 à 05:03, Michael Neuling a écrit :
> This stops us from doing code patching in init sections after they've
> been freed.
>
> In this chain:
> kvm_guest_init() ->
> kvm_use_magic_page() ->
> fault_in_pages_readable() ->
> __get_user() ->
> __get_user_nocheck() ->
> barrier_nospec();
>
> We have a code patching location at barrier_nospec() and
> kvm_guest_init() is an init function. This whole chain gets inlined,
> so when we free the init section (hence kvm_guest_init()), this code
> goes away and hence should no longer be patched.
>
> We seen this as userspace memory corruption when using a memory
> checker while doing partition migration testing on powervm (this
> starts the code patching post migration via
> /sys/kernel/mobility/migration). In theory, it could also happen when
> using /sys/kernel/debug/powerpc/barrier_nospec.
>
> cc: stable@vger.kernel.org # 4.13+
> Signed-off-by: Michael Neuling <mikey@neuling.org>
>
> ---
> For stable I've marked this as v4.13+ since that's when we refactored
> code-patching.c but it could go back even further than that. In
> reality though, I think we can only hit this since the first
> spectre/meltdown changes.
>
> v3:
> Add init_mem_free flag to avoid potential race.
> Feedback from Christophe Leroy:
> - use init_section_contains()
> - change order of init test for performance
> - use pr_debug()
> - remove blank line
>
> v2:
> Print when we skip an address
> ---
> arch/powerpc/include/asm/setup.h | 1 +
> arch/powerpc/lib/code-patching.c | 7 +++++++
> arch/powerpc/mm/mem.c | 2 ++
> 3 files changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
> index 1a951b0046..4b4522b738 100644
> --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -9,6 +9,7 @@ extern void ppc_printk_progress(char *s, unsigned short hex);
>
> extern unsigned int rtas_data;
> extern unsigned long long memory_limit;
> +extern bool init_mem_free;
Calling it init_mem_is_free would be more explicit.
Here one might think that it contains the amount of free memory.
> extern unsigned long klimit;
> extern void *zalloc_maybe_bootmem(size_t size, gfp_t mask);
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 850f3b8f4d..dacd6cef92 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -28,6 +28,13 @@ static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
> {
> int err;
>
> + /* Make sure we aren't patching a freed init section */
> + if (init_mem_free && init_section_contains(exec_addr, 4)) {
> + pr_debug("Skipping init section patching addr: 0x%lx\n",
> + (unsigned long)exec_addr);
Using %px instead of %lx would avoid having to cast exec_addr, and then
it would fit in one line (in arch/powerpc we accept lines of 90 chars)
> + return 0;
> + }
> +
> __put_user_size(instr, patch_addr, 4, err);
> if (err)
> return err;
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 5c8530d0c6..b9d59e1a83 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -63,6 +63,7 @@
> #endif
>
> unsigned long long memory_limit;
> +bool init_mem_free;
>
> #ifdef CONFIG_HIGHMEM
> pte_t *kmap_pte;
> @@ -396,6 +397,7 @@ void free_initmem(void)
> {
> ppc_md.progress = ppc_printk_progress;
> mark_initmem_nx();
> + init_mem_free = true;
> free_initmem_default(POISON_FREE_INITMEM);
> }
>
>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Christophe
^ permalink raw reply
* [PATCH] watchdog: mpc8xxx: provide boot status
From: Christophe Leroy @ 2018-09-13 8:07 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Radu Rendec
Cc: linux-kernel, linuxppc-dev, linux-watchdog
mpc8xxx watchdog driver supports the following platforms:
- mpc8xx
- mpc83xx
- mpc86xx
Those three platforms have a 32 bits register which provides the
reason of the last boot, including whether it was caused by the
watchdog.
mpc8xx: Register RSR, bit SWRS (bit 3)
mpc83xx: Register RSR, bit SWRS (bit 28)
mpc86xx: Register RSTRSCR, bit WDT_RR (bit 11)
This patch maps the register as defined in the device tree and updates
wdt.bootstatus based on the value of the watchdog related bit. Then
the information can be retrieved via the WDIOC_GETBOOTSTATUS ioctl.
Hereunder is an exemple of devicetree for mpc8xx,
the Reset Status Register being at offset 0x288:
WDT: watchdog@0 {
compatible = "fsl,mpc823-wdt";
reg = <0x0 0x10 0x288 0x4>;
};
On the mpc83xx, RSR is at offset 0x910
On the mpc86xx, RSTRSCR is at offset 0xe0094
Suggested-by: Radu Rendec <radu.rendec@gmail.com>
Tested-by: Christophe Leroy <christophe.leroy@c-s.fr> # On mpc885
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/watchdog/mpc8xxx_wdt.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/watchdog/mpc8xxx_wdt.c b/drivers/watchdog/mpc8xxx_wdt.c
index aca2d6323f8a..2951a485a6b4 100644
--- a/drivers/watchdog/mpc8xxx_wdt.c
+++ b/drivers/watchdog/mpc8xxx_wdt.c
@@ -49,10 +49,12 @@ struct mpc8xxx_wdt {
struct mpc8xxx_wdt_type {
int prescaler;
bool hw_enabled;
+ u32 rsr_mask;
};
struct mpc8xxx_wdt_ddata {
struct mpc8xxx_wdt __iomem *base;
+ u32 __iomem *rsr;
struct watchdog_device wdd;
spinlock_t lock;
u16 swtc;
@@ -137,6 +139,7 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
struct mpc8xxx_wdt_ddata *ddata;
u32 freq = fsl_get_sys_freq();
bool enabled;
+ struct device *dev = &ofdev->dev;
wdt_type = of_device_get_match_data(&ofdev->dev);
if (!wdt_type)
@@ -160,6 +163,22 @@ static int mpc8xxx_wdt_probe(struct platform_device *ofdev)
return -ENODEV;
}
+ res = platform_get_resource(ofdev, IORESOURCE_MEM, 1);
+ ddata->rsr = devm_ioremap_resource(dev, res);
+ if (IS_ERR(ddata->rsr)) {
+ dev_info(dev, "Could not map reset status register");
+ } else {
+ u32 rsr_v = in_be32(ddata->rsr);
+ bool status = rsr_v & wdt_type->rsr_mask;
+
+ ddata->wdd.bootstatus = status ? WDIOF_CARDRESET : 0;
+ /* clear reset status bits related to watchdog time */
+ out_be32(ddata->rsr, wdt_type->rsr_mask);
+
+ dev_info(dev, "Last boot was %s by watchdog (RSR = 0x%8.8x)\n",
+ status ? "caused" : "not caused", rsr_v);
+ }
+
spin_lock_init(&ddata->lock);
ddata->wdd.info = &mpc8xxx_wdt_info,
@@ -216,6 +235,7 @@ static const struct of_device_id mpc8xxx_wdt_match[] = {
.compatible = "mpc83xx_wdt",
.data = &(struct mpc8xxx_wdt_type) {
.prescaler = 0x10000,
+ .rsr_mask = BIT(3), /* RSR Bit 28 */
},
},
{
@@ -223,6 +243,7 @@ static const struct of_device_id mpc8xxx_wdt_match[] = {
.data = &(struct mpc8xxx_wdt_type) {
.prescaler = 0x10000,
.hw_enabled = true,
+ .rsr_mask = BIT(20), /* RSTRSCR Bit 11 */
},
},
{
@@ -230,6 +251,7 @@ static const struct of_device_id mpc8xxx_wdt_match[] = {
.data = &(struct mpc8xxx_wdt_type) {
.prescaler = 0x800,
.hw_enabled = true,
+ .rsr_mask = BIT(28), /* RSR Bit 3 */
},
},
{},
--
2.13.3
^ permalink raw reply related
* Re: MPC83xx reset status register (RSR, offset 0x910)
From: Christophe LEROY @ 2018-09-13 8:21 UTC (permalink / raw)
To: Radu Rendec; +Cc: linuxppc-dev, oss, mpe, eric.miao
In-Reply-To: <CAD5jUk-oEoMwndqqPUs+AWnQThUjH+DA6fJUb5_W2rOPB1Njtw@mail.gmail.com>
Le 11/09/2018 à 00:17, Radu Rendec a écrit :
> Hi,
>
> On Mon, 2018-09-10 at 07:37 +0200, Christophe Leroy wrote:
>> Le 10/09/2018 à 01:13, Radu Rendec a écrit :
>>>
>>> I'm using U-boot as well, but it's just not configured to read or clear
>>> the RSR. I'm curious: if U-boot reads/clears the RSR in your case, how
>>> do you make the initial value available to user space programs running
>>> under Linux?
>>
>> I'm surprised. When looking at U-boot code, I don't see any way to
>> configure that. It seems just do by default in function cpu_init_f():
>>
>> https://elixir.bootlin.com/u-boot/v2018.07/source/arch/powerpc/cpu/mpc83xx/cpu_init.c#L217
>>
>> /* RSR - Reset Status Register - clear all status (4.6.1.3) */
>> gd->arch.reset_status = __raw_readl(&im->reset.rsr);
>> __raw_writel(~(RSR_RES), &im->reset.rsr);
>
> I'm working as a contractor in a large embedded project, so I don't know
> all the bits and pieces. I just checked the U-boot code. Whoever was
> maintaining it, "configured" it by commenting out the __raw_writel()
> that clears the register :)
>
> Probably the reason was specifically to be able to read it from Linux,
> but unfortunately the guy is not here any more to ask him.
>
> It may make more sense to read it from U-boot, but (1) the value must
> still be passed to Linux somehow and (2) in my case, I would prefer not
> to touch U-boot.
>
>> Do you know any user space program in Linux that needs this value ?
>
> I don't know of any "standard" program that needs it. In the project I'm
> working on, there are multiple peripherals on the board and initialization
> is slightly different when the reset line is physically asserted vs. a
> soft CPU reset. Besides, we need to show the reset reason to the user.
>
> I guess in the embedded world this is a fairly common use case, so
> perhaps others can benefit from that if I fix it in a way that can be
> pushed upstream.
>
>>> Thank you very much for the patches. Is there any chance they can be
>>> submitted upstream?
>>
>> I see no problem submitting them upstream, but are they really worth it
>> ? Adding Michael in copy to get his opinion.
>
> I guess it's worth if they are changed to make the value available to
> the kernel and user space rather than just decoding/printing it, for the
> reasons I mentioned above.
>
> The MPC83xx also has a watchdog and the kernel driver (mpc8xxx_wdt.c)
> could also be improved to support the WDIOC_GETBOOTSTATUS ioctl and
> properly report if the system rebooted due to a watchdog.
Very good idea.
I just submitted a patch for that. Please look at it.
I'm sure any driver which needs reset status information can do the same.
If we want to do something more central, maybe we should look at what
was done on ARM:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=04fef228fb00
Christophe
>
>>> I tried to look for something similar on other platforms or architectures,
>>> but couldn't find anything.
>>
>> I believe furst thing is to identify some app needing such an
>> information, then we'll be able to investigate how to handle it.
>
> Well, I guess I explained my reasons and use case. If there is any
> interest in that, I will gladly implement it in a way that makes sense
> to upstream. Let's see what Michael thinks.
>
> Thanks,
> Radu Rendec
>
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: Add PPC contacts for PCI core error handling
From: Benjamin Herrenschmidt @ 2018-09-13 8:35 UTC (permalink / raw)
To: Bjorn Helgaas, linux-pci, Russell Currey, linuxppc-dev
Cc: Paul Mackerras, Michael Ellerman, linux-kernel, Oliver OHalloran
In-Reply-To: <20180912165846.GH118330@bhelgaas-glaptop.roam.corp.google.com>
On Wed, 2018-09-12 at 11:58 -0500, Bjorn Helgaas wrote:
> > Add the generic PCI core error recovery files to the powerpc EEH
> > MAINTAINERS entry so the powerpc folks will be copied on changes to the
> > generic PCI error handling strategy.
>
> I really want to make sure the powerpc folks are plugged into any PCI core
> error handling discussions. Please let me know if there's a better way
> than this patch, or if there are other people who should be added.
Sounds good. Oliver, you want to be looped in as well ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v2 03/17] compat_ioctl: use correct compat_ptr() translation in drivers
From: Felipe Balbi @ 2018-09-13 11:03 UTC (permalink / raw)
To: Arnd Bergmann, viro
Cc: linux-fsdevel, Arnd Bergmann, Frederic Barrat, Andrew Donnellan,
Greg Kroah-Hartman, Frank Haverkamp, Guilherme G. Piccoli,
Kashyap Desai, Sumit Saxena, Shivasharan S, James E.J. Bottomley,
Martin K. Petersen, linuxppc-dev, linux-kernel, megaraidlinux.pdl,
linux-scsi, linux-usb
In-Reply-To: <20180912150142.157913-3-arnd@arndb.de>
[-- Attachment #1: Type: text/plain, Size: 590 bytes --]
Arnd Bergmann <arnd@arndb.de> writes:
> A handful of drivers all have a trivial wrapper around their ioctl
> handler, but don't call the compat_ptr() conversion function at the
> moment. In practice this does not matter, since none of them are used
> on the s390 architecture and for all other architectures, compat_ptr()
> does not do anything, but using the new generic_compat_ioctl_ptrarg
> helper makes it more correct in theory, and simplifies the code.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Felipe Balbi <felipe.balbi@linux.intel.com>
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: Add PPC contacts for PCI core error handling
From: Oliver @ 2018-09-13 11:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Bjorn Helgaas, linux-pci, Russell Currey, linuxppc-dev,
Paul Mackerras, Michael Ellerman, Linux Kernel Mailing List,
Oliver OHalloran
In-Reply-To: <d51126f75116534ec6ba6caee907b9db82363cc7.camel@kernel.crashing.org>
On Thu, Sep 13, 2018 at 6:35 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2018-09-12 at 11:58 -0500, Bjorn Helgaas wrote:
>> > Add the generic PCI core error recovery files to the powerpc EEH
>> > MAINTAINERS entry so the powerpc folks will be copied on changes to the
>> > generic PCI error handling strategy.
>>
>> I really want to make sure the powerpc folks are plugged into any PCI core
>> error handling discussions. Please let me know if there's a better way
>> than this patch, or if there are other people who should be added.
>
> Sounds good. Oliver, you want to be looped in as well ?
Sure, putting all of EEH on Sam is probably not a nice thing to do.
> Cheers,
> Ben.
>
>
^ permalink raw reply
* Re: [PATCH 0/2] sriov enablement on s390
From: Sebastian Ott @ 2018-09-13 12:41 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Arnd Bergmann, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Russell Currey, linuxppc-dev, Oliver OHalloran
In-Reply-To: <20180912130205.GG118330@bhelgaas-glaptop.roam.corp.google.com>
On Wed, 12 Sep 2018, Bjorn Helgaas wrote:
> [+cc Arnd, powerpc folks]
>
> On Wed, Sep 12, 2018 at 02:34:09PM +0200, Sebastian Ott wrote:
> > Hello Bjorn,
> >
> > On s390 we currently handle SRIOV within firmware. Which means
> > that the PF is under firmware control and not visible to operating
> > systems. SRIOV enablement happens within firmware and VFs are
> > passed through to logical partitions.
> >
> > I'm working on a new mode were the PF is under operating system
> > control (including SRIOV enablement). However we still need
> > firmware support to access the VFs. The way this is supposed
> > to work is that when firmware traps the SRIOV enablement it
> > will present machine checks to the logical partition that
> > triggered the SRIOV enablement and provide the VFs via hotplug
> > events.
> >
> > The problem I'm faced with is that the VF detection code in
> > sriov_enable leads to unusable functions in s390.
>
> We're moving away from the weak function implementation style. Can
> you take a look at Arnd's work here, which uses pci_host_bridge
> callbacks instead?
>
> https://lkml.kernel.org/r/20180817102645.3839621-1-arnd@arndb.de
>
> I cc'd some powerpc folks because they also have a fair amount of
> arch-specific SR-IOV code that might one day move in this direction.
Rebased to Arnd's pci-probe-rework branch.
Sebastian Ott (2):
pci: provide add_vfs/del_vfs callbacks
s390/pci: handle function enumeration after sriov enablement
arch/s390/pci/pci.c | 11 +++++++++++
drivers/pci/iov.c | 51 +++++++++++++++++++++++++++++++++++++++------------
include/linux/pci.h | 2 ++
3 files changed, 52 insertions(+), 12 deletions(-)
--
2.13.4
^ permalink raw reply
* [PATCH 1/2] pci: provide add_vfs/del_vfs callbacks
From: Sebastian Ott @ 2018-09-13 12:41 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Arnd Bergmann, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Russell Currey, linuxppc-dev, Oliver OHalloran
In-Reply-To: <20180913124124.14632-1-sebott@linux.ibm.com>
Provide callbacks that can be used by PCI host bridge implementations
to override the behavior of the generic vf detection and device
creation code.
Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
---
drivers/pci/iov.c | 51 +++++++++++++++++++++++++++++++++++++++------------
include/linux/pci.h | 2 ++
2 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 0f04ae648cf1..b2ddfe30c5d8 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -251,6 +251,41 @@ int __weak pcibios_sriov_disable(struct pci_dev *pdev)
return 0;
}
+static int pcibios_sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
+{
+ struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
+ unsigned int i;
+ int rc;
+
+ if (bridge->add_vfs)
+ return bridge->add_vfs(dev, num_vfs);
+
+ for (i = 0; i < num_vfs; i++) {
+ rc = pci_iov_add_virtfn(dev, i);
+ if (rc)
+ goto failed;
+ }
+ return 0;
+failed:
+ while (i--)
+ pci_iov_remove_virtfn(dev, i);
+
+ return rc;
+}
+
+static void pcibios_sriov_del_vfs(struct pci_dev *dev)
+{
+ struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
+ struct pci_sriov *iov = dev->sriov;
+ int i;
+
+ if (bridge->del_vfs)
+ return bridge->del_vfs(dev);
+
+ for (i = 0; i < iov->num_VFs; i++)
+ pci_iov_remove_virtfn(dev, i);
+}
+
static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
{
int rc;
@@ -336,21 +371,15 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
msleep(100);
pci_cfg_access_unlock(dev);
- for (i = 0; i < initial; i++) {
- rc = pci_iov_add_virtfn(dev, i);
- if (rc)
- goto failed;
- }
+ rc = pcibios_sriov_add_vfs(dev, initial);
+ if (rc)
+ goto err_pcibios;
kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
iov->num_VFs = nr_virtfn;
return 0;
-failed:
- while (i--)
- pci_iov_remove_virtfn(dev, i);
-
err_pcibios:
iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
pci_cfg_access_lock(dev);
@@ -369,14 +398,12 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
static void sriov_disable(struct pci_dev *dev)
{
- int i;
struct pci_sriov *iov = dev->sriov;
if (!iov->num_VFs)
return;
- for (i = 0; i < iov->num_VFs; i++)
- pci_iov_remove_virtfn(dev, i);
+ pcibios_sriov_del_vfs(dev);
iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
pci_cfg_access_lock(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 680b6bcd0b97..bf99ae98ecb5 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -475,6 +475,8 @@ struct pci_host_bridge {
int (*free_irq)(struct pci_dev *);
void (*add_bus)(struct pci_bus *);
void (*remove_bus)(struct pci_bus *);
+ int (*add_vfs)(struct pci_dev *dev, u16 num_vfs);
+ void (*del_vfs)(struct pci_dev *dev);
void *release_data;
struct msi_controller *msi;
unsigned int ignore_reset_delay:1; /* For entire hierarchy */
--
2.13.4
^ permalink raw reply related
* [PATCH 2/2] s390/pci: handle function enumeration after sriov enablement
From: Sebastian Ott @ 2018-09-13 12:41 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, Arnd Bergmann, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Russell Currey, linuxppc-dev, Oliver OHalloran
In-Reply-To: <20180913124124.14632-1-sebott@linux.ibm.com>
Implement add_vfs|del_vfs callbacks as empty functions. VF
creation will be triggered by the hotplug code.
Signed-off-by: Sebastian Ott <sebott@linux.ibm.com>
---
arch/s390/pci/pci.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 9381d5d98156..2ba2cbfaa091 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -785,6 +785,15 @@ static void zpci_remove_bus(struct pci_bus *bus)
kfree(zdev);
}
+static int zpci_add_vfs(struct pci_dev *pdev, u16 num_vfs)
+{
+ return 0;
+}
+
+static void zpci_del_vfs(struct pci_dev *pdev)
+{
+}
+
static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
struct pci_ops *ops, void *sysdata, struct list_head *resources)
{
@@ -801,6 +810,8 @@ static struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
bridge->busnr = bus;
bridge->ops = ops;
bridge->remove_bus = zpci_remove_bus;
+ bridge->add_vfs = zpci_add_vfs;
+ bridge->del_vfs = zpci_del_vfs;
error = pci_scan_root_bus_bridge(bridge);
if (error < 0)
--
2.13.4
^ permalink raw reply related
* Re: [PATCH v2 02/24] drivers/video/fbdev: use ioremap_wc/wt() instead of __ioremap()
From: Daniel Vetter @ 2018-09-13 13:33 UTC (permalink / raw)
To: Christophe Leroy
Cc: Bartlomiej Zolnierkiewicz, Benjamin Herrenschmidt,
Dominik Brodowski, Geoff Levand, Jens Axboe, Kumar Gala, Li Yang,
Michael Ellerman, Nicholas Piggin, Paul Mackerras, Scott Wood,
aneesh.kumar, linux-fbdev, linux-kernel, dri-devel, linux-block,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <2f980cf32a05415662272cf052c2b9e2a94c756f.1536692743.git.christophe.leroy@c-s.fr>
On Wed, Sep 12, 2018 at 03:58:17PM +0000, Christophe Leroy wrote:
> _PAGE_NO_CACHE is a platform specific flag. In addition, this flag
> is misleading because one would think it requests a noncached page
> whereas a noncached page is _PAGE_NO_CACHE | _PAGE_GUARDED
>
> _PAGE_NO_CACHE alone means write combined noncached page, so lets
> use ioremap_wc() instead.
>
> _PAGE_WRITETHRU is also platform specific flag. Use ioremap_wt()
> instead.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
I wondered why this all showed up in my gfx inbox, then spotted this patch
here.
I trust you way more on the _PAGE_ flags than me, but this looks
reasonable.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/video/fbdev/chipsfb.c | 3 +--
> drivers/video/fbdev/controlfb.c | 5 +----
> drivers/video/fbdev/platinumfb.c | 5 +----
> drivers/video/fbdev/valkyriefb.c | 12 ++++++------
> 4 files changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/video/fbdev/chipsfb.c b/drivers/video/fbdev/chipsfb.c
> index f103665cad43..40182ed85648 100644
> --- a/drivers/video/fbdev/chipsfb.c
> +++ b/drivers/video/fbdev/chipsfb.c
> @@ -27,7 +27,6 @@
> #include <linux/init.h>
> #include <linux/pci.h>
> #include <linux/console.h>
> -#include <asm/io.h>
>
> #ifdef CONFIG_PMAC_BACKLIGHT
> #include <asm/backlight.h>
> @@ -401,7 +400,7 @@ static int chipsfb_pci_init(struct pci_dev *dp, const struct pci_device_id *ent)
> #endif /* CONFIG_PMAC_BACKLIGHT */
>
> #ifdef CONFIG_PPC
> - p->screen_base = __ioremap(addr, 0x200000, _PAGE_NO_CACHE);
> + p->screen_base = ioremap_wc(addr, 0x200000);
> #else
> p->screen_base = ioremap(addr, 0x200000);
> #endif
> diff --git a/drivers/video/fbdev/controlfb.c b/drivers/video/fbdev/controlfb.c
> index 8d14b29aafea..9cb0ef7ac29e 100644
> --- a/drivers/video/fbdev/controlfb.c
> +++ b/drivers/video/fbdev/controlfb.c
> @@ -48,9 +48,7 @@
> #include <linux/nvram.h>
> #include <linux/adb.h>
> #include <linux/cuda.h>
> -#include <asm/io.h>
> #include <asm/prom.h>
> -#include <asm/pgtable.h>
> #include <asm/btext.h>
>
> #include "macmodes.h"
> @@ -715,8 +713,7 @@ static int __init control_of_init(struct device_node *dp)
> goto error_out;
> }
> /* map at most 8MB for the frame buffer */
> - p->frame_buffer = __ioremap(p->frame_buffer_phys, 0x800000,
> - _PAGE_WRITETHRU);
> + p->frame_buffer = ioremap_wt(p->frame_buffer_phys, 0x800000);
>
> if (!p->control_regs_phys ||
> !request_mem_region(p->control_regs_phys, p->control_regs_size,
> diff --git a/drivers/video/fbdev/platinumfb.c b/drivers/video/fbdev/platinumfb.c
> index 377d3399a3ad..bf6b7fb83cf4 100644
> --- a/drivers/video/fbdev/platinumfb.c
> +++ b/drivers/video/fbdev/platinumfb.c
> @@ -32,9 +32,7 @@
> #include <linux/nvram.h>
> #include <linux/of_device.h>
> #include <linux/of_platform.h>
> -#include <asm/io.h>
> #include <asm/prom.h>
> -#include <asm/pgtable.h>
>
> #include "macmodes.h"
> #include "platinumfb.h"
> @@ -577,8 +575,7 @@ static int platinumfb_probe(struct platform_device* odev)
>
> /* frame buffer - map only 4MB */
> pinfo->frame_buffer_phys = pinfo->rsrc_fb.start;
> - pinfo->frame_buffer = __ioremap(pinfo->rsrc_fb.start, 0x400000,
> - _PAGE_WRITETHRU);
> + pinfo->frame_buffer = ioremap_wt(pinfo->rsrc_fb.start, 0x400000);
> pinfo->base_frame_buffer = pinfo->frame_buffer;
>
> /* registers */
> diff --git a/drivers/video/fbdev/valkyriefb.c b/drivers/video/fbdev/valkyriefb.c
> index 275fb98236d3..d51c3a8009cb 100644
> --- a/drivers/video/fbdev/valkyriefb.c
> +++ b/drivers/video/fbdev/valkyriefb.c
> @@ -54,13 +54,11 @@
> #include <linux/nvram.h>
> #include <linux/adb.h>
> #include <linux/cuda.h>
> -#include <asm/io.h>
> #ifdef CONFIG_MAC
> #include <asm/macintosh.h>
> #else
> #include <asm/prom.h>
> #endif
> -#include <asm/pgtable.h>
>
> #include "macmodes.h"
> #include "valkyriefb.h"
> @@ -318,7 +316,7 @@ static void __init valkyrie_choose_mode(struct fb_info_valkyrie *p)
> int __init valkyriefb_init(void)
> {
> struct fb_info_valkyrie *p;
> - unsigned long frame_buffer_phys, cmap_regs_phys, flags;
> + unsigned long frame_buffer_phys, cmap_regs_phys;
> int err;
> char *option = NULL;
>
> @@ -337,7 +335,6 @@ int __init valkyriefb_init(void)
> /* Hardcoded addresses... welcome to 68k Macintosh country :-) */
> frame_buffer_phys = 0xf9000000;
> cmap_regs_phys = 0x50f24000;
> - flags = IOMAP_NOCACHE_SER; /* IOMAP_WRITETHROUGH?? */
> #else /* ppc (!CONFIG_MAC) */
> {
> struct device_node *dp;
> @@ -354,7 +351,6 @@ int __init valkyriefb_init(void)
>
> frame_buffer_phys = r.start;
> cmap_regs_phys = r.start + 0x304000;
> - flags = _PAGE_WRITETHRU;
> }
> #endif /* ppc (!CONFIG_MAC) */
>
> @@ -369,7 +365,11 @@ int __init valkyriefb_init(void)
> }
> p->total_vram = 0x100000;
> p->frame_buffer_phys = frame_buffer_phys;
> - p->frame_buffer = __ioremap(frame_buffer_phys, p->total_vram, flags);
> +#ifdef CONFIG_MAC
> + p->frame_buffer = ioremap_nocache(frame_buffer_phys, p->total_vram);
> +#else
> + p->frame_buffer = ioremap_wt(frame_buffer_phys, p->total_vram);
> +#endif
> p->cmap_regs_phys = cmap_regs_phys;
> p->cmap_regs = ioremap(p->cmap_regs_phys, 0x1000);
> p->valkyrie_regs_phys = cmap_regs_phys+0x6000;
> --
> 2.13.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox