* Re: [PATCH v7 5/6] powerpc/pseries: Add support for FORM2 associativity
From: David Gibson @ 2021-08-12 1:41 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Nathan Lynch, Daniel Henrique Barboza, linuxppc-dev
In-Reply-To: <87a6loaagz.fsf@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]
On Wed, Aug 11, 2021 at 09:39:32AM +0530, Aneesh Kumar K.V wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
>
> > On Mon, Aug 09, 2021 at 10:54:33AM +0530, Aneesh Kumar K.V wrote:
> >> PAPR interface currently supports two different ways of communicating resource
> >> grouping details to the OS. These are referred to as Form 0 and Form 1
> >> associativity grouping. Form 0 is the older format and is now considered
> >> deprecated. This patch adds another resource grouping named FORM2.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> >
> > LGTM, with the exception of some minor nits noted below.
> ...
>
> > +
> >> + for (i = 0; i < max_numa_index; i++)
> >> + /* +1 skip the max_numa_index in the property */
> >> + numa_id_index_table[i] = of_read_number(&numa_lookup_index[i + 1], 1);
> >> +
> >> +
> >> + if (numa_dist_table_length != max_numa_index * max_numa_index) {
> >> +
> >
> > Stray extra whitespace line here.
> >
> >> + WARN(1, "Wrong NUMA distance information\n");
> >> + /* consider everybody else just remote. */
> >> + for (i = 0; i < max_numa_index; i++) {
> >> + for (j = 0; j < max_numa_index; j++) {
> >> + int nodeA = numa_id_index_table[i];
> >> + int nodeB = numa_id_index_table[j];
> >> +
> >> + if (nodeA == nodeB)
> >> + numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
> >> + else
> >> + numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
> >> + }
> >> + }
> >
> > I don't think it's necessarily a problem, but something to consider is
> > that this fallback will initialize distance for *all* node IDs,
> > whereas the normal path will only initialize it for nodes that are in
> > the index table. Since some later error checks key off whether
> > certain fields in the distance table are initialized, is that the
> > outcome you want?
> >
>
> With the device tree details not correct, one of the possible way to
> make progress is to consider everybody remote. With new node hotplug
> support we used to check whether the distance table entry is
> initialized. With the updated spec, we expect all possible numa node
> distance to be available during boot.
Sure. But my main point here is that the fallback behaviour in this
clause is different from the fallback behaviour if the table is there
and parseable, but incomplete - which is also not expected.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: clang/ld.lld build fails with `can't create dynamic relocation R_PPC64_ADDR64 against local symbol in readonly segment`
From: Paul Menzel @ 2021-08-11 23:09 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: Derek Parker, Dmitrii Okunev, linuxppc-dev
In-Reply-To: <886ac4a5-ad30-b27f-0b80-ec233c571e81@csgroup.eu>
Dear Christophe,
Am 11.08.21 um 16:10 schrieb Christophe Leroy:
> Le 10/08/2021 à 20:38, Paul Menzel a écrit :
>> Am 29.07.21 um 10:23 schrieb Paul Menzel:
>>
>>> I just wanted to make you aware that building Linux for ppc64le with
>>> clang/lld.ld fails with [1]:
>>>
>>> ld.lld: error: can't create dynamic relocation R_PPC64_ADDR64
>>> against symbol: empty_zero_page in readonly segment; recompile object
>>> files with -fPIC or pass '-Wl,-z,notext' to allow text relocations in
>>> the output
>>> >>> defined in arch/powerpc/kernel/head_64.o
>>> >>> referenced by
>>> arch/powerpc/kernel/head_64.o:(___ksymtab+empty_zero_page+0x0)
>>>
>>> The patch below from one of the comments [2] fixes it.
>>>
>>> --- i/arch/powerpc/Makefile
>>> +++ w/arch/powerpc/Makefile
>>> @@ -122,7 +122,7 @@ cflags-$(CONFIG_STACKPROTECTOR) +=
>>> -mstack-protector-guard-reg=r2
>>> endif
>>>
>>> LDFLAGS_vmlinux-y := -Bstatic
>>> -LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie
>>> +LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie -z notext
>>> LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-y)
>>> LDFLAGS_vmlinux += $(call ld-option,--orphan-handling=warn)
>>
>> Any comments, if this is the right fix? Current Linux master branch
>> still fails to build with `LLVM=1` on Ubuntu 21.04 without this change.
>
> Which kernel version are you building ?
>
> Since
> https://github.com/linuxppc/linux/commit/45b30fafe528601f1a4449c9d68d8ebe7bbc39ad
> , empty_zero_page[] is in arch/powerpc/mm/mem.c not in
> arch/powerpc/kernel/head_64.o
>
> Do you still have the issue with kernel 5.14 ?
Yes, before sending the message, I reproduced it with
$ git describe
v5.14-rc5-2-g9a73fa375d58
containing the commit you mentioned.
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH v2 40/60] KVM: PPC: Book3S HV P9: Implement TM fastpath for guest entry/exit
From: kernel test robot @ 2021-08-11 20:40 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20210811160134.904987-41-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5369 bytes --]
Hi Nicholas,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on linus/master v5.14-rc5 next-20210811]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/KVM-PPC-Book3S-HV-P9-entry-exit-optimisations/20210812-000748
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-r024-20210811 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/30a3a9ae99f124a863c41f268c68b647d7116b65
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nicholas-Piggin/KVM-PPC-Book3S-HV-P9-entry-exit-optimisations/20210812-000748
git checkout 30a3a9ae99f124a863c41f268c68b647d7116b65
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=powerpc SHELL=/bin/bash arch/powerpc/kvm/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from arch/powerpc/include/asm/processor.h:11,
from arch/powerpc/include/asm/thread_info.h:40,
from include/linux/thread_info.h:60,
from include/asm-generic/preempt.h:5,
from ./arch/powerpc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/percpu.h:6,
from include/linux/context_tracking_state.h:5,
from include/linux/hardirq.h:5,
from include/linux/kvm_host.h:7,
from arch/powerpc/kvm/book3s_hv_p9_entry.c:3:
arch/powerpc/kvm/book3s_hv_p9_entry.c: In function 'load_vcpu_state':
>> arch/powerpc/kvm/book3s_hv_p9_entry.c:297:33: error: 'struct kvm_vcpu_arch' has no member named 'texasr'
297 | mtspr(SPRN_TEXASR, vcpu->arch.texasr);
| ^
arch/powerpc/include/asm/reg.h:1396:33: note: in definition of macro 'mtspr'
1396 | : "r" ((unsigned long)(v)) \
| ^
>> arch/powerpc/kvm/book3s_hv_p9_entry.c:298:33: error: 'struct kvm_vcpu_arch' has no member named 'tfhar'; did you mean 'tar'?
298 | mtspr(SPRN_TFHAR, vcpu->arch.tfhar);
| ^~~~~
arch/powerpc/include/asm/reg.h:1396:33: note: in definition of macro 'mtspr'
1396 | : "r" ((unsigned long)(v)) \
| ^
>> arch/powerpc/kvm/book3s_hv_p9_entry.c:299:33: error: 'struct kvm_vcpu_arch' has no member named 'tfiar'; did you mean 'tar'?
299 | mtspr(SPRN_TFIAR, vcpu->arch.tfiar);
| ^~~~~
arch/powerpc/include/asm/reg.h:1396:33: note: in definition of macro 'mtspr'
1396 | : "r" ((unsigned long)(v)) \
| ^
arch/powerpc/kvm/book3s_hv_p9_entry.c: In function 'store_vcpu_state':
arch/powerpc/kvm/book3s_hv_p9_entry.c:331:14: error: 'struct kvm_vcpu_arch' has no member named 'texasr'
331 | vcpu->arch.texasr = mfspr(SPRN_TEXASR);
| ^
arch/powerpc/kvm/book3s_hv_p9_entry.c:332:15: error: 'struct kvm_vcpu_arch' has no member named 'tfhar'; did you mean 'tar'?
332 | vcpu->arch.tfhar = mfspr(SPRN_TFHAR);
| ^~~~~
| tar
arch/powerpc/kvm/book3s_hv_p9_entry.c:333:15: error: 'struct kvm_vcpu_arch' has no member named 'tfiar'; did you mean 'tar'?
333 | vcpu->arch.tfiar = mfspr(SPRN_TFIAR);
| ^~~~~
| tar
vim +297 arch/powerpc/kvm/book3s_hv_p9_entry.c
283
284 /* Returns true if current MSR and/or guest MSR may have changed */
285 bool load_vcpu_state(struct kvm_vcpu *vcpu,
286 struct p9_host_os_sprs *host_os_sprs)
287 {
288 bool ret = false;
289
290 if (cpu_has_feature(CPU_FTR_TM) ||
291 cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) {
292 unsigned long guest_msr = vcpu->arch.shregs.msr;
293 if (MSR_TM_ACTIVE(guest_msr)) {
294 kvmppc_restore_tm_hv(vcpu, guest_msr, true);
295 ret = true;
296 } else {
> 297 mtspr(SPRN_TEXASR, vcpu->arch.texasr);
> 298 mtspr(SPRN_TFHAR, vcpu->arch.tfhar);
> 299 mtspr(SPRN_TFIAR, vcpu->arch.tfiar);
300 }
301 }
302
303 load_spr_state(vcpu, host_os_sprs);
304
305 load_fp_state(&vcpu->arch.fp);
306 #ifdef CONFIG_ALTIVEC
307 load_vr_state(&vcpu->arch.vr);
308 #endif
309 mtspr(SPRN_VRSAVE, vcpu->arch.vrsave);
310
311 return ret;
312 }
313 EXPORT_SYMBOL_GPL(load_vcpu_state);
314
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43470 bytes --]
^ permalink raw reply
* Re: [PATCH v2 27/60] KVM: PPC: Book3S HV P9: Reduce mtmsrd instructions required to save host SPRs
From: kernel test robot @ 2021-08-11 19:49 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20210811160134.904987-28-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]
Hi Nicholas,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on linus/master v5.14-rc5 next-20210811]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/KVM-PPC-Book3S-HV-P9-entry-exit-optimisations/20210812-000748
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-r024-20210811 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b5149d8c735b6802aa0433a0cecc73e4d943e795
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nicholas-Piggin/KVM-PPC-Book3S-HV-P9-entry-exit-optimisations/20210812-000748
git checkout b5149d8c735b6802aa0433a0cecc73e4d943e795
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> arch/powerpc/kernel/process.c:596:6: error: no previous prototype for 'save_user_regs_kvm' [-Werror=missing-prototypes]
596 | void save_user_regs_kvm(void)
| ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/save_user_regs_kvm +596 arch/powerpc/kernel/process.c
595
> 596 void save_user_regs_kvm(void)
597 {
598 unsigned long usermsr;
599
600 if (!current->thread.regs)
601 return;
602
603 usermsr = current->thread.regs->msr;
604
605 if (usermsr & MSR_FP)
606 save_fpu(current);
607
608 if (usermsr & MSR_VEC)
609 save_altivec(current);
610
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43470 bytes --]
^ permalink raw reply
* Re: [PATCH v2 27/60] KVM: PPC: Book3S HV P9: Reduce mtmsrd instructions required to save host SPRs
From: kernel test robot @ 2021-08-11 19:18 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20210811160134.904987-28-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]
Hi Nicholas,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on linus/master v5.14-rc5 next-20210811]
[cannot apply to scottwood/next kvm-ppc/kvm-ppc-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/KVM-PPC-Book3S-HV-P9-entry-exit-optimisations/20210812-000748
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-buildonly-randconfig-r001-20210810 (attached as .config)
compiler: powerpc-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/b5149d8c735b6802aa0433a0cecc73e4d943e795
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nicholas-Piggin/KVM-PPC-Book3S-HV-P9-entry-exit-optimisations/20210812-000748
git checkout b5149d8c735b6802aa0433a0cecc73e4d943e795
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/powerpc/kernel/process.c:596:6: warning: no previous prototype for 'save_user_regs_kvm' [-Wmissing-prototypes]
596 | void save_user_regs_kvm(void)
| ^~~~~~~~~~~~~~~~~~
vim +/save_user_regs_kvm +596 arch/powerpc/kernel/process.c
595
> 596 void save_user_regs_kvm(void)
597 {
598 unsigned long usermsr;
599
600 if (!current->thread.regs)
601 return;
602
603 usermsr = current->thread.regs->msr;
604
605 if (usermsr & MSR_FP)
606 save_fpu(current);
607
608 if (usermsr & MSR_VEC)
609 save_altivec(current);
610
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31911 bytes --]
^ permalink raw reply
* Re: [PATCH v5 7/8] powerpc/64s: Initialize and use a temporary mm for patching on Radix
From: Christopher M. Riedl @ 2021-08-11 18:28 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <9cc03303-ca54-94b8-7d0b-42647ff4d5a7@csgroup.eu>
On Thu Aug 5, 2021 at 4:48 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > 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() and unmap_patch(), 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 <cmr@linux.ibm.com>
> >
> > ---
> >
> > 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 | 132 +++++++++++++++++++++++++++++--
> > 1 file changed, 125 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index 9f2eba9b70ee4..027dabd42b8dd 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -11,6 +11,7 @@
> > #include <linux/cpuhotplug.h>
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > +#include <linux/random.h>
> >
> > #include <asm/tlbflush.h>
> > #include <asm/page.h>
> > @@ -103,6 +104,7 @@ static inline void unuse_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);
> >
> > #if IS_BUILTIN(CONFIG_LKDTM)
> > unsigned long read_cpu_patching_addr(unsigned int cpu)
> > @@ -133,6 +135,51 @@ static int text_area_cpu_down(unsigned int cpu)
> > return 0;
> > }
> >
> > +static __always_inline void __poking_init_temp_mm(void)
> > +{
> > + int cpu;
> > + spinlock_t *ptl; /* for protecting pte table */
> > + pte_t *ptep;
> > + struct mm_struct *patching_mm;
> > + unsigned long patching_addr;
> > +
> > + for_each_possible_cpu(cpu) {
> > + /*
> > + * Some parts of the kernel (static keys for example) depend on
> > + * successful code patching. Code patching under
> > + * STRICT_KERNEL_RWX requires this setup - otherwise we cannot
> > + * patch at all. We use BUG_ON() here and later since an early
> > + * failure is preferred to buggy behavior and/or strange
> > + * crashes later.
> > + */
> > + patching_mm = copy_init_mm();
> > + BUG_ON(!patching_mm);
>
> Read
> https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
> and
> https://github.com/linuxppc/issues/issues/88
>
> Avoid BUG_ON()s thanks.
>
Fine, @mpe's reply on the GH issue says the check is probably redundant:
"In general we don't need to BUG_ON(!ptr), the MMU will catch NULL
pointer dereferences for us."
> > +
> > + per_cpu(cpu_patching_mm, cpu) = patching_mm;
> > +
> > + /*
> > + * Choose a randomized, page-aligned address from the range:
> > + * [PAGE_SIZE, DEFAULT_MAP_WINDOW - PAGE_SIZE] The lower
> > + * address bound is PAGE_SIZE to avoid the zero-page. The
> > + * upper address bound is DEFAULT_MAP_WINDOW - PAGE_SIZE to
> > + * stay under DEFAULT_MAP_WINDOW with the Book3s64 Hash MMU.
> > + */
> > + patching_addr = PAGE_SIZE + ((get_random_long() & PAGE_MASK)
> > + % (DEFAULT_MAP_WINDOW - 2 * PAGE_SIZE));
>
> % should be at the end of first line and the second line alignment
> should match open parenthesis in
> first line.
Ok - thanks!
>
> > +
> > + per_cpu(cpu_patching_addr, cpu) = patching_addr;
> > +
> > + /*
> > + * PTE allocation uses GFP_KERNEL which means we need to
> > + * pre-allocate the PTE here because we cannot do the
> > + * allocation during patching when IRQs are disabled.
> > + */
> > + ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
> > + BUG_ON(!ptep);
>
> Avoid BUG_ON() please
>
Yup, I'll remove these in the next spin.
>
> > + pte_unmap_unlock(ptep, ptl);
> > + }
> > +}
> > +
> > /*
> > * Although BUG_ON() is rude, in this case it should only happen if ENOMEM, and
> > * we judge it as being preferable to a kernel that will crash later when
> > @@ -140,6 +187,11 @@ static int text_area_cpu_down(unsigned int cpu)
> > */
> > void __init poking_init(void)
> > {
> > + if (radix_enabled()) {
> > + __poking_init_temp_mm();
> > + return;
> > + }
> > +
> > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > "powerpc/text_poke:online", text_area_cpu_up,
> > text_area_cpu_down));
> > @@ -213,30 +265,96 @@ static inline int unmap_patch_area(void)
> > return -EINVAL;
> > }
> >
> > +struct patch_mapping {
> > + spinlock_t *ptl; /* for protecting pte table */
> > + pte_t *ptep;
> > + struct temp_mm temp_mm;
> > +};
> > +
> > +/*
> > + * This can be called for kernel text or a module.
> > + */
> > +static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
> > +{
> > + struct page *page;
> > + pte_t pte;
> > + pgprot_t pgprot;
> > + struct mm_struct *patching_mm = __this_cpu_read(cpu_patching_mm);
> > + unsigned long patching_addr = __this_cpu_read(cpu_patching_addr);
> > +
> > + if (is_vmalloc_or_module_addr(addr))
> > + page = vmalloc_to_page(addr);
> > + else
> > + page = virt_to_page(addr);
> > +
> > + patch_mapping->ptep = get_locked_pte(patching_mm, patching_addr,
> > + &patch_mapping->ptl);
>
> Not sure you need to split this line, checkpatch now allows 100 chars
> per line.
>
I prefer sticking to 80 columns unless readability *really* improves by
going over that limit.
>
> > + if (unlikely(!patch_mapping->ptep)) {
> > + pr_warn("map patch: failed to allocate pte for patching\n");
>
> That's a lot better than all above BUG_ONs
>
>
> > + return -1;
> > + }
> > +
> > + pgprot = PAGE_KERNEL;
> > + pte = mk_pte(page, pgprot);
> > + pte = pte_mkdirty(pte);
>
> I'm sure you can do
>
> pte = pte_mkdirty(mk_pte(page, PAGE_KERNEL));
>
> And indeed PAGE_KERNEL already includes _PAGE_DIRTY, so all you should
> need is
>
> pte = mk_pte(page, PAGE_KERNEL);
>
> Or even just
>
> set_pte_at(patching_mm, patching_addr, patch_mapping->ptep, mk_pte(page,
> PAGE_KERNEL));
>
Ok, I'll consolidate this in the next spin. Thanks!
>
> > + set_pte_at(patching_mm, patching_addr, patch_mapping->ptep, pte);
> > +
> > + init_temp_mm(&patch_mapping->temp_mm, patching_mm);
> > + use_temporary_mm(&patch_mapping->temp_mm);
> > +
> > + return 0;
> > +}
> > +
> > +static void unmap_patch(struct patch_mapping *patch_mapping)
> > +{
> > + struct mm_struct *patching_mm = __this_cpu_read(cpu_patching_mm);
> > + unsigned long patching_addr = __this_cpu_read(cpu_patching_addr);
> > +
> > + pte_clear(patching_mm, patching_addr, patch_mapping->ptep);
> > +
> > + local_flush_tlb_mm(patching_mm);
> > +
> > + pte_unmap_unlock(patch_mapping->ptep, patch_mapping->ptl);
> > +
> > + unuse_temporary_mm(&patch_mapping->temp_mm);
>
> Shouldn't you stop using it before unmapping/unlocking it ?
>
Yes I think you're right - IIRC I had to do this for the Hash MMU (which
we don't support w/ this verion of the series anymore anyways). I'll
revisit this for the next spin.
>
> > +}
> > +
> > static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
> > {
> > int err, rc = 0;
> > u32 *patch_addr = NULL;
> > unsigned long flags;
> > + struct patch_mapping patch_mapping;
> >
> > /*
> > - * During early early boot patch_instruction is called
> > - * when text_poke_area is not ready, but we still need
> > - * to allow patching. We just do the plain old patching
> > + * During early early boot patch_instruction is called when the
> > + * patching_mm/text_poke_area is not ready, but we still need to allow
> > + * patching. We just do the plain old patching.
> > */
> > - if (!this_cpu_read(text_poke_area))
> > - return raw_patch_instruction(addr, instr);
> > + if (radix_enabled()) {
> > + if (!this_cpu_read(cpu_patching_mm))
> > + return raw_patch_instruction(addr, instr);
> > + } else {
> > + if (!this_cpu_read(text_poke_area))
> > + return raw_patch_instruction(addr, instr);
> > + }
> >
> > local_irq_save(flags);
> >
> > - err = map_patch_area(addr);
> > + if (radix_enabled())
> > + err = map_patch(addr, &patch_mapping);
>
> Maybe call it map_patch_mm() or map_patch_mapping() ?
Yes that does sound better.
>
> > + else
> > + err = map_patch_area(addr);
> > if (err)
> > goto out;
> >
> > patch_addr = (u32 *)(__this_cpu_read(cpu_patching_addr) | offset_in_page(addr));
> > rc = __patch_instruction(addr, instr, patch_addr);
> >
> > - err = unmap_patch_area();
> > + if (radix_enabled())
> > + unmap_patch(&patch_mapping);
>
> No err ? Would be better if it could return something, allthough always
> 0.
Ok I'll do that.
>
> And same comment about naming.
>
Yes I'll use your suggested names.
> > + else
> > + err = unmap_patch_area();
> >
> > out:
> > local_irq_restore(flags);
> >
^ permalink raw reply
* Re: [PATCH v5 6/8] powerpc: Rework and improve STRICT_KERNEL_RWX patching
From: Christopher M. Riedl @ 2021-08-11 18:10 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <9c53e997-3609-20f8-74c0-7776c867ce6c@csgroup.eu>
On Thu Aug 5, 2021 at 4:34 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > Rework code-patching with STRICT_KERNEL_RWX to prepare for the next
> > patch which uses a temporary mm for patching under the Book3s64 Radix
> > MMU. Make improvements by adding a WARN_ON when the patchsite doesn't
> > match after patching and return the error from __patch_instruction()
> > properly.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@linux.ibm.com>
> >
> > ---
> >
> > v5: * New to series.
> > ---
> > arch/powerpc/lib/code-patching.c | 51 +++++++++++++++++---------------
> > 1 file changed, 27 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index 3122d8e4cc013..9f2eba9b70ee4 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -102,11 +102,12 @@ static inline void unuse_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);
> >
> > #if IS_BUILTIN(CONFIG_LKDTM)
> > unsigned long read_cpu_patching_addr(unsigned int cpu)
> > {
> > - return (unsigned long)(per_cpu(text_poke_area, cpu))->addr;
> > + return per_cpu(cpu_patching_addr, cpu);
> > }
> > #endif
> >
> > @@ -121,6 +122,7 @@ static int text_area_cpu_up(unsigned int cpu)
> > return -1;
> > }
> > this_cpu_write(text_poke_area, area);
> > + this_cpu_write(cpu_patching_addr, (unsigned long)area->addr);
> >
> > return 0;
> > }
> > @@ -146,7 +148,7 @@ void __init poking_init(void)
> > /*
> > * This can be called for kernel text or a module.
> > */
> > -static int map_patch_area(void *addr, unsigned long text_poke_addr)
> > +static int map_patch_area(void *addr)
> > {
> > unsigned long pfn;
> > int err;
> > @@ -156,17 +158,20 @@ static int map_patch_area(void *addr, unsigned long text_poke_addr)
> > else
> > pfn = __pa_symbol(addr) >> PAGE_SHIFT;
> >
> > - err = map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
> > + err = map_kernel_page(__this_cpu_read(cpu_patching_addr),
> > + (pfn << PAGE_SHIFT), PAGE_KERNEL);
> >
> > - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err);
> > + pr_devel("Mapped addr %lx with pfn %lx:%d\n",
> > + __this_cpu_read(cpu_patching_addr), pfn, err);
> > if (err)
> > return -1;
> >
> > return 0;
> > }
> >
> > -static inline int unmap_patch_area(unsigned long addr)
> > +static inline int unmap_patch_area(void)
> > {
> > + unsigned long addr = __this_cpu_read(cpu_patching_addr);
> > pte_t *ptep;
> > pmd_t *pmdp;
> > pud_t *pudp;
> > @@ -175,23 +180,23 @@ static inline int unmap_patch_area(unsigned long addr)
> >
> > pgdp = pgd_offset_k(addr);
> > if (unlikely(!pgdp))
> > - return -EINVAL;
> > + goto out_err;
> >
> > p4dp = p4d_offset(pgdp, addr);
> > if (unlikely(!p4dp))
> > - return -EINVAL;
> > + goto out_err;
> >
> > pudp = pud_offset(p4dp, addr);
> > if (unlikely(!pudp))
> > - return -EINVAL;
> > + goto out_err;
> >
> > pmdp = pmd_offset(pudp, addr);
> > if (unlikely(!pmdp))
> > - return -EINVAL;
> > + goto out_err;
> >
> > ptep = pte_offset_kernel(pmdp, addr);
> > if (unlikely(!ptep))
> > - return -EINVAL;
> > + goto out_err;
> >
> > pr_devel("clearing mm %p, pte %p, addr %lx\n", &init_mm, ptep, addr);
> >
> > @@ -202,15 +207,17 @@ static inline int unmap_patch_area(unsigned long addr)
> > flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> >
> > return 0;
> > +
> > +out_err:
> > + pr_warn("failed to unmap %lx\n", addr);
> > + return -EINVAL;
>
> Can you keep that in the caller of unmap_patch_area() instead of all
> those goto stuff ?
>
Yeah I think that's fair. I'll do this in the next spin.
> > }
> >
> > static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
> > {
> > - int err;
> > + int err, rc = 0;
> > u32 *patch_addr = NULL;
> > unsigned long flags;
> > - unsigned long text_poke_addr;
> > - unsigned long kaddr = (unsigned long)addr;
> >
> > /*
> > * During early early boot patch_instruction is called
> > @@ -222,24 +229,20 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
> >
> > local_irq_save(flags);
> >
> > - text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
> > - if (map_patch_area(addr, text_poke_addr)) {
> > - err = -1;
> > + err = map_patch_area(addr);
> > + if (err)
> > goto out;
> > - }
> > -
> > - patch_addr = (u32 *)(text_poke_addr + (kaddr & ~PAGE_MASK));
> >
> > - __patch_instruction(addr, instr, patch_addr);
> > + patch_addr = (u32 *)(__this_cpu_read(cpu_patching_addr) | offset_in_page(addr));
> > + rc = __patch_instruction(addr, instr, patch_addr);
> >
> > - err = unmap_patch_area(text_poke_addr);
> > - if (err)
> > - pr_warn("failed to unmap %lx\n", text_poke_addr);
> > + err = unmap_patch_area();
> >
> > out:
> > local_irq_restore(flags);
> > + WARN_ON(!ppc_inst_equal(ppc_inst_read(addr), instr));
>
> Why adding that WARN_ON(), what could make that happen that is worth a
> WARN_ON() ?
Failing to patch something could cause very strange issues later, so
explicitly calling out a failure when it happens is warranted IMO.
>
> Patching is quite a critical fast path, I'm not sure we want to afford
> too many checks during
> patching, we want it quick at first.
Hmm, I'd prefer to measure the impact first - if it's a huge degradation
then sure we can drop the WARN_ON()... I'll add some data with the next
spin.
>
> >
> > - return err;
> > + return rc ? rc : err;
> > }
> > #else /* !CONFIG_STRICT_KERNEL_RWX */
> >
> >
^ permalink raw reply
* Re: [PATCH v5 2/8] lkdtm/powerpc: Add test to hijack a patch mapping
From: Kees Cook @ 2021-08-11 18:07 UTC (permalink / raw)
To: Christopher M. Riedl
Cc: peterz, x86, npiggin, tglx, dja, linuxppc-dev, linux-hardening
In-Reply-To: <CDGVLP8OS8N9.13R0RIGJ1WJ8R@oc8246131445.ibm.com>
On Wed, Aug 11, 2021 at 12:57:00PM -0500, Christopher M. Riedl wrote:
> On Thu Aug 5, 2021 at 4:13 AM CDT, Christophe Leroy wrote:
> >
> >
> > Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > > When live patching with STRICT_KERNEL_RWX the CPU doing the patching
> > > must temporarily remap the page(s) containing the patch site with +W
> > > permissions. While this temporary mapping is in use, another CPU could
> > > write to the same mapping and maliciously alter kernel text. Implement a
> > > LKDTM test to attempt to exploit such an opening during code patching.
> > > The test is implemented on powerpc and requires LKDTM built into the
> > > kernel (building LKDTM as a module is insufficient).
> > >
> > > The LKDTM "hijack" test works as follows:
> > >
> > > 1. A CPU executes an infinite loop to patch an instruction. This is
> > > the "patching" CPU.
> > > 2. Another CPU attempts to write to the address of the temporary
> > > mapping used by the "patching" CPU. This other CPU is the
> > > "hijacker" CPU. The hijack either fails with a fault/error or
> > > succeeds, in which case some kernel text is now overwritten.
> > > [...]
> > > +#if (IS_BUILTIN(CONFIG_LKDTM) && defined(CONFIG_STRICT_KERNEL_RWX) && \
> > > + defined(CONFIG_PPC))
> >
> > I think this test shouldn't be limited to CONFIG_PPC and shouldn't be
> > limited to CONFIG_STRICT_KERNEL_RWX. It should be there all the time.
Agreed: if the machinery exists to provide this defense on even one
arch/config/whatever combo, I'd like LKDTM to test for it. This lets use
compare defenses across different combinations more easily, and means
folks must answer questions like "why doesn't $combination provide
$defense?"
> > Also why limiting it to IS_BUILTIN(CONFIG_LKDTM) ?
>
> The test needs read_cpu_patching_addr() which definitely cannot be
> exposed outside of the kernel (ie. builtin).
FWIW, I'm okay with this. There isn't a solution that feels entirely
"right", so either a build-time requirement like this, or using an
exception for modules like this:
arch/x86/kernel/cpu/common.c:#if IS_MODULE(CONFIG_LKDTM)
arch/x86/kernel/cpu/common.c-EXPORT_SYMBOL_GPL(native_write_cr4);
arch/x86/kernel/cpu/common.c-#endif
I think neither is great. Another idea is maybe using a name-spaced
export, like:
EXPORT_SYMBOL_NS_GPL(native_write_cr4, LKDTM);
But that still means it gets exposed to malicious discovery, so probably
not.
I suspect the best is to just do the BUILTIN check, since building LKDTM
as a module on a _production_ kernel is rare if it exists at all. The
only downside is needing to completely reboot to perform updated tests,
but then, I frequently find myself breaking the kernel badly on bad
tests, so I have to reboot anyway. ;)
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v5 5/8] powerpc/64s: Introduce temporary mm for Radix MMU
From: Christopher M. Riedl @ 2021-08-11 18:02 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <c8b2291e-57f9-6d9a-583e-4ec65b2c9bcb@csgroup.eu>
On Thu Aug 5, 2021 at 4:27 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > 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.
>
> Can you explain more about that breakpoint stuff ? Why is it a special
> case here at all ? Isn't it
> the same when you switch from one user task to another one ? x86 commit
> doesn't say anythink about
> breakpoints.
>
We do not check if the breakpoint is on a kernel address (perf can do
this IIUC) and just disable all of them. I had to dig, but x86 has a
comment with their implementation at arch/x86/kernel/alternative.c:743.
I can reword that part of the commit message if it's unclear.
> >
> > Based on x86 implementation:
> >
> > commit cefa929c034e
> > ("x86/mm: Introduce temporary mm structs")
> >
> > Signed-off-by: Christopher M. Riedl <cmr@linux.ibm.com>
> >
> > ---
> >
> > 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 86a14736c76c3..dfd82635ea8b3 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 185beb2905801..a0776200772e8 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(¤t_brk[nr]), sizeof(*brk));
> > +}
> > +
> > void __set_breakpoint(int nr, struct arch_hw_breakpoint *brk)
> > {
> > memcpy(this_cpu_ptr(¤t_brk[nr]), brk, sizeof(*brk));
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index 54b6157d44e95..3122d8e4cc013 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -17,6 +17,9 @@
> > #include <asm/code-patching.h>
> > #include <asm/setup.h>
> > #include <asm/inst.h>
> > +#include <asm/mmu_context.h>
> > +#include <asm/debug.h>
> > +#include <asm/tlb.h>
> >
> > 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];
> > +};
> > +
> > +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(&temp_mm->brk, 0, sizeof(temp_mm->brk));
> > +}
> > +
> > +static inline void use_temporary_mm(struct temp_mm *temp_mm)
> > +{
> > + lockdep_assert_irqs_disabled();
> > +
> > + temp_mm->prev = current->active_mm;
> > + switch_mm_irqs_off(temp_mm->prev, temp_mm->temp, current);
> > +
> > + WARN_ON(!mm_is_thread_local(temp_mm->temp));
> > +
> > + if (ppc_breakpoint_available()) {
> > + struct arch_hw_breakpoint null_brk = {0};
> > + int i = 0;
> > +
> > + for (; i < nr_wp_slots(); ++i) {
> > + __get_breakpoint(i, &temp_mm->brk[i]);
> > + if (temp_mm->brk[i].type != 0)
> > + __set_breakpoint(i, &null_brk);
> > + }
> > + }
> > +}
> > +
> > +static inline void unuse_temporary_mm(struct temp_mm *temp_mm)
>
> not sure about the naming.
>
> Maybe start_using_temp_mm() and stop_using_temp_mm() would be more
> explicit.
>
Hehe I think we've discussed this before - naming things is hard :) I'll
take your suggestions for the next spin.
>
> > +{
> > + lockdep_assert_irqs_disabled();
> > +
> > + switch_mm_irqs_off(temp_mm->temp, temp_mm->prev, current);
> > +
> > + if (ppc_breakpoint_available()) {
> > + int i = 0;
> > +
> > + for (; i < nr_wp_slots(); ++i)
> > + if (temp_mm->brk[i].type != 0)
> > + __set_breakpoint(i, &temp_mm->brk[i]);
> > + }
> > +}
> > +
> > static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> >
> > #if IS_BUILTIN(CONFIG_LKDTM)
> >
>
> You'll probably get a bisecting hasard with those unused 'static inline'
> functions in a .c file
> because that patch alone probably fails build.
I just built the patch without any issue. The compiler only complains
for unused 'static' (non-inline) functions right?
^ permalink raw reply
* Re: [PATCH v5 8/8] lkdtm/powerpc: Fix code patching hijack test
From: Christopher M. Riedl @ 2021-08-11 17:57 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <7a6c97ed-815b-49fc-5568-ab4420f53122@csgroup.eu>
On Thu Aug 5, 2021 at 4:18 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > Code patching on powerpc with a STRICT_KERNEL_RWX uses a userspace
> > address in a temporary mm on Radix now. Use __put_user() to avoid write
> > failures due to KUAP when attempting a "hijack" on the patching address.
> > __put_user() also works with the non-userspace, vmalloc-based patching
> > address on non-Radix MMUs.
>
> It is not really clean to use __put_user() on non user address,
> allthought it works by change.
>
> I think it would be better to do something like
>
> if (is_kernel_addr(addr))
> copy_to_kernel_nofault(...);
> else
> copy_to_user_nofault(...);
>
Yes that looks much better. I'll pick this up and try it for the next
spin. Thanks!
>
>
> >
> > Signed-off-by: Christopher M. Riedl <cmr@linux.ibm.com>
> > ---
> > drivers/misc/lkdtm/perms.c | 9 ---------
> > 1 file changed, 9 deletions(-)
> >
> > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > index 41e87e5f9cc86..da6a34a0a49fb 100644
> > --- a/drivers/misc/lkdtm/perms.c
> > +++ b/drivers/misc/lkdtm/perms.c
> > @@ -262,16 +262,7 @@ static inline u32 lkdtm_read_patch_site(void)
> > /* Returns True if the write succeeds */
> > static inline bool lkdtm_try_write(u32 data, u32 *addr)
> > {
> > -#ifdef CONFIG_PPC
> > - __put_kernel_nofault(addr, &data, u32, err);
> > - return true;
> > -
> > -err:
> > - return false;
> > -#endif
> > -#ifdef CONFIG_X86_64
> > return !__put_user(data, addr);
> > -#endif
> > }
> >
> > static int lkdtm_patching_cpu(void *data)
> >
^ permalink raw reply
* Re: [PATCH v5 2/8] lkdtm/powerpc: Add test to hijack a patch mapping
From: Christopher M. Riedl @ 2021-08-11 17:57 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <f933e9de-ff3b-aa5a-bb6e-55770d5ab868@csgroup.eu>
On Thu Aug 5, 2021 at 4:13 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > When live patching with STRICT_KERNEL_RWX the CPU doing the patching
> > must temporarily remap the page(s) containing the patch site with +W
> > permissions. While this temporary mapping is in use, another CPU could
> > write to the same mapping and maliciously alter kernel text. Implement a
> > LKDTM test to attempt to exploit such an opening during code patching.
> > The test is implemented on powerpc and requires LKDTM built into the
> > kernel (building LKDTM as a module is insufficient).
> >
> > The LKDTM "hijack" test works as follows:
> >
> > 1. A CPU executes an infinite loop to patch an instruction. This is
> > the "patching" CPU.
> > 2. Another CPU attempts to write to the address of the temporary
> > mapping used by the "patching" CPU. This other CPU is the
> > "hijacker" CPU. The hijack either fails with a fault/error or
> > succeeds, in which case some kernel text is now overwritten.
> >
> > The virtual address of the temporary patch mapping is provided via an
> > LKDTM-specific accessor to the hijacker CPU. This test assumes a
> > hypothetical situation where this address was leaked previously.
> >
> > How to run the test:
> >
> > mount -t debugfs none /sys/kernel/debug
> > (echo HIJACK_PATCH > /sys/kernel/debug/provoke-crash/DIRECT)
> >
> > A passing test indicates that it is not possible to overwrite kernel
> > text from another CPU by using the temporary mapping established by
> > a CPU for patching.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@linux.ibm.com>
> >
> > ---
> >
> > v5: * Use `u32*` instead of `struct ppc_inst*` based on new series in
> > upstream.
> >
> > v4: * Separate the powerpc and x86_64 bits into individual patches.
> > * Use __put_kernel_nofault() when attempting to hijack the mapping
> > * Use raw_smp_processor_id() to avoid triggering the BUG() when
> > calling smp_processor_id() in preemptible code - the only thing
> > that matters is that one of the threads is bound to a different
> > CPU - we are not using smp_processor_id() to access any per-cpu
> > data or similar where preemption should be disabled.
> > * Rework the patching_cpu() kthread stop condition to avoid:
> > https://lwn.net/Articles/628628/
> > ---
> > drivers/misc/lkdtm/core.c | 1 +
> > drivers/misc/lkdtm/lkdtm.h | 1 +
> > drivers/misc/lkdtm/perms.c | 134 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 136 insertions(+)
> >
> > diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> > index 8024b6a5cc7fc..fbcb95eda337b 100644
> > --- a/drivers/misc/lkdtm/core.c
> > +++ b/drivers/misc/lkdtm/core.c
> > @@ -147,6 +147,7 @@ static const struct crashtype crashtypes[] = {
> > CRASHTYPE(WRITE_RO),
> > CRASHTYPE(WRITE_RO_AFTER_INIT),
> > CRASHTYPE(WRITE_KERN),
> > + CRASHTYPE(HIJACK_PATCH),
> > CRASHTYPE(REFCOUNT_INC_OVERFLOW),
> > CRASHTYPE(REFCOUNT_ADD_OVERFLOW),
> > CRASHTYPE(REFCOUNT_INC_NOT_ZERO_OVERFLOW),
> > diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> > index 99f90d3e5e9cb..87e7e6136d962 100644
> > --- a/drivers/misc/lkdtm/lkdtm.h
> > +++ b/drivers/misc/lkdtm/lkdtm.h
> > @@ -62,6 +62,7 @@ void lkdtm_EXEC_USERSPACE(void);
> > void lkdtm_EXEC_NULL(void);
> > void lkdtm_ACCESS_USERSPACE(void);
> > void lkdtm_ACCESS_NULL(void);
> > +void lkdtm_HIJACK_PATCH(void);
> >
> > /* refcount.c */
> > void lkdtm_REFCOUNT_INC_OVERFLOW(void);
> > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > index 2dede2ef658f3..39e7456852229 100644
> > --- a/drivers/misc/lkdtm/perms.c
> > +++ b/drivers/misc/lkdtm/perms.c
> > @@ -9,6 +9,7 @@
> > #include <linux/vmalloc.h>
> > #include <linux/mman.h>
> > #include <linux/uaccess.h>
> > +#include <linux/kthread.h>
> > #include <asm/cacheflush.h>
> >
> > /* Whether or not to fill the target memory area with do_nothing(). */
> > @@ -222,6 +223,139 @@ void lkdtm_ACCESS_NULL(void)
> > pr_err("FAIL: survived bad write\n");
> > }
> >
> > +#if (IS_BUILTIN(CONFIG_LKDTM) && defined(CONFIG_STRICT_KERNEL_RWX) && \
> > + defined(CONFIG_PPC))
>
>
> I think this test shouldn't be limited to CONFIG_PPC and shouldn't be
> limited to
> CONFIG_STRICT_KERNEL_RWX. It should be there all the time.
>
> Also why limiting it to IS_BUILTIN(CONFIG_LKDTM) ?
>
The test needs read_cpu_patching_addr() which definitely cannot be
exposed outside of the kernel (ie. builtin).
> > +/*
> > + * This is just a dummy location to patch-over.
> > + */
> > +static void patching_target(void)
> > +{
> > + return;
> > +}
> > +
> > +#include <asm/code-patching.h>
> > +const u32 *patch_site = (const u32 *)&patching_target;
> > +
> > +static inline int lkdtm_do_patch(u32 data)
> > +{
> > + return patch_instruction((u32 *)patch_site, ppc_inst(data));
> > +}
> > +
> > +static inline u32 lkdtm_read_patch_site(void)
> > +{
> > + return READ_ONCE(*patch_site);
> > +}
> > +
> > +/* Returns True if the write succeeds */
> > +static inline bool lkdtm_try_write(u32 data, u32 *addr)
> > +{
> > + __put_kernel_nofault(addr, &data, u32, err);
> > + return true;
> > +
> > +err:
> > + return false;
> > +}
> > +
> > +static int lkdtm_patching_cpu(void *data)
> > +{
> > + int err = 0;
> > + u32 val = 0xdeadbeef;
> > +
> > + pr_info("starting patching_cpu=%d\n", raw_smp_processor_id());
> > +
> > + do {
> > + err = lkdtm_do_patch(val);
> > + } while (lkdtm_read_patch_site() == val && !err && !kthread_should_stop());
> > +
> > + if (err)
> > + pr_warn("XFAIL: patch_instruction returned error: %d\n", err);
> > +
> > + while (!kthread_should_stop()) {
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule();
> > + }
> > +
> > + return err;
> > +}
> > +
> > +void lkdtm_HIJACK_PATCH(void)
> > +{
> > + struct task_struct *patching_kthrd;
> > + int patching_cpu, hijacker_cpu, attempts;
> > + unsigned long addr;
> > + bool hijacked;
> > + const u32 bad_data = 0xbad00bad;
> > + const u32 original_insn = lkdtm_read_patch_site();
> > +
> > + if (!IS_ENABLED(CONFIG_SMP)) {
> > + pr_err("XFAIL: this test requires CONFIG_SMP\n");
> > + return;
> > + }
> > +
> > + if (num_online_cpus() < 2) {
> > + pr_warn("XFAIL: this test requires at least two cpus\n");
> > + return;
> > + }
> > +
> > + hijacker_cpu = raw_smp_processor_id();
> > + patching_cpu = cpumask_any_but(cpu_online_mask, hijacker_cpu);
> > +
> > + patching_kthrd = kthread_create_on_node(&lkdtm_patching_cpu, NULL,
> > + cpu_to_node(patching_cpu),
> > + "lkdtm_patching_cpu");
> > + kthread_bind(patching_kthrd, patching_cpu);
> > + wake_up_process(patching_kthrd);
> > +
> > + addr = offset_in_page(patch_site) | read_cpu_patching_addr(patching_cpu);
> > +
> > + pr_info("starting hijacker_cpu=%d\n", hijacker_cpu);
> > + for (attempts = 0; attempts < 100000; ++attempts) {
> > + /* Try to write to the other CPU's temp patch mapping */
> > + hijacked = lkdtm_try_write(bad_data, (u32 *)addr);
> > +
> > + if (hijacked) {
> > + if (kthread_stop(patching_kthrd)) {
> > + pr_info("hijack attempts: %d\n", attempts);
> > + pr_err("XFAIL: error stopping patching cpu\n");
> > + return;
> > + }
> > + break;
> > + }
> > + }
> > + pr_info("hijack attempts: %d\n", attempts);
> > +
> > + if (hijacked) {
> > + if (lkdtm_read_patch_site() == bad_data)
> > + pr_err("overwrote kernel text\n");
> > + /*
> > + * There are window conditions where the hijacker cpu manages to
> > + * write to the patch site but the site gets overwritten again by
> > + * the patching cpu. We still consider that a "successful" hijack
> > + * since the hijacker cpu did not fault on the write.
> > + */
> > + pr_err("FAIL: wrote to another cpu's patching area\n");
> > + } else {
> > + kthread_stop(patching_kthrd);
> > + }
> > +
> > + /* Restore the original data to be able to run the test again */
> > + lkdtm_do_patch(original_insn);
> > +}
> > +
> > +#else
> > +
> > +void lkdtm_HIJACK_PATCH(void)
> > +{
> > + if (!IS_ENABLED(CONFIG_PPC))
> > + pr_err("XFAIL: this test only runs on powerpc\n");
> > + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> > + pr_err("XFAIL: this test requires CONFIG_STRICT_KERNEL_RWX\n");
> > + if (!IS_BUILTIN(CONFIG_LKDTM))
> > + pr_err("XFAIL: this test requires CONFIG_LKDTM=y (not =m!)\n");
> > +}
> > +
> > +#endif
> > +
> > void __init lkdtm_perms_init(void)
> > {
> > /* Make sure we can write to __ro_after_init values during __init */
> >
^ permalink raw reply
* Re: [PATCH v5 4/8] lkdtm/x86_64: Add test to hijack a patch mapping
From: Christopher M. Riedl @ 2021-08-11 17:53 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <95de5ec5-8d48-c969-3c9f-966561f9f58e@csgroup.eu>
On Thu Aug 5, 2021 at 4:09 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > A previous commit implemented an LKDTM test on powerpc to exploit the
> > temporary mapping established when patching code with STRICT_KERNEL_RWX
> > enabled. Extend the test to work on x86_64 as well.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@linux.ibm.com>
> > ---
> > drivers/misc/lkdtm/perms.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
> > index 39e7456852229..41e87e5f9cc86 100644
> > --- a/drivers/misc/lkdtm/perms.c
> > +++ b/drivers/misc/lkdtm/perms.c
> > @@ -224,7 +224,7 @@ void lkdtm_ACCESS_NULL(void)
> > }
> >
> > #if (IS_BUILTIN(CONFIG_LKDTM) && defined(CONFIG_STRICT_KERNEL_RWX) && \
> > - defined(CONFIG_PPC))
> > + (defined(CONFIG_PPC) || defined(CONFIG_X86_64)))
> > /*
> > * This is just a dummy location to patch-over.
> > */
> > @@ -233,12 +233,25 @@ static void patching_target(void)
> > return;
> > }
> >
> > -#include <asm/code-patching.h>
> > const u32 *patch_site = (const u32 *)&patching_target;
> >
> > +#ifdef CONFIG_PPC
> > +#include <asm/code-patching.h>
> > +#endif
> > +
> > +#ifdef CONFIG_X86_64
> > +#include <asm/text-patching.h>
> > +#endif
> > +
> > static inline int lkdtm_do_patch(u32 data)
> > {
> > +#ifdef CONFIG_PPC
> > return patch_instruction((u32 *)patch_site, ppc_inst(data));
> > +#endif
> > +#ifdef CONFIG_X86_64
> > + text_poke((void *)patch_site, &data, sizeof(u32));
> > + return 0;
> > +#endif
> > }
> >
> > static inline u32 lkdtm_read_patch_site(void)
> > @@ -249,11 +262,16 @@ static inline u32 lkdtm_read_patch_site(void)
> > /* Returns True if the write succeeds */
> > static inline bool lkdtm_try_write(u32 data, u32 *addr)
> > {
> > +#ifdef CONFIG_PPC
> > __put_kernel_nofault(addr, &data, u32, err);
> > return true;
> >
> > err:
> > return false;
> > +#endif
> > +#ifdef CONFIG_X86_64
> > + return !__put_user(data, addr);
> > +#endif
> > }
> >
> > static int lkdtm_patching_cpu(void *data)
> > @@ -346,8 +364,8 @@ void lkdtm_HIJACK_PATCH(void)
> >
> > void lkdtm_HIJACK_PATCH(void)
> > {
> > - if (!IS_ENABLED(CONFIG_PPC))
> > - pr_err("XFAIL: this test only runs on powerpc\n");
> > + if (!IS_ENABLED(CONFIG_PPC) && !IS_ENABLED(CONFIG_X86_64))
> > + pr_err("XFAIL: this test only runs on powerpc and x86_64\n");
> > if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> > pr_err("XFAIL: this test requires CONFIG_STRICT_KERNEL_RWX\n");
> > if (!IS_BUILTIN(CONFIG_LKDTM))
> >
>
> Instead of spreading arch specific stuff into LKDTM, wouldn't it make
> sence to define common a
> common API ? Because the day another arch like arm64 implements it own
> approach, do we add specific
> functions again and again into LKDTM ?
Hmm a common patch/poke kernel API is probably out of scope for this
series? I do agree though - since you suggested splitting the series
maybe that's something I can add along with the LKDTM patches.
>
> Also, I find it odd to define tests only when they can succeed. For
> other tests like
> ACCESS_USERSPACE, they are there all the time, regardless of whether we
> have selected
> CONFIG_PPC_KUAP or not. I think it should be the same here, have it all
> there time, if
> CONFIG_STRICT_KERNEL_RWX is selected the test succeeds otherwise it
> fails, but it is always there.
I followed the approach in lkdtm_DOUBLE_FAULT and others in
drivers/misc/lkdtm/bugs.c. I suppose it doesn't hurt to always build the
test irrespective of CONFIG_STRICT_KERNEL_RWX.
>
> Christophe
^ permalink raw reply
* Re: [PATCH v5 0/8] Use per-CPU temporary mappings for patching on Radix MMU
From: Christopher M. Riedl @ 2021-08-11 17:49 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
Cc: keescook, peterz, x86, npiggin, linux-hardening, tglx, dja
In-Reply-To: <0c3cb140-23ad-0eb6-7df9-633aa51a097c@csgroup.eu>
On Thu Aug 5, 2021 at 4:03 AM CDT, Christophe Leroy wrote:
>
>
> Le 13/07/2021 à 07:31, Christopher M. Riedl a écrit :
> > When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
> > temporary mappings when patching itself. These mappings temporarily
> > override the strict RWX text protections to permit a write. Currently,
> > powerpc allocates a per-CPU VM area for patching. Patching occurs as
> > follows:
> >
> > 1. Map page in per-CPU VM area w/ PAGE_KERNEL protection
> > 2. Patch text
> > 3. Remove the temporary mapping
> >
> > While the VM area is per-CPU, the mapping is actually inserted into the
> > kernel page tables. Presumably, this could allow another CPU to access
> > the normally write-protected text - either malicously or accidentally -
> > via this same mapping if the address of the VM area is known. Ideally,
> > the mapping should be kept local to the CPU doing the patching [0].
> >
> > x86 introduced "temporary mm" structs which allow the creation of mappings
> > local to a particular CPU [1]. This series intends to bring the notion of a
> > temporary mm to powerpc's Book3s64 Radix MMU and harden it by using such a
> > mapping for patching a kernel with strict RWX permissions.
> >
> > The first four patches implement an LKDTM test "proof-of-concept" which
> > exploits the potential vulnerability (ie. the temporary mapping during patching
> > is exposed in the kernel page tables and accessible by other CPUs) using a
> > simple brute-force approach. This test is implemented for both powerpc and
> > x86_64. The test passes on powerpc Radix with this new series, fails on
> > upstream powerpc, passes on upstream x86_64, and fails on an older (ancient)
> > x86_64 tree without the x86_64 temporary mm patches. The remaining patches add
> > support for and use a temporary mm for code patching on powerpc with the Radix
> > MMU.
>
> I think four first patches (together with last one) are quite
> independent from the heart of the
> series itself which is patches 5, 6, 7. Maybe you should split that
> series it two series ? After all
> those selftests are nice to have but are not absolutely necessary, that
> would help getting forward I
> think.
>
Hmm you're probably right. The selftest at least proves there is a
potential attack which I think is necessary for any hardening related
series/patch. I'll split the series into separate powerpc temp mm and
LKDTM series for the next spin.
> >
> > Tested boot, ftrace, and repeated LKDTM "hijack":
> > - QEMU+KVM (host: POWER9 Blackbird): Radix MMU w/ KUAP
> > - QEMU+KVM (host: POWER9 Blackbird): Hash MMU
> >
> > Tested repeated LKDTM "hijack":
> > - QEMU+KVM (host: AMD desktop): x86_64 upstream
> > - QEMU+KVM (host: AMD desktop): x86_64 w/o percpu temp mm to
> > verify the LKDTM "hijack" test fails
> >
> > Tested boot and ftrace:
> > - QEMU+TCG: ppc44x (bamboo)
> > - QEMU+TCG: g5 (mac99)
> >
> > I also tested with various extra config options enabled as suggested in
> > section 12) in Documentation/process/submit-checklist.rst.
> >
> > v5: * Only support Book3s64 Radix MMU for now. There are some issues with
> > the previous implementation on the Hash MMU as pointed out by Nick
> > Piggin. Fixing these is not trivial so we only support the Radix MMU
> > for now. I tried using realmode (no data translation) to patch with
> > Hash to at least avoid exposing the patch mapping to other CPUs but
> > this doesn't reliably work either since we cannot access vmalloc'd
> > space in realmode.
>
> So you now accept to have two different mode depending on the platform ?
By necessity yes.
> As far as I remember I commented some time ago that non SMP didn't need
> that feature and you were
> reluctant to have two different implementations. What made you change
> your mind ? (just curious).
>
The book3s64 hash mmu support is a pain ;) Supporting both the temp-mm
and vmalloc implementations turned out to be relatively simple - I
initially thought this would be messier. For now we will support both;
however, in the future I'd still like to implement the percpu temp-mm
support for the Hash MMU as well. I suppose we could re-evaluate then if
we want/need both implementations (I know you're in favor of keeping the
vmalloc-based approach for performance reasons on non-SMP).
>
> > * Use percpu variables for the patching_mm and patching_addr. This
> > avoids the need for synchronization mechanisms entirely. Thanks to
> > Peter Zijlstra for pointing out text_mutex which unfortunately didn't
> > work out without larger code changes in powerpc. Also thanks to Daniel
> > Axtens for comments about using percpu variables for the *percpu* temp
> > mm things off list.
> >
> > v4: * It's time to revisit this series again since @jpn and @mpe fixed
> > our known STRICT_*_RWX bugs on powerpc/64s.
> > * Rebase on linuxppc/next:
> > commit ee1bc694fbaec ("powerpc/kvm: Fix build error when PPC_MEM_KEYS/PPC_PSERIES=n")
> > * Completely rework how map_patch() works on book3s64 Hash MMU
> > * Split the LKDTM x86_64 and powerpc bits into separate patches
> > * Annotate commit messages with changes from v3 instead of
> > listing them here completely out-of context...
> >
> > v3: * Rebase on linuxppc/next: commit 9123e3a74ec7 ("Linux 5.9-rc1")
> > * Move temporary mm implementation into code-patching.c where it
> > belongs
> > * Implement LKDTM hijacker test on x86_64 (on IBM time oof) Do
> > * not use address zero for the patching address in the
> > temporary mm (thanks @dja for pointing this out!)
> > * Wrap the LKDTM test w/ CONFIG_SMP as suggested by Christophe
> > Leroy
> > * Comments to clarify PTE pre-allocation and patching addr
> > selection
> >
> > v2: * Rebase on linuxppc/next:
> > commit 105fb38124a4 ("powerpc/8xx: Modify ptep_get()")
> > * Always dirty pte when mapping patch
> > * Use `ppc_inst_len` instead of `sizeof` on instructions
> > * Declare LKDTM patching addr accessor in header where it belongs
> >
> > v1: * Rebase on linuxppc/next (4336b9337824)
> > * Save and restore second hw watchpoint
> > * Use new ppc_inst_* functions for patching check and in LKDTM test
> >
> > rfc-v2: * Many fixes and improvements mostly based on extensive feedback
> > and testing by Christophe Leroy (thanks!).
> > * Make patching_mm and patching_addr static and move
> > '__ro_after_init' to after the variable name (more common in
> > other parts of the kernel)
> > * Use 'asm/debug.h' header instead of 'asm/hw_breakpoint.h' to
> > fix PPC64e compile
> > * Add comment explaining why we use BUG_ON() during the init
> > call to setup for patching later
> > * Move ptep into patch_mapping to avoid walking page tables a
> > second time when unmapping the temporary mapping
> > * Use KUAP under non-radix, also manually dirty the PTE for patch
> > mapping on non-BOOK3S_64 platforms
> > * Properly return any error from __patch_instruction
> > * Do not use 'memcmp' where a simple comparison is appropriate
> > * Simplify expression for patch address by removing pointer maths
> > * Add LKDTM test
> >
> > [0]: https://github.com/linuxppc/issues/issues/224
> > [1]: https://lore.kernel.org/kernel-hardening/20190426232303.28381-1-nadav.amit@gmail.com/
> >
> > Christopher M. Riedl (8):
> > powerpc: Add LKDTM accessor for patching addr
> > lkdtm/powerpc: Add test to hijack a patch mapping
> > x86_64: Add LKDTM accessor for patching addr
> > lkdtm/x86_64: Add test to hijack a patch mapping
> > powerpc/64s: Introduce temporary mm for Radix MMU
> > powerpc: Rework and improve STRICT_KERNEL_RWX patching
> > powerpc/64s: Initialize and use a temporary mm for patching on Radix
> > lkdtm/powerpc: Fix code patching hijack test
> >
> > arch/powerpc/include/asm/code-patching.h | 4 +
> > arch/powerpc/include/asm/debug.h | 1 +
> > arch/powerpc/kernel/process.c | 5 +
> > arch/powerpc/lib/code-patching.c | 240 ++++++++++++++++++++---
> > arch/x86/include/asm/text-patching.h | 4 +
> > arch/x86/kernel/alternative.c | 7 +
> > drivers/misc/lkdtm/core.c | 1 +
> > drivers/misc/lkdtm/lkdtm.h | 1 +
> > drivers/misc/lkdtm/perms.c | 143 ++++++++++++++
> > 9 files changed, 378 insertions(+), 28 deletions(-)
> >
^ permalink raw reply
* [PATCH v2 60/60] KVM: PPC: Book3S HV P9: Remove subcore HMI handling
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
On POWER9 and newer, rather than the complex HMI synchronisation and
subcore state, have each thread un-apply the guest TB offset before
calling into the early HMI handler.
This allows the subcore state to be avoided, including subcore enter
/ exit guest, which includes an expensive divide that shows up
slightly in profiles.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/kvm_ppc.h | 1 +
arch/powerpc/kvm/book3s_hv.c | 12 +++---
arch/powerpc/kvm/book3s_hv_hmi.c | 7 +++-
arch/powerpc/kvm/book3s_hv_p9_entry.c | 2 +-
arch/powerpc/kvm/book3s_hv_ras.c | 54 +++++++++++++++++++++++++++
5 files changed, 67 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 2d88944f9f34..6355a6980ccf 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -760,6 +760,7 @@ void kvmppc_realmode_machine_check(struct kvm_vcpu *vcpu);
void kvmppc_subcore_enter_guest(void);
void kvmppc_subcore_exit_guest(void);
long kvmppc_realmode_hmi_handler(void);
+long kvmppc_p9_realmode_hmi_handler(struct kvm_vcpu *vcpu);
long kvmppc_h_enter(struct kvm_vcpu *vcpu, unsigned long flags,
long pte_index, unsigned long pteh, unsigned long ptel);
long kvmppc_h_remove(struct kvm_vcpu *vcpu, unsigned long flags,
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index e7f4525f2a74..f1f343307578 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4017,8 +4017,6 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
vcpu->arch.ceded = 0;
- kvmppc_subcore_enter_guest();
-
vcpu_vpa_increment_dispatch(vcpu);
if (kvmhv_on_pseries()) {
@@ -4071,8 +4069,6 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
vcpu_vpa_increment_dispatch(vcpu);
- kvmppc_subcore_exit_guest();
-
return trap;
}
@@ -6054,9 +6050,11 @@ static int kvmppc_book3s_init_hv(void)
if (r)
return r;
- r = kvm_init_subcore_bitmap();
- if (r)
- return r;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+ r = kvm_init_subcore_bitmap();
+ if (r)
+ return r;
+ }
/*
* We need a way of accessing the XICS interrupt controller,
diff --git a/arch/powerpc/kvm/book3s_hv_hmi.c b/arch/powerpc/kvm/book3s_hv_hmi.c
index 9af660476314..1ec50c69678b 100644
--- a/arch/powerpc/kvm/book3s_hv_hmi.c
+++ b/arch/powerpc/kvm/book3s_hv_hmi.c
@@ -20,10 +20,15 @@ void wait_for_subcore_guest_exit(void)
/*
* NULL bitmap pointer indicates that KVM module hasn't
- * been loaded yet and hence no guests are running.
+ * been loaded yet and hence no guests are running, or running
+ * on POWER9 or newer CPU.
+ *
* If no KVM is in use, no need to co-ordinate among threads
* as all of them will always be in host and no one is going
* to modify TB other than the opal hmi handler.
+ *
+ * POWER9 and newer don't need this synchronisation.
+ *
* Hence, just return from here.
*/
if (!local_paca->sibling_subcore_state)
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index 1e18c089478e..7d31ad3de723 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -934,7 +934,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
kvmppc_realmode_machine_check(vcpu);
} else if (unlikely(trap == BOOK3S_INTERRUPT_HMI)) {
- kvmppc_realmode_hmi_handler();
+ kvmppc_p9_realmode_hmi_handler(vcpu);
} else if (trap == BOOK3S_INTERRUPT_H_EMUL_ASSIST) {
vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
diff --git a/arch/powerpc/kvm/book3s_hv_ras.c b/arch/powerpc/kvm/book3s_hv_ras.c
index d4bca93b79f6..3f94f4080d04 100644
--- a/arch/powerpc/kvm/book3s_hv_ras.c
+++ b/arch/powerpc/kvm/book3s_hv_ras.c
@@ -136,6 +136,60 @@ void kvmppc_realmode_machine_check(struct kvm_vcpu *vcpu)
vcpu->arch.mce_evt = mce_evt;
}
+
+long kvmppc_p9_realmode_hmi_handler(struct kvm_vcpu *vcpu)
+{
+ struct kvmppc_vcore *vc = vcpu->arch.vcore;
+ long ret = 0;
+
+ /*
+ * Unapply and clear the offset first. That way, if the TB was not
+ * resynced then it will remain in host-offset, and if it was resynced
+ * then it is brought into host-offset. Then the tb offset is
+ * re-applied before continuing with the KVM exit.
+ *
+ * This way, we don't need to actualy know whether not OPAL resynced
+ * the timebase or do any of the complicated dance that the P7/8
+ * path requires.
+ */
+ if (vc->tb_offset_applied) {
+ u64 new_tb = mftb() - vc->tb_offset_applied;
+ mtspr(SPRN_TBU40, new_tb);
+ if ((mftb() & 0xffffff) < (new_tb & 0xffffff)) {
+ new_tb += 0x1000000;
+ mtspr(SPRN_TBU40, new_tb);
+ }
+ vc->tb_offset_applied = 0;
+ }
+
+ local_paca->hmi_irqs++;
+
+ if (hmi_handle_debugtrig(NULL) >= 0) {
+ ret = 1;
+ goto out;
+ }
+
+ if (ppc_md.hmi_exception_early)
+ ppc_md.hmi_exception_early(NULL);
+
+out:
+ if (vc->tb_offset) {
+ u64 new_tb = mftb() + vc->tb_offset;
+ mtspr(SPRN_TBU40, new_tb);
+ if ((mftb() & 0xffffff) < (new_tb & 0xffffff)) {
+ new_tb += 0x1000000;
+ mtspr(SPRN_TBU40, new_tb);
+ }
+ vc->tb_offset_applied = vc->tb_offset;
+ }
+
+ return ret;
+}
+
+/*
+ * The following subcore HMI handling is all only for pre-POWER9 CPUs.
+ */
+
/* Check if dynamic split is in force and return subcore size accordingly. */
static inline int kvmppc_cur_subcore_size(void)
{
--
2.23.0
^ permalink raw reply related
* [PATCH v2 59/60] KVM: PPC: Book3S HV P9: Stop using vc->dpdes
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
The P9 path uses vc->dpdes only for msgsndp / SMT emulation. This adds
an ordering requirement between vcpu->doorbell_request and vc->dpdes for
no real benefit. Use vcpu->doorbell_request directly.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv.c | 18 ++++++++++--------
arch/powerpc/kvm/book3s_hv_builtin.c | 2 ++
arch/powerpc/kvm/book3s_hv_p9_entry.c | 14 ++++++++++----
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 1dc98e553997..e7f4525f2a74 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -761,6 +761,8 @@ static bool kvmppc_doorbell_pending(struct kvm_vcpu *vcpu)
if (vcpu->arch.doorbell_request)
return true;
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ return false;
/*
* Ensure that the read of vcore->dpdes comes after the read
* of vcpu->doorbell_request. This barrier matches the
@@ -2188,8 +2190,10 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
* either vcore->dpdes or doorbell_request.
* On POWER8, doorbell_request is 0.
*/
- *val = get_reg_val(id, vcpu->arch.vcore->dpdes |
- vcpu->arch.doorbell_request);
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ *val = get_reg_val(id, vcpu->arch.doorbell_request);
+ else
+ *val = get_reg_val(id, vcpu->arch.vcore->dpdes);
break;
case KVM_REG_PPC_VTB:
*val = get_reg_val(id, vcpu->arch.vcore->vtb);
@@ -2426,7 +2430,10 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
vcpu->arch.pspb = set_reg_val(id, *val);
break;
case KVM_REG_PPC_DPDES:
- vcpu->arch.vcore->dpdes = set_reg_val(id, *val);
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ vcpu->arch.doorbell_request = set_reg_val(id, *val) & 1;
+ else
+ vcpu->arch.vcore->dpdes = set_reg_val(id, *val);
break;
case KVM_REG_PPC_VTB:
vcpu->arch.vcore->vtb = set_reg_val(id, *val);
@@ -4463,11 +4470,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
if (!nested) {
kvmppc_core_prepare_to_enter(vcpu);
- if (vcpu->arch.doorbell_request) {
- vc->dpdes = 1;
- smp_wmb();
- vcpu->arch.doorbell_request = 0;
- }
if (test_bit(BOOK3S_IRQPRIO_EXTERNAL,
&vcpu->arch.pending_exceptions))
lpcr |= LPCR_MER;
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c b/arch/powerpc/kvm/book3s_hv_builtin.c
index a10bf93054ca..3ed90149ed2e 100644
--- a/arch/powerpc/kvm/book3s_hv_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_builtin.c
@@ -660,6 +660,8 @@ void kvmppc_guest_entry_inject_int(struct kvm_vcpu *vcpu)
int ext;
unsigned long lpcr;
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
/* Insert EXTERNAL bit into LPCR at the MER bit position */
ext = (vcpu->arch.pending_exceptions >> BOOK3S_IRQPRIO_EXTERNAL) & 1;
lpcr = mfspr(SPRN_LPCR);
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index 5745a49021c3..1e18c089478e 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -701,6 +701,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
unsigned long host_pidr;
unsigned long host_dawr1;
unsigned long host_dawrx1;
+ unsigned long dpdes;
hdec = time_limit - *tb;
if (hdec < 0)
@@ -763,8 +764,10 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
if (vc->pcr)
mtspr(SPRN_PCR, vc->pcr | PCR_MASK);
- if (vc->dpdes)
- mtspr(SPRN_DPDES, vc->dpdes);
+ if (vcpu->arch.doorbell_request) {
+ vcpu->arch.doorbell_request = 0;
+ mtspr(SPRN_DPDES, 1);
+ }
if (dawr_enabled()) {
if (vcpu->arch.dawr0 != host_dawr0)
@@ -995,7 +998,10 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
vcpu->arch.shregs.sprg2 = mfspr(SPRN_SPRG2);
vcpu->arch.shregs.sprg3 = mfspr(SPRN_SPRG3);
- vc->dpdes = mfspr(SPRN_DPDES);
+ dpdes = mfspr(SPRN_DPDES);
+ if (dpdes)
+ vcpu->arch.doorbell_request = 1;
+
vc->vtb = mfspr(SPRN_VTB);
dec = mfspr(SPRN_DEC);
@@ -1057,7 +1063,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
}
}
- if (vc->dpdes)
+ if (dpdes)
mtspr(SPRN_DPDES, 0);
if (vc->pcr)
mtspr(SPRN_PCR, PCR_MASK);
--
2.23.0
^ permalink raw reply related
* [PATCH v2 58/60] KVM: PPC: Book3S HV P9: Tidy kvmppc_create_dtl_entry
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
This goes further to removing vcores from the P9 path. Also avoid the
memset in favour of explicitly initialising all fields.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv.c | 61 +++++++++++++++++++++---------------
1 file changed, 35 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index c8ea430d1955..1dc98e553997 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -698,41 +698,30 @@ static u64 vcore_stolen_time(struct kvmppc_vcore *vc, u64 now)
return p;
}
-static void kvmppc_create_dtl_entry(struct kvm_vcpu *vcpu,
- struct kvmppc_vcore *vc, u64 tb)
+static void __kvmppc_create_dtl_entry(struct kvm_vcpu *vcpu,
+ unsigned int pcpu, u64 now,
+ unsigned long stolen)
{
struct dtl_entry *dt;
struct lppaca *vpa;
- unsigned long stolen;
- unsigned long core_stolen;
- u64 now;
- unsigned long flags;
dt = vcpu->arch.dtl_ptr;
vpa = vcpu->arch.vpa.pinned_addr;
- now = tb;
-
- if (cpu_has_feature(CPU_FTR_ARCH_300)) {
- stolen = 0;
- } else {
- core_stolen = vcore_stolen_time(vc, now);
- stolen = core_stolen - vcpu->arch.stolen_logged;
- vcpu->arch.stolen_logged = core_stolen;
- spin_lock_irqsave(&vcpu->arch.tbacct_lock, flags);
- stolen += vcpu->arch.busy_stolen;
- vcpu->arch.busy_stolen = 0;
- spin_unlock_irqrestore(&vcpu->arch.tbacct_lock, flags);
- }
if (!dt || !vpa)
return;
- memset(dt, 0, sizeof(struct dtl_entry));
+
dt->dispatch_reason = 7;
- dt->processor_id = cpu_to_be16(vc->pcpu + vcpu->arch.ptid);
- dt->timebase = cpu_to_be64(now + vc->tb_offset);
+ dt->preempt_reason = 0;
+ dt->processor_id = cpu_to_be16(pcpu + vcpu->arch.ptid);
dt->enqueue_to_dispatch_time = cpu_to_be32(stolen);
+ dt->ready_to_enqueue_time = 0;
+ dt->waiting_to_ready_time = 0;
+ dt->timebase = cpu_to_be64(now);
+ dt->fault_addr = 0;
dt->srr0 = cpu_to_be64(kvmppc_get_pc(vcpu));
dt->srr1 = cpu_to_be64(vcpu->arch.shregs.msr);
+
++dt;
if (dt == vcpu->arch.dtl.pinned_end)
dt = vcpu->arch.dtl.pinned_addr;
@@ -743,6 +732,27 @@ static void kvmppc_create_dtl_entry(struct kvm_vcpu *vcpu,
vcpu->arch.dtl.dirty = true;
}
+static void kvmppc_create_dtl_entry(struct kvm_vcpu *vcpu,
+ struct kvmppc_vcore *vc)
+{
+ unsigned long stolen;
+ unsigned long core_stolen;
+ u64 now;
+ unsigned long flags;
+
+ now = mftb();
+
+ core_stolen = vcore_stolen_time(vc, now);
+ stolen = core_stolen - vcpu->arch.stolen_logged;
+ vcpu->arch.stolen_logged = core_stolen;
+ spin_lock_irqsave(&vcpu->arch.tbacct_lock, flags);
+ stolen += vcpu->arch.busy_stolen;
+ vcpu->arch.busy_stolen = 0;
+ spin_unlock_irqrestore(&vcpu->arch.tbacct_lock, flags);
+
+ __kvmppc_create_dtl_entry(vcpu, vc->pcpu, now + vc->tb_offset, stolen);
+}
+
/* See if there is a doorbell interrupt pending for a vcpu */
static bool kvmppc_doorbell_pending(struct kvm_vcpu *vcpu)
{
@@ -3753,7 +3763,7 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
pvc->pcpu = pcpu + thr;
for_each_runnable_thread(i, vcpu, pvc) {
kvmppc_start_thread(vcpu, pvc);
- kvmppc_create_dtl_entry(vcpu, pvc, mftb());
+ kvmppc_create_dtl_entry(vcpu, pvc);
trace_kvm_guest_enter(vcpu);
if (!vcpu->arch.ptid)
thr0_done = true;
@@ -4304,7 +4314,7 @@ static int kvmppc_run_vcpu(struct kvm_vcpu *vcpu)
if ((vc->vcore_state == VCORE_PIGGYBACK ||
vc->vcore_state == VCORE_RUNNING) &&
!VCORE_IS_EXITING(vc)) {
- kvmppc_create_dtl_entry(vcpu, vc, mftb());
+ kvmppc_create_dtl_entry(vcpu, vc);
kvmppc_start_thread(vcpu, vc);
trace_kvm_guest_enter(vcpu);
} else if (vc->vcore_state == VCORE_SLEEPING) {
@@ -4481,8 +4491,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
local_paca->kvm_hstate.ptid = 0;
local_paca->kvm_hstate.fake_suspend = 0;
- vc->pcpu = pcpu; // for kvmppc_create_dtl_entry
- kvmppc_create_dtl_entry(vcpu, vc, tb);
+ __kvmppc_create_dtl_entry(vcpu, pcpu, tb + vc->tb_offset, 0);
trace_kvm_guest_enter(vcpu);
--
2.23.0
^ permalink raw reply related
* [PATCH v2 57/60] KVM: PPC: Book3S HV P9: Remove most of the vcore logic
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
The P9 path always uses one vcpu per vcore, so none of the the vcore,
locks, stolen time, blocking logic, shared waitq, etc., is required.
Remove most of it.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv.c | 147 ++++++++++++++++++++---------------
1 file changed, 85 insertions(+), 62 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index d4df2add81ae..c8ea430d1955 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -276,6 +276,8 @@ static void kvmppc_core_start_stolen(struct kvmppc_vcore *vc, u64 tb)
{
unsigned long flags;
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
spin_lock_irqsave(&vc->stoltb_lock, flags);
vc->preempt_tb = tb;
spin_unlock_irqrestore(&vc->stoltb_lock, flags);
@@ -285,6 +287,8 @@ static void kvmppc_core_end_stolen(struct kvmppc_vcore *vc, u64 tb)
{
unsigned long flags;
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
spin_lock_irqsave(&vc->stoltb_lock, flags);
if (vc->preempt_tb != TB_NIL) {
vc->stolen_tb += tb - vc->preempt_tb;
@@ -297,7 +301,12 @@ static void kvmppc_core_vcpu_load_hv(struct kvm_vcpu *vcpu, int cpu)
{
struct kvmppc_vcore *vc = vcpu->arch.vcore;
unsigned long flags;
- u64 now = mftb();
+ u64 now;
+
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ return;
+
+ now = mftb();
/*
* We can test vc->runner without taking the vcore lock,
@@ -321,7 +330,12 @@ static void kvmppc_core_vcpu_put_hv(struct kvm_vcpu *vcpu)
{
struct kvmppc_vcore *vc = vcpu->arch.vcore;
unsigned long flags;
- u64 now = mftb();
+ u64 now;
+
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ return;
+
+ now = mftb();
if (vc->runner == vcpu && vc->vcore_state >= VCORE_SLEEPING)
kvmppc_core_start_stolen(vc, now);
@@ -673,6 +687,8 @@ static u64 vcore_stolen_time(struct kvmppc_vcore *vc, u64 now)
u64 p;
unsigned long flags;
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
spin_lock_irqsave(&vc->stoltb_lock, flags);
p = vc->stolen_tb;
if (vc->vcore_state != VCORE_INACTIVE &&
@@ -695,13 +711,19 @@ static void kvmppc_create_dtl_entry(struct kvm_vcpu *vcpu,
dt = vcpu->arch.dtl_ptr;
vpa = vcpu->arch.vpa.pinned_addr;
now = tb;
- core_stolen = vcore_stolen_time(vc, now);
- stolen = core_stolen - vcpu->arch.stolen_logged;
- vcpu->arch.stolen_logged = core_stolen;
- spin_lock_irqsave(&vcpu->arch.tbacct_lock, flags);
- stolen += vcpu->arch.busy_stolen;
- vcpu->arch.busy_stolen = 0;
- spin_unlock_irqrestore(&vcpu->arch.tbacct_lock, flags);
+
+ if (cpu_has_feature(CPU_FTR_ARCH_300)) {
+ stolen = 0;
+ } else {
+ core_stolen = vcore_stolen_time(vc, now);
+ stolen = core_stolen - vcpu->arch.stolen_logged;
+ vcpu->arch.stolen_logged = core_stolen;
+ spin_lock_irqsave(&vcpu->arch.tbacct_lock, flags);
+ stolen += vcpu->arch.busy_stolen;
+ vcpu->arch.busy_stolen = 0;
+ spin_unlock_irqrestore(&vcpu->arch.tbacct_lock, flags);
+ }
+
if (!dt || !vpa)
return;
memset(dt, 0, sizeof(struct dtl_entry));
@@ -898,13 +920,14 @@ static int kvm_arch_vcpu_yield_to(struct kvm_vcpu *target)
* mode handler is not called but no other threads are in the
* source vcore.
*/
-
- spin_lock(&vcore->lock);
- if (target->arch.state == KVMPPC_VCPU_RUNNABLE &&
- vcore->vcore_state != VCORE_INACTIVE &&
- vcore->runner)
- target = vcore->runner;
- spin_unlock(&vcore->lock);
+ if (!cpu_has_feature(CPU_FTR_ARCH_300)) {
+ spin_lock(&vcore->lock);
+ if (target->arch.state == KVMPPC_VCPU_RUNNABLE &&
+ vcore->vcore_state != VCORE_INACTIVE &&
+ vcore->runner)
+ target = vcore->runner;
+ spin_unlock(&vcore->lock);
+ }
return kvm_vcpu_yield_to(target);
}
@@ -3128,13 +3151,6 @@ static void kvmppc_start_thread(struct kvm_vcpu *vcpu, struct kvmppc_vcore *vc)
kvmppc_ipi_thread(cpu);
}
-/* Old path does this in asm */
-static void kvmppc_stop_thread(struct kvm_vcpu *vcpu)
-{
- vcpu->cpu = -1;
- vcpu->arch.thread_cpu = -1;
-}
-
static void kvmppc_wait_for_nap(int n_threads)
{
int cpu = smp_processor_id();
@@ -3223,6 +3239,8 @@ static void kvmppc_vcore_preempt(struct kvmppc_vcore *vc)
{
struct preempted_vcore_list *lp = this_cpu_ptr(&preempted_vcores);
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
vc->vcore_state = VCORE_PREEMPT;
vc->pcpu = smp_processor_id();
if (vc->num_threads < threads_per_vcore(vc->kvm)) {
@@ -3239,6 +3257,8 @@ static void kvmppc_vcore_end_preempt(struct kvmppc_vcore *vc)
{
struct preempted_vcore_list *lp;
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
kvmppc_core_end_stolen(vc, mftb());
if (!list_empty(&vc->preempt_list)) {
lp = &per_cpu(preempted_vcores, vc->pcpu);
@@ -3967,7 +3987,6 @@ static int kvmhv_vcpu_entry_p9_nested(struct kvm_vcpu *vcpu, u64 time_limit, uns
static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
unsigned long lpcr, u64 *tb)
{
- struct kvmppc_vcore *vc = vcpu->arch.vcore;
u64 next_timer;
int trap;
@@ -3983,9 +4002,6 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
kvmppc_subcore_enter_guest();
- vc->entry_exit_map = 1;
- vc->in_guest = 1;
-
vcpu_vpa_increment_dispatch(vcpu);
if (kvmhv_on_pseries()) {
@@ -4038,9 +4054,6 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
vcpu_vpa_increment_dispatch(vcpu);
- vc->entry_exit_map = 0x101;
- vc->in_guest = 0;
-
kvmppc_subcore_exit_guest();
return trap;
@@ -4106,6 +4119,13 @@ static bool kvmppc_vcpu_woken(struct kvm_vcpu *vcpu)
return false;
}
+static bool kvmppc_vcpu_check_block(struct kvm_vcpu *vcpu)
+{
+ if (!vcpu->arch.ceded || kvmppc_vcpu_woken(vcpu))
+ return true;
+ return false;
+}
+
/*
* Check to see if any of the runnable vcpus on the vcore have pending
* exceptions or are no longer ceded
@@ -4116,7 +4136,7 @@ static int kvmppc_vcore_check_block(struct kvmppc_vcore *vc)
int i;
for_each_runnable_thread(i, vcpu, vc) {
- if (!vcpu->arch.ceded || kvmppc_vcpu_woken(vcpu))
+ if (kvmppc_vcpu_check_block(vcpu))
return 1;
}
@@ -4133,6 +4153,8 @@ static void kvmppc_vcore_blocked(struct kvmppc_vcore *vc)
int do_sleep = 1;
u64 block_ns;
+ WARN_ON_ONCE(cpu_has_feature(CPU_FTR_ARCH_300));
+
/* Poll for pending exceptions and ceded state */
cur = start_poll = ktime_get();
if (vc->halt_poll_ns) {
@@ -4398,11 +4420,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
vcpu->arch.ceded = 0;
vcpu->arch.run_task = current;
vcpu->arch.state = KVMPPC_VCPU_RUNNABLE;
- vcpu->arch.busy_preempt = TB_NIL;
vcpu->arch.last_inst = KVM_INST_FETCH_FAILED;
- vc->runnable_threads[0] = vcpu;
- vc->n_runnable = 1;
- vc->runner = vcpu;
/* See if the MMU is ready to go */
if (unlikely(!kvm->arch.mmu_ready)) {
@@ -4420,11 +4438,8 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
kvmppc_update_vpas(vcpu);
- init_vcore_to_run(vc);
-
preempt_disable();
pcpu = smp_processor_id();
- vc->pcpu = pcpu;
if (kvm_is_radix(kvm))
kvmppc_prepare_radix_vcpu(vcpu, pcpu);
@@ -4453,21 +4468,23 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
goto out;
}
- tb = mftb();
+ if (vcpu->arch.timer_running) {
+ hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
+ vcpu->arch.timer_running = 0;
+ }
- vcpu->arch.stolen_logged = vcore_stolen_time(vc, tb);
- vc->preempt_tb = TB_NIL;
+ tb = mftb();
- kvmppc_clear_host_core(pcpu);
+ vcpu->cpu = pcpu;
+ vcpu->arch.thread_cpu = pcpu;
+ local_paca->kvm_hstate.kvm_vcpu = vcpu;
+ local_paca->kvm_hstate.ptid = 0;
+ local_paca->kvm_hstate.fake_suspend = 0;
- local_paca->kvm_hstate.napping = 0;
- local_paca->kvm_hstate.kvm_split_mode = NULL;
- kvmppc_start_thread(vcpu, vc);
+ vc->pcpu = pcpu; // for kvmppc_create_dtl_entry
kvmppc_create_dtl_entry(vcpu, vc, tb);
- trace_kvm_guest_enter(vcpu);
- vc->vcore_state = VCORE_RUNNING;
- trace_kvmppc_run_core(vc, 0);
+ trace_kvm_guest_enter(vcpu);
guest_enter_irqoff();
@@ -4489,11 +4506,10 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
set_irq_happened(trap);
- kvmppc_set_host_core(pcpu);
-
guest_exit_irqoff();
- kvmppc_stop_thread(vcpu);
+ vcpu->cpu = -1;
+ vcpu->arch.thread_cpu = -1;
powerpc_local_irq_pmu_restore(flags);
@@ -4520,28 +4536,31 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
}
vcpu->arch.ret = r;
- if (is_kvmppc_resume_guest(r) && vcpu->arch.ceded &&
- !kvmppc_vcpu_woken(vcpu)) {
+ if (is_kvmppc_resume_guest(r) && !kvmppc_vcpu_check_block(vcpu)) {
kvmppc_set_timer(vcpu);
- while (vcpu->arch.ceded && !kvmppc_vcpu_woken(vcpu)) {
+
+ prepare_to_rcuwait(&vcpu->wait);
+ for (;;) {
+ set_current_state(TASK_INTERRUPTIBLE);
if (signal_pending(current)) {
vcpu->stat.signal_exits++;
run->exit_reason = KVM_EXIT_INTR;
vcpu->arch.ret = -EINTR;
break;
}
- spin_lock(&vc->lock);
- kvmppc_vcore_blocked(vc);
- spin_unlock(&vc->lock);
+
+ if (kvmppc_vcpu_check_block(vcpu))
+ break;
+
+ trace_kvmppc_vcore_blocked(vc, 0);
+ schedule();
+ trace_kvmppc_vcore_blocked(vc, 1);
}
+ finish_rcuwait(&vcpu->wait);
}
vcpu->arch.ceded = 0;
- vc->vcore_state = VCORE_INACTIVE;
- trace_kvmppc_run_core(vc, 1);
-
done:
- kvmppc_remove_runnable(vc, vcpu, tb);
trace_kvmppc_run_vcpu_exit(vcpu);
return vcpu->arch.ret;
@@ -4625,7 +4644,8 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
kvmppc_save_current_sprs();
- vcpu->arch.waitp = &vcpu->arch.vcore->wait;
+ if (!cpu_has_feature(CPU_FTR_ARCH_300))
+ vcpu->arch.waitp = &vcpu->arch.vcore->wait;
vcpu->arch.pgdir = kvm->mm->pgd;
vcpu->arch.state = KVMPPC_VCPU_BUSY_IN_HOST;
@@ -5087,6 +5107,9 @@ void kvmppc_alloc_host_rm_ops(void)
int cpu, core;
int size;
+ if (cpu_has_feature(CPU_FTR_ARCH_300))
+ return;
+
/* Not the first time here ? */
if (kvmppc_host_rm_ops_hv != NULL)
return;
--
2.23.0
^ permalink raw reply related
* [PATCH v2 56/60] KVM: PPC: Book3S HV P9: Avoid cpu_in_guest atomics on entry and exit
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
cpu_in_guest is set to determine if a CPU needs to be IPI'ed to exit
the guest and notice the need_tlb_flush bit.
This can be implemented as a global per-CPU pointer to the currently
running guest instead of per-guest cpumasks, saving 2 atomics per
entry/exit. P7/8 doesn't require cpu_in_guest, nor does a nested HV
(only the L0 does), so move it to the P9 HV path.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s_64.h | 1 -
arch/powerpc/include/asm/kvm_host.h | 1 -
arch/powerpc/kvm/book3s_hv.c | 38 +++++++++++++-----------
3 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index ee25e93febe6..3109b41865b2 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -44,7 +44,6 @@ struct kvm_nested_guest {
struct mutex tlb_lock; /* serialize page faults and tlbies */
struct kvm_nested_guest *next;
cpumask_t need_tlb_flush;
- cpumask_t cpu_in_guest;
short prev_cpu[NR_CPUS];
u8 radix; /* is this nested guest radix */
};
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index ef60f5cce251..2bcac6da0a4b 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -288,7 +288,6 @@ struct kvm_arch {
u32 online_vcores;
atomic_t hpte_mod_interest;
cpumask_t need_tlb_flush;
- cpumask_t cpu_in_guest;
u8 radix;
u8 fwnmi_enabled;
u8 secure_guest;
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index d3fc486a4817..d4df2add81ae 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -3012,30 +3012,33 @@ static void kvmppc_release_hwthread(int cpu)
tpaca->kvm_hstate.kvm_split_mode = NULL;
}
+static DEFINE_PER_CPU(struct kvm *, cpu_in_guest);
+
static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
{
struct kvm_nested_guest *nested = vcpu->arch.nested;
- cpumask_t *cpu_in_guest;
int i;
cpu = cpu_first_tlb_thread_sibling(cpu);
- if (nested) {
+ if (nested)
cpumask_set_cpu(cpu, &nested->need_tlb_flush);
- cpu_in_guest = &nested->cpu_in_guest;
- } else {
+ else
cpumask_set_cpu(cpu, &kvm->arch.need_tlb_flush);
- cpu_in_guest = &kvm->arch.cpu_in_guest;
- }
/*
- * Make sure setting of bit in need_tlb_flush precedes
- * testing of cpu_in_guest bits. The matching barrier on
- * the other side is the first smp_mb() in kvmppc_run_core().
+ * Make sure setting of bit in need_tlb_flush precedes testing of
+ * cpu_in_guest. The matching barrier on the other side is hwsync
+ * when switching to guest MMU mode, which happens between
+ * cpu_in_guest being set to the guest kvm, and need_tlb_flush bit
+ * being tested.
*/
smp_mb();
for (i = cpu; i <= cpu_last_tlb_thread_sibling(cpu);
- i += cpu_tlb_thread_sibling_step())
- if (cpumask_test_cpu(i, cpu_in_guest))
+ i += cpu_tlb_thread_sibling_step()) {
+ struct kvm *running = *per_cpu_ptr(&cpu_in_guest, i);
+
+ if (running == kvm)
smp_call_function_single(i, do_nothing, NULL, 1);
+ }
}
static void do_migrate_away_vcpu(void *arg)
@@ -3103,7 +3106,6 @@ static void kvmppc_start_thread(struct kvm_vcpu *vcpu, struct kvmppc_vcore *vc)
{
int cpu;
struct paca_struct *tpaca;
- struct kvm *kvm = vc->kvm;
cpu = vc->pcpu;
if (vcpu) {
@@ -3114,7 +3116,6 @@ static void kvmppc_start_thread(struct kvm_vcpu *vcpu, struct kvmppc_vcore *vc)
cpu += vcpu->arch.ptid;
vcpu->cpu = vc->pcpu;
vcpu->arch.thread_cpu = cpu;
- cpumask_set_cpu(cpu, &kvm->arch.cpu_in_guest);
}
tpaca = paca_ptrs[cpu];
tpaca->kvm_hstate.kvm_vcpu = vcpu;
@@ -3832,7 +3833,6 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
kvmppc_release_hwthread(pcpu + i);
if (sip && sip->napped[i])
kvmppc_ipi_thread(pcpu + i);
- cpumask_clear_cpu(pcpu + i, &vc->kvm->arch.cpu_in_guest);
}
spin_unlock(&vc->lock);
@@ -4000,8 +4000,14 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
}
} else {
+ struct kvm *kvm = vcpu->kvm;
+
kvmppc_xive_push_vcpu(vcpu);
+
+ __this_cpu_write(cpu_in_guest, kvm);
trap = kvmhv_vcpu_entry_p9(vcpu, time_limit, lpcr, tb);
+ __this_cpu_write(cpu_in_guest, NULL);
+
if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
!(vcpu->arch.shregs.msr & MSR_PR)) {
unsigned long req = kvmppc_get_gpr(vcpu, 3);
@@ -4026,7 +4032,7 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
}
kvmppc_xive_pull_vcpu(vcpu);
- if (kvm_is_radix(vcpu->kvm))
+ if (kvm_is_radix(kvm))
vcpu->arch.slb_max = 0;
}
@@ -4491,8 +4497,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
powerpc_local_irq_pmu_restore(flags);
- cpumask_clear_cpu(pcpu, &kvm->arch.cpu_in_guest);
-
preempt_enable();
/*
--
2.23.0
^ permalink raw reply related
* [PATCH v2 55/60] KVM: PPC: Book3S HV P9: Add unlikely annotation for !mmu_ready
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
The mmu will almost always be ready.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 979223018c8e..d3fc486a4817 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4399,7 +4399,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
vc->runner = vcpu;
/* See if the MMU is ready to go */
- if (!kvm->arch.mmu_ready) {
+ if (unlikely(!kvm->arch.mmu_ready)) {
r = kvmhv_setup_mmu(vcpu);
if (r) {
run->exit_reason = KVM_EXIT_FAIL_ENTRY;
--
2.23.0
^ permalink raw reply related
* [PATCH v2 54/60] KVM: PPC: Book3S HV P9: Avoid changing MSR[RI] in entry and exit
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
kvm_hstate.in_guest provides the equivalent of MSR[RI]=0 protection,
and it covers the existing MSR[RI]=0 section in late entry and early
exit, so clearing and setting MSR[RI] in those cases does not
actually do anything useful.
Remove the RI manipulation and replace it with comments. Make the
in_guest memory accesses a bit closer to a proper critical section
pattern. This speeds up guest entry/exit performance.
This also removes the MSR[RI] warnings which aren't very interesting
and would cause crashes if they hit due to causing an interrupt in
non-recoverable code.
From: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv_p9_entry.c | 50 ++++++++++++---------------
1 file changed, 23 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index cb865fe2580d..5745a49021c3 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -825,7 +825,15 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
* But TM could be split out if this would be a significant benefit.
*/
- local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_HV_P9;
+ /*
+ * MSR[RI] does not need to be cleared (and is not, for radix guests
+ * with no prefetch bug), because in_guest is set. If we take a SRESET
+ * or MCE with in_guest set but still in HV mode, then
+ * kvmppc_p9_bad_interrupt handles the interrupt, which effectively
+ * clears MSR[RI] and doesn't return.
+ */
+ WRITE_ONCE(local_paca->kvm_hstate.in_guest, KVM_GUEST_MODE_HV_P9);
+ barrier(); /* Open in_guest critical section */
/*
* Hash host, hash guest, or radix guest with prefetch bug, all have
@@ -837,14 +845,10 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
save_clear_host_mmu(kvm);
- if (kvm_is_radix(kvm)) {
+ if (kvm_is_radix(kvm))
switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
- if (!cpu_has_feature(CPU_FTR_P9_RADIX_PREFETCH_BUG))
- __mtmsrd(0, 1); /* clear RI */
-
- } else {
+ else
switch_mmu_to_guest_hpt(kvm, vcpu, lpcr);
- }
/* TLBIEL uses LPID=LPIDR, so run this after setting guest LPID */
kvmppc_check_need_tlb_flush(kvm, vc->pcpu, nested);
@@ -899,19 +903,16 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
vcpu->arch.regs.gpr[3] = local_paca->kvm_hstate.scratch2;
/*
- * Only set RI after reading machine check regs (DAR, DSISR, SRR0/1)
- * and hstate scratch (which we need to move into exsave to make
- * re-entrant vs SRESET/MCE)
+ * After reading machine check regs (DAR, DSISR, SRR0/1) and hstate
+ * scratch (which we need to move into exsave to make re-entrant vs
+ * SRESET/MCE), register state is protected from reentrancy. However
+ * timebase, MMU, among other state is still set to guest, so don't
+ * enable MSR[RI] here. It gets enabled at the end, after in_guest
+ * is cleared.
+ *
+ * It is possible an NMI could come in here, which is why it is
+ * important to save the above state early so it can be debugged.
*/
- if (ri_set) {
- if (unlikely(!(mfmsr() & MSR_RI))) {
- __mtmsrd(MSR_RI, 1);
- WARN_ON_ONCE(1);
- }
- } else {
- WARN_ON_ONCE(mfmsr() & MSR_RI);
- __mtmsrd(MSR_RI, 1);
- }
vcpu->arch.regs.gpr[9] = exsave[EX_R9/sizeof(u64)];
vcpu->arch.regs.gpr[10] = exsave[EX_R10/sizeof(u64)];
@@ -969,13 +970,6 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
*/
mtspr(SPRN_HSRR0, vcpu->arch.regs.nip);
mtspr(SPRN_HSRR1, vcpu->arch.shregs.msr);
-
- /*
- * tm_return_to_guest re-loads SRR0/1, DAR,
- * DSISR after RI is cleared, in case they had
- * been clobbered by a MCE.
- */
- __mtmsrd(0, 1); /* clear RI */
goto tm_return_to_guest;
}
}
@@ -1075,7 +1069,9 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
restore_p9_host_os_sprs(vcpu, &host_os_sprs);
- local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_NONE;
+ barrier(); /* Close in_guest critical section */
+ WRITE_ONCE(local_paca->kvm_hstate.in_guest, KVM_GUEST_MODE_NONE);
+ /* Interrupts are recoverable at this point */
/*
* cp_abort is required if the processor supports local copy-paste
--
2.23.0
^ permalink raw reply related
* [PATCH v2 53/60] KVM: PPC: Book3S HV P9: Optimise hash guest SLB saving
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
slbmfee/slbmfev instructions are very expensive, moreso than a regular
mfspr instruction, so minimising them significantly improves hash guest
exit performance. The slbmfev is only required if slbmfee found a valid
SLB entry.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv_p9_entry.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index fa6ac153c0f9..cb865fe2580d 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -483,10 +483,22 @@ static void __accumulate_time(struct kvm_vcpu *vcpu, struct kvmhv_tb_accumulator
#define accumulate_time(vcpu, next) do {} while (0)
#endif
-static inline void mfslb(unsigned int idx, u64 *slbee, u64 *slbev)
+static inline u64 mfslbv(unsigned int idx)
{
- asm volatile("slbmfev %0,%1" : "=r" (*slbev) : "r" (idx));
- asm volatile("slbmfee %0,%1" : "=r" (*slbee) : "r" (idx));
+ u64 slbev;
+
+ asm volatile("slbmfev %0,%1" : "=r" (slbev) : "r" (idx));
+
+ return slbev;
+}
+
+static inline u64 mfslbe(unsigned int idx)
+{
+ u64 slbee;
+
+ asm volatile("slbmfee %0,%1" : "=r" (slbee) : "r" (idx));
+
+ return slbee;
}
static inline void mtslb(u64 slbee, u64 slbev)
@@ -616,8 +628,10 @@ static void save_clear_guest_mmu(struct kvm *kvm, struct kvm_vcpu *vcpu)
*/
for (i = 0; i < vcpu->arch.slb_nr; i++) {
u64 slbee, slbev;
- mfslb(i, &slbee, &slbev);
+
+ slbee = mfslbe(i);
if (slbee & SLB_ESID_V) {
+ slbev = mfslbv(i);
vcpu->arch.slb[nr].orige = slbee | i;
vcpu->arch.slb[nr].origv = slbev;
nr++;
--
2.23.0
^ permalink raw reply related
* [PATCH v2 52/60] KVM: PPC: Book3S HV P9: Improve mfmsr performance on entry
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
Rearrange the MSR saving on entry so it does not follow the mtmsrd to
disable interrupts, avoiding a possible RAW scoreboard stall.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/kvm_book3s_64.h | 2 +
arch/powerpc/kvm/book3s_hv.c | 18 ++-----
arch/powerpc/kvm/book3s_hv_p9_entry.c | 66 +++++++++++++++---------
3 files changed, 47 insertions(+), 39 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
index f721bb1a5eab..ee25e93febe6 100644
--- a/arch/powerpc/include/asm/kvm_book3s_64.h
+++ b/arch/powerpc/include/asm/kvm_book3s_64.h
@@ -154,6 +154,8 @@ static inline bool kvmhv_vcpu_is_radix(struct kvm_vcpu *vcpu)
return radix;
}
+unsigned long kvmppc_msr_hard_disable_set_facilities(struct kvm_vcpu *vcpu, unsigned long msr);
+
int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpcr, u64 *tb);
#define KVM_DEFAULT_HPT_ORDER 24 /* 16MB HPT by default */
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 7337c0ca94c6..979223018c8e 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -3881,6 +3881,8 @@ static int kvmhv_vcpu_entry_p9_nested(struct kvm_vcpu *vcpu, u64 time_limit, uns
s64 dec;
int trap;
+ msr = mfmsr();
+
save_p9_host_os_sprs(&host_os_sprs);
/*
@@ -3891,24 +3893,10 @@ static int kvmhv_vcpu_entry_p9_nested(struct kvm_vcpu *vcpu, u64 time_limit, uns
*/
host_psscr = mfspr(SPRN_PSSCR_PR);
- hard_irq_disable();
+ kvmppc_msr_hard_disable_set_facilities(vcpu, msr);
if (lazy_irq_pending())
return 0;
- /* MSR bits may have been cleared by context switch */
- msr = 0;
- if (IS_ENABLED(CONFIG_PPC_FPU))
- msr |= MSR_FP;
- if (cpu_has_feature(CPU_FTR_ALTIVEC))
- msr |= MSR_VEC;
- if (cpu_has_feature(CPU_FTR_VSX))
- msr |= MSR_VSX;
- if ((cpu_has_feature(CPU_FTR_TM) ||
- cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) &&
- (vcpu->arch.hfscr & HFSCR_TM))
- msr |= MSR_TM;
- msr = msr_check_and_set(msr);
-
if (unlikely(load_vcpu_state(vcpu, &host_os_sprs)))
msr = mfmsr(); /* TM restore can update msr */
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index 94b15294a388..fa6ac153c0f9 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -628,6 +628,44 @@ static void save_clear_guest_mmu(struct kvm *kvm, struct kvm_vcpu *vcpu)
}
}
+unsigned long kvmppc_msr_hard_disable_set_facilities(struct kvm_vcpu *vcpu, unsigned long msr)
+{
+ unsigned long msr_needed = 0;
+
+ msr &= ~MSR_EE;
+
+ /* MSR bits may have been cleared by context switch so must recheck */
+ if (IS_ENABLED(CONFIG_PPC_FPU))
+ msr_needed |= MSR_FP;
+ if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ msr_needed |= MSR_VEC;
+ if (cpu_has_feature(CPU_FTR_VSX))
+ msr_needed |= MSR_VSX;
+ if ((cpu_has_feature(CPU_FTR_TM) ||
+ cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) &&
+ (vcpu->arch.hfscr & HFSCR_TM))
+ msr_needed |= MSR_TM;
+
+ /*
+ * This could be combined with MSR[RI] clearing, but that expands
+ * the unrecoverable window. It would be better to cover unrecoverable
+ * with KVM bad interrupt handling rather than use MSR[RI] at all.
+ *
+ * Much more difficult and less worthwhile to combine with IR/DR
+ * disable.
+ */
+ if ((msr & msr_needed) != msr_needed) {
+ msr |= msr_needed;
+ __mtmsrd(msr, 0);
+ } else {
+ __hard_irq_disable();
+ }
+ local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
+
+ return msr;
+}
+EXPORT_SYMBOL_GPL(kvmppc_msr_hard_disable_set_facilities);
+
int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpcr, u64 *tb)
{
struct p9_host_os_sprs host_os_sprs;
@@ -661,6 +699,9 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
vcpu->arch.ceded = 0;
+ /* Save MSR for restore, with EE clear. */
+ msr = mfmsr() & ~MSR_EE;
+
host_hfscr = mfspr(SPRN_HFSCR);
host_ciabr = mfspr(SPRN_CIABR);
host_psscr = mfspr(SPRN_PSSCR_PR);
@@ -682,35 +723,12 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
save_p9_host_os_sprs(&host_os_sprs);
- /*
- * This could be combined with MSR[RI] clearing, but that expands
- * the unrecoverable window. It would be better to cover unrecoverable
- * with KVM bad interrupt handling rather than use MSR[RI] at all.
- *
- * Much more difficult and less worthwhile to combine with IR/DR
- * disable.
- */
- hard_irq_disable();
+ msr = kvmppc_msr_hard_disable_set_facilities(vcpu, msr);
if (lazy_irq_pending()) {
trap = 0;
goto out;
}
- /* MSR bits may have been cleared by context switch */
- msr = 0;
- if (IS_ENABLED(CONFIG_PPC_FPU))
- msr |= MSR_FP;
- if (cpu_has_feature(CPU_FTR_ALTIVEC))
- msr |= MSR_VEC;
- if (cpu_has_feature(CPU_FTR_VSX))
- msr |= MSR_VSX;
- if ((cpu_has_feature(CPU_FTR_TM) ||
- cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) &&
- (vcpu->arch.hfscr & HFSCR_TM))
- msr |= MSR_TM;
- msr = msr_check_and_set(msr);
- /* Save MSR for restore. This is after hard disable, so EE is clear. */
-
if (unlikely(load_vcpu_state(vcpu, &host_os_sprs)))
msr = mfmsr(); /* MSR may have been updated */
--
2.23.0
^ permalink raw reply related
* [PATCH v2 51/60] KVM: PPC: Book3S HV Nested: Avoid extra mftb() in nested entry
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
mftb() is expensive and one can be avoided on nested guest dispatch.
If the time checking code distinguishes between the L0 timer and the
nested HV timer, then both can be tested in the same place with the
same mftb() value.
This also nicely illustrates the relationship between the L0 and nested
HV timers.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/kvm_asm.h | 1 +
arch/powerpc/kvm/book3s_hv.c | 12 ++++++++++++
arch/powerpc/kvm/book3s_hv_nested.c | 5 -----
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_asm.h b/arch/powerpc/include/asm/kvm_asm.h
index fbbf3cec92e9..d68d71987d5c 100644
--- a/arch/powerpc/include/asm/kvm_asm.h
+++ b/arch/powerpc/include/asm/kvm_asm.h
@@ -79,6 +79,7 @@
#define BOOK3S_INTERRUPT_FP_UNAVAIL 0x800
#define BOOK3S_INTERRUPT_DECREMENTER 0x900
#define BOOK3S_INTERRUPT_HV_DECREMENTER 0x980
+#define BOOK3S_INTERRUPT_NESTED_HV_DECREMENTER 0x1980
#define BOOK3S_INTERRUPT_DOORBELL 0xa00
#define BOOK3S_INTERRUPT_SYSCALL 0xc00
#define BOOK3S_INTERRUPT_TRACE 0xd00
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 7d08b826d355..7337c0ca94c6 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1486,6 +1486,10 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
run->ready_for_interrupt_injection = 1;
switch (vcpu->arch.trap) {
/* We're good on these - the host merely wanted to get our attention */
+ case BOOK3S_INTERRUPT_NESTED_HV_DECREMENTER:
+ WARN_ON_ONCE(1); /* Should never happen */
+ vcpu->arch.trap = BOOK3S_INTERRUPT_HV_DECREMENTER;
+ fallthrough;
case BOOK3S_INTERRUPT_HV_DECREMENTER:
vcpu->stat.dec_exits++;
r = RESUME_GUEST;
@@ -1817,6 +1821,12 @@ static int kvmppc_handle_nested_exit(struct kvm_vcpu *vcpu)
vcpu->stat.ext_intr_exits++;
r = RESUME_GUEST;
break;
+ /* These need to go to the nested HV */
+ case BOOK3S_INTERRUPT_NESTED_HV_DECREMENTER:
+ vcpu->arch.trap = BOOK3S_INTERRUPT_HV_DECREMENTER;
+ vcpu->stat.dec_exits++;
+ r = RESUME_HOST;
+ break;
/* SR/HMI/PMI are HV interrupts that host has handled. Resume guest.*/
case BOOK3S_INTERRUPT_HMI:
case BOOK3S_INTERRUPT_PERFMON:
@@ -3978,6 +3988,8 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
return BOOK3S_INTERRUPT_HV_DECREMENTER;
if (next_timer < time_limit)
time_limit = next_timer;
+ else if (*tb >= time_limit) /* nested time limit */
+ return BOOK3S_INTERRUPT_NESTED_HV_DECREMENTER;
vcpu->arch.ceded = 0;
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index 7bed0b91245e..e57c08b968c0 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -375,11 +375,6 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
vcpu->arch.ret = RESUME_GUEST;
vcpu->arch.trap = 0;
do {
- if (mftb() >= hdec_exp) {
- vcpu->arch.trap = BOOK3S_INTERRUPT_HV_DECREMENTER;
- r = RESUME_HOST;
- break;
- }
r = kvmhv_run_single_vcpu(vcpu, hdec_exp, lpcr);
} while (is_kvmppc_resume_guest(r));
--
2.23.0
^ permalink raw reply related
* [PATCH v2 50/60] KVM: PPC: Book3S HV P9: Avoid tlbsync sequence on radix guest exit
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
Use the existing TLB flushing logic to IPI the previous CPU and run the
necessary barriers before running a guest vCPU on a new physical CPU,
to do the necessary radix GTSE barriers for handling the case of an
interrupted guest tlbie sequence.
This results in more IPIs than the TLB flush logic requires, but it's
a significant win for common case scheduling when the vCPU remains on
the same physical CPU.
This saves about 520 cycles (nearly 10%) on a guest entry+exit micro
benchmark on a POWER9.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv.c | 31 +++++++++++++++++++++++----
arch/powerpc/kvm/book3s_hv_p9_entry.c | 9 --------
2 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 3983c5fa065a..7d08b826d355 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -3028,6 +3028,25 @@ static void radix_flush_cpu(struct kvm *kvm, int cpu, struct kvm_vcpu *vcpu)
smp_call_function_single(i, do_nothing, NULL, 1);
}
+static void do_migrate_away_vcpu(void *arg)
+{
+ struct kvm_vcpu *vcpu = arg;
+ struct kvm *kvm = vcpu->kvm;
+
+ /*
+ * If the guest has GTSE, it may execute tlbie, so do a eieio; tlbsync;
+ * ptesync sequence on the old CPU before migrating to a new one, in
+ * case we interrupted the guest between a tlbie ; eieio ;
+ * tlbsync; ptesync sequence.
+ *
+ * Otherwise, ptesync is sufficient.
+ */
+ if (kvm->arch.lpcr & LPCR_GTSE)
+ asm volatile("eieio; tlbsync; ptesync");
+ else
+ asm volatile("ptesync");
+}
+
static void kvmppc_prepare_radix_vcpu(struct kvm_vcpu *vcpu, int pcpu)
{
struct kvm_nested_guest *nested = vcpu->arch.nested;
@@ -3055,10 +3074,14 @@ static void kvmppc_prepare_radix_vcpu(struct kvm_vcpu *vcpu, int pcpu)
* so we use a single bit in .need_tlb_flush for all 4 threads.
*/
if (prev_cpu != pcpu) {
- if (prev_cpu >= 0 &&
- cpu_first_tlb_thread_sibling(prev_cpu) !=
- cpu_first_tlb_thread_sibling(pcpu))
- radix_flush_cpu(kvm, prev_cpu, vcpu);
+ if (prev_cpu >= 0) {
+ if (cpu_first_tlb_thread_sibling(prev_cpu) !=
+ cpu_first_tlb_thread_sibling(pcpu))
+ radix_flush_cpu(kvm, prev_cpu, vcpu);
+
+ smp_call_function_single(prev_cpu,
+ do_migrate_away_vcpu, vcpu, 1);
+ }
if (nested)
nested->prev_cpu[vcpu->arch.nested_vcpu_id] = pcpu;
else
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index 183d5884e362..94b15294a388 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -1045,15 +1045,6 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_NONE;
- if (kvm_is_radix(kvm)) {
- /*
- * Since this is radix, do a eieio; tlbsync; ptesync sequence
- * in case we interrupted the guest between a tlbie and a
- * ptesync.
- */
- asm volatile("eieio; tlbsync; ptesync");
- }
-
/*
* cp_abort is required if the processor supports local copy-paste
* to clear the copy buffer that was under control of the guest.
--
2.23.0
^ permalink raw reply related
* [PATCH v2 49/60] KVM: PPC: Book3S HV P9: Don't restore PSSCR if not needed
From: Nicholas Piggin @ 2021-08-11 16:01 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <20210811160134.904987-1-npiggin@gmail.com>
This also moves the PSSCR update in nested entry to avoid a SPR
scoreboard stall.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv.c | 7 +++++--
arch/powerpc/kvm/book3s_hv_p9_entry.c | 26 +++++++++++++++++++-------
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 719d8bec4436..3983c5fa065a 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -3879,7 +3879,9 @@ static int kvmhv_vcpu_entry_p9_nested(struct kvm_vcpu *vcpu, u64 time_limit, uns
if (unlikely(load_vcpu_state(vcpu, &host_os_sprs)))
msr = mfmsr(); /* TM restore can update msr */
- mtspr(SPRN_PSSCR_PR, vcpu->arch.psscr);
+ if (vcpu->arch.psscr != host_psscr)
+ mtspr(SPRN_PSSCR_PR, vcpu->arch.psscr);
+
kvmhv_save_hv_regs(vcpu, &hvregs);
hvregs.lpcr = lpcr;
vcpu->arch.regs.msr = vcpu->arch.shregs.msr;
@@ -3920,7 +3922,6 @@ static int kvmhv_vcpu_entry_p9_nested(struct kvm_vcpu *vcpu, u64 time_limit, uns
vcpu->arch.shregs.dar = mfspr(SPRN_DAR);
vcpu->arch.shregs.dsisr = mfspr(SPRN_DSISR);
vcpu->arch.psscr = mfspr(SPRN_PSSCR_PR);
- mtspr(SPRN_PSSCR_PR, host_psscr);
store_vcpu_state(vcpu);
@@ -3933,6 +3934,8 @@ static int kvmhv_vcpu_entry_p9_nested(struct kvm_vcpu *vcpu, u64 time_limit, uns
timer_rearm_host_dec(*tb);
restore_p9_host_os_sprs(vcpu, &host_os_sprs);
+ if (vcpu->arch.psscr != host_psscr)
+ mtspr(SPRN_PSSCR_PR, host_psscr);
return trap;
}
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index f8599e6f75fc..183d5884e362 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -645,6 +645,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
unsigned long host_dawr0;
unsigned long host_dawrx0;
unsigned long host_psscr;
+ unsigned long host_hpsscr;
unsigned long host_pidr;
unsigned long host_dawr1;
unsigned long host_dawrx1;
@@ -662,7 +663,9 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
host_hfscr = mfspr(SPRN_HFSCR);
host_ciabr = mfspr(SPRN_CIABR);
- host_psscr = mfspr(SPRN_PSSCR);
+ host_psscr = mfspr(SPRN_PSSCR_PR);
+ if (cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST))
+ host_hpsscr = mfspr(SPRN_PSSCR);
host_pidr = mfspr(SPRN_PID);
if (dawr_enabled()) {
@@ -746,8 +749,14 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
if (vcpu->arch.ciabr != host_ciabr)
mtspr(SPRN_CIABR, vcpu->arch.ciabr);
- mtspr(SPRN_PSSCR, vcpu->arch.psscr | PSSCR_EC |
- (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
+
+ if (cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) {
+ mtspr(SPRN_PSSCR, vcpu->arch.psscr | PSSCR_EC |
+ (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
+ } else {
+ if (vcpu->arch.psscr != host_psscr)
+ mtspr(SPRN_PSSCR_PR, vcpu->arch.psscr);
+ }
mtspr(SPRN_HFSCR, vcpu->arch.hfscr);
@@ -953,7 +962,7 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
vcpu->arch.ic = mfspr(SPRN_IC);
vcpu->arch.pid = mfspr(SPRN_PID);
- vcpu->arch.psscr = mfspr(SPRN_PSSCR) & PSSCR_GUEST_VIS;
+ vcpu->arch.psscr = mfspr(SPRN_PSSCR_PR);
vcpu->arch.shregs.sprg0 = mfspr(SPRN_SPRG0);
vcpu->arch.shregs.sprg1 = mfspr(SPRN_SPRG1);
@@ -999,9 +1008,12 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
mtspr(SPRN_PURR, local_paca->kvm_hstate.host_purr);
mtspr(SPRN_SPURR, local_paca->kvm_hstate.host_spurr);
- /* Preserve PSSCR[FAKE_SUSPEND] until we've called kvmppc_save_tm_hv */
- mtspr(SPRN_PSSCR, host_psscr |
- (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
+ if (cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST)) {
+ /* Preserve PSSCR[FAKE_SUSPEND] until we've called kvmppc_save_tm_hv */
+ mtspr(SPRN_PSSCR, host_hpsscr |
+ (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
+ }
+
mtspr(SPRN_HFSCR, host_hfscr);
if (vcpu->arch.ciabr != host_ciabr)
mtspr(SPRN_CIABR, host_ciabr);
--
2.23.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