* Re: [PATCH] powerpc/mm: Fix CONFIG_PPC_KUAP_DEBUG on PPC32
From: Michael Ellerman @ 2020-04-25 23:51 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <540242f7d4573f7cdf1b3bf46bb35f743b2cd68f.1587124651.git.christophe.leroy@c-s.fr>
On Fri, 2020-04-17 at 11:58:36 UTC, Christophe Leroy wrote:
> CONFIG_PPC_KUAP_DEBUG is not selectable because it depends on PPC_32
> which doesn't exists.
>
> Fixing it leads to a deadlock due to a vital register getting
> clobbered in _switch().
>
> Change dependency to PPC32 and use r0 instead of r4 in _switch()
>
> Fixes: e2fb9f544431 ("powerpc/32: Prepare for Kernel Userspace Access Protection")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/feb8e960d780e170e992a70491eec9dd68f4dbf2
cheers
^ permalink raw reply
* Re: [PATCH v3] powerpc/setup_64: Set cache-line-size based on cache-block-size
From: Michael Ellerman @ 2020-04-25 23:51 UTC (permalink / raw)
To: Chris Packham, benh, christophe.leroy, tglx, paulus, cai, oss
Cc: Chris Packham, linuxppc-dev, linux-kernel
In-Reply-To: <20200416221908.7886-1-chris.packham@alliedtelesis.co.nz>
On Thu, 2020-04-16 at 22:19:08 UTC, Chris Packham wrote:
> If {i,d}-cache-block-size is set and {i,d}-cache-line-size is not, use
> the block-size value for both. Per the devicetree spec cache-line-size
> is only needed if it differs from the block size.
>
> Originally the code would fallback from block size to line size. An
> error message was printed if both properties were missing.
>
> Later the code was refactored to use clearer names and logic but it
> inadvertently made line size a required property. This caused the
> default values to be used and in turn leads to Power9 systems using the
> wrong size.
>
> Fixes: bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache lin=
> e")
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/94c0b013c98583614e1ad911e8795ca36da34a85
cheers
^ permalink raw reply
* Re: [PATCH] lib/mpi: Fix building for powerpc with clang
From: Michael Ellerman @ 2020-04-25 23:51 UTC (permalink / raw)
To: Nathan Chancellor, Herbert Xu
Cc: kbuild test robot, linux-kernel, clang-built-linux,
Paul Mackerras, linux-crypto, Nathan Chancellor, linuxppc-dev
In-Reply-To: <20200413195041.24064-1-natechancellor@gmail.com>
On Mon, 2020-04-13 at 19:50:42 UTC, Nathan Chancellor wrote:
> 0day reports over and over on an powerpc randconfig with clang:
>
> lib/mpi/generic_mpih-mul1.c:37:13: error: invalid use of a cast in a
> inline asm context requiring an l-value: remove the cast or build with
> -fheinous-gnu-extensions
>
> Remove the superfluous casts, which have been done previously for x86
> and arm32 in commit dea632cadd12 ("lib/mpi: fix build with clang") and
> commit 7b7c1df2883d ("lib/mpi/longlong.h: fix building with 32-bit
> x86").
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/991
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/5990cdee689c6885b27c6d969a3d58b09002b0bc
cheers
^ permalink raw reply
* Re: [musl] Re: New powerpc vdso calling convention
From: Nicholas Piggin @ 2020-04-25 23:07 UTC (permalink / raw)
To: Rich Felker
Cc: libc-alpha, Andy Lutomirski, musl, binutils, Adhemerval Zanella,
libc-dev, Thomas Gleixner, Vincenzo Frascino, linuxppc-dev
In-Reply-To: <20200425162204.GJ11469@brightrain.aerifal.cx>
Excerpts from Rich Felker's message of April 26, 2020 2:22 am:
> On Sat, Apr 25, 2020 at 08:56:54PM +1000, Nicholas Piggin wrote:
>> >> The ELF v2 ABI convention would suit it well, because the caller already
>> >> requires the function address for ctr, so having it in r12 will
>> >> eliminate the need for address calculation, which suits the vdso data
>> >> page access.
>> >>
>> >> Is there a need for ELF v1 specific calls as well, or could those just be
>> >> deprecated and remain on existing functions or required to use the ELF
>> >> v2 calls using asm wrappers?
>> >
>> > What's ELF v1 and ELF v2 ? Is ELF v1 what PPC32 uses ? If so, I'd say
>> > yes, it would be good to have it to avoid going through ASM in the middle..
>>
>> I'm not sure about PPC32. On PPC64, ELFv2 functions must be called with
>> their address in r12 if called at their global entry point. ELFv1 have a
>> function descriptor with call address and TOC in it, caller has to load
>> the TOC if it's global.
>>
>> The vdso doesn't have TOC, it has one global address (the vdso data
>> page) which it loads by calculating its own address.
>
> A function descriptor could be put in the VDSO data page, or as it's
> done now by glibc the vdso linkage code could create it. My leaning is
> to at least have a version of the code that's callable (with the right
> descriptor around it) by v1 binaries, but since musl does not use
> ELFv1 at all we really have no stake in this and I'm fine with
> whatever outcome users of v1 decide on.
I agree, I think it would be good to make it look as much like a normal
function as possible.
>> The kernel doesn't change the vdso based on whether it's called by a v1
>> or v2 userspace (it doesn't really know itself and would have to export
>> different functions). glibc has a hack to create something:
>
> I'm pretty sure it does know because signal invocation has to know
> whether the function pointer points to a descriptor or code. At least
> for FDPIC archs (similar to PPC64 ELFv1 function descriptors) it knows
> and has to know.
It knows on a per-executable basis (by looking at the ELF header). It
doesn't know per-system though so we can't patch the vdso accordingly.
But we could include both sets of entry points and map in the
appropriate one at exec time I think.
>> >> Is there a good reason for the system call fallback to go in the vdso
>> >> function rather than have the caller handle it?
>> >
>> > I've seen at least one while porting powerpc to the C VDSO: arguments
>> > toward VDSO functions are in volatile registers. If the caller has to
>> > call the fallback by itself, it has to save them before calling the
>> > VDSO, allthought in 99% of cases it won't use them again. With the
>> > fallback called by the VDSO itself, the arguments are still hot in
>> > volatile registers and ready for calling the fallback. That make it very
>> > easy to call them, see patch 5 in the series
>> > (https://patchwork.ozlabs.org/project/linuxppc-dev/patch/59bea35725ab4cefc67a678577da8b3ab7771af5.1587401492.git.christophe.leroy@c-s.fr/)
>
> This is actually a good reason not to spuriously fail and fallback. At
> present musl wouldn't take advantage of it because musl uses the
> fallback path for lazy initialization of the vdso function pointer and
> doesn't special-case the MIPS badness, but if it made a big difference
> we probably could shuffle things around to only do the fallback on
> archs that need it and avoid saving the input arg registers across the
> vdso call.
It's a point for it yes. I don't know if any libc or app would want to
instrument it or do special accounting or something for system calls.
Thanks,
Nick
^ permalink raw reply
* Re: New powerpc vdso calling convention
From: Rich Felker @ 2020-04-25 23:11 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-alpha, Andy Lutomirski, musl, binutils, Adhemerval Zanella,
libc-dev, Thomas Gleixner, Vincenzo Frascino, linuxppc-dev
In-Reply-To: <1587855423.jug0f1n0b8.astroid@bobo.none>
On Sun, Apr 26, 2020 at 08:58:19AM +1000, Nicholas Piggin wrote:
> Excerpts from Christophe Leroy's message of April 25, 2020 10:20 pm:
> >
> >
> > Le 25/04/2020 à 12:56, Nicholas Piggin a écrit :
> >> Excerpts from Christophe Leroy's message of April 25, 2020 5:47 pm:
> >>>
> >>>
> >>> Le 25/04/2020 à 07:22, Nicholas Piggin a écrit :
> >>>> As noted in the 'scv' thread, powerpc's vdso calling convention does not
> >>>> match the C ELF ABI calling convention (or the proposed scv convention).
> >>>> I think we could implement a new ABI by basically duplicating function
> >>>> entry points with different names.
> >>>
> >>> I think doing this is a real good idea.
> >>>
> >>> I've been working at porting powerpc VDSO to the GENERIC C VDSO, and the
> >>> main pitfall has been that our vdso calling convention is not compatible
> >>> with C calling convention, so we have go through an ASM entry/exit.
> >>>
> >>> See https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=171469
> >>>
> >>> We should kill this error flag return through CR[SO] and get it the
> >>> "modern" way like other architectectures implementing the C VDSO: return
> >>> 0 when successfull, return -err when failed.
> >>
> >> Agreed.
> >>
> >>>> The ELF v2 ABI convention would suit it well, because the caller already
> >>>> requires the function address for ctr, so having it in r12 will
> >>>> eliminate the need for address calculation, which suits the vdso data
> >>>> page access.
> >>>>
> >>>> Is there a need for ELF v1 specific calls as well, or could those just be
> >>>> deprecated and remain on existing functions or required to use the ELF
> >>>> v2 calls using asm wrappers?
> >>>
> >>> What's ELF v1 and ELF v2 ? Is ELF v1 what PPC32 uses ? If so, I'd say
> >>> yes, it would be good to have it to avoid going through ASM in the middle.
> >>
> >> I'm not sure about PPC32. On PPC64, ELFv2 functions must be called with
> >> their address in r12 if called at their global entry point. ELFv1 have a
> >> function descriptor with call address and TOC in it, caller has to load
> >> the TOC if it's global.
> >>
> >> The vdso doesn't have TOC, it has one global address (the vdso data
> >> page) which it loads by calculating its own address.
> >>
> >> The kernel doesn't change the vdso based on whether it's called by a v1
> >> or v2 userspace (it doesn't really know itself and would have to export
> >> different functions). glibc has a hack to create something:
> >>
> >> # define VDSO_IFUNC_RET(value) \
> >> ({ \
> >> static Elf64_FuncDesc vdso_opd = { .fd_toc = ~0x0 }; \
> >> vdso_opd.fd_func = (Elf64_Addr)value; \
> >> &vdso_opd; \
> >> })
> >>
> >> If we could make something which links more like any other dso with
> >> ELFv1, that would be good. Otherwise I think v2 is preferable so it
> >> doesn't have to calculate its own address.
> >
> > I see the following in glibc. So looks like PPC32 is like PPC64 elfv1.
> > By the way, they are talking about something not completely finished in
> > the kernel. Can we finish it ?
>
> Possibly can. It seems like a good idea to fix all loose ends if we are
> going to add new versions. Will have to check with the toolchain people
> to make sure we're doing the right thing.
"ELFv1" and "ELFv2" are PPC64-specific names for the old and new
version of the ELF psABI for PPC64. They have nothing at all to do
with PPC32 which is a completely different ABI from either.
Rich
^ permalink raw reply
* Re: New powerpc vdso calling convention
From: Nicholas Piggin @ 2020-04-25 22:58 UTC (permalink / raw)
To: binutils, Christophe Leroy, linuxppc-dev
Cc: Rich Felker, libc-alpha, Adhemerval Zanella, musl,
Andy Lutomirski, libc-dev, Thomas Gleixner, Vincenzo Frascino
In-Reply-To: <976551e8-229e-54c1-8fb2-c5df94b979c3@c-s.fr>
Excerpts from Christophe Leroy's message of April 25, 2020 10:20 pm:
>
>
> Le 25/04/2020 à 12:56, Nicholas Piggin a écrit :
>> Excerpts from Christophe Leroy's message of April 25, 2020 5:47 pm:
>>>
>>>
>>> Le 25/04/2020 à 07:22, Nicholas Piggin a écrit :
>>>> As noted in the 'scv' thread, powerpc's vdso calling convention does not
>>>> match the C ELF ABI calling convention (or the proposed scv convention).
>>>> I think we could implement a new ABI by basically duplicating function
>>>> entry points with different names.
>>>
>>> I think doing this is a real good idea.
>>>
>>> I've been working at porting powerpc VDSO to the GENERIC C VDSO, and the
>>> main pitfall has been that our vdso calling convention is not compatible
>>> with C calling convention, so we have go through an ASM entry/exit.
>>>
>>> See https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=171469
>>>
>>> We should kill this error flag return through CR[SO] and get it the
>>> "modern" way like other architectectures implementing the C VDSO: return
>>> 0 when successfull, return -err when failed.
>>
>> Agreed.
>>
>>>> The ELF v2 ABI convention would suit it well, because the caller already
>>>> requires the function address for ctr, so having it in r12 will
>>>> eliminate the need for address calculation, which suits the vdso data
>>>> page access.
>>>>
>>>> Is there a need for ELF v1 specific calls as well, or could those just be
>>>> deprecated and remain on existing functions or required to use the ELF
>>>> v2 calls using asm wrappers?
>>>
>>> What's ELF v1 and ELF v2 ? Is ELF v1 what PPC32 uses ? If so, I'd say
>>> yes, it would be good to have it to avoid going through ASM in the middle.
>>
>> I'm not sure about PPC32. On PPC64, ELFv2 functions must be called with
>> their address in r12 if called at their global entry point. ELFv1 have a
>> function descriptor with call address and TOC in it, caller has to load
>> the TOC if it's global.
>>
>> The vdso doesn't have TOC, it has one global address (the vdso data
>> page) which it loads by calculating its own address.
>>
>> The kernel doesn't change the vdso based on whether it's called by a v1
>> or v2 userspace (it doesn't really know itself and would have to export
>> different functions). glibc has a hack to create something:
>>
>> # define VDSO_IFUNC_RET(value) \
>> ({ \
>> static Elf64_FuncDesc vdso_opd = { .fd_toc = ~0x0 }; \
>> vdso_opd.fd_func = (Elf64_Addr)value; \
>> &vdso_opd; \
>> })
>>
>> If we could make something which links more like any other dso with
>> ELFv1, that would be good. Otherwise I think v2 is preferable so it
>> doesn't have to calculate its own address.
>
> I see the following in glibc. So looks like PPC32 is like PPC64 elfv1.
> By the way, they are talking about something not completely finished in
> the kernel. Can we finish it ?
Possibly can. It seems like a good idea to fix all loose ends if we are
going to add new versions. Will have to check with the toolchain people
to make sure we're doing the right thing.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Arvind Sankar @ 2020-04-25 22:25 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Jakub Jelinek, jgross, Michael Matz, Kees Cook,
Frédéric Pierret (fepitre), Peter Zijlstra,
Martin Liška, x86, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Arvind Sankar, Ingo Molnar,
Borislav Petkov, Andy Lutomirski, H. Peter Anvin, Paul Mackerras,
Thomas Gleixner, linuxppc-dev, boris.ostrovsky
In-Reply-To: <20200425191549.GF17645@gate.crashing.org>
On Sat, Apr 25, 2020 at 02:15:49PM -0500, Segher Boessenkool wrote:
> On Sat, Apr 25, 2020 at 08:53:13PM +0200, Borislav Petkov wrote:
> > On Sat, Apr 25, 2020 at 01:37:01PM -0500, Segher Boessenkool wrote:
> > > That is a lot more typing then
> > > asm("");
> >
> > That's why a macro with a hopefully more descriptive name would be
> > telling more than a mere asm("").
>
> My point is that you should explain at *every use* of this why you cannot
> have tail calls *there*. This is very unusual, after all.
>
> There are *very* few places where you want to prevent tail calls, that's
> why there is no attribute for it.
>
>
> Segher
Well, there is -fno-optimize-sibling-calls, but we wouldn't be able to
use it via the optimize attribute for the same reason we couldn't use
no-stack-protector.
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Borislav Petkov @ 2020-04-25 22:17 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Jakub Jelinek, jgross, Michael Matz, Kees Cook, Peter Zijlstra,
Martin Liška, x86, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Arvind Sankar, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin,
Frédéric Pierret (fepitre), Thomas Gleixner,
linuxppc-dev, boris.ostrovsky
In-Reply-To: <20200425191549.GF17645@gate.crashing.org>
On Sat, Apr 25, 2020 at 02:15:49PM -0500, Segher Boessenkool wrote:
> My point is that you should explain at *every use* of this why you cannot
> have tail calls *there*. This is very unusual, after all.
>
> There are *very* few places where you want to prevent tail calls, that's
> why there is no attribute for it.
Well, there is only one reason *why* so far - to prevent the stack
canary cookie from being checked before returning from the function
which set it. That could be explained once over the macro definition so
that it can be looked up.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC
From: Kuppuswamy, Sathyanarayanan @ 2020-04-25 20:46 UTC (permalink / raw)
To: Derrick, Jonathan, helgaas@kernel.org
Cc: bhelgaas@google.com, Patel, Mayurkumar, fred@fredlawl.com,
sbobroff@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
Wysocki, Rafael J, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, andriy.shevchenko@linux.intel.com,
olof@lixom.net, alex.williamson@redhat.com, oohall@gmail.com,
kbusch@kernel.org, rajatja@google.com,
mika.westerberg@linux.intel.com
In-Reply-To: <ea21d9475b0af277c7288504ff2cd32b3f91e4ba.camel@intel.com>
On 4/23/20 8:11 AM, Derrick, Jonathan wrote:
> Hi Sathyanarayanan,
>
> On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote:
>>
>> On 4/20/20 2:37 PM, Jon Derrick wrote:
>>> The existing portdrv model prevents DPC services without either OS
>>> control (_OSC) granted to AER services, a Host Bridge requesting Native
>>> AER, or using one of the 'pcie_ports=' parameters of 'native' or
>>> 'dpc-native'.
>>>
>>> The DPC port service driver itself will also fail to probe if the kernel
>>> assumes the port is using Firmware-First AER. It's a reasonable
>>> expectation that a port using Firmware-First AER will also be using
>>> Firmware-First DPC, however if a Host Bridge requests Native DPC, the
>>> DPC driver should allow it and not fail to bind due to AER capability
>>> settings.
>>>
>>> Host Bridges which request Native DPC port services will also likely
>>> request Native AER, however it shouldn't be a requirement. This patch
>>> allows ports on those Host Bridges to have DPC port services.
>>>
>>> This will avoid the unlikely situation where the port is Firmware-First
>>> AER and Native DPC, and a BIOS or switch firmware preconfiguration of
>>> the DPC trigger could result in unhandled DPC events.
>>>
>>> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
>>> ---
>>> drivers/pci/pcie/dpc.c | 3 ++-
>>> drivers/pci/pcie/portdrv_core.c | 3 ++-
>>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
>>> index 7621704..3f3106f 100644
>>> --- a/drivers/pci/pcie/dpc.c
>>> +++ b/drivers/pci/pcie/dpc.c
>>> @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev)
>>> int status;
>>> u16 ctl, cap;
>>>
>>> - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
>>> + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native &&
>>> + !pci_find_host_bridge(pdev->bus)->native_dpc)
>> Why do it in probe as well ? if host->native_dpc is not set then the
>> device DPC probe it self won't happen right ?
>
> Portdrv only enables the interrupt and allows the probe to occur.
Please check the following snippet of code (from portdrv_core.c).
IIUC, pcie_device_init() will not be called if PCIE_PORT_SERVICE_DPC is
not set in capabilities. Your change in portdrv_core.c already
selectively enables the PCIE_PORT_SERVICE_DPC service based on
native_dpc value.
So IMO, adding native_dpc check in dpc_probe() is redundant.
int pcie_port_device_register(struct pci_dev *dev)
/* Allocate child services if any */
status = -ENODEV;
nr_service = 0;
for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) {
int service = 1 << i;
if (!(capabilities & service))
continue;
if (!pcie_device_init(dev, service, irqs[i]))
nr_service++;
}
>
> The probe itself will still fail if there's a mixed-mode _OSC
> negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1
> for AER and no check for DPC.
>
> I don't know if such a platform will exist, but the kernel is already
> wired for 'dpc-native' so it makes sense to extend it for this..
>
> This transform might be more readable:
> if (pcie_aer_get_firmware_first(pdev) &&
> !(pcie_ports_dpc_native || hb->native_dpc))
>
>
>
>>> return -ENOTSUPP;
>>>
>>> status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
>>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>>> index 50a9522..f2139a1 100644
>>> --- a/drivers/pci/pcie/portdrv_core.c
>>> +++ b/drivers/pci/pcie/portdrv_core.c
>>> @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev)
>>> */
>>> if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
>>> pci_aer_available() &&
>>> - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
>>> + (pcie_ports_dpc_native || host->native_dpc ||
>>> + (services & PCIE_PORT_SERVICE_AER)))
>>> services |= PCIE_PORT_SERVICE_DPC;
>>>
>>> if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
>>>
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Segher Boessenkool @ 2020-04-25 19:15 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jakub Jelinek, jgross, Michael Matz, Kees Cook, Peter Zijlstra,
Martin Liška, x86, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Arvind Sankar, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin,
Frédéric Pierret (fepitre), Thomas Gleixner,
linuxppc-dev, boris.ostrovsky
In-Reply-To: <20200425185313.GD24294@zn.tnic>
On Sat, Apr 25, 2020 at 08:53:13PM +0200, Borislav Petkov wrote:
> On Sat, Apr 25, 2020 at 01:37:01PM -0500, Segher Boessenkool wrote:
> > That is a lot more typing then
> > asm("");
>
> That's why a macro with a hopefully more descriptive name would be
> telling more than a mere asm("").
My point is that you should explain at *every use* of this why you cannot
have tail calls *there*. This is very unusual, after all.
There are *very* few places where you want to prevent tail calls, that's
why there is no attribute for it.
Segher
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Borislav Petkov @ 2020-04-25 18:53 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Jakub Jelinek, jgross, Michael Matz, Kees Cook, Peter Zijlstra,
Martin Liška, x86, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Arvind Sankar, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin,
Frédéric Pierret (fepitre), Thomas Gleixner,
linuxppc-dev, boris.ostrovsky
In-Reply-To: <20200425183701.GE17645@gate.crashing.org>
On Sat, Apr 25, 2020 at 01:37:01PM -0500, Segher Boessenkool wrote:
> That is a lot more typing then
> asm("");
That's why a macro with a hopefully more descriptive name would be
telling more than a mere asm("").
> but more seriously, you probably should explain why you do not want a
> tail call *anyway*, and in such a comment you can say that is what the
> asm is for.
Yes, the final version will have a comment and the whole spiel. This
diff is just me polling the maintainers: "do you want this for your arch
too?" Well, the PPC maintainers only, actually.
The other call in init/main.c would be for everybody.
> I don't see anything that prevents the tailcall in current code either,
> fwiw.
Right, and I don't see a reason why gcc-10 would do that optimization on
x86 only but I better ask first.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Segher Boessenkool @ 2020-04-25 18:37 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jakub Jelinek, jgross, Michael Matz, Kees Cook, Peter Zijlstra,
Martin Liška, x86, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Arvind Sankar, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin,
Frédéric Pierret (fepitre), Thomas Gleixner,
linuxppc-dev, boris.ostrovsky
In-Reply-To: <20200425173140.GB24294@zn.tnic>
On Sat, Apr 25, 2020 at 07:31:40PM +0200, Borislav Petkov wrote:
> > There's also the one in init/main.c which is used by multiple
> > architectures. On x86 at least, the call to arch_call_rest_init at the
> > end of start_kernel does not get tail-call optimized by gcc-10, but I
> > don't see anything that actually prevents that from happening. We should
> > add the asm("") there as well I think, unless the compiler guys see
> > something about this function that will always prevent the optimization?
>
> Hmm, that's what I was afraid of - having to sprinkle this around. Yah, let's
> wait for compiler guys to have a look here and then maybe I'll convert that
> thing to a macro called
>
> compiler_prevent_tail_call_opt()
>
> or so, so that it can be sprinkled around. ;-\
That is a lot more typing then
asm("");
but more seriously, you probably should explain why you do not want a
tail call *anyway*, and in such a comment you can say that is what the
asm is for.
I don't see anything that prevents the tailcall in current code either,
fwiw.
Segher
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Borislav Petkov @ 2020-04-25 17:52 UTC (permalink / raw)
To: Arvind Sankar
Cc: Jakub Jelinek, jgross, x86, Kees Cook, Peter Zijlstra,
linuxppc-dev, Michael Matz, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin,
Frédéric Pierret (fepitre), Thomas Gleixner,
Martin Liška, boris.ostrovsky
In-Reply-To: <20200425173140.GB24294@zn.tnic>
On Sat, Apr 25, 2020 at 07:31:40PM +0200, Borislav Petkov wrote:
> Hmm, that's what I was afraid of - having to sprinkle this around. Yah, let's
> wait for compiler guys to have a look here and then maybe I'll convert that
> thing to a macro called
>
> compiler_prevent_tail_call_opt()
>
> or so, so that it can be sprinkled around. ;-\
IOW, something like this (ontop) which takes care of the xen case too.
If it needs to be used by all arches, then I'll split the patch:
---
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 73bf8450afa1..4f275ac7830b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -273,7 +273,7 @@ static void notrace start_secondary(void *unused)
* boot_init_stack_canary() and must not be checked before tail calling
* another function.
*/
- asm ("");
+ prevent_tail_call_optimization();
}
/**
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 8fb8a50a28b4..f2adb63b2d7c 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -93,6 +93,7 @@ asmlinkage __visible void cpu_bringup_and_idle(void)
cpu_bringup();
boot_init_stack_canary();
cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
+ prevent_tail_call_optimization();
}
void xen_smp_intr_free_pv(unsigned int cpu)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 034b0a644efc..73f889f64513 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -356,4 +356,7 @@ static inline void *offset_to_ptr(const int *off)
/* &a[0] degrades to a pointer: a different type from an array */
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
+
+#define prevent_tail_call_optimization() asm("")
+
#endif /* __LINUX_COMPILER_H */
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply related
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Borislav Petkov @ 2020-04-25 17:31 UTC (permalink / raw)
To: Arvind Sankar
Cc: Jakub Jelinek, jgross, x86, Kees Cook, Peter Zijlstra,
linuxppc-dev, Michael Matz, Nick Desaulniers, LKML,
Sergei Trofimovich, clang-built-linux, Ingo Molnar,
Paul Mackerras, Andy Lutomirski, H. Peter Anvin,
Frédéric Pierret (fepitre), Thomas Gleixner,
Martin Liška, boris.ostrovsky
In-Reply-To: <20200425150440.GA470719@rani.riverdale.lan>
On Sat, Apr 25, 2020 at 11:04:40AM -0400, Arvind Sankar wrote:
> I'd put the clause about stack protector being disabled and the
> tail-call one together, to make clear that you still need the never
> return and always inline bits.
Done.
> Also, this function is implemented by multiple arch's and they all
> have similar comments -- might be better to consolidate the comment in
> the generic (dummy) one in include/linux/stackprotector.h laying out
> the restrictions that arch implementations should follow?
I'm not sure gcc-10 does the same thing on other arches - I'd let gcc
guys chime in here and other arch maintainers to decide what to do.
> There's also the one in init/main.c which is used by multiple
> architectures. On x86 at least, the call to arch_call_rest_init at the
> end of start_kernel does not get tail-call optimized by gcc-10, but I
> don't see anything that actually prevents that from happening. We should
> add the asm("") there as well I think, unless the compiler guys see
> something about this function that will always prevent the optimization?
Hmm, that's what I was afraid of - having to sprinkle this around. Yah, let's
wait for compiler guys to have a look here and then maybe I'll convert that
thing to a macro called
compiler_prevent_tail_call_opt()
or so, so that it can be sprinkled around. ;-\
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 15/21] mm: memmap_init: iterate over memblock regions rather that check each PFN
From: Mike Rapoport @ 2020-04-25 16:49 UTC (permalink / raw)
To: David Hildenbrand
Cc: Rich Felker, linux-ia64, linux-doc, Catalin Marinas,
Heiko Carstens, Michal Hocko, James E.J. Bottomley, Max Filippov,
Guo Ren, linux-csky, linux-parisc, sparclinux, linux-hexagon,
linux-riscv, Mike Rapoport, Greg Ungerer, linux-arch, linux-s390,
linux-snps-arc, linux-c6x-dev, Baoquan He, Jonathan Corbet,
linux-sh, Helge Deller, x86, Russell King, Ley Foon Tan,
Yoshinori Sato, Geert Uytterhoeven, linux-arm-kernel, Mark Salter,
Matt Turner, linux-mips, uclinux-h8-devel, linux-xtensa,
linux-alpha, linux-um, linux-m68k, Tony Luck, Greentime Hu,
Paul Walmsley, Stafford Horne, Guan Xuetao, Hoan Tran,
Michal Simek, Thomas Bogendoerfer, Brian Cain, Nick Hu, linux-mm,
Vineet Gupta, linux-kernel, openrisc, Richard Weinberger,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <9143538a-4aaa-ca1d-9c8f-72ac949cf593@redhat.com>
On Fri, Apr 24, 2020 at 09:22:32AM +0200, David Hildenbrand wrote:
> On 12.04.20 21:48, Mike Rapoport wrote:
> > From: Baoquan He <bhe@redhat.com>
> >
> > When called during boot the memmap_init_zone() function checks if each PFN
> > is valid and actually belongs to the node being initialized using
> > early_pfn_valid() and early_pfn_in_nid().
> >
> > Each such check may cost up to O(log(n)) where n is the number of memory
> > banks, so for large amount of memory overall time spent in early_pfn*()
> > becomes substantial.
> >
> > Since the information is anyway present in memblock, we can iterate over
> > memblock memory regions in memmap_init() and only call memmap_init_zone()
> > for PFN ranges that are know to be valid and in the appropriate node.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > ---
> > mm/page_alloc.c | 26 ++++++++++++++++----------
> > 1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 7f6a3081edb8..c43ce8709457 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5995,14 +5995,6 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
> > * function. They do not exist on hotplugged memory.
> > */
>
> After this change, the comment above is stale. the "holes in boot-time
> mem_map" are handled by the caller now AFAIKs.
Right, will update in v2.
Thanks!
> > if (context == MEMMAP_EARLY) {
> > - if (!early_pfn_valid(pfn)) {
> > - pfn = next_pfn(pfn);
> > - continue;
> > - }
> > - if (!early_pfn_in_nid(pfn, nid)) {
> > - pfn++;
> > - continue;
> > - }
> > if (overlap_memmap_init(zone, &pfn))
> > continue;
> > if (defer_init(nid, pfn, end_pfn))
>
>
> --
> Thanks,
>
> David / dhildenb
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [musl] Re: New powerpc vdso calling convention
From: Rich Felker @ 2020-04-25 16:22 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-alpha, Andy Lutomirski, musl, binutils, Adhemerval Zanella,
libc-dev, Thomas Gleixner, Vincenzo Frascino, linuxppc-dev
In-Reply-To: <1587810370.tg8ym9yjpc.astroid@bobo.none>
On Sat, Apr 25, 2020 at 08:56:54PM +1000, Nicholas Piggin wrote:
> >> The ELF v2 ABI convention would suit it well, because the caller already
> >> requires the function address for ctr, so having it in r12 will
> >> eliminate the need for address calculation, which suits the vdso data
> >> page access.
> >>
> >> Is there a need for ELF v1 specific calls as well, or could those just be
> >> deprecated and remain on existing functions or required to use the ELF
> >> v2 calls using asm wrappers?
> >
> > What's ELF v1 and ELF v2 ? Is ELF v1 what PPC32 uses ? If so, I'd say
> > yes, it would be good to have it to avoid going through ASM in the middle..
>
> I'm not sure about PPC32. On PPC64, ELFv2 functions must be called with
> their address in r12 if called at their global entry point. ELFv1 have a
> function descriptor with call address and TOC in it, caller has to load
> the TOC if it's global.
>
> The vdso doesn't have TOC, it has one global address (the vdso data
> page) which it loads by calculating its own address.
A function descriptor could be put in the VDSO data page, or as it's
done now by glibc the vdso linkage code could create it. My leaning is
to at least have a version of the code that's callable (with the right
descriptor around it) by v1 binaries, but since musl does not use
ELFv1 at all we really have no stake in this and I'm fine with
whatever outcome users of v1 decide on.
> The kernel doesn't change the vdso based on whether it's called by a v1
> or v2 userspace (it doesn't really know itself and would have to export
> different functions). glibc has a hack to create something:
I'm pretty sure it does know because signal invocation has to know
whether the function pointer points to a descriptor or code. At least
for FDPIC archs (similar to PPC64 ELFv1 function descriptors) it knows
and has to know.
> >> Is there a good reason for the system call fallback to go in the vdso
> >> function rather than have the caller handle it?
> >
> > I've seen at least one while porting powerpc to the C VDSO: arguments
> > toward VDSO functions are in volatile registers. If the caller has to
> > call the fallback by itself, it has to save them before calling the
> > VDSO, allthought in 99% of cases it won't use them again. With the
> > fallback called by the VDSO itself, the arguments are still hot in
> > volatile registers and ready for calling the fallback. That make it very
> > easy to call them, see patch 5 in the series
> > (https://patchwork.ozlabs.org/project/linuxppc-dev/patch/59bea35725ab4cefc67a678577da8b3ab7771af5.1587401492.git.christophe.leroy@c-s.fr/)
This is actually a good reason not to spuriously fail and fallback. At
present musl wouldn't take advantage of it because musl uses the
fallback path for lazy initialization of the vdso function pointer and
doesn't special-case the MIPS badness, but if it made a big difference
we probably could shuffle things around to only do the fallback on
archs that need it and avoid saving the input arg registers across the
vdso call.
Rich
^ permalink raw reply
* Re: [PATCH] x86: Fix early boot crash on gcc-10, next try
From: Arvind Sankar @ 2020-04-25 15:04 UTC (permalink / raw)
To: Borislav Petkov
Cc: Peter Zijlstra, Arvind Sankar, Paul Mackerras, H. Peter Anvin,
boris.ostrovsky, x86, clang-built-linux, Ingo Molnar,
Frédéric Pierret (fepitre), Jakub Jelinek, Kees Cook,
Michael Matz, Sergei Trofimovich, Andy Lutomirski,
Thomas Gleixner, jgross, Martin Liška, Nick Desaulniers,
LKML, linuxppc-dev
In-Reply-To: <20200425085759.GA24294@zn.tnic>
On Sat, Apr 25, 2020 at 10:57:59AM +0200, Borislav Petkov wrote:
> On Fri, Apr 24, 2020 at 09:46:57PM -0400, Arvind Sankar wrote:
> > The comment above boot_init_stack_canary's definition should be updated
> > to note that it needs to be called from a function that, in addition to
> > not returning, either has stackprotector disabled or avoids ending in a
> > tail call.
>
> How's that?
>
> diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
> index 91e29b6a86a5..237a54f60d6b 100644
> --- a/arch/x86/include/asm/stackprotector.h
> +++ b/arch/x86/include/asm/stackprotector.h
> @@ -55,8 +55,12 @@
> /*
> * Initialize the stackprotector canary value.
> *
> - * NOTE: this must only be called from functions that never return,
> - * and it must always be inlined.
> + * NOTE: this must only be called from functions that never return, it must
> + * always be inlined and it should be called from a compilation unit for
> + * which stack protector is disabled.
> + *
> + * Alternatively, the caller should not end with a function call which gets
> + * tail-call optimized as that would lead to checking a modified canary value.
> */
> static __always_inline void boot_init_stack_canary(void)
> {
I'd put the clause about stack protector being disabled and the
tail-call one together, to make clear that you still need the never
return and always inline bits. Also, this function is implemented by
multiple arch's and they all have similar comments -- might be better to
consolidate the comment in the generic (dummy) one in
include/linux/stackprotector.h laying out the restrictions that arch
implementations should follow?
>
> > There are also other calls that likely need to be fixed as well -- in
> > init/main.c, arch/x86/xen/smp_pv.c, and there is a powerpc version of
> > start_secondary in arch/powerpc/kernel/smp.c which may also be affected.
>
> Yes, there was an attempt to fix former:
>
> https://lkml.kernel.org/r/20200413123535.10884-1-frederic.pierret@qubes-os.org
There's also the one in init/main.c which is used by multiple
architectures. On x86 at least, the call to arch_call_rest_init at the
end of start_kernel does not get tail-call optimized by gcc-10, but I
don't see anything that actually prevents that from happening. We should
add the asm("") there as well I think, unless the compiler guys see
something about this function that will always prevent the optimization?
Cc'ing PPC list for powerpc start_secondary.
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.4 69/78] powerpc/powernv/ioda: Fix ref count for devices with their own PE
From: Sasha Levin @ 2020-04-25 15:00 UTC (permalink / raw)
To: Frederic Barrat; +Cc: linuxppc-dev, linux-kernel, stable, Andrew Donnellan
In-Reply-To: <b4fcb316-4fe8-47ec-81c7-4a79b0543b15@linux.ibm.com>
On Tue, Apr 21, 2020 at 01:02:31PM +0200, Frederic Barrat wrote:
>
>
>Le 18/04/2020 à 16:40, Sasha Levin a écrit :
>>From: Frederic Barrat <fbarrat@linux.ibm.com>
>>
>>[ Upstream commit 05dd7da76986937fb288b4213b1fa10dbe0d1b33 ]
>
>
>This shouldn't be backported to stable.
I've dropped this and the other two commits you've pointed out from all
branches, thanks!
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()
From: Steven Rostedt @ 2020-04-25 14:11 UTC (permalink / raw)
To: Christopher M. Riedl; +Cc: Naveen N. Rao, linuxppc-dev
In-Reply-To: <20200425101014.0c1b5fe0@oasis.local.home>
On Sat, 25 Apr 2020 10:10:14 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> Deciding to BUG on not based on the return code of patch_instruction()
That was suppose to be "to BUG on or not,"
-- Steve
> is the way to go IMO.
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()
From: Steven Rostedt @ 2020-04-25 14:10 UTC (permalink / raw)
To: Christopher M. Riedl; +Cc: Naveen N. Rao, linuxppc-dev
In-Reply-To: <C29ONNE5PMZ3.2R5TT1FV2RFHC@geist>
On Fri, 24 Apr 2020 14:26:02 -0500
"Christopher M. Riedl" <cmr@informatik.wtf> wrote:
> On Fri Apr 24, 2020 at 9:15 AM, Steven Rostedt wrote:
> > On Thu, 23 Apr 2020 18:21:14 +0200
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >
> > > Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> > > > With STRICT_KERNEL_RWX, we are currently ignoring return value from
> > > > __patch_instruction() in do_patch_instruction(), resulting in the error
> > > > not being propagated back. Fix the same.
> > >
> > > Good patch.
> > >
> > > Be aware that there is ongoing work which tend to wanting to replace
> > > error reporting by BUG_ON() . See
> > > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
> >
> >
> > Thanks for the reference. I still believe that WARN_ON() should be used
> > in
> > 99% of the cases, including here. And only do a BUG_ON() when you know
> > there's no recovering from it.
> >
> >
> > In fact, there's still BUG_ON()s in my code that I need to convert to
> > WARN_ON() (it was written when BUG_ON() was still acceptable ;-)
> >
> Figured I'd chime in since I am working on that other series :) The
> BUG_ON()s are _only_ in the init code to set things up to allow a
> temporary mapping for patching a STRICT_RWX kernel later. There's no
> ongoing work to "replace error reporting by BUG_ON()". If that initial
> setup fails we cannot patch under STRICT_KERNEL_RWX at all which imo
> warrants a BUG_ON(). I am still working on v2 of my RFC which does
> return any __patch_instruction() error back to the caller of
> patch_instruction() similar to this patch.
I agree certain locations may warrant a BUG_ON(), but I wouldn't make a
generic operation like patch_instruction() BUG, as it may be used in
cases that do not warrant it (like setting up ftrace).
Deciding to BUG on not based on the return code of patch_instruction()
is the way to go IMO.
-- Steve
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
From: Steven Rostedt @ 2020-04-25 14:06 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Naveen N. Rao, linuxppc-dev
In-Reply-To: <e743c9db-847a-2612-bf36-c23a57a056c5@c-s.fr>
On Sat, 25 Apr 2020 10:11:56 +0000
Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
> Sure it's be more explicit, but then more lines also. 3 lines for only
> one really usefull.
>
> With goto, I would look like:
>
> diff --git a/arch/powerpc/kernel/optprobes.c
> b/arch/powerpc/kernel/optprobes.c
> index 046485bb0a52..938208f824da 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -139,14 +139,14 @@ void arch_remove_optimized_kprobe(struct
> optimized_kprobe *op)
> }
> }
>
> -#define PATCH_INSN(addr, instr) \
> +#define PATCH_INSN(addr, instr, label) \
With the explicit label as a parameter, makes it more evident that it
will do something (like jump) with that label.
I like this solution the best!
-- Steve
> do { \
> int rc = patch_instruction((unsigned int *)(addr), instr); \
> if (rc) { \
> pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
> __func__, __LINE__, \
> (void *)(addr), (void *)(addr), rc); \
> - return rc; \
> + goto label; \
> } \
> } while (0)
>
^ permalink raw reply
* Re: New powerpc vdso calling convention
From: Christophe Leroy @ 2020-04-25 12:20 UTC (permalink / raw)
To: Nicholas Piggin, binutils, linuxppc-dev
Cc: Rich Felker, libc-alpha, Adhemerval Zanella, musl,
Andy Lutomirski, libc-dev, Thomas Gleixner, Vincenzo Frascino
In-Reply-To: <1587810370.tg8ym9yjpc.astroid@bobo.none>
Le 25/04/2020 à 12:56, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of April 25, 2020 5:47 pm:
>>
>>
>> Le 25/04/2020 à 07:22, Nicholas Piggin a écrit :
>>> As noted in the 'scv' thread, powerpc's vdso calling convention does not
>>> match the C ELF ABI calling convention (or the proposed scv convention).
>>> I think we could implement a new ABI by basically duplicating function
>>> entry points with different names.
>>
>> I think doing this is a real good idea.
>>
>> I've been working at porting powerpc VDSO to the GENERIC C VDSO, and the
>> main pitfall has been that our vdso calling convention is not compatible
>> with C calling convention, so we have go through an ASM entry/exit.
>>
>> See https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=171469
>>
>> We should kill this error flag return through CR[SO] and get it the
>> "modern" way like other architectectures implementing the C VDSO: return
>> 0 when successfull, return -err when failed.
>
> Agreed.
>
>>> The ELF v2 ABI convention would suit it well, because the caller already
>>> requires the function address for ctr, so having it in r12 will
>>> eliminate the need for address calculation, which suits the vdso data
>>> page access.
>>>
>>> Is there a need for ELF v1 specific calls as well, or could those just be
>>> deprecated and remain on existing functions or required to use the ELF
>>> v2 calls using asm wrappers?
>>
>> What's ELF v1 and ELF v2 ? Is ELF v1 what PPC32 uses ? If so, I'd say
>> yes, it would be good to have it to avoid going through ASM in the middle.
>
> I'm not sure about PPC32. On PPC64, ELFv2 functions must be called with
> their address in r12 if called at their global entry point. ELFv1 have a
> function descriptor with call address and TOC in it, caller has to load
> the TOC if it's global.
>
> The vdso doesn't have TOC, it has one global address (the vdso data
> page) which it loads by calculating its own address.
>
> The kernel doesn't change the vdso based on whether it's called by a v1
> or v2 userspace (it doesn't really know itself and would have to export
> different functions). glibc has a hack to create something:
>
> # define VDSO_IFUNC_RET(value) \
> ({ \
> static Elf64_FuncDesc vdso_opd = { .fd_toc = ~0x0 }; \
> vdso_opd.fd_func = (Elf64_Addr)value; \
> &vdso_opd; \
> })
>
> If we could make something which links more like any other dso with
> ELFv1, that would be good. Otherwise I think v2 is preferable so it
> doesn't have to calculate its own address.
I see the following in glibc. So looks like PPC32 is like PPC64 elfv1.
By the way, they are talking about something not completely finished in
the kernel. Can we finish it ?
#if (defined(__PPC64__) || defined(__powerpc64__)) && _CALL_ELF != 2
/* The correct solution is for _dl_vdso_vsym to return the address of
the OPD
for the kernel VDSO function. That address would then be stored in the
__vdso_* variables and returned as the result of the IFUNC resolver
function.
Yet, the kernel does not contain any OPD entries for the VDSO functions
(incomplete implementation). However, PLT relocations for IFUNCs
still expect
the address of an OPD to be returned from the IFUNC resolver
function (since
PLT entries on PPC64 are just copies of OPDs). The solution for now
is to
create an artificial static OPD for each VDSO function returned by a
resolver
function. The TOC value is set to a non-zero value to avoid
triggering lazy
symbol resolution via .glink0/.plt0 for a zero TOC (requires
thread-safe PLT
sequences) when the dynamic linker isn't prepared for it e.g.
RTLD_NOW. None
of the kernel VDSO routines use the TOC or AUX values so any
non-zero value
will work. Note that function pointer comparisons will not use this
artificial
static OPD since those are resolved via ADDR64 relocations and will
point at
the non-IFUNC default OPD for the symbol. Lastly, because the IFUNC
relocations
are processed immediately at startup the resolver functions and this
code need
not be thread-safe, but if the caller writes to a PLT slot it must
do so in a
thread-safe manner with all the required barriers. */
#define VDSO_IFUNC_RET(value) \
({ \
static Elf64_FuncDesc vdso_opd = { .fd_toc = ~0x0 }; \
vdso_opd.fd_func = (Elf64_Addr)value; \
&vdso_opd; \
})
#else
#define VDSO_IFUNC_RET(value) ((void *) (value))
#endif
Christophe
^ permalink raw reply
* Re: New powerpc vdso calling convention
From: Nicholas Piggin @ 2020-04-25 10:56 UTC (permalink / raw)
To: binutils, Christophe Leroy, linuxppc-dev
Cc: Rich Felker, libc-alpha, Adhemerval Zanella, musl,
Andy Lutomirski, libc-dev, Thomas Gleixner, Vincenzo Frascino
In-Reply-To: <9371cac5-20bb-0552-2609-0d537f41fecd@c-s.fr>
Excerpts from Christophe Leroy's message of April 25, 2020 5:47 pm:
>
>
> Le 25/04/2020 à 07:22, Nicholas Piggin a écrit :
>> As noted in the 'scv' thread, powerpc's vdso calling convention does not
>> match the C ELF ABI calling convention (or the proposed scv convention).
>> I think we could implement a new ABI by basically duplicating function
>> entry points with different names.
>
> I think doing this is a real good idea.
>
> I've been working at porting powerpc VDSO to the GENERIC C VDSO, and the
> main pitfall has been that our vdso calling convention is not compatible
> with C calling convention, so we have go through an ASM entry/exit.
>
> See https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=171469
>
> We should kill this error flag return through CR[SO] and get it the
> "modern" way like other architectectures implementing the C VDSO: return
> 0 when successfull, return -err when failed.
Agreed.
>> The ELF v2 ABI convention would suit it well, because the caller already
>> requires the function address for ctr, so having it in r12 will
>> eliminate the need for address calculation, which suits the vdso data
>> page access.
>>
>> Is there a need for ELF v1 specific calls as well, or could those just be
>> deprecated and remain on existing functions or required to use the ELF
>> v2 calls using asm wrappers?
>
> What's ELF v1 and ELF v2 ? Is ELF v1 what PPC32 uses ? If so, I'd say
> yes, it would be good to have it to avoid going through ASM in the middle.
I'm not sure about PPC32. On PPC64, ELFv2 functions must be called with
their address in r12 if called at their global entry point. ELFv1 have a
function descriptor with call address and TOC in it, caller has to load
the TOC if it's global.
The vdso doesn't have TOC, it has one global address (the vdso data
page) which it loads by calculating its own address.
The kernel doesn't change the vdso based on whether it's called by a v1
or v2 userspace (it doesn't really know itself and would have to export
different functions). glibc has a hack to create something:
# define VDSO_IFUNC_RET(value) \
({ \
static Elf64_FuncDesc vdso_opd = { .fd_toc = ~0x0 }; \
vdso_opd.fd_func = (Elf64_Addr)value; \
&vdso_opd; \
})
If we could make something which links more like any other dso with
ELFv1, that would be good. Otherwise I think v2 is preferable so it
doesn't have to calculate its own address.
>> Is there a good reason for the system call fallback to go in the vdso
>> function rather than have the caller handle it?
>
> I've seen at least one while porting powerpc to the C VDSO: arguments
> toward VDSO functions are in volatile registers. If the caller has to
> call the fallback by itself, it has to save them before calling the
> VDSO, allthought in 99% of cases it won't use them again. With the
> fallback called by the VDSO itself, the arguments are still hot in
> volatile registers and ready for calling the fallback. That make it very
> easy to call them, see patch 5 in the series
> (https://patchwork.ozlabs.org/project/linuxppc-dev/patch/59bea35725ab4cefc67a678577da8b3ab7771af5.1587401492.git.christophe.leroy@c-s.fr/)
I see. Well the kernel can probably patch in sc or scv depending on
which is supported, so we could keep the automatic fallback.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
From: Christophe Leroy @ 2020-04-25 10:11 UTC (permalink / raw)
To: Naveen N. Rao, Steven Rostedt; +Cc: linuxppc-dev
In-Reply-To: <1587751684.agx3nt8uvf.naveen@linux.ibm.com>
On 04/24/2020 06:26 PM, Naveen N. Rao wrote:
> Steven Rostedt wrote:
>> On Thu, 23 Apr 2020 17:41:52 +0200
>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>> > diff --git a/arch/powerpc/kernel/optprobes.c
>>> b/arch/powerpc/kernel/optprobes.c
>>> > index 024f7aad1952..046485bb0a52 100644
>>> > --- a/arch/powerpc/kernel/optprobes.c
>>> > +++ b/arch/powerpc/kernel/optprobes.c
>>> > @@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct
>>> optimized_kprobe *op)
>>> > }
>>> > }
>>> > > +#define PATCH_INSN(addr, instr) \
>>> > +do { \
>>> > + int rc = patch_instruction((unsigned int *)(addr),
>>> instr); \
>>> > + if (rc) { \
>>> > + pr_err("%s:%d Error patching instruction at 0x%pK (%pS):
>>> %d\n", \
>>> > + __func__, __LINE__, \
>>> > + (void *)(addr), (void *)(addr), rc); \
>>> > + return rc; \
>>> > + } \
>>> > +} while (0)
>>> > +
>>> I hate this kind of macro which hides the "return".
>>>
>>> What about keeping the return action in the caller ?
>>>
>>> Otherwise, what about implementing something based on the use of
>>> goto, on the same model as unsafe_put_user() for instance ?
>
> Thanks for the review.
>
> I noticed this as a warning from checkpatch.pl, but this looked compact
> and correct for use in the two following functions. You'll notice that I
> added it just before the two functions this is used in.
>
> I suppose 'goto err' is usable too, but the ftrace code (patch 2) will
> end up with more changes. I'm also struggling to see how a 'goto' is
> less offensive. I think Steve's suggestion below would be the better way
> to go, to make things explicit.
>
Sure it's be more explicit, but then more lines also. 3 lines for only
one really usefull.
With goto, I would look like:
diff --git a/arch/powerpc/kernel/optprobes.c
b/arch/powerpc/kernel/optprobes.c
index 046485bb0a52..938208f824da 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,14 +139,14 @@ void arch_remove_optimized_kprobe(struct
optimized_kprobe *op)
}
}
-#define PATCH_INSN(addr, instr) \
+#define PATCH_INSN(addr, instr, label) \
do { \
int rc = patch_instruction((unsigned int *)(addr), instr); \
if (rc) { \
pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
__func__, __LINE__, \
(void *)(addr), (void *)(addr), rc); \
- return rc; \
+ goto label; \
} \
} while (0)
@@ -159,14 +159,17 @@ static int patch_imm32_load_insns(unsigned int
val, kprobe_opcode_t *addr)
{
/* addis r4,0,(insn)@h */
PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
- ((val >> 16) & 0xffff));
+ ((val >> 16) & 0xffff), failed);
addr++;
/* ori r4,r4,(insn)@l */
PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(4) |
- ___PPC_RS(4) | (val & 0xffff));
+ ___PPC_RS(4) | (val & 0xffff), failed);
return 0;
+
+failed:
+ return -EFAULT;
}
/*
@@ -177,29 +180,32 @@ static int patch_imm64_load_insns(unsigned long
val, kprobe_opcode_t *addr)
{
/* lis r3,(op)@highest */
PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
- ((val >> 48) & 0xffff));
+ ((val >> 48) & 0xffff), failed);
addr++;
/* ori r3,r3,(op)@higher */
PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
- ___PPC_RS(3) | ((val >> 32) & 0xffff));
+ ___PPC_RS(3) | ((val >> 32) & 0xffff), failed);
addr++;
/* rldicr r3,r3,32,31 */
PATCH_INSN(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
- ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31));
+ ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31), failed);
addr++;
/* oris r3,r3,(op)@h */
PATCH_INSN(addr, PPC_INST_ORIS | ___PPC_RA(3) |
- ___PPC_RS(3) | ((val >> 16) & 0xffff));
+ ___PPC_RS(3) | ((val >> 16) & 0xffff), failed);
addr++;
/* ori r3,r3,(op)@l */
PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
- ___PPC_RS(3) | (val & 0xffff));
+ ___PPC_RS(3) | (val & 0xffff), failed);
return 0;
+
+failed:
+ return -EFAULT;
}
int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct
kprobe *p)
@@ -291,23 +297,8 @@ int arch_prepare_optimized_kprobe(struct
optimized_kprobe *op, struct kprobe *p)
goto error;
}
- rc = patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
- if (rc) {
- pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
- __func__, __LINE__,
- (void *)(buff + TMPL_CALL_HDLR_IDX), rc);
- rc = -EFAULT;
- goto error;
- }
-
- rc = patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
- if (rc) {
- pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
- __func__, __LINE__,
- (void *)(buff + TMPL_EMULATE_IDX), rc);
- rc = -EFAULT;
- goto error;
- }
+ PATCH_INSN(buff + TMPL_CALL_HDLR_IDX, branch_op_callback, efault);
+ PATCH_INSN(buff + TMPL_EMULATE_IDX, branch_emulate_step, efault);
/*
* 3. load instruction to be emulated into relevant register, and
@@ -336,6 +327,8 @@ int arch_prepare_optimized_kprobe(struct
optimized_kprobe *op, struct kprobe *p)
return 0;
+efault:
+ rc = -EFAULT;
error:
free_ppc_optinsn_slot(buff, 0);
return rc;
Christophe
^ permalink raw reply related
* [PATCH v4 12/13] powerpc/40x: Avoid using r12 in TLB miss handlers
From: Christophe Leroy @ 2020-04-25 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
michal.simek, arnd
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <cover.1587804057.git.christophe.leroy@c-s.fr>
Let's reduce the number of registers used in TLB miss handlers.
We have both r9 and r12 available for any temporary use.
r9 is enough, avoid using r12.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/head_40x.S | 70 ++++++++++++++++------------------
1 file changed, 33 insertions(+), 37 deletions(-)
diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
index afa93a36437b..804cbd0899ac 100644
--- a/arch/powerpc/kernel/head_40x.S
+++ b/arch/powerpc/kernel/head_40x.S
@@ -254,9 +254,9 @@ _ENTRY(saved_ksp_limit)
mtspr SPRN_SPRG_SCRATCH3, r12
mtspr SPRN_SPRG_SCRATCH4, r9
mfcr r11
- mfspr r12, SPRN_PID
+ mfspr r9, SPRN_PID
mtspr SPRN_SPRG_SCRATCH6, r11
- mtspr SPRN_SPRG_SCRATCH5, r12
+ mtspr SPRN_SPRG_SCRATCH5, r9
mfspr r10, SPRN_DEAR /* Get faulting address */
/* If we are faulting a kernel address, we have to use the
@@ -279,12 +279,12 @@ _ENTRY(saved_ksp_limit)
4:
tophys(r11, r11)
rlwimi r11, r10, 12, 20, 29 /* Create L1 (pgdir/pmd) address */
- lwz r12, 0(r11) /* Get L1 entry */
- andi. r9, r12, _PMD_PRESENT /* Check if it points to a PTE page */
+ lwz r11, 0(r11) /* Get L1 entry */
+ andi. r9, r11, _PMD_PRESENT /* Check if it points to a PTE page */
beq 2f /* Bail if no table */
- rlwimi r12, r10, 22, 20, 29 /* Compute PTE address */
- lwz r11, 0(r12) /* Get Linux PTE */
+ rlwimi r11, r10, 22, 20, 29 /* Compute PTE address */
+ lwz r11, 0(r11) /* Get Linux PTE */
#ifdef CONFIG_SWAP
li r9, _PAGE_PRESENT | _PAGE_ACCESSED
#else
@@ -300,13 +300,13 @@ _ENTRY(saved_ksp_limit)
/* Create TLB tag. This is the faulting address plus a static
* set of bits. These are size, valid, E, U0.
*/
- li r12, 0x00c0
- rlwimi r10, r12, 0, 20, 31
+ li r9, 0x00c0
+ rlwimi r10, r9, 0, 20, 31
b finish_tlb_load
2: /* Check for possible large-page pmd entry */
- rlwinm. r9, r12, 2, 22, 24
+ rlwinm. r9, r11, 2, 22, 24
beq 5f
/* Create TLB tag. This is the faulting address, plus a static
@@ -314,7 +314,6 @@ _ENTRY(saved_ksp_limit)
*/
ori r9, r9, 0x40
rlwimi r10, r9, 0, 20, 31
- mr r11, r12
b finish_tlb_load
@@ -322,9 +321,9 @@ _ENTRY(saved_ksp_limit)
/* The bailout. Restore registers to pre-exception conditions
* and call the heavyweights to help us out.
*/
- mfspr r12, SPRN_SPRG_SCRATCH5
+ mfspr r9, SPRN_SPRG_SCRATCH5
mfspr r11, SPRN_SPRG_SCRATCH6
- mtspr SPRN_PID, r12
+ mtspr SPRN_PID, r9
mtcr r11
mfspr r9, SPRN_SPRG_SCRATCH4
mfspr r12, SPRN_SPRG_SCRATCH3
@@ -342,9 +341,9 @@ _ENTRY(saved_ksp_limit)
mtspr SPRN_SPRG_SCRATCH3, r12
mtspr SPRN_SPRG_SCRATCH4, r9
mfcr r11
- mfspr r12, SPRN_PID
+ mfspr r9, SPRN_PID
mtspr SPRN_SPRG_SCRATCH6, r11
- mtspr SPRN_SPRG_SCRATCH5, r12
+ mtspr SPRN_SPRG_SCRATCH5, r9
mfspr r10, SPRN_SRR0 /* Get faulting address */
/* If we are faulting a kernel address, we have to use the
@@ -367,12 +366,12 @@ _ENTRY(saved_ksp_limit)
4:
tophys(r11, r11)
rlwimi r11, r10, 12, 20, 29 /* Create L1 (pgdir/pmd) address */
- lwz r12, 0(r11) /* Get L1 entry */
- andi. r9, r12, _PMD_PRESENT /* Check if it points to a PTE page */
+ lwz r11, 0(r11) /* Get L1 entry */
+ andi. r9, r11, _PMD_PRESENT /* Check if it points to a PTE page */
beq 2f /* Bail if no table */
- rlwimi r12, r10, 22, 20, 29 /* Compute PTE address */
- lwz r11, 0(r12) /* Get Linux PTE */
+ rlwimi r11, r10, 22, 20, 29 /* Compute PTE address */
+ lwz r11, 0(r11) /* Get Linux PTE */
#ifdef CONFIG_SWAP
li r9, _PAGE_PRESENT | _PAGE_ACCESSED | _PAGE_EXEC
#else
@@ -388,13 +387,13 @@ _ENTRY(saved_ksp_limit)
/* Create TLB tag. This is the faulting address plus a static
* set of bits. These are size, valid, E, U0.
*/
- li r12, 0x00c0
- rlwimi r10, r12, 0, 20, 31
+ li r9, 0x00c0
+ rlwimi r10, r9, 0, 20, 31
b finish_tlb_load
2: /* Check for possible large-page pmd entry */
- rlwinm. r9, r12, 2, 22, 24
+ rlwinm. r9, r11, 2, 22, 24
beq 5f
/* Create TLB tag. This is the faulting address, plus a static
@@ -402,7 +401,6 @@ _ENTRY(saved_ksp_limit)
*/
ori r9, r9, 0x40
rlwimi r10, r9, 0, 20, 31
- mr r11, r12
b finish_tlb_load
@@ -410,9 +408,9 @@ _ENTRY(saved_ksp_limit)
/* The bailout. Restore registers to pre-exception conditions
* and call the heavyweights to help us out.
*/
- mfspr r12, SPRN_SPRG_SCRATCH5
+ mfspr r9, SPRN_SPRG_SCRATCH5
mfspr r11, SPRN_SPRG_SCRATCH6
- mtspr SPRN_PID, r12
+ mtspr SPRN_PID, r9
mtcr r11
mfspr r9, SPRN_SPRG_SCRATCH4
mfspr r12, SPRN_SPRG_SCRATCH3
@@ -528,7 +526,7 @@ WDTException:
* miss get to this point to load the TLB.
* r10 - TLB_TAG value
* r11 - Linux PTE
- * r12, r9 - available to use
+ * r9 - available to use
* PID - loaded with proper value when we get here
* Upon exit, we reload everything and RFI.
* Actually, it will fit now, but oh well.....a common place
@@ -537,30 +535,28 @@ WDTException:
tlb_4xx_index:
.long 0
finish_tlb_load:
- /* load the next available TLB index.
- */
- lwz r9, tlb_4xx_index@l(0)
- addi r9, r9, 1
- andi. r9, r9, (PPC40X_TLB_SIZE-1)
- stw r9, tlb_4xx_index@l(0)
-
-6:
/*
* Clear out the software-only bits in the PTE to generate the
* TLB_DATA value. These are the bottom 2 bits of the RPM, the
* top 3 bits of the zone field, and M.
*/
- li r12, 0x0ce2
- andc r11, r11, r12
+ li r9, 0x0ce2
+ andc r11, r11, r9
+
+ /* load the next available TLB index. */
+ lwz r9, tlb_4xx_index@l(0)
+ addi r9, r9, 1
+ andi. r9, r9, PPC40X_TLB_SIZE - 1
+ stw r9, tlb_4xx_index@l(0)
tlbwe r11, r9, TLB_DATA /* Load TLB LO */
tlbwe r10, r9, TLB_TAG /* Load TLB HI */
/* Done...restore registers and get out of here.
*/
- mfspr r12, SPRN_SPRG_SCRATCH5
+ mfspr r9, SPRN_SPRG_SCRATCH5
mfspr r11, SPRN_SPRG_SCRATCH6
- mtspr SPRN_PID, r12
+ mtspr SPRN_PID, r9
mtcr r11
mfspr r9, SPRN_SPRG_SCRATCH4
mfspr r12, SPRN_SPRG_SCRATCH3
--
2.25.0
^ permalink raw reply related
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