* Re: [PATCH 2/7] powerpc/watchpoint/kvm: Add infrastructure to support 2nd DAWR
From: Paul Mackerras @ 2020-09-02 2:01 UTC (permalink / raw)
To: Ravi Bangoria
Cc: christophe.leroy, mikey, rogealve, kvm, linux-kernel, npiggin,
kvm-ppc, linux-kselftest, jniethe5, pedromfc, pbonzini,
linuxppc-dev
In-Reply-To: <20200723102058.312282-3-ravi.bangoria@linux.ibm.com>
On Thu, Jul 23, 2020 at 03:50:53PM +0530, Ravi Bangoria wrote:
> kvm code assumes single DAWR everywhere. Add code to support 2nd DAWR.
> DAWR is a hypervisor resource and thus H_SET_MODE hcall is used to set/
> unset it. Introduce new case H_SET_MODE_RESOURCE_SET_DAWR1 for 2nd DAWR.
Is this the same interface as will be defined in PAPR and available
under PowerVM, or is it a new/different interface for KVM?
> Also, kvm will support 2nd DAWR only if CPU_FTR_DAWR1 is set.
In general QEMU wants to be able to control all aspects of the virtual
machine presented to the guest, meaning that just because a host has a
particular hardware capability does not mean we should automatically
present that capability to the guest.
In this case, QEMU will want a way to control whether the guest sees
the availability of the second DAWR/X registers or not, i.e. whether a
H_SET_MODE to set DAWR[X]1 will succeed or fail.
Paul.
^ permalink raw reply
* Re: [PATCH 1/7] powerpc/watchpoint/kvm: Rename current DAWR macros and variables
From: Paul Mackerras @ 2020-09-02 1:49 UTC (permalink / raw)
To: Ravi Bangoria
Cc: christophe.leroy, mikey, rogealve, kvm, linux-kernel, npiggin,
kvm-ppc, linux-kselftest, jniethe5, pedromfc, pbonzini,
linuxppc-dev
In-Reply-To: <20200723102058.312282-2-ravi.bangoria@linux.ibm.com>
On Thu, Jul 23, 2020 at 03:50:52PM +0530, Ravi Bangoria wrote:
> Power10 is introducing second DAWR. Use real register names (with
> suffix 0) from ISA for current macros and variables used by kvm.
Most of this looks fine, but I think we should not change the existing
names in arch/powerpc/include/uapi/asm/kvm.h (and therefore also
Documentation/virt/kvm/api.rst).
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 426f94582b7a..4dc18fe6a2bf 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -2219,8 +2219,8 @@ registers, find a list below:
> PPC KVM_REG_PPC_BESCR 64
> PPC KVM_REG_PPC_TAR 64
> PPC KVM_REG_PPC_DPDES 64
> - PPC KVM_REG_PPC_DAWR 64
> - PPC KVM_REG_PPC_DAWRX 64
> + PPC KVM_REG_PPC_DAWR0 64
> + PPC KVM_REG_PPC_DAWRX0 64
> PPC KVM_REG_PPC_CIABR 64
> PPC KVM_REG_PPC_IC 64
> PPC KVM_REG_PPC_VTB 64
...
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> index 264e266a85bf..38d61b73f5ed 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -608,8 +608,8 @@ struct kvm_ppc_cpu_char {
> #define KVM_REG_PPC_BESCR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa7)
> #define KVM_REG_PPC_TAR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa8)
> #define KVM_REG_PPC_DPDES (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xa9)
> -#define KVM_REG_PPC_DAWR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xaa)
> -#define KVM_REG_PPC_DAWRX (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xab)
> +#define KVM_REG_PPC_DAWR0 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xaa)
> +#define KVM_REG_PPC_DAWRX0 (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xab)
> #define KVM_REG_PPC_CIABR (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xac)
> #define KVM_REG_PPC_IC (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xad)
> #define KVM_REG_PPC_VTB (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0xae)
The existing names are an API, and if you change them you will break
compilation of existing userspace programs. I don't see that adding
the '0' on the end is so important that we need to break userspace.
Paul.
^ permalink raw reply
* [PATCH v2] powerpc/powernv/pci: Drop pnv_phb->initialized
From: Oliver O'Halloran @ 2020-09-02 1:36 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Oliver O'Halloran
The pnv_phb->initialized flag is an odd beast. It was added back in 2012 in
commit db1266c85261 ("powerpc/powernv: Skip check on PE if necessary") to
allow devices to be enabled even if the device had not yet been assigned to
a PE. Allowing the device to be enabled before the PE is configured may
cause spurious EEH events since none of the IOMMU context has been setup.
I'm not entirely sure why this was ever necessary. My best guess is that it
was an workaround for a bug or some other undesireable behaviour from the
PCI core. Either way, it's unnecessary now since as of commit dc3d8f85bb57
("powerpc/powernv/pci: Re-work bus PE configuration") we can guarantee that
the PE will be configured before the PCI core will allow drivers to bind to
the device.
It's also worth pointing out that the ->initialized flag is only set in
pnv_pci_ioda_create_dbgfs(). That function has its entire body wrapped
in #ifdef CONFIG_DEBUG_FS. As a result, for kernels built without debugfs
(i.e. petitboot) the other checks in pnv_pci_enable_device_hook() are
bypassed entirely.
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
v2: reword commit message a bit
---
arch/powerpc/platforms/powernv/pci-ioda.c | 17 -----------------
arch/powerpc/platforms/powernv/pci.h | 1 -
2 files changed, 18 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 023a4f987bb2..6ac3c637b313 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2410,9 +2410,6 @@ static void pnv_pci_ioda_create_dbgfs(void)
list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
phb = hose->private_data;
- /* Notify initialization of PHB done */
- phb->initialized = 1;
-
sprintf(name, "PCI%04x", hose->global_number);
phb->dbgfs = debugfs_create_dir(name, powerpc_debugfs_root);
@@ -2609,17 +2606,8 @@ static resource_size_t pnv_pci_default_alignment(void)
*/
static bool pnv_pci_enable_device_hook(struct pci_dev *dev)
{
- struct pnv_phb *phb = pci_bus_to_pnvhb(dev->bus);
struct pci_dn *pdn;
- /* The function is probably called while the PEs have
- * not be created yet. For example, resource reassignment
- * during PCI probe period. We just skip the check if
- * PEs isn't ready.
- */
- if (!phb->initialized)
- return true;
-
pdn = pci_get_pdn(dev);
if (!pdn || pdn->pe_number == IODA_INVALID_PE)
return false;
@@ -2629,14 +2617,9 @@ static bool pnv_pci_enable_device_hook(struct pci_dev *dev)
static bool pnv_ocapi_enable_device_hook(struct pci_dev *dev)
{
- struct pci_controller *hose = pci_bus_to_host(dev->bus);
- struct pnv_phb *phb = hose->private_data;
struct pci_dn *pdn;
struct pnv_ioda_pe *pe;
- if (!phb->initialized)
- return true;
-
pdn = pci_get_pdn(dev);
if (!pdn)
return false;
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 739a0b3b72e1..36d22920f5a3 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -119,7 +119,6 @@ struct pnv_phb {
int flags;
void __iomem *regs;
u64 regs_phys;
- int initialized;
spinlock_t lock;
#ifdef CONFIG_DEBUG_FS
--
2.26.2
^ permalink raw reply related
* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
From: Herbert Xu @ 2020-09-02 1:34 UTC (permalink / raw)
To: Li Yang
Cc: linuxppc-dev@lists.ozlabs.org, Linux Kernel Mailing List,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CADRPPNTt5dCX1pRUp5OenZBuMNJcN+k8jMVmUo5qw5g0VLZ4hQ@mail.gmail.com>
On Tue, Sep 01, 2020 at 04:40:16PM -0500, Li Yang wrote:
>
> Looks like the CAAM driver and dependent QBMAN driver doesn't support
> COMPILE_TEST yet. Are you trying to add the support for it?
Yes.
> I think this is a valid concern that if the parent structure doesn't
> meet certain alignment requirements, the alignment for the
> sub-structure cannot be guaranteed. If we just remove the __packed
> attribute from the parent structure, the compiler could try to add
> padding in the parent structure to fulfill the alignment requirements
> of the sub structure which is not good. I think the following changes
> are a better fix for the warnings:
This works for me. Thanks!
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] cpuidle-pseries: Fix CEDE latency conversion from tb to us
From: Joel Stanley @ 2020-09-02 1:08 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: linux-pm, Rafael J. Wysocki, Linux Kernel Mailing List,
Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <1598969293-29228-1-git-send-email-ego@linux.vnet.ibm.com>
On Tue, 1 Sep 2020 at 14:09, Gautham R. Shenoy <ego@linux.vnet.ibm.com> wrote:
>
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> CEDE(0)") sets the exit latency of CEDE(0) based on the latency values
> of the Extended CEDE states advertised by the platform. The values
> advertised by the platform are in timebase ticks. However the cpuidle
> framework requires the latency values in microseconds.
>
> If the tb-ticks value advertised by the platform correspond to a value
> smaller than 1us, during the conversion from tb-ticks to microseconds,
> in the current code, the result becomes zero. This is incorrect as it
> puts a CEDE state on par with the snooze state.
>
> This patch fixes this by rounding up the result obtained while
> converting the latency value from tb-ticks to microseconds.
>
> Fixes: commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> CEDE(0)")
>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Should you check for the zero case and print a warning?
> ---
> drivers/cpuidle/cpuidle-pseries.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index ff6d99e..9043358 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -361,7 +361,7 @@ static void __init fixup_cede0_latency(void)
> for (i = 0; i < nr_xcede_records; i++) {
> struct xcede_latency_record *record = &payload->records[i];
> u64 latency_tb = be64_to_cpu(record->latency_ticks);
> - u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
> + u64 latency_us = DIV_ROUND_UP_ULL(tb_to_ns(latency_tb), NSEC_PER_USEC);
>
> if (latency_us < min_latency_us)
> min_latency_us = latency_us;
> --
> 1.9.4
>
^ permalink raw reply
* Re: ptrace_syscall_32 is failing
From: Andy Lutomirski @ 2020-09-02 0:09 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-s390, linuxppc-dev, Vasily Gorbik, Brian Gerst,
Heiko Carstens, X86 ML, LKML, Christian Borntraeger,
Paul Mackerras, Catalin Marinas, Andy Lutomirski, Will Deacon,
linux-arm-kernel
In-Reply-To: <87k0xdjbtt.fsf@nanos.tec.linutronix.de>
On Tue, Sep 1, 2020 at 4:50 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sun, Aug 30 2020 at 08:52, Andy Lutomirski wrote:
> >> > [RUN] SYSCALL
> >> > [FAIL] Initial args are wrong (nr=29, args=0 0 0 0 0 4289172732)
> >> > [RUN] SYSCALL
> >> > [OK] Args after SIGUSR1 are correct (ax = -514)
> >> > [OK] Child got SIGUSR1
> >> > [RUN] Step again
> >> > [OK] pause(2) restarted correctly
> >>
> >> Bisected to commit 0b085e68f407 ("x86/entry: Consolidate 32/64 bit
> >> syscall entry").
> >> It looks like it is because syscall_enter_from_user_mode() is called
> >> before reading the 6th argument from the user stack.
>
> Bah.I don't know how I managed to miss that part and interestingly
> enough that none of the robots caught that either
>
> > Thomas, can we revert the syscall_enter() and syscall_exit() part of
> > the series?
>
> Hrm.
>
> > I think that they almost work for x86, but not quite as
> > indicated by this bug. Even if we imagine we can somehow hack around
> > this bug, I imagine we're going to find other problems with this
> > model, e.g. the potential upcoming exit problem I noted in my review.
>
> What's the upcoming problem?
If we ever want to get single-stepping fully correct across syscalls,
we might need to inject SIGTRAP on syscall return. This would be more
awkward if we can't run instrumentable code after the syscall part of
the syscall is done.
>
> > I really think the model should be:
> >
> > void do_syscall_whatever(...)
> > {
> > irqentry_enter(...);
> > instrumentation_begin();
> >
> > /* Do whatever arch ABI oddities are needed on entry. */
> >
> > Then either:
> > syscall_begin(arch, nr, regs);
> > dispatch the syscall;
> > syscall_end(arch, nr, regs);
> >
> > Or just:
> > generic_do_syscall(arch, nr, regs);
> >
> > /* Do whatever arch ABI oddities are needed on exit from the syscall. */
> >
> > instrumentation_end();
> > irqentry_exit(...);
> > }
>
> I don't think we want that in general. The current variant is perfectly
> fine for everything except the 32bit fast syscall nonsense. Also
> irqentry_entry/exit is not equivalent to the syscall_enter/exit
> counterparts.
If there are any architectures in which actual work is needed to
figure out whether something is a syscall in the first place, they'll
want to do the usual kernel entry work before the syscall entry work.
Maybe your patch actually makes this possible -- I haven't digested
all the details yet.
Who advised you to drop the arch parameter?
> ---
> arch/x86/entry/common.c | 29 ++++++++++++++++--------
> include/linux/entry-common.h | 51 +++++++++++++++++++++++++++++++++++--------
> kernel/entry/common.c | 35 ++++++++++++++++++++++++-----
> 3 files changed, 91 insertions(+), 24 deletions(-)
>
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -60,16 +60,10 @@
> #if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
> static __always_inline unsigned int syscall_32_enter(struct pt_regs *regs)
> {
> - unsigned int nr = (unsigned int)regs->orig_ax;
> -
> if (IS_ENABLED(CONFIG_IA32_EMULATION))
> current_thread_info()->status |= TS_COMPAT;
> - /*
> - * Subtlety here: if ptrace pokes something larger than 2^32-1 into
> - * orig_ax, the unsigned int return value truncates it. This may
> - * or may not be necessary, but it matches the old asm behavior.
> - */
> - return (unsigned int)syscall_enter_from_user_mode(regs, nr);
> +
> + return (unsigned int)regs->orig_ax;
> }
>
> /*
> @@ -91,15 +85,29 @@ static __always_inline void do_syscall_3
> {
> unsigned int nr = syscall_32_enter(regs);
>
> + /*
> + * Subtlety here: if ptrace pokes something larger than 2^32-1 into
> + * orig_ax, the unsigned int return value truncates it. This may
> + * or may not be necessary, but it matches the old asm behavior.
> + */
> + nr = (unsigned int)syscall_enter_from_user_mode(regs, nr);
> +
> do_syscall_32_irqs_on(regs, nr);
> syscall_exit_to_user_mode(regs);
> }
>
> static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
> {
> - unsigned int nr = syscall_32_enter(regs);
> + unsigned int nr = syscall_32_enter(regs);
> int res;
>
> + /*
> + * This cannot use syscall_enter_from_user_mode() as it has to
> + * fetch EBP before invoking any of the syscall entry work
> + * functions.
> + */
> + syscall_enter_from_user_mode_prepare(regs);
I'm getting lost in all these "enter" functions...
^ permalink raw reply
* [PATCH v3] powerpc: Warn about use of smt_snooze_delay
From: Joel Stanley @ 2020-09-02 0:00 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Gautham R . Shenoy
It's not done anything for a long time. Save the percpu variable, and
emit a warning to remind users to not expect it to do anything.
This uses pr_warn_once instead of pr_warn_ratelimit as testing
'ppc64_cpu --smt=off' on a 24 core / 4 SMT system showed the warning to
be noisy, as the online/offline loop is slow.
Fixes: 3fa8cad82b94 ("powerpc/pseries/cpuidle: smt-snooze-delay cleanup.")
Cc: stable@vger.kernel.org # v3.14
Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
--
v3:
pr_warn_once instead of pr_warn_ratelimited
Update meessages with mpe's suggestions
v2:
Use pr_warn instead of WARN
Reword and print proccess name with pid in message
Leave CPU_FTR_SMT test in
---
arch/powerpc/kernel/sysfs.c | 42 +++++++++++++++----------------------
1 file changed, 17 insertions(+), 25 deletions(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 46b4ebc33db7..5dea98fa2f93 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -32,29 +32,27 @@
static DEFINE_PER_CPU(struct cpu, cpu_devices);
-/*
- * SMT snooze delay stuff, 64-bit only for now
- */
-
#ifdef CONFIG_PPC64
-/* Time in microseconds we delay before sleeping in the idle loop */
-static DEFINE_PER_CPU(long, smt_snooze_delay) = { 100 };
+/*
+ * Snooze delay has not been hooked up since 3fa8cad82b94 ("powerpc/pseries/cpuidle:
+ * smt-snooze-delay cleanup.") and has been broken even longer. As was foretold in
+ * 2014:
+ *
+ * "ppc64_util currently utilises it. Once we fix ppc64_util, propose to clean
+ * up the kernel code."
+ *
+ * powerpc-utils stopped using it as of 1.3.8. At some point in the future this
+ * code should be removed.
+ */
static ssize_t store_smt_snooze_delay(struct device *dev,
struct device_attribute *attr,
const char *buf,
size_t count)
{
- struct cpu *cpu = container_of(dev, struct cpu, dev);
- ssize_t ret;
- long snooze;
-
- ret = sscanf(buf, "%ld", &snooze);
- if (ret != 1)
- return -EINVAL;
-
- per_cpu(smt_snooze_delay, cpu->dev.id) = snooze;
+ pr_warn_once("%s (%d) stored to unsupported smt_snooze_delay, which has no effect.\n",
+ current->comm, current->pid);
return count;
}
@@ -62,9 +60,9 @@ static ssize_t show_smt_snooze_delay(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct cpu *cpu = container_of(dev, struct cpu, dev);
-
- return sprintf(buf, "%ld\n", per_cpu(smt_snooze_delay, cpu->dev.id));
+ pr_warn_once("%s (%d) read from unsupported smt_snooze_delay\n",
+ current->comm, current->pid);
+ return sprintf(buf, "100\n");
}
static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,
@@ -72,16 +70,10 @@ static DEVICE_ATTR(smt_snooze_delay, 0644, show_smt_snooze_delay,
static int __init setup_smt_snooze_delay(char *str)
{
- unsigned int cpu;
- long snooze;
-
if (!cpu_has_feature(CPU_FTR_SMT))
return 1;
- snooze = simple_strtol(str, NULL, 10);
- for_each_possible_cpu(cpu)
- per_cpu(smt_snooze_delay, cpu) = snooze;
-
+ pr_warn("smt-snooze-delay command line option has no effect\n");
return 1;
}
__setup("smt-snooze-delay=", setup_smt_snooze_delay);
--
2.28.0
^ permalink raw reply related
* Re: ptrace_syscall_32 is failing
From: Thomas Gleixner @ 2020-09-01 23:50 UTC (permalink / raw)
To: Andy Lutomirski, Brian Gerst
Cc: linux-s390, linuxppc-dev, Catalin Marinas, Vasily Gorbik,
Heiko Carstens, X86 ML, LKML, Christian Borntraeger,
Paul Mackerras, Andy Lutomirski, Will Deacon, linux-arm-kernel
In-Reply-To: <CALCETrXY1x0MReMoTOG2awcZvr4c7gp99JVNthK37vUUk-kyew@mail.gmail.com>
On Sun, Aug 30 2020 at 08:52, Andy Lutomirski wrote:
>> > [RUN] SYSCALL
>> > [FAIL] Initial args are wrong (nr=29, args=0 0 0 0 0 4289172732)
>> > [RUN] SYSCALL
>> > [OK] Args after SIGUSR1 are correct (ax = -514)
>> > [OK] Child got SIGUSR1
>> > [RUN] Step again
>> > [OK] pause(2) restarted correctly
>>
>> Bisected to commit 0b085e68f407 ("x86/entry: Consolidate 32/64 bit
>> syscall entry").
>> It looks like it is because syscall_enter_from_user_mode() is called
>> before reading the 6th argument from the user stack.
Bah.I don't know how I managed to miss that part and interestingly
enough that none of the robots caught that either
> Thomas, can we revert the syscall_enter() and syscall_exit() part of
> the series?
Hrm.
> I think that they almost work for x86, but not quite as
> indicated by this bug. Even if we imagine we can somehow hack around
> this bug, I imagine we're going to find other problems with this
> model, e.g. the potential upcoming exit problem I noted in my review.
What's the upcoming problem?
> I really think the model should be:
>
> void do_syscall_whatever(...)
> {
> irqentry_enter(...);
> instrumentation_begin();
>
> /* Do whatever arch ABI oddities are needed on entry. */
>
> Then either:
> syscall_begin(arch, nr, regs);
> dispatch the syscall;
> syscall_end(arch, nr, regs);
>
> Or just:
> generic_do_syscall(arch, nr, regs);
>
> /* Do whatever arch ABI oddities are needed on exit from the syscall. */
>
> instrumentation_end();
> irqentry_exit(...);
> }
I don't think we want that in general. The current variant is perfectly
fine for everything except the 32bit fast syscall nonsense. Also
irqentry_entry/exit is not equivalent to the syscall_enter/exit
counterparts.
> x86 has an ABI oddity needed on entry: this fast syscall argument
> fixup. We also might end up with ABI oddities on exit if we ever try
> to make single-stepping of syscalls work fully correctly. x86 sort of
> gets away without specifying arch because the arch helpers that get
> called for audit, etc can deduce the arch, but this is kind of gross.
> I suppose we could omit arch as an explicit parameter.
I had that in one of the early versions and was advised to drop that.
> Or I suppose we could try to rejigger the API in time for 5.9.
> Fortunately only x86 uses the new APIs so far. I cc'd a bunch of
> other arch maintainers to see if other architectures fit well in the
> new syscall_enter() model, but I feel like the fact that x86 is
> already broken indicates that we messed it up a bit.
It's not unfixable and the fix is close to what you suggested above
except that it preserves the straight forward stuff for the !32bit fast
syscall case. Completely untested patch below. I run proper tests
tomorrow with brain awake.
Thanks,
tglx
---
arch/x86/entry/common.c | 29 ++++++++++++++++--------
include/linux/entry-common.h | 51 +++++++++++++++++++++++++++++++++++--------
kernel/entry/common.c | 35 ++++++++++++++++++++++++-----
3 files changed, 91 insertions(+), 24 deletions(-)
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -60,16 +60,10 @@
#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
static __always_inline unsigned int syscall_32_enter(struct pt_regs *regs)
{
- unsigned int nr = (unsigned int)regs->orig_ax;
-
if (IS_ENABLED(CONFIG_IA32_EMULATION))
current_thread_info()->status |= TS_COMPAT;
- /*
- * Subtlety here: if ptrace pokes something larger than 2^32-1 into
- * orig_ax, the unsigned int return value truncates it. This may
- * or may not be necessary, but it matches the old asm behavior.
- */
- return (unsigned int)syscall_enter_from_user_mode(regs, nr);
+
+ return (unsigned int)regs->orig_ax;
}
/*
@@ -91,15 +85,29 @@ static __always_inline void do_syscall_3
{
unsigned int nr = syscall_32_enter(regs);
+ /*
+ * Subtlety here: if ptrace pokes something larger than 2^32-1 into
+ * orig_ax, the unsigned int return value truncates it. This may
+ * or may not be necessary, but it matches the old asm behavior.
+ */
+ nr = (unsigned int)syscall_enter_from_user_mode(regs, nr);
+
do_syscall_32_irqs_on(regs, nr);
syscall_exit_to_user_mode(regs);
}
static noinstr bool __do_fast_syscall_32(struct pt_regs *regs)
{
- unsigned int nr = syscall_32_enter(regs);
+ unsigned int nr = syscall_32_enter(regs);
int res;
+ /*
+ * This cannot use syscall_enter_from_user_mode() as it has to
+ * fetch EBP before invoking any of the syscall entry work
+ * functions.
+ */
+ syscall_enter_from_user_mode_prepare(regs);
+
instrumentation_begin();
/* Fetch EBP from where the vDSO stashed it. */
if (IS_ENABLED(CONFIG_X86_64)) {
@@ -122,6 +130,9 @@ static noinstr bool __do_fast_syscall_32
return false;
}
+ /* The case truncates any ptrace induced syscall nr > 2^32 -1 */
+ nr = (unsigned int)syscall_enter_from_user_mode_work(regs, nr);
+
/* Now this is just like a normal syscall. */
do_syscall_32_irqs_on(regs, nr);
syscall_exit_to_user_mode(regs);
--- a/include/linux/entry-common.h
+++ b/include/linux/entry-common.h
@@ -110,15 +110,30 @@ static inline __must_check int arch_sysc
#endif
/**
- * syscall_enter_from_user_mode - Check and handle work before invoking
- * a syscall
+ * syscall_enter_from_user_mode_prepare - Establish state and enable interrupts
* @regs: Pointer to currents pt_regs
- * @syscall: The syscall number
*
* Invoked from architecture specific syscall entry code with interrupts
* disabled. The calling code has to be non-instrumentable. When the
- * function returns all state is correct and the subsequent functions can be
- * instrumented.
+ * function returns all state is correct, interrupts are enabled and the
+ * subsequent functions can be instrumented.
+ *
+ * This handles lockdep, RCU (context tracking) and tracing state.
+ *
+ * This is invoked when there is extra architecture specific functionality
+ * to be done between establishing state and handling user mode entry work.
+ */
+void syscall_enter_from_user_mode_prepare(struct pt_regs *regs);
+
+/**
+ * syscall_enter_from_user_mode_work - Check and handle work before invoking
+ * a syscall
+ * @regs: Pointer to currents pt_regs
+ * @syscall: The syscall number
+ *
+ * Invoked from architecture specific syscall entry code with interrupts
+ * enabled after invoking syscall_enter_from_user_mode_prepare() and extra
+ * architecture specific work.
*
* Returns: The original or a modified syscall number
*
@@ -127,12 +142,30 @@ static inline __must_check int arch_sysc
* syscall_set_return_value() first. If neither of those are called and -1
* is returned, then the syscall will fail with ENOSYS.
*
- * The following functionality is handled here:
+ * It handles the following work items:
*
- * 1) Establish state (lockdep, RCU (context tracking), tracing)
- * 2) TIF flag dependent invocations of arch_syscall_enter_tracehook(),
+ * 1) TIF flag dependent invocations of arch_syscall_enter_tracehook(),
* __secure_computing(), trace_sys_enter()
- * 3) Invocation of audit_syscall_entry()
+ * 2) Invocation of audit_syscall_entry()
+ */
+long syscall_enter_from_user_mode_work(struct pt_regs *regs, long syscall);
+
+/**
+ * syscall_enter_from_user_mode - Establish state and check and handle work
+ * before invoking a syscall
+ * @regs: Pointer to currents pt_regs
+ * @syscall: The syscall number
+ *
+ * Invoked from architecture specific syscall entry code with interrupts
+ * disabled. The calling code has to be non-instrumentable. When the
+ * function returns all state is correct, interrupts are enabled and the
+ * subsequent functions can be instrumented.
+ *
+ * This is combination of syscall_enter_from_user_mode_prepare() and
+ * syscall_enter_from_user_mode_work().
+ *
+ * Returns: The original or a modified syscall number. See
+ * syscall_enter_from_user_mode_work() for further explanation.
*/
long syscall_enter_from_user_mode(struct pt_regs *regs, long syscall);
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -68,22 +68,45 @@ static long syscall_trace_enter(struct p
return ret ? : syscall;
}
-noinstr long syscall_enter_from_user_mode(struct pt_regs *regs, long syscall)
+static __always_inline long
+__syscall_enter_from_user_work(struct pt_regs *regs, long syscall)
{
unsigned long ti_work;
- enter_from_user_mode(regs);
- instrumentation_begin();
-
- local_irq_enable();
ti_work = READ_ONCE(current_thread_info()->flags);
if (ti_work & SYSCALL_ENTER_WORK)
syscall = syscall_trace_enter(regs, syscall, ti_work);
- instrumentation_end();
return syscall;
}
+long syscall_enter_from_user_mode_work(struct pt_regs *regs, long syscall)
+{
+ return __syscall_enter_from_user_work(regs, syscall);
+}
+
+noinstr long syscall_enter_from_user_mode(struct pt_regs *regs, long syscall)
+{
+ long ret;
+
+ enter_from_user_mode(regs);
+
+ instrumentation_begin();
+ local_irq_enable();
+ ret = __syscall_enter_from_user_work(regs, syscall);
+ instrumentation_end();
+
+ return ret;
+}
+
+noinstr void syscall_enter_from_user_mode_prepare(struct pt_regs *regs)
+{
+ enter_from_user_mode(regs);
+ instrumentation_begin();
+ local_irq_enable();
+ instrumentation_end();
+}
+
/**
* exit_to_user_mode - Fixup state when exiting to user mode
*
^ permalink raw reply
* Re: fsl_espi errors on v5.7.15
From: Chris Packham @ 2020-09-01 23:31 UTC (permalink / raw)
To: Nicholas Piggin, benh@kernel.crashing.org, broonie@kernel.org,
Heiner Kallweit, mpe@ellerman.id.au, paulus@samba.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-spi@vger.kernel.org
In-Reply-To: <bfaaa982-33b9-c427-48a4-ddf9dd35e7b9@alliedtelesis.co.nz>
On 2/09/20 11:29 am, Chris Packham wrote:
>
> On 1/09/20 6:14 pm, Nicholas Piggin wrote:
>> Excerpts from Chris Packham's message of September 1, 2020 11:25 am:
>>> On 1/09/20 12:33 am, Heiner Kallweit wrote:
>>>> On 30.08.2020 23:59, Chris Packham wrote:
>>>>> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>>>>>> On 30.08.2020 23:00, Chris Packham wrote:
>>>>>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>>>>>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>>>>>>> <snip>
>>>>>>>
>>>>>>>>>>>>> I've also now seen the RX FIFO not empty error on the
>>>>>>>>>>>>> T2080RDB
>>>>>>>>>>>>>
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's
>>>>>>>>>>>>> aren't empty!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>>>
>>>>>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>>>>>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra
>>>>>>>>>>>>> byte in the
>>>>>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a
>>>>>>>>>>>>> READ_FSR.
>>>>>>>>>>>>>
>>>>>>>>>>>>> fsl_espi ffe110000.spi: tx 70
>>>>>>>>>>>>> fsl_espi ffe110000.spi: rx 03
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's
>>>>>>>>>>>>> aren't empty!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's
>>>>>>>>>>>>> aren't empty!
>>>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>>>>>
>>>>>>>>>>>>> From all the Micron SPI-NOR datasheets I've got
>>>>>>>>>>>>> access to it is
>>>>>>>>>>>>> possible to continually read the SR/FSR. But I've no idea
>>>>>>>>>>>>> why it
>>>>>>>>>>>>> happens some times and not others.
>>>>>>>>>>>> So I think I've got a reproduction and I think I've
>>>>>>>>>>>> bisected the problem
>>>>>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft
>>>>>>>>>>>> interrupt replay in
>>>>>>>>>>>> C"). My day is just finishing now so I haven't applied too
>>>>>>>>>>>> much scrutiny
>>>>>>>>>>>> to this result. Given the various rabbit holes I've been
>>>>>>>>>>>> down on this
>>>>>>>>>>>> issue already I'd take this information with a good degree
>>>>>>>>>>>> of skepticism.
>>>>>>>>>>>>
>>>>>>>>>>> OK, so an easy test should be to re-test with a 5.4 kernel.
>>>>>>>>>>> It doesn't have yet the change you're referring to, and the
>>>>>>>>>>> fsl-espi driver
>>>>>>>>>>> is basically the same as in 5.7 (just two small changes in
>>>>>>>>>>> 5.7).
>>>>>>>>>> There's 6cc0c16d82f88 and maybe also other interrupt related
>>>>>>>>>> patches
>>>>>>>>>> around this time that could affect book E, so it's good if
>>>>>>>>>> that exact
>>>>>>>>>> patch is confirmed.
>>>>>>>>> My confirmation is basically that I can induce the issue in a
>>>>>>>>> 5.4 kernel
>>>>>>>>> by cherry-picking 3282a3da25bd. I'm also able to "fix" the
>>>>>>>>> issue in
>>>>>>>>> 5.9-rc2 by reverting that one commit.
>>>>>>>>>
>>>>>>>>> I both cases it's not exactly a clean cherry-pick/revert so I
>>>>>>>>> also
>>>>>>>>> confirmed the bisection result by building at 3282a3da25bd
>>>>>>>>> (which sees
>>>>>>>>> the issue) and the commit just before (which does not).
>>>>>>>> Thanks for testing, that confirms it well.
>>>>>>>>
>>>>>>>> [snip patch]
>>>>>>>>
>>>>>>>>> I still saw the issue with this change applied.
>>>>>>>>> PPC_IRQ_SOFT_MASK_DEBUG
>>>>>>>>> didn't report anything (either with or without the change above).
>>>>>>>> Okay, it was a bit of a shot in the dark. I still can't see what
>>>>>>>> else has changed.
>>>>>>>>
>>>>>>>> What would cause this, a lost interrupt? A spurious interrupt? Or
>>>>>>>> higher interrupt latency?
>>>>>>>>
>>>>>>>> I don't think the patch should cause significantly worse latency,
>>>>>>>> (it's supposed to be a bit better if anything because it
>>>>>>>> doesn't set
>>>>>>>> up the full interrupt frame). But it's possible.
>>>>>>> My working theory is that the SPI_DON indication is all about
>>>>>>> the TX
>>>>>>> direction an now that the interrupts are faster we're hitting an
>>>>>>> error
>>>>>>> because there is still RX activity going on. Heiner disagrees
>>>>>>> with my
>>>>>>> interpretation of the SPI_DON indication and the fact that it
>>>>>>> doesn't
>>>>>>> happen every time does throw doubt on it.
>>>>>>>
>>>>>> It's right that the eSPI spec can be interpreted that SPI_DON
>>>>>> refers to
>>>>>> TX only. However this wouldn't really make sense, because also
>>>>>> for RX
>>>>>> we program the frame length, and therefore want to be notified
>>>>>> once the
>>>>>> full frame was received. Also practical experience shows that
>>>>>> SPI_DON
>>>>>> is set also after RX-only transfers.
>>>>>> Typical SPI NOR use case is that you write read command + start
>>>>>> address,
>>>>>> followed by a longer read. If the TX-only interpretation would be
>>>>>> right,
>>>>>> we'd always end up with SPI_DON not being set.
>>>>>>
>>>>>>> I can't really explain the extra RX byte in the fifo. We know
>>>>>>> how many
>>>>>>> bytes to expect and we pull that many from the fifo so it's not
>>>>>>> as if
>>>>>>> we're missing an interrupt causing us to skip the last byte.
>>>>>>> I've been
>>>>>>> looking for some kind of off-by-one calculation but again if it
>>>>>>> were
>>>>>>> something like that it'd happen all the time.
>>>>>>>
>>>>>> Maybe it helps to know what value this extra byte in the FIFO
>>>>>> has. Is it:
>>>>>> - a duplicate of the last read byte
>>>>>> - or the next byte (at <end address> + 1)
>>>>>> - or a fixed value, e.g. always 0x00 or 0xff
>>>>> The values were up thread a bit but I'll repeat them here
>>>>>
>>>>> fsl_espi ffe110000.spi: tx 70
>>>>> fsl_espi ffe110000.spi: rx 03
>>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>> fsl_espi ffe110000.spi: tx 05
>>>>> fsl_espi ffe110000.spi: rx 00
>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>> fsl_espi ffe110000.spi: tx 05
>>>>> fsl_espi ffe110000.spi: rx 00
>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>
>>>>>
>>>>> The rx 00 Extra RX 03 is a bit concerning. I've only ever seen
>>>>> them with
>>>>> either a READ_SR or a READ_FSR. Never a data read.
>>>>>
>>>> Just remembered something about SPIE_DON:
>>>> Transfers are always full duplex, therefore in case of a read the chip
>>>> sends dummy zero's. Having said that in case of a read SPIE_DON means
>>>> that the last dummy zero was shifted out.
>>>>
>>>> READ_SR and READ_FSR are the shortest transfers, 1 byte out and 1
>>>> byte in.
>>>> So the issue may have a dependency on the length of the transfer.
>>>> However I see no good explanation so far. You can try adding a
>>>> delay of
>>>> a few miroseconds between the following to commands in
>>>> fsl_espi_bufs().
>>>>
>>>> fsl_espi_write_reg(espi, ESPI_SPIM, mask);
>>>>
>>>> /* Prevent filling the fifo from getting interrupted */
>>>> spin_lock_irq(&espi->lock);
>>>>
>>>> Maybe enabling interrupts and seeing the SPIE_DON interrupt are too
>>>> close.
>>> I think this might be heading in the right direction. Playing about
>>> with
>>> a delay does seem to make the two symptoms less likely. Although I have
>>> to set it quite high (i.e. msleep(100)) to completely avoid any
>>> possibility of seeing either message.
>> The patch might replay the interrupt a little bit faster, but it would
>> be a few microseconds at most I think (just from improved code).
>>
>> Would you be able to ftrace the interrupt handler function and see if
>> you
>> can see a difference in number or timing of interrupts? I'm at a bit of
>> a loss.
> This is getting really weird. I was setting up to run with ftrace and
> found I couldn't reproduce it on the tip of Linus's tree (currently
> pointing at e7a522c83b86). But I swear I could last week. Sure enough
> if I checkout 5.9-rc2 (or 5.7.15) I can reproduce the problem again.
*Sigh* my master branch still has the interrupt changes reverted.
^ permalink raw reply
* Re: fsl_espi errors on v5.7.15
From: Chris Packham @ 2020-09-01 23:29 UTC (permalink / raw)
To: Nicholas Piggin, benh@kernel.crashing.org, broonie@kernel.org,
Heiner Kallweit, mpe@ellerman.id.au, paulus@samba.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-spi@vger.kernel.org
In-Reply-To: <1598940515.6e06nwgi0c.astroid@bobo.none>
On 1/09/20 6:14 pm, Nicholas Piggin wrote:
> Excerpts from Chris Packham's message of September 1, 2020 11:25 am:
>> On 1/09/20 12:33 am, Heiner Kallweit wrote:
>>> On 30.08.2020 23:59, Chris Packham wrote:
>>>> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>>>>> On 30.08.2020 23:00, Chris Packham wrote:
>>>>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>>>>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>>>>>> <snip>
>>>>>>
>>>>>>>>>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>>>>>>>>>>
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>>
>>>>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>>>>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra byte in the
>>>>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>>>>>>>>>>
>>>>>>>>>>>> fsl_espi ffe110000.spi: tx 70
>>>>>>>>>>>> fsl_espi ffe110000.spi: rx 03
>>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>> fsl_espi ffe110000.spi: tx 05
>>>>>>>>>>>> fsl_espi ffe110000.spi: rx 00
>>>>>>>>>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>>>>>>>>>
>>>>>>>>>>>> From all the Micron SPI-NOR datasheets I've got access to it is
>>>>>>>>>>>> possible to continually read the SR/FSR. But I've no idea why it
>>>>>>>>>>>> happens some times and not others.
>>>>>>>>>>> So I think I've got a reproduction and I think I've bisected the problem
>>>>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay in
>>>>>>>>>>> C"). My day is just finishing now so I haven't applied too much scrutiny
>>>>>>>>>>> to this result. Given the various rabbit holes I've been down on this
>>>>>>>>>>> issue already I'd take this information with a good degree of skepticism.
>>>>>>>>>>>
>>>>>>>>>> OK, so an easy test should be to re-test with a 5.4 kernel.
>>>>>>>>>> It doesn't have yet the change you're referring to, and the fsl-espi driver
>>>>>>>>>> is basically the same as in 5.7 (just two small changes in 5.7).
>>>>>>>>> There's 6cc0c16d82f88 and maybe also other interrupt related patches
>>>>>>>>> around this time that could affect book E, so it's good if that exact
>>>>>>>>> patch is confirmed.
>>>>>>>> My confirmation is basically that I can induce the issue in a 5.4 kernel
>>>>>>>> by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in
>>>>>>>> 5.9-rc2 by reverting that one commit.
>>>>>>>>
>>>>>>>> I both cases it's not exactly a clean cherry-pick/revert so I also
>>>>>>>> confirmed the bisection result by building at 3282a3da25bd (which sees
>>>>>>>> the issue) and the commit just before (which does not).
>>>>>>> Thanks for testing, that confirms it well.
>>>>>>>
>>>>>>> [snip patch]
>>>>>>>
>>>>>>>> I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG
>>>>>>>> didn't report anything (either with or without the change above).
>>>>>>> Okay, it was a bit of a shot in the dark. I still can't see what
>>>>>>> else has changed.
>>>>>>>
>>>>>>> What would cause this, a lost interrupt? A spurious interrupt? Or
>>>>>>> higher interrupt latency?
>>>>>>>
>>>>>>> I don't think the patch should cause significantly worse latency,
>>>>>>> (it's supposed to be a bit better if anything because it doesn't set
>>>>>>> up the full interrupt frame). But it's possible.
>>>>>> My working theory is that the SPI_DON indication is all about the TX
>>>>>> direction an now that the interrupts are faster we're hitting an error
>>>>>> because there is still RX activity going on. Heiner disagrees with my
>>>>>> interpretation of the SPI_DON indication and the fact that it doesn't
>>>>>> happen every time does throw doubt on it.
>>>>>>
>>>>> It's right that the eSPI spec can be interpreted that SPI_DON refers to
>>>>> TX only. However this wouldn't really make sense, because also for RX
>>>>> we program the frame length, and therefore want to be notified once the
>>>>> full frame was received. Also practical experience shows that SPI_DON
>>>>> is set also after RX-only transfers.
>>>>> Typical SPI NOR use case is that you write read command + start address,
>>>>> followed by a longer read. If the TX-only interpretation would be right,
>>>>> we'd always end up with SPI_DON not being set.
>>>>>
>>>>>> I can't really explain the extra RX byte in the fifo. We know how many
>>>>>> bytes to expect and we pull that many from the fifo so it's not as if
>>>>>> we're missing an interrupt causing us to skip the last byte. I've been
>>>>>> looking for some kind of off-by-one calculation but again if it were
>>>>>> something like that it'd happen all the time.
>>>>>>
>>>>> Maybe it helps to know what value this extra byte in the FIFO has. Is it:
>>>>> - a duplicate of the last read byte
>>>>> - or the next byte (at <end address> + 1)
>>>>> - or a fixed value, e.g. always 0x00 or 0xff
>>>> The values were up thread a bit but I'll repeat them here
>>>>
>>>> fsl_espi ffe110000.spi: tx 70
>>>> fsl_espi ffe110000.spi: rx 03
>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>> fsl_espi ffe110000.spi: tx 05
>>>> fsl_espi ffe110000.spi: rx 00
>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>> fsl_espi ffe110000.spi: tx 05
>>>> fsl_espi ffe110000.spi: rx 00
>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>
>>>>
>>>> The rx 00 Extra RX 03 is a bit concerning. I've only ever seen them with
>>>> either a READ_SR or a READ_FSR. Never a data read.
>>>>
>>> Just remembered something about SPIE_DON:
>>> Transfers are always full duplex, therefore in case of a read the chip
>>> sends dummy zero's. Having said that in case of a read SPIE_DON means
>>> that the last dummy zero was shifted out.
>>>
>>> READ_SR and READ_FSR are the shortest transfers, 1 byte out and 1 byte in.
>>> So the issue may have a dependency on the length of the transfer.
>>> However I see no good explanation so far. You can try adding a delay of
>>> a few miroseconds between the following to commands in fsl_espi_bufs().
>>>
>>> fsl_espi_write_reg(espi, ESPI_SPIM, mask);
>>>
>>> /* Prevent filling the fifo from getting interrupted */
>>> spin_lock_irq(&espi->lock);
>>>
>>> Maybe enabling interrupts and seeing the SPIE_DON interrupt are too close.
>> I think this might be heading in the right direction. Playing about with
>> a delay does seem to make the two symptoms less likely. Although I have
>> to set it quite high (i.e. msleep(100)) to completely avoid any
>> possibility of seeing either message.
> The patch might replay the interrupt a little bit faster, but it would
> be a few microseconds at most I think (just from improved code).
>
> Would you be able to ftrace the interrupt handler function and see if you
> can see a difference in number or timing of interrupts? I'm at a bit of
> a loss.
This is getting really weird. I was setting up to run with ftrace and
found I couldn't reproduce it on the tip of Linus's tree (currently
pointing at e7a522c83b86). But I swear I could last week. Sure enough if
I checkout 5.9-rc2 (or 5.7.15) I can reproduce the problem again.
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH v1 02/10] powerpc/kernel/iommu: Align size for IOMMU_PAGE_SIZE on iommu_*_coherent()
From: Leonardo Bras @ 2020-09-01 22:34 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <81f106bd-8962-22f2-f14a-378d3486f57e@ozlabs.ru>
On Mon, 2020-08-31 at 10:47 +1000, Alexey Kardashevskiy wrote:
> >
> > Maybe testing with host 64k pagesize and IOMMU 16MB pagesize in qemu
> > should be enough, is there any chance to get indirect mapping in qemu
> > like this? (DDW but with smaller DMA window available)
>
> You will have to hack the guest kernel to always do indirect mapping or
> hack QEMU's rtas_ibm_query_pe_dma_window() to return a small number of
> available TCEs. But you will be testing QEMU/KVM which behave quite
> differently to pHyp in this particular case.
>
As you suggested before, building for 4k cpu pagesize should be the
best approach. It would allow testing for both pHyp and qemu scenarios.
> > > > > Because if we want the former (==support), then we'll have to align the
> > > > > size up to the bigger page size when allocating/zeroing system pages,
> > > > > etc.
> > > >
> > > > This part I don't understand. Why do we need to align everything to the
> > > > bigger pagesize?
> > > >
> > > > I mean, is not that enough that the range [ret, ret + size[ is both
> > > > allocated by mm and mapped on a iommu range?
> > > >
> > > > Suppose a iommu_alloc_coherent() of 16kB on PAGESIZE = 4k and
> > > > IOMMU_PAGE_SIZE() == 64k.
> > > > Why 4 * cpu_pages mapped by a 64k IOMMU page is not enough?
> > > > All the space the user asked for is allocated and mapped for DMA.
> > >
> > > The user asked to map 16K, the rest - 48K - is used for something else
> > > (may be even mapped to another device) but you are making all 64K
> > > accessible by the device which only should be able to access 16K.
> > >
> > > In practice, if this happens, H_PUT_TCE will simply fail.
> >
> > I have noticed mlx5 driver getting a few bytes in a buffer, and using
> > iommu_map_page(). It does map a whole page for as few bytes as the user
>
> Whole 4K system page or whole 64K iommu page?
I tested it in 64k system page + 64k iommu page.
The 64K system page may be used for anything, and a small portion of it
(say 128 bytes) needs to be used for DMA.
The whole page is mapped by IOMMU, and the driver gets info of the
memory range it should access / modify.
>
> > wants mapped, and the other bytes get used for something else, or just
> > mapped on another DMA page.
> > It seems to work fine.
>
>
> With 4K system page and 64K IOMMU page? In practice it would take an
> effort or/and bad luck to see it crashing. Thanks,
I haven't tested it yet. On a 64k system page and 4k/64k iommu page, it
works as described above.
I am new to this, so I am trying to understand how a memory page mapped
as DMA, and used for something else could be a problem.
Thanks!
>
> > >
> > > > > Bigger pages are not the case here as I understand it.
> > > >
> > > > I did not get this part, what do you mean?
> > >
> > > Possible IOMMU page sizes are 4K, 64K, 2M, 16M, 256M, 1GB, and the
> > > supported set of sizes is different for P8/P9 and type of IO (PHB,
> > > NVLink/CAPI).
> > >
> > >
> > > > > > Update those functions to guarantee alignment with requested size
> > > > > > using IOMMU_PAGE_ALIGN() before doing iommu_alloc() / iommu_free().
> > > > > >
> > > > > > Also, on iommu_range_alloc(), replace ALIGN(n, 1 << tbl->it_page_shift)
> > > > > > with IOMMU_PAGE_ALIGN(n, tbl), which seems easier to read.
> > > > > >
> > > > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > > > > ---
> > > > > > arch/powerpc/kernel/iommu.c | 17 +++++++++--------
> > > > > > 1 file changed, 9 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> > > > > > index 9704f3f76e63..d7086087830f 100644
> > > > > > --- a/arch/powerpc/kernel/iommu.c
> > > > > > +++ b/arch/powerpc/kernel/iommu.c
> > > > > > @@ -237,10 +237,9 @@ static unsigned long iommu_range_alloc(struct device *dev,
> > > > > > }
> > > > > >
> > > > > > if (dev)
> > > > > > - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
> > > > > > - 1 << tbl->it_page_shift);
> > > > > > + boundary_size = IOMMU_PAGE_ALIGN(dma_get_seg_boundary(dev) + 1, tbl);
> > > > >
> > > > > Run checkpatch.pl, should complain about a long line.
> > > >
> > > > It's 86 columns long, which is less than the new limit of 100 columns
> > > > Linus announced a few weeks ago. checkpatch.pl was updated too:
> > > > https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
> > >
> > > Yay finally :) Thanks,
> >
> > :)
> >
> > >
> > > > > > else
> > > > > > - boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
> > > > > > + boundary_size = IOMMU_PAGE_ALIGN(1UL << 32, tbl);
> > > > > > /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> > > > > >
> > > > > > n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
> > > > > > @@ -858,6 +857,7 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
> > > > > > unsigned int order;
> > > > > > unsigned int nio_pages, io_order;
> > > > > > struct page *page;
> > > > > > + size_t size_io = size;
> > > > > >
> > > > > > size = PAGE_ALIGN(size);
> > > > > > order = get_order(size);
> > > > > > @@ -884,8 +884,9 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
> > > > > > memset(ret, 0, size);
> > > > > >
> > > > > > /* Set up tces to cover the allocated range */
> > > > > > - nio_pages = size >> tbl->it_page_shift;
> > > > > > - io_order = get_iommu_order(size, tbl);
> > > > > > + size_io = IOMMU_PAGE_ALIGN(size_io, tbl);
> > > > > > + nio_pages = size_io >> tbl->it_page_shift;
> > > > > > + io_order = get_iommu_order(size_io, tbl);
> > > > > > mapping = iommu_alloc(dev, tbl, ret, nio_pages, DMA_BIDIRECTIONAL,
> > > > > > mask >> tbl->it_page_shift, io_order, 0);
> > > > > > if (mapping == DMA_MAPPING_ERROR) {
> > > > > > @@ -900,11 +901,11 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
> > > > > > void *vaddr, dma_addr_t dma_handle)
> > > > > > {
> > > > > > if (tbl) {
> > > > > > - unsigned int nio_pages;
> > > > > > + size_t size_io = IOMMU_PAGE_ALIGN(size, tbl);
> > > > > > + unsigned int nio_pages = size_io >> tbl->it_page_shift;
> > > > > >
> > > > > > - size = PAGE_ALIGN(size);
> > > > > > - nio_pages = size >> tbl->it_page_shift;
> > > > > > iommu_free(tbl, dma_handle, nio_pages);
> > > > > > +
> > > > >
> > > > > Unrelated new line.
> > > >
> > > > Will be removed. Thanks!
> > > >
> > > > > > size = PAGE_ALIGN(size);
> > > > > > free_pages((unsigned long)vaddr, get_order(size));
> > > > > > }
> > > > > >
^ permalink raw reply
* [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Nick Desaulniers @ 2020-09-01 22:25 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
Nick Desaulniers, linux-kernel, clang-built-linux, Paul Mackerras,
linuxppc-dev
In-Reply-To: <20200901222523.1941988-1-ndesaulniers@google.com>
Rather than invoke the compiler as the driver, use the linker. That way
we can check --orphan-handling=warn support correctly, as cc-ldoption
was removed in
commit 055efab3120b ("kbuild: drop support for cc-ldoption").
Requires dropping the .got section. I couldn't find how it was used in
the vdso32.
Fixes: commit f2af201002a8 ("powerpc/build: vdso linker warning for orphan sections")
Link: https://lore.kernel.org/lkml/CAKwvOdnn3wxYdJomvnveyD_njwRku3fABWT_bS92duihhywLJQ@mail.gmail.com/
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Not sure removing .got is a good idea or not. Otherwise I observe the
following link error:
powerpc-linux-gnu-ld: warning: orphan section `.got' from `arch/powerpc/kernel/vdso32/sigtramp.o' being placed in section `.got'
powerpc-linux-gnu-ld: _GLOBAL_OFFSET_TABLE_ not defined in linker created .got
powerpc-linux-gnu-ld: final link failed: bad value
sigtramp.c doesn't mention anything from the GOT AFAICT, and doesn't
look like it contains relocations that do, so I'm not sure where
references to _GLOBAL_OFFSET_TABLE_ are coming from.
arch/powerpc/kernel/vdso32/Makefile | 7 +++++--
arch/powerpc/kernel/vdso32/vdso32.lds.S | 3 ++-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
index 87ab1152d5ce..611a5951945a 100644
--- a/arch/powerpc/kernel/vdso32/Makefile
+++ b/arch/powerpc/kernel/vdso32/Makefile
@@ -27,6 +27,9 @@ UBSAN_SANITIZE := n
ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
-Wl,-soname=linux-vdso32.so.1 -Wl,--hash-style=both
asflags-y := -D__VDSO32__ -s
+ldflags-y := -shared -soname linux-vdso32.so.1 \
+ $(call ld-option, --eh-frame-hdr) \
+ $(call ld-option, --orphan-handling=warn) -T
obj-y += vdso32_wrapper.o
extra-y += vdso32.lds
@@ -49,8 +52,8 @@ $(obj-vdso32): %.o: %.S FORCE
$(call if_changed_dep,vdso32as)
# actual build commands
-quiet_cmd_vdso32ld = VDSO32L $@
- cmd_vdso32ld = $(VDSOCC) $(c_flags) $(CC32FLAGS) -o $@ $(call cc-ldoption, -Wl$(comma)--orphan-handling=warn) -Wl,-T$(filter %.lds,$^) $(filter %.o,$^)
+quiet_cmd_vdso32ld = LD $@
+ cmd_vdso32ld = $(cmd_ld)
quiet_cmd_vdso32as = VDSO32A $@
cmd_vdso32as = $(VDSOCC) $(a_flags) $(CC32FLAGS) -c -o $@ $<
diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S b/arch/powerpc/kernel/vdso32/vdso32.lds.S
index 4c985467a668..0ccdebad18b8 100644
--- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
+++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
@@ -61,7 +61,6 @@ SECTIONS
.fixup : { *(.fixup) }
.dynamic : { *(.dynamic) } :text :dynamic
- .got : { *(.got) } :text
.plt : { *(.plt) }
_end = .;
@@ -108,7 +107,9 @@ SECTIONS
.debug_varnames 0 : { *(.debug_varnames) }
/DISCARD/ : {
+ *(.got)
*(.note.GNU-stack)
+ *(.branch_lt)
*(.data .data.* .gnu.linkonce.d.* .sdata*)
*(.bss .sbss .dynbss .dynsbss)
*(.glink .iplt .plt .rela*)
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related
* [PATCH 1/2] powerpc/vdso64: link vdso64 with linker
From: Nick Desaulniers @ 2020-09-01 22:25 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
Nick Desaulniers, linux-kernel, clang-built-linux, Paul Mackerras,
linuxppc-dev
In-Reply-To: <20200901222523.1941988-1-ndesaulniers@google.com>
Rather than invoke the compiler as the driver, use the linker. That way
we can check --orphan-handling=warn support correctly, as cc-ldoption
was removed in
commit 055efab3120b ("kbuild: drop support for cc-ldoption").
Painstakingly compared the output between `objdump -a` before and after
this change. Now function symbols have the correct type of FUNC rather
than NONE, and the entry is slightly different (which doesn't matter for
the vdso). Binary size is the same.
Fixes: commit f2af201002a8 ("powerpc/build: vdso linker warning for orphan sections")
Link: https://lore.kernel.org/lkml/CAKwvOdnn3wxYdJomvnveyD_njwRku3fABWT_bS92duihhywLJQ@mail.gmail.com/
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
arch/powerpc/include/asm/vdso.h | 17 ++---------------
arch/powerpc/kernel/vdso64/Makefile | 8 ++++++--
arch/powerpc/kernel/vdso64/vdso64.lds.S | 1 -
3 files changed, 8 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/vdso.h b/arch/powerpc/include/asm/vdso.h
index 2ff884853f97..11b2ecf49f79 100644
--- a/arch/powerpc/include/asm/vdso.h
+++ b/arch/powerpc/include/asm/vdso.h
@@ -24,19 +24,7 @@ int vdso_getcpu_init(void);
#else /* __ASSEMBLY__ */
-#ifdef __VDSO64__
-#define V_FUNCTION_BEGIN(name) \
- .globl name; \
- name: \
-
-#define V_FUNCTION_END(name) \
- .size name,.-name;
-
-#define V_LOCAL_FUNC(name) (name)
-#endif /* __VDSO64__ */
-
-#ifdef __VDSO32__
-
+#if defined(__VDSO32__) || defined (__VDSO64__)
#define V_FUNCTION_BEGIN(name) \
.globl name; \
.type name,@function; \
@@ -46,8 +34,7 @@ int vdso_getcpu_init(void);
.size name,.-name;
#define V_LOCAL_FUNC(name) (name)
-
-#endif /* __VDSO32__ */
+#endif /* __VDSO{32|64}__ */
#endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
index 38c317f25141..7ea3ce537d0a 100644
--- a/arch/powerpc/kernel/vdso64/Makefile
+++ b/arch/powerpc/kernel/vdso64/Makefile
@@ -32,9 +32,13 @@ $(obj)/%.so: OBJCOPYFLAGS := -S
$(obj)/%.so: $(obj)/%.so.dbg FORCE
$(call if_changed,objcopy)
+ldflags-y := -shared -soname linux-vdso64.so.1 \
+ $(call ld-option, --eh-frame-hdr) \
+ $(call ld-option, --orphan-handling=warn) -T
+
# actual build commands
-quiet_cmd_vdso64ld = VDSO64L $@
- cmd_vdso64ld = $(CC) $(c_flags) -o $@ -Wl,-T$(filter %.lds,$^) $(filter %.o,$^) $(call cc-ldoption, -Wl$(comma)--orphan-handling=warn)
+quiet_cmd_vdso64ld = LD $@
+ cmd_vdso64ld = $(cmd_ld)
# install commands for the unstripped file
quiet_cmd_vdso_install = INSTALL $@
diff --git a/arch/powerpc/kernel/vdso64/vdso64.lds.S b/arch/powerpc/kernel/vdso64/vdso64.lds.S
index 4e3a8d4ee614..58c33b704b6a 100644
--- a/arch/powerpc/kernel/vdso64/vdso64.lds.S
+++ b/arch/powerpc/kernel/vdso64/vdso64.lds.S
@@ -11,7 +11,6 @@ OUTPUT_FORMAT("elf64-powerpcle", "elf64-powerpcle", "elf64-powerpcle")
OUTPUT_FORMAT("elf64-powerpc", "elf64-powerpc", "elf64-powerpc")
#endif
OUTPUT_ARCH(powerpc:common64)
-ENTRY(_start)
SECTIONS
{
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply related
* [PATCH 0/2] link vdso with linker
From: Nick Desaulniers @ 2020-09-01 22:25 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
Nick Desaulniers, linux-kernel, clang-built-linux, Paul Mackerras,
linuxppc-dev
Kees Cook is working on series that adds --orphan-section=warn to arm,
arm64, and x86. I noticed that ppc vdso were still using cc-ldoption
for these which I removed. It seems this results in that flag being
silently dropped.
I'm very confident with the first patch, but the second needs closer
review around the error mentioned below the fold related to the .got
section.
Nick Desaulniers (2):
powerpc/vdso64: link vdso64 with linker
powerpc/vdso32: link vdso64 with linker
arch/powerpc/include/asm/vdso.h | 17 ++---------------
arch/powerpc/kernel/vdso32/Makefile | 7 +++++--
arch/powerpc/kernel/vdso32/vdso32.lds.S | 3 ++-
arch/powerpc/kernel/vdso64/Makefile | 8 ++++++--
arch/powerpc/kernel/vdso64/vdso64.lds.S | 1 -
5 files changed, 15 insertions(+), 21 deletions(-)
--
2.28.0.402.g5ffc5be6b7-goog
^ permalink raw reply
* [PATCH 2/2] dma-mapping: set default segment_boundary_mask to ULONG_MAX
From: Nicolin Chen @ 2020-09-01 22:16 UTC (permalink / raw)
To: hch
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, sfr, deller,
x86, borntraeger, mingo, mattst88, fenghua.yu, gor, schnelle, hca,
ink, tglx, gerald.schaefer, rth, tony.luck, linux-parisc,
linux-s390, linux-kernel, linux-alpha, bp, linuxppc-dev, davem
In-Reply-To: <20200901221646.26491-1-nicoleotsuka@gmail.com>
The default segment_boundary_mask was set to DMA_BIT_MAKS(32)
a decade ago by referencing SCSI/block subsystem, as a 32-bit
mask was good enough for most of the devices.
Now more and more drivers set dma_masks above DMA_BIT_MAKS(32)
while only a handful of them call dma_set_seg_boundary(). This
means that most drivers have a 4GB segmention boundary because
DMA API returns a 32-bit default value, though they might not
really have such a limit.
The default segment_boundary_mask should mean "no limit" since
the device doesn't explicitly set the mask. But a 32-bit mask
certainly limits those devices capable of 32+ bits addressing.
So this patch sets default segment_boundary_mask to ULONG_MAX.
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
include/linux/dma-mapping.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index faab0a8210b9..df0bff2ea750 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -629,7 +629,7 @@ static inline unsigned long dma_get_seg_boundary(struct device *dev)
{
if (dev->dma_parms && dev->dma_parms->segment_boundary_mask)
return dev->dma_parms->segment_boundary_mask;
- return DMA_BIT_MASK(32);
+ return ULONG_MAX;
}
/**
--
2.17.1
^ permalink raw reply related
* [PATCH 0/2] dma-mapping: update default segment_boundary_mask
From: Nicolin Chen @ 2020-09-01 22:16 UTC (permalink / raw)
To: hch
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, sfr, deller,
x86, borntraeger, mingo, mattst88, fenghua.yu, gor, schnelle, hca,
ink, tglx, gerald.schaefer, rth, tony.luck, linux-parisc,
linux-s390, linux-kernel, linux-alpha, bp, linuxppc-dev, davem
These two patches are to update default segment_boundary_mask.
PATCH-1 fixes overflow issues in callers of dma_get_seg_boundary.
Previous version was a series: https://lkml.org/lkml/2020/8/31/1026
Then PATCH-2 sets default segment_boundary_mask to ULONG_MAX.
Nicolin Chen (2):
dma-mapping: introduce dma_get_seg_boundary_nr_pages()
dma-mapping: set default segment_boundary_mask to ULONG_MAX
arch/alpha/kernel/pci_iommu.c | 7 +------
arch/ia64/hp/common/sba_iommu.c | 3 +--
arch/powerpc/kernel/iommu.c | 9 ++-------
arch/s390/pci/pci_dma.c | 6 ++----
arch/sparc/kernel/iommu-common.c | 10 +++-------
arch/sparc/kernel/iommu.c | 3 +--
arch/sparc/kernel/pci_sun4v.c | 3 +--
arch/x86/kernel/amd_gart_64.c | 3 +--
drivers/parisc/ccio-dma.c | 3 +--
drivers/parisc/sba_iommu.c | 3 +--
include/linux/dma-mapping.h | 21 ++++++++++++++++++++-
11 files changed, 34 insertions(+), 37 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 1/2] dma-mapping: introduce dma_get_seg_boundary_nr_pages()
From: Nicolin Chen @ 2020-09-01 22:16 UTC (permalink / raw)
To: hch
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, sfr, deller,
x86, borntraeger, mingo, mattst88, fenghua.yu, gor, schnelle, hca,
ink, tglx, gerald.schaefer, rth, tony.luck, linux-parisc,
linux-s390, linux-kernel, linux-alpha, bp, linuxppc-dev, davem
In-Reply-To: <20200901221646.26491-1-nicoleotsuka@gmail.com>
We found that callers of dma_get_seg_boundary mostly do an ALIGN
with page mask and then do a page shift to get number of pages:
ALIGN(boundary + 1, 1 << shift) >> shift
However, the boundary might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1" or
passing it to ALIGN() would potentially overflow.
According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
We can simplify the logic here into a helper function doing:
ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1
This patch introduces and applies dma_get_seg_boundary_nr_pages()
as an overflow-free helper for the dma_get_seg_boundary() callers
to get numbers of pages. It also takes care of the NULL dev case
for non-DMA API callers.
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
arch/alpha/kernel/pci_iommu.c | 7 +------
arch/ia64/hp/common/sba_iommu.c | 3 +--
arch/powerpc/kernel/iommu.c | 9 ++-------
arch/s390/pci/pci_dma.c | 6 ++----
arch/sparc/kernel/iommu-common.c | 10 +++-------
arch/sparc/kernel/iommu.c | 3 +--
arch/sparc/kernel/pci_sun4v.c | 3 +--
arch/x86/kernel/amd_gart_64.c | 3 +--
drivers/parisc/ccio-dma.c | 3 +--
drivers/parisc/sba_iommu.c | 3 +--
include/linux/dma-mapping.h | 19 +++++++++++++++++++
11 files changed, 33 insertions(+), 36 deletions(-)
diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
index 81037907268d..6f7de4f4e191 100644
--- a/arch/alpha/kernel/pci_iommu.c
+++ b/arch/alpha/kernel/pci_iommu.c
@@ -141,12 +141,7 @@ iommu_arena_find_pages(struct device *dev, struct pci_iommu_arena *arena,
unsigned long boundary_size;
base = arena->dma_base >> PAGE_SHIFT;
- if (dev) {
- boundary_size = dma_get_seg_boundary(dev) + 1;
- boundary_size >>= PAGE_SHIFT;
- } else {
- boundary_size = 1UL << (32 - PAGE_SHIFT);
- }
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
/* Search forward for the first mask-aligned sequence of N free ptes */
ptes = arena->ptes;
diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
index 656a4888c300..b49b73a95067 100644
--- a/arch/ia64/hp/common/sba_iommu.c
+++ b/arch/ia64/hp/common/sba_iommu.c
@@ -485,8 +485,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
ASSERT(((unsigned long) ioc->res_hint & (sizeof(unsigned long) - 1UL)) == 0);
ASSERT(res_ptr < res_end);
- boundary_size = (unsigned long long)dma_get_seg_boundary(dev) + 1;
- boundary_size = ALIGN(boundary_size, 1ULL << iovp_shift) >> iovp_shift;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, iovp_shift);
BUG_ON(ioc->ibase & ~iovp_mask);
shift = ioc->ibase >> iovp_shift;
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 9704f3f76e63..cbc2e62db597 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -236,15 +236,10 @@ static unsigned long iommu_range_alloc(struct device *dev,
}
}
- if (dev)
- boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- 1 << tbl->it_page_shift);
- else
- boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
- /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, tbl->it_page_shift);
n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
- boundary_size >> tbl->it_page_shift, align_mask);
+ boundary_size, align_mask);
if (n == -1) {
if (likely(pass == 0)) {
/* First try the pool from the start */
diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
index 64b1399a73f0..4a37d8f4de9d 100644
--- a/arch/s390/pci/pci_dma.c
+++ b/arch/s390/pci/pci_dma.c
@@ -261,13 +261,11 @@ static unsigned long __dma_alloc_iommu(struct device *dev,
unsigned long start, int size)
{
struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
- unsigned long boundary_size;
- boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- PAGE_SIZE) >> PAGE_SHIFT;
return iommu_area_alloc(zdev->iommu_bitmap, zdev->iommu_pages,
start, size, zdev->start_dma >> PAGE_SHIFT,
- boundary_size, 0);
+ dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT),
+ 0);
}
static dma_addr_t dma_alloc_address(struct device *dev, int size)
diff --git a/arch/sparc/kernel/iommu-common.c b/arch/sparc/kernel/iommu-common.c
index 59cb16691322..23ca75f09277 100644
--- a/arch/sparc/kernel/iommu-common.c
+++ b/arch/sparc/kernel/iommu-common.c
@@ -166,13 +166,6 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
}
}
- if (dev)
- boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- 1 << iommu->table_shift);
- else
- boundary_size = ALIGN(1ULL << 32, 1 << iommu->table_shift);
-
- boundary_size = boundary_size >> iommu->table_shift;
/*
* if the skip_span_boundary_check had been set during init, we set
* things up so that iommu_is_span_boundary() merely checks if the
@@ -181,6 +174,9 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
if ((iommu->flags & IOMMU_NO_SPAN_BOUND) != 0) {
shift = 0;
boundary_size = iommu->poolsize * iommu->nr_pools;
+ } else {
+ boundary_size = dma_get_seg_boundary_nr_pages(dev,
+ iommu->table_shift);
}
n = iommu_area_alloc(iommu->map, limit, start, npages, shift,
boundary_size, align_mask);
diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
index 4ae7388b1bff..c3e4e2df26a8 100644
--- a/arch/sparc/kernel/iommu.c
+++ b/arch/sparc/kernel/iommu.c
@@ -472,8 +472,7 @@ static int dma_4u_map_sg(struct device *dev, struct scatterlist *sglist,
outs->dma_length = 0;
max_seg_size = dma_get_max_seg_size(dev);
- seg_boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
+ seg_boundary_size = dma_get_seg_boundary_nr_pages(dev, IO_PAGE_SHIFT);
base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
for_each_sg(sglist, s, nelems, i) {
unsigned long paddr, npages, entry, out_entry = 0, slen;
diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
index 14b93c5564e3..6b92dd51c002 100644
--- a/arch/sparc/kernel/pci_sun4v.c
+++ b/arch/sparc/kernel/pci_sun4v.c
@@ -508,8 +508,7 @@ static int dma_4v_map_sg(struct device *dev, struct scatterlist *sglist,
iommu_batch_start(dev, prot, ~0UL);
max_seg_size = dma_get_max_seg_size(dev);
- seg_boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
+ seg_boundary_size = dma_get_seg_boundary_nr_pages(dev, IO_PAGE_SHIFT);
mask = *dev->dma_mask;
if (!iommu_use_atu(iommu, mask))
diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
index e89031e9c847..bccc5357bffd 100644
--- a/arch/x86/kernel/amd_gart_64.c
+++ b/arch/x86/kernel/amd_gart_64.c
@@ -96,8 +96,7 @@ static unsigned long alloc_iommu(struct device *dev, int size,
base_index = ALIGN(iommu_bus_base & dma_get_seg_boundary(dev),
PAGE_SIZE) >> PAGE_SHIFT;
- boundary_size = ALIGN((u64)dma_get_seg_boundary(dev) + 1,
- PAGE_SIZE) >> PAGE_SHIFT;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
spin_lock_irqsave(&iommu_bitmap_lock, flags);
offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, next_bit,
diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
index a5507f75b524..ba16b7f8f806 100644
--- a/drivers/parisc/ccio-dma.c
+++ b/drivers/parisc/ccio-dma.c
@@ -356,8 +356,7 @@ ccio_alloc_range(struct ioc *ioc, struct device *dev, size_t size)
** ggg sacrifices another 710 to the computer gods.
*/
- boundary_size = ALIGN((unsigned long long)dma_get_seg_boundary(dev) + 1,
- 1ULL << IOVP_SHIFT) >> IOVP_SHIFT;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, IOVP_SHIFT);
if (pages_needed <= 8) {
/*
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index d4314fba0269..959bda193b96 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -342,8 +342,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
unsigned long shift;
int ret;
- boundary_size = ALIGN((unsigned long long)dma_get_seg_boundary(dev) + 1,
- 1ULL << IOVP_SHIFT) >> IOVP_SHIFT;
+ boundary_size = dma_get_seg_boundary_nr_pages(dev, IOVP_SHIFT);
#if defined(ZX1_SUPPORT)
BUG_ON(ioc->ibase & ~IOVP_MASK);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 52635e91143b..faab0a8210b9 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -632,6 +632,25 @@ static inline unsigned long dma_get_seg_boundary(struct device *dev)
return DMA_BIT_MASK(32);
}
+/**
+ * dma_get_seg_boundary_nr_pages - return the segment boundary in "page" units
+ * @dev: device to guery the boundary for
+ * @page_shift: ilog() of the IOMMU page size
+ *
+ * Return the segment boundary in IOMMU page units (which may be different from
+ * the CPU page size) for the passed in device.
+ *
+ * If @dev is NULL a boundary of U32_MAX is assumed, this case is just for
+ * non-DMA API callers.
+ */
+static inline unsigned long dma_get_seg_boundary_nr_pages(struct device *dev,
+ unsigned int page_shift)
+{
+ if (!dev)
+ return (U32_MAX >> page_shift) + 1;
+ return (dma_get_seg_boundary(dev) >> page_shift) + 1;
+}
+
static inline int dma_set_seg_boundary(struct device *dev, unsigned long mask)
{
if (dev->dma_parms) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
From: Li Yang @ 2020-09-01 21:40 UTC (permalink / raw)
To: Herbert Xu
Cc: linuxppc-dev@lists.ozlabs.org, Linux Kernel Mailing List,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20200901015630.GA9065@gondor.apana.org.au>
On Mon, Aug 31, 2020 at 8:57 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
> >
> > Sorry for the late response. I missed this email previously.
> >
> > These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler. The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> >
> > Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario. I just tried a ARM64 build but didn't see the warnings. Could you share the warning you got and the build setup? Thanks.
>
> Just do a COMPILE_TEST build on x86-64:
>
> In file included from ../drivers/crypto/caam/qi.c:12:
Looks like the CAAM driver and dependent QBMAN driver doesn't support
COMPILE_TEST yet. Are you trying to add the support for it?
I changed the Kconfig to enable the COMPILE_TEST anyway and updated my
toolchain to gcc-10 trying to duplicate the issue. The issues can
only be reproduced with "W=1".
> ../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
> } __packed;
> ^
> ../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
> } __packed ern;
> ^
I think this is a valid concern that if the parent structure doesn't
meet certain alignment requirements, the alignment for the
sub-structure cannot be guaranteed. If we just remove the __packed
attribute from the parent structure, the compiler could try to add
padding in the parent structure to fulfill the alignment requirements
of the sub structure which is not good. I think the following changes
are a better fix for the warnings:
diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
index cfe00e08e85b..9f484113cfda 100644
--- a/include/soc/fsl/qman.h
+++ b/include/soc/fsl/qman.h
@@ -256,7 +256,7 @@ struct qm_dqrr_entry {
__be32 context_b;
struct qm_fd fd;
u8 __reserved4[32];
-} __packed;
+} __packed __aligned(64);
#define QM_DQRR_VERB_VBIT 0x80
#define QM_DQRR_VERB_MASK 0x7f /* where the verb contains; */
#define QM_DQRR_VERB_FRAME_DEQUEUE 0x60 /* "this format" */
@@ -289,7 +289,7 @@ union qm_mr_entry {
__be32 tag;
struct qm_fd fd;
u8 __reserved1[32];
- } __packed ern;
+ } __packed __aligned(64) ern;
struct {
u8 verb;
u8 fqs; /* Frame Queue Status */
Regards,
Leo
^ permalink raw reply related
* Re: [PATCH v1 01/10] powerpc/pseries/iommu: Replace hard-coded page shift
From: Leonardo Bras @ 2020-09-01 21:38 UTC (permalink / raw)
To: Alexey Kardashevskiy, Oliver O'Halloran
Cc: Christophe Leroy, David Dai, Ram Pai, Linux Kernel Mailing List,
Murilo Fossa Vicentini, Paul Mackerras, Joel Stanley, Brian King,
linuxppc-dev, Thiago Jung Bauermann
In-Reply-To: <1bba12c6-f1ec-9f1e-1d3e-c1efa5ceb7c7@ozlabs.ru>
On Mon, 2020-08-31 at 13:48 +1000, Alexey Kardashevskiy wrote:
> > > > Well, I created this TCE_RPN_BITS = 52 because the previous mask was a
> > > > hardcoded 40-bit mask (0xfffffffffful), for hard-coded 12-bit (4k)
> > > > pagesize, and on PAPR+/LoPAR also defines TCE as having bits 0-51
> > > > described as RPN, as described before.
> > > >
> > > > IODA3 Revision 3.0_prd1 (OpenPowerFoundation), Figure 3.4 and 3.5.
> > > > shows system memory mapping into a TCE, and the TCE also has bits 0-51
> > > > for the RPN (52 bits). "Table 3.6. TCE Definition" also shows it.
> > > > In fact, by the looks of those figures, the RPN_MASK should always be a
> > > > 52-bit mask, and RPN = (page >> tceshift) & RPN_MASK.
> > >
> > > I suspect the mask is there in the first place for extra protection
> > > against too big addresses going to the TCE table (or/and for virtial vs
> > > physical addresses). Using 52bit mask makes no sense for anything, you
> > > could just drop the mask and let c compiler deal with 64bit "uint" as it
> > > is basically a 4K page address anywhere in the 64bit space. Thanks,
> >
> > Assuming 4K pages you need 52 RPN bits to cover the whole 64bit
> > physical address space. The IODA3 spec does explicitly say the upper
> > bits are optional and the implementation only needs to support enough
> > to cover up to the physical address limit, which is 56bits of P9 /
> > PHB4. If you want to validate that the address will fit inside of
> > MAX_PHYSMEM_BITS then fine, but I think that should be done as a
> > WARN_ON or similar rather than just silently masking off the bits.
>
> We can do this and probably should anyway but I am also pretty sure we
> can just ditch the mask and have the hypervisor return an error which
> will show up in dmesg.
Ok then, ditching the mask.
Thanks!
^ permalink raw reply
* Re: [RESEND][PATCH 1/7] powerpc/iommu: Avoid overflow at boundary_size
From: Nicolin Chen @ 2020-09-01 20:53 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, hch, sfr,
deller, x86, borntraeger, mingo, mattst88, fenghua.yu, gor,
schnelle, hca, ink, tglx, gerald.schaefer, rth, tony.luck,
linux-parisc, linux-s390, linux-kernel, linux-alpha, bp,
linuxppc-dev, davem
In-Reply-To: <87lfht1vav.fsf@mpe.ellerman.id.au>
On Tue, Sep 01, 2020 at 11:27:36PM +1000, Michael Ellerman wrote:
> Nicolin Chen <nicoleotsuka@gmail.com> writes:
> > The boundary_size might be as large as ULONG_MAX, which means
> > that a device has no specific boundary limit. So either "+ 1"
> > or passing it to ALIGN() would potentially overflow.
> >
> > According to kernel defines:
> > #define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
> > #define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
> >
> > We can simplify the logic here:
> > ALIGN(boundary + 1, 1 << shift) >> shift
> > = ALIGN_MASK(b + 1, (1 << s) - 1) >> s
> > = {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
> > = [b + 1 + (1 << s) - 1] >> s
> > = [b + (1 << s)] >> s
> > = (b >> s) + 1
> >
> > So fixing a potential overflow with the safer shortcut.
> >
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > ---
> > arch/powerpc/kernel/iommu.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
>
> Are you asking for acks, or for maintainers to merge the patches
> individually?
I was expecting that but Christoph just suggested me to squash them
into one so he would merge it: https://lkml.org/lkml/2020/9/1/159
Though I feel it'd be nice to get maintainers' acks before merging.
> > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> > index 9704f3f76e63..c01ccbf8afdd 100644
> > --- a/arch/powerpc/kernel/iommu.c
> > +++ b/arch/powerpc/kernel/iommu.c
> > @@ -236,15 +236,14 @@ static unsigned long iommu_range_alloc(struct device *dev,
> > }
> > }
> >
> > - if (dev)
> > - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
> > - 1 << tbl->it_page_shift);
> > - else
> > - boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
> > /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> > + boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
>
> Is there any path that passes a NULL dev anymore?
>
> Both iseries_hv_alloc() and iseries_hv_map() were removed years ago.
> See:
> 8ee3e0d69623 ("powerpc: Remove the main legacy iSerie platform code")
>
>
> So maybe we should do a lead-up patch that drops the NULL dev support,
> which will then make this patch simpler.
The next version of this change will follow Christoph's suggestion
by having a helper function that takes care of !dev internally.
Thanks
Nic
>
>
> > + /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> > + boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
> >
> > n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
> > - boundary_size >> tbl->it_page_shift, align_mask);
> > + boundary_size, align_mask);
> > if (n == -1) {
> > if (likely(pass == 0)) {
> > /* First try the pool from the start */
> > --
> > 2.17.1
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc v2
From: Christophe Leroy @ 2020-09-01 19:01 UTC (permalink / raw)
To: Al Viro
Cc: linux-arch, Kees Cook, x86, linuxppc-dev, linux-kernel,
linux-fsdevel, Linus Torvalds, Christoph Hellwig
In-Reply-To: <20200901172512.GI1236603@ZenIV.linux.org.uk>
Le 01/09/2020 à 19:25, Al Viro a écrit :
> On Tue, Sep 01, 2020 at 07:13:00PM +0200, Christophe Leroy wrote:
>
>> 10.92% dd [kernel.kallsyms] [k] iov_iter_zero
>
> Interesting... Could you get an instruction-level profile inside iov_iter_zero(),
> along with the disassembly of that sucker?
>
As a comparison, hereunder is the perf annotate of the 5.9-rc2 without
the series:
Percent | Source code & Disassembly of vmlinux for cpu-clock (2581
samples)
---------------------------------------------------------------------------------
:
:
:
: Disassembly of section .text:
:
: c02cbb80 <iov_iter_zero>:
: iov_iter_zero():
3.22 : c02cbb80: stwu r1,-80(r1)
3.25 : c02cbb84: stw r30,72(r1)
0.00 : c02cbb88: mr r30,r4
2.91 : c02cbb8c: stw r31,76(r1)
0.00 : c02cbb90: mr r31,r3
0.19 : c02cbb94: stw r27,60(r1)
: iov_iter_type():
1.82 : c02cbb98: lwz r10,0(r4)
0.54 : c02cbb9c: rlwinm r9,r10,0,0,30
: iov_iter_zero():
1.98 : c02cbba0: cmpwi r9,32
0.00 : c02cbba4: lwz r9,624(r2)
0.35 : c02cbba8: stw r9,28(r1)
0.00 : c02cbbac: li r9,0
0.00 : c02cbbb0: beq c02cbd00 <iov_iter_zero+0x180>
2.67 : c02cbbb4: lwz r9,8(r4)
1.98 : c02cbbb8: cmplw r9,r3
0.00 : c02cbbbc: mr r27,r9
0.00 : c02cbbc0: bgt c02cbce8 <iov_iter_zero+0x168>
0.31 : c02cbbc4: cmpwi r9,0
0.00 : c02cbbc8: beq c02cbcbc <iov_iter_zero+0x13c>
3.22 : c02cbbcc: andi. r8,r10,16
1.70 : c02cbbd0: lwz r31,4(r30)
0.00 : c02cbbd4: bne c02cbe10 <iov_iter_zero+0x290>
0.31 : c02cbbd8: andi. r8,r10,8
0.00 : c02cbbdc: bne c02cbf64 <iov_iter_zero+0x3e4>
1.82 : c02cbbe0: andi. r10,r10,64
0.00 : c02cbbe4: bne c02cc080 <iov_iter_zero+0x500>
0.27 : c02cbbe8: stw r29,68(r1)
1.94 : c02cbbec: stw r28,64(r1)
1.98 : c02cbbf0: lwz r28,12(r30)
0.31 : c02cbbf4: lwz r7,4(r28)
2.13 : c02cbbf8: subf r29,r31,r7
1.78 : c02cbbfc: cmplw r29,r27
0.08 : c02cbc00: bgt c02cbcf8 <iov_iter_zero+0x178>
28.24 : c02cbc04: cmpwi r29,0
0.00 : c02cbc08: beq c02cc08c <iov_iter_zero+0x50c>
2.01 : c02cbc0c: lwz r3,0(r28)
3.10 : c02cbc10: lwz r10,1208(r2)
0.00 : c02cbc14: add r3,r3,r31
: __access_ok():
0.00 : c02cbc18: cmplw r3,r10
0.00 : c02cbc1c: bgt c02cbc7c <iov_iter_zero+0xfc>
3.37 : c02cbc20: subf r10,r3,r10
0.00 : c02cbc24: addi r8,r29,-1
3.14 : c02cbc28: cmplw r8,r10
0.08 : c02cbc2c: mflr r0
0.00 : c02cbc30: stw r0,84(r1)
0.00 : c02cbc34: bgt c02cbd40 <iov_iter_zero+0x1c0>
: clear_user():
0.00 : c02cbc38: mr r4,r29
2.40 : c02cbc3c: bl c001a428 <__arch_clear_user>
: iov_iter_zero():
1.55 : c02cbc40: add r31,r31,r29
0.00 : c02cbc44: cmpwi r3,0
1.94 : c02cbc48: subf r29,r29,r27
0.00 : c02cbc4c: subf r31,r3,r31
0.00 : c02cbc50: add r29,r29,r3
0.00 : c02cbc54: beq c02cc0ac <iov_iter_zero+0x52c>
0.00 : c02cbc58: lwz r9,8(r30)
0.00 : c02cbc5c: subf r10,r27,r29
0.00 : c02cbc60: lwz r0,84(r1)
0.00 : c02cbc64: subf r27,r29,r27
0.00 : c02cbc68: add r9,r10,r9
0.00 : c02cbc6c: lwz r7,4(r28)
0.00 : c02cbc70: lwz r10,12(r30)
0.00 : c02cbc74: mtlr r0
0.00 : c02cbc78: b c02cbc84 <iov_iter_zero+0x104>
: __access_ok():
0.00 : c02cbc7c: li r27,0
0.00 : c02cbc80: mr r10,r28
: iov_iter_zero():
0.00 : c02cbc84: cmplw r31,r7
0.00 : c02cbc88: bne c02cbc94 <iov_iter_zero+0x114>
0.93 : c02cbc8c: addi r28,r28,8
0.00 : c02cbc90: li r31,0
1.28 : c02cbc94: lwz r8,16(r30)
0.00 : c02cbc98: subf r10,r10,r28
1.05 : c02cbc9c: srawi r10,r10,3
0.00 : c02cbca0: stw r28,12(r30)
0.00 : c02cbca4: subf r10,r10,r8
0.93 : c02cbca8: stw r10,16(r30)
0.04 : c02cbcac: lwz r28,64(r1)
0.00 : c02cbcb0: lwz r29,68(r1)
1.05 : c02cbcb4: stw r9,8(r30)
0.00 : c02cbcb8: stw r31,4(r30)
1.39 : c02cbcbc: lwz r9,28(r1)
0.00 : c02cbcc0: lwz r10,624(r2)
1.08 : c02cbcc4: xor. r9,r9,r10
0.00 : c02cbcc8: li r10,0
0.00 : c02cbccc: bne c02cc180 <iov_iter_zero+0x600>
1.08 : c02cbcd0: mr r3,r27
0.00 : c02cbcd4: lwz r30,72(r1)
0.08 : c02cbcd8: lwz r27,60(r1)
1.01 : c02cbcdc: lwz r31,76(r1)
0.00 : c02cbce0: addi r1,r1,80
0.04 : c02cbce4: blr
0.00 : c02cbce8: cmpwi r9,0
0.00 : c02cbcec: mr r27,r3
0.00 : c02cbcf0: beq c02cbcbc <iov_iter_zero+0x13c>
0.00 : c02cbcf4: b c02cbbcc <iov_iter_zero+0x4c>
0.00 : c02cbcf8: mr r29,r27
0.00 : c02cbcfc: b c02cbc04 <iov_iter_zero+0x84>
: pipe_zero():
0.00 : c02cbd00: mflr r0
0.00 : c02cbd04: stw r26,56(r1)
0.00 : c02cbd08: stw r0,84(r1)
0.00 : c02cbd0c: mr r3,r4
0.00 : c02cbd10: stw r28,64(r1)
0.00 : c02cbd14: lwz r28,12(r4)
0.00 : c02cbd18: lwz r26,40(r28)
0.00 : c02cbd1c: bl c02c95d0 <sanity>
0.00 : c02cbd20: cmpwi r3,0
0.00 : c02cbd24: bne c02cbd54 <iov_iter_zero+0x1d4>
0.00 : c02cbd28: lwz r0,84(r1)
0.00 : c02cbd2c: li r27,0
0.00 : c02cbd30: lwz r26,56(r1)
0.00 : c02cbd34: lwz r28,64(r1)
0.00 : c02cbd38: mtlr r0
0.00 : c02cbd3c: b c02cbcbc <iov_iter_zero+0x13c>
: __access_ok():
0.00 : c02cbd40: lwz r0,84(r1)
0.00 : c02cbd44: li r27,0
0.00 : c02cbd48: mr r10,r28
0.00 : c02cbd4c: mtlr r0
0.00 : c02cbd50: b c02cbc84 <iov_iter_zero+0x104>
: pipe_zero():
0.00 : c02cbd54: mr r4,r31
0.00 : c02cbd58: addi r6,r1,24
0.00 : c02cbd5c: addi r5,r1,20
0.00 : c02cbd60: mr r3,r30
0.00 : c02cbd64: bl c02c97ac <push_pipe>
0.00 : c02cbd68: mr. r27,r3
0.00 : c02cbd6c: beq c02cbd28 <iov_iter_zero+0x1a8>
0.00 : c02cbd70: lwz r4,24(r1)
0.00 : c02cbd74: addi r26,r26,-1
0.00 : c02cbd78: lwz r9,20(r1)
0.00 : c02cbd7c: stw r25,52(r1)
0.00 : c02cbd80: li r25,0
0.00 : c02cbd84: stw r29,68(r1)
0.00 : c02cbd88: mr r29,r27
0.00 : c02cbd8c: subfic r31,r4,4096
0.00 : c02cbd90: cmplw r31,r29
0.00 : c02cbd94: ble c02cbd9c <iov_iter_zero+0x21c>
0.00 : c02cbd98: mr r31,r29
0.00 : c02cbd9c: and r9,r26,r9
0.00 : c02cbda0: lwz r8,80(r28)
0.00 : c02cbda4: rlwinm r10,r9,1,0,30
0.00 : c02cbda8: add r9,r10,r9
0.00 : c02cbdac: rlwinm r9,r9,3,0,28
0.00 : c02cbdb0: lwzx r3,r8,r9
0.00 : c02cbdb4: mr r5,r31
0.00 : c02cbdb8: bl c02c99d0 <memzero_page>
0.00 : c02cbdbc: subf. r29,r31,r29
0.00 : c02cbdc0: lwz r9,20(r1)
0.00 : c02cbdc4: li r4,0
0.00 : c02cbdc8: lwz r10,24(r1)
0.00 : c02cbdcc: stw r9,16(r30)
0.00 : c02cbdd0: addi r9,r9,1
0.00 : c02cbdd4: add r10,r10,r31
0.00 : c02cbdd8: stw r9,20(r1)
0.00 : c02cbddc: stw r10,4(r30)
0.00 : c02cbde0: stw r25,24(r1)
0.00 : c02cbde4: bne c02cbd8c <iov_iter_zero+0x20c>
0.00 : c02cbde8: lwz r9,8(r30)
0.00 : c02cbdec: subf r9,r27,r9
0.00 : c02cbdf0: stw r9,8(r30)
: iov_iter_zero():
0.00 : c02cbdf4: lwz r0,84(r1)
0.00 : c02cbdf8: lwz r25,52(r1)
0.00 : c02cbdfc: lwz r26,56(r1)
0.00 : c02cbe00: mtlr r0
0.00 : c02cbe04: lwz r28,64(r1)
0.00 : c02cbe08: lwz r29,68(r1)
0.00 : c02cbe0c: b c02cbcbc <iov_iter_zero+0x13c>
0.00 : c02cbe10: stw r23,44(r1)
0.00 : c02cbe14: cmpwi r27,0
0.00 : c02cbe18: stw r28,64(r1)
0.00 : c02cbe1c: mr r23,r27
0.00 : c02cbe20: stw r24,48(r1)
0.00 : c02cbe24: li r28,0
0.00 : c02cbe28: lwz r24,12(r30)
0.00 : c02cbe2c: mr r8,r24
0.00 : c02cbe30: beq c02cbf08 <iov_iter_zero+0x388>
0.00 : c02cbe34: mflr r0
0.00 : c02cbe38: stw r25,52(r1)
0.00 : c02cbe3c: stw r0,84(r1)
0.00 : c02cbe40: stw r26,56(r1)
0.00 : c02cbe44: stw r29,68(r1)
0.00 : c02cbe48: rlwinm r25,r28,1,0,30
0.00 : c02cbe4c: add r25,r25,r28
0.00 : c02cbe50: rlwinm r25,r25,2,0,29
0.00 : c02cbe54: add r10,r8,r25
0.00 : c02cbe58: lwz r26,4(r10)
0.00 : c02cbe5c: mr r29,r25
0.00 : c02cbe60: lwz r9,8(r10)
0.00 : c02cbe64: subf r26,r31,r26
0.00 : c02cbe68: cmplw r26,r23
0.00 : c02cbe6c: add r9,r31,r9
0.00 : c02cbe70: clrlwi r4,r9,20
0.00 : c02cbe74: ble c02cbe7c <iov_iter_zero+0x2fc>
0.00 : c02cbe78: mr r26,r23
0.00 : c02cbe7c: subfic r7,r4,4096
0.00 : c02cbe80: cmplw r26,r7
0.00 : c02cbe84: ble c02cbe8c <iov_iter_zero+0x30c>
0.00 : c02cbe88: mr r26,r7
0.00 : c02cbe8c: cmpwi r26,0
0.00 : c02cbe90: beq c02cbeb4 <iov_iter_zero+0x334>
0.00 : c02cbe94: lwz r3,0(r10)
0.00 : c02cbe98: rlwinm r9,r9,25,7,26
0.00 : c02cbe9c: mr r5,r26
0.00 : c02cbea0: add r3,r3,r9
0.00 : c02cbea4: bl c02c99d0 <memzero_page>
: bvec_iter_advance():
0.00 : c02cbea8: cmplw r23,r26
: iov_iter_zero():
0.00 : c02cbeac: lwz r8,12(r30)
: bvec_iter_advance():
0.00 : c02cbeb0: blt c02cc044 <iov_iter_zero+0x4c4>
0.00 : c02cbeb4: add. r31,r31,r26
0.00 : c02cbeb8: subf r23,r26,r23
0.00 : c02cbebc: addi r10,r8,4
0.00 : c02cbec0: bne c02cbed8 <iov_iter_zero+0x358>
0.00 : c02cbec4: b c02cbee4 <iov_iter_zero+0x364>
0.00 : c02cbec8: subf. r31,r9,r31
0.00 : c02cbecc: addi r28,r28,1
0.00 : c02cbed0: addi r29,r29,12
0.00 : c02cbed4: beq c02cbf54 <iov_iter_zero+0x3d4>
0.00 : c02cbed8: lwzx r9,r10,r29
0.00 : c02cbedc: cmplw r31,r9
0.00 : c02cbee0: bge c02cbec8 <iov_iter_zero+0x348>
: iov_iter_zero():
0.00 : c02cbee4: cmpwi r23,0
0.00 : c02cbee8: bne c02cbe48 <iov_iter_zero+0x2c8>
0.00 : c02cbeec: add r8,r8,r29
0.00 : c02cbef0: lwz r0,84(r1)
0.00 : c02cbef4: lwz r9,8(r30)
0.00 : c02cbef8: lwz r25,52(r1)
0.00 : c02cbefc: mtlr r0
0.00 : c02cbf00: lwz r26,56(r1)
0.00 : c02cbf04: lwz r29,68(r1)
0.00 : c02cbf08: subf r24,r24,r8
0.00 : c02cbf0c: stw r8,12(r30)
0.00 : c02cbf10: srawi r6,r24,2
0.00 : c02cbf14: lwz r7,16(r30)
0.00 : c02cbf18: rlwinm r10,r24,0,0,29
0.00 : c02cbf1c: add r10,r10,r6
0.00 : c02cbf20: rlwinm r8,r10,4,0,27
0.00 : c02cbf24: add r10,r10,r8
0.00 : c02cbf28: rlwinm r8,r10,8,0,23
0.00 : c02cbf2c: add r10,r10,r8
0.00 : c02cbf30: rlwinm r8,r10,16,0,15
0.00 : c02cbf34: add r10,r10,r8
0.00 : c02cbf38: add r10,r7,r10
0.00 : c02cbf3c: stw r10,16(r30)
0.00 : c02cbf40: subf r9,r27,r9
0.00 : c02cbf44: lwz r23,44(r1)
0.00 : c02cbf48: lwz r24,48(r1)
0.00 : c02cbf4c: lwz r28,64(r1)
0.00 : c02cbf50: b c02cbcb4 <iov_iter_zero+0x134>
0.00 : c02cbf54: rlwinm r29,r28,1,0,30
0.00 : c02cbf58: add r29,r29,r28
0.00 : c02cbf5c: rlwinm r29,r29,2,0,29
0.00 : c02cbf60: b c02cbee4 <iov_iter_zero+0x364>
0.00 : c02cbf64: mflr r0
0.00 : c02cbf68: stw r26,56(r1)
0.00 : c02cbf6c: stw r0,84(r1)
0.00 : c02cbf70: stw r28,64(r1)
0.00 : c02cbf74: stw r29,68(r1)
0.00 : c02cbf78: lwz r28,12(r30)
0.00 : c02cbf7c: lwz r29,4(r28)
0.00 : c02cbf80: subf r29,r31,r29
0.00 : c02cbf84: cmplw r29,r27
0.00 : c02cbf88: ble c02cbf90 <iov_iter_zero+0x410>
0.00 : c02cbf8c: mr r29,r27
0.00 : c02cbf90: cmpwi r29,0
0.00 : c02cbf94: beq c02cc0b8 <iov_iter_zero+0x538>
0.00 : c02cbf98: lwz r3,0(r28)
0.00 : c02cbf9c: mr r5,r29
0.00 : c02cbfa0: li r4,0
0.00 : c02cbfa4: add r3,r3,r31
0.00 : c02cbfa8: subf r26,r29,r27
0.00 : c02cbfac: bl c001999c <memset>
0.00 : c02cbfb0: add r31,r31,r29
0.00 : c02cbfb4: cmpwi r26,0
0.00 : c02cbfb8: bne c02cc00c <iov_iter_zero+0x48c>
0.00 : c02cbfbc: lwz r9,4(r28)
0.00 : c02cbfc0: cmpw r9,r31
0.00 : c02cbfc4: bne c02cbfd0 <iov_iter_zero+0x450>
0.00 : c02cbfc8: addi r28,r28,8
0.00 : c02cbfcc: li r31,0
0.00 : c02cbfd0: lwz r9,12(r30)
0.00 : c02cbfd4: lwz r8,16(r30)
0.00 : c02cbfd8: subf r10,r9,r28
0.00 : c02cbfdc: stw r28,12(r30)
0.00 : c02cbfe0: srawi r10,r10,3
0.00 : c02cbfe4: lwz r9,8(r30)
0.00 : c02cbfe8: subf r10,r10,r8
0.00 : c02cbfec: stw r10,16(r30)
0.00 : c02cbff0: subf r9,r27,r9
0.00 : c02cbff4: lwz r0,84(r1)
0.00 : c02cbff8: lwz r26,56(r1)
0.00 : c02cbffc: lwz r28,64(r1)
0.00 : c02cc000: mtlr r0
0.00 : c02cc004: lwz r29,68(r1)
0.00 : c02cc008: b c02cbcb4 <iov_iter_zero+0x134>
0.00 : c02cc00c: lwz r31,12(r28)
0.00 : c02cc010: addi r28,r28,8
0.00 : c02cc014: cmplw r31,r26
0.00 : c02cc018: ble c02cc020 <iov_iter_zero+0x4a0>
0.00 : c02cc01c: mr r31,r26
0.00 : c02cc020: cmpwi r31,0
0.00 : c02cc024: beq c02cc00c <iov_iter_zero+0x48c>
0.00 : c02cc028: lwz r3,0(r28)
0.00 : c02cc02c: mr r5,r31
0.00 : c02cc030: li r4,0
0.00 : c02cc034: bl c001999c <memset>
0.00 : c02cc038: subf. r26,r31,r26
0.00 : c02cc03c: beq c02cbfbc <iov_iter_zero+0x43c>
0.00 : c02cc040: b c02cc00c <iov_iter_zero+0x48c>
: bvec_iter_advance():
0.00 : c02cc044: lis r9,-16236
0.00 : c02cc048: lbz r10,-20202(r9)
0.00 : c02cc04c: cmpwi r10,0
0.00 : c02cc050: beq c02cc05c <iov_iter_zero+0x4dc>
: iov_iter_zero():
0.00 : c02cc054: add r8,r8,r25
0.00 : c02cc058: b c02cbef0 <iov_iter_zero+0x370>
: bvec_iter_advance():
0.00 : c02cc05c: lis r3,-16253
0.00 : c02cc060: li r10,1
0.00 : c02cc064: addi r3,r3,7692
0.00 : c02cc068: stb r10,-20202(r9)
0.00 : c02cc06c: bl c0029bc0 <__warn_printk>
0.00 : c02cc070: twui r0,0
: iov_iter_zero():
0.00 : c02cc074: lwz r8,12(r30)
0.00 : c02cc078: add r8,r8,r25
0.00 : c02cc07c: b c02cbef0 <iov_iter_zero+0x370>
0.00 : c02cc080: add r31,r31,r27
0.00 : c02cc084: subf r9,r27,r9
0.00 : c02cc088: b c02cbcb4 <iov_iter_zero+0x134>
0.00 : c02cc08c: mr r29,r27
0.00 : c02cc090: cmpwi r29,0
0.00 : c02cc094: bne c02cc0c0 <iov_iter_zero+0x540>
1.51 : c02cc098: lwz r9,8(r30)
0.00 : c02cc09c: lwz r7,4(r28)
0.00 : c02cc0a0: lwz r10,12(r30)
0.00 : c02cc0a4: subf r9,r27,r9
0.00 : c02cc0a8: b c02cbc84 <iov_iter_zero+0x104>
1.47 : c02cc0ac: lwz r0,84(r1)
6.47 : c02cc0b0: mtlr r0
0.00 : c02cc0b4: b c02cc090 <iov_iter_zero+0x510>
0.00 : c02cc0b8: mr r26,r27
0.00 : c02cc0bc: b c02cbfb4 <iov_iter_zero+0x434>
0.00 : c02cc0c0: stw r26,56(r1)
0.00 : c02cc0c4: lwz r7,12(r28)
0.00 : c02cc0c8: addi r26,r28,8
0.00 : c02cc0cc: mr r31,r29
0.00 : c02cc0d0: cmplw r29,r7
0.00 : c02cc0d4: ble c02cc0dc <iov_iter_zero+0x55c>
0.00 : c02cc0d8: mr r31,r7
0.00 : c02cc0dc: cmpwi r31,0
0.00 : c02cc0e0: beq c02cc1d8 <iov_iter_zero+0x658>
0.00 : c02cc0e4: lwz r3,0(r26)
: clear_user():
0.00 : c02cc0e8: lwz r9,1208(r2)
: __access_ok():
0.00 : c02cc0ec: cmplw r3,r9
0.00 : c02cc0f0: bgt c02cc114 <iov_iter_zero+0x594>
0.00 : c02cc0f4: subf r9,r3,r9
0.00 : c02cc0f8: addi r10,r31,-1
0.00 : c02cc0fc: cmplw r10,r9
0.00 : c02cc100: mflr r0
0.00 : c02cc104: stw r0,84(r1)
0.00 : c02cc108: ble c02cc138 <iov_iter_zero+0x5b8>
0.00 : c02cc10c: lwz r0,84(r1)
0.00 : c02cc110: mtlr r0
: iov_iter_zero():
0.00 : c02cc114: lwz r9,8(r30)
0.00 : c02cc118: subf r8,r27,r29
0.00 : c02cc11c: mr r28,r26
0.00 : c02cc120: lwz r10,12(r30)
0.00 : c02cc124: lwz r26,56(r1)
0.00 : c02cc128: add r9,r8,r9
0.00 : c02cc12c: subf r27,r29,r27
0.00 : c02cc130: li r31,0
0.00 : c02cc134: b c02cbc84 <iov_iter_zero+0x104>
: clear_user():
0.00 : c02cc138: mr r4,r31
0.00 : c02cc13c: bl c001a428 <__arch_clear_user>
: iov_iter_zero():
0.00 : c02cc140: subf r29,r31,r29
0.00 : c02cc144: cmpwi r3,0
0.00 : c02cc148: subf r31,r3,r31
0.00 : c02cc14c: add r29,r3,r29
0.00 : c02cc150: beq c02cc1a4 <iov_iter_zero+0x624>
0.00 : c02cc154: lwz r9,8(r30)
0.00 : c02cc158: subf r8,r27,r29
0.00 : c02cc15c: lwz r0,84(r1)
0.00 : c02cc160: subf r27,r29,r27
0.00 : c02cc164: lwz r7,12(r28)
0.00 : c02cc168: add r9,r8,r9
0.00 : c02cc16c: mr r28,r26
0.00 : c02cc170: lwz r10,12(r30)
0.00 : c02cc174: lwz r26,56(r1)
0.00 : c02cc178: mtlr r0
0.00 : c02cc17c: b c02cbc84 <iov_iter_zero+0x104>
0.00 : c02cc180: mflr r0
0.00 : c02cc184: stw r23,44(r1)
0.00 : c02cc188: stw r0,84(r1)
0.00 : c02cc18c: stw r24,48(r1)
0.00 : c02cc190: stw r25,52(r1)
0.00 : c02cc194: stw r26,56(r1)
0.00 : c02cc198: stw r28,64(r1)
0.00 : c02cc19c: stw r29,68(r1)
0.00 : c02cc1a0: bl c071e2b0 <__stack_chk_fail>
0.00 : c02cc1a4: cmpwi r29,0
0.00 : c02cc1a8: bne c02cc1d0 <iov_iter_zero+0x650>
0.00 : c02cc1ac: lwz r9,8(r30)
0.00 : c02cc1b0: lwz r0,84(r1)
0.00 : c02cc1b4: lwz r7,12(r28)
0.00 : c02cc1b8: subf r9,r27,r9
0.00 : c02cc1bc: mr r28,r26
0.00 : c02cc1c0: lwz r10,12(r30)
0.00 : c02cc1c4: lwz r26,56(r1)
0.00 : c02cc1c8: mtlr r0
0.00 : c02cc1cc: b c02cbc84 <iov_iter_zero+0x104>
0.00 : c02cc1d0: lwz r0,84(r1)
0.00 : c02cc1d4: mtlr r0
0.00 : c02cc1d8: mr r28,r26
0.00 : c02cc1dc: b c02cc0c4 <iov_iter_zero+0x544>
Christophe
^ permalink raw reply
* Re: [PATCH 05/10] lkdtm: disable set_fs-based tests for !CONFIG_SET_FS
From: Kees Cook @ 2020-09-01 18:57 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, linuxppc-dev, the arch/x86 maintainers,
Linux Kernel Mailing List, Al Viro, linux-fsdevel, Linus Torvalds
In-Reply-To: <20200829092406.GB8833@lst.de>
On Sat, Aug 29, 2020 at 11:24:06AM +0200, Christoph Hellwig wrote:
> On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
> > On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > Once we can't manipulate the address limit, we also can't test what
> > > happens when the manipulation is abused.
> >
> > Just remove these tests entirely.
> >
> > Once set_fs() doesn't exist on x86, the tests no longer make any sense
> > what-so-ever, because test coverage will be basically zero.
> >
> > So don't make the code uglier just to maintain a fiction that
> > something is tested when it isn't really.
>
> Sure fine with me unless Kees screams.
To clarify: if any of x86, arm64, arm, powerpc, riscv, and s390 are
using set_fs(), I want to keep this test. "ugly" is fine in lkdtm. :)
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 05/10] lkdtm: disable set_fs-based tests for !CONFIG_SET_FS
From: Kees Cook @ 2020-09-01 18:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, linuxppc-dev, the arch/x86 maintainers,
Linux Kernel Mailing List, Al Viro, linux-fsdevel, Linus Torvalds
In-Reply-To: <20200829092406.GB8833@lst.de>
On Sat, Aug 29, 2020 at 11:24:06AM +0200, Christoph Hellwig wrote:
> On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
> > On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > Once we can't manipulate the address limit, we also can't test what
> > > happens when the manipulation is abused.
> >
> > Just remove these tests entirely.
> >
> > Once set_fs() doesn't exist on x86, the tests no longer make any sense
> > what-so-ever, because test coverage will be basically zero.
> >
> > So don't make the code uglier just to maintain a fiction that
> > something is tested when it isn't really.
>
> Sure fine with me unless Kees screams.
If we don't have set_fs, we don't need the tests. :)
--
Kees Cook
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc v2
From: Christophe Leroy @ 2020-09-01 18:39 UTC (permalink / raw)
To: Al Viro
Cc: linux-arch, Kees Cook, x86, linuxppc-dev, linux-kernel,
linux-fsdevel, Linus Torvalds, Christoph Hellwig
In-Reply-To: <20200901172512.GI1236603@ZenIV.linux.org.uk>
Le 01/09/2020 à 19:25, Al Viro a écrit :
> On Tue, Sep 01, 2020 at 07:13:00PM +0200, Christophe Leroy wrote:
>
>> 10.92% dd [kernel.kallsyms] [k] iov_iter_zero
>
> Interesting... Could you get an instruction-level profile inside iov_iter_zero(),
> along with the disassembly of that sucker?
>
Output of perf annotate:
Percent | Source code & Disassembly of vmlinux for cpu-clock (3579
samples)
---------------------------------------------------------------------------------
:
:
:
: Disassembly of section .text:
:
: c02cb3a4 <iov_iter_zero>:
: iov_iter_zero():
2.24 : c02cb3a4: stwu r1,-80(r1)
0.31 : c02cb3a8: stw r30,72(r1)
0.00 : c02cb3ac: mr r30,r4
0.11 : c02cb3b0: stw r31,76(r1)
0.00 : c02cb3b4: mr r31,r3
1.06 : c02cb3b8: stw r27,60(r1)
: iov_iter_type():
0.03 : c02cb3bc: lwz r10,0(r4)
0.06 : c02cb3c0: rlwinm r9,r10,0,0,30
: iov_iter_zero():
0.03 : c02cb3c4: cmpwi r9,32
0.00 : c02cb3c8: lwz r9,624(r2)
2.15 : c02cb3cc: stw r9,28(r1)
0.00 : c02cb3d0: li r9,0
0.00 : c02cb3d4: beq c02cb520 <iov_iter_zero+0x17c>
0.14 : c02cb3d8: lwz r9,8(r4)
0.08 : c02cb3dc: cmplw r9,r3
0.00 : c02cb3e0: mr r27,r9
0.03 : c02cb3e4: bgt c02cb4fc <iov_iter_zero+0x158>
1.34 : c02cb3e8: cmpwi r9,0
0.00 : c02cb3ec: beq c02cb4d0 <iov_iter_zero+0x12c>
0.11 : c02cb3f0: andi. r8,r10,16
0.17 : c02cb3f4: lwz r31,4(r30)
1.79 : c02cb3f8: bne c02cb61c <iov_iter_zero+0x278>
0.00 : c02cb3fc: andi. r8,r10,8
0.06 : c02cb400: bne c02cb770 <iov_iter_zero+0x3cc>
0.22 : c02cb404: andi. r10,r10,64
0.03 : c02cb408: bne c02cb88c <iov_iter_zero+0x4e8>
0.11 : c02cb40c: stw r29,68(r1)
1.59 : c02cb410: stw r28,64(r1)
0.03 : c02cb414: lwz r28,12(r30)
0.00 : c02cb418: lwz r7,4(r28)
1.87 : c02cb41c: subf r29,r31,r7
0.28 : c02cb420: cmplw r29,r27
0.03 : c02cb424: bgt c02cb50c <iov_iter_zero+0x168>
0.03 : c02cb428: cmpwi r29,0
0.00 : c02cb42c: beq c02cb898 <iov_iter_zero+0x4f4>
1.34 : c02cb430: lwz r3,0(r28)
: __access_ok():
0.00 : c02cb434: lis r10,-16384
: iov_iter_zero():
0.36 : c02cb438: add r3,r3,r31
: __access_ok():
0.03 : c02cb43c: cmplw r3,r10
1.79 : c02cb440: bge c02cb514 <iov_iter_zero+0x170>
13.19 : c02cb444: subf r10,r3,r10
: clear_user():
0.00 : c02cb448: cmplw r29,r10
4.41 : c02cb44c: mflr r0
0.00 : c02cb450: stw r0,84(r1)
0.00 : c02cb454: bgt c02cb8c4 <iov_iter_zero+0x520>
0.00 : c02cb458: mr r4,r29
0.00 : c02cb45c: bl c001a41c <__arch_clear_user>
: iov_iter_zero():
0.70 : c02cb460: add r31,r31,r29
0.00 : c02cb464: cmpwi r3,0
17.13 : c02cb468: subf r29,r29,r27
0.00 : c02cb46c: subf r31,r3,r31
1.20 : c02cb470: add r29,r29,r3
0.00 : c02cb474: beq c02cb8b8 <iov_iter_zero+0x514>
0.00 : c02cb478: lwz r9,8(r30)
0.00 : c02cb47c: subf r10,r27,r29
0.00 : c02cb480: lwz r0,84(r1)
0.00 : c02cb484: subf r27,r29,r27
0.00 : c02cb488: add r9,r10,r9
0.00 : c02cb48c: lwz r7,4(r28)
0.00 : c02cb490: lwz r10,12(r30)
0.00 : c02cb494: mtlr r0
1.65 : c02cb498: cmplw r31,r7
14.61 : c02cb49c: bne c02cb4a8 <iov_iter_zero+0x104>
1.65 : c02cb4a0: addi r28,r28,8
0.00 : c02cb4a4: li r31,0
14.92 : c02cb4a8: lwz r8,16(r30)
0.00 : c02cb4ac: subf r10,r10,r28
1.12 : c02cb4b0: srawi r10,r10,3
0.56 : c02cb4b4: stw r28,12(r30)
0.00 : c02cb4b8: subf r10,r10,r8
1.23 : c02cb4bc: stw r10,16(r30)
0.00 : c02cb4c0: lwz r28,64(r1)
0.56 : c02cb4c4: lwz r29,68(r1)
0.00 : c02cb4c8: stw r9,8(r30)
2.12 : c02cb4cc: stw r31,4(r30)
0.00 : c02cb4d0: lwz r9,28(r1)
0.61 : c02cb4d4: lwz r10,624(r2)
0.00 : c02cb4d8: xor. r9,r9,r10
0.00 : c02cb4dc: li r10,0
0.00 : c02cb4e0: bne c02cb9a8 <iov_iter_zero+0x604>
0.00 : c02cb4e4: mr r3,r27
0.00 : c02cb4e8: lwz r30,72(r1)
1.73 : c02cb4ec: lwz r27,60(r1)
0.50 : c02cb4f0: lwz r31,76(r1)
0.00 : c02cb4f4: addi r1,r1,80
0.00 : c02cb4f8: blr
0.00 : c02cb4fc: cmpwi r9,0
0.00 : c02cb500: mr r27,r3
0.00 : c02cb504: beq c02cb4d0 <iov_iter_zero+0x12c>
0.00 : c02cb508: b c02cb3f0 <iov_iter_zero+0x4c>
0.00 : c02cb50c: mr r29,r27
0.00 : c02cb510: b c02cb428 <iov_iter_zero+0x84>
: __access_ok():
0.00 : c02cb514: li r27,0
0.00 : c02cb518: mr r10,r28
0.00 : c02cb51c: b c02cb498 <iov_iter_zero+0xf4>
: pipe_zero():
0.00 : c02cb520: mflr r0
0.00 : c02cb524: stw r26,56(r1)
0.00 : c02cb528: stw r0,84(r1)
0.00 : c02cb52c: mr r3,r4
0.00 : c02cb530: stw r28,64(r1)
0.00 : c02cb534: lwz r28,12(r4)
0.00 : c02cb538: lwz r26,40(r28)
0.00 : c02cb53c: bl c02c8e48 <sanity>
0.00 : c02cb540: cmpwi r3,0
0.00 : c02cb544: bne c02cb560 <iov_iter_zero+0x1bc>
0.00 : c02cb548: lwz r0,84(r1)
0.00 : c02cb54c: li r27,0
0.00 : c02cb550: lwz r26,56(r1)
0.00 : c02cb554: lwz r28,64(r1)
0.00 : c02cb558: mtlr r0
0.00 : c02cb55c: b c02cb4d0 <iov_iter_zero+0x12c>
0.00 : c02cb560: mr r4,r31
0.00 : c02cb564: addi r6,r1,24
0.00 : c02cb568: addi r5,r1,20
0.00 : c02cb56c: mr r3,r30
0.00 : c02cb570: bl c02c9030 <push_pipe>
0.00 : c02cb574: mr. r27,r3
0.00 : c02cb578: beq c02cb548 <iov_iter_zero+0x1a4>
0.00 : c02cb57c: lwz r4,24(r1)
0.00 : c02cb580: addi r26,r26,-1
0.00 : c02cb584: lwz r9,20(r1)
0.00 : c02cb588: stw r25,52(r1)
0.00 : c02cb58c: li r25,0
0.00 : c02cb590: stw r29,68(r1)
0.00 : c02cb594: mr r29,r27
0.00 : c02cb598: subfic r31,r4,4096
0.00 : c02cb59c: cmplw r31,r29
0.00 : c02cb5a0: ble c02cb5a8 <iov_iter_zero+0x204>
0.00 : c02cb5a4: mr r31,r29
0.00 : c02cb5a8: and r9,r26,r9
0.00 : c02cb5ac: lwz r8,80(r28)
0.00 : c02cb5b0: rlwinm r10,r9,1,0,30
0.00 : c02cb5b4: add r9,r10,r9
0.00 : c02cb5b8: rlwinm r9,r9,3,0,28
0.00 : c02cb5bc: lwzx r3,r8,r9
0.00 : c02cb5c0: mr r5,r31
0.00 : c02cb5c4: bl c02c92ec <memzero_page>
0.00 : c02cb5c8: subf. r29,r31,r29
0.00 : c02cb5cc: lwz r9,20(r1)
0.00 : c02cb5d0: li r4,0
0.00 : c02cb5d4: lwz r10,24(r1)
0.00 : c02cb5d8: stw r9,16(r30)
0.00 : c02cb5dc: addi r9,r9,1
0.00 : c02cb5e0: add r10,r10,r31
0.00 : c02cb5e4: stw r9,20(r1)
0.00 : c02cb5e8: stw r10,4(r30)
0.00 : c02cb5ec: stw r25,24(r1)
0.00 : c02cb5f0: bne c02cb598 <iov_iter_zero+0x1f4>
0.00 : c02cb5f4: lwz r9,8(r30)
0.00 : c02cb5f8: subf r9,r27,r9
0.00 : c02cb5fc: stw r9,8(r30)
: iov_iter_zero():
0.00 : c02cb600: lwz r0,84(r1)
0.00 : c02cb604: lwz r25,52(r1)
0.00 : c02cb608: lwz r26,56(r1)
0.00 : c02cb60c: mtlr r0
0.00 : c02cb610: lwz r28,64(r1)
0.00 : c02cb614: lwz r29,68(r1)
0.00 : c02cb618: b c02cb4d0 <iov_iter_zero+0x12c>
0.00 : c02cb61c: stw r23,44(r1)
0.00 : c02cb620: cmpwi r27,0
0.00 : c02cb624: stw r28,64(r1)
0.00 : c02cb628: mr r23,r27
0.00 : c02cb62c: stw r24,48(r1)
0.00 : c02cb630: li r28,0
0.00 : c02cb634: lwz r24,12(r30)
0.00 : c02cb638: mr r8,r24
0.00 : c02cb63c: beq c02cb714 <iov_iter_zero+0x370>
0.00 : c02cb640: mflr r0
0.00 : c02cb644: stw r25,52(r1)
0.00 : c02cb648: stw r0,84(r1)
0.00 : c02cb64c: stw r26,56(r1)
0.00 : c02cb650: stw r29,68(r1)
0.00 : c02cb654: rlwinm r25,r28,1,0,30
0.00 : c02cb658: add r25,r25,r28
0.00 : c02cb65c: rlwinm r25,r25,2,0,29
0.00 : c02cb660: add r10,r8,r25
0.00 : c02cb664: lwz r26,4(r10)
0.00 : c02cb668: mr r29,r25
0.00 : c02cb66c: lwz r9,8(r10)
0.00 : c02cb670: subf r26,r31,r26
0.00 : c02cb674: cmplw r26,r23
0.00 : c02cb678: add r9,r31,r9
0.00 : c02cb67c: clrlwi r4,r9,20
0.00 : c02cb680: ble c02cb688 <iov_iter_zero+0x2e4>
0.00 : c02cb684: mr r26,r23
0.00 : c02cb688: subfic r7,r4,4096
0.00 : c02cb68c: cmplw r26,r7
0.00 : c02cb690: ble c02cb698 <iov_iter_zero+0x2f4>
0.00 : c02cb694: mr r26,r7
0.00 : c02cb698: cmpwi r26,0
0.00 : c02cb69c: beq c02cb6c0 <iov_iter_zero+0x31c>
0.00 : c02cb6a0: lwz r3,0(r10)
0.00 : c02cb6a4: rlwinm r9,r9,25,7,26
0.00 : c02cb6a8: mr r5,r26
0.00 : c02cb6ac: add r3,r3,r9
0.00 : c02cb6b0: bl c02c92ec <memzero_page>
: bvec_iter_advance():
0.00 : c02cb6b4: cmplw r23,r26
: iov_iter_zero():
0.00 : c02cb6b8: lwz r8,12(r30)
: bvec_iter_advance():
0.00 : c02cb6bc: blt c02cb850 <iov_iter_zero+0x4ac>
0.00 : c02cb6c0: add. r31,r31,r26
0.00 : c02cb6c4: subf r23,r26,r23
0.00 : c02cb6c8: addi r10,r8,4
0.00 : c02cb6cc: bne c02cb6e4 <iov_iter_zero+0x340>
0.00 : c02cb6d0: b c02cb6f0 <iov_iter_zero+0x34c>
0.00 : c02cb6d4: subf. r31,r9,r31
0.00 : c02cb6d8: addi r28,r28,1
0.00 : c02cb6dc: addi r29,r29,12
0.00 : c02cb6e0: beq c02cb760 <iov_iter_zero+0x3bc>
0.00 : c02cb6e4: lwzx r9,r10,r29
0.00 : c02cb6e8: cmplw r31,r9
0.00 : c02cb6ec: bge c02cb6d4 <iov_iter_zero+0x330>
: iov_iter_zero():
0.00 : c02cb6f0: cmpwi r23,0
0.00 : c02cb6f4: bne c02cb654 <iov_iter_zero+0x2b0>
0.00 : c02cb6f8: add r8,r8,r29
0.00 : c02cb6fc: lwz r0,84(r1)
0.00 : c02cb700: lwz r9,8(r30)
0.00 : c02cb704: lwz r25,52(r1)
0.00 : c02cb708: mtlr r0
0.00 : c02cb70c: lwz r26,56(r1)
0.00 : c02cb710: lwz r29,68(r1)
0.00 : c02cb714: subf r24,r24,r8
0.00 : c02cb718: stw r8,12(r30)
0.00 : c02cb71c: srawi r6,r24,2
0.00 : c02cb720: lwz r7,16(r30)
0.00 : c02cb724: rlwinm r10,r24,0,0,29
0.00 : c02cb728: add r10,r10,r6
0.00 : c02cb72c: rlwinm r8,r10,4,0,27
0.00 : c02cb730: add r10,r10,r8
0.00 : c02cb734: rlwinm r8,r10,8,0,23
0.00 : c02cb738: add r10,r10,r8
0.00 : c02cb73c: rlwinm r8,r10,16,0,15
0.00 : c02cb740: add r10,r10,r8
0.00 : c02cb744: add r10,r7,r10
0.00 : c02cb748: stw r10,16(r30)
0.00 : c02cb74c: subf r9,r27,r9
0.00 : c02cb750: lwz r23,44(r1)
0.00 : c02cb754: lwz r24,48(r1)
0.00 : c02cb758: lwz r28,64(r1)
0.00 : c02cb75c: b c02cb4c8 <iov_iter_zero+0x124>
0.00 : c02cb760: rlwinm r29,r28,1,0,30
0.00 : c02cb764: add r29,r29,r28
0.00 : c02cb768: rlwinm r29,r29,2,0,29
0.00 : c02cb76c: b c02cb6f0 <iov_iter_zero+0x34c>
0.00 : c02cb770: mflr r0
0.00 : c02cb774: stw r26,56(r1)
0.00 : c02cb778: stw r0,84(r1)
0.00 : c02cb77c: stw r28,64(r1)
0.00 : c02cb780: stw r29,68(r1)
0.00 : c02cb784: lwz r28,12(r30)
0.00 : c02cb788: lwz r29,4(r28)
0.00 : c02cb78c: subf r29,r31,r29
0.00 : c02cb790: cmplw r29,r27
0.00 : c02cb794: ble c02cb79c <iov_iter_zero+0x3f8>
0.00 : c02cb798: mr r29,r27
0.00 : c02cb79c: cmpwi r29,0
0.00 : c02cb7a0: beq c02cb8d8 <iov_iter_zero+0x534>
0.00 : c02cb7a4: lwz r3,0(r28)
0.00 : c02cb7a8: mr r5,r29
0.00 : c02cb7ac: li r4,0
0.00 : c02cb7b0: add r3,r3,r31
0.00 : c02cb7b4: subf r26,r29,r27
0.00 : c02cb7b8: bl c001999c <memset>
0.00 : c02cb7bc: add r31,r31,r29
0.00 : c02cb7c0: cmpwi r26,0
0.00 : c02cb7c4: bne c02cb818 <iov_iter_zero+0x474>
0.00 : c02cb7c8: lwz r9,4(r28)
0.00 : c02cb7cc: cmpw r9,r31
0.00 : c02cb7d0: bne c02cb7dc <iov_iter_zero+0x438>
0.00 : c02cb7d4: addi r28,r28,8
0.00 : c02cb7d8: li r31,0
0.00 : c02cb7dc: lwz r9,12(r30)
0.00 : c02cb7e0: lwz r8,16(r30)
0.00 : c02cb7e4: subf r10,r9,r28
0.00 : c02cb7e8: stw r28,12(r30)
0.00 : c02cb7ec: srawi r10,r10,3
0.00 : c02cb7f0: lwz r9,8(r30)
0.00 : c02cb7f4: subf r10,r10,r8
0.00 : c02cb7f8: stw r10,16(r30)
0.00 : c02cb7fc: subf r9,r27,r9
0.00 : c02cb800: lwz r0,84(r1)
0.00 : c02cb804: lwz r26,56(r1)
0.00 : c02cb808: lwz r28,64(r1)
0.00 : c02cb80c: mtlr r0
0.00 : c02cb810: lwz r29,68(r1)
0.00 : c02cb814: b c02cb4c8 <iov_iter_zero+0x124>
0.00 : c02cb818: lwz r31,12(r28)
0.00 : c02cb81c: addi r28,r28,8
0.00 : c02cb820: cmplw r31,r26
0.00 : c02cb824: ble c02cb82c <iov_iter_zero+0x488>
0.00 : c02cb828: mr r31,r26
0.00 : c02cb82c: cmpwi r31,0
0.00 : c02cb830: beq c02cb818 <iov_iter_zero+0x474>
0.00 : c02cb834: lwz r3,0(r28)
0.00 : c02cb838: mr r5,r31
0.00 : c02cb83c: li r4,0
0.00 : c02cb840: bl c001999c <memset>
0.00 : c02cb844: subf. r26,r31,r26
0.00 : c02cb848: beq c02cb7c8 <iov_iter_zero+0x424>
0.00 : c02cb84c: b c02cb818 <iov_iter_zero+0x474>
: bvec_iter_advance():
0.00 : c02cb850: lis r9,-16236
0.00 : c02cb854: lbz r10,-20170(r9)
0.00 : c02cb858: cmpwi r10,0
0.00 : c02cb85c: beq c02cb868 <iov_iter_zero+0x4c4>
: iov_iter_zero():
0.00 : c02cb860: add r8,r8,r25
0.00 : c02cb864: b c02cb6fc <iov_iter_zero+0x358>
: bvec_iter_advance():
0.00 : c02cb868: lis r3,-16253
0.00 : c02cb86c: li r10,1
0.00 : c02cb870: addi r3,r3,7580
0.00 : c02cb874: stb r10,-20170(r9)
0.00 : c02cb878: bl c0029b1c <__warn_printk>
0.00 : c02cb87c: twui r0,0
: iov_iter_zero():
0.00 : c02cb880: lwz r8,12(r30)
0.00 : c02cb884: add r8,r8,r25
0.00 : c02cb888: b c02cb6fc <iov_iter_zero+0x358>
0.00 : c02cb88c: add r31,r31,r27
0.00 : c02cb890: subf r9,r27,r9
0.00 : c02cb894: b c02cb4c8 <iov_iter_zero+0x124>
0.00 : c02cb898: mr r29,r27
0.00 : c02cb89c: cmpwi r29,0
1.65 : c02cb8a0: bne c02cb8e0 <iov_iter_zero+0x53c>
0.00 : c02cb8a4: lwz r9,8(r30)
0.53 : c02cb8a8: lwz r7,4(r28)
0.00 : c02cb8ac: lwz r10,12(r30)
0.00 : c02cb8b0: subf r9,r27,r9
0.00 : c02cb8b4: b c02cb498 <iov_iter_zero+0xf4>
0.25 : c02cb8b8: lwz r0,84(r1)
2.26 : c02cb8bc: mtlr r0
0.00 : c02cb8c0: b c02cb89c <iov_iter_zero+0x4f8>
: clear_user():
0.00 : c02cb8c4: lwz r0,84(r1)
0.00 : c02cb8c8: li r27,0
0.00 : c02cb8cc: mr r10,r28
0.00 : c02cb8d0: mtlr r0
0.00 : c02cb8d4: b c02cb498 <iov_iter_zero+0xf4>
: iov_iter_zero():
0.00 : c02cb8d8: mr r26,r27
0.00 : c02cb8dc: b c02cb7c0 <iov_iter_zero+0x41c>
0.00 : c02cb8e0: stw r26,56(r1)
0.00 : c02cb8e4: stw r25,52(r1)
: __access_ok():
0.00 : c02cb8e8: lis r25,-16384
: iov_iter_zero():
0.00 : c02cb8ec: lwz r7,12(r28)
0.00 : c02cb8f0: addi r26,r28,8
0.00 : c02cb8f4: mr r31,r29
0.00 : c02cb8f8: cmplw r29,r7
0.00 : c02cb8fc: ble c02cb904 <iov_iter_zero+0x560>
0.00 : c02cb900: mr r31,r7
0.00 : c02cb904: cmpwi r31,0
0.00 : c02cb908: beq c02cba04 <iov_iter_zero+0x660>
0.00 : c02cb90c: lwz r3,0(r26)
: __access_ok():
0.00 : c02cb910: cmplw r3,r25
0.00 : c02cb914: bge c02cb980 <iov_iter_zero+0x5dc>
0.00 : c02cb918: subf r9,r3,r25
: clear_user():
0.00 : c02cb91c: cmplw r31,r9
0.00 : c02cb920: mflr r0
0.00 : c02cb924: stw r0,84(r1)
0.00 : c02cb928: bgt c02cb978 <iov_iter_zero+0x5d4>
0.00 : c02cb92c: mr r4,r31
0.00 : c02cb930: bl c001a41c <__arch_clear_user>
: iov_iter_zero():
0.00 : c02cb934: subf r29,r31,r29
0.00 : c02cb938: cmpwi r3,0
0.00 : c02cb93c: subf r31,r3,r31
0.00 : c02cb940: add r29,r3,r29
0.00 : c02cb944: beq c02cb9cc <iov_iter_zero+0x628>
0.00 : c02cb948: lwz r9,8(r30)
0.00 : c02cb94c: subf r8,r27,r29
0.00 : c02cb950: lwz r0,84(r1)
0.00 : c02cb954: subf r27,r29,r27
0.00 : c02cb958: lwz r7,12(r28)
0.00 : c02cb95c: add r9,r8,r9
0.00 : c02cb960: mr r28,r26
0.00 : c02cb964: lwz r10,12(r30)
0.00 : c02cb968: lwz r25,52(r1)
0.00 : c02cb96c: mtlr r0
0.00 : c02cb970: lwz r26,56(r1)
0.00 : c02cb974: b c02cb498 <iov_iter_zero+0xf4>
0.00 : c02cb978: lwz r0,84(r1)
0.00 : c02cb97c: mtlr r0
0.00 : c02cb980: lwz r9,8(r30)
0.00 : c02cb984: subf r8,r27,r29
0.00 : c02cb988: mr r28,r26
0.00 : c02cb98c: lwz r10,12(r30)
0.00 : c02cb990: lwz r25,52(r1)
0.00 : c02cb994: add r9,r8,r9
0.00 : c02cb998: lwz r26,56(r1)
0.00 : c02cb99c: subf r27,r29,r27
0.00 : c02cb9a0: li r31,0
0.00 : c02cb9a4: b c02cb498 <iov_iter_zero+0xf4>
0.00 : c02cb9a8: mflr r0
0.00 : c02cb9ac: stw r23,44(r1)
0.00 : c02cb9b0: stw r0,84(r1)
0.00 : c02cb9b4: stw r24,48(r1)
0.00 : c02cb9b8: stw r25,52(r1)
0.00 : c02cb9bc: stw r26,56(r1)
0.00 : c02cb9c0: stw r28,64(r1)
0.00 : c02cb9c4: stw r29,68(r1)
0.00 : c02cb9c8: bl c071db48 <__stack_chk_fail>
0.00 : c02cb9cc: cmpwi r29,0
0.00 : c02cb9d0: bne c02cb9fc <iov_iter_zero+0x658>
0.00 : c02cb9d4: lwz r9,8(r30)
0.00 : c02cb9d8: lwz r0,84(r1)
0.00 : c02cb9dc: lwz r7,12(r28)
0.00 : c02cb9e0: subf r9,r27,r9
0.00 : c02cb9e4: mr r28,r26
0.00 : c02cb9e8: lwz r10,12(r30)
0.00 : c02cb9ec: lwz r25,52(r1)
0.00 : c02cb9f0: mtlr r0
0.00 : c02cb9f4: lwz r26,56(r1)
0.00 : c02cb9f8: b c02cb498 <iov_iter_zero+0xf4>
0.00 : c02cb9fc: lwz r0,84(r1)
0.00 : c02cba00: mtlr r0
0.00 : c02cba04: mr r28,r26
0.00 : c02cba08: b c02cb8ec <iov_iter_zero+0x548>
Christophe
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc v2
From: Matthew Wilcox @ 2020-09-01 17:42 UTC (permalink / raw)
To: Al Viro
Cc: linux-arch, Kees Cook, x86, linuxppc-dev, linux-kernel,
linux-fsdevel, Linus Torvalds, Christoph Hellwig
In-Reply-To: <20200901172512.GI1236603@ZenIV.linux.org.uk>
On Tue, Sep 01, 2020 at 06:25:12PM +0100, Al Viro wrote:
> On Tue, Sep 01, 2020 at 07:13:00PM +0200, Christophe Leroy wrote:
>
> > 10.92% dd [kernel.kallsyms] [k] iov_iter_zero
>
> Interesting... Could you get an instruction-level profile inside iov_iter_zero(),
> along with the disassembly of that sucker?
Also, does [1] make any difference? Probably not since it's translating
O flags into IOCB flags instead of RWF flags into IOCB flags. I wonder
if there's a useful trick we can play here ... something like:
static inline int iocb_flags(struct file *file)
{
int res = 0;
if (likely(!file->f_flags & O_APPEND | O_DIRECT | O_DSYNC | __O_SYNC)) && !IS_SYNC(file->f_mapping->host))
return res;
if (file->f_flags & O_APPEND)
res |= IOCB_APPEND;
if (file->f_flags & O_DIRECT)
res |= IOCB_DIRECT;
if ((file->f_flags & O_DSYNC) || IS_SYNC(file->f_mapping->host))
res |= IOCB_DSYNC;
if (file->f_flags & __O_SYNC)
res |= IOCB_SYNC;
return res;
}
Can we do something like force O_DSYNC to be set if the inode IS_SYNC()
at the time of open? Or is setting the sync bit on the inode required
to affect currently-open files?
[1] https://lore.kernel.org/linux-fsdevel/95de7ce4-9254-39f1-304f-4455f66bf0f4@kernel.dk/
^ 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