Re: [PATCH 0/2] powerpc/perf: Add instruction and data address registers to extended regs
Em Mon, Sep 06, 2021 at 08:13:13AM +0530, Athira Rajeev escreveu: > > > > On 02-Sep-2021, at 1:04 PM, kajoljain wrote: > > > > > > > > On 6/20/21 8:15 PM, Athira Rajeev wrote: > >> Patch set adds PMU registers namely Sampled Instruction Address Register > >> (SIAR) and Sampled Data Address Register (SDAR) as part of extended regs > >> in PowerPC. These registers provides the instruction/data address and > >> adding these to extended regs helps in debug purposes. > >> > >> Patch 1/2 adds SIAR and SDAR as part of the extended regs mask. > >> Patch 2/2 includes perf tools side changes to add the SPRs to > >> sample_reg_mask to use with -I? option. > >> > >> Athira Rajeev (2): > >> powerpc/perf: Expose instruction and data address registers as part of > >>extended regs > >> tools/perf: Add perf tools support to expose instruction and data > >>address registers as part of extended regs > >> > > > > Patchset looks good to me. > > > > Reviewed-By: kajol Jain > > Hi Arnaldo, > > Requesting for your review on this patchset. So, this touches the kernel, usually I get a patchkit when the kernel bits landed, is that the case by now? - Arnaldo > > Thanks > Athira > > > > Thanks, > > Kajol Jain > > > >> arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++- > >> arch/powerpc/perf/perf_regs.c | 4 > >> tools/arch/powerpc/include/uapi/asm/perf_regs.h | 12 +++- > >> tools/perf/arch/powerpc/include/perf_regs.h | 2 ++ > >> tools/perf/arch/powerpc/util/perf_regs.c| 2 ++ > >> 5 files changed, 22 insertions(+), 10 deletions(-) -- - Arnaldo
Re: [PATCH v2 3/5] signal: Add unsafe_copy_siginfo_to_user()
Christophe Leroy writes:
> On 9/8/21 6:17 PM, Eric W. Biederman wrote:
>> Christophe Leroy writes:
>>
>>> Le 02/09/2021 à 20:43, Eric W. Biederman a écrit :
Christophe Leroy writes:
> In the same spirit as commit fb05121fd6a2 ("signal: Add
> unsafe_get_compat_sigset()"), implement an 'unsafe' version of
> copy_siginfo_to_user() in order to use it within user access blocks.
>
> For that, also add an 'unsafe' version of clear_user().
Looking at your use cases you need the 32bit compat version of this
as well.
The 32bit compat version is too complicated to become a macro, so I
don't think you can make this work correctly for the 32bit compat case.
>>>
>>> When looking into patch 5/5 that you nacked, I think you missed the fact
>>> that we
>>> keep using copy_siginfo_to_user32() as it for the 32 bit compat case.
>>
>> I did. My mistake.
>>
>> However that mistake was so easy I think it mirrors the comments others
>> have made that this looks like a maintenance hazard.
>>
>> Is improving the performance of 32bit kernels interesting?
>
> Yes it is, and that's what this series do.
>
>> Is improving the performance of 32bit compat support interesting?
>
> For me this is a corner case, so I left it aside for now.
>
>>
>> If performance one or either of those cases is interesting it looks like
>> we already have copy_siginfo_to_external32 the factor you would need
>> to build unsafe_copy_siginfo_to_user32.
>
> I'm not sure I understand your saying here. What do you expect me to
> do with copy_siginfo_to_external32() ?
Implement unsafe_copy_siginfo_to_user32.
> copy_siginfo_to_user32() is for compat only.
>
> Native 32 bits powerpc use copy_siginfo_to_user()
What you implemented doubles the number of test cases necessary to
compile test the 32bit ppc signal code, and makes the code noticeably
harder to follow.
Having a unsafe_copy_to_siginfo_to_user32 at least would allow the
number of test cases to remain the same as the current code.
>> So I am not going to say impossible but please make something
>> maintainable. I unified all of the compat 32bit siginfo logic because
>> it simply did not get enough love and attention when it was implemented
>> per architecture.
>
> Yes, and ? I didn't do any modification to the compat case, so what
> you did remains.
You undid the unification between the 32bit code and the 32bit compat
code.
>> In general I think that concern applies to this case as well. We really
>> need an implementation that shares as much burden as possible with other
>> architectures.
>
> I think yes, that's the reason why I made a generic
> unsafe_copy_siginfo_to_user() and didn't make a powerpc dedicated
> change.
>
> Once this is merged any other architecture can use
> unsafe_copy_siginfo_to_user().
>
> Did I miss something ?
Not dealing with the compat case and making the code signal stack frame
code noticeably more complicated.
If this optimization profitably applies to other architectures we need
to figure out how to implement unsafe_copy_siginfo_to_user32 or risk
making them all much worse to maintain.
Eric
Re: [PATCH 06/10] powerpc: remove GCC version check for UPD_CONSTR
Le 11/09/2021 à 01:40, Nick Desaulniers a écrit : Now that GCC 5.1 is the minimum supported version, we can drop this workaround for older versions of GCC. This adversely affected clang, too. Why do you say that GCC 5.1 is the minimum supported ? As far as I can see, the minimum supported is still 4.9, see https://github.com/torvalds/linux/blob/master/Documentation/process/changes.rst Cc: Michael Ellerman Cc: Benjamin Herrenschmidt Cc: Paul Mackerras Cc: Segher Boessenkool Cc: Christophe Leroy Cc: linuxppc-dev@lists.ozlabs.org Signed-off-by: Nick Desaulniers --- arch/powerpc/include/asm/asm-const.h | 10 -- 1 file changed, 10 deletions(-) diff --git a/arch/powerpc/include/asm/asm-const.h b/arch/powerpc/include/asm/asm-const.h index 0ce2368bd20f..dbfa5e1e3198 100644 --- a/arch/powerpc/include/asm/asm-const.h +++ b/arch/powerpc/include/asm/asm-const.h @@ -12,16 +12,6 @@ # define ASM_CONST(x)__ASM_CONST(x) #endif -/* - * Inline assembly memory constraint - * - * GCC 4.9 doesn't properly handle pre update memory constraint "m<>" - * - */ -#if defined(GCC_VERSION) && GCC_VERSION < 5 -#define UPD_CONSTR "" -#else #define UPD_CONSTR "<>" -#endif There is no point in keeping UPD_CONSTR if it becomes invariant. You should just replace all instances of UPD_CONSTR with <> and drop UPD_CONSTR completely. Christophe
Re: [PATCH AUTOSEL 5.14 38/99] KVM: PPC: Book3S HV: XICS: Fix mapping of passthrough interrupts
On Fri, Sep 10, 2021 at 07:48:18AM +0200, Cédric Le Goater wrote:
On 9/10/21 2:14 AM, Sasha Levin wrote:
From: Cédric Le Goater
[ Upstream commit 1753081f2d445f9157550692fcc4221cd3ff0958 ]
PCI MSIs now live in an MSI domain but the underlying calls, which
will EOI the interrupt in real mode, need an HW IRQ number mapped in
the XICS IRQ domain. Grab it there.
Signed-off-by: Cédric Le Goater
Signed-off-by: Michael Ellerman
Link: https://lore.kernel.org/r/20210701132750.1475580-31-...@kaod.org
Signed-off-by: Sasha Levin
Why are we backporting this patch in stable trees ?
It should be fine but to compile, we need a partial backport of commit
51be9e51a800 ("KVM: PPC: Book3S HV: XIVE: Fix mapping of passthrough
interrupts") which exports irq_get_default_host().
Or, I can drop it if it makes no sense?
--
Thanks,
Sasha
Re: [PATCH 1/1] selftests/powerpc: Add memmove_64 test
Ritesh Harjani writes: > While debugging an issue, we wanted to check whether the arch specific > kernel memmove implementation is correct. This selftest could help test that. > > Suggested-by: Aneesh Kumar K.V > Suggested-by: Vaibhav Jain > Signed-off-by: Ritesh Harjani > --- > tools/testing/selftests/powerpc/Makefile | 1 + > .../selftests/powerpc/memmoveloop/.gitignore | 2 + > .../selftests/powerpc/memmoveloop/Makefile| 19 +++ > .../powerpc/memmoveloop/asm/asm-compat.h | 0 > .../powerpc/memmoveloop/asm/export.h | 4 ++ > .../powerpc/memmoveloop/asm/feature-fixups.h | 0 > .../selftests/powerpc/memmoveloop/asm/kasan.h | 0 > .../powerpc/memmoveloop/asm/ppc_asm.h | 39 + > .../powerpc/memmoveloop/asm/processor.h | 0 > .../selftests/powerpc/memmoveloop/mem_64.S| 1 + > .../selftests/powerpc/memmoveloop/memcpy_64.S | 1 + > .../selftests/powerpc/memmoveloop/stubs.S | 8 +++ > .../selftests/powerpc/memmoveloop/validate.c | 56 +++ > 13 files changed, 131 insertions(+) > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/.gitignore > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/Makefile > create mode 100644 > tools/testing/selftests/powerpc/memmoveloop/asm/asm-compat.h > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/asm/export.h > create mode 100644 > tools/testing/selftests/powerpc/memmoveloop/asm/feature-fixups.h > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/asm/kasan.h > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/asm/ppc_asm.h > create mode 100644 > tools/testing/selftests/powerpc/memmoveloop/asm/processor.h > create mode 12 tools/testing/selftests/powerpc/memmoveloop/mem_64.S > create mode 12 tools/testing/selftests/powerpc/memmoveloop/memcpy_64.S > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/stubs.S > create mode 100644 tools/testing/selftests/powerpc/memmoveloop/validate.c You've copied a lot of tools/testing/selftests/powerpc/copyloops I'd be happier if you integrated the memmove test into that existing code. I realise memmove is not technically a copy loop, but it's close enough. Did you try that and hit some roadblock? cheers > diff --git a/tools/testing/selftests/powerpc/Makefile > b/tools/testing/selftests/powerpc/Makefile > index 0830e63818c1..d110b3e5cbcd 100644 > --- a/tools/testing/selftests/powerpc/Makefile > +++ b/tools/testing/selftests/powerpc/Makefile > @@ -16,6 +16,7 @@ export CFLAGS > SUB_DIRS = alignment \ > benchmarks \ > cache_shape \ > +memmoveloop \ > copyloops\ > dscr \ > mm \ > diff --git a/tools/testing/selftests/powerpc/memmoveloop/.gitignore > b/tools/testing/selftests/powerpc/memmoveloop/.gitignore > new file mode 100644 > index ..56c1426013d5 > --- /dev/null > +++ b/tools/testing/selftests/powerpc/memmoveloop/.gitignore > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +memmove_64 > diff --git a/tools/testing/selftests/powerpc/memmoveloop/Makefile > b/tools/testing/selftests/powerpc/memmoveloop/Makefile > new file mode 100644 > index ..d58d8c100099 > --- /dev/null > +++ b/tools/testing/selftests/powerpc/memmoveloop/Makefile > @@ -0,0 +1,19 @@ > +# SPDX-License-Identifier: GPL-2.0 > +CFLAGS += -m64 > +CFLAGS += -I$(CURDIR) > +CFLAGS += -D SELFTEST > +CFLAGS += -maltivec > + > +ASFLAGS = $(CFLAGS) -Wa,-mpower4 > + > +TEST_GEN_PROGS := memmove_64 > +EXTRA_SOURCES := validate.c ../harness.c stubs.S > +CPPFLAGS += -D TEST_MEMMOVE=test_memmove > + > +top_srcdir = ../../../../.. > +include ../../lib.mk > + > +$(OUTPUT)/memmove_64: mem_64.S memcpy_64.S $(EXTRA_SOURCES) > + $(CC) $(CPPFLAGS) $(CFLAGS) \ > + -D TEST_MEMMOVE=test_memmove \ > + -o $@ $^ > diff --git a/tools/testing/selftests/powerpc/memmoveloop/asm/asm-compat.h > b/tools/testing/selftests/powerpc/memmoveloop/asm/asm-compat.h > new file mode 100644 > index ..e69de29bb2d1 > diff --git a/tools/testing/selftests/powerpc/memmoveloop/asm/export.h > b/tools/testing/selftests/powerpc/memmoveloop/asm/export.h > new file mode 100644 > index ..e6b80d5fbd14 > --- /dev/null > +++ b/tools/testing/selftests/powerpc/memmoveloop/asm/export.h > @@ -0,0 +1,4 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#define EXPORT_SYMBOL(x) > +#define EXPORT_SYMBOL_GPL(x) > +#define EXPORT_SYMBOL_KASAN(x) > diff --git a/tools/testing/selftests/powerpc/memmoveloop/asm/feature-fixups.h > b/tools/testing/selftests/powerpc/memmoveloop/asm/feature-fixups.h > new file mode 100644 > index ..e69de29bb2d1 > diff --git a/tools/testing/selftests/powerpc/memmoveloop/asm/kasan.h > b/tools/testing/selftests/powerpc/memmoveloop/asm/kasan.h > new file mode 100644 > index
Re: [PATCH 1/1] powerpc: Drop superfluous pci_dev_is_added() calls
Niklas Schnelle writes:
> On powerpc, pci_dev_is_added() is called as part of SR-IOV fixups
> that are done under pcibios_add_device() which in turn is only called in
> pci_device_add() whih is called when a PCI device is scanned.
Thanks for cleaning this up for us.
> Now pci_dev_assign_added() is called in pci_bus_add_device() which is
> only called after scanning the device. Thus pci_dev_is_added() is always
> false and can be dropped.
My only query is whether we can pin down when that changed.
Oliver said:
The use of pci_dev_is_added() in arch/powerpc was because in the past
pci_bus_add_device() could be called before pci_device_add(). That was
fixed a while ago so It should be safe to remove those calls now.
I trawled back through the history a bit but I can't remember/find which
commit changed that, Oliver can you remember?
cheers
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c
> b/arch/powerpc/platforms/powernv/pci-sriov.c
> index 28aac933a439..deddbb233fde 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -9,9 +9,6 @@
>
> #include "pci.h"
>
> -/* for pci_dev_is_added() */
> -#include "../../../../drivers/pci/pci.h"
> -
> /*
> * The majority of the complexity in supporting SR-IOV on PowerNV comes from
> * the need to put the MMIO space for each VF into a separate PE. Internally
> @@ -228,9 +225,6 @@ static void pnv_pci_ioda_fixup_iov_resources(struct
> pci_dev *pdev)
>
> void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
> {
> - if (WARN_ON(pci_dev_is_added(pdev)))
> - return;
> -
> if (pdev->is_virtfn) {
> struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev);
>
> diff --git a/arch/powerpc/platforms/pseries/setup.c
> b/arch/powerpc/platforms/pseries/setup.c
> index f79126f16258..2188054470c1 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -74,7 +74,6 @@
> #include
>
> #include "pseries.h"
> -#include "../../../../drivers/pci/pci.h"
>
> DEFINE_STATIC_KEY_FALSE(shared_processor);
> EXPORT_SYMBOL(shared_processor);
> @@ -750,7 +749,7 @@ static void pseries_pci_fixup_iov_resources(struct
> pci_dev *pdev)
> const int *indexes;
> struct device_node *dn = pci_device_to_OF_node(pdev);
>
> - if (!pdev->is_physfn || pci_dev_is_added(pdev))
> + if (!pdev->is_physfn)
> return;
> /*Firmware must support open sriov otherwise dont configure*/
> indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);
> --
> 2.25.1
Re: [PATCH 06/10] powerpc: remove GCC version check for UPD_CONSTR
Nathan Chancellor writes:
> On 9/10/2021 4:40 PM, Nick Desaulniers wrote:
>> Now that GCC 5.1 is the minimum supported version, we can drop this
>> workaround for older versions of GCC. This adversely affected clang,
>> too.
>>
>> Cc: Michael Ellerman
>> Cc: Benjamin Herrenschmidt
>> Cc: Paul Mackerras
>> Cc: Segher Boessenkool
>> Cc: Christophe Leroy
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Nick Desaulniers
>> ---
>> arch/powerpc/include/asm/asm-const.h | 10 --
>> 1 file changed, 10 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/asm-const.h
>> b/arch/powerpc/include/asm/asm-const.h
>> index 0ce2368bd20f..dbfa5e1e3198 100644
>> --- a/arch/powerpc/include/asm/asm-const.h
>> +++ b/arch/powerpc/include/asm/asm-const.h
>> @@ -12,16 +12,6 @@
>> # define ASM_CONST(x) __ASM_CONST(x)
>> #endif
>>
>> -/*
>> - * Inline assembly memory constraint
>> - *
>> - * GCC 4.9 doesn't properly handle pre update memory constraint "m<>"
>> - *
>> - */
>> -#if defined(GCC_VERSION) && GCC_VERSION < 5
>> -#define UPD_CONSTR ""
>> -#else
>> #define UPD_CONSTR "<>"
>> -#endif
>
> The only reason this exists is because of commit 592bbe9c505d
> ("powerpc/uaccess: Don't use "m<>" constraint with GCC 4.9"). It is
> probably just worth sinking "<>" into all of the callsites and removing
> UPD_CONSTR.
Yeah that would be great if you're doing a v2. Or we can do it as a
follow-up.
cheers
Re: [PATCH v3 3/8] x86/sev: Add an x86 version of cc_platform_has()
On Wed, Sep 08, 2021 at 05:58:34PM -0500, Tom Lendacky wrote:
> diff --git a/arch/x86/kernel/cc_platform.c b/arch/x86/kernel/cc_platform.c
> new file mode 100644
> index ..3c9bacd3c3f3
> --- /dev/null
> +++ b/arch/x86/kernel/cc_platform.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Confidential Computing Platform Capability checks
> + *
> + * Copyright (C) 2021 Advanced Micro Devices, Inc.
> + *
> + * Author: Tom Lendacky
> + */
> +
> +#include
> +#include
> +#include
> +
> +bool cc_platform_has(enum cc_attr attr)
> +{
> + if (sme_me_mask)
Why are you still checking the sme_me_mask here? AFAIR, we said that
we'll do that only when the KVM folks come with a valid use case...
> + return amd_cc_platform_has(attr);
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(cc_platform_has);
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index ff08dc463634..18fe19916bc3 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -389,6 +390,26 @@ bool noinstr sev_es_active(void)
> return sev_status & MSR_AMD64_SEV_ES_ENABLED;
> }
>
> +bool amd_cc_platform_has(enum cc_attr attr)
> +{
> + switch (attr) {
> + case CC_ATTR_MEM_ENCRYPT:
> + return sme_me_mask != 0;
No need for the "!= 0"
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 4/4] powerpc/64s: Initialize and use a temporary mm for patching on Radix
On Sat, Sep 11, 2021 at 12:39 PM Christopher M. Riedl
wrote:
>
> When code patching a STRICT_KERNEL_RWX kernel the page containing the
> address to be patched is temporarily mapped as writeable. Currently, a
> per-cpu vmalloc patch area is used for this purpose. While the patch
> area is per-cpu, the temporary page mapping is inserted into the kernel
> page tables for the duration of patching. The mapping is exposed to CPUs
> other than the patching CPU - this is undesirable from a hardening
> perspective. Use a temporary mm instead which keeps the mapping local to
> the CPU doing the patching.
>
> Use the `poking_init` init hook to prepare a temporary mm and patching
> address. Initialize the temporary mm by copying the init mm. Choose a
> randomized patching address inside the temporary mm userspace address
> space. The patching address is randomized between PAGE_SIZE and
> DEFAULT_MAP_WINDOW-PAGE_SIZE.
>
> Bits of entropy with 64K page size on BOOK3S_64:
>
> bits of entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
>
> PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
> bits of entropy = log2(128TB / 64K)
> bits of entropy = 31
>
> The upper limit is DEFAULT_MAP_WINDOW due to how the Book3s64 Hash MMU
> operates - by default the space above DEFAULT_MAP_WINDOW is not
> available. Currently the Hash MMU does not use a temporary mm so
> technically this upper limit isn't necessary; however, a larger
> randomization range does not further "harden" this overall approach and
> future work may introduce patching with a temporary mm on Hash as well.
>
> Randomization occurs only once during initialization at boot for each
> possible CPU in the system.
>
> Introduce two new functions, map_patch_mm() and unmap_patch_mm(), to
> respectively create and remove the temporary mapping with write
> permissions at patching_addr. Map the page with PAGE_KERNEL to set
> EAA[0] for the PTE which ignores the AMR (so no need to unlock/lock
> KUAP) according to PowerISA v3.0b Figure 35 on Radix.
>
> Based on x86 implementation:
>
> commit 4fc19708b165
> ("x86/alternatives: Initialize temporary mm for patching")
>
> and:
>
> commit b3fd8e83ada0
> ("x86/alternatives: Use temporary mm for text poking")
>
> Signed-off-by: Christopher M. Riedl
>
> ---
>
> v6: * Small clean-ups (naming, formatting, style, etc).
> * Call stop_using_temporary_mm() before pte_unmap_unlock() after
>patching.
> * Replace BUG_ON()s in poking_init() w/ WARN_ON()s.
>
> v5: * Only support Book3s64 Radix MMU for now.
> * Use a per-cpu datastructure to hold the patching_addr and
>patching_mm to avoid the need for a synchronization lock/mutex.
>
> v4: * In the previous series this was two separate patches: one to init
>the temporary mm in poking_init() (unused in powerpc at the time)
>and the other to use it for patching (which removed all the
>per-cpu vmalloc code). Now that we use poking_init() in the
>existing per-cpu vmalloc approach, that separation doesn't work
>as nicely anymore so I just merged the two patches into one.
> * Preload the SLB entry and hash the page for the patching_addr
>when using Hash on book3s64 to avoid taking an SLB and Hash fault
>during patching. The previous implementation was a hack which
>changed current->mm to allow the SLB and Hash fault handlers to
>work with the temporary mm since both of those code-paths always
>assume mm == current->mm.
> * Also (hmm - seeing a trend here) with the book3s64 Hash MMU we
>have to manage the mm->context.active_cpus counter and mm cpumask
>since they determine (via mm_is_thread_local()) if the TLB flush
>in pte_clear() is local or not - it should always be local when
>we're using the temporary mm. On book3s64's Radix MMU we can
>just call local_flush_tlb_mm().
> * Use HPTE_USE_KERNEL_KEY on Hash to avoid costly lock/unlock of
>KUAP.
> ---
> arch/powerpc/lib/code-patching.c | 119 +--
> 1 file changed, 112 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c
> index e802e42c2789..af8e2a02a9dd 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -103,6 +104,7 @@ static inline void stop_using_temporary_mm(struct temp_mm
> *temp_mm)
>
> static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> static DEFINE_PER_CPU(unsigned long, cpu_patching_addr);
> +static DEFINE_PER_CPU(struct mm_struct *, cpu_patching_mm);
>
> static int text_area_cpu_up(unsigned int cpu)
> {
> @@ -126,8 +128,48 @@ static int text_area_cpu_down(unsigned int cpu)
> return 0;
> }
>
> +static __always_inline void __poking_init_temp_mm(void)
> +{
> + int cpu;
> +
Re: [PATCH v6 1/4] powerpc/64s: Introduce temporary mm for Radix MMU
On Sat, Sep 11, 2021 at 12:35 PM Christopher M. Riedl
wrote:
>
> x86 supports the notion of a temporary mm which restricts access to
> temporary PTEs to a single CPU. A temporary mm is useful for situations
> where a CPU needs to perform sensitive operations (such as patching a
> STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
> said mappings to other CPUs. Another benefit is that other CPU TLBs do
> not need to be flushed when the temporary mm is torn down.
>
> Mappings in the temporary mm can be set in the userspace portion of the
> address-space.
>
> Interrupts must be disabled while the temporary mm is in use. HW
> breakpoints, which may have been set by userspace as watchpoints on
> addresses now within the temporary mm, are saved and disabled when
> loading the temporary mm. The HW breakpoints are restored when unloading
> the temporary mm. All HW breakpoints are indiscriminately disabled while
> the temporary mm is in use - this may include breakpoints set by perf.
I had thought CPUs with a DAWR might not need to do this because the
privilege level that breakpoints trigger on can be configured. But it
turns out in ptrace, etc we use HW_BRK_TYPE_PRIV_ALL.
>
> Based on x86 implementation:
>
> commit cefa929c034e
> ("x86/mm: Introduce temporary mm structs")
>
> Signed-off-by: Christopher M. Riedl
>
> ---
>
> v6: * Use {start,stop}_using_temporary_mm() instead of
>{use,unuse}_temporary_mm() as suggested by Christophe.
>
> v5: * Drop support for using a temporary mm on Book3s64 Hash MMU.
>
> v4: * Pass the prev mm instead of NULL to switch_mm_irqs_off() when
>using/unusing the temp mm as suggested by Jann Horn to keep
>the context.active counter in-sync on mm/nohash.
> * Disable SLB preload in the temporary mm when initializing the
>temp_mm struct.
> * Include asm/debug.h header to fix build issue with
>ppc44x_defconfig.
> ---
> arch/powerpc/include/asm/debug.h | 1 +
> arch/powerpc/kernel/process.c| 5 +++
> arch/powerpc/lib/code-patching.c | 56
> 3 files changed, 62 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/debug.h
> b/arch/powerpc/include/asm/debug.h
> index 86a14736c76c..dfd82635ea8b 100644
> --- a/arch/powerpc/include/asm/debug.h
> +++ b/arch/powerpc/include/asm/debug.h
> @@ -46,6 +46,7 @@ static inline int debugger_fault_handler(struct pt_regs
> *regs) { return 0; }
> #endif
>
> void __set_breakpoint(int nr, struct arch_hw_breakpoint *brk);
> +void __get_breakpoint(int nr, struct arch_hw_breakpoint *brk);
> bool ppc_breakpoint_available(void);
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> extern void do_send_trap(struct pt_regs *regs, unsigned long address,
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 50436b52c213..6aa1f5c4d520 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -865,6 +865,11 @@ static inline int set_breakpoint_8xx(struct
> arch_hw_breakpoint *brk)
> return 0;
> }
>
> +void __get_breakpoint(int nr, struct arch_hw_breakpoint *brk)
> +{
> + memcpy(brk, this_cpu_ptr(_brk[nr]), sizeof(*brk));
> +}
The breakpoint code is already a little hard to follow. I'm worried
doing this might spread breakpoint handling into more places in the
future.
What about something like having a breakpoint_pause() function which
clears the hardware registers only and then a breakpoint_resume()
function that copies from current_brk[] back to the hardware
registers?
Then we don't have to make another copy of the breakpoint state.
> +
> void __set_breakpoint(int nr, struct arch_hw_breakpoint *brk)
> {
> memcpy(this_cpu_ptr(_brk[nr]), brk, sizeof(*brk));
> diff --git a/arch/powerpc/lib/code-patching.c
> b/arch/powerpc/lib/code-patching.c
> index f9a3019e37b4..8d61a7d35b89 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
Sorry I might have missed it, but what was the reason for not putting
this stuff in mmu_context.h?
> @@ -17,6 +17,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32
> *patch_addr)
> {
> @@ -45,6 +48,59 @@ int raw_patch_instruction(u32 *addr, struct ppc_inst instr)
> }
>
> #ifdef CONFIG_STRICT_KERNEL_RWX
> +
> +struct temp_mm {
> + struct mm_struct *temp;
> + struct mm_struct *prev;
> + struct arch_hw_breakpoint brk[HBP_NUM_MAX];
^ Then we wouldn't need this.
> +};
> +
> +static inline void init_temp_mm(struct temp_mm *temp_mm, struct mm_struct
> *mm)
> +{
> + /* We currently only support temporary mm on the Book3s64 Radix MMU */
> + WARN_ON(!radix_enabled());
> +
> + temp_mm->temp = mm;
> + temp_mm->prev = NULL;
> + memset(_mm->brk, 0, sizeof(temp_mm->brk));
> +}
> +
> +static inline void start_using_temporary_mm(struct temp_mm *temp_mm)
> +{
> +
