* Re: [PATCH 5/5] powerpc/8xx: fix possible object reference leak
From: Christophe Leroy @ 2019-03-22 7:09 UTC (permalink / raw)
To: Wen Yang, linux-kernel; +Cc: wang.yi59, linuxppc-dev, Paul Mackerras
In-Reply-To: <1553223955-7350-5-git-send-email-wen.yang99@zte.com.cn>
On 03/22/2019 03:05 AM, Wen Yang wrote:
> The call to of_find_compatible_node returns a node pointer with refcount
> incremented thus it must be explicitly decremented after the last
> usage.
> irq_domain_add_linear also calls of_node_get to increase refcount,
> so irq_domain will not be affected when it is released.
Should you have a:
Fixes: a8db8cf0d894 ("irq_domain: Replace irq_alloc_host() with
revmap-specific initializers")
If not, it means your change is in contradiction with commit
b1725c9319aa ("[POWERPC] arch/powerpc/sysdev: Add missing of_node_put")
>
> Detected by coccinelle with the following warnings:
> ./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 136, but without a corresponding object release within this function.
>
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: Vitaly Bordug <vitb@kernel.crashing.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-kernel@vger.kernel.org
> ---
> arch/powerpc/platforms/8xx/pic.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
> index 8d5a25d..13d880b 100644
> --- a/arch/powerpc/platforms/8xx/pic.c
> +++ b/arch/powerpc/platforms/8xx/pic.c
> @@ -155,6 +155,7 @@ int mpc8xx_pic_init(void)
> ret = -ENOMEM;
> goto out;
> }
> + of_node_put(np);
> return 0;
>
> out:
>
I guess it would be better as follows:
--- a/arch/powerpc/platforms/8xx/pic.c
+++ b/arch/powerpc/platforms/8xx/pic.c
@@ -153,9 +153,7 @@ int mpc8xx_pic_init(void)
if (mpc8xx_pic_host == NULL) {
printk(KERN_ERR "MPC8xx PIC: failed to allocate irq
host!\n");
ret = -ENOMEM;
- goto out;
}
- return 0;
out:
of_node_put(np);
Christophe
^ permalink raw reply
* [PATCH 0/2] Auto-promotion logic for cpuidle states
From: Abhishek Goel @ 2019-03-22 6:25 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm; +Cc: daniel.lezcano, rjw, Abhishek Goel
Currently, the cpuidle governors (menu/ladder) determine what idle state a
idling CPU should enter into based on heuristics that depend on the idle
history on that CPU. Given that no predictive heuristic is perfect, there
are cases where the governor predicts a shallow idle state, hoping that
the CPU will be busy soon. However, if no new workload is scheduled on
that CPU in the near future, the CPU will end up in the shallow state.
Motivation
----------
In case of POWER, this is problematic, when the predicted state in the
aforementioned scenario is a lite stop state, as such lite states will
inhibit SMT folding, thereby depriving the other threads in the core from
using the core resources.
To address this, such lite states need to be autopromoted. The cpuidle-core
can queue timer to correspond with the residency value of the next
available state. Thus leading to auto-promotion to a deeper idle state as
soon as possible.
Experiment
----------
Without this patch -
It was seen that for a idle system, a cpu may remain in stop0_lite for few
seconds and then directly goes to a deeper state such as stop2.
With this patch -
A cpu will not remain in stop0_lite for more than the residency of next
available state, and thus it will go to a deeper state in conservative
fashion. Using this, we may spent even less than 20 milliseconds if
susbsequent stop states are enabled. In the worst case, we may end up
spending more than a second, as was the case without this patch. The
worst case will occur in the scenario when no other shallow states are
enbaled, and only deep states are available for auto-promotion.
Abhishek Goel (2):
cpuidle : auto-promotion for cpuidle states
cpuidle : Add auto-promotion flag to cpuidle flags
arch/powerpc/include/asm/opal-api.h | 1 +
drivers/cpuidle/Kconfig | 4 ++++
drivers/cpuidle/cpuidle-powernv.c | 13 +++++++++++--
drivers/cpuidle/cpuidle.c | 3 ---
4 files changed, 16 insertions(+), 5 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH 2/2] cpuidle : Add auto-promotion flag to cpuidle flags
From: Abhishek Goel @ 2019-03-22 6:25 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm; +Cc: daniel.lezcano, rjw, Abhishek Goel
In-Reply-To: <20190322062530.7586-1-huntbag@linux.vnet.ibm.com>
This patch sets up flags for the state which needs to be auto-promoted.
For powernv systems, lite states do not even lose user context. That
information has been used to set the flag for lite states.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/opal-api.h | 1 +
drivers/cpuidle/Kconfig | 4 ++++
drivers/cpuidle/cpuidle-powernv.c | 13 +++++++++++--
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index 870fb7b23..735dec731 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -226,6 +226,7 @@
*/
#define OPAL_PM_TIMEBASE_STOP 0x00000002
+#define OPAL_PM_LOSE_USER_CONTEXT 0x00001000
#define OPAL_PM_LOSE_HYP_CONTEXT 0x00002000
#define OPAL_PM_LOSE_FULL_CONTEXT 0x00004000
#define OPAL_PM_NAP_ENABLED 0x00010000
diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index 7e48eb5bf..0ece62684 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -26,6 +26,10 @@ config CPU_IDLE_GOV_MENU
config DT_IDLE_STATES
bool
+config CPU_IDLE_AUTO_PROMOTION
+ bool
+ default y if PPC_POWERNV
+
menu "ARM CPU Idle Drivers"
depends on ARM || ARM64
source "drivers/cpuidle/Kconfig.arm"
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 84b1ebe21..e351f5f9c 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -299,6 +299,7 @@ static int powernv_add_idle_states(void)
for (i = 0; i < dt_idle_states; i++) {
unsigned int exit_latency, target_residency;
bool stops_timebase = false;
+ bool lose_user_context = false;
struct pnv_idle_states_t *state = &pnv_idle_states[i];
/*
@@ -324,6 +325,9 @@ static int powernv_add_idle_states(void)
if (has_stop_states && !(state->valid))
continue;
+ if (state->flags & OPAL_PM_LOSE_USER_CONTEXT)
+ lose_user_context = true;
+
if (state->flags & OPAL_PM_TIMEBASE_STOP)
stops_timebase = true;
@@ -332,12 +336,17 @@ static int powernv_add_idle_states(void)
add_powernv_state(nr_idle_states, "Nap",
CPUIDLE_FLAG_NONE, nap_loop,
target_residency, exit_latency, 0, 0);
+ } else if (has_stop_states & !lose_user_context) {
+ add_powernv_state(nr_idle_states, state->name,
+ CPUIDLE_FLAG_AUTO_PROMOTION,
+ stop_loop, target_residency,
+ exit_latency, state->psscr_val,
+ state->psscr_mask);
} else if (has_stop_states && !stops_timebase) {
add_powernv_state(nr_idle_states, state->name,
CPUIDLE_FLAG_NONE, stop_loop,
target_residency, exit_latency,
- state->psscr_val,
- state->psscr_mask);
+ state->psscr_val, state->psscr_mask);
}
/*
--
2.17.1
^ permalink raw reply related
* [PATCH 1/2] cpuidle : auto-promotion for cpuidle states
From: Abhishek Goel @ 2019-03-22 6:25 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-pm; +Cc: daniel.lezcano, rjw, Abhishek Goel
In-Reply-To: <20190322062530.7586-1-huntbag@linux.vnet.ibm.com>
Currently, the cpuidle governors (menu /ladder) determine what idle state
an idling CPU should enter into based on heuristics that depend on the
idle history on that CPU. Given that no predictive heuristic is perfect,
there are cases where the governor predicts a shallow idle state, hoping
that the CPU will be busy soon. However, if no new workload is scheduled
on that CPU in the near future, the CPU will end up in the shallow state.
In case of POWER, this is problematic, when the predicted state in the
aforementioned scenario is a lite stop state, as such lite states will
inhibit SMT folding, thereby depriving the other threads in the core from
using the core resources.
To address this, such lite states need to be autopromoted. The cpuidle-
core can queue timer to correspond with the residency value of the next
available state. Thus leading to auto-promotion to a deeper idle state as
soon as possible.
Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
---
drivers/cpuidle/cpuidle.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 2406e2655..c4d1c1b38 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -584,11 +584,8 @@ static void __cpuidle_unregister_device(struct cpuidle_device *dev)
static void __cpuidle_device_init(struct cpuidle_device *dev)
{
- int i;
memset(dev->states_usage, 0, sizeof(dev->states_usage));
dev->last_residency = 0;
- for (i = 0; i < CPUIDLE_STATE_MAX; i++)
- dev->states_usage[i].disable = true;
}
/**
--
2.17.1
^ permalink raw reply related
* [PATCH] powerpc: vdso: Make vdso32 installation conditional in vdso_install
From: Ben Hutchings @ 2019-03-22 4:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, 785065
[-- Attachment #1: Type: text/plain, Size: 831 bytes --]
The 32-bit vDSO is not needed and not normally built for 64-bit
little-endian configurations. However, the vdso_install target still
builds and installs it. Add the same config condition as is normally
used for the build.
Fixes: e0d005916994 ("powerpc/vdso: Disable building the 32-bit VDSO ...")
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
arch/powerpc/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 488c9edffa58..3def265cf1cf 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -406,7 +406,9 @@ endef
ifdef CONFIG_PPC64
$(Q)$(MAKE) $(build)=arch/$(ARCH)/kernel/vdso64 $@
endif
+ifdef CONFIG_VDSO32
$(Q)$(MAKE) $(build)=arch/$(ARCH)/kernel/vdso32 $@
+endif
archclean:
$(Q)$(MAKE) $(clean)=$(boot)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH linux-next v8 5/7] powerpc: define syscall_get_error()
From: Dmitry V. Levin @ 2019-03-22 4:16 UTC (permalink / raw)
To: Michael Ellerman
Cc: Eugene Syromyatnikov, Oleg Nesterov, Elvira Khabirova,
Paul Mackerras, Andy Lutomirski, linuxppc-dev, linux-kernel
In-Reply-To: <20190322041409.GA27266@altlinux.org>
syscall_get_error() is required to be implemented on this
architecture in addition to already implemented syscall_get_nr(),
syscall_get_arguments(), syscall_get_return_value(), and
syscall_get_arch() functions in order to extend the generic
ptrace API with PTRACE_GET_SYSCALL_INFO request.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Elvira Khabirova <lineprinter@altlinux.org>
Cc: Eugene Syromyatnikov <esyr@redhat.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
Notes:
v8: unchanged
v7: unchanged
v6: unchanged
v5: initial revision
This change has been tested with
tools/testing/selftests/ptrace/get_syscall_info.c and strace,
so it's correct from PTRACE_GET_SYSCALL_INFO point of view.
This cast doubts on commit v4.3-rc1~86^2~81 that changed
syscall_set_return_value() in a way that doesn't quite match
syscall_get_error(), but syscall_set_return_value() is out
of scope of this series, so I'll just let you know my concerns.
arch/powerpc/include/asm/syscall.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index efb50429c9f4..7375808c566c 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -38,6 +38,16 @@ static inline void syscall_rollback(struct task_struct *task,
regs->gpr[3] = regs->orig_gpr3;
}
+static inline long syscall_get_error(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ /*
+ * If the system call failed,
+ * regs->gpr[3] contains a positive ERRORCODE.
+ */
+ return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+}
+
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
--
ldv
^ permalink raw reply related
* [PATCH linux-next v8 0/7] ptrace: add PTRACE_GET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2019-03-22 4:14 UTC (permalink / raw)
To: Oleg Nesterov, Andy Lutomirski
Cc: James E.J. Bottomley, Paul Mackerras, linux-kselftest,
Vincent Chen, Shuah Khan, Helge Deller, Eugene Syromyatnikov,
Elvira Khabirova, James Hogan, strace-devel, Kees Cook,
linux-kernel, Greentime Hu, linux-parisc, linux-api, linux-mips,
Ralf Baechle, Richard Kuo, Paul Burton, linux-hexagon,
linuxppc-dev
PTRACE_GET_SYSCALL_INFO is a generic ptrace API that lets ptracer obtain
details of the syscall the tracee is blocked in.
There are two reasons for a special syscall-related ptrace request.
Firstly, with the current ptrace API there are cases when ptracer cannot
retrieve necessary information about syscalls. Some examples include:
* The notorious int-0x80-from-64-bit-task issue. See [1] for details.
In short, if a 64-bit task performs a syscall through int 0x80, its tracer
has no reliable means to find out that the syscall was, in fact,
a compat syscall, and misidentifies it.
* Syscall-enter-stop and syscall-exit-stop look the same for the tracer.
Common practice is to keep track of the sequence of ptrace-stops in order
not to mix the two syscall-stops up. But it is not as simple as it looks;
for example, strace had a (just recently fixed) long-standing bug where
attaching strace to a tracee that is performing the execve system call
led to the tracer identifying the following syscall-exit-stop as
syscall-enter-stop, which messed up all the state tracking.
* Since the introduction of commit 84d77d3f06e7e8dea057d10e8ec77ad71f721be3
("ptrace: Don't allow accessing an undumpable mm"), both PTRACE_PEEKDATA
and process_vm_readv become unavailable when the process dumpable flag
is cleared. On such architectures as ia64 this results in all syscall
arguments being unavailable for the tracer.
Secondly, ptracers also have to support a lot of arch-specific code for
obtaining information about the tracee. For some architectures, this
requires a ptrace(PTRACE_PEEKUSER, ...) invocation for every syscall
argument and return value.
PTRACE_GET_SYSCALL_INFO returns the following structure:
struct ptrace_syscall_info {
__u8 op; /* PTRACE_SYSCALL_INFO_* */
__u32 arch __attribute__((__aligned__(sizeof(__u32))));
__u64 instruction_pointer;
__u64 stack_pointer;
union {
struct {
__u64 nr;
__u64 args[6];
} entry;
struct {
__s64 rval;
__u8 is_error;
} exit;
struct {
__u64 nr;
__u64 args[6];
__u32 ret_data;
} seccomp;
};
};
The structure was chosen according to [2], except for the following
changes:
* seccomp substructure was added as a superset of entry substructure;
* the type of nr field was changed from int to __u64 because syscall
numbers are, as a practical matter, 64 bits;
* stack_pointer field was added along with instruction_pointer field
since it is readily available and can save the tracer from extra
PTRACE_GETREGS/PTRACE_GETREGSET calls;
* arch is always initialized to aid with tracing system calls
* such as execve();
* instruction_pointer and stack_pointer are always initialized
so they could be easily obtained for non-syscall stops;
* a boolean is_error field was added along with rval field, this way
the tracer can more reliably distinguish a return value
from an error value.
strace has been ported to PTRACE_GET_SYSCALL_INFO.
Starting with release 4.26, strace uses PTRACE_GET_SYSCALL_INFO API
as the preferred mechanism of obtaining syscall information.
[1] https://lore.kernel.org/lkml/CA+55aFzcSVmdDj9Lh_gdbz1OzHyEm6ZrGPBDAJnywm2LF_eVyg@mail.gmail.com/
[2] https://lore.kernel.org/lkml/CAObL_7GM0n80N7J_DFw_eQyfLyzq+sf4y2AvsCCV88Tb3AwEHA@mail.gmail.com/
---
Notes:
v8:
* Moved syscall_get_arch() specific patches to a separate patchset
which is now merged into audit/next tree.
* Rebased to linux-next.
* Moved ptrace_get_syscall_info code under #ifdef CONFIG_HAVE_ARCH_TRACEHOOK,
narrowing down the set of architectures supported by this implementation
back to those 19 that enable CONFIG_HAVE_ARCH_TRACEHOOK because
I failed to get all syscall_get_*(), instruction_pointer(),
and user_stack_pointer() functions implemented on some niche
architectures. This leaves the following architectures out:
alpha, h8300, m68k, microblaze, and unicore32.
v7:
* Rebased to v5.0-rc1.
* 5 arch-specific preparatory patches out of 25 have been merged
into v5.0-rc1 via arch trees.
v6:
* Add syscall_get_arguments and syscall_set_arguments wrappers
to asm-generic/syscall.h, requested by Geert.
* Change PTRACE_GET_SYSCALL_INFO return code: do not take trailing paddings
into account, use the end of the last field of the structure being written.
* Change struct ptrace_syscall_info:
* remove .frame_pointer field, is is not needed and not portable;
* make .arch field explicitly aligned, remove no longer needed
padding before .arch field;
* remove trailing pads, they are no longer needed.
v5:
* Merge separate series and patches into the single series.
* Change PTRACE_EVENTMSG_SYSCALL_{ENTRY,EXIT} values as requested by Oleg.
* Change struct ptrace_syscall_info: generalize instruction_pointer,
stack_pointer, and frame_pointer fields by moving them from
ptrace_syscall_info.{entry,seccomp} substructures to ptrace_syscall_info
and initializing them for all stops.
* Add PTRACE_SYSCALL_INFO_NONE, set it when not in a syscall stop,
so e.g. "strace -i" could use PTRACE_SYSCALL_INFO_SECCOMP to obtain
instruction_pointer when the tracee is in a signal stop.
* Patch all remaining architectures to provide all necessary
syscall_get_* functions.
* Make available for all architectures: do not conditionalize on
CONFIG_HAVE_ARCH_TRACEHOOK since all syscall_get_* functions
are implemented on all architectures.
* Add a test for PTRACE_GET_SYSCALL_INFO to selftests/ptrace.
v4:
* Do not introduce task_struct.ptrace_event,
use child->last_siginfo->si_code instead.
* Implement PTRACE_SYSCALL_INFO_SECCOMP and ptrace_syscall_info.seccomp
support along with PTRACE_SYSCALL_INFO_{ENTRY,EXIT} and
ptrace_syscall_info.{entry,exit}.
v3:
* Change struct ptrace_syscall_info.
* Support PTRACE_EVENT_SECCOMP by adding ptrace_event to task_struct.
* Add proper defines for ptrace_syscall_info.op values.
* Rename PT_SYSCALL_IS_ENTERING and PT_SYSCALL_IS_EXITING to
PTRACE_EVENTMSG_SYSCALL_ENTRY and PTRACE_EVENTMSG_SYSCALL_EXIT
* and move them to uapi.
v2:
* Do not use task->ptrace.
* Replace entry_info.is_compat with entry_info.arch, use syscall_get_arch().
* Use addr argument of sys_ptrace to get expected size of the struct;
return full size of the struct.
Dmitry V. Levin (6):
nds32: fix asm/syscall.h
hexagon: define syscall_get_error() and syscall_get_return_value()
mips: define syscall_get_error()
parisc: define syscall_get_error()
powerpc: define syscall_get_error()
selftests/ptrace: add a test case for PTRACE_GET_SYSCALL_INFO
Elvira Khabirova (1):
ptrace: add PTRACE_GET_SYSCALL_INFO request
arch/hexagon/include/asm/syscall.h | 14 +
arch/mips/include/asm/syscall.h | 6 +
arch/nds32/include/asm/syscall.h | 29 +-
arch/parisc/include/asm/syscall.h | 7 +
arch/powerpc/include/asm/syscall.h | 10 +
include/linux/tracehook.h | 9 +-
include/uapi/linux/ptrace.h | 35 +++
kernel/ptrace.c | 103 ++++++-
tools/testing/selftests/ptrace/.gitignore | 1 +
tools/testing/selftests/ptrace/Makefile | 2 +-
.../selftests/ptrace/get_syscall_info.c | 271 ++++++++++++++++++
11 files changed, 471 insertions(+), 16 deletions(-)
create mode 100644 tools/testing/selftests/ptrace/get_syscall_info.c
--
ldv
^ permalink raw reply
* Re: [PATCH kernel RFC 2/2] vfio-pci-nvlink2: Implement interconnect isolation
From: David Gibson @ 2019-03-22 3:08 UTC (permalink / raw)
To: Alex Williamson
Cc: Jose Ricardo Ziviani, Alexey Kardashevskiy,
Daniel Henrique Barboza, kvm-ppc, Piotr Jaroszynski,
Leonardo Augusto Guimarães Garcia, linuxppc-dev
In-Reply-To: <20190321121934.283cdc67@x1.home>
[-- Attachment #1: Type: text/plain, Size: 14424 bytes --]
On Thu, Mar 21, 2019 at 12:19:34PM -0600, Alex Williamson wrote:
> On Thu, 21 Mar 2019 10:56:00 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Wed, Mar 20, 2019 at 01:09:08PM -0600, Alex Williamson wrote:
> > > On Wed, 20 Mar 2019 15:38:24 +1100
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >
> > > > On Tue, Mar 19, 2019 at 10:36:19AM -0600, Alex Williamson wrote:
> > > > > On Fri, 15 Mar 2019 19:18:35 +1100
> > > > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > > > >
> > > > > > The NVIDIA V100 SXM2 GPUs are connected to the CPU via PCIe links and
> > > > > > (on POWER9) NVLinks. In addition to that, GPUs themselves have direct
> > > > > > peer to peer NVLinks in groups of 2 to 4 GPUs. At the moment the POWERNV
> > > > > > platform puts all interconnected GPUs to the same IOMMU group.
> > > > > >
> > > > > > However the user may want to pass individual GPUs to the userspace so
> > > > > > in order to do so we need to put them into separate IOMMU groups and
> > > > > > cut off the interconnects.
> > > > > >
> > > > > > Thankfully V100 GPUs implement an interface to do by programming link
> > > > > > disabling mask to BAR0 of a GPU. Once a link is disabled in a GPU using
> > > > > > this interface, it cannot be re-enabled until the secondary bus reset is
> > > > > > issued to the GPU.
> > > > > >
> > > > > > This defines a reset_done() handler for V100 NVlink2 device which
> > > > > > determines what links need to be disabled. This relies on presence
> > > > > > of the new "ibm,nvlink-peers" device tree property of a GPU telling which
> > > > > > PCI peers it is connected to (which includes NVLink bridges or peer GPUs).
> > > > > >
> > > > > > This does not change the existing behaviour and instead adds
> > > > > > a new "isolate_nvlink" kernel parameter to allow such isolation.
> > > > > >
> > > > > > The alternative approaches would be:
> > > > > >
> > > > > > 1. do this in the system firmware (skiboot) but for that we would need
> > > > > > to tell skiboot via an additional OPAL call whether or not we want this
> > > > > > isolation - skiboot is unaware of IOMMU groups.
> > > > > >
> > > > > > 2. do this in the secondary bus reset handler in the POWERNV platform -
> > > > > > the problem with that is at that point the device is not enabled, i.e.
> > > > > > config space is not restored so we need to enable the device (i.e. MMIO
> > > > > > bit in CMD register + program valid address to BAR0) in order to disable
> > > > > > links and then perhaps undo all this initialization to bring the device
> > > > > > back to the state where pci_try_reset_function() expects it to be.
> > > > >
> > > > > The trouble seems to be that this approach only maintains the isolation
> > > > > exposed by the IOMMU group when vfio-pci is the active driver for the
> > > > > device. IOMMU groups can be used by any driver and the IOMMU core is
> > > > > incorporating groups in various ways.
> > > >
> > > > I don't think that reasoning is quite right. An IOMMU group doesn't
> > > > necessarily represent devices which *are* isolated, just devices which
> > > > *can be* isolated. There are plenty of instances when we don't need
> > > > to isolate devices in different IOMMU groups: passing both groups to
> > > > the same guest or userspace VFIO driver for example, or indeed when
> > > > both groups are owned by regular host kernel drivers.
> > > >
> > > > In at least some of those cases we also don't want to isolate the
> > > > devices when we don't have to, usually for performance reasons.
> > >
> > > I see IOMMU groups as representing the current isolation of the device,
> > > not just the possible isolation. If there are ways to break down that
> > > isolation then ideally the group would be updated to reflect it. The
> > > ACS disable patches seem to support this, at boot time we can choose to
> > > disable ACS at certain points in the topology to favor peer-to-peer
> > > performance over isolation. This is then reflected in the group
> > > composition, because even though ACS *can be* enabled at the given
> > > isolation points, it's intentionally not with this option. Whether or
> > > not a given user who owns multiple devices needs that isolation is
> > > really beside the point, the user can choose to connect groups via IOMMU
> > > mappings or reconfigure the system to disable ACS and potentially more
> > > direct routing. The IOMMU groups are still accurately reflecting the
> > > topology and IOMMU based isolation.
> >
> > Huh, ok, I think we need to straighten this out. Thinking of iommu
> > groups as possible rather than potential isolation was a conscious
>
> possible ~= potential
Sorry, I meant "current" not "potential".
> > decision on my part when we were first coming up with them. The
> > rationale was that that way iommu groups could be static for the
> > lifetime of boot, with more dynamic isolation state layered on top.
> >
> > Now, that was based on analogy with PAPR's concept of "Partitionable
> > Endpoints" which are decided by firmware before boot. However, I
> > think it makes sense in other contexts too: if iommu groups represent
> > current isolation, then we need some other way to advertise possible
> > isolation - otherwise how will the admin (and/or tools) know how it
> > can configure the iommu groups.
> >
> > VFIO already has the container, which represents explicitly a "group
> > of groups" that we don't care to isolate from each other. I don't
> > actually know what other uses of the iommu group infrastructure we
> > have at present and how they treat them.
>
> s/that we don't care to isolate/that the user doesn't care to isolate/
Well, true, I guess we still want to isolate them when possible for
additional safety. But I don't think we should do so when that
isolation has a real performance cost, which it would in the case of
isolating linked GPUs here.
> Though even that is not necessarily accurate, the container represents
> a shared IOMMU context, that context doesn't necessarily include
> mappings between devices, so the device can still be isolated *from
> each other*.
Well, sure, but it's the only mechanism the user has for indicating
that they don't care about isolation between these device. That might
be because they simply don't care, but it might also be because they
want to use interlinks between those devices for additional
performance.
> > So, if we now have dynamically reconfigurable groups which are a
> > departure from that design, how can we go from here trying to bring
> > things back to consistency.
>
> We don't currently have dynamically reconfigurable groups,
Ah, ok. So what were these other use cases you were describing? Boot
time options which change how the iommu groups are generated? Or
something else.
> I think the
> question is how do we introduce dynamically configurable groups within
> the existing design, because whatever intentions were 7.5 years ago is
> rather irrelevant now.
Well, yeah, I guess.
> VFIO groups are mapped directly to IOMMU groups. IOMMU groups are the
> smallest set of devices which can be considered isolated from other
> sets of devices (not potentially, but as configured).
Hm.. but what actually makes that assumption that this is the case as
configured, rather than just possibly - I'm taking "possible" to mean
possible with this host kernel, not just possible with this hardware.
> VFIO groups are
> the unit of ownership to userspace, therefore a VFIO group must be
> (currently) isolated from other groups.
It must be currently isolated once the VFIO group is instantiated, but
is there actually anything that requires that current isolation before
the iommu group is instantiated as a vfio group.
> The user owns and manages the
> IOMMU context for one or more groups via a container. A container
> allows efficiency in managing a shared IOVA space between groups.
Yes, and it seems to me an obvious extension to have the container
also permit other efficiencies which aren't compatible with full
isolation.
> > > > > So, if there's a device specific
> > > > > way to configure the isolation reported in the group, which requires
> > > > > some sort of active management against things like secondary bus
> > > > > resets, then I think we need to manage it above the attached endpoint
> > > > > driver.
> > > >
> > > > The problem is that above the endpoint driver, we don't actually have
> > > > enough information about what should be isolated. For VFIO we want to
> > > > isolate things if they're in different containers, for most regular
> > > > host kernel drivers we don't need to isolate at all (although we might
> > > > as well when it doesn't have a cost).
> > >
> > > This idea that we only want to isolate things if they're in different
> > > containers is bogus, imo. There are performance reasons why we might
> > > not want things isolated, but there are also address space reasons why
> > > we do. If there are direct routes between devices, the user needs to
> > > be aware of the IOVA pollution, if we maintain singleton groups, they
> > > don't. Granted we don't really account for this well in most
> > > userspaces and fumble through it by luck of the address space layout
> > > and lack of devices really attempting peer to peer access.
> >
> > I don't really follow what you're saying here.
>
> If the lack of isolation between devices includes peer-to-peer channels
> then the MMIO of the devices within the group pollutes the IOVA space
> of the container.
Yes, if the permissible IOVA addresses overlap with valid MMIO
addresses. I don't really see why that's significant, though. We
already have basically the same situation if there are multiple
devices in a group. e.g. if you have several devices behind a dumb
PCI-E to PCI bridge, you can't actually prohibit peer-to-peer DMA
between them, and so those devices' MMIOs pollute the IOVA space for
each other, even if they don't for other devices in the container.
There's only so far we can go to prevent the user from shooting
themselves in the foot.
> > > For in-kernel users, we're still theoretically trying to isolate
> > > devices such that they have restricted access to only the resources
> > > they need. Disabling things like ACS in the topology reduces that
> > > isolation. AFAICT, most users don't really care about that degree of
> > > isolation, so they run with iommu=pt for native driver performance
> > > while still having the IOMMU available for isolation use cases running
> > > in parallel. We don't currently have support for on-demand enabling
> > > isolation.
> >
> > Ok.
> >
> > > > The host side nVidia GPGPU
> > > > drivers also won't want to isolate the (host owned) NVLink devices
> > > > from each other, since they'll want to use the fast interconnects
> > >
> > > This falls into the same mixed use case scenario above where we don't
> > > really have a good solution today. Things like ACS are dynamically
> > > configurable, but we don't expose any interfaces to let drivers or
> > > users change it (aside from setpci, which we don't account for
> > > dynamically). We assume a simplistic model where if you want IOMMU,
> > > then you must also want the maximum configurable isolation.
> > > Dynamically changing routing is not necessarily the most foolproof
> > > thing either with potentially in-flight transactions and existing DMA
> > > mappings, which is why I've suggested a couple times that perhaps we
> > > could do a software hot-unplug of a sub-hierarchy, muck with isolation
> > > at the remaining node, then re-discover the removed devices.
> >
> > Ok, so I feel like we need to go fully one way or the other. Either:
> >
> > 1) Groups represent current isolation status, in which case we
> > deprecate vfio containers in favour of fusing groups beforehand,
> > and we need some new concept ("isolation atoms"?) to represent what
> > isolation is possible
>
> Nope, containers are owned by users and serve a purpose beyond what
> you're assuming here. Also, there's 7.5 years of userspace tooling
> broken by this. I do remember we had discussions about merging groups,
> but what we have now is what we agreed on.
Well, quite - and I thought the reasoning that led to that design was
that groups would be a representation of isolation granularity, not
actual isolation. That allows them to be static, which as you say,
tools are likely to assume.
> > or
> >
> > 2) Groups represent potential isolation, with a higher level construct
> > representing current isolation. This could involve making vfio
> > containers essentially a wrapper around some more generic concept
> > ("isolation clusters"?) of a group of groups.
>
> Have fun. Containers and groups are exposed to userspace, so you're
> essentially suggesting to throw away all that support, for... I don't
> really know what.
No, I'm not proposing removing containers, just changing them from
being purely VFIO specific to being the VFIO front end to a generic
concept. Roughly speaking VFIO containers would be to "isolation
clusters" (or whatever) as VFIO groups are to IOMMU groups now.
> Groups are involved in IOMMU context, so dynamically
> changing what a group defines is hard.
Absolutely! That's why I don't want to, and why my conception of
groups was defined so that they didn't have to.
> Thus my suggestions that mixed
> or dynamic workloads could make use of soft remove and rediscovery for
> sub-hierarchies, unbinding and rebinding drivers such that we have the
> proper expectations of DMA context. Thanks,
Well, if we have to change groups, then that sounds like the sanest
available way to do it. But I'm not yet convinced that altering
groups makes sense rather than using a group-of-groups concept on top
of it - which would map to containers in the case of VFIO.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 3/5] powerpc/powernv: fix possible object reference leak
From: Wen Yang @ 2019-03-22 3:05 UTC (permalink / raw)
To: linux-kernel
Cc: wang.yi59, Nicholas Piggin, Mike Rapoport, Mahesh Salgaonkar,
Paul Mackerras, Andrew Morton, linuxppc-dev, Wen Yang,
Haren Myneni
In-Reply-To: <1553223955-7350-1-git-send-email-wen.yang99@zte.com.cn>
The call to of_find_node_by_path returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
Detected by coccinelle with the following warnings:
./arch/powerpc/platforms/powernv/opal.c:741:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 733, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Cc: Haren Myneni <haren@linux.vnet.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
arch/powerpc/platforms/powernv/opal.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b0eca1..d7736a5 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -738,6 +738,7 @@ static void opal_export_attrs(void)
kobj = kobject_create_and_add("exports", opal_kobj);
if (!kobj) {
pr_warn("kobject_create_and_add() of exports failed\n");
+ of_node_put(np);
return;
}
--
2.9.5
^ permalink raw reply related
* [PATCH 5/5] powerpc/8xx: fix possible object reference leak
From: Wen Yang @ 2019-03-22 3:05 UTC (permalink / raw)
To: linux-kernel; +Cc: wang.yi59, Paul Mackerras, linuxppc-dev, Wen Yang
In-Reply-To: <1553223955-7350-1-git-send-email-wen.yang99@zte.com.cn>
The call to of_find_compatible_node returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
irq_domain_add_linear also calls of_node_get to increase refcount,
so irq_domain will not be affected when it is released.
Detected by coccinelle with the following warnings:
./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 136, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Vitaly Bordug <vitb@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
arch/powerpc/platforms/8xx/pic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
index 8d5a25d..13d880b 100644
--- a/arch/powerpc/platforms/8xx/pic.c
+++ b/arch/powerpc/platforms/8xx/pic.c
@@ -155,6 +155,7 @@ int mpc8xx_pic_init(void)
ret = -ENOMEM;
goto out;
}
+ of_node_put(np);
return 0;
out:
--
2.9.5
^ permalink raw reply related
* [PATCH 4/5] powerpc/embedded6xx: fix possible object reference leak
From: Wen Yang @ 2019-03-22 3:05 UTC (permalink / raw)
To: linux-kernel; +Cc: wang.yi59, Paul Mackerras, linuxppc-dev, Wen Yang
In-Reply-To: <1553223955-7350-1-git-send-email-wen.yang99@zte.com.cn>
The call to of_find_compatible_node returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
Detected by coccinelle with the following warnings:
./arch/powerpc/platforms/embedded6xx/mvme5100.c:89:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 80, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
arch/powerpc/platforms/embedded6xx/mvme5100.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/embedded6xx/mvme5100.c b/arch/powerpc/platforms/embedded6xx/mvme5100.c
index 273dfa3..660654f4 100644
--- a/arch/powerpc/platforms/embedded6xx/mvme5100.c
+++ b/arch/powerpc/platforms/embedded6xx/mvme5100.c
@@ -86,6 +86,7 @@ static void __init mvme5100_pic_init(void)
cirq = irq_of_parse_and_map(cp, 0);
if (!cirq) {
pr_warn("mvme5100_pic_init: no cascade interrupt?\n");
+ of_node_put(cp);
return;
}
--
2.9.5
^ permalink raw reply related
* [PATCH 2/5] powerpc/83xx: fix possible object reference leak
From: Wen Yang @ 2019-03-22 3:05 UTC (permalink / raw)
To: linux-kernel
Cc: wang.yi59, Scott Wood, Paul Mackerras, linuxppc-dev, Wen Yang
In-Reply-To: <1553223955-7350-1-git-send-email-wen.yang99@zte.com.cn>
The call to of_find_node_by_name returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
Detected by coccinelle with the following warnings:
./arch/powerpc/platforms/83xx/km83xx.c:68:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 59, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Scott Wood <oss@buserror.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
arch/powerpc/platforms/83xx/km83xx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/platforms/83xx/km83xx.c b/arch/powerpc/platforms/83xx/km83xx.c
index d8642a4..11eea7c 100644
--- a/arch/powerpc/platforms/83xx/km83xx.c
+++ b/arch/powerpc/platforms/83xx/km83xx.c
@@ -65,6 +65,7 @@ static void quirk_mpc8360e_qe_enet10(void)
ret = of_address_to_resource(np_par, 0, &res);
if (ret) {
pr_warn("%s couldn;t map par_io registers\n", __func__);
+ of_node_put(np_par);
return;
}
--
2.9.5
^ permalink raw reply related
* [PATCH 1/5] powerpc/sparse: fix possible object reference leak
From: Wen Yang @ 2019-03-22 3:05 UTC (permalink / raw)
To: linux-kernel; +Cc: wang.yi59, Paul Mackerras, linuxppc-dev, Wen Yang
The call to of_find_node_by_path returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
Detected by coccinelle with the following warnings:
./arch/powerpc/platforms/pseries/pseries_energy.c:101:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 46, but without a corresponding object release within this function.
./arch/powerpc/platforms/pseries/pseries_energy.c:172:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 111, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
arch/powerpc/platforms/pseries/pseries_energy.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c
index 6ed2212..e3913e4 100644
--- a/arch/powerpc/platforms/pseries/pseries_energy.c
+++ b/arch/powerpc/platforms/pseries/pseries_energy.c
@@ -69,7 +69,7 @@ static u32 cpu_to_drc_index(int cpu)
of_read_drc_info_cell(&info, &value, &drc);
if (strncmp(drc.drc_type, "CPU", 3))
- goto err;
+ goto err_of_node_put;
if (thread_index < drc.last_drc_index)
break;
@@ -131,7 +131,7 @@ static int drc_index_to_cpu(u32 drc_index)
of_read_drc_info_cell(&info, &value, &drc);
if (strncmp(drc.drc_type, "CPU", 3))
- goto err;
+ goto err_of_node_put;
if (drc_index > drc.last_drc_index) {
cpu += drc.num_sequential_elems;
--
2.9.5
^ permalink raw reply related
* Re: [PATCH] arch/powerpc/crypto/crc-vpmsum_test: Use cheaper random numbers for self-test
From: Daniel Axtens @ 2019-03-22 2:13 UTC (permalink / raw)
To: George Spelvin, linuxppc-dev; +Cc: George Spelvin, Paul Mackerras, Herbert Xu
In-Reply-To: <201903211042.x2LAgMp3003053@sdf.org>
Hi George,
> This code was filling a 64K buffer from /dev/urandom in order to
> compute a CRC over (on average half of) it by two different methods,
> comparing the CRCs, and repeating.
>
> This is not a remotely security-critical application, so use the far
> faster and cheaper prandom_u32() generator.
>
I've had a quick look at the prandom_u32 generator and I agree that it's
suitable for this.
> And, while we're at it, only fill as much of the buffer as we plan to use.
This also looks good to me.
Acked-by: Daniel Axtens <dja@axtens.net>
Regards,
Daniel
>
> Signed-off-by: George Spelvin <lkml@sdf.org>
> Cc: Daniel Axtens <dja@axtens.net>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/crypto/crc-vpmsum_test.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/crypto/crc-vpmsum_test.c b/arch/powerpc/crypto/crc-vpmsum_test.c
> index 0153a9c6f4af..98ea4f4d3dde 100644
> --- a/arch/powerpc/crypto/crc-vpmsum_test.c
> +++ b/arch/powerpc/crypto/crc-vpmsum_test.c
> @@ -78,16 +78,12 @@ static int __init crc_test_init(void)
>
> pr_info("crc-vpmsum_test begins, %lu iterations\n", iterations);
> for (i=0; i<iterations; i++) {
> - size_t len, offset;
> + size_t offset = prandom_u32_max(16);
> + size_t len = prandom_u32_max(MAX_CRC_LENGTH);
>
> - get_random_bytes(data, MAX_CRC_LENGTH);
> - get_random_bytes(&len, sizeof(len));
> - get_random_bytes(&offset, sizeof(offset));
> -
> - len %= MAX_CRC_LENGTH;
> - offset &= 15;
> if (len <= offset)
> continue;
> + prandom_bytes(data, len);
> len -= offset;
>
> crypto_shash_update(crct10dif_shash, data+offset, len);
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
From: Michael Ellerman @ 2019-03-22 0:35 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: npiggin
In-Reply-To: <7fad467c-1e63-02c8-636e-7f8ab456f76c@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 20/03/2019 à 13:57, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>>>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>
>>>> This patch implements a framework for Kernel Userspace Access
>>>> Protection.
>>>>
>>>> Then subarches will have the possibility to provide their own
>>>> implementation by providing setup_kuap() and
>>>> allow/prevent_user_access().
>>>>
>>>> Some platforms will need to know the area accessed and whether it is
>>>> accessed from read, write or both. Therefore source, destination and
>>>> size and handed over to the two functions.
>>>>
>>>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>>>> read/write wrappers. Drop the 32-bit code for now until we have an
>>>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>>>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>> ---
>>>> v5: Futex ops need read/write so use allow_user_acccess() there.
>>>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>>>> Allow subarch to override allow_read/write_from/to_user().
>>>
>>> Those little helpers that will just call allow_user_access() when
>>> distinct read/write handling is not performed looks overkill to me.
>>>
>>> Can't the subarch do it by itself based on the nullity of from/to ?
>>>
>>> static inline void allow_user_access(void __user *to, const void __user
>>> *from,
>>> unsigned long size)
>>> {
>>> if (to & from)
>>> set_kuap(0);
>>> else if (to)
>>> set_kuap(AMR_KUAP_BLOCK_READ);
>>> else if (from)
>>> set_kuap(AMR_KUAP_BLOCK_WRITE);
>>> }
>>
>> You could implement it that way, but it reads better at the call sites
>> if we have:
>>
>> allow_write_to_user(uaddr, sizeof(*uaddr));
>> vs:
>> allow_user_access(uaddr, NULL, sizeof(*uaddr));
>>
>> So I'm inclined to keep them. It should all end up inlined and generate
>> the same code at the end of the day.
>>
>
> I was not suggesting to completly remove allow_write_to_user(), I fully
> agree that it reads better at the call sites.
>
> I was just thinking that allow_write_to_user() could remain generic and
> call the subarch specific allow_user_access() instead of making multiple
> subarch's allow_write_to_user()
Yep OK I see what you mean.
Your suggestion above should work, and involves the least amount of
ifdefs and so on.
I'll try and get time to post a v6.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: vmlinux.lds: Drop Binutils 2.18 workarounds
From: Michael Ellerman @ 2019-03-22 0:22 UTC (permalink / raw)
To: Segher Boessenkool, Joel Stanley; +Cc: linuxppc-dev
In-Reply-To: <20190321233420.GD3969@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Mar 21, 2019 at 11:02:53AM +1030, Joel Stanley wrote:
>> Segher added some workarounds for GCC 4.2 and bintuils 2.18. We now set
>> 4.6 and 2.20 as the minimum, so they can be dropped.
>
> It was a bug in binutils _before_ 2.18, only seen by people using GCC
> _before_ 4.2.
>
> It's all ancient history by now, and good riddance :-)
>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>
> Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
Thanks.
I updated the change log slightly:
powerpc/vmlinux.lds: Drop binutils < 2.18 workarounds
Segher added some workarounds for binutils < 2.18 and GCC < 4.2. We
now set GCC 4.6 and binutils 2.20 as the minimum, so the workarounds
can be dropped.
This is mostly a revert of c69cccc95fe4 ("powerpc: Fix build bug with
binutils < 2.18 and GCC < 4.2").
cheers
^ permalink raw reply
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Thiago Jung Bauermann @ 2019-03-22 0:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <20190320171027-mutt-send-email-mst@kernel.org>
Michael S. Tsirkin <mst@redhat.com> writes:
> On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote:
>> >> Another way of looking at this issue which also explains our reluctance
>> >> is that the only difference between a secure guest and a regular guest
>> >> (at least regarding virtio) is that the former uses swiotlb while the
>> >> latter doens't.
>> >
>> > But swiotlb is just one implementation. It's a guest internal thing. The
>> > issue is that memory isn't host accessible.
>>
>> >From what I understand of the ACCESS_PLATFORM definition, the host will
>> only ever try to access memory addresses that are supplied to it by the
>> guest, so all of the secure guest memory that the host cares about is
>> accessible:
>>
>> If this feature bit is set to 0, then the device has same access to
>> memory addresses supplied to it as the driver has. In particular,
>> the device will always use physical addresses matching addresses
>> used by the driver (typically meaning physical addresses used by the
>> CPU) and not translated further, and can access any address supplied
>> to it by the driver. When clear, this overrides any
>> platform-specific description of whether device access is limited or
>> translated in any way, e.g. whether an IOMMU may be present.
>>
>> All of the above is true for POWER guests, whether they are secure
>> guests or not.
>>
>> Or are you saying that a virtio device may want to access memory
>> addresses that weren't supplied to it by the driver?
>
> Your logic would apply to IOMMUs as well. For your mode, there are
> specific encrypted memory regions that driver has access to but device
> does not. that seems to violate the constraint.
Right, if there's a pre-configured 1:1 mapping in the IOMMU such that
the device can ignore the IOMMU for all practical purposes I would
indeed say that the logic would apply to IOMMUs as well. :-)
I guess I'm still struggling with the purpose of signalling to the
driver that the host may not have access to memory addresses that it
will never try to access.
>> >> And from the device's point of view they're
>> >> indistinguishable. It can't tell one guest that is using swiotlb from
>> >> one that isn't. And that implies that secure guest vs regular guest
>> >> isn't a virtio interface issue, it's "guest internal affairs". So
>> >> there's no reason to reflect that in the feature flags.
>> >
>> > So don't. The way not to reflect that in the feature flags is
>> > to set ACCESS_PLATFORM. Then you say *I don't care let platform device*.
>> >
>> >
>> > Without ACCESS_PLATFORM
>> > virtio has a very specific opinion about the security of the
>> > device, and that opinion is that device is part of the guest
>> > supervisor security domain.
>>
>> Sorry for being a bit dense, but not sure what "the device is part of
>> the guest supervisor security domain" means. In powerpc-speak,
>> "supervisor" is the operating system so perhaps that explains my
>> confusion. Are you saying that without ACCESS_PLATFORM, the guest
>> considers the host to be part of the guest operating system's security
>> domain?
>
> I think so. The spec says "device has same access as driver".
Ok, makes sense.
>> If so, does that have any other implication besides "the host
>> can access any address supplied to it by the driver"? If that is the
>> case, perhaps the definition of ACCESS_PLATFORM needs to be amended to
>> include that information because it's not part of the current
>> definition.
>>
>> >> > But the name "sev_active" makes me scared because at least AMD guys who
>> >> > were doing the sensible thing and setting ACCESS_PLATFORM
>> >>
>> >> My understanding is, AMD guest-platform knows in advance that their
>> >> guest will run in secure mode and hence sets the flag at the time of VM
>> >> instantiation. Unfortunately we dont have that luxury on our platforms.
>> >
>> > Well you do have that luxury. It looks like that there are existing
>> > guests that already acknowledge ACCESS_PLATFORM and you are not happy
>> > with how that path is slow. So you are trying to optimize for
>> > them by clearing ACCESS_PLATFORM and then you have lost ability
>> > to invoke DMA API.
>> >
>> > For example if there was another flag just like ACCESS_PLATFORM
>> > just not yet used by anyone, you would be all fine using that right?
>>
>> Yes, a new flag sounds like a great idea. What about the definition
>> below?
>>
>> VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as
>> VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the
>> exception that the IOMMU is explicitly defined to be off or bypassed
>> when accessing memory addresses supplied to the device by the
>> driver. This flag should be set by the guest if offered, but to
>> allow for backward-compatibility device implementations allow for it
>> to be left unset by the guest. It is an error to set both this flag
>> and VIRTIO_F_ACCESS_PLATFORM.
>
> It looks kind of narrow but it's an option.
Great!
> I wonder how we'll define what's an iommu though.
Hm, it didn't occur to me it could be an issue. I'll try.
> Another idea is maybe something like virtio-iommu?
You mean, have legacy guests use virtio-iommu to request an IOMMU
bypass? If so, it's an interesting idea for new guests but it doesn't
help with guests that are out today in the field, which don't have A
virtio-iommu driver.
>> > Is there any justification to doing that beyond someone putting
>> > out slow code in the past?
>>
>> The definition of the ACCESS_PLATFORM flag is generic and captures the
>> notion of memory access restrictions for the device. Unfortunately, on
>> powerpc pSeries guests it also implies that the IOMMU is turned on
>
> IIUC that's really because on pSeries IOMMU is *always* turned on.
> Platform has no way to say what you want it to say
> which is bypass the iommu for the specific device.
Yes, that's correct. pSeries guests running on KVM are in a gray area
where theoretically they use an IOMMU but in practice KVM ignores it.
It's unfortunate but it's the reality on the ground today. :-/
--
Thiago Jung Bauermann
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH] powerpc: vmlinux.lds: Drop Binutils 2.18 workarounds
From: Segher Boessenkool @ 2019-03-21 23:34 UTC (permalink / raw)
To: Joel Stanley; +Cc: linuxppc-dev
In-Reply-To: <20190321003253.22100-1-joel@jms.id.au>
On Thu, Mar 21, 2019 at 11:02:53AM +1030, Joel Stanley wrote:
> Segher added some workarounds for GCC 4.2 and bintuils 2.18. We now set
> 4.6 and 2.20 as the minimum, so they can be dropped.
It was a bug in binutils _before_ 2.18, only seen by people using GCC
_before_ 4.2.
It's all ancient history by now, and good riddance :-)
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Segher Boessenkool <segher@kernel.crashing.org>
Segher
^ permalink raw reply
* Re: [PATCH 1/4] add generic builtin command line
From: Andrew Morton @ 2019-03-21 22:15 UTC (permalink / raw)
To: Daniel Walker
Cc: Maksym Kokhan, linux-kernel, Rob Herring, Paul Mackerras,
xe-linux-external, Daniel Walker, linuxppc-dev
In-Reply-To: <20190321151308.yt6uc3mxgppm5zko@zorba>
On Thu, 21 Mar 2019 08:13:08 -0700 Daniel Walker <danielwa@cisco.com> wrote:
> > The patches (or some version of them) are already in linux-next,
> > which messes me up. I'll disable them for now.
>
> Those are from my tree, but I remove them when you picked up the series. The
> next linux-next should not have them.
Yup, thanks, all looks good now.
^ permalink raw reply
* Re: powerpc32 boot crash in 5.1-rc1
From: LEROY Christophe @ 2019-03-21 21:16 UTC (permalink / raw)
To: Meelis Roos; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <91966c00-b09f-7e07-89cb-04ac208d5dc1@linux.ee>
Meelis Roos <mroos@linux.ee> a écrit :
> While 5.0.0 worked fine on my PowerMac G4, 5.0 + git (unknown rev as
> of Mar 13), 5.0.0-11520-gf261c4e and todays git all fail to boot.
>
> The problem seems to be in page fault handler in load_elf_binary()
> of init process.
The patch at https://patchwork.ozlabs.org/patch/1053385/ should fix it
Christophe
>
> Two different screenshots are attached (let's see if they come through).
>
> --
> Meelis Roos <mroos@linux.ee>
^ permalink raw reply
* powerpc32 boot crash in 5.1-rc1
From: Meelis Roos @ 2019-03-21 20:25 UTC (permalink / raw)
To: linuxppc-dev, linux-kernel
In-Reply-To: <91966c00-b09f-7e07-89cb-04ac208d5dc1@linux.ee>
While 5.0.0 worked fine on my PowerMac G4, 5.0 + git (unknown rev as of Mar 13),
5.0.0-11520-gf261c4e and todays git all fail to boot.
The problem seems to be in page fault handler in load_elf_binary() of init process.
Two different screenshots are at http://kodu.ut.ee/~mroos/powerpc-boot-hang-1.jpg and
http://kodu.ut.ee/~mroos/powerpc-boot-hang-1.jpg .
--
Meelis Roos <mroos@linux.ee>
^ permalink raw reply
* [PATCH] arch/powerpc/crypto/crc-vpmsum_test: Use cheaper random numbers for self-test
From: George Spelvin @ 2019-03-21 10:42 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev; +Cc: George Spelvin, Paul Mackerras, Herbert Xu
This code was filling a 64K buffer from /dev/urandom in order to
compute a CRC over (on average half of) it by two different methods,
comparing the CRCs, and repeating.
This is not a remotely security-critical application, so use the far
faster and cheaper prandom_u32() generator.
And, while we're at it, only fill as much of the buffer as we plan to use.
Signed-off-by: George Spelvin <lkml@sdf.org>
Cc: Daniel Axtens <dja@axtens.net>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/crypto/crc-vpmsum_test.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/crypto/crc-vpmsum_test.c b/arch/powerpc/crypto/crc-vpmsum_test.c
index 0153a9c6f4af..98ea4f4d3dde 100644
--- a/arch/powerpc/crypto/crc-vpmsum_test.c
+++ b/arch/powerpc/crypto/crc-vpmsum_test.c
@@ -78,16 +78,12 @@ static int __init crc_test_init(void)
pr_info("crc-vpmsum_test begins, %lu iterations\n", iterations);
for (i=0; i<iterations; i++) {
- size_t len, offset;
+ size_t offset = prandom_u32_max(16);
+ size_t len = prandom_u32_max(MAX_CRC_LENGTH);
- get_random_bytes(data, MAX_CRC_LENGTH);
- get_random_bytes(&len, sizeof(len));
- get_random_bytes(&offset, sizeof(offset));
-
- len %= MAX_CRC_LENGTH;
- offset &= 15;
if (len <= offset)
continue;
+ prandom_bytes(data, len);
len -= offset;
crypto_shash_update(crct10dif_shash, data+offset, len);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH kernel RFC 2/2] vfio-pci-nvlink2: Implement interconnect isolation
From: Alex Williamson @ 2019-03-21 18:19 UTC (permalink / raw)
To: David Gibson
Cc: Jose Ricardo Ziviani, Alexey Kardashevskiy,
Daniel Henrique Barboza, kvm-ppc, Piotr Jaroszynski,
Leonardo Augusto Guimarães Garcia, linuxppc-dev
In-Reply-To: <20190320235600.GB2501@umbus.fritz.box>
On Thu, 21 Mar 2019 10:56:00 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Mar 20, 2019 at 01:09:08PM -0600, Alex Williamson wrote:
> > On Wed, 20 Mar 2019 15:38:24 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >
> > > On Tue, Mar 19, 2019 at 10:36:19AM -0600, Alex Williamson wrote:
> > > > On Fri, 15 Mar 2019 19:18:35 +1100
> > > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> > > >
> > > > > The NVIDIA V100 SXM2 GPUs are connected to the CPU via PCIe links and
> > > > > (on POWER9) NVLinks. In addition to that, GPUs themselves have direct
> > > > > peer to peer NVLinks in groups of 2 to 4 GPUs. At the moment the POWERNV
> > > > > platform puts all interconnected GPUs to the same IOMMU group.
> > > > >
> > > > > However the user may want to pass individual GPUs to the userspace so
> > > > > in order to do so we need to put them into separate IOMMU groups and
> > > > > cut off the interconnects.
> > > > >
> > > > > Thankfully V100 GPUs implement an interface to do by programming link
> > > > > disabling mask to BAR0 of a GPU. Once a link is disabled in a GPU using
> > > > > this interface, it cannot be re-enabled until the secondary bus reset is
> > > > > issued to the GPU.
> > > > >
> > > > > This defines a reset_done() handler for V100 NVlink2 device which
> > > > > determines what links need to be disabled. This relies on presence
> > > > > of the new "ibm,nvlink-peers" device tree property of a GPU telling which
> > > > > PCI peers it is connected to (which includes NVLink bridges or peer GPUs).
> > > > >
> > > > > This does not change the existing behaviour and instead adds
> > > > > a new "isolate_nvlink" kernel parameter to allow such isolation.
> > > > >
> > > > > The alternative approaches would be:
> > > > >
> > > > > 1. do this in the system firmware (skiboot) but for that we would need
> > > > > to tell skiboot via an additional OPAL call whether or not we want this
> > > > > isolation - skiboot is unaware of IOMMU groups.
> > > > >
> > > > > 2. do this in the secondary bus reset handler in the POWERNV platform -
> > > > > the problem with that is at that point the device is not enabled, i.e.
> > > > > config space is not restored so we need to enable the device (i.e. MMIO
> > > > > bit in CMD register + program valid address to BAR0) in order to disable
> > > > > links and then perhaps undo all this initialization to bring the device
> > > > > back to the state where pci_try_reset_function() expects it to be.
> > > >
> > > > The trouble seems to be that this approach only maintains the isolation
> > > > exposed by the IOMMU group when vfio-pci is the active driver for the
> > > > device. IOMMU groups can be used by any driver and the IOMMU core is
> > > > incorporating groups in various ways.
> > >
> > > I don't think that reasoning is quite right. An IOMMU group doesn't
> > > necessarily represent devices which *are* isolated, just devices which
> > > *can be* isolated. There are plenty of instances when we don't need
> > > to isolate devices in different IOMMU groups: passing both groups to
> > > the same guest or userspace VFIO driver for example, or indeed when
> > > both groups are owned by regular host kernel drivers.
> > >
> > > In at least some of those cases we also don't want to isolate the
> > > devices when we don't have to, usually for performance reasons.
> >
> > I see IOMMU groups as representing the current isolation of the device,
> > not just the possible isolation. If there are ways to break down that
> > isolation then ideally the group would be updated to reflect it. The
> > ACS disable patches seem to support this, at boot time we can choose to
> > disable ACS at certain points in the topology to favor peer-to-peer
> > performance over isolation. This is then reflected in the group
> > composition, because even though ACS *can be* enabled at the given
> > isolation points, it's intentionally not with this option. Whether or
> > not a given user who owns multiple devices needs that isolation is
> > really beside the point, the user can choose to connect groups via IOMMU
> > mappings or reconfigure the system to disable ACS and potentially more
> > direct routing. The IOMMU groups are still accurately reflecting the
> > topology and IOMMU based isolation.
>
> Huh, ok, I think we need to straighten this out. Thinking of iommu
> groups as possible rather than potential isolation was a conscious
possible ~= potential
> decision on my part when we were first coming up with them. The
> rationale was that that way iommu groups could be static for the
> lifetime of boot, with more dynamic isolation state layered on top.
>
> Now, that was based on analogy with PAPR's concept of "Partitionable
> Endpoints" which are decided by firmware before boot. However, I
> think it makes sense in other contexts too: if iommu groups represent
> current isolation, then we need some other way to advertise possible
> isolation - otherwise how will the admin (and/or tools) know how it
> can configure the iommu groups.
>
> VFIO already has the container, which represents explicitly a "group
> of groups" that we don't care to isolate from each other. I don't
> actually know what other uses of the iommu group infrastructure we
> have at present and how they treat them.
s/that we don't care to isolate/that the user doesn't care to isolate/
Though even that is not necessarily accurate, the container represents
a shared IOMMU context, that context doesn't necessarily include
mappings between devices, so the device can still be isolated *from
each other*.
> So, if we now have dynamically reconfigurable groups which are a
> departure from that design, how can we go from here trying to bring
> things back to consistency.
We don't currently have dynamically reconfigurable groups, I think the
question is how do we introduce dynamically configurable groups within
the existing design, because whatever intentions were 7.5 years ago is
rather irrelevant now.
VFIO groups are mapped directly to IOMMU groups. IOMMU groups are the
smallest set of devices which can be considered isolated from other
sets of devices (not potentially, but as configured). VFIO groups are
the unit of ownership to userspace, therefore a VFIO group must be
(currently) isolated from other groups. The user owns and manages the
IOMMU context for one or more groups via a container. A container
allows efficiency in managing a shared IOVA space between groups.
> > > > So, if there's a device specific
> > > > way to configure the isolation reported in the group, which requires
> > > > some sort of active management against things like secondary bus
> > > > resets, then I think we need to manage it above the attached endpoint
> > > > driver.
> > >
> > > The problem is that above the endpoint driver, we don't actually have
> > > enough information about what should be isolated. For VFIO we want to
> > > isolate things if they're in different containers, for most regular
> > > host kernel drivers we don't need to isolate at all (although we might
> > > as well when it doesn't have a cost).
> >
> > This idea that we only want to isolate things if they're in different
> > containers is bogus, imo. There are performance reasons why we might
> > not want things isolated, but there are also address space reasons why
> > we do. If there are direct routes between devices, the user needs to
> > be aware of the IOVA pollution, if we maintain singleton groups, they
> > don't. Granted we don't really account for this well in most
> > userspaces and fumble through it by luck of the address space layout
> > and lack of devices really attempting peer to peer access.
>
> I don't really follow what you're saying here.
If the lack of isolation between devices includes peer-to-peer channels
then the MMIO of the devices within the group pollutes the IOVA space
of the container.
> > For in-kernel users, we're still theoretically trying to isolate
> > devices such that they have restricted access to only the resources
> > they need. Disabling things like ACS in the topology reduces that
> > isolation. AFAICT, most users don't really care about that degree of
> > isolation, so they run with iommu=pt for native driver performance
> > while still having the IOMMU available for isolation use cases running
> > in parallel. We don't currently have support for on-demand enabling
> > isolation.
>
> Ok.
>
> > > The host side nVidia GPGPU
> > > drivers also won't want to isolate the (host owned) NVLink devices
> > > from each other, since they'll want to use the fast interconnects
> >
> > This falls into the same mixed use case scenario above where we don't
> > really have a good solution today. Things like ACS are dynamically
> > configurable, but we don't expose any interfaces to let drivers or
> > users change it (aside from setpci, which we don't account for
> > dynamically). We assume a simplistic model where if you want IOMMU,
> > then you must also want the maximum configurable isolation.
> > Dynamically changing routing is not necessarily the most foolproof
> > thing either with potentially in-flight transactions and existing DMA
> > mappings, which is why I've suggested a couple times that perhaps we
> > could do a software hot-unplug of a sub-hierarchy, muck with isolation
> > at the remaining node, then re-discover the removed devices.
>
> Ok, so I feel like we need to go fully one way or the other. Either:
>
> 1) Groups represent current isolation status, in which case we
> deprecate vfio containers in favour of fusing groups beforehand,
> and we need some new concept ("isolation atoms"?) to represent what
> isolation is possible
Nope, containers are owned by users and serve a purpose beyond what
you're assuming here. Also, there's 7.5 years of userspace tooling
broken by this. I do remember we had discussions about merging groups,
but what we have now is what we agreed on.
> or
>
> 2) Groups represent potential isolation, with a higher level construct
> representing current isolation. This could involve making vfio
> containers essentially a wrapper around some more generic concept
> ("isolation clusters"?) of a group of groups.
Have fun. Containers and groups are exposed to userspace, so you're
essentially suggesting to throw away all that support, for... I don't
really know what. Groups are involved in IOMMU context, so dynamically
changing what a group defines is hard. Thus my suggestions that mixed
or dynamic workloads could make use of soft remove and rediscovery for
sub-hierarchies, unbinding and rebinding drivers such that we have the
proper expectations of DMA context. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH] kmemleak: powerpc: skip scanning holes in the .bss section
From: Qian Cai @ 2019-03-21 18:17 UTC (permalink / raw)
To: Catalin Marinas, Andrew Morton
Cc: linux-mm, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <20190321171917.62049-1-catalin.marinas@arm.com>
On Thu, 2019-03-21 at 17:19 +0000, Catalin Marinas wrote:
> The commit 2d4f567103ff ("KVM: PPC: Introduce kvm_tmp framework") adds
> kvm_tmp[] into the .bss section and then free the rest of unused spaces
> back to the page allocator.
>
> kernel_init
> kvm_guest_init
> kvm_free_tmp
> free_reserved_area
> free_unref_page
> free_unref_page_prepare
>
> With DEBUG_PAGEALLOC=y, it will unmap those pages from kernel. As the
> result, kmemleak scan will trigger a panic when it scans the .bss
> section with unmapped pages.
>
> This patch creates dedicated kmemleak objects for the .data, .bss and
> potentially .data..ro_after_init sections to allow partial freeing via
> the kmemleak_free_part() in the powerpc kvm_free_tmp() function.
>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> Reported-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Tested-by: Qian Cai <cai@lca.pw>
^ permalink raw reply
* [PATCH] kmemleak: powerpc: skip scanning holes in the .bss section
From: Catalin Marinas @ 2019-03-21 17:19 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, kvm-ppc, linux-mm, Qian Cai, linuxppc-dev
The commit 2d4f567103ff ("KVM: PPC: Introduce kvm_tmp framework") adds
kvm_tmp[] into the .bss section and then free the rest of unused spaces
back to the page allocator.
kernel_init
kvm_guest_init
kvm_free_tmp
free_reserved_area
free_unref_page
free_unref_page_prepare
With DEBUG_PAGEALLOC=y, it will unmap those pages from kernel. As the
result, kmemleak scan will trigger a panic when it scans the .bss
section with unmapped pages.
This patch creates dedicated kmemleak objects for the .data, .bss and
potentially .data..ro_after_init sections to allow partial freeing via
the kmemleak_free_part() in the powerpc kvm_free_tmp() function.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
Posting as a proper patch following the inlined one here:
http://lkml.kernel.org/r/20190320181656.GB38229@arrakis.emea.arm.com
Changes from the above:
- Added comment to the powerpc kmemleak_free_part() call
- Only register the .data..ro_after_init in kmemleak if not contained
within the .data sections (which seems to be the case for lots of
architectures)
I preserved part of Qian's original commit message but changed the
author since I rewrote the patch.
arch/powerpc/kernel/kvm.c | 7 +++++++
mm/kmemleak.c | 16 +++++++++++-----
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
index 683b5b3805bd..cd381e2291df 100644
--- a/arch/powerpc/kernel/kvm.c
+++ b/arch/powerpc/kernel/kvm.c
@@ -22,6 +22,7 @@
#include <linux/kvm_host.h>
#include <linux/init.h>
#include <linux/export.h>
+#include <linux/kmemleak.h>
#include <linux/kvm_para.h>
#include <linux/slab.h>
#include <linux/of.h>
@@ -712,6 +713,12 @@ static void kvm_use_magic_page(void)
static __init void kvm_free_tmp(void)
{
+ /*
+ * Inform kmemleak about the hole in the .bss section since the
+ * corresponding pages will be unmapped with DEBUG_PAGEALLOC=y.
+ */
+ kmemleak_free_part(&kvm_tmp[kvm_tmp_index],
+ ARRAY_SIZE(kvm_tmp) - kvm_tmp_index);
free_reserved_area(&kvm_tmp[kvm_tmp_index],
&kvm_tmp[ARRAY_SIZE(kvm_tmp)], -1, NULL);
}
diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index 707fa5579f66..6c318f5ac234 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -1529,11 +1529,6 @@ static void kmemleak_scan(void)
}
rcu_read_unlock();
- /* data/bss scanning */
- scan_large_block(_sdata, _edata);
- scan_large_block(__bss_start, __bss_stop);
- scan_large_block(__start_ro_after_init, __end_ro_after_init);
-
#ifdef CONFIG_SMP
/* per-cpu sections scanning */
for_each_possible_cpu(i)
@@ -2071,6 +2066,17 @@ void __init kmemleak_init(void)
}
local_irq_restore(flags);
+ /* register the data/bss sections */
+ create_object((unsigned long)_sdata, _edata - _sdata,
+ KMEMLEAK_GREY, GFP_ATOMIC);
+ create_object((unsigned long)__bss_start, __bss_stop - __bss_start,
+ KMEMLEAK_GREY, GFP_ATOMIC);
+ /* only register .data..ro_after_init if not within .data */
+ if (__start_ro_after_init < _sdata || __end_ro_after_init > _edata)
+ create_object((unsigned long)__start_ro_after_init,
+ __end_ro_after_init - __start_ro_after_init,
+ KMEMLEAK_GREY, GFP_ATOMIC);
+
/*
* This is the point where tracking allocations is safe. Automatic
* scanning is started during the late initcall. Add the early logged
^ 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