* Re: [PATCH v6 0/5] clean up redundant 'kvm_run' parameters
From: Paolo Bonzini @ 2020-07-10 8:06 UTC (permalink / raw)
To: Tianjia Zhang, tsbogend, paulus, mpe, benh, borntraeger, frankja,
david, cohuck, heiko.carstens, gor, sean.j.christopherson,
vkuznets, wanpengli, jmattson, joro, tglx, mingo, bp, x86, hpa,
maz, james.morse, julien.thierry.kdev, suzuki.poulose,
christoffer.dall, peterx, thuth, chenhuacai
Cc: linux-s390, kvm, linux-mips, kvm-ppc, linux-kernel, linuxppc-dev,
kvmarm, linux-arm-kernel
In-Reply-To: <6604e273-d7b1-5007-8721-75c4a4dec68e@linux.alibaba.com>
On 10/07/20 09:32, Tianjia Zhang wrote:
> Hi Paolo,
>
> Any opinion on this series patches? Can I help with this patchset ?
I was hoping to have some Tested-by, for now I'm queuing patches 1 and
2. Thanks,
Paolo
> Thanks and best,
> Tianjia
>
> On 2020/6/23 21:14, Tianjia Zhang wrote:
>> In the current kvm version, 'kvm_run' has been included in the 'kvm_vcpu'
>> structure. For historical reasons, many kvm-related function parameters
>> retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This
>> patch does a unified cleanup of these remaining redundant parameters.
>>
>> This series of patches has completely cleaned the architecture of
>> arm64, mips, ppc, and s390 (no such redundant code on x86). Due to
>> the large number of modified codes, a separate patch is made for each
>> platform. On the ppc platform, there is also a redundant structure
>> pointer of 'kvm_run' in 'vcpu_arch', which has also been cleaned
>> separately.
>>
>> ---
>> v6 changes:
>> Rearrange patch sets, only keep the unmerged patch.
>> rebase on mainline.
>>
>> v5 change:
>> ppc: fix for review.
>>
>> v4 change:
>> mips: fixes two errors in entry.c.
>>
>> v3 change:
>> Keep the existing `vcpu->run` in the function body unchanged.
>>
>> v2 change:
>> s390 retains the original variable name and minimizes modification.
>>
>> Tianjia Zhang (5):
>> KVM: s390: clean up redundant 'kvm_run' parameters
>> KVM: arm64: clean up redundant 'kvm_run' parameters
>> KVM: PPC: clean up redundant kvm_run parameters in assembly
>> KVM: MIPS: clean up redundant 'kvm_run' parameters
>> KVM: MIPS: clean up redundant kvm_run parameters in assembly
>>
>> arch/arm64/include/asm/kvm_coproc.h | 12 +--
>> arch/arm64/include/asm/kvm_host.h | 11 +--
>> arch/arm64/include/asm/kvm_mmu.h | 2 +-
>> arch/arm64/kvm/arm.c | 6 +-
>> arch/arm64/kvm/handle_exit.c | 36 ++++----
>> arch/arm64/kvm/mmio.c | 11 +--
>> arch/arm64/kvm/mmu.c | 5 +-
>> arch/arm64/kvm/sys_regs.c | 13 ++-
>> arch/mips/include/asm/kvm_host.h | 32 ++------
>> arch/mips/kvm/emulate.c | 59 +++++--------
>> arch/mips/kvm/entry.c | 21 ++---
>> arch/mips/kvm/mips.c | 14 ++--
>> arch/mips/kvm/trap_emul.c | 114 +++++++++++---------------
>> arch/mips/kvm/vz.c | 26 +++---
>> arch/powerpc/include/asm/kvm_ppc.h | 2 +-
>> arch/powerpc/kvm/book3s_interrupts.S | 22 +++--
>> arch/powerpc/kvm/book3s_pr.c | 9 +-
>> arch/powerpc/kvm/booke.c | 9 +-
>> arch/powerpc/kvm/booke_interrupts.S | 9 +-
>> arch/powerpc/kvm/bookehv_interrupts.S | 10 +--
>> arch/s390/kvm/kvm-s390.c | 23 ++++--
>> 21 files changed, 188 insertions(+), 258 deletions(-)
>>
>
^ permalink raw reply
* Re: [RFC PATCH 7/7] lazy tlb: shoot lazies, a non-refcounting lazy tlb option
From: Peter Zijlstra @ 2020-07-10 9:35 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Arnd Bergmann, x86, linux-kernel, linux-mm,
Mathieu Desnoyers, linuxppc-dev
In-Reply-To: <20200710015646.2020871-8-npiggin@gmail.com>
On Fri, Jul 10, 2020 at 11:56:46AM +1000, Nicholas Piggin wrote:
> On big systems, the mm refcount can become highly contented when doing
> a lot of context switching with threaded applications (particularly
> switching between the idle thread and an application thread).
>
> Abandoning lazy tlb slows switching down quite a bit in the important
> user->idle->user cases, so so instead implement a non-refcounted scheme
> that causes __mmdrop() to IPI all CPUs in the mm_cpumask and shoot down
> any remaining lazy ones.
>
> On a 16-socket 192-core POWER8 system, a context switching benchmark
> with as many software threads as CPUs (so each switch will go in and
> out of idle), upstream can achieve a rate of about 1 million context
> switches per second. After this patch it goes up to 118 million.
That's mighty impressive, however:
> +static void shoot_lazy_tlbs(struct mm_struct *mm)
> +{
> + if (IS_ENABLED(CONFIG_MMU_LAZY_TLB_SHOOTDOWN)) {
> + smp_call_function_many(mm_cpumask(mm), do_shoot_lazy_tlb, (void *)mm, 1);
> + do_shoot_lazy_tlb(mm);
> + }
> +}
IIRC you (power) never clear a CPU from that mask, so for other
workloads I can see this resulting in massive amounts of IPIs.
For instance, take as many processes as you have CPUs. For each,
manually walk the task across all CPUs and exit. Again.
Clearly, that's an extreme, but still...
^ permalink raw reply
* Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
From: Peter Zijlstra @ 2020-07-10 9:42 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Arnd Bergmann, x86, linux-kernel, linux-mm,
Mathieu Desnoyers, linuxppc-dev
In-Reply-To: <20200710015646.2020871-5-npiggin@gmail.com>
On Fri, Jul 10, 2020 at 11:56:43AM +1000, Nicholas Piggin wrote:
> And get rid of the generic sync_core_before_usermode facility.
>
> This helper is the wrong way around I think. The idea that membarrier
> state requires a core sync before returning to user is the easy one
> that does not need hiding behind membarrier calls. The gap in core
> synchronization due to x86's sysret/sysexit and lazy tlb mode, is the
> tricky detail that is better put in x86 lazy tlb code.
>
> Consider if an arch did not synchronize core in switch_mm either, then
> membarrier_mm_sync_core_before_usermode would be in the wrong place
> but arch specific mmu context functions would still be the right place.
> There is also a exit_lazy_tlb case that is not covered by this call, which
> could be a bugs (kthread use mm the membarrier process's mm then context
> switch back to the process without switching mm or lazy mm switch).
>
> This makes lazy tlb code a bit more modular.
Hurmph, I know I've been staring at this at some point. I think I meant
to have a TIF to force the IRET path in the case of MEMBAR_SYNC_CORE.
But I was discouraged by amluto.
^ permalink raw reply
* Re: [RFC PATCH 5/7] lazy tlb: introduce lazy mm refcount helper functions
From: Peter Zijlstra @ 2020-07-10 9:48 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Arnd Bergmann, x86, linux-kernel, linux-mm,
Mathieu Desnoyers, linuxppc-dev
In-Reply-To: <20200710015646.2020871-6-npiggin@gmail.com>
On Fri, Jul 10, 2020 at 11:56:44AM +1000, Nicholas Piggin wrote:
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 73199470c265..ad95812d2a3f 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1253,7 +1253,7 @@ void start_secondary(void *unused)
> unsigned int cpu = smp_processor_id();
> struct cpumask *(*sibling_mask)(int) = cpu_sibling_mask;
>
> - mmgrab(&init_mm);
> + mmgrab(&init_mm); /* XXX: where is the mmput for this? */
> current->active_mm = &init_mm;
>
> smp_store_cpu_info(cpu);
Right; so IIRC it should be this one:
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 134688d79589..ff9fcbc4e76b 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -578,7 +578,7 @@ static int finish_cpu(unsigned int cpu)
> */
> if (mm != &init_mm)
> idle->active_mm = &init_mm;
> - mmdrop(mm);
> + mmdrop_lazy_tlb(mm);
> return 0;
> }
^ permalink raw reply
* Re: PowerNV PCI & SR-IOV cleanups
From: Oliver O'Halloran @ 2020-07-10 12:45 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-pci, linuxppc-dev
In-Reply-To: <20200710064535.GA8354@infradead.org>
On Fri, Jul 10, 2020 at 4:45 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Jul 10, 2020 at 03:23:25PM +1000, Oliver O'Halloran wrote:
> > This is largely prep work for supporting VFs in the 32bit MMIO window.
> > This is an unfortunate necessity due to how the Linux BAR allocator
> > handles BARs marked as non-prefetchable. The distinction
> > between prefetch and non-prefetchable BARs was made largely irrelevant
> > with the introduction of PCIe, but the BAR allocator is overly
> > conservative. It will always place non-pref bars in the prefetchable
> > window, which is 32bit only. This results in us being unable to use VFs
> > from NVMe drives and a few different RAID cards.
>
> How about fixing that in the core PCI code?
I've been kicking around the idea but I've never managed to convince
myself that ignoring the non-prefetchable bit is a safe thing to do in
generic code. Since Gen3 at least the PCIe Base spec has provided some
guidance about when you can put non-prefetchable BARs in the
prefetchable window and as of the Gen5 spec it lists these conditions:
> 1) The entire path from the host to the adapter is over PCI Express.
> 2) No conventional PCI or PCI-X devices do peer-to-peer reads to the range mapped by the BAR.
> 3) The PCI Express Host Bridge does no byte merging. (This is believed to be true on most platforms.)
> 4) Any locations with read side-effects are never the target of Memory Reads with the TH bit Set.
> 5) The range mapped by the BAR is never the target of a speculative Memory Read, either Host initiated or peer-to-peer.
1) Is easy enough to verify.
2) Is probably true, but who knows.
3) I know this is true for the platforms I'm looking at since the HW
designers assure me there is no merging happening at the host-bridge
level. Merging of MMIO ops does seem like an insane thing to do so
it's probably true in general too, but there's no real way to tell.
4) Is also *probably* true since the TH bit is only set when it's
explicitly enabled via the TLP Processing Hints extended capability in
config space. I guess it's possible firmware might enable that without
Linux realising, but in that case Linux is probably not doing BAR
allocation.
5) I have no idea about, but it seems difficult to make any kind of
general statement about.
I might just be being paranoid.
Oliver
^ permalink raw reply
* Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
From: Mathieu Desnoyers @ 2020-07-10 14:02 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Arnd Bergmann, Peter Zijlstra, x86, linux-kernel,
linux-mm, linuxppc-dev
In-Reply-To: <20200710015646.2020871-5-npiggin@gmail.com>
----- On Jul 9, 2020, at 9:56 PM, Nicholas Piggin npiggin@gmail.com wrote:
> And get rid of the generic sync_core_before_usermode facility.
>
> This helper is the wrong way around I think. The idea that membarrier
> state requires a core sync before returning to user is the easy one
> that does not need hiding behind membarrier calls. The gap in core
> synchronization due to x86's sysret/sysexit and lazy tlb mode, is the
> tricky detail that is better put in x86 lazy tlb code.
>
> Consider if an arch did not synchronize core in switch_mm either, then
> membarrier_mm_sync_core_before_usermode would be in the wrong place
> but arch specific mmu context functions would still be the right place.
> There is also a exit_lazy_tlb case that is not covered by this call, which
> could be a bugs (kthread use mm the membarrier process's mm then context
> switch back to the process without switching mm or lazy mm switch).
>
> This makes lazy tlb code a bit more modular.
I agree that moving this logic to exit_lazy_tlb is much more modular and
cleaner.
Thanks,
Mathieu
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> .../membarrier-sync-core/arch-support.txt | 6 +++-
> arch/x86/include/asm/mmu_context.h | 35 +++++++++++++++++++
> arch/x86/include/asm/sync_core.h | 28 ---------------
> include/linux/sched/mm.h | 14 --------
> include/linux/sync_core.h | 21 -----------
> kernel/cpu.c | 4 ++-
> kernel/kthread.c | 2 +-
> kernel/sched/core.c | 16 ++++-----
> 8 files changed, 51 insertions(+), 75 deletions(-)
> delete mode 100644 arch/x86/include/asm/sync_core.h
> delete mode 100644 include/linux/sync_core.h
>
> diff --git a/Documentation/features/sched/membarrier-sync-core/arch-support.txt
> b/Documentation/features/sched/membarrier-sync-core/arch-support.txt
> index 52ad74a25f54..bd43fb1f5986 100644
> --- a/Documentation/features/sched/membarrier-sync-core/arch-support.txt
> +++ b/Documentation/features/sched/membarrier-sync-core/arch-support.txt
> @@ -5,6 +5,10 @@
> #
> # Architecture requirements
> #
> +# If your architecture returns to user-space through non-core-serializing
> +# instructions, you need to ensure these are done in switch_mm and
> exit_lazy_tlb
> +# (if lazy tlb switching is implemented).
> +#
> # * arm/arm64/powerpc
> #
> # Rely on implicit context synchronization as a result of exception return
> @@ -24,7 +28,7 @@
> # instead on write_cr3() performed by switch_mm() to provide core serialization
> # after changing the current mm, and deal with the special case of kthread ->
> # uthread (temporarily keeping current mm into active_mm) by issuing a
> -# sync_core_before_usermode() in that specific case.
> +# serializing instruction in exit_lazy_mm() in that specific case.
> #
> -----------------------
> | arch |status|
> diff --git a/arch/x86/include/asm/mmu_context.h
> b/arch/x86/include/asm/mmu_context.h
> index 255750548433..5263863a9be8 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -6,6 +6,7 @@
> #include <linux/atomic.h>
> #include <linux/mm_types.h>
> #include <linux/pkeys.h>
> +#include <linux/sched/mm.h>
>
> #include <trace/events/tlb.h>
>
> @@ -95,6 +96,40 @@ static inline void switch_ldt(struct mm_struct *prev, struct
> mm_struct *next)
> #define enter_lazy_tlb enter_lazy_tlb
> extern void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk);
>
> +#ifdef CONFIG_MEMBARRIER
> +/*
> + * Ensure that a core serializing instruction is issued before returning
> + * to user-mode, if a SYNC_CORE was requested. x86 implements return to
> + * user-space through sysexit, sysrel, and sysretq, which are not core
> + * serializing.
> + *
> + * See the membarrier comment in finish_task_switch as to why this is done
> + * in exit_lazy_tlb.
> + */
> +#define exit_lazy_tlb exit_lazy_tlb
> +static inline void exit_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
> +{
> + /* Switching mm is serializing with write_cr3 */
> + if (tsk->mm != mm)
> + return;
> +
> + if (likely(!(atomic_read(&mm->membarrier_state) &
> + MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE)))
> + return;
> +
> + /* With PTI, we unconditionally serialize before running user code. */
> + if (static_cpu_has(X86_FEATURE_PTI))
> + return;
> + /*
> + * Return from interrupt and NMI is done through iret, which is core
> + * serializing.
> + */
> + if (in_irq() || in_nmi())
> + return;
> + sync_core();
> +}
> +#endif
> +
> /*
> * Init a new mm. Used on mm copies, like at fork()
> * and on mm's that are brand-new, like at execve().
> diff --git a/arch/x86/include/asm/sync_core.h b/arch/x86/include/asm/sync_core.h
> deleted file mode 100644
> index c67caafd3381..000000000000
> --- a/arch/x86/include/asm/sync_core.h
> +++ /dev/null
> @@ -1,28 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _ASM_X86_SYNC_CORE_H
> -#define _ASM_X86_SYNC_CORE_H
> -
> -#include <linux/preempt.h>
> -#include <asm/processor.h>
> -#include <asm/cpufeature.h>
> -
> -/*
> - * Ensure that a core serializing instruction is issued before returning
> - * to user-mode. x86 implements return to user-space through sysexit,
> - * sysrel, and sysretq, which are not core serializing.
> - */
> -static inline void sync_core_before_usermode(void)
> -{
> - /* With PTI, we unconditionally serialize before running user code. */
> - if (static_cpu_has(X86_FEATURE_PTI))
> - return;
> - /*
> - * Return from interrupt and NMI is done through iret, which is core
> - * serializing.
> - */
> - if (in_irq() || in_nmi())
> - return;
> - sync_core();
> -}
> -
> -#endif /* _ASM_X86_SYNC_CORE_H */
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 480a4d1b7dd8..9b026264b445 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -7,7 +7,6 @@
> #include <linux/sched.h>
> #include <linux/mm_types.h>
> #include <linux/gfp.h>
> -#include <linux/sync_core.h>
>
> /*
> * Routines for handling mm_structs
> @@ -364,16 +363,6 @@ enum {
> #include <asm/membarrier.h>
> #endif
>
> -static inline void membarrier_mm_sync_core_before_usermode(struct mm_struct
> *mm)
> -{
> - if (current->mm != mm)
> - return;
> - if (likely(!(atomic_read(&mm->membarrier_state) &
> - MEMBARRIER_STATE_PRIVATE_EXPEDITED_SYNC_CORE)))
> - return;
> - sync_core_before_usermode();
> -}
> -
> extern void membarrier_exec_mmap(struct mm_struct *mm);
>
> #else
> @@ -387,9 +376,6 @@ static inline void membarrier_arch_switch_mm(struct
> mm_struct *prev,
> static inline void membarrier_exec_mmap(struct mm_struct *mm)
> {
> }
> -static inline void membarrier_mm_sync_core_before_usermode(struct mm_struct
> *mm)
> -{
> -}
> #endif
>
> #endif /* _LINUX_SCHED_MM_H */
> diff --git a/include/linux/sync_core.h b/include/linux/sync_core.h
> deleted file mode 100644
> index 013da4b8b327..000000000000
> --- a/include/linux/sync_core.h
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef _LINUX_SYNC_CORE_H
> -#define _LINUX_SYNC_CORE_H
> -
> -#ifdef CONFIG_ARCH_HAS_SYNC_CORE_BEFORE_USERMODE
> -#include <asm/sync_core.h>
> -#else
> -/*
> - * This is a dummy sync_core_before_usermode() implementation that can be used
> - * on all architectures which return to user-space through core serializing
> - * instructions.
> - * If your architecture returns to user-space through non-core-serializing
> - * instructions, you need to write your own functions.
> - */
> -static inline void sync_core_before_usermode(void)
> -{
> -}
> -#endif
> -
> -#endif /* _LINUX_SYNC_CORE_H */
> -
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6ff2578ecf17..134688d79589 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -572,7 +572,9 @@ static int finish_cpu(unsigned int cpu)
>
> /*
> * idle_task_exit() will have switched to &init_mm, now
> - * clean up any remaining active_mm state.
> + * clean up any remaining active_mm state. exit_lazy_tlb
> + * is not done, if an arch did any accounting in these
> + * functions it would have to be added.
> */
> if (mm != &init_mm)
> idle->active_mm = &init_mm;
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index e813d92f2eab..6f93c649aa97 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -1251,9 +1251,9 @@ void kthread_use_mm(struct mm_struct *mm)
> finish_arch_post_lock_switch();
> #endif
>
> + exit_lazy_tlb(active_mm, tsk);
> if (active_mm != mm)
> mmdrop(active_mm);
> - exit_lazy_tlb(active_mm, tsk);
>
> to_kthread(tsk)->oldfs = get_fs();
> set_fs(USER_DS);
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index debc917bc69b..31e22c79826c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3294,22 +3294,19 @@ static struct rq *finish_task_switch(struct task_struct
> *prev)
> kcov_finish_switch(current);
>
> fire_sched_in_preempt_notifiers(current);
> +
> /*
> * When switching through a kernel thread, the loop in
> * membarrier_{private,global}_expedited() may have observed that
> * kernel thread and not issued an IPI. It is therefore possible to
> * schedule between user->kernel->user threads without passing though
> - * switch_mm(). Membarrier requires a barrier after storing to
> - * rq->curr, before returning to userspace, so provide them here:
> - *
> - * - a full memory barrier for {PRIVATE,GLOBAL}_EXPEDITED, implicitly
> - * provided by mmdrop(),
> - * - a sync_core for SYNC_CORE.
> + * switch_mm(). Membarrier requires a full barrier after storing to
> + * rq->curr, before returning to userspace, for
> + * {PRIVATE,GLOBAL}_EXPEDITED. This is implicitly provided by mmdrop().
> */
> - if (mm) {
> - membarrier_mm_sync_core_before_usermode(mm);
> + if (mm)
> mmdrop(mm);
> - }
> +
> if (unlikely(prev_state == TASK_DEAD)) {
> if (prev->sched_class->task_dead)
> prev->sched_class->task_dead(prev);
> @@ -6292,6 +6289,7 @@ void idle_task_exit(void)
> BUG_ON(current != this_rq()->idle);
>
> if (mm != &init_mm) {
> + /* enter_lazy_tlb is not done because we're about to go down */
> switch_mm(mm, &init_mm, current);
> finish_arch_post_lock_switch();
> }
> --
> 2.23.0
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH] powerpc/rtas: Restrict RTAS requests from userspace
From: Sasha Levin @ 2020-07-10 14:02 UTC (permalink / raw)
To: Sasha Levin, Andrew Donnellan, linuxppc-dev; +Cc: nathanl, leobras.c, stable
In-Reply-To: <20200702161932.18176-1-ajd@linux.ibm.com>
Hi
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.7.7, v5.4.50, v4.19.131, v4.14.187, v4.9.229, v4.4.229.
v5.7.7: Build OK!
v5.4.50: Failed to apply! Possible dependencies:
1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
4238fad366a66 ("powerpc/ima: Add support to initialize ima policy rules")
9155e2341aa8b ("powerpc/powernv: Add OPAL API interface to access secure variable")
bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
v4.19.131: Failed to apply! Possible dependencies:
1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
4238fad366a66 ("powerpc/ima: Add support to initialize ima policy rules")
6f5f193e84d3d ("powerpc/opal: add MPIPL interface definitions")
75d9fc7fd94eb ("powerpc/powernv: move OPAL call wrapper tracing and interrupt handling to C")
8139046a5a347 ("powerpc/powernv: Make possible for user to force a full ipl cec reboot")
88ec6b93c8e7d ("powerpc/xive: add OPAL extensions for the XIVE native exploitation support")
9155e2341aa8b ("powerpc/powernv: Add OPAL API interface to access secure variable")
bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
fb0b0a73b223f ("powerpc: Enable kcov")
v4.14.187: Failed to apply! Possible dependencies:
04baaf28f40c6 ("powerpc/powernv: Add support to enable sensor groups")
10dac04c79b18 ("mips: fix an off-by-one in dma_capable")
1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
32ce3862af3c4 ("powerpc/lib: Implement PMEM API")
5cdcb01e0af5a ("powernv: opal-sensor: Add support to read 64bit sensor values")
656ecc16e8fc2 ("crypto/nx: Initialize 842 high and normal RxFIFO control registers")
6f5f193e84d3d ("powerpc/opal: add MPIPL interface definitions")
74d656d219b98 ("powerpc/powernv: Add opal calls for opencapi")
77adbd2207e85 ("powerpc/powernv: Add OPAL_BUSY to opal_error_code()")
88ec6b93c8e7d ("powerpc/xive: add OPAL extensions for the XIVE native exploitation support")
8c4cdce8b1ab0 ("mtd: nand: qcom: add command elements in BAM transaction")
9155e2341aa8b ("powerpc/powernv: Add OPAL API interface to access secure variable")
92e3da3cf193f ("powerpc: initial pkey plumbing")
bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
d6a90bb83b508 ("powerpc/powernv: Enable tunneled operations")
e36d0a2ed5019 ("powerpc/powernv: Implement NMI IPI with OPAL_SIGNAL_SYSTEM_RESET")
ea8c64ace8664 ("dma-mapping: move swiotlb arch helpers to a new header")
fb0b0a73b223f ("powerpc: Enable kcov")
v4.9.229: Failed to apply! Possible dependencies:
1515ab9321562 ("powerpc/mm: Dump hash table")
1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
40565b5aedd6d ("sched/cputime, powerpc, s390: Make scaled cputime arch specific")
4ad8622dc5489 ("powerpc/8xx: Implement hw_breakpoint")
51c9c08439935 ("powerpc/kprobes: Implement Optprobes")
5b9ff02785986 ("powerpc: Build-time sort the exception table")
6533b7c16ee57 ("powerpc: Initial stack protector (-fstack-protector) support")
65c059bcaa731 ("powerpc: Enable support for GCC plugins")
8eb07b187000d ("powerpc/mm: Dump linux pagetables")
92e3da3cf193f ("powerpc: initial pkey plumbing")
981ee2d444408 ("sched/cputime, powerpc: Remove cputime_to_scaled()")
a7d2475af7aed ("powerpc: Sort the selects under CONFIG_PPC")
bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
d6c569b99558b ("powerpc/64: Move HAVE_CONTEXT_TRACKING from pseries to common Kconfig")
dd5ac03e09554 ("powerpc/mm: Fix page table dump build on non-Book3S")
ea8c64ace8664 ("dma-mapping: move swiotlb arch helpers to a new header")
ebfa50df435ee ("powerpc: Add helper to check if offset is within relative branch range")
fa769d3f58e6b ("powerpc/32: Enable HW_BREAKPOINT on BOOK3S")
fb0b0a73b223f ("powerpc: Enable kcov")
v4.4.229: Failed to apply! Possible dependencies:
019132ff3daf3 ("x86/mm/pkeys: Fill in pkey field in siginfo")
01c8f1c44b83a ("mm, dax, gpu: convert vm_insert_mixed to pfn_t")
0e749e54244ee ("dax: increase granularity of dax_clear_blocks() operations")
1874f6895c92d ("x86/mm/gup: Simplify get_user_pages() PTE bit handling")
1a8916ee3ac29 ("powerpc: Detect the secure boot mode of the system")
33a709b25a760 ("mm/gup, x86/mm/pkeys: Check VMAs and PTEs for protection keys")
34c0fd540e79f ("mm, dax, pmem: introduce pfn_t")
3565fce3a6597 ("mm, x86: get_user_pages() for dax mappings")
52db400fcd502 ("pmem, dax: clean up clear_pmem()")
7b2d0dbac4890 ("x86/mm/pkeys: Pass VMA down in to fault signal generation code")
8f62c883222c9 ("x86/mm/pkeys: Add arch-specific VMA protection bits")
92e3da3cf193f ("powerpc: initial pkey plumbing")
b2e0d1625e193 ("dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()")
bd5d9c743d38f ("powerpc: expose secure variables to userspace via sysfs")
f25748e3c34eb ("mm, dax: convert vmf_insert_pfn_pmd() to pfn_t")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
--
Thanks
Sasha
^ permalink raw reply
* Re: [PATCH 0/2] ASoC: fsl_spdif: Clear the validity bit for TX
From: Mark Brown @ 2020-07-10 15:39 UTC (permalink / raw)
To: timur, alsa-devel, perex, festevam, Shengjiu Wang, nicoleotsuka,
tiwai, Xiubo.Lee
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1594112066-31297-1-git-send-email-shengjiu.wang@nxp.com>
On Tue, 7 Jul 2020 16:54:24 +0800, Shengjiu Wang wrote:
> Clear the validity bit for TX
> Add kctl for configuring TX validity bit
>
> Shengjiu Wang (2):
> ASoC: fsl_spdif: Clear the validity bit for TX
> ASoC: fsl_spdif: Add kctl for configuring TX validity bit
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: fsl_spdif: Clear the validity bit for TX
commit: 055b082156704b85a059770359d6cdedfb24831d
[2/2] ASoC: fsl_spdif: Add kctl for configuring TX validity bit
commit: aa3fce5cd454db551a4ea1656bab9c2bacd2d1f4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply
* Re: [RFC PATCH 4/7] x86: use exit_lazy_tlb rather than membarrier_mm_sync_core_before_usermode
From: Andy Lutomirski @ 2020-07-10 17:04 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Arnd Bergmann, Peter Zijlstra, X86 ML, LKML, Linux-MM,
Mathieu Desnoyers, linuxppc-dev
In-Reply-To: <20200710015646.2020871-5-npiggin@gmail.com>
On Thu, Jul 9, 2020 at 6:57 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> And get rid of the generic sync_core_before_usermode facility.
>
> This helper is the wrong way around I think. The idea that membarrier
> state requires a core sync before returning to user is the easy one
> that does not need hiding behind membarrier calls. The gap in core
> synchronization due to x86's sysret/sysexit and lazy tlb mode, is the
> tricky detail that is better put in x86 lazy tlb code.
>
> Consider if an arch did not synchronize core in switch_mm either, then
> membarrier_mm_sync_core_before_usermode would be in the wrong place
> but arch specific mmu context functions would still be the right place.
> There is also a exit_lazy_tlb case that is not covered by this call, which
> could be a bugs (kthread use mm the membarrier process's mm then context
> switch back to the process without switching mm or lazy mm switch).
>
> This makes lazy tlb code a bit more modular.
The mm-switching and TLB-management has often had the regrettable
property that it gets wired up in a way that seems to work at the time
but doesn't have clear semantics, and I'm a bit concerned that this
patch is in that category. If I'm understanding right, you're trying
to enforce the property that exiting lazy TLB mode will promise to
sync the core eventually. But this has all kinds of odd properties:
- Why is exit_lazy_tlb() getting called at all in the relevant cases?
When is it permissible to call it? I look at your new code and see:
> +/*
> + * Ensure that a core serializing instruction is issued before returning
> + * to user-mode, if a SYNC_CORE was requested. x86 implements return to
> + * user-space through sysexit, sysrel, and sysretq, which are not core
> + * serializing.
> + *
> + * See the membarrier comment in finish_task_switch as to why this is done
> + * in exit_lazy_tlb.
> + */
> +#define exit_lazy_tlb exit_lazy_tlb
> +static inline void exit_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
> +{
> + /* Switching mm is serializing with write_cr3 */
> + if (tsk->mm != mm)
> + return;
And my brain says WTF? Surely you meant something like if
(WARN_ON_ONCE(tsk->mm != mm)) { /* egads, what even happened? how do
we try to recover well enough to get a crashed logged at least? */ }
So this needs actual documentation, preferably in comments near the
function, of what the preconditions are and what this mm parameter is.
Once that's done, then we could consider whether it's appropriate to
have this function promise to sync the core under some conditions.
- This whole structure seems to rely on the idea that switching mm
syncs something. I periodically ask chip vendor for non-serializing
mm switches. Specifically, in my dream world, we have totally
separate user and kernel page tables. Changing out the user tables
doesn't serialize or even create a fence. Instead it creates the
minimum required pipeline hazard such that user memory access and
switches to usermode will make sure they access memory through the
correct page tables. I haven't convinced a chip vendor yet, but there
are quite a few hundreds of cycles to be saved here. With this in
mind, I see the fencing aspects of the TLB handling code as somewhat
of an accident. I'm fine with documenting them and using them to
optimize other paths, but I think it should be explicit. For example:
/* Also does a full barrier? (Or a sync_core()-style barrier.)
However, if you rely on this, you must document it in a comment where
you call this function. *?
void switch_mm_irqs_off()
{
}
This is kind of like how we strongly encourage anyone using smp_?mb()
to document what they are fencing against.
Also, as it stands, I can easily see in_irq() ceasing to promise to
serialize. There are older kernels for which it does not promise to
serialize. And I have plans to make it stop serializing in the
nearish future.
--Andy
^ permalink raw reply
* Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
From: Mimi Zohar @ 2020-07-10 17:23 UTC (permalink / raw)
To: Bruno Meneguele, linux-kernel, x86, linuxppc-dev, linux-s390,
linux-integrity
Cc: erichte, nayna, stable
In-Reply-To: <20200709164647.45153-1-bmeneg@redhat.com>
On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in compile
> time, enforcing the appraisal whenever the kernel had the arch policy option
> enabled.
> However it breaks systems where the option is set but the system didn't
> boot in a "secure boot" platform. In this scenario, anytime an appraisal
> policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, without
> giving the user the opportunity to label the filesystem, before enforcing
> integrity.
>
> Considering the ARCH_POLICY is only effective when secure boot is actually
> enabled this patch remove the compile time dependency and move it to a
> runtime decision, based on the secure boot state of that platform.
Perhaps we could simplify this patch description a bit?
The IMA_APPRAISE_BOOTPARAM config allows enabling different
"ima_appraise=" modes - log, fix, enforce - at run time, but not when
IMA architecture specific policies are enabled. This prevents
properly labeling the filesystem on systems where secure boot is
supported, but not enabled on the platform. Only when secure boot is
enabled, should these IMA appraise modes be disabled.
This patch removes the compile time dependency and makes it a runtime
decision, based on the secure boot state of that platform.
<snip>
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index a9649b04b9f1..884de471b38a 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -19,6 +19,11 @@
> static int __init default_appraise_setup(c
> har *str)
> {
> #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> + if (arch_ima_get_secureboot()) {
> + pr_info("appraise boot param ignored: secure boot enabled");
Instead of a generic statement, is it possible to include the actual
option being denied? Perhaps something like: "Secure boot enabled,
ignoring %s boot command line option"
Mimi
> + return 1;
> + }
> +
> if (strncmp(str, "off", 3) == 0)
> ima_appraise = 0;
> else if (strncmp(str, "log", 3) == 0)
^ permalink raw reply
* Re: [PATCH] powerpc/numa: Restrict possible nodes based on platform
From: Nathan Lynch @ 2020-07-10 17:41 UTC (permalink / raw)
To: Srikar Dronamraju; +Cc: linuxppc-dev, Bharata B Rao
In-Reply-To: <20200707084203.GC874@linux.vnet.ibm.com>
Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
> * Michael Ellerman <mpe@ellerman.id.au> [2020-07-07 15:02:17]:
>
>> Srikar Dronamraju <srikar@linux.vnet.ibm.com> writes:
>> > $ lsprop /proc/device-tree/rtas/ibm,*associ*-domains
>> > /proc/device-tree/rtas/ibm,current-associativity-domains
>> > 00000005 00000001 00000002 00000002 00000002 00000010
>> > /proc/device-tree/rtas/ibm,max-associativity-domains
>> > 00000005 00000001 00000008 00000020 00000020 00000100
>> >
>> > $ cat /sys/devices/system/node/possible ##Before patch
>> > 0-31
>> >
>> > $ cat /sys/devices/system/node/possible ##After patch
>> > 0-1
>> >
>> > Note the maximum nodes this platform can support is only 2 but the
>> > possible nodes is set to 32.
>>
>> But what about LPM to a system with more nodes?
>>
>
> I have very less info on LPM, so I checked with Nathan Lynch before posting
> and as per Nathan in the current design of LPM, Linux wouldn't use the new
> node numbers.
(I see a v2 has been posted already but I needed a little time to check
with our hypervisor people on this point.)
It's less of a design and more of a least-bad option in the absence of a
more flexible NUMA architecture in Linux.
For now, the "rule" with LPM and NUMA has to be that Linux uses the NUMA
information from the device tree that it was booted with, and it must
disregard ibm,associativity and similar information after LPM or certain
other platform events. Historically there has been code that tried to
honor changes in NUMA information but it caused much worse problems than
degraded performance. That code has been disabled by default since last
year and is now subject to removal:
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=182897
Most NUMA-aware code happens to follow that rule because the device tree
associativity information tends to get cached on first access in Linux's
own data structures. It all feels a little fragile to me though,
especially since we can DLPAR add processors and memory after LPM with
"new" associativity properties which don't relate to the logical
topology Linux has already built. However, on currently available hw, as
long as we're using ibm,max-associativity-domains to limit the set of
possible nodes, I believe such resources will always receive valid (but
possibly suboptimal) NUMA assignments. That's because as of this
writing ibm,max-associativity-domains has the same contents on all
currently available PowerVM systems.
Now if we change to using ibm,current-associativity-domains, which we
*can* expect to differ between differently configured systems, post-LPM
DLPAR additions can yield resources with node assignments that
fall outside of the possible range, especially when we've migrated from
a smaller system to a larger one.
Is the current code robust against that possibility? I don't think so:
it looks like of_node_to_nid_single(), of_drconf_to_nid_single() and
possibly more code need to guard against this in order to prevent
NODE_DATA() null dereferences and the like. Probably those functions
should be made to clamp the nid assignment at num_possible_nodes()
instead of MAX_NUMNODES, which strikes me as more correct regardless of
your patch.
^ permalink raw reply
* Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
From: Bruno Meneguele @ 2020-07-10 18:03 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-s390, nayna, erichte, x86, linux-kernel, stable,
linux-integrity, linuxppc-dev
In-Reply-To: <1594401804.14405.8.camel@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]
On Fri, Jul 10, 2020 at 01:23:24PM -0400, Mimi Zohar wrote:
> On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> > APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in compile
> > time, enforcing the appraisal whenever the kernel had the arch policy option
> > enabled.
>
> > However it breaks systems where the option is set but the system didn't
> > boot in a "secure boot" platform. In this scenario, anytime an appraisal
> > policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, without
> > giving the user the opportunity to label the filesystem, before enforcing
> > integrity.
> >
> > Considering the ARCH_POLICY is only effective when secure boot is actually
> > enabled this patch remove the compile time dependency and move it to a
> > runtime decision, based on the secure boot state of that platform.
>
> Perhaps we could simplify this patch description a bit?
>
> The IMA_APPRAISE_BOOTPARAM config allows enabling different
> "ima_appraise=" modes - log, fix, enforce - at run time, but not when
> IMA architecture specific policies are enabled. This prevents
> properly labeling the filesystem on systems where secure boot is
> supported, but not enabled on the platform. Only when secure boot is
> enabled, should these IMA appraise modes be disabled.
>
> This patch removes the compile time dependency and makes it a runtime
> decision, based on the secure boot state of that platform.
>
Sounds good to me.
> <snip>
>
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index a9649b04b9f1..884de471b38a 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -19,6 +19,11 @@
> > static int __init default_appraise_setup(c
>
> > har *str)
> > {
> > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > + if (arch_ima_get_secureboot()) {
> > + pr_info("appraise boot param ignored: secure boot enabled");
>
> Instead of a generic statement, is it possible to include the actual
> option being denied? Perhaps something like: "Secure boot enabled,
> ignoring %s boot command line option"
>
> Mimi
>
Yes, sure.
Thanks!
> > + return 1;
> > + }
> > +
> > if (strncmp(str, "off", 3) == 0)
> > ima_appraise = 0;
> > else if (strncmp(str, "log", 3) == 0)
>
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
From: Bruno Meneguele @ 2020-07-10 18:34 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-s390, nayna, erichte, x86, linux-kernel, stable,
linux-integrity, linuxppc-dev
In-Reply-To: <20200710180338.GA10547@glitch>
[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]
On Fri, Jul 10, 2020 at 03:03:38PM -0300, Bruno Meneguele wrote:
> On Fri, Jul 10, 2020 at 01:23:24PM -0400, Mimi Zohar wrote:
> > On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> > > APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in compile
> > > time, enforcing the appraisal whenever the kernel had the arch policy option
> > > enabled.
> >
> > > However it breaks systems where the option is set but the system didn't
> > > boot in a "secure boot" platform. In this scenario, anytime an appraisal
> > > policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, without
> > > giving the user the opportunity to label the filesystem, before enforcing
> > > integrity.
> > >
> > > Considering the ARCH_POLICY is only effective when secure boot is actually
> > > enabled this patch remove the compile time dependency and move it to a
> > > runtime decision, based on the secure boot state of that platform.
> >
> > Perhaps we could simplify this patch description a bit?
> >
> > The IMA_APPRAISE_BOOTPARAM config allows enabling different
> > "ima_appraise=" modes - log, fix, enforce - at run time, but not when
> > IMA architecture specific policies are enabled. This prevents
> > properly labeling the filesystem on systems where secure boot is
> > supported, but not enabled on the platform. Only when secure boot is
> > enabled, should these IMA appraise modes be disabled.
> >
> > This patch removes the compile time dependency and makes it a runtime
> > decision, based on the secure boot state of that platform.
> >
>
> Sounds good to me.
>
> > <snip>
> >
> > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > > index a9649b04b9f1..884de471b38a 100644
> > > --- a/security/integrity/ima/ima_appraise.c
> > > +++ b/security/integrity/ima/ima_appraise.c
> > > @@ -19,6 +19,11 @@
> > > static int __init default_appraise_setup(c
> >
> > > har *str)
> > > {
> > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > > + if (arch_ima_get_secureboot()) {
> > > + pr_info("appraise boot param ignored: secure boot enabled");
> >
> > Instead of a generic statement, is it possible to include the actual
> > option being denied? Perhaps something like: "Secure boot enabled,
> > ignoring %s boot command line option"
> >
> > Mimi
> >
>
> Yes, sure.
>
Btw, would it make sense to first make sure we have a valid "str"
option and not something random to print?
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a9649b04b9f1..1f1175531d3e 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -25,6 +25,16 @@ static int __init default_appraise_setup(char *str)
ima_appraise = IMA_APPRAISE_LOG;
else if (strncmp(str, "fix", 3) == 0)
ima_appraise = IMA_APPRAISE_FIX;
+ else
+ pr_info("invalid \"%s\" appraise option");
+
+ if (arch_ima_get_secureboot()) {
+ if (!is_ima_appraise_enabled()) {
+ pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option",
+ str);
+ ima_appraise = IMA_APPRAISE_ENFORCE;
+ }
+ }
#endif
return 1;
}
The "else" there I think would make sense as well, at least to give the
user some feedback about a possible mispelling of him (as a separate
patch).
And "if(!is_ima_appraise_enabled())" would avoid to print anything about
"ignoring the option" to the user in case he explicitly set "enforce",
which we know there isn't any real effect but is allowed and shown in
kernel-parameters.txt.
> Thanks!
>
> > > + return 1;
> > > + }
> > > +
> > > if (strncmp(str, "off", 3) == 0)
> > > ima_appraise = 0;
> > > else if (strncmp(str, "log", 3) == 0)
> >
>
> --
> bmeneg
> PGP Key: http://bmeneg.com/pubkey.txt
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related
* Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
From: Mimi Zohar @ 2020-07-10 18:54 UTC (permalink / raw)
To: Bruno Meneguele
Cc: linux-s390, nayna, erichte, x86, linux-kernel, stable,
linux-integrity, linuxppc-dev
In-Reply-To: <20200710183420.GB10547@glitch>
On Fri, 2020-07-10 at 15:34 -0300, Bruno Meneguele wrote:
> On Fri, Jul 10, 2020 at 03:03:38PM -0300, Bruno Meneguele wrote:
> > On Fri, Jul 10, 2020 at 01:23:24PM -0400, Mimi Zohar wrote:
> > > On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> > > > APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in compile
> > > > time, enforcing the appraisal whenever the kernel had the arch policy option
> > > > enabled.
> > >
> > > > However it breaks systems where the option is set but the system didn't
> > > > boot in a "secure boot" platform. In this scenario, anytime an appraisal
> > > > policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, without
> > > > giving the user the opportunity to label the filesystem, before enforcing
> > > > integrity.
> > > >
> > > > Considering the ARCH_POLICY is only effective when secure boot is actually
> > > > enabled this patch remove the compile time dependency and move it to a
> > > > runtime decision, based on the secure boot state of that platform.
> > >
> > > Perhaps we could simplify this patch description a bit?
> > >
> > > The IMA_APPRAISE_BOOTPARAM config allows enabling different
> > > "ima_appraise=" modes - log, fix, enforce - at run time, but not when
> > > IMA architecture specific policies are enabled. This prevents
> > > properly labeling the filesystem on systems where secure boot is
> > > supported, but not enabled on the platform. Only when secure boot is
> > > enabled, should these IMA appraise modes be disabled.
> > >
> > > This patch removes the compile time dependency and makes it a runtime
> > > decision, based on the secure boot state of that platform.
> > >
> >
> > Sounds good to me.
> >
> > > <snip>
> > >
> > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > > > index a9649b04b9f1..884de471b38a 100644
> > > > --- a/security/integrity/ima/ima_appraise.c
> > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > @@ -19,6 +19,11 @@
> > > > static int __init default_appraise_setup(c
> > >
> > > > har *str)
> > > > {
> > > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > > > + if (arch_ima_get_secureboot()) {
> > > > + pr_info("appraise boot param ignored: secure boot enabled");
> > >
> > > Instead of a generic statement, is it possible to include the actual
> > > option being denied? Perhaps something like: "Secure boot enabled,
> > > ignoring %s boot command line option"
> > >
> > > Mimi
> > >
> >
> > Yes, sure.
> >
>
> Btw, would it make sense to first make sure we have a valid "str"
> option and not something random to print?
>
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index a9649b04b9f1..1f1175531d3e 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -25,6 +25,16 @@ static int __init default_appraise_setup(char *str)
> ima_appraise = IMA_APPRAISE_LOG;
> else if (strncmp(str, "fix", 3) == 0)
> ima_appraise = IMA_APPRAISE_FIX;
> + else
> + pr_info("invalid \"%s\" appraise option");
> +
> + if (arch_ima_get_secureboot()) {
> + if (!is_ima_appraise_enabled()) {
> + pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option",
> + str);
> + ima_appraise = IMA_APPRAISE_ENFORCE;
> + }
> + }
Providing feedback is probably a good idea. However, the
"arch_ima_get_secureboot" test can't come after setting
"ima_appraise."
Mimi
> #endif
> return 1;
> }
>
>
> The "else" there I think would make sense as well, at least to give the
> user some feedback about a possible mispelling of him (as a separate
> patch).
>
> And "if(!is_ima_appraise_enabled())" would avoid to print anything about
> "ignoring the option" to the user in case he explicitly set "enforce",
> which we know there isn't any real effect but is allowed and shown in
> kernel-parameters.txt.
>
> > Thanks!
> >
> > > > + return 1;
> > > > + }
> > > > +
> > > > if (strncmp(str, "off", 3) == 0)
> > > > ima_appraise = 0;
> > > > else if (strncmp(str, "log", 3) == 0)
> > >
> >
> > --
> > bmeneg
> > PGP Key: http://bmeneg.com/pubkey.txt
>
>
>
^ permalink raw reply
* Re: [PATCH v5] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
From: Bruno Meneguele @ 2020-07-10 19:25 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-s390, nayna, erichte, x86, linux-kernel, stable,
linux-integrity, linuxppc-dev
In-Reply-To: <1594407288.14405.36.camel@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]
On Fri, Jul 10, 2020 at 02:54:48PM -0400, Mimi Zohar wrote:
> On Fri, 2020-07-10 at 15:34 -0300, Bruno Meneguele wrote:
> > On Fri, Jul 10, 2020 at 03:03:38PM -0300, Bruno Meneguele wrote:
> > > On Fri, Jul 10, 2020 at 01:23:24PM -0400, Mimi Zohar wrote:
> > > > On Thu, 2020-07-09 at 13:46 -0300, Bruno Meneguele wrote:
> > > > > APPRAISE_BOOTPARAM has been marked as dependent on !ARCH_POLICY in compile
> > > > > time, enforcing the appraisal whenever the kernel had the arch policy option
> > > > > enabled.
> > > >
> > > > > However it breaks systems where the option is set but the system didn't
> > > > > boot in a "secure boot" platform. In this scenario, anytime an appraisal
> > > > > policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, without
> > > > > giving the user the opportunity to label the filesystem, before enforcing
> > > > > integrity.
> > > > >
> > > > > Considering the ARCH_POLICY is only effective when secure boot is actually
> > > > > enabled this patch remove the compile time dependency and move it to a
> > > > > runtime decision, based on the secure boot state of that platform.
> > > >
> > > > Perhaps we could simplify this patch description a bit?
> > > >
> > > > The IMA_APPRAISE_BOOTPARAM config allows enabling different
> > > > "ima_appraise=" modes - log, fix, enforce - at run time, but not when
> > > > IMA architecture specific policies are enabled. This prevents
> > > > properly labeling the filesystem on systems where secure boot is
> > > > supported, but not enabled on the platform. Only when secure boot is
> > > > enabled, should these IMA appraise modes be disabled.
> > > >
> > > > This patch removes the compile time dependency and makes it a runtime
> > > > decision, based on the secure boot state of that platform.
> > > >
> > >
> > > Sounds good to me.
> > >
> > > > <snip>
> > > >
> > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > > > > index a9649b04b9f1..884de471b38a 100644
> > > > > --- a/security/integrity/ima/ima_appraise.c
> > > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > > @@ -19,6 +19,11 @@
> > > > > static int __init default_appraise_setup(c
> > > >
> > > > > har *str)
> > > > > {
> > > > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
> > > > > + if (arch_ima_get_secureboot()) {
> > > > > + pr_info("appraise boot param ignored: secure boot enabled");
> > > >
> > > > Instead of a generic statement, is it possible to include the actual
> > > > option being denied? Perhaps something like: "Secure boot enabled,
> > > > ignoring %s boot command line option"
> > > >
> > > > Mimi
> > > >
> > >
> > > Yes, sure.
> > >
> >
> > Btw, would it make sense to first make sure we have a valid "str"
> > option and not something random to print?
> >
> > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > index a9649b04b9f1..1f1175531d3e 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -25,6 +25,16 @@ static int __init default_appraise_setup(char *str)
> > ima_appraise = IMA_APPRAISE_LOG;
> > else if (strncmp(str, "fix", 3) == 0)
> > ima_appraise = IMA_APPRAISE_FIX;
> > + else
> > + pr_info("invalid \"%s\" appraise option");
> > +
> > + if (arch_ima_get_secureboot()) {
> > + if (!is_ima_appraise_enabled()) {
> > + pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option",
> > + str);
> > + ima_appraise = IMA_APPRAISE_ENFORCE;
> > + }
> > + }
>
> Providing feedback is probably a good idea. However, the
> "arch_ima_get_secureboot" test can't come after setting
> "ima_appraise."
>
Sorry, but I'm not sure if I got the reason to why it can't be done
after: would it be basically to prevent any further processing about
ima_appraise as a matter of security principle? Or maybe to keep the
dependency between secureboot and bootparam truly strict?
Or are there something else I'm missing?
> Mimi
>
> > #endif
> > return 1;
> > }
> >
> >
> > The "else" there I think would make sense as well, at least to give the
> > user some feedback about a possible mispelling of him (as a separate
> > patch).
> >
> > And "if(!is_ima_appraise_enabled())" would avoid to print anything about
> > "ignoring the option" to the user in case he explicitly set "enforce",
> > which we know there isn't any real effect but is allowed and shown in
> > kernel-parameters.txt.
> >
> > > Thanks!
> > >
> > > > > + return 1;
> > > > > + }
> > > > > +
> > > > > if (strncmp(str, "off", 3) == 0)
> > > > > ima_appraise = 0;
> > > > > else if (strncmp(str, "log", 3) == 0)
> > > >
> > >
> > > --
> > > bmeneg
> > > PGP Key: http://bmeneg.com/pubkey.txt
> >
> >
> >
>
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH 0/14 v3] PCI: Remove '*val = 0' from pcie_capability_read_*()
From: Saheed Olayemi Bolarinwa @ 2020-07-10 21:20 UTC (permalink / raw)
To: helgaas
Cc: linux-wireless, linux-pci, QCA ath9k Development, netdev,
Oliver O'Halloran, Stanislaw Gruszka, linux-acpi, linux-rdma,
Jason Gunthorpe, Doug Ledford, Jakub Kicinski,
linux-kernel-mentees, Len Brown, Arnd Bergmann, skhan, bjorn,
Kalle Valo, Mike Marciniszyn, Sam Bobroff, Greg Kroah-Hartman,
Dennis Dalessandro, Rafael J. Wysocki, linux-kernel, Lukas Wunner,
Bolarinwa Olayemi Saheed, linuxppc-dev, David S. Miller
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
v3 CHANGES:
- Split previous PATCH 6/13 into two : PATCH 6/14 and PATCH 7/14
- Fix commit message of PATCH 5/14
- Update Patch numbering and Commit messages
- Add 'Acked by Greg KH' to PATCH 2/14
- Add PATCH version
v2 CHANGES:
- Fix missing comma, causing the email cc error
- Fix typos and numbering errors in commit messages
- Add commit message to 13/13
- Add two more patches: PATCH 3/13 and PATCH 4/13
MERGING:
Patch 7/14 depends on Patch 6/14. However Patch 6/14 has no dependency.
Please, merge PATCH 7/14 only after Patch 6/14.
Patch 14/14 depend on all preceeding patchs. Except for Patch 6/14 and
Patch 7/14, all other patches are independent of one another. Hence,
please merge Patch 14/14 only after other patches in this series have
been merged.
PATCH 6/14:
Make the function set status to "Power On" by default and only set to
Set "Power Off" only if pcie_capability_read_word() is successful and
(slot_ctrl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF.
PATCH 1/14 to 13/14:
Check the return value of pcie_capability_read_*() to ensure success or
confirm failure. While maintaining these functions, this ensures that the
changes in PATCH 14/14 does not introduce any bug.
PATCH 14/14:
There are several reasons why a PCI capability read may fail whether the
device is present or not. If this happens, pcie_capability_read_*() will
return -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER or PCIBIOS_DEVICE_NOT_FOUND
and *val is set to 0.
This behaviour if further ensured by this code inside
pcie_capability_read_*()
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
/*
* Reset *val to 0 if pci_read_config_dword() fails, it may
* have been written as 0xFFFFFFFF if hardware error happens
* during pci_read_config_dword().
*/
if (ret)
*val = 0;
return ret;
a) Since all pci_generic_config_read() does is read a register value,
it may return success after reading a ~0 which *may* have been fabricated
by the PCI host bridge due to a read timeout. Hence pci_read_config_*()
will return success with a fabricated ~0 in *val, indicating a problem.
In this case, the assumed behaviour of pcie_capability_read_*() will be
wrong. To avoid error slipping through, more checks are necessary.
b) pci_read_config_*() will return PCIBIOS_DEVICE_NOT_FOUND only if
dev->error_state = pci_channel_io_perm_failure (i.e.
pci_dev_is_disconnected()) or if pci_generic_config_read() can't find the
device. In both cases *val is initially set to ~0 but as shown in the code
above pcie_capability_read_*() resets it back to 0. Even with this effort,
drivers still have to perform validation checks more so if 0 is a valid
value.
Most drivers only consider the case (b) and in some cases, there is the
expectation that on timeout *val has a fabricated value of ~0, which *may*
not always be true as explained in (a).
In any case, checks need to be done to validate the value read and maybe
confirm which error has occurred. It is better left to the drivers to do.
Check the return value of pcie_capability_read_dword() to ensure success
and avoid bug as a result of Patch 14/14.
Remove the reset of *val to 0 when pci_read_config_*() fails.
Bolarinwa Olayemi Saheed (14):
IB/hfi1: Check the return value of pcie_capability_read_*()
misc: rtsx: Check the return value of pcie_capability_read_*()
ath9k: Check the return value of pcie_capability_read_*()
iwlegacy: Check the return value of pcie_capability_read_*()
PCI: pciehp: Check the return value of pcie_capability_read_*()
PCI: pciehp: Make "Power On" the default
PCI: pciehp: Check the return value of pcie_capability_read_*()
PCI/ACPI: Check the return value of pcie_capability_read_*()
PCI: pciehp: Check the return value of pcie_capability_read_*()
PCI: Check the return value of pcie_capability_read_*()
PCI/PM: Check return value of pcie_capability_read_*()
PCI/AER: Check the return value of pcie_capability_read_*()
PCI/ASPM: Check the return value of pcie_capability_read_*()
PCI: Remove '*val = 0' from pcie_capability_read_*()
drivers/net/wireless/ath/ath9k/pci.c | 5 +++--
drivers/net/wireless/intel/iwlegacy/common.c | 4 ++--
drivers/infiniband/hw/hfi1/aspm.c | 7 ++++---
drivers/misc/cardreader/rts5227.c | 5 +++--
drivers/misc/cardreader/rts5249.c | 5 +++--
drivers/misc/cardreader/rts5260.c | 5 +++--
drivers/misc/cardreader/rts5261.c | 5 +++--
drivers/pci/pcie/aer.c | 5 +++--
drivers/pci/pcie/aspm.c | 33 +++++++++++++++++----------------
drivers/pci/hotplug/pciehp_hpc.c | 47 ++++++++++++++++----------------
drivers/pci/pci-acpi.c | 10 ++++---
drivers/pci/probe.c | 29 ++++++++++++--------
drivers/pci/access.c | 14 --------------
13 files changed, 87 insertions(+), 87 deletions(-)
--
2.18.2
^ permalink raw reply
* [PATCH 14/14 v3] PCI: Remove '*val = 0' from pcie_capability_read_*()
From: Saheed Olayemi Bolarinwa @ 2020-07-10 21:20 UTC (permalink / raw)
To: helgaas
Cc: linux-wireless, linux-pci, QCA ath9k Development, netdev,
Oliver O'Halloran, Stanislaw Gruszka, linux-acpi, linux-rdma,
Jason Gunthorpe, Doug Ledford, Jakub Kicinski,
linux-kernel-mentees, Len Brown, Arnd Bergmann, skhan, bjorn,
Kalle Valo, Mike Marciniszyn, Sam Bobroff, Greg Kroah-Hartman,
Dennis Dalessandro, Rafael J. Wysocki, linux-kernel, Lukas Wunner,
Bolarinwa Olayemi Saheed, linuxppc-dev, David S. Miller
In-Reply-To: <20200710212026.27136-1-refactormyself@gmail.com>
From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
There are several reasons why a PCI capability read may fail whether the
device is present or not. If this happens, pcie_capability_read_*() will
return -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER or PCIBIOS_DEVICE_NOT_FOUND
and *val is set to 0.
This behaviour if further ensured by this code inside
pcie_capability_read_*()
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
/*
* Reset *val to 0 if pci_read_config_dword() fails, it may
* have been written as 0xFFFFFFFF if hardware error happens
* during pci_read_config_dword().
*/
if (ret)
*val = 0;
return ret;
a) Since all pci_generic_config_read() does is read a register value,
it may return success after reading a ~0 which *may* have been fabricated
by the PCI host bridge due to a read timeout. Hence pci_read_config_*()
will return success with a fabricated ~0 in *val, indicating a problem.
In this case, the assumed behaviour of pcie_capability_read_*() will be
wrong. To avoid error slipping through, more checks are necessary.
b) pci_read_config_*() will return PCIBIOS_DEVICE_NOT_FOUND only if
dev->error_state = pci_channel_io_perm_failure (i.e.
pci_dev_is_disconnected()) or if pci_generic_config_read() can't find the
device. In both cases *val is initially set to ~0 but as shown in the code
above pcie_capability_read_*() resets it back to 0. Even with this effort,
drivers still have to perform validation checks more so if 0 is a valid
value.
Most drivers only consider the case (b) and in some cases, there is the
expectation that on timeout *val has a fabricated value of ~0, which *may*
not always be true as explained in (a).
In any case, checks need to be done to validate the value read and maybe
confirm which error has occurred. It is better left to the drivers to do.
Remove the reset of *val to 0 when pci_read_config_*() fails.
Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
---
This patch depends on all of the preceeding patches in this series,
otherwise it will introduce bugs as pointed out in the commit message
of each.
drivers/pci/access.c | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 79c4a2ef269a..ec95edbb1ac8 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -413,13 +413,6 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
if (pcie_capability_reg_implemented(dev, pos)) {
ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val);
- /*
- * Reset *val to 0 if pci_read_config_word() fails, it may
- * have been written as 0xFFFF if hardware error happens
- * during pci_read_config_word().
- */
- if (ret)
- *val = 0;
return ret;
}
@@ -448,13 +441,6 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
if (pcie_capability_reg_implemented(dev, pos)) {
ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
- /*
- * Reset *val to 0 if pci_read_config_dword() fails, it may
- * have been written as 0xFFFFFFFF if hardware error happens
- * during pci_read_config_dword().
- */
- if (ret)
- *val = 0;
return ret;
}
--
2.18.2
^ permalink raw reply related
* Re: generic DMA bypass flag v4
From: Jesper Dangaard Brouer @ 2020-07-10 13:26 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Daniel Borkmann, Alexey Kardashevskiy, Greg Kroah-Hartman,
Joerg Roedel, brouer, Robin Murphy, linux-kernel, iommu,
Björn Töpel, linuxppc-dev, Lu Baolu
In-Reply-To: <20200708152449.316476-1-hch@lst.de>
On Wed, 8 Jul 2020 17:24:44 +0200
Christoph Hellwig <hch@lst.de> wrote:
> Note that as-is this breaks the XSK buffer pool, which unfortunately
> poked directly into DMA internals. A fix for that is already queued
> up in the netdev tree.
>
> Jesper and XDP gang: this should not regress any performance as
> the dma-direct calls are now inlined into the out of line DMA mapping
> calls. But if you can verify the performance numbers that would be
> greatly appreciated.
From a superficial review of the patches, they look okay to me. I don't
have time to run a performance benchmark (before I go on vacation).
I hoped Björn could test/benchmark this(?), given (as mentioned) this
also affect XSK / AF_XDP performance.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH V2 1/2] powerpc/vas: Report proper error code for address translation failure
From: Haren Myneni @ 2020-07-10 23:47 UTC (permalink / raw)
To: mpe; +Cc: tulioqm, abali, linuxppc-dev, rzinsly
P9 DD2 NX workbook (Table 4-36) says DMA controller uses CC=5
internally for translation fault handling. NX reserves CC=250 for
OS to notify user space when NX encounters address translation
failure on the request buffer. Not an issue in earlier releases
as NX does not get faults on kernel addresses.
This patch defines CSB_CC_FAULT_ADDRESS(250) and updates CSB.CC with
this proper error code for user space.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
Changes in V2:
- Use CSB_CC_FAULT_ADDRESS instead of CSB_CC_ADDRESS_TRANSLATION
to distinguish from other error codes.
- Add NX workbook reference in the comment.
---
Documentation/powerpc/vas-api.rst | 2 +-
arch/powerpc/include/asm/icswx.h | 4 ++++
arch/powerpc/platforms/powernv/vas-fault.c | 2 +-
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/Documentation/powerpc/vas-api.rst b/Documentation/powerpc/vas-api.rst
index 1217c2f..788dc83 100644
--- a/Documentation/powerpc/vas-api.rst
+++ b/Documentation/powerpc/vas-api.rst
@@ -213,7 +213,7 @@ request buffers are not in memory. The operating system handles the fault by
updating CSB with the following data:
csb.flags = CSB_V;
- csb.cc = CSB_CC_TRANSLATION;
+ csb.cc = CSB_CC_FAULT_ADDRESS;
csb.ce = CSB_CE_TERMINATION;
csb.address = fault_address;
diff --git a/arch/powerpc/include/asm/icswx.h b/arch/powerpc/include/asm/icswx.h
index 965b1f3..9bc7c58 100644
--- a/arch/powerpc/include/asm/icswx.h
+++ b/arch/powerpc/include/asm/icswx.h
@@ -77,6 +77,10 @@ struct coprocessor_completion_block {
#define CSB_CC_CHAIN (37)
#define CSB_CC_SEQUENCE (38)
#define CSB_CC_HW (39)
+/*
+ * P9 DD NX Workbook 3.2 (Table 4-36): Address translation fault
+ */
+#define CSB_CC_FAULT_ADDRESS (250)
#define CSB_SIZE (0x10)
#define CSB_ALIGN CSB_SIZE
diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
index 266a6ca..3d21fce 100644
--- a/arch/powerpc/platforms/powernv/vas-fault.c
+++ b/arch/powerpc/platforms/powernv/vas-fault.c
@@ -79,7 +79,7 @@ static void update_csb(struct vas_window *window,
csb_addr = (void __user *)be64_to_cpu(crb->csb_addr);
memset(&csb, 0, sizeof(csb));
- csb.cc = CSB_CC_TRANSLATION;
+ csb.cc = CSB_CC_FAULT_ADDRESS;
csb.ce = CSB_CE_TERMINATION;
csb.cs = 0;
csb.count = 0;
--
1.8.3.1
^ permalink raw reply related
* [PATCH V2 2/2] selftests/powerpc: Use proper error code to check fault address
From: Haren Myneni @ 2020-07-10 23:49 UTC (permalink / raw)
To: mpe; +Cc: tulioqm, linuxppc-dev, abali, rzinsly
In-Reply-To: <019fd53e7538c6f8f332d175df74b1815ef5aa8c.camel@linux.ibm.com>
ERR_NX_TRANSLATION(CSB.CC=5) is for internal to VAS for fault handling
and should not used by OS. ERR_NX_AT_FAULT(CSB.CC=250) is the proper
error code should be reported by OS when NX encounters address
translation failure.
This patch uses CC=250 to determine the fault address when the request
is not successful.
Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
tools/testing/selftests/powerpc/nx-gzip/gunz_test.c | 4 ++--
tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c b/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
index 6ee0fde..7c23d3d 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gunz_test.c
@@ -698,13 +698,13 @@ int decompress_file(int argc, char **argv, void *devhandle)
switch (cc) {
- case ERR_NX_TRANSLATION:
+ case ERR_NX_AT_FAULT:
/* We touched the pages ahead of time. In the most common case
* we shouldn't be here. But may be some pages were paged out.
* Kernel should have placed the faulting address to fsaddr.
*/
- NXPRT(fprintf(stderr, "ERR_NX_TRANSLATION %p\n",
+ NXPRT(fprintf(stderr, "ERR_NX_AT_FAULT %p\n",
(void *)cmdp->crb.csb.fsaddr));
if (pgfault_retries == NX_MAX_FAULTS) {
diff --git a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
index 7496a83..02dffb6 100644
--- a/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
+++ b/tools/testing/selftests/powerpc/nx-gzip/gzfht_test.c
@@ -306,13 +306,13 @@ int compress_file(int argc, char **argv, void *handle)
lzcounts, cmdp, handle);
if (cc != ERR_NX_OK && cc != ERR_NX_TPBC_GT_SPBC &&
- cc != ERR_NX_TRANSLATION) {
+ cc != ERR_NX_AT_FAULT) {
fprintf(stderr, "nx error: cc= %d\n", cc);
exit(-1);
}
/* Page faults are handled by the user code */
- if (cc == ERR_NX_TRANSLATION) {
+ if (cc == ERR_NX_AT_FAULT) {
NXPRT(fprintf(stderr, "page fault: cc= %d, ", cc));
NXPRT(fprintf(stderr, "try= %d, fsa= %08llx\n",
fault_tries,
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2] powerpc/pseries: detect secure and trusted boot state of the system.
From: Nayna Jain @ 2020-07-11 2:25 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nayna Jain, linux-kernel, Mimi Zohar
The device-tree property to check secure and trusted boot state is
different for guests(pseries) compared to baremetal(powernv).
This patch updates the existing is_ppc_secureboot_enabled() and
is_ppc_trustedboot_enabled() function to add support for pseries.
Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Reviewed-by: Daniel Axtens <dja@axtens.net>
---
v2:
* included Michael Ellerman's feedback.
* added Daniel Axtens's Reviewed-by.
arch/powerpc/kernel/secure_boot.c | 23 +++++++++++++++++++++--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
index 4b982324d368..efb325cbd42f 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -6,6 +6,7 @@
#include <linux/types.h>
#include <linux/of.h>
#include <asm/secure_boot.h>
+#include <asm/machdep.h>
static struct device_node *get_ppc_fw_sb_node(void)
{
@@ -23,12 +24,21 @@ bool is_ppc_secureboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ u32 secureboot;
node = get_ppc_fw_sb_node();
enabled = of_property_read_bool(node, "os-secureboot-enforcing");
-
of_node_put(node);
+ if (enabled)
+ goto out;
+
+ if (!of_property_read_u32(of_root, "ibm,secure-boot", &secureboot)) {
+ if (secureboot)
+ enabled = (secureboot > 1) ? true : false;
+ }
+
+out:
pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
@@ -38,12 +48,21 @@ bool is_ppc_trustedboot_enabled(void)
{
struct device_node *node;
bool enabled = false;
+ u32 trustedboot;
node = get_ppc_fw_sb_node();
enabled = of_property_read_bool(node, "trusted-enabled");
-
of_node_put(node);
+ if (enabled)
+ goto out;
+
+ if (!of_property_read_u32(of_root, "ibm,trusted-boot", &trustedboot)) {
+ if (trustedboot)
+ enabled = (trustedboot > 0) ? true : false;
+ }
+
+out:
pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
return enabled;
--
2.26.2
^ permalink raw reply related
* [v3 2/5] KVM: PPC: Book3S HV: track the state of GFNs associated with secure VMs
From: Ram Pai @ 2020-07-11 9:13 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
In-Reply-To: <1594458827-31866-1-git-send-email-linuxram@us.ibm.com>
During the life of SVM, its GFNs transition through normal, secure and
shared states. Since the kernel does not track GFNs that are shared, it
is not possible to disambiguate a shared GFN from a GFN whose PFN has
not yet been migrated to a secure-PFN. Also it is not possible to
disambiguate a secure-GFN from a GFN whose GFN has been pagedout from
the ultravisor.
The ability to identify the state of a GFN is needed to skip migration
of its PFN to secure-PFN during ESM transition.
The code is re-organized to track the states of a GFN as explained
below.
************************************************************************
1. States of a GFN
---------------
The GFN can be in one of the following states.
(a) Secure - The GFN is secure. The GFN is associated with
a Secure VM, the contents of the GFN is not accessible
to the Hypervisor. This GFN can be backed by a secure-PFN,
or can be backed by a normal-PFN with contents encrypted.
The former is true when the GFN is paged-in into the
ultravisor. The latter is true when the GFN is paged-out
of the ultravisor.
(b) Shared - The GFN is shared. The GFN is associated with a
a secure VM. The contents of the GFN is accessible to
Hypervisor. This GFN is backed by a normal-PFN and its
content is un-encrypted.
(c) Normal - The GFN is a normal. The GFN is associated with
a normal VM. The contents of the GFN is accesible to
the Hypervisor. Its content is never encrypted.
2. States of a VM.
---------------
(a) Normal VM: A VM whose contents are always accessible to
the hypervisor. All its GFNs are normal-GFNs.
(b) Secure VM: A VM whose contents are not accessible to the
hypervisor without the VM's consent. Its GFNs are
either Shared-GFN or Secure-GFNs.
(c) Transient VM: A Normal VM that is transitioning to secure VM.
The transition starts on successful return of
H_SVM_INIT_START, and ends on successful return
of H_SVM_INIT_DONE. This transient VM, can have GFNs
in any of the three states; i.e Secure-GFN, Shared-GFN,
and Normal-GFN. The VM never executes in this state
in supervisor-mode.
3. Memory slot State.
------------------
The state of a memory slot mirrors the state of the
VM the memory slot is associated with.
4. VM State transition.
--------------------
A VM always starts in Normal Mode.
H_SVM_INIT_START moves the VM into transient state. During this
time the Ultravisor may request some of its GFNs to be shared or
secured. So its GFNs can be in one of the three GFN states.
H_SVM_INIT_DONE moves the VM entirely from transient state to
secure-state. At this point any left-over normal-GFNs are
transitioned to Secure-GFN.
H_SVM_INIT_ABORT moves the transient VM back to normal VM.
All its GFNs are moved to Normal-GFNs.
UV_TERMINATE transitions the secure-VM back to normal-VM. All
the secure-GFN and shared-GFNs are tranistioned to normal-GFN
Note: The contents of the normal-GFN is undefined at this point.
5. GFN state implementation:
-------------------------
Secure GFN is associated with a secure-PFN; also called uvmem_pfn,
when the GFN is paged-in. Its pfn[] has KVMPPC_GFN_UVMEM_PFN flag
set, and contains the value of the secure-PFN.
It is associated with a normal-PFN; also called mem_pfn, when
the GFN is pagedout. Its pfn[] has KVMPPC_GFN_MEM_PFN flag set.
The value of the normal-PFN is not tracked.
Shared GFN is associated with a normal-PFN. Its pfn[] has
KVMPPC_UVMEM_SHARED_PFN flag set. The value of the normal-PFN
is not tracked.
Normal GFN is associated with normal-PFN. Its pfn[] has
no flag set. The value of the normal-PFN is not tracked.
6. Life cycle of a GFN
--------------------
--------------------------------------------------------------
| | Share | Unshare | SVM |H_SVM_INIT_DONE|
| |operation |operation | abort/ | |
| | | | terminate | |
-------------------------------------------------------------
| | | | | |
| Secure | Shared | Secure |Normal |Secure |
| | | | | |
| Shared | Shared | Secure |Normal |Shared |
| | | | | |
| Normal | Shared | Secure |Normal |Secure |
--------------------------------------------------------------
7. Life cycle of a VM
--------------------
--------------------------------------------------------------------
| | start | H_SVM_ |H_SVM_ |H_SVM_ |UV_SVM_ |
| | VM |INIT_START|INIT_DONE|INIT_ABORT |TERMINATE |
| | | | | | |
--------- ----------------------------------------------------------
| | | | | | |
| Normal | Normal | Transient|Error |Error |Normal |
| | | | | | |
| Secure | Error | Error |Error |Error |Normal |
| | | | | | |
|Transient| N/A | Error |Secure |Normal |Normal |
--------------------------------------------------------------------
************************************************************************
Cc: Paul Mackerras <paulus@ozlabs.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Bharata B Rao <bharata@linux.ibm.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Cc: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Claudio Carvalho <cclaudio@linux.ibm.com>
Cc: kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 187 +++++++++++++++++++++++++++++++++----
1 file changed, 168 insertions(+), 19 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index bfc3841..6d6c256 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -98,7 +98,127 @@
static unsigned long *kvmppc_uvmem_bitmap;
static DEFINE_SPINLOCK(kvmppc_uvmem_bitmap_lock);
-#define KVMPPC_UVMEM_PFN (1UL << 63)
+/*
+ * States of a GFN
+ * ---------------
+ * The GFN can be in one of the following states.
+ *
+ * (a) Secure - The GFN is secure. The GFN is associated with
+ * a Secure VM, the contents of the GFN is not accessible
+ * to the Hypervisor. This GFN can be backed by a secure-PFN,
+ * or can be backed by a normal-PFN with contents encrypted.
+ * The former is true when the GFN is paged-in into the
+ * ultravisor. The latter is true when the GFN is paged-out
+ * of the ultravisor.
+ *
+ * (b) Shared - The GFN is shared. The GFN is associated with a
+ * a secure VM. The contents of the GFN is accessible to
+ * Hypervisor. This GFN is backed by a normal-PFN and its
+ * content is un-encrypted.
+ *
+ * (c) Normal - The GFN is a normal. The GFN is associated with
+ * a normal VM. The contents of the GFN is accesible to
+ * the Hypervisor. Its content is never encrypted.
+ *
+ * States of a VM.
+ * ---------------
+ *
+ * Normal VM: A VM whose contents are always accessible to
+ * the hypervisor. All its GFNs are normal-GFNs.
+ *
+ * Secure VM: A VM whose contents are not accessible to the
+ * hypervisor without the VM's consent. Its GFNs are
+ * either Shared-GFN or Secure-GFNs.
+ *
+ * Transient VM: A Normal VM that is transitioning to secure VM.
+ * The transition starts on successful return of
+ * H_SVM_INIT_START, and ends on successful return
+ * of H_SVM_INIT_DONE. This transient VM, can have GFNs
+ * in any of the three states; i.e Secure-GFN, Shared-GFN,
+ * and Normal-GFN. The VM never executes in this state
+ * in supervisor-mode.
+ *
+ * Memory slot State.
+ * -----------------------------
+ * The state of a memory slot mirrors the state of the
+ * VM the memory slot is associated with.
+ *
+ * VM State transition.
+ * --------------------
+ *
+ * A VM always starts in Normal Mode.
+ *
+ * H_SVM_INIT_START moves the VM into transient state. During this
+ * time the Ultravisor may request some of its GFNs to be shared or
+ * secured. So its GFNs can be in one of the three GFN states.
+ *
+ * H_SVM_INIT_DONE moves the VM entirely from transient state to
+ * secure-state. At this point any left-over normal-GFNs are
+ * transitioned to Secure-GFN.
+ *
+ * H_SVM_INIT_ABORT moves the transient VM back to normal VM.
+ * All its GFNs are moved to Normal-GFNs.
+ *
+ * UV_TERMINATE transitions the secure-VM back to normal-VM. All
+ * the secure-GFN and shared-GFNs are tranistioned to normal-GFN
+ * Note: The contents of the normal-GFN is undefined at this point.
+ *
+ * GFN state implementation:
+ * -------------------------
+ *
+ * Secure GFN is associated with a secure-PFN; also called uvmem_pfn,
+ * when the GFN is paged-in. Its pfn[] has KVMPPC_GFN_UVMEM_PFN flag
+ * set, and contains the value of the secure-PFN.
+ * It is associated with a normal-PFN; also called mem_pfn, when
+ * the GFN is pagedout. Its pfn[] has KVMPPC_GFN_MEM_PFN flag set.
+ * The value of the normal-PFN is not tracked.
+ *
+ * Shared GFN is associated with a normal-PFN. Its pfn[] has
+ * KVMPPC_UVMEM_SHARED_PFN flag set. The value of the normal-PFN
+ * is not tracked.
+ *
+ * Normal GFN is associated with normal-PFN. Its pfn[] has
+ * no flag set. The value of the normal-PFN is not tracked.
+ *
+ * Life cycle of a GFN
+ * --------------------
+ *
+ * --------------------------------------------------------------
+ * | | Share | Unshare | SVM |H_SVM_INIT_DONE|
+ * | |operation |operation | abort/ | |
+ * | | | | terminate | |
+ * -------------------------------------------------------------
+ * | | | | | |
+ * | Secure | Shared | Secure |Normal |Secure |
+ * | | | | | |
+ * | Shared | Shared | Secure |Normal |Shared |
+ * | | | | | |
+ * | Normal | Shared | Secure |Normal |Secure |
+ * --------------------------------------------------------------
+ *
+ * Life cycle of a VM
+ * --------------------
+ *
+ * --------------------------------------------------------------------
+ * | | start | H_SVM_ |H_SVM_ |H_SVM_ |UV_SVM_ |
+ * | | VM |INIT_START|INIT_DONE|INIT_ABORT |TERMINATE |
+ * | | | | | | |
+ * --------- ----------------------------------------------------------
+ * | | | | | | |
+ * | Normal | Normal | Transient|Error |Error |Normal |
+ * | | | | | | |
+ * | Secure | Error | Error |Error |Error |Normal |
+ * | | | | | | |
+ * |Transient| N/A | Error |Secure |Normal |Normal |
+ * --------------------------------------------------------------------
+ */
+
+#define KVMPPC_GFN_UVMEM_PFN (1UL << 63)
+#define KVMPPC_GFN_MEM_PFN (1UL << 62)
+#define KVMPPC_GFN_SHARED (1UL << 61)
+#define KVMPPC_GFN_SECURE (KVMPPC_GFN_UVMEM_PFN | KVMPPC_GFN_MEM_PFN)
+#define KVMPPC_GFN_FLAG_MASK (KVMPPC_GFN_SECURE | KVMPPC_GFN_SHARED)
+#define KVMPPC_GFN_PFN_MASK (~KVMPPC_GFN_FLAG_MASK)
struct kvmppc_uvmem_slot {
struct list_head list;
@@ -106,11 +226,11 @@ struct kvmppc_uvmem_slot {
unsigned long base_pfn;
unsigned long *pfns;
};
-
struct kvmppc_uvmem_page_pvt {
struct kvm *kvm;
unsigned long gpa;
bool skip_page_out;
+ bool remove_gfn;
};
bool kvmppc_uvmem_available(void)
@@ -163,8 +283,8 @@ void kvmppc_uvmem_slot_free(struct kvm *kvm, const struct kvm_memory_slot *slot)
mutex_unlock(&kvm->arch.uvmem_lock);
}
-static void kvmppc_uvmem_pfn_insert(unsigned long gfn, unsigned long uvmem_pfn,
- struct kvm *kvm)
+static void kvmppc_mark_gfn(unsigned long gfn, struct kvm *kvm,
+ unsigned long flag, unsigned long uvmem_pfn)
{
struct kvmppc_uvmem_slot *p;
@@ -172,24 +292,41 @@ static void kvmppc_uvmem_pfn_insert(unsigned long gfn, unsigned long uvmem_pfn,
if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
unsigned long index = gfn - p->base_pfn;
- p->pfns[index] = uvmem_pfn | KVMPPC_UVMEM_PFN;
+ if (flag == KVMPPC_GFN_UVMEM_PFN)
+ p->pfns[index] = uvmem_pfn | flag;
+ else
+ p->pfns[index] = flag;
return;
}
}
}
-static void kvmppc_uvmem_pfn_remove(unsigned long gfn, struct kvm *kvm)
+/* mark the GFN as secure-GFN associated with @uvmem pfn device-PFN. */
+static void kvmppc_gfn_secure_uvmem_pfn(unsigned long gfn,
+ unsigned long uvmem_pfn, struct kvm *kvm)
{
- struct kvmppc_uvmem_slot *p;
+ kvmppc_mark_gfn(gfn, kvm, KVMPPC_GFN_UVMEM_PFN, uvmem_pfn);
+}
- list_for_each_entry(p, &kvm->arch.uvmem_pfns, list) {
- if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
- p->pfns[gfn - p->base_pfn] = 0;
- return;
- }
- }
+/* mark the GFN as secure-GFN associated with a memory-PFN. */
+static void kvmppc_gfn_secure_mem_pfn(unsigned long gfn, struct kvm *kvm)
+{
+ kvmppc_mark_gfn(gfn, kvm, KVMPPC_GFN_MEM_PFN, 0);
+}
+
+/* mark the GFN as a shared GFN. */
+static void kvmppc_gfn_shared(unsigned long gfn, struct kvm *kvm)
+{
+ kvmppc_mark_gfn(gfn, kvm, KVMPPC_GFN_SHARED, 0);
+}
+
+/* mark the GFN as a non-existent GFN. */
+static void kvmppc_gfn_remove(unsigned long gfn, struct kvm *kvm)
+{
+ kvmppc_mark_gfn(gfn, kvm, 0, 0);
}
+/* return true, if the GFN is a secure-GFN backed by a secure-PFN */
static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
unsigned long *uvmem_pfn)
{
@@ -199,10 +336,10 @@ static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
unsigned long index = gfn - p->base_pfn;
- if (p->pfns[index] & KVMPPC_UVMEM_PFN) {
+ if (p->pfns[index] & KVMPPC_GFN_UVMEM_PFN) {
if (uvmem_pfn)
*uvmem_pfn = p->pfns[index] &
- ~KVMPPC_UVMEM_PFN;
+ KVMPPC_GFN_PFN_MASK;
return true;
} else
return false;
@@ -353,6 +490,7 @@ void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
mutex_lock(&kvm->arch.uvmem_lock);
if (!kvmppc_gfn_is_uvmem_pfn(gfn, kvm, &uvmem_pfn)) {
+ kvmppc_gfn_remove(gfn, kvm);
mutex_unlock(&kvm->arch.uvmem_lock);
continue;
}
@@ -360,6 +498,7 @@ void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
uvmem_page = pfn_to_page(uvmem_pfn);
pvt = uvmem_page->zone_device_data;
pvt->skip_page_out = skip_page_out;
+ pvt->remove_gfn = true;
mutex_unlock(&kvm->arch.uvmem_lock);
pfn = gfn_to_pfn(kvm, gfn);
@@ -429,7 +568,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
goto out_clear;
uvmem_pfn = bit + pfn_first;
- kvmppc_uvmem_pfn_insert(gpa >> PAGE_SHIFT, uvmem_pfn, kvm);
+ kvmppc_gfn_secure_uvmem_pfn(gpa >> PAGE_SHIFT, uvmem_pfn, kvm);
pvt->gpa = gpa;
pvt->kvm = kvm;
@@ -524,6 +663,7 @@ static unsigned long kvmppc_share_page(struct kvm *kvm, unsigned long gpa,
uvmem_page = pfn_to_page(uvmem_pfn);
pvt = uvmem_page->zone_device_data;
pvt->skip_page_out = true;
+ pvt->remove_gfn = false;
}
retry:
@@ -537,12 +677,16 @@ static unsigned long kvmppc_share_page(struct kvm *kvm, unsigned long gpa,
uvmem_page = pfn_to_page(uvmem_pfn);
pvt = uvmem_page->zone_device_data;
pvt->skip_page_out = true;
+ pvt->remove_gfn = false;
kvm_release_pfn_clean(pfn);
goto retry;
}
- if (!uv_page_in(kvm->arch.lpid, pfn << page_shift, gpa, 0, page_shift))
+ if (!uv_page_in(kvm->arch.lpid, pfn << page_shift, gpa, 0,
+ page_shift)) {
+ kvmppc_gfn_shared(gfn, kvm);
ret = H_SUCCESS;
+ }
kvm_release_pfn_clean(pfn);
mutex_unlock(&kvm->arch.uvmem_lock);
out:
@@ -598,6 +742,7 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
if (!kvmppc_svm_page_in(vma, start, end, gpa, kvm, page_shift))
ret = H_SUCCESS;
+
out_unlock:
mutex_unlock(&kvm->arch.uvmem_lock);
out:
@@ -706,7 +851,8 @@ static vm_fault_t kvmppc_uvmem_migrate_to_ram(struct vm_fault *vmf)
/*
* Release the device PFN back to the pool
*
- * Gets called when secure page becomes a normal page during H_SVM_PAGE_OUT.
+ * Gets called when secure GFN tranistions from a secure-PFN
+ * to a normal PFN during H_SVM_PAGE_OUT.
* Gets called with kvm->arch.uvmem_lock held.
*/
static void kvmppc_uvmem_page_free(struct page *page)
@@ -721,7 +867,10 @@ static void kvmppc_uvmem_page_free(struct page *page)
pvt = page->zone_device_data;
page->zone_device_data = NULL;
- kvmppc_uvmem_pfn_remove(pvt->gpa >> PAGE_SHIFT, pvt->kvm);
+ if (pvt->remove_gfn)
+ kvmppc_gfn_remove(pvt->gpa >> PAGE_SHIFT, pvt->kvm);
+ else
+ kvmppc_gfn_secure_mem_pfn(pvt->gpa >> PAGE_SHIFT, pvt->kvm);
kfree(pvt);
}
--
1.8.3.1
^ permalink raw reply related
* [v3 1/5] KVM: PPC: Book3S HV: Disable page merging in H_SVM_INIT_START
From: Ram Pai @ 2020-07-11 9:13 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
In-Reply-To: <1594458827-31866-1-git-send-email-linuxram@us.ibm.com>
Merging of pages associated with each memslot of a SVM is
disabled the page is migrated in H_SVM_PAGE_IN handler.
This operation should have been done much earlier; the moment the VM
is initiated for secure-transition. Delaying this operation, increases
the probability for those pages to acquire new references , making it
impossible to migrate those pages in H_SVM_PAGE_IN handler.
Disable page-migration in H_SVM_INIT_START handling.
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 96 +++++++++++++++++++++++++++++---------
1 file changed, 74 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 3d987b1..bfc3841 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -211,6 +211,65 @@ static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
return false;
}
+static int kvmppc_memslot_page_merge(struct kvm *kvm,
+ struct kvm_memory_slot *memslot, bool merge)
+{
+ unsigned long gfn = memslot->base_gfn;
+ unsigned long end, start = gfn_to_hva(kvm, gfn);
+ int ret = 0;
+ struct vm_area_struct *vma;
+ int merge_flag = (merge) ? MADV_MERGEABLE : MADV_UNMERGEABLE;
+
+ if (kvm_is_error_hva(start))
+ return H_STATE;
+
+ end = start + (memslot->npages << PAGE_SHIFT);
+
+ down_write(&kvm->mm->mmap_sem);
+ do {
+ vma = find_vma_intersection(kvm->mm, start, end);
+ if (!vma) {
+ ret = H_STATE;
+ break;
+ }
+ ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
+ merge_flag, &vma->vm_flags);
+ if (ret) {
+ ret = H_STATE;
+ break;
+ }
+ start = vma->vm_end + 1;
+ } while (end > vma->vm_end);
+
+ up_write(&kvm->mm->mmap_sem);
+ return ret;
+}
+
+static int __kvmppc_page_merge(struct kvm *kvm, bool merge)
+{
+ struct kvm_memslots *slots;
+ struct kvm_memory_slot *memslot;
+ int ret = 0;
+
+ slots = kvm_memslots(kvm);
+ kvm_for_each_memslot(memslot, slots) {
+ ret = kvmppc_memslot_page_merge(kvm, memslot, merge);
+ if (ret)
+ break;
+ }
+ return ret;
+}
+
+static inline int kvmppc_disable_page_merge(struct kvm *kvm)
+{
+ return __kvmppc_page_merge(kvm, false);
+}
+
+static inline int kvmppc_enable_page_merge(struct kvm *kvm)
+{
+ return __kvmppc_page_merge(kvm, true);
+}
+
unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
{
struct kvm_memslots *slots;
@@ -232,11 +291,18 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
return H_AUTHORITY;
srcu_idx = srcu_read_lock(&kvm->srcu);
+
+ /* disable page-merging for all memslot */
+ ret = kvmppc_disable_page_merge(kvm);
+ if (ret)
+ goto out;
+
+ /* register the memslot */
slots = kvm_memslots(kvm);
kvm_for_each_memslot(memslot, slots) {
if (kvmppc_uvmem_slot_init(kvm, memslot)) {
ret = H_PARAMETER;
- goto out;
+ break;
}
ret = uv_register_mem_slot(kvm->arch.lpid,
memslot->base_gfn << PAGE_SHIFT,
@@ -245,9 +311,12 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
if (ret < 0) {
kvmppc_uvmem_slot_free(kvm, memslot);
ret = H_PARAMETER;
- goto out;
+ break;
}
}
+
+ if (ret)
+ kvmppc_enable_page_merge(kvm);
out:
srcu_read_unlock(&kvm->srcu, srcu_idx);
return ret;
@@ -384,7 +453,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
*/
static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
unsigned long end, unsigned long gpa, struct kvm *kvm,
- unsigned long page_shift, bool *downgrade)
+ unsigned long page_shift)
{
unsigned long src_pfn, dst_pfn = 0;
struct migrate_vma mig;
@@ -400,18 +469,6 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
mig.src = &src_pfn;
mig.dst = &dst_pfn;
- /*
- * We come here with mmap_sem write lock held just for
- * ksm_madvise(), otherwise we only need read mmap_sem.
- * Hence downgrade to read lock once ksm_madvise() is done.
- */
- ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
- MADV_UNMERGEABLE, &vma->vm_flags);
- downgrade_write(&kvm->mm->mmap_sem);
- *downgrade = true;
- if (ret)
- return ret;
-
ret = migrate_vma_setup(&mig);
if (ret)
return ret;
@@ -503,7 +560,6 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
unsigned long flags,
unsigned long page_shift)
{
- bool downgrade = false;
unsigned long start, end;
struct vm_area_struct *vma;
int srcu_idx;
@@ -540,16 +596,12 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
if (!vma || vma->vm_start > start || vma->vm_end < end)
goto out_unlock;
- if (!kvmppc_svm_page_in(vma, start, end, gpa, kvm, page_shift,
- &downgrade))
+ if (!kvmppc_svm_page_in(vma, start, end, gpa, kvm, page_shift))
ret = H_SUCCESS;
out_unlock:
mutex_unlock(&kvm->arch.uvmem_lock);
out:
- if (downgrade)
- up_read(&kvm->mm->mmap_sem);
- else
- up_write(&kvm->mm->mmap_sem);
+ up_write(&kvm->mm->mmap_sem);
srcu_read_unlock(&kvm->srcu, srcu_idx);
return ret;
}
--
1.8.3.1
^ permalink raw reply related
* [v3 3/5] KVM: PPC: Book3S HV: migrate remaining normal-GFNs to secure-GFNs in H_SVM_INIT_DONE
From: Ram Pai @ 2020-07-11 9:13 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
In-Reply-To: <1594458827-31866-1-git-send-email-linuxram@us.ibm.com>
The Ultravisor is expected to explicitly call H_SVM_PAGE_IN for all the pages
of the SVM before calling H_SVM_INIT_DONE. This causes a huge delay in
tranistioning the VM to SVM. The Ultravisor is interested in the pages that
contain the kernel, initrd and other important data structures. The rest of the
pages contain throw-away content. Hence requesting just the necessary and
sufficient pages from the Hypervisor is sufficient.
However if not all pages are requested by the Ultravisor, the Hypervisor
continues to consider the GFNs corresponding to the non-requested pages as
normal GFNs. This can lead to data-corruption and undefined behavior.
Move all the PFNs associated with the SVM's GFNs to secure-PFNs, in
H_SVM_INIT_DONE. Skip the GFNs that are already Paged-in or Shared or
Paged-in followed by a Paged-out.
Cc: Paul Mackerras <paulus@ozlabs.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Bharata B Rao <bharata@linux.ibm.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Cc: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Claudio Carvalho <cclaudio@linux.ibm.com>
Cc: kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
Documentation/powerpc/ultravisor.rst | 2 +
arch/powerpc/include/asm/kvm_book3s_uvmem.h | 2 +
arch/powerpc/kvm/book3s_hv_uvmem.c | 166 +++++++++++++++++++++++++---
3 files changed, 156 insertions(+), 14 deletions(-)
diff --git a/Documentation/powerpc/ultravisor.rst b/Documentation/powerpc/ultravisor.rst
index df136c8..d98fc85 100644
--- a/Documentation/powerpc/ultravisor.rst
+++ b/Documentation/powerpc/ultravisor.rst
@@ -933,6 +933,8 @@ Return values
* H_UNSUPPORTED if called from the wrong context (e.g.
from an SVM or before an H_SVM_INIT_START
hypercall).
+ * H_STATE if the hypervisor could not successfully
+ transition the VM to Secure VM.
Description
~~~~~~~~~~~
diff --git a/arch/powerpc/include/asm/kvm_book3s_uvmem.h b/arch/powerpc/include/asm/kvm_book3s_uvmem.h
index 9cb7d8b..f229ab5 100644
--- a/arch/powerpc/include/asm/kvm_book3s_uvmem.h
+++ b/arch/powerpc/include/asm/kvm_book3s_uvmem.h
@@ -23,6 +23,8 @@ unsigned long kvmppc_h_svm_page_out(struct kvm *kvm,
unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm);
void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
struct kvm *kvm, bool skip_page_out);
+int kvmppc_uv_migrate_mem_slot(struct kvm *kvm,
+ const struct kvm_memory_slot *memslot);
#else
static inline int kvmppc_uvmem_init(void)
{
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 6d6c256..12ed52a 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -93,6 +93,7 @@
#include <asm/ultravisor.h>
#include <asm/mman.h>
#include <asm/kvm_ppc.h>
+#include <asm/kvm_book3s_uvmem.h>
static struct dev_pagemap kvmppc_uvmem_pgmap;
static unsigned long *kvmppc_uvmem_bitmap;
@@ -348,6 +349,43 @@ static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
return false;
}
+/*
+ * starting from *gfn search for the next available GFN that is not yet
+ * transitioned to a secure GFN. return the value of that GFN in *gfn. If a
+ * GFN is found, return true, else return false
+ */
+static bool kvmppc_next_nontransitioned_gfn(const struct kvm_memory_slot *memslot,
+ struct kvm *kvm, unsigned long *gfn)
+{
+ struct kvmppc_uvmem_slot *p;
+ bool ret = false;
+ unsigned long i;
+
+ mutex_lock(&kvm->arch.uvmem_lock);
+
+ list_for_each_entry(p, &kvm->arch.uvmem_pfns, list)
+ if (*gfn >= p->base_pfn && *gfn < p->base_pfn + p->nr_pfns)
+ break;
+ if (!p)
+ goto out;
+ /*
+ * The code below assumes, one to one correspondence between
+ * kvmppc_uvmem_slot and memslot.
+ */
+ for (i = *gfn; i < p->base_pfn + p->nr_pfns; i++) {
+ unsigned long index = i - p->base_pfn;
+
+ if (!(p->pfns[index] & KVMPPC_GFN_FLAG_MASK)) {
+ *gfn = i;
+ ret = true;
+ break;
+ }
+ }
+out:
+ mutex_unlock(&kvm->arch.uvmem_lock);
+ return ret;
+}
+
static int kvmppc_memslot_page_merge(struct kvm *kvm,
struct kvm_memory_slot *memslot, bool merge)
{
@@ -461,12 +499,31 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
unsigned long kvmppc_h_svm_init_done(struct kvm *kvm)
{
+ struct kvm_memslots *slots;
+ struct kvm_memory_slot *memslot;
+ int srcu_idx;
+ long ret = H_SUCCESS;
+
if (!(kvm->arch.secure_guest & KVMPPC_SECURE_INIT_START))
return H_UNSUPPORTED;
+ /* migrate any unmoved normal pfn to device pfns*/
+ srcu_idx = srcu_read_lock(&kvm->srcu);
+ slots = kvm_memslots(kvm);
+ kvm_for_each_memslot(memslot, slots) {
+ ret = kvmppc_uv_migrate_mem_slot(kvm, memslot);
+ if (ret) {
+ ret = H_STATE;
+ goto out;
+ }
+ }
+
kvm->arch.secure_guest |= KVMPPC_SECURE_INIT_DONE;
pr_info("LPID %d went secure\n", kvm->arch.lpid);
- return H_SUCCESS;
+
+out:
+ srcu_read_unlock(&kvm->srcu, srcu_idx);
+ return ret;
}
/*
@@ -587,12 +644,14 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
}
/*
- * Alloc a PFN from private device memory pool and copy page from normal
- * memory to secure memory using UV_PAGE_IN uvcall.
+ * Alloc a PFN from private device memory pool. If @pagein is true,
+ * copy page from normal memory to secure memory using UV_PAGE_IN uvcall.
*/
-static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
- unsigned long end, unsigned long gpa, struct kvm *kvm,
- unsigned long page_shift)
+static int kvmppc_svm_migrate_page(struct vm_area_struct *vma,
+ unsigned long start,
+ unsigned long end, unsigned long gpa, struct kvm *kvm,
+ unsigned long page_shift,
+ bool pagein)
{
unsigned long src_pfn, dst_pfn = 0;
struct migrate_vma mig;
@@ -613,7 +672,7 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
return ret;
if (!(*mig.src & MIGRATE_PFN_MIGRATE)) {
- ret = -1;
+ ret = -2;
goto out_finalize;
}
@@ -623,11 +682,16 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
goto out_finalize;
}
- pfn = *mig.src >> MIGRATE_PFN_SHIFT;
- spage = migrate_pfn_to_page(*mig.src);
- if (spage)
- uv_page_in(kvm->arch.lpid, pfn << page_shift, gpa, 0,
- page_shift);
+ if (pagein) {
+ pfn = *mig.src >> MIGRATE_PFN_SHIFT;
+ spage = migrate_pfn_to_page(*mig.src);
+ if (spage) {
+ ret = uv_page_in(kvm->arch.lpid, pfn << page_shift,
+ gpa, 0, page_shift);
+ if (ret)
+ goto out_finalize;
+ }
+ }
*mig.dst = migrate_pfn(page_to_pfn(dpage)) | MIGRATE_PFN_LOCKED;
migrate_vma_pages(&mig);
@@ -637,6 +701,77 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
}
/*
+ * return 1, if some page migration failed because of transient error,
+ * while the remaining pages migrated successfully.
+ * The caller can use this as a hint to retry.
+ *
+ * return 0 otherwise. *ret indicates the success status
+ * of this call.
+ */
+static int __kvmppc_uv_migrate_mem_slot(struct kvm *kvm,
+ const struct kvm_memory_slot *memslot, int *ret)
+{
+ unsigned long gfn = memslot->base_gfn;
+ struct vm_area_struct *vma;
+ unsigned long start, end;
+ bool retry = 0;
+
+ *ret = 0;
+ while (kvmppc_next_nontransitioned_gfn(memslot, kvm, &gfn)) {
+
+ down_write(&kvm->mm->mmap_sem);
+ start = gfn_to_hva(kvm, gfn);
+ if (kvm_is_error_hva(start)) {
+ *ret = H_STATE;
+ goto next;
+ }
+
+ end = start + (1UL << PAGE_SHIFT);
+ vma = find_vma_intersection(kvm->mm, start, end);
+ if (!vma || vma->vm_start > start || vma->vm_end < end) {
+ *ret = H_STATE;
+ goto next;
+ }
+
+ mutex_lock(&kvm->arch.uvmem_lock);
+ *ret = kvmppc_svm_migrate_page(vma, start, end,
+ (gfn << PAGE_SHIFT), kvm, PAGE_SHIFT, false);
+ mutex_unlock(&kvm->arch.uvmem_lock);
+
+next:
+ up_write(&kvm->mm->mmap_sem);
+
+ if (*ret == -2) {
+ retry = 1;
+ continue;
+ }
+
+ if (*ret)
+ return 0;
+ }
+ return retry;
+}
+
+#define REPEAT_COUNT 10
+
+int kvmppc_uv_migrate_mem_slot(struct kvm *kvm,
+ const struct kvm_memory_slot *memslot)
+{
+ int ret = 0, repeat_count = REPEAT_COUNT;
+
+ /*
+ * try migration of pages in the memslot 'repeat_count' number of
+ * times, provided each time migration fails because of transient
+ * errors only.
+ */
+ while (__kvmppc_uv_migrate_mem_slot(kvm, memslot, &ret) &&
+ repeat_count--)
+ ;
+
+ return ret;
+}
+
+/*
* Shares the page with HV, thus making it a normal page.
*
* - If the page is already secure, then provision a new page and share
@@ -740,8 +875,11 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
if (!vma || vma->vm_start > start || vma->vm_end < end)
goto out_unlock;
- if (!kvmppc_svm_page_in(vma, start, end, gpa, kvm, page_shift))
- ret = H_SUCCESS;
+ if (kvmppc_svm_migrate_page(vma, start, end, gpa, kvm, page_shift,
+ true))
+ goto out_unlock;
+
+ ret = H_SUCCESS;
out_unlock:
mutex_unlock(&kvm->arch.uvmem_lock);
--
1.8.3.1
^ permalink raw reply related
* [v3 5/5] KVM: PPC: Book3S HV: migrate hot plugged memory
From: Ram Pai @ 2020-07-11 9:13 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
In-Reply-To: <1594458827-31866-1-git-send-email-linuxram@us.ibm.com>
From: Laurent Dufour <ldufour@linux.ibm.com>
When a memory slot is hot plugged to a SVM, PFNs associated with the
GFNs in that slot must be migrated to the secure-PFNs, aka device-PFNs.
kvmppc_uv_migrate_mem_slot() is called to accomplish this. UV_PAGE_IN
ucall is skipped, since the ultravisor does not trust the content of
those pages and hence ignores it.
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
[resolved conflicts, and modified the commit log]
---
arch/powerpc/kvm/book3s_hv.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 819f96d..b0d4231 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4523,10 +4523,12 @@ static void kvmppc_core_commit_memory_region_hv(struct kvm *kvm,
case KVM_MR_CREATE:
if (kvmppc_uvmem_slot_init(kvm, new))
return;
- uv_register_mem_slot(kvm->arch.lpid,
- new->base_gfn << PAGE_SHIFT,
- new->npages * PAGE_SIZE,
- 0, new->id);
+ if (uv_register_mem_slot(kvm->arch.lpid,
+ new->base_gfn << PAGE_SHIFT,
+ new->npages * PAGE_SIZE,
+ 0, new->id))
+ return;
+ kvmppc_uv_migrate_mem_slot(kvm, new);
break;
case KVM_MR_DELETE:
uv_unregister_mem_slot(kvm->arch.lpid, old->id);
--
1.8.3.1
^ 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