* Re: [PATCH v5 05/11] kselftest: arm64: mangle_pstate_ssbs_regs
From: Cristian Marussi @ 2019-09-09 15:51 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114836.GV27757@arm.com>
Hi
On 04/09/2019 12:48, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:26pm +0100, Cristian Marussi wrote:
>> Add a simple mangle testcase which messes with the ucontext_t from within
>> the signal handler, trying to set the PSTATE SSBS bit.
>> Expect SIGILL if SSBS feature is unsupported or that, on test PASS, the
>> value set in PSTATE.SSBS in the signal frame is preserved by sigreturn.
>>
>> Additionally, in order to support this test specific needs:
>> - extend signal testing framework to allow the definition of a custom per
>> test initialization function to be run at the end of test setup.
>> - introduced a set_regval() helper to set system register values in a
>> toolchain independent way.
>> - introduce also a new common utility function: get_current_context()
>> which can be used to grab a ucontext without the help of libc, and
>> detect if such ucontext has been actively used to jump back into it.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit message
>> - missing include signal.h
>> - added .init per-test init-func
>> - added set_regval() helper
>> - added SSBS clear to 0 custom .init function
>> - removed volatile qualifier associated with sig_atomic_t data
>> - added dsb inside handler to ensure the writes related to the
>> grabbed ucontext have completed
>> - added test description
>> ---
>> .../selftests/arm64/signal/test_signals.h | 20 +++-
>> .../arm64/signal/test_signals_utils.c | 98 +++++++++++++++++++
>> .../arm64/signal/test_signals_utils.h | 2 +
>> .../testcases/mangle_pstate_ssbs_regs.c | 69 +++++++++++++
>> 4 files changed, 184 insertions(+), 5 deletions(-)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h
>> index a1cf69997604..0767e27fbe78 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals.h
>> @@ -27,6 +27,14 @@
>> : "memory"); \
>> }
>>
>> +#define set_regval(regname, in) \
>> +{ \
>> + asm volatile("msr " __stringify(regname) ", %0" \
>> + : \
>> + : "r" (in) \
>> + : "memory"); \
>> +}
>> +
>> /* Regs encoding and masks naming copied in from sysreg.h */
>> #define SYS_ID_AA64MMFR1_EL1 S3_0_C0_C7_1 /* MRS Emulated */
>> #define SYS_ID_AA64MMFR2_EL1 S3_0_C0_C7_2 /* MRS Emulated */
>> @@ -89,12 +97,16 @@ struct tdescr {
>> /* optional sa_flags for the installed handler */
>> int sa_flags;
>> ucontext_t saved_uc;
>> -
>> - /* a custom setup function to be called before test starts */
>> + /* used by get_current_ctx() */
>> + size_t live_sz;
>> + ucontext_t *live_uc;
>> + sig_atomic_t live_uc_valid;
>> + /* a custom setup: called alternatively to default_setup */
>> int (*setup)(struct tdescr *td);
>> + /* a custom init: called by default test initialization */
>> + void (*init)(struct tdescr *td);
>> /* a custom cleanup function called before test exits */
>> void (*cleanup)(struct tdescr *td);
>> -
>> /* an optional function to be used as a trigger for test starting */
>> int (*trigger)(struct tdescr *td);
>> /*
>> @@ -102,10 +114,8 @@ struct tdescr {
>> * presence of the trigger function above; this is mandatory
>> */
>> int (*run)(struct tdescr *td, siginfo_t *si, ucontext_t *uc);
>> -
>> /* an optional function for custom results' processing */
>> void (*check_result)(struct tdescr *td);
>> -
>> void *priv;
>> };
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> index e2a5f37e6ad3..c6fdcb23f246 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> @@ -11,12 +11,16 @@
>> #include <linux/auxvec.h>
>> #include <ucontext.h>
>>
>> +#include <asm/unistd.h>
>> +
>> #include "test_signals.h"
>> #include "test_signals_utils.h"
>> #include "testcases/testcases.h"
>>
>> extern struct tdescr *current;
>>
>> +static int sig_copyctx = SIGUSR2;
>> +
>> static char *feats_store[FMAX_END] = {
>> " SSBS ",
>> " PAN ",
>> @@ -43,6 +47,81 @@ static inline char *feats_to_string(unsigned long feats)
>> return feats_string;
>> }
>>
>> +/*
>> + * Obtaining a valid and full-blown ucontext_t from userspace is tricky:
>> + * libc getcontext does() not save all the regs and messes with some of
>> + * them (pstate value in particular is not reliable).
>> + * Here we use a service signal to grab the ucontext_t from inside a
>> + * dedicated signal handler, since there, it is populated by Kernel
>> + * itself in setup_sigframe(). The grabbed context is then stored and
>> + * made available in td->live_uc.
>> + *
>> + * Anyway this function really serves a dual purpose:
>> + *
>> + * 1. grab a valid sigcontext into td->live_uc for result analysis: in
>> + * such case it returns 1.
>> + *
>> + * 2. detect if somehow a previously grabbed live_uc context has been
>> + * used actively with a sigreturn: in such a case the execution would have
>> + * magically resumed in the middle of the function itself (seen_already==1):
>> + * in such a case return 0, since in fact we have not just simply grabbed
>> + * the context.
>> + *
>> + * This latter case is useful to detect when a fake_sigreturn test-case has
>> + * unexpectedly survived without hittig a SEGV.
>> + */
>> +bool get_current_context(struct tdescr *td, ucontext_t *dest_uc)
>> +{
>> + static sig_atomic_t seen_already;
>> +
>> + assert(td && dest_uc);
>> + /* it's a genuine invocation..reinit */
>> + seen_already = 0;
>> + td->live_uc_valid = 0;
>> + td->live_sz = sizeof(*dest_uc);
>> + memset(dest_uc, 0x00, td->live_sz);
>> + td->live_uc = dest_uc;
>> + /*
>> + * Grab ucontext_t triggering a signal...
>> + * ASM equivalent of raise(sig_copyctx);
>> + *
>> + * Note that:
>> + * - live_uc_valid is declared sig_atomic_t in struct tdescr
>> + * since it will be changed inside the sig_copyctx handler
>> + * - the kill() syscall invocation returns only after any possible
>> + * registered signal handler for the invoked signal has returned,
>> + * so that live_uc_valid flag is surely up to date when this
>> + * function return it.
>> + * - the additional 'memory' clobber is there to avoid possible
>> + * compiler's assumption on live_uc_valid, seen-already and
>> + * the content pointed by dest_uc, which are all changed inside
>> + * the signal handler, without resorting to the volatile qualifier
>> + * (and keeping quiet checkpatch.pl)
>> + */
>> + asm volatile ("mov x8, %0\n\t"
>> + "svc #0\n\t"
>> + "mov x1, %1\n\t"
>> + "mov x8, %2\n\t"
>> + "svc #0"
>> + :
>> + : "i" (__NR_getpid), "r" (sig_copyctx), "i" (__NR_kill)
>> + : "x1", "x8", "x0", "memory");
>> + /*
>> + * If we get here with seen_already==1 it implies the td->live_uc
>> + * context has been used to get back here....this probably means
>> + * a test has failed to cause a SEGV...anyway the live_uc has not
>> + * just been acquired...so return 0
>> + */
>> + if (seen_already) {
>> + fprintf(stdout,
>> + "Successful sigreturn detected: live_uc is stale !\n");
>> + return 0;
>> + }
>> + seen_already = 1;
>> +
>> + return td->live_uc_valid;
>> +}
>> +
>> static void unblock_signal(int signum)
>> {
>> sigset_t sset;
>> @@ -124,6 +203,17 @@ static void default_handler(int signum, siginfo_t *si, void *uc)
>> * to terminate immediately exiting straight away
>> */
>> default_result(current, 1);
>> + } else if (signum == sig_copyctx && current->live_uc) {
>> + memcpy(current->live_uc, uc, current->live_sz);
>> + ASSERT_GOOD_CONTEXT(current->live_uc);
>> + current->live_uc_valid = 1;
>> + /*
>> + * Ensure above writes have completed before signal
>> + * handler terminates
>> + */
>> + asm volatile ("dsb sy" ::: "memory");
>
> The dsb doesn't help here: this has no effect on how the compiler caches
> variables in registers etc.
>
> Overall, I think some details need a bit of a rethink here.
>
Beside the dsb which I understand is useless, I thought the memory clobber
could have helped at least with the compiler optimization/re-ordering issues
while avoiding the volatile. (but not fully)
> We need some way to ensure coherency of accesses to variables around
> and inside the signal handler here, but since we're running in a single
> thread that may be interrupted by a signal handler (running in the same
> thread), it's compiler<->compiler coherency that's the issue here, not
> cpu<->cpu or cpu<->device coherency.
>
> There may also be atomicity concerns, since the compiler might move
> stuff across and/or duplicate or tear reads/writes around the asm where
> the signal is delivered.
>
> The classic solution to these problems is to use volatile, but this
> is a blunt tool and you often end up having to mark more objects
> volatile than you really want to in order to ensure correctness. The
> ordering behaviour of accesses to volatiles is also ill-specified for
> accesses made in different threads.
>
> That said, efficiency is of no concern here and we're single-threaded,
> so a blunt, simple tool may still be adequate.
>
I'll revert back to use volatile despite checkpatch complaints, and sig_atomic_t
where needed, and add an output param in the inline asm for *dest_uc to avoid
compiler making assumptions about it
>
> Another issue is that nothing stops the stack frame the captured SP
> points to from disappearing between get_current_context() and the
> fake_sigreturn() that tries to jump back to it.
>
> To avoid this issue, we'd probably need to inline more of
> get_current_context(), i.e., turn it into a macro.
>
I made it a static __always_inline.
Moreover I moved away from kill(mypid, USR1) approach since the syscall path
will zero the SVE regs before grabbing the sigframe
(and is bad for future signal/SVE tests).
I'm instead now using brk and catch the SIGTRAP (picked form the arm64/signal
SVE tests patch still to be published)
>> + fprintf(stderr,
>> + "GOOD CONTEXT grabbed from sig_copyctx handler\n");
>> } else {
>> if (signum == current->sig_unsupp && !are_feats_ok(current)) {
>> fprintf(stderr,
>> @@ -222,7 +312,15 @@ static int test_init(struct tdescr *td)
>> !feats_ok ? "NOT " : "");
>> }
>>
>> + if (td->sig_trig == sig_copyctx)
>> + sig_copyctx = SIGUSR1;
>> + unblock_signal(sig_copyctx);
>> +
>> + /* Perform test specific additional initialization */
>> + if (td->init)
>> + td->init(td);
>> td->initialized = 1;
>> +
>> return 1;
>> }
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> index 8658d1a7d4b9..ce35be8ebc8e 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> @@ -10,4 +10,6 @@ int test_setup(struct tdescr *td);
>> void test_cleanup(struct tdescr *td);
>> int test_run(struct tdescr *td);
>> void test_result(struct tdescr *td);
>> +
>> +bool get_current_context(struct tdescr *td, ucontext_t *dest_uc);
>> #endif
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>> new file mode 100644
>> index 000000000000..15e6f62512d5
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Try to mangle the ucontext from inside a signal handler, setting the
>> + * SSBS bit to 1 and veryfing that such modification is preserved.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +static void mangle_invalid_pstate_ssbs_init(struct tdescr *td)
>> +{
>> + fprintf(stderr, "Clearing SSBS to 0\n");
>> + set_regval(SSBS_SYSREG, 0);
>> +}
>> +
>> +static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + ASSERT_GOOD_CONTEXT(uc);
>> +
>> + /* set bit value */
>> + uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
>
> Can we check that uc->uc_mcontext.pstate & PSR_SSBS_BIT is initially 0?
>
> If not, it suggests either a test bug, or modification of the SSBS
> flag by other C code before the test signal was delivered.
>
Here the situation is a bit tricky: architecture defines three levels of
SSBS support:
1. SSBS bit + MRS SSBS instruction or
2. SSBS bit ONLY
3. no support
HW_SSBS capability is reported as supported by kernel only for full support (1.)
Otherwise it is reported as unsupported, even if in 2. the PSTATE.SSBS bit is working
and I can look it up using util get_current_context().
Moreover the test is supposed to check that the sigreturn Kernel path does NOT clear
improperly the SSBS bit while returning: so as long as we can set the SSBS bit in uc PSTATE
we can check if it is cleared.
So in order to gather as much info as possible from the test without incurring in unneeded
SIGILL (attempting to use MRS), my new approach is:
- test anyway no matter if SSBS is supported: check that bit is NOT cleared on sigreturn
- use MRS clear to 0 on test_init only if SSBS declared as supported
- abort when PSTATE.SSBS bit is not cleared on test start (trigger) ONLY if SSBS was declared supported (and so cleared in test_init)
- check test result using:
- MRS SSBS if HW_SSBS supported
- PSTATE.SSBS get_current_context if HW_SSBS NOT supported
- print out always PSTATE retrieved values
This way I was able to avoid SIGILL and properly test at any level of support (1,2,3)
both the PASS and the FAIL path (using a Kernel which does NOT properly preserve the
SSBS bit)
>> + fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",
>> + uc->uc_mcontext.pstate);
>> + /* Save after mangling...it should be preserved */
>> + td->saved_uc = *uc;
>> +
>> + return 1;
>> +}
>> +
>> +static void pstate_ssbs_bit_checks(struct tdescr *td)
>> +{
>> + uint64_t val = 0;
>> + ucontext_t uc;
>> +
>> + /* This check reports some result even if MRS SSBS unsupported */
>> + if (get_current_context(td, &uc))
>> + fprintf(stderr,
>> + "INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
>> + uc.uc_mcontext.pstate,
>> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
>> + (uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
>> + "PRESERVED" : "CLEARED");
>> +
>> + fprintf(stderr, "Checking with MRS SSBS...\n");
>> + get_regval(SSBS_SYSREG, val);
>> + fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
>> + /* pass when preserved */
>> + td->pass = (val & PSR_SSBS_BIT) ==
>> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
>> +}
>> +
>> +struct tdescr tde = {
>> + .sanity_disabled = true,
>> + .name = "MANGLE_PSTATE_SSBS_REGS",
>> + .descr = "Mangling uc_mcontext changing SSBS.(PRESERVE)",
>
> Can we come up with a clearer description here? I'm not sure how to
> read this.
>
Ok..I was trying to please checkpatch.
> [...]
>
> Cheers
> ---Dave
>
Cheers
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] KVM: arm/arm64: Allow user injection of external data aborts
From: Peter Maydell @ 2019-09-09 15:56 UTC (permalink / raw)
To: Christoffer Dall
Cc: Daniel P. Berrangé, Suzuki K Poulose, Marc Zyngier,
James Morse, Julien Thierry, Stefan Hajnoczi, Heinrich Schuchardt,
Alexander Graf, kvmarm, arm-mail-list
In-Reply-To: <20190909151631.GA26449@lvm>
On Mon, 9 Sep 2019 at 16:16, Christoffer Dall <christoffer.dall@arm.com> wrote:
>
> On Mon, Sep 09, 2019 at 01:32:46PM +0100, Peter Maydell wrote:
> > This API seems to be missing support for userspace to specify
> > whether the ESR_ELx for the guest should have the EA bit set
> > (and more generally other syndrome/fault status bits). I think
> > if we have an API for "KVM_EXIT_MMIO but the access failed"
> > then it should either (a) be architecture agnostic, since
> > pretty much any architecture might have a concept of "access
> > gave some bus-error-type failure" and it would be nice if userspace
> > didn't have to special case them all in arch-specific code,
> > or (b) have the same flexibility for specifying exactly what
> > kind of fault as the architecture does. This sort of seems to
> > fall between two stools. (My ideal for KVM_EXIT_MMIO faults
> > would be a generic API which included space for optional
> > arch-specific info, which for Arm would pretty much just be
> > the EA bit.)
>
> I'm not sure I understand exactly what would be improved by making this
> either more architecture speific or more architecture generic. The
> EA bit will always be set, that's why the field is called
> 'ext_dabt_pending'.
ESR_EL1.EA doesn't mean "this is an external abort". It means
"given that this is an external abort as indicated by ESR_EL1.DFSC,
specify the external abort type". Traditionally this is 0 for
an AXI bus Decode error ("interconnect says there's nothing there")
and 1 for a Slave error ("there's something there but it told us
to go away"), though architecturally it's specified as impdef
because not everybody uses AXI. In QEMU we track the difference
between these two things and for TCG will raise external aborts
with the correct EA bit value.
> I thought as per the previous discussion, that we were specifically
> trying to avoid userspace emulating the exception in detail, so I
> designed this to provide the minimal effort API for userspace.
>
> Since we already have an architecture specific ioctl, kvm_vcpu_events, I
> don't think we're painting ourselves into a corner by using that. Is a
> natural consequence of what you're saying not that we should try to make
> that whole call architecture generic?
>
> Unless we already have specific examples of how other architectures
> would want to use something like this, and given the impact of this
> patch, I'm not sure it's worth trying to speculate about that.
In QEMU, use of a generic API would look something like
this in kvm-all.c:
case KVM_EXIT_MMIO:
DPRINTF("handle_mmio\n");
/* Called outside BQL */
MemTxResult res;
res = address_space_rw(&address_space_memory,
run->mmio.phys_addr, attrs,
run->mmio.data,
run->mmio.len,
run->mmio.is_write);
if (res != MEMTX_OK) {
/* tell the kernel the access failed, eg
* by updating the kvm_run struct to say so
*/
} else {
/* access passed, we have updated the kvm_run
* struct's mmio subfield, proceed as usual
*/
}
ret = 0;
break;
[this is exactly the current QEMU code except that today
we throw away the 'res' that tells us if the transaction
succeeded because we have no way to report it to KVM and
effectively always RAZ/WI the access.]
This is nice because you don't need anything here that has to do
"bail out to architecture specific handling of anything",
you just say "nope, the access failed", and let the kernel handle
that however the CPU would handle it. It just immediately works
for all architectures on the userspace side (assuming the kernel
defaults to not actually trying to report an abort to the guest
if nobody's implemented that on the kernel side, which is exactly
what happens today where there's no way to report the error for
any architecture).
The downside is that you lose the ability to be more specific about
architecture-specific fine distinctions like decode errors vs slave
errors, though.
Or you could have an arm-specific API that does care about
fine details like the EA bit (and maybe also other ESR_ELx
fields); that has the downside that userspace needs to
make the handling of error returns from "handle this MMIO
access" architecture specific, but you get architecture-specific
benefits as a result. (Preferably the architecture-specific
APIs should at least be basically the same, eg same ioctl
or same bit of the kvm_run struct being updated with some parts
being arch-specific data, rather than 3 different mechanisms.)
Having an API that is architecture specific but doesn't actually
let you define any of the architecture-specific aspects of
what the abort might imply seems like the worst of both worlds.
If all we can say is "this aborted" then we might as well have
the API be generic.
thanks
-- PMM
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 4/4] arm64: dts: add support for A1 based Amlogic AD401
From: Jianxin Pan @ 2019-09-09 16:12 UTC (permalink / raw)
To: Jerome Brunet, Martin Blumenstingl, Kevin Hilman
Cc: devicetree, Hanjie Lin, Victor Wan, Neil Armstrong, linux-kernel,
Jian Hu, Xingyu Chen, Tao Zeng, Qiufang Dai, linux-amlogic,
linux-arm-kernel
In-Reply-To: <1jv9u1ya3o.fsf@starbuckisacylon.baylibre.com>
Hi Jerome,
On 2019/9/9 19:36, Jerome Brunet wrote:
>
> On Sat 07 Sep 2019 at 17:02, Martin Blumenstingl wrote:
>
>> Hi Jianxin,
>>
>> On Fri, Sep 6, 2019 at 7:58 AM Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>> [...]
>>>> also I'm a bit surprised to see no busses (like aobus, cbus, periphs, ...) here
>>>> aren't there any busses defined in the A1 SoC implementation or are
>>>> were you planning to add them later?
>>> Unlike previous series,there is no Cortex-M3 AO CPU in A1, and there is no AO/EE power domain.
>>> Most of the registers are on the apb_32b bus. aobus, cbus and periphs are not used in A1.
>> OK, thank you for the explanation
>> since you're going to re-send the patch anyways: can you please
>> include the apb_32b bus?
>
> unless there is an 64 bits apb bus as well, I suppose 'apb' would be enough ?
>
There is no 64bits apb bus in A1, only apb32.
Unlike the previous series, For A1 and C1, we can not get bus information for each register from the memmap and datesheet.
Do we need to add bus description for them too? If yes, I can add 'apb' .
>> all other upstream Amlogic .dts are using the bus definitions, so that
>> will make A1 consistent with the other SoCs
>>
>>
>> Martin
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] bus: ti-sysc: Remove unpaired sysc_clkdm_deny_idle()
From: Tony Lindgren @ 2019-09-09 16:19 UTC (permalink / raw)
To: Keerthy
Cc: Nishanth Menon, Tero Kristo, Vignesh Raghavendra, Dave Gerlach,
Greg Kroah-Hartman, linux-kernel, Andrew F . Davis,
Peter Ujfalusi, Faiz Abbas, linux-omap, linux-arm-kernel,
Roger Quadros
In-Reply-To: <40e5c2a1-3682-584a-4eb9-4d96901bbfda@ti.com>
Hi,
* Keerthy <j-keerthy@ti.com> [190909 01:57]:
> I believe still the previous fix [1] for nfs boot is still not on
> linux-next. Are you planning on more testing or it will be queued as fixes?
I pushed out the pending fixes on Saturday so they should be in
the next version of Linux next.
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 6/8] mm: Introduce Reported pages
From: Alexander Duyck @ 2019-09-09 16:25 UTC (permalink / raw)
To: Kirill A. Shutemov, Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas, mhocko,
linux-mm, will, aarcange, virtio-dev, mst, willy, wei.w.wang,
ying.huang, riel, dan.j.williams, lcapitulino, linux-arm-kernel,
osalvador, nitesh, konrad.wilk, dave.hansen, linux-kernel,
pbonzini, akpm, fengguang.wu, kirill.shutemov
In-Reply-To: <20190909144209.jcrx6o3ntecdaqmh@box>
On Mon, 2019-09-09 at 17:42 +0300, Kirill A. Shutemov wrote:
> On Sat, Sep 07, 2019 at 10:25:53AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > In order to pave the way for free page reporting in virtualized
> > environments we will need a way to get pages out of the free lists and
> > identify those pages after they have been returned. To accomplish this,
> > this patch adds the concept of a Reported Buddy, which is essentially
> > meant to just be the Uptodate flag used in conjunction with the Buddy
> > page type.
> >
> > It adds a set of pointers we shall call "boundary" which represents the
> > upper boundary between the unreported and reported pages. The general idea
> > is that in order for a page to cross from one side of the boundary to the
> > other it will need to go through the reporting process. Ultimately a
> > free_list has been fully processed when the boundary has been moved from
> > the tail all they way up to occupying the first entry in the list.
> >
> > Doing this we should be able to make certain that we keep the reported
> > pages as one contiguous block in each free list. This will allow us to
> > efficiently manipulate the free lists whenever we need to go in and start
> > sending reports to the hypervisor that there are new pages that have been
> > freed and are no longer in use.
> >
> > An added advantage to this approach is that we should be reducing the
> > overall memory footprint of the guest as it will be more likely to recycle
> > warm pages versus trying to allocate the reported pages that were likely
> > evicted from the guest memory.
> >
> > Since we will only be reporting one zone at a time we keep the boundary
> > limited to being defined for just the zone we are currently reporting pages
> > from. Doing this we can keep the number of additional pointers needed quite
> > small. To flag that the boundaries are in place we use a single bit
> > in the zone to indicate that reporting and the boundaries are active.
> >
> > The determination of when to start reporting is based on the tracking of
> > the number of free pages in a given area versus the number of reported
> > pages in that area. We keep track of the number of reported pages per
> > free_area in a separate zone specific area. We do this to avoid modifying
> > the free_area structure as this can lead to false sharing for the highest
> > order with the zone lock which leads to a noticeable performance
> > degradation.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> > include/linux/mmzone.h | 52 +++++-
> > include/linux/page-flags.h | 11 +
> > include/linux/page_reporting.h | 178 ++++++++++++++++++++
> > mm/Kconfig | 5 +
> > mm/Makefile | 1
> > mm/memory_hotplug.c | 1
> > mm/page_alloc.c | 115 ++++++++++++-
> > mm/page_reporting.c | 358 ++++++++++++++++++++++++++++++++++++++++
> > 8 files changed, 711 insertions(+), 10 deletions(-)
> > create mode 100644 include/linux/page_reporting.h
> > create mode 100644 mm/page_reporting.c
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index 2ddf1f1971c0..4b2c44d7e266 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -463,6 +463,14 @@ struct zone {
> > seqlock_t span_seqlock;
> > #endif
> >
> > +#ifdef CONFIG_PAGE_REPORTING
> > + /*
> > + * Pointer to reported page tracking statistics array. The size of
> > + * the array is MAX_ORDER - PAGE_REPORTING_MIN_ORDER. NULL when
> > + * unused page reporting is not present.
> > + */
> > + unsigned long *reported_pages;
> > +#endif
> > int initialized;
> >
> > /* Write-intensive fields used from the page allocator */
> > @@ -538,6 +546,14 @@ enum zone_flags {
> > ZONE_BOOSTED_WATERMARK, /* zone recently boosted watermarks.
> > * Cleared when kswapd is woken.
> > */
> > + ZONE_PAGE_REPORTING_REQUESTED, /* zone enabled page reporting and has
> > + * requested flushing the data out of
> > + * higher order pages.
> > + */
> > + ZONE_PAGE_REPORTING_ACTIVE, /* zone enabled page reporting and is
> > + * activly flushing the data out of
> > + * higher order pages.
> > + */
> > };
> >
> > static inline unsigned long zone_managed_pages(struct zone *zone)
> > @@ -764,6 +780,8 @@ static inline bool pgdat_is_empty(pg_data_t *pgdat)
> > return !pgdat->node_start_pfn && !pgdat->node_spanned_pages;
> > }
> >
> > +#include <linux/page_reporting.h>
> > +
> > /* Used for pages not on another list */
> > static inline void add_to_free_list(struct page *page, struct zone *zone,
> > unsigned int order, int migratetype)
> > @@ -778,24 +796,48 @@ static inline void add_to_free_list(struct page *page, struct zone *zone,
> > static inline void add_to_free_list_tail(struct page *page, struct zone *zone,
> > unsigned int order, int migratetype)
> > {
> > - struct free_area *area = &zone->free_area[order];
> > + struct list_head *tail = get_unreported_tail(zone, order, migratetype);
> >
> > - list_add_tail(&page->lru, &area->free_list[migratetype]);
> > - area->nr_free++;
> > + /*
> > + * To prevent the unreported pages from being interleaved with the
> > + * reported ones while we are actively processing pages we will use
> > + * the head of the reported pages to determine the tail of the free
> > + * list.
> > + */
> > + list_add_tail(&page->lru, tail);
> > + zone->free_area[order].nr_free++;
> > }
> >
> > /* Used for pages which are on another list */
> > static inline void move_to_free_list(struct page *page, struct zone *zone,
> > unsigned int order, int migratetype)
> > {
> > - struct free_area *area = &zone->free_area[order];
> > + struct list_head *tail = get_unreported_tail(zone, order, migratetype);
> > +
> > + /*
> > + * We must get the tail for our target list before moving the page on
> > + * the reported list as we will possibly be replacing the tail page of
> > + * the list with our current page if it is reported.
> > + */
> > + if (unlikely(PageReported(page)))
> > + move_page_to_reported_list(page, zone, migratetype);
> >
> > - list_move(&page->lru, &area->free_list[migratetype]);
> > + /*
> > + * To prevent unreported pages from being mixed with the reported
> > + * ones we add pages to the tail of the list. By doing this the function
> > + * above can either label them as included in the reported list or not
> > + * and the result will be consistent.
> > + */
> > + list_move_tail(&page->lru, tail);
> > }
> >
> > static inline void del_page_from_free_list(struct page *page, struct zone *zone,
> > unsigned int order)
> > {
> > + /* remove page from reported list, and clear reported state */
> > + if (unlikely(PageReported(page)))
> > + del_page_from_reported_list(page, zone);
> > +
> > list_del(&page->lru);
> > __ClearPageBuddy(page);
> > set_page_private(page, 0);
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index f91cb8898ff0..759a3b3956f2 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -163,6 +163,9 @@ enum pageflags {
> >
> > /* non-lru isolated movable page */
> > PG_isolated = PG_reclaim,
> > +
> > + /* Buddy pages. Used to track which pages have been reported */
> > + PG_reported = PG_uptodate,
> > };
> >
> > #ifndef __GENERATING_BOUNDS_H
> > @@ -432,6 +435,14 @@ static inline bool set_hwpoison_free_buddy_page(struct page *page)
> > #endif
> >
> > /*
> > + * PageReported() is used to track reported free pages within the Buddy
> > + * allocator. We can use the non-atomic version of the test and set
> > + * operations as both should be shielded with the zone lock to prevent
> > + * any possible races on the setting or clearing of the bit.
> > + */
> > +__PAGEFLAG(Reported, reported, PF_NO_COMPOUND)
> > +
> > +/*
> > * On an anonymous page mapped into a user virtual memory area,
> > * page->mapping points to its anon_vma, not to a struct address_space;
> > * with the PAGE_MAPPING_ANON bit set to distinguish it. See rmap.h.
> > diff --git a/include/linux/page_reporting.h b/include/linux/page_reporting.h
> > new file mode 100644
> > index 000000000000..836033ca237b
> > --- /dev/null
> > +++ b/include/linux/page_reporting.h
> > @@ -0,0 +1,178 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LINUX_PAGE_REPORTING_H
> > +#define _LINUX_PAGE_REPORTING_H
> > +
> > +#include <linux/mmzone.h>
> > +#include <linux/jump_label.h>
> > +#include <linux/pageblock-flags.h>
> > +
> > +#define PAGE_REPORTING_MIN_ORDER pageblock_order
> > +#define PAGE_REPORTING_HWM 32
> > +
> > +#ifdef CONFIG_PAGE_REPORTING
> > +struct page_reporting_dev_info {
> > + /* function that alters pages to make them "reported" */
> > + void (*report)(struct page_reporting_dev_info *phdev,
> > + unsigned int nents);
> > +
> > + /* scatterlist containing pages to be processed */
> > + struct scatterlist *sg;
> > +
> > + /*
> > + * Upper limit on the number of pages that the react function
> > + * expects to be placed into the batch list to be processed.
> > + */
> > + unsigned long capacity;
> > +
> > + /* work struct for processing reports */
> > + struct delayed_work work;
> > +
> > + /*
> > + * The number of zones requesting reporting, plus one additional if
> > + * processing thread is active.
> > + */
> > + atomic_t refcnt;
> > +};
> > +
> > +/* Boundary functions */
> > +struct list_head *__page_reporting_get_boundary(unsigned int order,
> > + int migratetype);
> > +void page_reporting_del_from_boundary(struct page *page);
> > +void page_reporting_add_to_boundary(struct page *page, int migratetype);
> > +void page_reporting_move_to_boundary(struct page *page, struct zone *zone,
> > + int migratetype);
> > +
> > +/* Reported page accessors, defined in page_alloc.c */
> > +struct page *get_unreported_page(struct zone *zone, unsigned int order,
> > + int migratetype);
> > +void free_reported_page(struct page *page, unsigned int order);
> > +
> > +/* Tear-down and bring-up for page reporting devices */
> > +void page_reporting_shutdown(struct page_reporting_dev_info *phdev);
> > +int page_reporting_startup(struct page_reporting_dev_info *phdev);
> > +
> > +void __page_reporting_free_stats(struct zone *zone);
> > +void __page_reporting_request(struct zone *zone);
> > +
> > +static inline void __del_page_from_reported_list(struct page *page,
> > + struct zone *zone)
> > +{
> > + /* page_private will contain the page order, so just use it directly */
> > + zone->reported_pages[page_private(page) - PAGE_REPORTING_MIN_ORDER]--;
> > +
> > + /* clear the flag so we can report on it when it returns */
> > + __ClearPageReported(page);
> > +}
> > +#endif /* CONFIG_PAGE_REPORTING */
> > +
> > +/*
> > + * Method for obtaining the tail of the free list. Using this allows for
> > + * tail insertions of unreported pages into the region that is currently
> > + * being scanned so as to avoid interleaving reported and unreported pages.
> > + */
> > +static inline struct list_head *
> > +get_unreported_tail(struct zone *zone, unsigned int order, int migratetype)
> > +{
> > +#ifdef CONFIG_PAGE_REPORTING
> > + if (order >= PAGE_REPORTING_MIN_ORDER &&
> > + test_bit(ZONE_PAGE_REPORTING_ACTIVE, &zone->flags))
> > + return __page_reporting_get_boundary(order, migratetype);
> > +#endif
> > + return &zone->free_area[order].free_list[migratetype];
> > +}
> > +
> > +/*
> > + * Functions for adding/removing pages from reported end of list.
> > + * All of them expect the zone lock to be held to maintain
> > + * consistency of the reported list as a subset of the free list.
> > + */
> > +static inline void add_page_to_reported_list(struct page *page,
> > + struct zone *zone,
> > + int order,
> > + int migratetype)
> > +{
> > +#ifdef CONFIG_PAGE_REPORTING
>
> Instead of ifdefing full body of the helpers, can we have two sets of
> these helpres defined for CONFIG_PAGE_REPORTING and for
> !CONFIG_PAGE_REPORTING cases?
>
> Or can we get all this working with IS_ENABLED() instead of #ifdef?
I can probably split the helpers and just duplicate them with the ifdef
content stripped.
The problem with IS_ENABLED() is that we are calling functions that don't
exist if page reporting isn't enabled so that would cause build issues.
> > + /* flag page as reported */
> > + __SetPageReported(page);
> > +
> > + /* update areated page accounting */
> > + zone->reported_pages[order - PAGE_REPORTING_MIN_ORDER]++;
> > +
> > + /* update boundary of new migratetype and record it */
> > + page_reporting_add_to_boundary(page, migratetype);
> > +#endif
> > +}
> > +
> > +static inline void del_page_from_reported_list(struct page *page,
> > + struct zone *zone)
> > +{
> > +#ifdef CONFIG_PAGE_REPORTING
> > + /* push boundary back if we removed the upper boundary */
> > + if (test_bit(ZONE_PAGE_REPORTING_ACTIVE, &zone->flags))
> > + page_reporting_del_from_boundary(page);
> > +
> > + __del_page_from_reported_list(page, zone);
> > +#endif
> > +}
> > +
> > +static inline void move_page_to_reported_list(struct page *page,
> > + struct zone *zone,
> > + int migratetype)
> > +{
> > +#ifdef CONFIG_PAGE_REPORTING
> > + page_reporting_move_to_boundary(page, zone, migratetype);
> > +#endif
> > +}
> > +
> > +/* Free reported_pages and reset reported page tracking count to 0 */
> > +static inline void page_reporting_reset(struct zone *zone)
> > +{
> > +#ifdef CONFIG_PAGE_REPORTING
> > + if (zone->reported_pages)
> > + __page_reporting_free_stats(zone);
> > +#endif
> > +}
> > +
> > +DECLARE_STATIC_KEY_FALSE(page_reporting_notify_enabled);
> > +/**
> > + * page_reporting_notify_free - Free page notification to start page processing
> > + * @zone: Pointer to current zone of last page processed
> > + * @order: Order of last page added to zone
> > + *
> > + * This function is meant to act as a screener for __page_reporting_request
> > + * which will determine if a give zone has crossed over the high-water mark
> > + * that will justify us beginning page treatment. If we have crossed that
> > + * threshold then it will start the process of pulling some pages and
> > + * placing them in the batch list for treatment.
> > + */
> > +static inline void page_reporting_notify_free(struct zone *zone, int order)
> > +{
> > +#ifdef CONFIG_PAGE_REPORTING
> > + unsigned long nr_reported;
> > +
> > + /* Called from hot path in __free_one_page() */
> > + if (!static_branch_unlikely(&page_reporting_notify_enabled))
> > + return;
> > +
> > + /* Limit notifications only to higher order pages */
> > + if (order < PAGE_REPORTING_MIN_ORDER)
> > + return;
> > +
> > + /* Do not bother with tests if we have already requested reporting */
> > + if (test_bit(ZONE_PAGE_REPORTING_REQUESTED, &zone->flags))
> > + return;
>
> How is it not racy wrt page_reporting_fill()? Do we hold zone->lock or
> something?
Yes. This is called at the end of __free_one_page with the zone->lock
held.
> > +
> > + /* If reported_pages is not populated, assume 0 */
> > + nr_reported = zone->reported_pages ?
> > + zone->reported_pages[order - PAGE_REPORTING_MIN_ORDER] : 0;
> > +
> > + /* Only request it if we have enough to begin the page reporting */
> > + if (zone->free_area[order].nr_free < nr_reported + PAGE_REPORTING_HWM)
> > + return;
> > +
> > + /* This is slow, but should be called very rarely */
> > + __page_reporting_request(zone);
> > +#endif
> > +}
> > +#endif /*_LINUX_PAGE_REPORTING_H */
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index a5dae9a7eb51..be1a5db50df5 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -237,6 +237,11 @@ config COMPACTION
> > linux-mm@kvack.org.
> >
> > #
> > +# support for unused page reporting
> > +config PAGE_REPORTING
> > + bool
> > +
> > +#
>
> Proper description for the config option?
I can add one. However the feature doesn't do anything without a caller
that makes use of it. I guess it would make sense to enable this for
something such as an out-of-tree module to later use.
> > # support for page migration
> > #
> > config MIGRATION
> > diff --git a/mm/Makefile b/mm/Makefile
> > index d996846697ef..fc4fa17b6c83 100644
> > --- a/mm/Makefile
> > +++ b/mm/Makefile
> > @@ -107,3 +107,4 @@ obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
> > obj-$(CONFIG_ZONE_DEVICE) += memremap.o
> > obj-$(CONFIG_HMM_MIRROR) += hmm.o
> > obj-$(CONFIG_MEMFD_CREATE) += memfd.o
> > +obj-$(CONFIG_PAGE_REPORTING) += page_reporting.o
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 49f7bf91c25a..cb71a7190682 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1613,6 +1613,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
> > if (!populated_zone(zone)) {
> > zone_pcp_reset(zone);
> > build_all_zonelists(NULL);
> > + page_reporting_reset(zone);
> > } else
> > zone_pcp_update(zone);
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index f85dc1561b85..615aea24c082 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -68,6 +68,7 @@
> > #include <linux/lockdep.h>
> > #include <linux/nmi.h>
> > #include <linux/psi.h>
> > +#include <linux/page_reporting.h>
> >
> > #include <asm/sections.h>
> > #include <asm/tlbflush.h>
> > @@ -916,7 +917,7 @@ static inline struct capture_control *task_capc(struct zone *zone)
> > static inline void __free_one_page(struct page *page,
> > unsigned long pfn,
> > struct zone *zone, unsigned int order,
> > - int migratetype)
> > + int migratetype, bool reported)
> > {
> > struct capture_control *capc = task_capc(zone);
> > unsigned long uninitialized_var(buddy_pfn);
> > @@ -991,11 +992,20 @@ static inline void __free_one_page(struct page *page,
> > done_merging:
> > set_page_order(page, order);
> >
> > - if (is_shuffle_order(order) ? shuffle_pick_tail() :
> > - buddy_merge_likely(pfn, buddy_pfn, page, order))
> > + if (reported ||
> > + (is_shuffle_order(order) ? shuffle_pick_tail() :
> > + buddy_merge_likely(pfn, buddy_pfn, page, order)))
> > add_to_free_list_tail(page, zone, order, migratetype);
> > else
> > add_to_free_list(page, zone, order, migratetype);
> > +
> > + /*
> > + * No need to notify on a reported page as the total count of
> > + * unreported pages will not have increased since we have essentially
> > + * merged the reported page with one or more unreported pages.
> > + */
> > + if (!reported)
> > + page_reporting_notify_free(zone, order);
> > }
> >
> > /*
> > @@ -1306,7 +1316,7 @@ static void free_pcppages_bulk(struct zone *zone, int count,
> > if (unlikely(isolated_pageblocks))
> > mt = get_pageblock_migratetype(page);
> >
> > - __free_one_page(page, page_to_pfn(page), zone, 0, mt);
> > + __free_one_page(page, page_to_pfn(page), zone, 0, mt, false);
> > trace_mm_page_pcpu_drain(page, 0, mt);
> > }
> > spin_unlock(&zone->lock);
> > @@ -1322,7 +1332,7 @@ static void free_one_page(struct zone *zone,
> > is_migrate_isolate(migratetype))) {
> > migratetype = get_pfnblock_migratetype(page, pfn);
> > }
> > - __free_one_page(page, pfn, zone, order, migratetype);
> > + __free_one_page(page, pfn, zone, order, migratetype, false);
> > spin_unlock(&zone->lock);
> > }
> >
> > @@ -2184,6 +2194,101 @@ struct page *__rmqueue_smallest(struct zone *zone, unsigned int order,
> > return NULL;
> > }
> >
> > +#ifdef CONFIG_PAGE_REPORTING
> > +/**
> > + * free_reported_page - Return a now-reported page back where we got it
> > + * @page: Page that was reported
> > + * @order: Order of the reported page
> > + *
> > + * This function will pull the migratetype and order information out
> > + * of the page and attempt to return it where it found it. If the page
> > + * is added to the free list without changes we will mark it as being
> > + * reported.
> > + */
> > +void free_reported_page(struct page *page, unsigned int order)
> > +{
> > + struct zone *zone = page_zone(page);
> > + unsigned long pfn;
> > + unsigned int mt;
> > +
> > + /* zone lock should be held when this function is called */
> > + lockdep_assert_held(&zone->lock);
> > +
> > + pfn = page_to_pfn(page);
> > + mt = get_pfnblock_migratetype(page, pfn);
> > + __free_one_page(page, pfn, zone, order, mt, true);
> > +
> > + /*
> > + * If page was not comingled with another page we can consider
> > + * the result to be "reported" since part of the page hasn't been
> > + * modified, otherwise we would need to report on the new larger
> > + * page.
> > + */
> > + if (PageBuddy(page) && page_order(page) == order)
> > + add_page_to_reported_list(page, zone, order, mt);
> > +}
> > +
> > +/**
> > + * get_unreported_page - Pull an unreported page from the free_list
> > + * @zone: Zone to draw pages from
> > + * @order: Order to draw pages from
> > + * @mt: Migratetype to draw pages from
> > + *
> > + * This function will obtain a page from the free list. It will start by
> > + * attempting to pull from the tail of the free list and if that is already
> > + * reported on it will instead pull the head if that is unreported.
> > + *
> > + * The page will have the migrate type and order stored in the page
> > + * metadata. While being processed the page will not be avaialble for
> > + * allocation.
> > + *
> > + * Return: page pointer if raw page found, otherwise NULL
> > + */
> > +struct page *get_unreported_page(struct zone *zone, unsigned int order, int mt)
> > +{
> > + struct list_head *tail = get_unreported_tail(zone, order, mt);
> > + struct free_area *area = &(zone->free_area[order]);
> > + struct list_head *list = &area->free_list[mt];
> > + struct page *page;
> > +
> > + /* zone lock should be held when this function is called */
> > + lockdep_assert_held(&zone->lock);
> > +
> > + /* Find a page of the appropriate size in the preferred list */
> > + page = list_last_entry(tail, struct page, lru);
> > + list_for_each_entry_from_reverse(page, list, lru) {
> > + /* If we entered this loop then the "raw" list isn't empty */
> > +
> > + /* If the page is reported try the head of the list */
> > + if (PageReported(page)) {
> > + page = list_first_entry(list, struct page, lru);
> > +
> > + /*
> > + * If both the head and tail are reported then reset
> > + * the boundary so that we read as an empty list
> > + * next time and bail out.
> > + */
> > + if (PageReported(page)) {
> > + page_reporting_add_to_boundary(page, mt);
> > + break;
> > + }
> > + }
> > +
> > + del_page_from_free_list(page, zone, order);
> > +
> > + /*
> > + * Page will not be available for allocation while we are
> > + * processing it so update the freepage state.
> > + */
> > + __mod_zone_freepage_state(zone, -(1 << order), mt);
> > +
> > + return page;
> > + }
> > +
> > + return NULL;
> > +}
> > +#endif /* CONFIG_PAGE_REPORTING */
> > +
> > /*
> > * This array describes the order lists are fallen back to when
> > * the free lists for the desirable migrate type are depleted
> > diff --git a/mm/page_reporting.c b/mm/page_reporting.c
> > new file mode 100644
> > index 000000000000..a59ef53eb0b8
> > --- /dev/null
> > +++ b/mm/page_reporting.c
> > @@ -0,0 +1,358 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/mm.h>
> > +#include <linux/mmzone.h>
> > +#include <linux/page-isolation.h>
> > +#include <linux/gfp.h>
> > +#include <linux/export.h>
> > +#include <linux/delay.h>
> > +#include <linux/slab.h>
> > +#include <linux/scatterlist.h>
> > +#include "internal.h"
> > +
> > +static struct page_reporting_dev_info __rcu *ph_dev_info __read_mostly;
> > +struct list_head **boundary __read_mostly;
> > +
> > +static inline struct list_head **get_boundary_ptr(unsigned int order,
> > + unsigned int migratetype)
> > +{
> > + return boundary +
> > + (order - PAGE_REPORTING_MIN_ORDER) * MIGRATE_TYPES + migratetype;
> > +}
> > +
> > +static void page_reporting_reset_boundary(struct zone *zone, unsigned int order,
> > + unsigned int migratetype)
> > +{
> > + struct list_head **tail = get_boundary_ptr(order, migratetype);
> > +
> > + *tail = &zone->free_area[order].free_list[migratetype];
> > +}
> > +
> > +#define for_each_reporting_migratetype_order(_order, _type) \
> > + for (_order = MAX_ORDER; _order-- != PAGE_REPORTING_MIN_ORDER;) \
> > + for (_type = MIGRATE_TYPES; _type--;) \
> > + if (!is_migrate_isolate(_type))
> > +
> > +static int page_reporting_populate_metadata(struct zone *zone)
> > +{
> > + unsigned int order, mt;
> > +
> > + /*
> > + * We need to make sure we have somewhere to store the tracking
> > + * data for how many reported pages are in the zone. To do that
> > + * we need to make certain zone->reported_pages is populated.
> > + */
> > + if (!zone->reported_pages) {
> > + zone->reported_pages =
> > + kcalloc(MAX_ORDER - PAGE_REPORTING_MIN_ORDER,
> > + sizeof(unsigned long),
> > + GFP_KERNEL);
> > + if (!zone->reported_pages)
> > + return -ENOMEM;
> > + }
> > +
> > + /* Update boundary data to reflect the zone we are currently working */
> > + for_each_reporting_migratetype_order(order, mt)
> > + page_reporting_reset_boundary(zone, order, mt);
> > +
> > + return 0;
> > +}
> > +
> > +struct list_head *__page_reporting_get_boundary(unsigned int order,
> > + int migratetype)
> > +{
> > + return *get_boundary_ptr(order, migratetype);
> > +}
> > +
> > +void page_reporting_del_from_boundary(struct page *page)
> > +{
> > + unsigned int order = page_private(page);
> > + int mt = get_pcppage_migratetype(page);
> > + struct list_head **tail = get_boundary_ptr(order, mt);
> > +
> > + if (*tail == &page->lru)
> > + *tail = page->lru.next;
> > +}
> > +
> > +void page_reporting_add_to_boundary(struct page *page, int migratetype)
> > +{
> > + unsigned int order = page_private(page);
> > + struct list_head **tail = get_boundary_ptr(order, migratetype);
> > +
> > + *tail = &page->lru;
> > + set_pcppage_migratetype(page, migratetype);
> > +}
> > +
> > +void page_reporting_move_to_boundary(struct page *page, struct zone *zone,
> > + int dest_mt)
> > +{
> > + /*
> > + * We essentially have two options available to us. The first is to
> > + * move the page from the boundary list on one migratetype to the
> > + * list for the new migratetype assuming reporting is still active.
> > + *
> > + * The other option is to clear the reported state of the page as
> > + * we will not be adding it to the group of pages that were already
> > + * reported. It is cheaper to just rereport such pages then go
> > + * through and do a special search to skip over them. If the page
> > + * is being moved into isolation we can defer this until the page
> > + * comes out of isolation since we do not scan the isolated
> > + * migratetype.
> > + */
> > + if (test_bit(ZONE_PAGE_REPORTING_ACTIVE, &zone->flags)) {
> > + page_reporting_del_from_boundary(page);
> > + page_reporting_add_to_boundary(page, dest_mt);
> > + } else if (!is_migrate_isolate(dest_mt)) {
> > + __del_page_from_reported_list(page, zone);
> > + }
> > +}
> > +
> > +static unsigned int page_reporting_fill(struct zone *zone,
> > + struct page_reporting_dev_info *phdev)
> > +{
> > + struct scatterlist *sg = phdev->sg;
> > + unsigned int order, mt, count = 0;
> > +
> > + sg_init_table(phdev->sg, phdev->capacity);
> > +
> > + for_each_reporting_migratetype_order(order, mt) {
> > + struct page *page;
> > +
> > + /*
> > + * Pull pages from free list until we have drained
> > + * it or we have reached capacity.
> > + */
> > + while ((page = get_unreported_page(zone, order, mt))) {
> > + sg_set_page(&sg[count], page, PAGE_SIZE << order, 0);
> > +
> > + if (++count == phdev->capacity)
> > + return count;
> > + }
> > + }
> > +
> > + /* mark end of scatterlist due to underflow */
> > + if (count)
> > + sg_mark_end(&sg[count - 1]);
> > +
> > + /*
> > + * If there are no longer enough free pages to fully populate
> > + * the scatterlist, then we can just shut it down for this zone.
> > + */
> > + __clear_bit(ZONE_PAGE_REPORTING_REQUESTED, &zone->flags);
> > + atomic_dec(&phdev->refcnt);
> > +
> > + return count;
> > +}
> > +
> > +static void page_reporting_drain(struct page_reporting_dev_info *phdev)
> > +{
> > + struct scatterlist *sg = phdev->sg;
> > +
> > + /*
> > + * Drain the now reported pages back into their respective
> > + * free lists/areas. We assume at least one page is populated.
> > + */
> > + do {
> > + free_reported_page(sg_page(sg), get_order(sg->length));
> > + } while (!sg_is_last(sg++));
> > +}
> > +
> > +/*
> > + * The page reporting cycle consists of 4 stages, fill, report, drain, and idle.
> > + * We will cycle through the first 3 stages until we fail to obtain any
> > + * pages, in that case we will switch to idle.
> > + */
> > +static void page_reporting_cycle(struct zone *zone,
> > + struct page_reporting_dev_info *phdev)
> > +{
> > + /*
> > + * Guarantee boundaries and stats are populated before we
> > + * start placing reported pages in the zone.
> > + */
> > + if (page_reporting_populate_metadata(zone))
> > + return;
> > +
> > + spin_lock_irq(&zone->lock);
> > +
> > + /* set bit indicating boundaries are present */
> > + __set_bit(ZONE_PAGE_REPORTING_ACTIVE, &zone->flags);
> > +
> > + do {
> > + /* Pull pages out of allocator into a scaterlist */
> > + unsigned int nents = page_reporting_fill(zone, phdev);
> > +
> > + /* no pages were acquired, give up */
> > + if (!nents)
> > + break;
> > +
> > + spin_unlock_irq(&zone->lock);
> > +
> > + /* begin processing pages in local list */
> > + phdev->report(phdev, nents);
> > +
> > + spin_lock_irq(&zone->lock);
> > +
> > + /*
> > + * We should have a scatterlist of pages that have been
> > + * processed. Return them to their original free lists.
> > + */
> > + page_reporting_drain(phdev);
> > +
> > + /* keep pulling pages till there are none to pull */
> > + } while (test_bit(ZONE_PAGE_REPORTING_REQUESTED, &zone->flags));
> > +
> > + /* processing of the zone is complete, we can disable boundaries */
> > + __clear_bit(ZONE_PAGE_REPORTING_ACTIVE, &zone->flags);
> > +
> > + spin_unlock_irq(&zone->lock);
> > +}
> > +
> > +static void page_reporting_process(struct work_struct *work)
> > +{
> > + struct delayed_work *d_work = to_delayed_work(work);
> > + struct page_reporting_dev_info *phdev =
> > + container_of(d_work, struct page_reporting_dev_info, work);
> > + struct zone *zone = first_online_pgdat()->node_zones;
> > +
> > + do {
> > + if (test_bit(ZONE_PAGE_REPORTING_REQUESTED, &zone->flags))
> > + page_reporting_cycle(zone, phdev);
> > +
> > + /* Move to next zone, if at end of list start over */
> > + zone = next_zone(zone) ? : first_online_pgdat()->node_zones;
> > +
> > + /*
> > + * As long as refcnt has not reached zero there are still
> > + * zones to be processed.
> > + */
> > + } while (atomic_read(&phdev->refcnt));
> > +}
> > +
> > +/* request page reporting on this zone */
> > +void __page_reporting_request(struct zone *zone)
> > +{
> > + struct page_reporting_dev_info *phdev;
> > +
> > + rcu_read_lock();
> > +
> > + /*
> > + * We use RCU to protect the ph_dev_info pointer. In almost all
> > + * cases this should be present, however in the unlikely case of
> > + * a shutdown this will be NULL and we should exit.
> > + */
> > + phdev = rcu_dereference(ph_dev_info);
> > + if (unlikely(!phdev))
> > + goto out;
> > +
> > + /*
> > + * We can use separate test and set operations here as there
> > + * is nothing else that can set or clear this bit while we are
> > + * holding the zone lock. The advantage to doing it this way is
> > + * that we don't have to dirty the cacheline unless we are
> > + * changing the value.
> > + */
> > + __set_bit(ZONE_PAGE_REPORTING_REQUESTED, &zone->flags);
> > +
> > + /*
> > + * Delay the start of work to allow a sizable queue to
> > + * build. For now we are limiting this to running no more
> > + * than 10 times per second.
> > + */
> > + if (!atomic_fetch_inc(&phdev->refcnt))
> > + schedule_delayed_work(&phdev->work, HZ / 10);
> > +out:
> > + rcu_read_unlock();
> > +}
> > +
> > +void __page_reporting_free_stats(struct zone *zone)
> > +{
> > + /* free reported_page statisitics */
> > + kfree(zone->reported_pages);
> > + zone->reported_pages = NULL;
> > +}
> > +
> > +static DEFINE_MUTEX(page_reporting_mutex);
> > +DEFINE_STATIC_KEY_FALSE(page_reporting_notify_enabled);
> > +
> > +void page_reporting_shutdown(struct page_reporting_dev_info *phdev)
> > +{
> > + mutex_lock(&page_reporting_mutex);
> > +
> > + if (rcu_access_pointer(ph_dev_info) == phdev) {
> > + /* Disable page reporting notification */
> > + static_branch_disable(&page_reporting_notify_enabled);
> > + RCU_INIT_POINTER(ph_dev_info, NULL);
> > + synchronize_rcu();
> > +
> > + /* Flush any existing work, and lock it out */
> > + cancel_delayed_work_sync(&phdev->work);
> > +
> > + /* Free scatterlist */
> > + kfree(phdev->sg);
> > + phdev->sg = NULL;
> > +
> > + /* Free boundaries */
> > + kfree(boundary);
> > + boundary = NULL;
> > + }
> > +
> > + mutex_unlock(&page_reporting_mutex);
> > +}
> > +EXPORT_SYMBOL_GPL(page_reporting_shutdown);
> > +
> > +int page_reporting_startup(struct page_reporting_dev_info *phdev)
> > +{
> > + struct zone *zone;
> > + int err = 0;
> > +
> > + /* No point in enabling this if it cannot handle any pages */
> > + if (!phdev->capacity)
> > + return -EINVAL;
>
> Looks like a usage error. Maybe WARN_ON()?
I can do that. We shouldn't really be accessing this with that setup
anyway.
> > +
> > + mutex_lock(&page_reporting_mutex);
> > +
> > + /* nothing to do if already in use */
> > + if (rcu_access_pointer(ph_dev_info)) {
> > + err = -EBUSY;
> > + goto err_out;
> > + }
>
> Again, it's from "something went horribly wrong" category.
> Maybe WARN_ON()?
That one I am not so sure about. Right now we only have one user for the
page reporting interface. My concern is if we ever have more than one we
may experience collisions. The device driver requesting this should
display an error message if it is not able tor register the interface.
> > +
> > + boundary = kcalloc(MAX_ORDER - PAGE_REPORTING_MIN_ORDER,
> > + sizeof(struct list_head *) * MIGRATE_TYPES,
> > + GFP_KERNEL);
>
> Could you comment here on why this size of array is allocated?
> The calculation is not obvious to a reader.
Would something like the following work for you?
/*
* Allocate space to store the boundaries for the zone we are
* actively reporting on. We will need to store one boundary
* pointer per migratetype, and then we need to have one of these
* arrays per order for orders greater than or equal to
* PAGE_REPORTING_MIN_ORDER.
*/
> > + if (!boundary) {
> > + err = -ENOMEM;
> > + goto err_out;
> > + }
> > +
> > + /* allocate scatterlist to store pages being reported on */
> > + phdev->sg = kcalloc(phdev->capacity, sizeof(*phdev->sg), GFP_KERNEL);
> > + if (!phdev->sg) {
> > + err = -ENOMEM;
> > +
> > + kfree(boundary);
> > + boundary = NULL;
> > +
> > + goto err_out;
> > + }
> > +
> > +
> > + /* initialize refcnt and work structures */
> > + atomic_set(&phdev->refcnt, 0);
> > + INIT_DELAYED_WORK(&phdev->work, &page_reporting_process);
> > +
> > + /* assign device, and begin initial flush of populated zones */
> > + rcu_assign_pointer(ph_dev_info, phdev);
> > + for_each_populated_zone(zone) {
> > + spin_lock_irq(&zone->lock);
> > + __page_reporting_request(zone);
> > + spin_unlock_irq(&zone->lock);
> > + }
> > +
> > + /* enable page reporting notification */
> > + static_branch_enable(&page_reporting_notify_enabled);
> > +err_out:
> > + mutex_unlock(&page_reporting_mutex);
> > +
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(page_reporting_startup);
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] dt-bindings: timer: Convert Exynos MCT bindings to json-schema
From: Krzysztof Kozlowski @ 2019-09-09 16:25 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner, Rob Herring, Mark Rutland,
Kukjin Kim, Krzysztof Kozlowski, linux-kernel, devicetree,
linux-arm-kernel, linux-samsung-soc
Convert Samsung Exynos Soc Multi Core Timer bindings to DT schema format
using json-schema.
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
.../bindings/timer/samsung,exynos4210-mct.txt | 88 --------------
.../timer/samsung,exynos4210-mct.yaml | 115 ++++++++++++++++++
2 files changed, 115 insertions(+), 88 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
create mode 100644 Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
deleted file mode 100644
index 8f78640ad64c..000000000000
--- a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.txt
+++ /dev/null
@@ -1,88 +0,0 @@
-Samsung's Multi Core Timer (MCT)
-
-The Samsung's Multi Core Timer (MCT) module includes two main blocks, the
-global timer and CPU local timers. The global timer is a 64-bit free running
-up-counter and can generate 4 interrupts when the counter reaches one of the
-four preset counter values. The CPU local timers are 32-bit free running
-down-counters and generate an interrupt when the counter expires. There is
-one CPU local timer instantiated in MCT for every CPU in the system.
-
-Required properties:
-
-- compatible: should be "samsung,exynos4210-mct".
- (a) "samsung,exynos4210-mct", for mct compatible with Exynos4210 mct.
- (b) "samsung,exynos4412-mct", for mct compatible with Exynos4412 mct.
-
-- reg: base address of the mct controller and length of the address space
- it occupies.
-
-- interrupts: the list of interrupts generated by the controller. The following
- should be the order of the interrupts specified. The local timer interrupts
- should be specified after the four global timer interrupts have been
- specified.
-
- 0: Global Timer Interrupt 0
- 1: Global Timer Interrupt 1
- 2: Global Timer Interrupt 2
- 3: Global Timer Interrupt 3
- 4: Local Timer Interrupt 0
- 5: Local Timer Interrupt 1
- 6: ..
- 7: ..
- i: Local Timer Interrupt n
-
- For MCT block that uses a per-processor interrupt for local timers, such
- as ones compatible with "samsung,exynos4412-mct", only one local timer
- interrupt might be specified, meaning that all local timers use the same
- per processor interrupt.
-
-Example 1: In this example, the IP contains two local timers, using separate
- interrupts, so two local timer interrupts have been specified,
- in addition to four global timer interrupts.
-
- mct@10050000 {
- compatible = "samsung,exynos4210-mct";
- reg = <0x10050000 0x800>;
- interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
- <0 42 0>, <0 48 0>;
- };
-
-Example 2: In this example, the timer interrupts are connected to two separate
- interrupt controllers. Hence, an interrupt-map is created to map
- the interrupts to the respective interrupt controllers.
-
- mct@101c0000 {
- compatible = "samsung,exynos4210-mct";
- reg = <0x101C0000 0x800>;
- interrupt-parent = <&mct_map>;
- interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
-
- mct_map: mct-map {
- #interrupt-cells = <1>;
- #address-cells = <0>;
- #size-cells = <0>;
- interrupt-map = <0 &gic 0 57 0>,
- <1 &gic 0 69 0>,
- <2 &combiner 12 6>,
- <3 &combiner 12 7>,
- <4 &gic 0 42 0>,
- <5 &gic 0 48 0>;
- };
- };
-
-Example 3: In this example, the IP contains four local timers, but using
- a per-processor interrupt to handle them. Either all the local
- timer interrupts can be specified, with the same interrupt specifier
- value or just the first one.
-
- mct@10050000 {
- compatible = "samsung,exynos4412-mct";
- reg = <0x10050000 0x800>;
-
- /* Both ways are possible in this case. Either: */
- interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
- <0 42 0>;
- /* or: */
- interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
- <0 42 0>, <0 42 0>, <0 42 0>, <0 42 0>;
- };
diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
new file mode 100644
index 000000000000..b96d2877955f
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/samsung,exynos4210-mct.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung Exynos SoC Multi Core Timer (MCT)
+
+maintainers:
+ - Krzysztof Kozlowski <krzk@kernel.org>
+
+description: |+
+ The Samsung's Multi Core Timer (MCT) module includes two main blocks, the
+ global timer and CPU local timers. The global timer is a 64-bit free running
+ up-counter and can generate 4 interrupts when the counter reaches one of the
+ four preset counter values. The CPU local timers are 32-bit free running
+ down-counters and generate an interrupt when the counter expires. There is
+ one CPU local timer instantiated in MCT for every CPU in the system.
+
+properties:
+ compatible:
+ enum:
+ - samsung,exynos4210-mct
+ - samsung,exynos4412-mct
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description: |
+ Interrupts should be put in specific order. This is, the local timer
+ interrupts should be specified after the four global timer interrupts
+ have been specified:
+ 0: Global Timer Interrupt 0
+ 1: Global Timer Interrupt 1
+ 2: Global Timer Interrupt 2
+ 3: Global Timer Interrupt 3
+ 4: Local Timer Interrupt 0
+ 5: Local Timer Interrupt 1
+ 6: ..
+ 7: ..
+ i: Local Timer Interrupt n
+ For MCT block that uses a per-processor interrupt for local timers, such
+ as ones compatible with "samsung,exynos4412-mct", only one local timer
+ interrupt might be specified, meaning that all local timers use the same
+ per processor interrupt.
+ minItems: 5 # 4 Global + 1 local
+ maxItems: 20 # 4 Global + 16 local
+
+required:
+ - compatible
+ - interrupts
+ - reg
+
+examples:
+ - |
+ // In this example, the IP contains two local timers, using separate
+ // interrupts, so two local timer interrupts have been specified,
+ // in addition to four global timer interrupts.
+ mct@10050000 {
+ compatible = "samsung,exynos4210-mct";
+ reg = <0x10050000 0x800>;
+ interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
+ <0 42 0>, <0 48 0>;
+ };
+
+ - |
+ // In this example, the timer interrupts are connected to two separate
+ // interrupt controllers. Hence, an interrupt-map is created to map
+ // the interrupts to the respective interrupt controllers.
+
+ mct@101c0000 {
+ compatible = "samsung,exynos4210-mct";
+ reg = <0x101C0000 0x800>;
+ interrupt-parent = <&mct_map>;
+ interrupts = <0>, <1>, <2>, <3>, <4>, <5>;
+
+ mct_map: mct-map {
+ #interrupt-cells = <1>;
+ #address-cells = <0>;
+ #size-cells = <0>;
+ interrupt-map = <0 &gic 0 57 0>,
+ <1 &gic 0 69 0>,
+ <2 &combiner 12 6>,
+ <3 &combiner 12 7>,
+ <4 &gic 0 42 0>,
+ <5 &gic 0 48 0>;
+ };
+ };
+
+ - |
+ // In this example, the IP contains four local timers, but using
+ // a per-processor interrupt to handle them. Only one first local
+ // interrupt is specified.
+
+ mct@10050000 {
+ compatible = "samsung,exynos4412-mct";
+ reg = <0x10050000 0x800>;
+
+ interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
+ <0 42 0>;
+ };
+
+ - |
+ // In this example, the IP contains four local timers, but using
+ // a per-processor interrupt to handle them. All the local timer
+ // interrupts are specified.
+
+ mct@10050000 {
+ compatible = "samsung,exynos4412-mct";
+ reg = <0x10050000 0x800>;
+
+ interrupts = <0 57 0>, <0 69 0>, <0 70 0>, <0 71 0>,
+ <0 42 0>, <0 42 0>, <0 42 0>, <0 42 0>;
+ };
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v9 6/8] mm: Introduce Reported pages
From: Kirill A. Shutemov @ 2019-09-09 16:33 UTC (permalink / raw)
To: Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas,
Alexander Duyck, mhocko, linux-mm, will, aarcange, virtio-dev,
mst, willy, wei.w.wang, ying.huang, riel, dan.j.williams,
lcapitulino, linux-arm-kernel, osalvador, nitesh, konrad.wilk,
dave.hansen, linux-kernel, pbonzini, akpm, fengguang.wu,
kirill.shutemov
In-Reply-To: <acfe9744deaede8f8c4fa4f40a04514d9f843259.camel@linux.intel.com>
On Mon, Sep 09, 2019 at 09:25:04AM -0700, Alexander Duyck wrote:
> > Proper description for the config option?
>
> I can add one. However the feature doesn't do anything without a caller
> that makes use of it. I guess it would make sense to enable this for
> something such as an out-of-tree module to later use.
Description under 'help' section will not make the option user selectable
if you leave 'bool' without description.
> > > + mutex_lock(&page_reporting_mutex);
> > > +
> > > + /* nothing to do if already in use */
> > > + if (rcu_access_pointer(ph_dev_info)) {
> > > + err = -EBUSY;
> > > + goto err_out;
> > > + }
> >
> > Again, it's from "something went horribly wrong" category.
> > Maybe WARN_ON()?
>
> That one I am not so sure about. Right now we only have one user for the
> page reporting interface. My concern is if we ever have more than one we
> may experience collisions. The device driver requesting this should
> display an error message if it is not able tor register the interface.
Fair enough.
> > > + boundary = kcalloc(MAX_ORDER - PAGE_REPORTING_MIN_ORDER,
> > > + sizeof(struct list_head *) * MIGRATE_TYPES,
> > > + GFP_KERNEL);
> >
> > Could you comment here on why this size of array is allocated?
> > The calculation is not obvious to a reader.
>
> Would something like the following work for you?
> /*
> * Allocate space to store the boundaries for the zone we are
> * actively reporting on. We will need to store one boundary
> * pointer per migratetype, and then we need to have one of these
> * arrays per order for orders greater than or equal to
> * PAGE_REPORTING_MIN_ORDER.
> */
Ack.
--
Kirill A. Shutemov
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC] ARM: omap3: Enable HWMODS for HW Random Number Generator
From: Tony Lindgren @ 2019-09-09 16:35 UTC (permalink / raw)
To: Pali Rohár
Cc: Mark Rutland, devicetree, Paul Walmsley, Aaro Koskinen, Adam Ford,
Russell King, Linux Kernel Mailing List, Tero Kristo, Rob Herring,
Benoît Cousson, Linux-OMAP, Adam Ford, arm-soc
In-Reply-To: <20190909134033.s26eiurpat3iekse@pali>
* Pali Rohár <pali.rohar@gmail.com> [190909 13:41]:
> On Monday 09 September 2019 08:37:09 Adam Ford wrote:
> > I applied this on 5.3 and it is working. I assume the same is true in for-next.
Hmm I noticed I stopped getting RNG data after several rmmod modprobe
cycles, or several hd /dev/random reads. Anybody else seeing that?
> > Do you want to submit a formal patch? I can mark it as 'tested-by'
> > This really helps speed up the startup sequence on boards with sshd
> > because it delays for nearly 80 seconds waiting for entropy without
> > the hwrng.
>
> Hi! When applying a patch, could you please disable this rng for n900?
>
> In omap3-n900.dts for rng should be status = "disabled" (as Tony already
> wrote), similarly like for aes.
Yeah I'll post a proper patch after -rc1.
Regards,
Tony
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 6/6] arm64: dts: khadas-vim3: add commented support for PCIe
From: Marc Zyngier @ 2019-09-09 16:37 UTC (permalink / raw)
To: Neil Armstrong
Cc: lorenzo.pieralisi, khilman, linux-kernel, kishon, repk, linux-pci,
bhelgaas, linux-amlogic, yue.wang, linux-arm-kernel
In-Reply-To: <1567950178-4466-7-git-send-email-narmstrong@baylibre.com>
On Sun, 08 Sep 2019 14:42:58 +0100,
Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> The VIM3 on-board MCU can mux the PCIe/USB3.0 shared differential
> lines using a FUSB340TMX USB 3.1 SuperSpeed Data Switch between
> an USB3.0 Type A connector and a M.2 Key M slot.
> The PHY driving these differential lines is shared between
> the USB3.0 controller and the PCIe Controller, thus only
> a single controller can use it.
>
> The needed DT configuration when the MCU is configured to mux
> the PCIe/USB3.0 differential lines to the M.2 Key M slot is
> added commented and may uncommented to disable USB3.0 from the
> USB Complex and enable the PCIe controller.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> .../amlogic/meson-g12b-a311d-khadas-vim3.dts | 22 +++++++++++++++++++
> .../amlogic/meson-g12b-s922x-khadas-vim3.dts | 22 +++++++++++++++++++
> .../boot/dts/amlogic/meson-khadas-vim3.dtsi | 4 ++++
> .../dts/amlogic/meson-sm1-khadas-vim3l.dts | 22 +++++++++++++++++++
> 4 files changed, 70 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts
> index 3a6a1e0c1e32..0577b1435cbb 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts
> @@ -14,3 +14,25 @@
> / {
> compatible = "khadas,vim3", "amlogic,a311d", "amlogic,g12b";
> };
> +
> +/*
> + * The VIM3 on-board MCU can mux the PCIe/USB3.0 shared differential
> + * lines using a FUSB340TMX USB 3.1 SuperSpeed Data Switch between
> + * an USB3.0 Type A connector and a M.2 Key M slot.
> + * The PHY driving these differential lines is shared between
> + * the USB3.0 controller and the PCIe Controller, thus only
> + * a single controller can use it.
> + * If the MCU is configured to mux the PCIe/USB3.0 differential lines
> + * to the M.2 Key M slot, uncomment the following block to disable
> + * USB3.0 from the USB Complex and enable the PCIe controller.
> + */
> +/*
> +&pcie {
> + status = "okay";
> +};
> +
> +&usb {
> + phys = <&usb2_phy0>, <&usb2_phy1>;
> + phy-names = "usb2-phy0", "usb2-phy1";
> +};
> + */
Although you can't do much more than this here, I'd expect firmware on
the machine to provide the DT that matches its configuration. Is it
the way it actually works? Or is the user actually expected to edit
this file?
Thanks,
M.
--
Jazz is not dead, it just smells funny.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 2/8] mm: Adjust shuffle code to allow for future coalescing
From: Alexander Duyck @ 2019-09-09 16:43 UTC (permalink / raw)
To: Kirill A. Shutemov, Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas, mhocko,
linux-mm, will, aarcange, virtio-dev, mst, willy, wei.w.wang,
ying.huang, riel, dan.j.williams, lcapitulino, linux-arm-kernel,
osalvador, nitesh, konrad.wilk, dave.hansen, linux-kernel,
pbonzini, akpm, fengguang.wu, kirill.shutemov
In-Reply-To: <20190909094700.bbslsxpuwvxmodal@box>
On Mon, 2019-09-09 at 12:47 +0300, Kirill A. Shutemov wrote:
> On Sat, Sep 07, 2019 at 10:25:20AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Move the head/tail adding logic out of the shuffle code and into the
> > __free_one_page function since ultimately that is where it is really
> > needed anyway. By doing this we should be able to reduce the overhead
> > and can consolidate all of the list addition bits in one spot.
> >
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> > include/linux/mmzone.h | 12 --------
> > mm/page_alloc.c | 70 +++++++++++++++++++++++++++---------------------
> > mm/shuffle.c | 9 +-----
> > mm/shuffle.h | 12 ++++++++
> > 4 files changed, 53 insertions(+), 50 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index bda20282746b..125f300981c6 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -116,18 +116,6 @@ static inline void add_to_free_area_tail(struct page *page, struct free_area *ar
> > area->nr_free++;
> > }
> >
> > -#ifdef CONFIG_SHUFFLE_PAGE_ALLOCATOR
> > -/* Used to preserve page allocation order entropy */
> > -void add_to_free_area_random(struct page *page, struct free_area *area,
> > - int migratetype);
> > -#else
> > -static inline void add_to_free_area_random(struct page *page,
> > - struct free_area *area, int migratetype)
> > -{
> > - add_to_free_area(page, area, migratetype);
> > -}
> > -#endif
> > -
> > /* Used for pages which are on another list */
> > static inline void move_to_free_area(struct page *page, struct free_area *area,
> > int migratetype)
>
> Looks like add_to_free_area() and add_to_free_area_tail() can be moved to
> mm/page_alloc.c as all users are there now. And the same for struct
> free_area definition (but not declaration).
This can probably be worked into patch 4 instead of doing it here. I could
pull all the functions that are renamed to _free_list from _free_area into
page_alloc.c and leave behind the ones that remained as _free_area such as
get_page_from_free_area. That should make it easier for me to avoid having
to include page_reporting.h in mmzone.h.
I'm not sure I follow what you are saying about the free_area definition.
It looks like it is a part of the zone structure so I would think it still
needs to be defined in the header.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers
From: Gerald Schaefer @ 2019-09-09 16:51 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Tetsuo Handa, Heiko Carstens, Michal Hocko, linux-mm, Dave Hansen,
Paul Mackerras, sparclinux, Thomas Gleixner, linux-s390,
Michael Ellerman, x86, Russell King - ARM Linux, Matthew Wilcox,
Steven Price, Jason Gunthorpe, linux-arm-kernel, linux-snps-arc,
Kees Cook, Masahiro Yamada, Mark Brown, Dan Williams,
Vlastimil Babka, Sri Krishna chowdary, Ard Biesheuvel,
Greg Kroah-Hartman, linux-mips, Ralf Baechle, linux-kernel,
Paul Burton, Mike Rapoport, Vineet Gupta, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <3d5de35f-8192-1c75-50a9-03e66e3b8e5c@arm.com>
On Mon, 9 Sep 2019 11:56:50 +0530
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
[..]
> >
> > Hmm, I simply used this on my system to make pud_clear_tests() work, not
> > sure if it works on all archs:
> >
> > pud_val(*pudp) |= RANDOM_NZVALUE;
>
> Which compiles on arm64 but then fails on x86 because of the way pmd_val()
> has been defined there. on arm64 and s390 (with many others) pmd_val() is
> a macro which still got the variable that can be used as lvalue but that is
> not true for some other platforms like x86.
>
> arch/arm64/include/asm/pgtable-types.h: #define pmd_val(x) ((x).pmd)
> arch/s390/include/asm/page.h: #define pmd_val(x) ((x).pmd)
> arch/x86/include/asm/pgtable.h: #define pmd_val(x) native_pmd_val(x)
>
> static inline pmdval_t native_pmd_val(pmd_t pmd)
> {
> return pmd.pmd;
> }
>
> Unless I am mistaken, the return value from this function can not be used as
> lvalue for future assignments.
>
> mm/arch_pgtable_test.c: In function ‘pud_clear_tests’:
> mm/arch_pgtable_test.c:156:17: error: lvalue required as left operand of assignment
> pud_val(*pudp) |= RANDOM_ORVALUE;
> ^~
> AFAICS pxx_val() were never intended to be used as lvalue and using it that way
> might just happen to work on all those platforms which define them as macros.
> They meant to just provide values for an entry as being determined by the platform.
>
> In principle pxx_val() on an entry was not supposed to be modified directly from
> generic code without going through (again) platform helpers for any specific state
> change (write, old, dirty, special, huge etc). The current use case is a deviation
> for that.
>
> I originally went with memset() just to load up the entries with non-zero value so
> that we know pxx_clear() are really doing the clearing. The same is being followed
> for all pxx_same() checks.
>
> Another way for fixing the problem would be to mark them with known attributes
> like write/young/huge etc instead which for sure will create non-zero entries.
> We can do that for pxx_clear() and pxx_same() tests and drop RANDOM_NZVALUE
> completely. Does that sound good ?
Umm, not really. Those mkwrite/young/huge etc. helpers do only exist for
page table levels where we can also have large mappings, at least on s390.
Also, we do (on s390) again check for certain sanity before actually setting
the bits.
Good news is that at least for the pxx_same() checks the memset() is no
problem, because pxx_same() does not do any checks other than the same check.
For the pxx_clear_tests(), maybe it could be an option to put them behind the
pxx_populate_tests(), and rely on them having properly populated (non-clear)
values after that?
[...]
> >
> > Actually, using get_unmapped_area() as suggested by Kirill could also
> > solve this issue. We do create a new mm with 3-level page tables on s390,
> > and the dynamic upgrade to 4 or 5 levels is then triggered exactly by
> > arch_get_unmapped_area(), depending on the addr. But I currently don't
> > see how / where arch_get_unmapped_area() is set up for such a dummy mm
> > created by mm_alloc().
>
> Normally they are set during program loading but we can set it up explicitly
> for the test mm_struct if we need to but there are some other challenges.
>
> load_[aout|elf|flat|..]_binary()
> setup_new_exec()
> arch_pick_mmap_layout().
>
> I did some initial experiments around get_unmapped_area(). Seems bit tricky
> to get it working on a pure 'test' mm_struct. It expects a real user context
> in the form of current->mm.
Yes, that's where I stopped because it looked rather complicated :-)
Not sure why Kirill suggested it initially, but if using get_unmapped_area()
would only be necessary to get properly initialized page table levels
on s390, you could also defer this to a later add-on patch.
Regards,
Gerald
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH net-next] net: stmmac: pci: Add HAPS support using GMAC5
From: Jose Abreu @ 2019-09-09 16:54 UTC (permalink / raw)
To: netdev
Cc: Jose Abreu, Joao Pinto, Alexandre Torgue, linux-kernel,
linux-stm32, Maxime Coquelin, Giuseppe Cavallaro, David S. Miller,
linux-arm-kernel
Add the support for Synopsys HAPS board that uses GMAC5.
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
---
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 71 ++++++++++++++++++++++++
1 file changed, 71 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 20906287b6d4..292045f4581f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -375,6 +375,75 @@ static const struct stmmac_pci_info quark_pci_info = {
.setup = quark_default_data,
};
+static int snps_gmac5_default_data(struct pci_dev *pdev,
+ struct plat_stmmacenet_data *plat)
+{
+ int i;
+
+ plat->clk_csr = 5;
+ plat->has_gmac4 = 1;
+ plat->force_sf_dma_mode = 1;
+ plat->tso_en = 1;
+ plat->pmt = 1;
+
+ plat->mdio_bus_data->phy_mask = 0;
+
+ /* Set default value for multicast hash bins */
+ plat->multicast_filter_bins = HASH_TABLE_SIZE;
+
+ /* Set default value for unicast filter entries */
+ plat->unicast_filter_entries = 1;
+
+ /* Set the maxmtu to a default of JUMBO_LEN */
+ plat->maxmtu = JUMBO_LEN;
+
+ /* Set default number of RX and TX queues to use */
+ plat->tx_queues_to_use = 4;
+ plat->rx_queues_to_use = 4;
+
+ plat->tx_sched_algorithm = MTL_TX_ALGORITHM_WRR;
+ for (i = 0; i < plat->tx_queues_to_use; i++) {
+ plat->tx_queues_cfg[i].use_prio = false;
+ plat->tx_queues_cfg[i].mode_to_use = MTL_QUEUE_DCB;
+ plat->tx_queues_cfg[i].weight = 25;
+ }
+
+ plat->rx_sched_algorithm = MTL_RX_ALGORITHM_SP;
+ for (i = 0; i < plat->rx_queues_to_use; i++) {
+ plat->rx_queues_cfg[i].use_prio = false;
+ plat->rx_queues_cfg[i].mode_to_use = MTL_QUEUE_DCB;
+ plat->rx_queues_cfg[i].pkt_route = 0x0;
+ plat->rx_queues_cfg[i].chan = i;
+ }
+
+ plat->bus_id = 1;
+ plat->phy_addr = -1;
+ plat->interface = PHY_INTERFACE_MODE_GMII;
+
+ plat->dma_cfg->pbl = 32;
+ plat->dma_cfg->pblx8 = true;
+
+ /* Axi Configuration */
+ plat->axi = devm_kzalloc(&pdev->dev, sizeof(*plat->axi), GFP_KERNEL);
+ if (!plat->axi)
+ return -ENOMEM;
+
+ plat->axi->axi_wr_osr_lmt = 31;
+ plat->axi->axi_rd_osr_lmt = 31;
+
+ plat->axi->axi_fb = false;
+ plat->axi->axi_blen[0] = 4;
+ plat->axi->axi_blen[1] = 8;
+ plat->axi->axi_blen[2] = 16;
+ plat->axi->axi_blen[3] = 32;
+
+ return 0;
+}
+
+static const struct stmmac_pci_info snps_gmac5_pci_info = {
+ .setup = snps_gmac5_default_data,
+};
+
/**
* stmmac_pci_probe
*
@@ -518,6 +587,7 @@ static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_pci_suspend, stmmac_pci_resume);
#define STMMAC_EHL_RGMII1G_ID 0x4b30
#define STMMAC_EHL_SGMII1G_ID 0x4b31
#define STMMAC_TGL_SGMII1G_ID 0xa0ac
+#define STMMAC_GMAC5_ID 0x7102
#define STMMAC_DEVICE(vendor_id, dev_id, info) { \
PCI_VDEVICE(vendor_id, dev_id), \
@@ -531,6 +601,7 @@ static const struct pci_device_id stmmac_id_table[] = {
STMMAC_DEVICE(INTEL, STMMAC_EHL_RGMII1G_ID, ehl_rgmii1g_pci_info),
STMMAC_DEVICE(INTEL, STMMAC_EHL_SGMII1G_ID, ehl_sgmii1g_pci_info),
STMMAC_DEVICE(INTEL, STMMAC_TGL_SGMII1G_ID, tgl_sgmii1g_pci_info),
+ STMMAC_DEVICE(SYNOPSYS, STMMAC_GMAC5_ID, snps_gmac5_pci_info),
{}
};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2 02/14] soc: ti: k3: add navss ringacc driver
From: Grygorii Strashko @ 2019-09-09 16:58 UTC (permalink / raw)
To: Peter Ujfalusi, Tero Kristo, vkoul, robh+dt, nm, ssantosh
Cc: devicetree, lokeshvutla, j-keerthy, linux-kernel, tony, dmaengine,
dan.j.williams, linux-arm-kernel
In-Reply-To: <ba083c56-782b-3c44-2778-b56f4a5be912@ti.com>
On 09/09/2019 16:00, Peter Ujfalusi wrote:
> Hi,
>
> Grygorii, can you take a look?
>
> On 09/09/2019 9.09, Tero Kristo wrote:
>> Hi,
>>
>> Mostly some cosmetic comments below, other than that seems fine to me.
>>
>> On 30/07/2019 12:34, Peter Ujfalusi wrote:
>>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>>
>>> The Ring Accelerator (RINGACC or RA) provides hardware acceleration to
>>> enable straightforward passing of work between a producer and a consumer.
>>> There is one RINGACC module per NAVSS on TI AM65x SoCs.
>>>
>>> The RINGACC converts constant-address read and write accesses to
>>> equivalent
>>> read or write accesses to a circular data structure in memory. The
>>> RINGACC
>>> eliminates the need for each DMA controller which needs to access ring
>>> elements from having to know the current state of the ring (base address,
>>> current offset). The DMA controller performs a read or write access to a
>>> specific address range (which maps to the source interface on the
>>> RINGACC)
>>> and the RINGACC replaces the address for the transaction with a new
>>> address
>>> which corresponds to the head or tail element of the ring (head for
>>> reads,
>>> tail for writes). Since the RINGACC maintains the state, multiple DMA
>>> controllers or channels are allowed to coherently share the same rings as
>>> applicable. The RINGACC is able to place data which is destined towards
>>> software into cached memory directly.
>>>
>>> Supported ring modes:
>>> - Ring Mode
>>> - Messaging Mode
>>> - Credentials Mode
>>> - Queue Manager Mode
>>>
>>> TI-SCI integration:
>>>
>>> Texas Instrument's System Control Interface (TI-SCI) Message Protocol now
>>> has control over Ringacc module resources management (RM) and Rings
>>> configuration.
>>>
>>> The corresponding support of TI-SCI Ringacc module RM protocol
>>> introduced as option through DT parameters:
>>> - ti,sci: phandle on TI-SCI firmware controller DT node
>>> - ti,sci-dev-id: TI-SCI device identifier as per TI-SCI firmware spec
>>>
>>> if both parameters present - Ringacc driver will configure/free/reset
>>> Rings
>>> using TI-SCI Message Ringacc RM Protocol.
>>>
>>> The Ringacc driver manages Rings allocation by itself now and requests
>>> TI-SCI firmware to allocate and configure specific Rings only. It's done
>>> this way because, Linux driver implements two stage Rings allocation and
>>> configuration (allocate ring and configure ring) while I-SCI Message
>>
>> I-SCI should be TI-SCI I believe.
>
> Yes, it supposed to be.
>
>>
>>> Protocol supports only one combined operation (allocate+configure).
>>>
>>> Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Above seems to be missing SoB?
>
> Oh, it is really missing.
>
>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>> ---
>>> drivers/soc/ti/Kconfig | 17 +
>>> drivers/soc/ti/Makefile | 1 +
>>> drivers/soc/ti/k3-ringacc.c | 1191 +++++++++++++++++++++++++++++
>>> include/linux/soc/ti/k3-ringacc.h | 262 +++++++
>>> 4 files changed, 1471 insertions(+)
>>> create mode 100644 drivers/soc/ti/k3-ringacc.c
>>> create mode 100644 include/linux/soc/ti/k3-ringacc.h
>>>
>>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>>> index cf545f428d03..10c76faa503e 100644
>>> --- a/drivers/soc/ti/Kconfig
>>> +++ b/drivers/soc/ti/Kconfig
>>> @@ -80,6 +80,23 @@ config TI_SCI_PM_DOMAINS
>>> called ti_sci_pm_domains. Note this is needed early in boot
>>> before
>>> rootfs may be available.
>>> +config TI_K3_RINGACC
>>> + tristate "K3 Ring accelerator Sub System"
>>> + depends on ARCH_K3 || COMPILE_TEST
>>> + depends on TI_SCI_INTA_IRQCHIP
>>> + default y
>>> + help
>>> + Say y here to support the K3 Ring accelerator module.
>>> + The Ring Accelerator (RINGACC or RA) provides hardware
>>> acceleration
>>> + to enable straightforward passing of work between a producer
>>> + and a consumer. There is one RINGACC module per NAVSS on TI
>>> AM65x SoCs
>>> + If unsure, say N.
>>> +
>>> +config TI_K3_RINGACC_DEBUG
>>> + tristate "K3 Ring accelerator Sub System tests and debug"
>>> + depends on TI_K3_RINGACC
>>> + default n
>>> +
>>> endif # SOC_TI
>>> config TI_SCI_INTA_MSI_DOMAIN
>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>> index b3868d392d4f..cc4bc8b08bf5 100644
>>> --- a/drivers/soc/ti/Makefile
>>> +++ b/drivers/soc/ti/Makefile
>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_AMX3_PM) += pm33xx.o
>>> obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o
>>> obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o
>>> obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN) += ti_sci_inta_msi.o
>>> +obj-$(CONFIG_TI_K3_RINGACC) += k3-ringacc.o
>>> diff --git a/drivers/soc/ti/k3-ringacc.c b/drivers/soc/ti/k3-ringacc.c
>>> new file mode 100644
>>> index 000000000000..401dfc963319
>>> --- /dev/null
>>> +++ b/drivers/soc/ti/k3-ringacc.c
>>> @@ -0,0 +1,1191 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * TI K3 NAVSS Ring Accelerator subsystem driver
>>> + *
>>> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com
>>> + */
>>> +
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/soc/ti/k3-ringacc.h>
>>> +#include <linux/soc/ti/ti_sci_protocol.h>
>>> +#include <linux/soc/ti/ti_sci_inta_msi.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/irqdomain.h>
>>> +
>>> +static LIST_HEAD(k3_ringacc_list);
>>> +static DEFINE_MUTEX(k3_ringacc_list_lock);
>>> +
>>> +#ifdef CONFIG_TI_K3_RINGACC_DEBUG
>>> +#define k3_nav_dbg(dev, arg...) dev_err(dev, arg)
>>
>> dev_err seems exaggeration for debug purposes, maybe just dev_info.
>>
>>> +static void dbg_writel(u32 v, void __iomem *reg)
>>> +{
>>> + pr_err("WRITEL(32): v(%08X)-->reg(%p)\n", v, reg);
>>
>> Again, maybe just pr_info.
>
> I think I'll just drop CONFIG_TI_K3_RINGACC_DEBUG altogether along with
> dbg_writel/dbg_readl/k3_nav_dbg and use dev_dbg() when appropriate.
Sounds good.
>
>>
>>> + writel(v, reg);
>>> +}
>>> +
>>> +static u32 dbg_readl(void __iomem *reg)
>>> +{
>>> + u32 v;
>>> +
>>> + v = readl(reg);
>>> + pr_err("READL(32): v(%08X)<--reg(%p)\n", v, reg);
>>> + return v;
>>> +}
>>> +#else
>>> +#define k3_nav_dbg(dev, arg...) dev_dbg(dev, arg)
>>> +#define dbg_writel(v, reg) writel(v, reg)
>>
>> Do you need to use hard writel, writel_relaxed is not enough?
>
> not sure if we really need the barriers, but __raw_writel() should be
> fine here imho
xxx_relaxed relaxed versions should be used only when necessary and with
adding appropriate comments why they've been used and what benefits from using
them for each particular case.
So, i do not agree with this blind conversation.
>
>>> +
>>> +#define dbg_readl(reg) readl(reg)
>>
>> Same as above but for read?
>
> __raw_readl() could be fine in also.
No. __raw_xxx api should never be used by drivers.
>
> ...
>
>>> +/**
>>> + * struct k3_ringacc - Rings accelerator descriptor
>>> + *
>>> + * @dev - pointer on RA device
>>> + * @proxy_gcfg - RA proxy global config registers
>>> + * @proxy_target_base - RA proxy datapath region
>>> + * @num_rings - number of ring in RA
>>> + * @rm_gp_range - general purpose rings range from tisci
>>> + * @dma_ring_reset_quirk - DMA reset w/a enable
>>> + * @num_proxies - number of RA proxies
>>> + * @rings - array of rings descriptors (struct @k3_ring)
>>> + * @list - list of RAs in the system
>>> + * @tisci - pointer ti-sci handle
>>> + * @tisci_ring_ops - ti-sci rings ops
>>> + * @tisci_dev_id - ti-sci device id
>>> + */
...
>>> +
>>> +#ifdef CONFIG_TI_K3_RINGACC_DEBUG
>>> +void k3_ringacc_ring_dump(struct k3_ring *ring)
>>> +{
>>> + struct device *dev = ring->parent->dev;
>>> +
>>> + k3_nav_dbg(dev, "dump ring: %d\n", ring->ring_id);
>>> + k3_nav_dbg(dev, "dump mem virt %p, dma %pad\n",
>>> + ring->ring_mem_virt, &ring->ring_mem_dma);
>>> + k3_nav_dbg(dev, "dump elmsize %d, size %d, mode %d, proxy_id %d\n",
>>> + ring->elm_size, ring->size, ring->mode, ring->proxy_id);
>>> +
>>> + k3_nav_dbg(dev, "dump ring_rt_regs: db%08x\n",
>>> + readl(&ring->rt->db));
>>
>> Why not use readl_relaxed in this func?
>
> __raw_readl() might be enough?
No Raw, but this seems only one place where relaxed version can be used.
>
>>
>>> + k3_nav_dbg(dev, "dump occ%08x\n",
>>> + readl(&ring->rt->occ));
>>> + k3_nav_dbg(dev, "dump indx%08x\n",
>>> + readl(&ring->rt->indx));
>>> + k3_nav_dbg(dev, "dump hwocc%08x\n",
>>> + readl(&ring->rt->hwocc));
>>> + k3_nav_dbg(dev, "dump hwindx%08x\n",
>>> + readl(&ring->rt->hwindx));
>>> +
>>> + if (ring->ring_mem_virt)
>>> + print_hex_dump(KERN_ERR, "dump ring_mem_virt ",
>>> + DUMP_PREFIX_NONE, 16, 1,
>>> + ring->ring_mem_virt, 16 * 8, false);
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_dump);
>>
>> Do you really need to export a debug function?
>
> It might come helpful for clients to dump the ring status runtime, but
> since we don't have users, I'll move it to static.
Yep. It was exported for debug purposes. But hence there are no active users - cna be removed.
>
>>> +#endif
>>> +
>>> +struct k3_ring *k3_ringacc_request_ring(struct k3_ringacc *ringacc,
>>> + int id, u32 flags)
>>> +{
>>> + int proxy_id = K3_RINGACC_PROXY_NOT_USED;
>>> +
>>> + mutex_lock(&ringacc->req_lock);
>>> +
>>> + if (id == K3_RINGACC_RING_ID_ANY) {
>>> + /* Request for any general purpose ring */
>>> + struct ti_sci_resource_desc *gp_rings =
>>> + &ringacc->rm_gp_range->desc[0];
>>> + unsigned long size;
>>> +
>>> + size = gp_rings->start + gp_rings->num;
>>> + id = find_next_zero_bit(ringacc->rings_inuse, size,
>>> + gp_rings->start);
>>> + if (id == size)
>>> + goto error;
>>> + } else if (id < 0) {
>>> + goto error;
>>> + }
>>> +
>>> + if (test_bit(id, ringacc->rings_inuse) &&
>>> + !(ringacc->rings[id].flags & K3_RING_FLAG_SHARED))
>>> + goto error;
>>> + else if (ringacc->rings[id].flags & K3_RING_FLAG_SHARED)
>>> + goto out;
>>> +
>>> + if (flags & K3_RINGACC_RING_USE_PROXY) {
>>> + proxy_id = find_next_zero_bit(ringacc->proxy_inuse,
>>> + ringacc->num_proxies, 0);
>>> + if (proxy_id == ringacc->num_proxies)
>>> + goto error;
>>> + }
>>> +
>>> + if (!try_module_get(ringacc->dev->driver->owner))
>>> + goto error;
>>> +
>>> + if (proxy_id != K3_RINGACC_PROXY_NOT_USED) {
>>> + set_bit(proxy_id, ringacc->proxy_inuse);
>>> + ringacc->rings[id].proxy_id = proxy_id;
>>> + k3_nav_dbg(ringacc->dev, "Giving ring#%d proxy#%d\n",
>>> + id, proxy_id);
>>> + } else {
>>> + k3_nav_dbg(ringacc->dev, "Giving ring#%d\n", id);
>>> + }
>>> +
>>> + set_bit(id, ringacc->rings_inuse);
>>> +out:
>>> + ringacc->rings[id].use_count++;
>>> + mutex_unlock(&ringacc->req_lock);
>>> + return &ringacc->rings[id];
>>> +
>>> +error:
>>> + mutex_unlock(&ringacc->req_lock);
>>> + return NULL;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_request_ring);
>>> +
>>> +static void k3_ringacc_ring_reset_sci(struct k3_ring *ring)
>>> +{
>>> + struct k3_ringacc *ringacc = ring->parent;
>>> + int ret;
>>> +
>>> + ret = ringacc->tisci_ring_ops->config(
>>> + ringacc->tisci,
>>> + TI_SCI_MSG_VALUE_RM_RING_COUNT_VALID,
>>> + ringacc->tisci_dev_id,
>>> + ring->ring_id,
>>> + 0,
>>> + 0,
>>> + ring->size,
>>> + 0,
>>> + 0,
>>> + 0);
>>> + if (ret)
>>> + dev_err(ringacc->dev, "TISCI reset ring fail (%d) ring_idx
>>> %d\n",
>>> + ret, ring->ring_id);
>>
>> Return value of sci ops is masked, why not return it and let the caller
>> handle it properly?
>>
>> Same comment for anything similar that follows.
>
> Hrm, there is not much a caller can do other than PANIC in case the ring
> configuration fails.
> I can probagate the error, but not sure what action can be taken, if any.
>
>>> +}
>>> +
>>> +void k3_ringacc_ring_reset(struct k3_ring *ring)
>>> +{
>>> + if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>>> + return;
>>> +
>>> + ring->occ = 0;
>>> + ring->free = 0;
>>> + ring->rindex = 0;
>>> + ring->windex = 0;
>>> +
>>> + k3_ringacc_ring_reset_sci(ring);
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_reset);
>>> +
>>> +static void k3_ringacc_ring_reconfig_qmode_sci(struct k3_ring *ring,
>>> + enum k3_ring_mode mode)
>>> +{
>>> + struct k3_ringacc *ringacc = ring->parent;
>>> + int ret;
>>> +
>>> + ret = ringacc->tisci_ring_ops->config(
>>> + ringacc->tisci,
>>> + TI_SCI_MSG_VALUE_RM_RING_MODE_VALID,
>>> + ringacc->tisci_dev_id,
>>> + ring->ring_id,
>>> + 0,
>>> + 0,
>>> + 0,
>>> + mode,
>>> + 0,
>>> + 0);
>>> + if (ret)
>>> + dev_err(ringacc->dev, "TISCI reconf qmode fail (%d) ring_idx
>>> %d\n",
>>> + ret, ring->ring_id);
>>> +}
>>> +
>>> +void k3_ringacc_ring_reset_dma(struct k3_ring *ring, u32 occ)
>>> +{
>>> + if (!ring || !(ring->flags & K3_RING_FLAG_BUSY))
>>> + return;
>>> +
>>> + if (!ring->parent->dma_ring_reset_quirk)
>>> + return;
>>> +
>>> + if (!occ)
>>> + occ = dbg_readl(&ring->rt->occ);
>>> +
>>> + if (occ) {
>>> + u32 db_ring_cnt, db_ring_cnt_cur;
>>> +
>>> + k3_nav_dbg(ring->parent->dev, "%s %u occ: %u\n", __func__,
>>> + ring->ring_id, occ);
>>> + /* 2. Reset the ring */
>>
>> 2? Where is 1?
>
> Oh, I'll fix the numbering.
1. is 'Get ring occupancy count"
I think you can just drop numbering
>
>>
>>> + k3_ringacc_ring_reset_sci(ring);
>>> +
>>> + /*
>>> + * 3. Setup the ring in ring/doorbell mode
>>> + * (if not already in this mode)
>>> + */
>>> + if (ring->mode != K3_RINGACC_RING_MODE_RING)
>>> + k3_ringacc_ring_reconfig_qmode_sci(
>>> + ring, K3_RINGACC_RING_MODE_RING);
>>> + /*
>>> + * 4. Ring the doorbell 2**22 – ringOcc times.
>>> + * This will wrap the internal UDMAP ring state occupancy
>>> + * counter (which is 21-bits wide) to 0.
>>> + */
>>> + db_ring_cnt = (1U << 22) - occ;
>>> +
>>> + while (db_ring_cnt != 0) {
>>> + /*
>>> + * Ring the doorbell with the maximum count each
>>> + * iteration if possible to minimize the total
>>> + * of writes
>>> + */
>>> + if (db_ring_cnt > K3_RINGACC_MAX_DB_RING_CNT)
>>> + db_ring_cnt_cur = K3_RINGACC_MAX_DB_RING_CNT;
>>> + else
>>> + db_ring_cnt_cur = db_ring_cnt;
>>> +
>>> + writel(db_ring_cnt_cur, &ring->rt->db);
>>> + db_ring_cnt -= db_ring_cnt_cur;
>>> + }
>>> +
>>> + /* 5. Restore the original ring mode (if not ring mode) */
>>> + if (ring->mode != K3_RINGACC_RING_MODE_RING)
>>> + k3_ringacc_ring_reconfig_qmode_sci(ring, ring->mode);
>>> + }
>>> +
>>> + /* 2. Reset the ring */
>>
>>> +
>>> +u32 k3_ringacc_get_tisci_dev_id(struct k3_ring *ring)
>>> +{
>>> + if (!ring)
>>> + return -EINVAL;
>>> +
>>
>> What if parent is NULL? Can it ever be here?
>
> No, parent can not be NULL as the client would not have the ring in the
> first place.
>
>>
>>> + return ring->parent->tisci_dev_id;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_get_tisci_dev_id);
>>> +
>>> +int k3_ringacc_get_ring_irq_num(struct k3_ring *ring)
>>> +{
>>> + int irq_num;
>>> +
>>> + if (!ring)
>>> + return -EINVAL;
>>> +
>>> + irq_num = ti_sci_inta_msi_get_virq(ring->parent->dev,
>>> ring->ring_id);
>>> + if (irq_num <= 0)
>>> + irq_num = -EINVAL;
>>> + return irq_num;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_ringacc_get_ring_irq_num);
>>> +
>>> +static int k3_ringacc_ring_cfg_sci(struct k3_ring *ring)
>>> +{
>>> + struct k3_ringacc *ringacc = ring->parent;
>>> + u32 ring_idx;
>>> + int ret;
>>> +
>>> + if (!ringacc->tisci)
>>> + return -EINVAL;
>>> +
>>> + ring_idx = ring->ring_id;
>>> + ret = ringacc->tisci_ring_ops->config(
>>> + ringacc->tisci,
>>> + TI_SCI_MSG_VALUE_RM_ALL_NO_ORDER,
>>> + ringacc->tisci_dev_id,
>>> + ring_idx,
>>> + lower_32_bits(ring->ring_mem_dma),
>>> + upper_32_bits(ring->ring_mem_dma),
>>> + ring->size,
>>> + ring->mode,
>>> + ring->elm_size,
>>> + 0);
>>> + if (ret)
>>> + dev_err(ringacc->dev, "TISCI config ring fail (%d) ring_idx
>>> %d\n",
>>> + ret, ring_idx);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +int k3_ringacc_ring_cfg(struct k3_ring *ring, struct k3_ring_cfg *cfg)
>>> +{
>>> + struct k3_ringacc *ringacc = ring->parent;
>>> + int ret = 0;
>>> +
>>> + if (!ring || !cfg)
>>> + return -EINVAL;
>>> + if (cfg->elm_size > K3_RINGACC_RING_ELSIZE_256 ||
>>> + cfg->mode > K3_RINGACC_RING_MODE_QM ||
>>> + cfg->size & ~K3_RINGACC_CFG_RING_SIZE_ELCNT_MASK ||
>>> + !test_bit(ring->ring_id, ringacc->rings_inuse))
>>> + return -EINVAL;
>>> +
>>> + if (ring->use_count != 1)
>>
>> Hmm, isn't this a failure actually?
>
> Yes, it is: -EBUSY
No. This is for shared rings.
0 - should never happens once ring is requested.
1 - only one user - configure ring
>1 - shared ring which is configured already - just exit as ring configure already.
>
>>> + return 0;
>>> +
>>> + ring->size = cfg->size;
>>> + ring->elm_size = cfg->elm_size;
>>> + ring->mode = cfg->mode;
>>> + ring->occ = 0;
>>> + ring->free = 0;
>>> + ring->rindex = 0;
>>> + ring->windex = 0;
>>> +
[...]
--
Best regards,
grygorii
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 2/8] mm: Adjust shuffle code to allow for future coalescing
From: Kirill A. Shutemov @ 2019-09-09 17:00 UTC (permalink / raw)
To: Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas,
Alexander Duyck, mhocko, linux-mm, will, aarcange, virtio-dev,
mst, willy, wei.w.wang, ying.huang, riel, dan.j.williams,
lcapitulino, linux-arm-kernel, osalvador, nitesh, konrad.wilk,
dave.hansen, linux-kernel, pbonzini, akpm, fengguang.wu,
kirill.shutemov
In-Reply-To: <171e0e86cde2012e8bda647c0370e902768ba0b5.camel@linux.intel.com>
On Mon, Sep 09, 2019 at 09:43:00AM -0700, Alexander Duyck wrote:
> I'm not sure I follow what you are saying about the free_area definition.
> It looks like it is a part of the zone structure so I would think it still
> needs to be defined in the header.
Yeah, you are right. I didn't noticed this.
--
Kirill A. Shutemov
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 4/4] arm64: dts: add support for A1 based Amlogic AD401
From: Martin Blumenstingl @ 2019-09-09 17:24 UTC (permalink / raw)
To: Jianxin Pan
Cc: devicetree, Hanjie Lin, Victor Wan, Neil Armstrong, Kevin Hilman,
linux-kernel, Qiufang Dai, Rob Herring, Jian Hu, Xingyu Chen,
Tao Zeng, Carlo Caione, linux-amlogic, linux-arm-kernel,
Jerome Brunet
In-Reply-To: <a82336e2-44df-5682-1c86-daf8a8448d30@amlogic.com>
Hi Jianxin,
On Mon, Sep 9, 2019 at 2:03 PM Jianxin Pan <jianxin.pan@amlogic.com> wrote:
>
> Hi Martin,
>
> On 2019/9/7 23:02, Martin Blumenstingl wrote:
> > Hi Jianxin,
> >
> > On Fri, Sep 6, 2019 at 7:58 AM Jianxin Pan <jianxin.pan@amlogic.com> wrote:
> > [...]
> >>> also I'm a bit surprised to see no busses (like aobus, cbus, periphs, ...) here
> >>> aren't there any busses defined in the A1 SoC implementation or are
> >>> were you planning to add them later?
> >> Unlike previous series,there is no Cortex-M3 AO CPU in A1, and there is no AO/EE power domain.
> >> Most of the registers are on the apb_32b bus. aobus, cbus and periphs are not used in A1.
> > OK, thank you for the explanation
> > since you're going to re-send the patch anyways: can you please
> > include the apb_32b bus?
> > all other upstream Amlogic .dts are using the bus definitions, so that
> > will make A1 consistent with the other SoCs
> In A1 (and the later C1), BUS is not mentioned in the memmap and register spec.
> Registers are organized and grouped by functions, and we can not find information about buses from the SoC document.
do you know why the busses are not part of the documentation?
> Maybe it's better to remove bus definitions for these chips.
my understanding is that devicetree describes the hardware
so if there's a bus in hardware (that we know about) then we should
describe it in devicetree
personally I think busses also make the .dts easier to read:
instead of a huge .dts with all nodes on one level it's split into
multiple smaller sub-nodes - thus making it easier to keep track of
"where am I in this file".
Martin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 06/11] kselftest: arm64: fake_sigreturn_bad_magic
From: Cristian Marussi @ 2019-09-09 17:31 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114845.GW27757@arm.com>
Hi
On 04/09/2019 12:48, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:27pm +0100, Cristian Marussi wrote:
>> Add a simple fake_sigreturn testcase which builds a ucontext_t with a bad
>> magic header and place it onto the stack. Expects a SIGSEGV on test PASS.
>>
>> Introduce a common utility assembly trampoline function to invoke a
>> sigreturn while placing the provided sigframe at wanted alignment and
>> also an helper to make space when needed inside the sigframe reserved
>> area.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit
>> - fix signal.S, handle misalign requests too
>> - remove unneeded comments
>> - add signal.h include
>> - added get_starting_head() helper
>> - added test description
>> ---
>> tools/testing/selftests/arm64/signal/Makefile | 2 +-
>> .../testing/selftests/arm64/signal/signals.S | 62 +++++++++++++++++++
>> .../arm64/signal/test_signals_utils.h | 1 +
>> .../testcases/fake_sigreturn_bad_magic.c | 54 ++++++++++++++++
>> .../arm64/signal/testcases/testcases.c | 28 +++++++++
>> .../arm64/signal/testcases/testcases.h | 4 ++
>> 6 files changed, 150 insertions(+), 1 deletion(-)
>> create mode 100644 tools/testing/selftests/arm64/signal/signals.S
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile
>> index f78f5190e3d4..b497cfea4643 100644
>> --- a/tools/testing/selftests/arm64/signal/Makefile
>> +++ b/tools/testing/selftests/arm64/signal/Makefile
>> @@ -28,5 +28,5 @@ clean:
>> # Common test-unit targets to build common-layout test-cases executables
>> # Needs secondary expansion to properly include the testcase c-file in pre-reqs
>> .SECONDEXPANSION:
>> -$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
>> +$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c signals.S $$@.c test_signals.h test_signals_utils.h testcases/testcases.h
>> $(CC) $(CFLAGS) $^ -o $@
>> diff --git a/tools/testing/selftests/arm64/signal/signals.S b/tools/testing/selftests/arm64/signal/signals.S
>> new file mode 100644
>> index 000000000000..b89fec0d5ba0
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/signals.S
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 ARM Limited */
>> +
>> +#include <asm/unistd.h>
>> +
>> +.section .rodata, "a"
>> +call_fmt:
>> + .asciz "Calling sigreturn with fake sigframe sized:%zd at SP @%08lX\n"
>> +
>> +.text
>> +
>> +.globl fake_sigreturn
>> +
>> +/* fake_sigreturn x0:&sigframe, x1:sigframe_size, x2:misalign_bytes */
>> +fake_sigreturn:
>
> Nit: the "bl printf" later on destroys lr.
>
> This isn't a problem, since the function never tries to return anyway --
> if things go wrong you just "b .".
>
> But it may be helpful for debug purposes to at least create a frame
> record, e.g.:
>
> stp x29, x30, [sp, #-16]!
> mov x29, sp
>
> before doing anything else.
>
ok
>> + mov x20, x0
>> + mov x21, x1
>> + mov x22, x2
>> + mov x23, sp
>
> Nit: to follow the conventional asm style for arm64 kernel code, can you
> format lines as
>
> <TAB> op<TAB> operands
>
ok
>> +
>> + /* create space on the stack for fake sigframe 16 bytes-aligned */
>> + add x0, x21, #16
>> + bic x0, x0, #15
>> + sub x23, x23, x0
>> + /* any misalignment requested ? */
>> + add x23, x23, x22
>
> Aren't we actually reducing the allocation here, rather than increasing it?
>
For the misalignment bytes if any yes...
> Doing something like this may work to allocate guaranteed sufficient
> space:
>
> add x0, x21, x22
> add x0, x0, #15
> bic x0, x0, #15 /* round_up(sigframe_size + misglian_bytes, 16) */
> sub sp, sp, x0
> add x23, sp, x22 /* new sigframe base with misaligment */
>
> (You can drop the mov into x23 above in your function prologue if you
> code it this way.)
>
ok...but...shouldn't be
add x0, x0, #16
before
bic x-, x0, #15
instead of adding #15 ?
>> +
>> + ldr x0, =call_fmt
>> + mov x1, x21
>> + mov x2, x23
>> + bl printf
>> +
>> + mov sp, x23
>
> AAPCS64 requires sp to be 16-byte aligned at function boundaries, so
> we may get stack alignments faults in mempcy() here. Possibly these
> can be confused with test failure SEGVs (I can't remember offhand how
> stack alignment faults are supported).
>
Didn't know about function boundaries requirements.
> Coding something like what I have above to guarantee stack alignment
> should avoid this.
>
Nice I'll do.
>> + /* now fill it with the provided content... */
>> + mov x0, sp
>
> With my version this would be mov x0, x23
>
ok
>> + mov x1, x20
>> + mov x2, x21
>> + bl memcpy
>> +
>> + /*
>> + * Here saving a last minute SP to current->token acts as a marker:
>> + * if we got here, we are successfully faking a sigreturn; in other
>> + * words we are sure no bad fatal signal has been raised till now
>> + * for unrelated reasons, so we should consider the possibly observed
>> + * fatal signal like SEGV coming from Kernel restore_sigframe() and
>> + * triggered as expected from our test-case.
>> + * For simplicity this assumes that current field 'token' is laid out
>> + * as first in struct tdescr
>> + */
>> + ldr x0, current
>
> Nit: it probably doesn't matter since this will be a small binary
> after linking, but to avoid possible fixup errors during linking you
> could also do:
>
> adrp x0, current
> ldr x0, [x0, #:lo12:current]
>
> This raises the addressing range from 0.5 MB or so to a few GB, making
> link errors much more unlikely.
>
Ok...I'll fix this nitpick once I'll have understood it :D
>> + str x23, [x0]
>> + /* SP is already pointing back to the just built fake sigframe here */
>> + mov x8, #__NR_rt_sigreturn
>
> And finally we would mov sp, x23 here.
>
Yes
>> + svc #0
>> +
>> + /*
>> + * Above sigreturn should not return...looping here leads to a timeout
>> + * and ensure proper and clean test failure, instead of jumping around
>> + * on a potentially corrupted stack.
>> + */
>> + b .
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> index ce35be8ebc8e..68930f1e46e5 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> @@ -12,4 +12,5 @@ int test_run(struct tdescr *td);
>> void test_result(struct tdescr *td);
>>
>> bool get_current_context(struct tdescr *td, ucontext_t *dest_uc);
>> +int fake_sigreturn(void *sigframe, size_t sz, int misalign_bytes);
>> #endif
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>> new file mode 100644
>> index 000000000000..7fb700b9801b
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>> @@ -0,0 +1,54 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Place a fake sigframe on the stack including a BAD Unknown magic
>> + * record: on sigreturn Kernel must spot this attempt and the test
>> + * case is expected to be terminated via SEGV.
>> + */
>> +
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct fake_sigframe sf;
>> +
>> +static int fake_sigreturn_bad_magic_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + size_t resv_sz, need_sz;
>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
>> +
>> + /* just to fill the ucontext_t with something real */
>> + if (!get_current_context(td, &sf.uc))
>> + return 1;
>> +
>> + resv_sz = GET_SF_RESV_SIZE(sf);
>> + /* need at least 2*HDR_SZ space: KSFT_BAD_MAGIC + terminator. */
>> + need_sz = HDR_SZ * 2;
>> + head = get_starting_head(shead, need_sz, resv_sz, NULL);
>
> Nit: are the need_sz and resv_sz variables required?
>
> Maybe they help to highlight what these expressions mean in the
> get_starting_head() call though. I'm happy either way.
Not really required...I'll remove. (probably in other tests were and they
landed here too...)
>
>> + if (head) {
>> + /*
>> + * use a well known NON existent bad magic...something
>> + * we should pretty sure won't be ever defined in Kernel
>> + */
>> + head->magic = KSFT_BAD_MAGIC;
>> + head->size = HDR_SZ;
>> + write_terminator_record(GET_RESV_NEXT_HEAD(head));
>> +
>> + ASSERT_BAD_CONTEXT(&sf.uc);
>> + fake_sigreturn(&sf, sizeof(sf), 0);
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +struct tdescr tde = {
>> + .name = "FAKE_SIGRETURN_BAD_MAGIC",
>> + .descr = "Trigger a sigreturn with a sigframe with a bad magic",
>> + .sig_ok = SIGSEGV,
>> + .timeout = 3,
>> + .run = fake_sigreturn_bad_magic_run,
>> +};
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c
>> index 72e3f482b177..2effb8ded935 100644
>> --- a/tools/testing/selftests/arm64/signal/testcases/testcases.c
>> +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c
>> @@ -149,3 +149,31 @@ bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err)
>>
>> return true;
>> }
>> +
>
> Maybe add a comment saying what this function does.
>
> To check my understanding:
> The purpose is to find a place to append a new record, right?
> By default we append at the end (i.e., at the terminator), but
> because extra_context is optional we replace that instead if
> there isn't sufficient space after the terminator in __reserved[].
>
Yes I'll add a comment.
>> +struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
>> + size_t need_sz, size_t resv_sz,
>> + size_t *offset)
>> +{
>> + size_t offs = 0;
>> + struct _aarch64_ctx *head;
>> +
>> + head = get_terminator(shead, resv_sz, &offs);
>> + /* not found a terminator...no need to update offset if any */
>> + if (!head)
>> + return head;
>> + if (resv_sz - offs < need_sz) {
>> + fprintf(stderr, "Low on space:%zd. Discarding extra_context.\n",
>> + resv_sz - offs);
>> + head = get_header(shead, EXTRA_MAGIC, resv_sz, &offs);
>> + if (!head || resv_sz - offs < need_sz) {
>> + fprintf(stderr,
>> + "Failed to reclaim space on sigframe.\n");
>> + return NULL;
>> + }
>> + }
>> +
>> + fprintf(stderr, "Available space:%zd\n", resv_sz - offs);
>> + if (offset)
>> + *offset = offs;
>> + return head;
>> +}
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.h b/tools/testing/selftests/arm64/signal/testcases/testcases.h
>> index 00618c3202bb..7653f8a64b3d 100644
>> --- a/tools/testing/selftests/arm64/signal/testcases/testcases.h
>> +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.h
>> @@ -83,4 +83,8 @@ static inline void write_terminator_record(struct _aarch64_ctx *tail)
>> tail->size = 0;
>> }
>> }
>> +
>> +struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
>> + size_t need_sz, size_t resv_sz,
>> + size_t *offset);
>> #endif
>
> Apart from the comments above, this looks reasonable.
>
> Cheers
> ---Dave
>
Cheers
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] KVM: arm/arm64: Allow user injection of external data aborts
From: Christoffer Dall @ 2019-09-09 17:36 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrangé, Suzuki K Poulose, Marc Zyngier,
James Morse, Julien Thierry, Stefan Hajnoczi, Heinrich Schuchardt,
Alexander Graf, kvmarm, arm-mail-list
In-Reply-To: <CAFEAcA-F3YLEQgKAvfbyGGYdzs_wYPz+QMuxk4qZd_oeU-_PBA@mail.gmail.com>
On Mon, Sep 09, 2019 at 04:56:23PM +0100, Peter Maydell wrote:
> On Mon, 9 Sep 2019 at 16:16, Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > On Mon, Sep 09, 2019 at 01:32:46PM +0100, Peter Maydell wrote:
> > > This API seems to be missing support for userspace to specify
> > > whether the ESR_ELx for the guest should have the EA bit set
> > > (and more generally other syndrome/fault status bits). I think
> > > if we have an API for "KVM_EXIT_MMIO but the access failed"
> > > then it should either (a) be architecture agnostic, since
> > > pretty much any architecture might have a concept of "access
> > > gave some bus-error-type failure" and it would be nice if userspace
> > > didn't have to special case them all in arch-specific code,
> > > or (b) have the same flexibility for specifying exactly what
> > > kind of fault as the architecture does. This sort of seems to
> > > fall between two stools. (My ideal for KVM_EXIT_MMIO faults
> > > would be a generic API which included space for optional
> > > arch-specific info, which for Arm would pretty much just be
> > > the EA bit.)
> >
> > I'm not sure I understand exactly what would be improved by making this
> > either more architecture speific or more architecture generic. The
> > EA bit will always be set, that's why the field is called
> > 'ext_dabt_pending'.
>
> ESR_EL1.EA doesn't mean "this is an external abort". It means
> "given that this is an external abort as indicated by ESR_EL1.DFSC,
> specify the external abort type". Traditionally this is 0 for
> an AXI bus Decode error ("interconnect says there's nothing there")
> and 1 for a Slave error ("there's something there but it told us
> to go away"), though architecturally it's specified as impdef
> because not everybody uses AXI. In QEMU we track the difference
> between these two things and for TCG will raise external aborts
> with the correct EA bit value.
>
Ah, I missed that. I don't think we want to allow userspace to supply
any implementation defined values for the VM, though.
> > I thought as per the previous discussion, that we were specifically
> > trying to avoid userspace emulating the exception in detail, so I
> > designed this to provide the minimal effort API for userspace.
> >
> > Since we already have an architecture specific ioctl, kvm_vcpu_events, I
> > don't think we're painting ourselves into a corner by using that. Is a
> > natural consequence of what you're saying not that we should try to make
> > that whole call architecture generic?
> >
> > Unless we already have specific examples of how other architectures
> > would want to use something like this, and given the impact of this
> > patch, I'm not sure it's worth trying to speculate about that.
>
> In QEMU, use of a generic API would look something like
> this in kvm-all.c:
>
> case KVM_EXIT_MMIO:
> DPRINTF("handle_mmio\n");
> /* Called outside BQL */
> MemTxResult res;
>
> res = address_space_rw(&address_space_memory,
> run->mmio.phys_addr, attrs,
> run->mmio.data,
> run->mmio.len,
> run->mmio.is_write);
> if (res != MEMTX_OK) {
> /* tell the kernel the access failed, eg
> * by updating the kvm_run struct to say so
> */
> } else {
> /* access passed, we have updated the kvm_run
> * struct's mmio subfield, proceed as usual
> */
> }
> ret = 0;
> break;
>
> [this is exactly the current QEMU code except that today
> we throw away the 'res' that tells us if the transaction
> succeeded because we have no way to report it to KVM and
> effectively always RAZ/WI the access.]
>
> This is nice because you don't need anything here that has to do
> "bail out to architecture specific handling of anything",
> you just say "nope, the access failed", and let the kernel handle
> that however the CPU would handle it. It just immediately works
> for all architectures on the userspace side (assuming the kernel
> defaults to not actually trying to report an abort to the guest
> if nobody's implemented that on the kernel side, which is exactly
> what happens today where there's no way to report the error for
> any architecture).
> The downside is that you lose the ability to be more specific about
> architecture-specific fine distinctions like decode errors vs slave
> errors, though.
I understand that it's convenient to avoid having to write an
architecture hook, but I simply don't know if it makes sense to do this
on other architectures, and while it can be more code to have to write
the architecture hooks in QEMU, it's hardly a strong argument against
using an existing architecture-specific mechanism to inject an event to
a guest.
Note that I looked at using a an appropriate field in the kvm_run
structure, but nothing elegant came to mind.
Do you have a concrete example of how you would like to change the
kvm_run structure?
>
> Or you could have an arm-specific API that does care about
> fine details like the EA bit (and maybe also other ESR_ELx
> fields); that has the downside that userspace needs to
> make the handling of error returns from "handle this MMIO
> access" architecture specific, but you get architecture-specific
> benefits as a result. (Preferably the architecture-specific
> APIs should at least be basically the same, eg same ioctl
> or same bit of the kvm_run struct being updated with some parts
> being arch-specific data, rather than 3 different mechanisms.)
Are there other bits of the ESR than the EA that you think we should be
able to specify?
Can we decide if we need to allow userspace to provide additional
information or not, and then decide on the mechanism, instead of
conflating the two questions?
I think we should either expose the minimal mechanism to user space, or
just leave it to user space to emulate the whole thing.
Thanks,
Christoffer
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 07/11] kselftest: arm64: fake_sigreturn_bad_size_for_magic0
From: Cristian Marussi @ 2019-09-09 17:47 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114901.GX27757@arm.com>
On 04/09/2019 12:49, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:28pm +0100, Cristian Marussi wrote:
>> Add a simple fake_sigreturn testcase which builds a ucontext_t with a
>> badly sized terminator record and place it onto the stack.
>> Expects a SIGSEGV on test PASS.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit
>> - add signal.h include
>> - using new get_starting_head() helper
>> - added test description
>> ---
>> .../fake_sigreturn_bad_size_for_magic0.c | 49 +++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c
>> new file mode 100644
>> index 000000000000..25017fe18214
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c
>> @@ -0,0 +1,49 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Place a fake sigframe on the stack including a badly sized terminator
>> + * record: on sigreturn Kernel must spot this attempt and the test case
>> + * is expected to be terminated via SEGV.
>> + */
>> +
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct fake_sigframe sf;
>> +
>> +static int fake_sigreturn_bad_size_for_magic0_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + size_t resv_sz, need_sz;
>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
>> +
>> + /* just to fill the ucontext_t with something real */
>> + if (!get_current_context(td, &sf.uc))
>> + return 1;
>> +
>> + resv_sz = GET_SF_RESV_SIZE(sf);
>> + /* at least HDR_SZ for the badly sized terminator. */
>> + need_sz = HDR_SZ;
>
> Nit: do we need the resv_sz and need_sz variables here?
>
No
>> + head = get_starting_head(shead, need_sz, resv_sz, NULL);
>> + if (head) {
>
> Perhaps we could fail immediately rather than relying on timeout here?
>
> Probably not a huge deal though.
Yes I'll do. (I'll review slightly the exit/termination strategy in main()
but it won't be a problem)
>
>> + head->magic = 0;
>> + head->size = HDR_SZ;
>> +
>> + ASSERT_BAD_CONTEXT(&sf.uc);
>> + fake_sigreturn(&sf, sizeof(sf), 0);
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +struct tdescr tde = {
>> + .name = "FAKE_SIGRETURN_BAD_SIZE_FOR_TERMINATOR",
>> + .descr = "Trigger a sigreturn using non-zero size terminator",
>> + .sig_ok = SIGSEGV,
>> + .timeout = 3,
>> + .run = fake_sigreturn_bad_size_for_magic0_run,
>> +};
>
> Either way,
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>
Thanks
Cheers
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 6/6] arm64: dts: khadas-vim3: add commented support for PCIe
From: Neil Armstrong @ 2019-09-09 17:50 UTC (permalink / raw)
To: Marc Zyngier
Cc: lorenzo.pieralisi, khilman, linux-kernel, kishon, repk, linux-pci,
bhelgaas, linux-amlogic, yue.wang, linux-arm-kernel
In-Reply-To: <864l1ls9wy.wl-maz@kernel.org>
Hi Marc,
Le 09/09/2019 à 18:37, Marc Zyngier a écrit :
> On Sun, 08 Sep 2019 14:42:58 +0100,
> Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> The VIM3 on-board MCU can mux the PCIe/USB3.0 shared differential
>> lines using a FUSB340TMX USB 3.1 SuperSpeed Data Switch between
>> an USB3.0 Type A connector and a M.2 Key M slot.
>> The PHY driving these differential lines is shared between
>> the USB3.0 controller and the PCIe Controller, thus only
>> a single controller can use it.
>>
>> The needed DT configuration when the MCU is configured to mux
>> the PCIe/USB3.0 differential lines to the M.2 Key M slot is
>> added commented and may uncommented to disable USB3.0 from the
>> USB Complex and enable the PCIe controller.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>> .../amlogic/meson-g12b-a311d-khadas-vim3.dts | 22 +++++++++++++++++++
>> .../amlogic/meson-g12b-s922x-khadas-vim3.dts | 22 +++++++++++++++++++
>> .../boot/dts/amlogic/meson-khadas-vim3.dtsi | 4 ++++
>> .../dts/amlogic/meson-sm1-khadas-vim3l.dts | 22 +++++++++++++++++++
>> 4 files changed, 70 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts b/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts
>> index 3a6a1e0c1e32..0577b1435cbb 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts
>> +++ b/arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dts
>> @@ -14,3 +14,25 @@
>> / {
>> compatible = "khadas,vim3", "amlogic,a311d", "amlogic,g12b";
>> };
>> +
>> +/*
>> + * The VIM3 on-board MCU can mux the PCIe/USB3.0 shared differential
>> + * lines using a FUSB340TMX USB 3.1 SuperSpeed Data Switch between
>> + * an USB3.0 Type A connector and a M.2 Key M slot.
>> + * The PHY driving these differential lines is shared between
>> + * the USB3.0 controller and the PCIe Controller, thus only
>> + * a single controller can use it.
>> + * If the MCU is configured to mux the PCIe/USB3.0 differential lines
>> + * to the M.2 Key M slot, uncomment the following block to disable
>> + * USB3.0 from the USB Complex and enable the PCIe controller.
>> + */
>> +/*
>> +&pcie {
>> + status = "okay";
>> +};
>> +
>> +&usb {
>> + phys = <&usb2_phy0>, <&usb2_phy1>;
>> + phy-names = "usb2-phy0", "usb2-phy1";
>> +};
>> + */
>
> Although you can't do much more than this here, I'd expect firmware on
> the machine to provide the DT that matches its configuration. Is it
> the way it actually works? Or is the user actually expected to edit
> this file?
It's the plan when initial VIM3 support will be merged in u-boot mainline,
and the MCU driver will be added aswell :
https://patchwork.ozlabs.org/cover/1156618/
A custom board support altering the DT will be added when this patchset
is merged upstream.
But since these are separate projects, leaving this as commented is ugly,
but necessary.
Thanks,
Neil
>
> Thanks,
>
> M.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 08/11] kselftest: arm64: fake_sigreturn_missing_fpsimd
From: Cristian Marussi @ 2019-09-09 17:51 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114910.GY27757@arm.com>
On 04/09/2019 12:49, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:29pm +0100, Cristian Marussi wrote:
>> Add a simple fake_sigreturn testcase which builds a ucontext_t without
>> the required fpsimd_context and place it onto the stack.
>> Expects a SIGSEGV on test PASS.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit
>> - added signal.h
>> - added test description
>> ---
>> .../testcases/fake_sigreturn_missing_fpsimd.c | 50 +++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c
>> new file mode 100644
>> index 000000000000..08ecd8073a1a
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c
>> @@ -0,0 +1,50 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Place a fake sigframe on the stack missing the mandatory FPSIMD
>> + * record: on sigreturn Kernel must spot this attempt and the test
>> + * case is expected to be terminated via SEGV.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct fake_sigframe sf;
>> +
>> +static int fake_sigreturn_missing_fpsimd_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + size_t resv_sz, offset;
>> + struct _aarch64_ctx *head = GET_SF_RESV_HEAD(sf);
>> +
>> + /* just to fill the ucontext_t with something real */
>> + if (!get_current_context(td, &sf.uc))
>> + return 1;
>> +
>> + resv_sz = GET_SF_RESV_SIZE(sf);
>> + head = get_header(head, FPSIMD_MAGIC, resv_sz, &offset);
>> + if (head && resv_sz - offset >= HDR_SZ) {
>> + fprintf(stderr, "Mangling template header. Spare space:%zd\n",
>> + resv_sz - offset);
>> + /* Just overwrite fpsmid_context */
>> + write_terminator_record(head);
>
> Strictly speaking, we may be throwing away more than just the
> fpsimd_context record here.
>
> But I think the test works nonetheless. fpsimd_context is the only
> record that's mandatory in any case.
Ok
>
>> +
>> + ASSERT_BAD_CONTEXT(&sf.uc);
>> + fake_sigreturn(&sf, sizeof(sf), 0);
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +struct tdescr tde = {
>> + .name = "FAKE_SIGRETURN_MISSING_FPSIMD",
>> + .descr = "Triggers a sigreturn with a missing fpsimd_context",
>> + .sig_ok = SIGSEGV,
>> + .timeout = 3,
>> + .run = fake_sigreturn_missing_fpsimd_run,
>> +};
>
> Assuming the comment I just posted on v3 of this patch makes sense to you,
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>
Thanks
Cheers
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* kexec arm support
From: Aggelis Aggelis @ 2019-09-09 17:58 UTC (permalink / raw)
To: linux-arm-kernel
I use MitySOM-5CSX dev kit (cyclone v)and i would like to boot to a
different kernel using kexec.
In my configuration:
KERNEL : https://github.com/altera-opensource/linux-socfpga/archive/socfpga-4.9.76-ltsi-rt.zip
COMPILER : https://releases.linaro.org/archive/14.04/components/toolchain/binaries/gcc-linaro-arm-linux-gnueabihf-4.8-2014.04_linux.tar.bz2
In the kernel configuration kexec is enabled
zcat /proc/config.gz |grep KEXEC
CONFIG_KEXEC_CORE=y
CONFIG_KEXEC=y
and the default kernel cmdline is
# cat /proc/cmdline
root=/dev/mmcblk0p3 rootwait rw earlycon
First we load kernel with
#./kexec --version
kexec-tools 2.0.19
kexec -d -l zImagebkx --dtb=socfpga_cyclone5_mitysom5csx_devkit.dtb
--command-line="root=/dev/mmcblk0p3 rootwait rw earlycon"
syscall kexec_file_load not available.
kernel: 0xb6a9d008 kernel_size: 0x4a55c8
MEMORY RANGES
0000000000000000-000000003fffffff (0)
zImage header: 0x016f2818 0x00000000 0x004a55c8
zImage size 0x4a55c8, file size 0x4a55c8
zImage requires 0x004b65c8 bytes
Kernel: address=0x00008000 size=0x0178fce8
DT : address=0x01799000 size=0x00007cf4
kexec_load: entry = 0x8000 flags = 0x280000
nr_segments = 2
segment[0].buf = 0xb6a9d008
segment[0].bufsz = 0x4a55cc
segment[0].mem = 0x8000
segment[0].memsz = 0x4a6000
segment[1].buf = 0x8e610
segment[1].bufsz = 0x7cf4
segment[1].mem = 0x1799000
segment[1].memsz = 0x8000
and kexec kernel with
kexec -e
[ 134.110855] kexec_core: Starting new kernel
[ 134.115064] Disabling non-boot CPUs ...
[ 134.176961] CPU1: shutdown
[ 134.180624] Bye!
Uncompressing Linux... done, booting the kernel.
and then nothing no messages on console. The same kernel boots
successfully with U-Boot
using kernel 4.1.22 from https://github.com/dlaut/linux-socfpga and
applying the patch described in
https://patchwork.kernel.org/patch/6504321/
i successfully kexeced 4.9.76 kernel from 4.1.22 using the same
kexec-tools 2.0.19.
root@node1:/mnt/test#uname -a Linux node1 4.1.22-ltsi-altera #2 SMP
PREEMPT Mon Jul 29 12:38:06 EEST 2019 armv7l GNU/L
root@node1:/mnt/test# kexec -d -e
[ 46.306102] kexec: Starting new kernel
[ 46.309928] Disabling non-boot CPUs ...
[ 46.306102] kexec: Starting new kernel
[ 46.378053] CPU1: shutdown
[ 46.381875] Bye!
Uncompressing Linux... done, booting the kernel.
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.9.76-rt61-ltsi-altera (aggelis@corei5)
(gcc version 4.8.3 20140401 (prerelease) (crosstool-NG l9
[ 0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7), cr=10c5387d
[ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[ 0.000000] OF: fdt:Machine model: MitySOM-5CSX Altera SOCFPGA Cyclone V
[ 0.000000] cma: Reserved 16 MiB at 0x3f000000
[ 0.000000] Memory policy: Data cache writealloc
[ 0.000000] percpu: Embedded 15 pages/cpu @ef6bf000 s29184 r8192
d24064 u61440
[ 0.000000] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 260416
[ 0.000000] Kernel command line: root=/dev/mmcblk0p3 rootwait
[ 0.000000] PID hash table entries: 4096 (order: 2, 16384 bytes)
[ 0.000000] Dentry cache hash table entries: 131072 (order: 8, 1048576 bytes)
[ 0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
[ 0.000000] allocated 1048576 bytes of page_ext
...
...
Angstrom v2017.12 - Kernel 4.9.76-rt61-ltsi-altera
node1 login:
The kexec patch applied on the 4.1.22 kernel (enabling kexec on
socfpga) is already present in 4.9.76 kernel.
Did something break in kexec implementation in later 4 series kernels?
Aggelis Aggelis
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 3/8] mm: Move set/get_pcppage_migratetype to mmzone.h
From: Alexander Duyck @ 2019-09-09 18:01 UTC (permalink / raw)
To: Kirill A. Shutemov, Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas, mhocko,
linux-mm, will, aarcange, virtio-dev, mst, willy, wei.w.wang,
ying.huang, riel, dan.j.williams, lcapitulino, linux-arm-kernel,
osalvador, nitesh, konrad.wilk, dave.hansen, linux-kernel,
pbonzini, akpm, fengguang.wu, kirill.shutemov
In-Reply-To: <20190909095608.jwachx3womhqmjbl@box>
On Mon, 2019-09-09 at 12:56 +0300, Kirill A. Shutemov wrote:
> On Sat, Sep 07, 2019 at 10:25:28AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > In order to support page reporting it will be necessary to store and
> > retrieve the migratetype of a page. To enable that I am moving the set and
> > get operations for pcppage_migratetype into the mm/internal.h header so
> > that they can be used outside of the page_alloc.c file.
> >
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
>
> I'm not sure that it's great idea to export this functionality beyond
> mm/page_alloc.c without any additional safeguards. How would we avoid to
> messing with ->index when the page is not in the right state of its
> life-cycle. Can we add some VM_BUG_ON()s here?
I am not sure what we would need to check on though. There are essentially
2 cases where we are using this. The first is the percpu page lists so the
value is set either as a result of __rmqueue_smallest or
free_unref_page_prepare. The second one which hasn't been added yet is for
the Reported pages case which I add with patch 6.
When I use it for page reporting I am essentially using the Reported flag
to identify what pages in the buddy list will have this value set versus
those that may not. I didn't explicitly define it that way, but that is
how I am using it so that the value cannot be trusted unless the Reported
flag is set.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd
From: Cristian Marussi @ 2019-09-09 18:03 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114923.GZ27757@arm.com>
On 04/09/2019 12:49, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:30pm +0100, Cristian Marussi wrote:
>> Add a simple fake_sigreturn testcase which builds a ucontext_t with
>> an anomalous additional fpsimd_context and place it onto the stack.
>> Expects a SIGSEGV on test PASS.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit
>> - missing include
>> - using new get_starting_head() helper
>> - added test description
>> ---
>> .../fake_sigreturn_duplicated_fpsimd.c | 52 +++++++++++++++++++
>> 1 file changed, 52 insertions(+)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>> new file mode 100644
>> index 000000000000..c7122c44f53f
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>> @@ -0,0 +1,52 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Place a fake sigframe on the stack including an additional FPSIMD
>> + * record: on sigreturn Kernel must spot this attempt and the test
>> + * case is expected to be terminated via SEGV.
>> + */
>> +
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct fake_sigframe sf;
>> +
>> +static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + size_t resv_sz, need_sz;
>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
>> +
>> + /* just to fill the ucontext_t with something real */
>> + if (!get_current_context(td, &sf.uc))
>> + return 1;
>> +
>> + resv_sz = GET_SF_RESV_SIZE(sf);
>> + need_sz = HDR_SZ + sizeof(struct fpsimd_context);
>
> Nit: Maybe write this sum in the same order as the records we're going
> o append, i.e.:
>
> need_sz = sizeof(struct fpsimd_context) + HDR_SZ; /* for terminator */
>
Ok
> Also, maybe fail this test if there is no fpsimd_context in the initial
> frame from get_current_context(): that would be a kernel bug, but we
> wouldn't be giving fake_sigreturn() duplicate fpsimd_contexts in that
> case -- so this test wouldn't test the thing it's supposed to test.
>
I've reviewed ASSERT_BAD/GOOD_CONTEXT() macros in this series to use abort()
instead of assert(): this means any frame get with get_current_context() will
be checked to be GOOD before the test starts.
>> +
>> + head = get_starting_head(shead, need_sz, resv_sz, NULL);
>> + if (head) {
>> + /* Add a spurios fpsimd_context */
>> + head->magic = FPSIMD_MAGIC;
>> + head->size = sizeof(struct fpsimd_context);
>> + /* and terminate */
>> + write_terminator_record(GET_RESV_NEXT_HEAD(head));
>> +
>> + ASSERT_BAD_CONTEXT(&sf.uc);
>> + fake_sigreturn(&sf, sizeof(sf), 0);
>> + }
>> +
>> + return 1;
>
Here I'll avoid the timeout on !head and exit straight away (like in other fake_ tests)
> [...]
>
> Cheers
> ---Dave
>
Cheers
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: kexec arm support
From: Russell King - ARM Linux admin @ 2019-09-09 18:10 UTC (permalink / raw)
To: Aggelis Aggelis; +Cc: linux-arm-kernel
In-Reply-To: <CAKUkA52n+g4=G8r47P4Jt1LpUaPStEkLUNmU6szSCW9sYhW4Mw@mail.gmail.com>
On Mon, Sep 09, 2019 at 08:58:58PM +0300, Aggelis Aggelis wrote:
> I use MitySOM-5CSX dev kit (cyclone v)and i would like to boot to a
> different kernel using kexec.
>
> In my configuration:
>
> KERNEL : https://github.com/altera-opensource/linux-socfpga/archive/socfpga-4.9.76-ltsi-rt.zip
> COMPILER : https://releases.linaro.org/archive/14.04/components/toolchain/binaries/gcc-linaro-arm-linux-gnueabihf-4.8-2014.04_linux.tar.bz2
>
> In the kernel configuration kexec is enabled
>
> zcat /proc/config.gz |grep KEXEC
> CONFIG_KEXEC_CORE=y
> CONFIG_KEXEC=y
>
> and the default kernel cmdline is
>
> # cat /proc/cmdline
> root=/dev/mmcblk0p3 rootwait rw earlycon
>
> First we load kernel with
>
> #./kexec --version
> kexec-tools 2.0.19
>
> kexec -d -l zImagebkx --dtb=socfpga_cyclone5_mitysom5csx_devkit.dtb
> --command-line="root=/dev/mmcblk0p3 rootwait rw earlycon"
> syscall kexec_file_load not available.
> kernel: 0xb6a9d008 kernel_size: 0x4a55c8
> MEMORY RANGES
> 0000000000000000-000000003fffffff (0)
> zImage header: 0x016f2818 0x00000000 0x004a55c8
> zImage size 0x4a55c8, file size 0x4a55c8
> zImage requires 0x004b65c8 bytes
> Kernel: address=0x00008000 size=0x0178fce8
> DT : address=0x01799000 size=0x00007cf4
It looks like your kernel predates the addition of additional
information that allows kexec to adequately lay out the physical
address space, which was added around the 4.15 timeframe.
See commits c772568788b5 ("ARM: add additional table to compressed
kernel") and the preceeding commit.
These improvements were added to kexec-tools in the 2.0.16 timeframe,
and the combination of both allows kexec-tools to more accurately
place the DT image.
It is highly likely with the above placement that the kernel is
overwriting the DT image during decompression, resulting in the
kernel attempting to boot knowing nothing about the platform.
> kexec_load: entry = 0x8000 flags = 0x280000
> nr_segments = 2
> segment[0].buf = 0xb6a9d008
> segment[0].bufsz = 0x4a55cc
> segment[0].mem = 0x8000
> segment[0].memsz = 0x4a6000
> segment[1].buf = 0x8e610
> segment[1].bufsz = 0x7cf4
> segment[1].mem = 0x1799000
> segment[1].memsz = 0x8000
>
>
> and kexec kernel with
>
>
> kexec -e
> [ 134.110855] kexec_core: Starting new kernel
> [ 134.115064] Disabling non-boot CPUs ...
> [ 134.176961] CPU1: shutdown
> [ 134.180624] Bye!
> Uncompressing Linux... done, booting the kernel.
>
> and then nothing no messages on console. The same kernel boots
> successfully with U-Boot
>
> using kernel 4.1.22 from https://github.com/dlaut/linux-socfpga and
> applying the patch described in
> https://patchwork.kernel.org/patch/6504321/
> i successfully kexeced 4.9.76 kernel from 4.1.22 using the same
> kexec-tools 2.0.19.
>
> root@node1:/mnt/test#uname -a Linux node1 4.1.22-ltsi-altera #2 SMP
> PREEMPT Mon Jul 29 12:38:06 EEST 2019 armv7l GNU/L
>
>
> root@node1:/mnt/test# kexec -d -e
> [ 46.306102] kexec: Starting new kernel
> [ 46.309928] Disabling non-boot CPUs ...
> [ 46.306102] kexec: Starting new kernel
> [ 46.378053] CPU1: shutdown
> [ 46.381875] Bye!
> Uncompressing Linux... done, booting the kernel.
> [ 0.000000] Booting Linux on physical CPU 0x0
> [ 0.000000] Linux version 4.9.76-rt61-ltsi-altera (aggelis@corei5)
> (gcc version 4.8.3 20140401 (prerelease) (crosstool-NG l9
> [ 0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7), cr=10c5387d
> [ 0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
> instruction cache
> [ 0.000000] OF: fdt:Machine model: MitySOM-5CSX Altera SOCFPGA Cyclone V
> [ 0.000000] cma: Reserved 16 MiB at 0x3f000000
> [ 0.000000] Memory policy: Data cache writealloc
> [ 0.000000] percpu: Embedded 15 pages/cpu @ef6bf000 s29184 r8192
> d24064 u61440
> [ 0.000000] Built 1 zonelists in Zone order, mobility grouping on.
> Total pages: 260416
> [ 0.000000] Kernel command line: root=/dev/mmcblk0p3 rootwait
> [ 0.000000] PID hash table entries: 4096 (order: 2, 16384 bytes)
> [ 0.000000] Dentry cache hash table entries: 131072 (order: 8, 1048576 bytes)
> [ 0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
> [ 0.000000] allocated 1048576 bytes of page_ext
> ...
> ...
> Angstrom v2017.12 - Kernel 4.9.76-rt61-ltsi-altera
> node1 login:
>
>
>
> The kexec patch applied on the 4.1.22 kernel (enabling kexec on
> socfpga) is already present in 4.9.76 kernel.
>
> Did something break in kexec implementation in later 4 series kernels?
>
> Aggelis Aggelis
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 10/11] kselftest: arm64: fake_sigreturn_bad_size
From: Cristian Marussi @ 2019-09-09 18:11 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114933.GA27757@arm.com>
On 04/09/2019 12:49, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:31pm +0100, Cristian Marussi wrote:
>> Add a simple fake_sigreturn testcase which builds a ucontext_t with a
>> badly sized header that causes a overrun in the __reserved area and
>> place it onto the stack. Expects a SIGSEGV on test PASS.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit
>> - missing include
>> - using new get_starting_head() helper
>> - added test description
>> ---
>> .../testcases/fake_sigreturn_bad_size.c | 77 +++++++++++++++++++
>> 1 file changed, 77 insertions(+)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c
>> new file mode 100644
>> index 000000000000..b1156afdb691
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c
>> @@ -0,0 +1,77 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Place a fake sigframe on the stack including a bad record overflowing
>> + * the __reserved space: on sigreturn Kernel must spot this attempt and
>> + * the test case is expected to be terminated via SEGV.
>> + */
>> +
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct fake_sigframe sf;
>> +
>> +#define MIN_SZ_ALIGN 16
>> +
>> +static int fake_sigreturn_bad_size_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + size_t resv_sz, need_sz, offset;
>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
>> +
>> + /* just to fill the ucontext_t with something real */
>> + if (!get_current_context(td, &sf.uc))
>> + return 1;
>> +
>> + resv_sz = GET_SF_RESV_SIZE(sf);
>> + /* at least HDR_SZ + bad sized esr_context needed */
>> + need_sz = HDR_SZ + sizeof(struct esr_context);
>
> Nit: can we write this sum the other way round (see comment on patch 9)?
>
Ok
>> + head = get_starting_head(shead, need_sz, resv_sz, &offset);
I'll also fail straight away too here on !head (no timeout) like in others
>> + if (head) {
>> + /*
>> + * Use an esr_context to build a fake header with a
>> + * size greater then the free __reserved area minus HDR_SZ;
>> + * using ESR_MAGIC here since it is not checked for size nor
>> + * is limited to one instance.
>> + *
>> + * At first inject an additional normal esr_context
>> + */
>> + head->magic = ESR_MAGIC;
>> + head->size = sizeof(struct esr_context);
>> + /* and terminate properly */
>> + write_terminator_record(GET_RESV_NEXT_HEAD(head));
>> + ASSERT_GOOD_CONTEXT(&sf.uc);
>> +
>> + /*
>> + * now mess with fake esr_context size: leaving less space than
>> + * needed while keeping size value 16-aligned
>> + *
>> + * It must trigger a SEGV from Kernel on:
>> + *
>> + * resv_sz - offset < sizeof(*head)
>> + */
>> + /* at first set the maximum good 16-aligned size */
>> + head->size =
>> + (resv_sz - offset - need_sz + MIN_SZ_ALIGN) & ~0xfUL;
>> + /* plus a bit more of 16-aligned sized stuff */
>> + head->size += MIN_SZ_ALIGN;
>
> Can we also have versions of this test that try:
>
> a) a size that doesn't overflow __reserved[], but is not a multiple of 16
> b) a size that is less than 16
> c) a size that does overflow __reserved[], but by less than 16 bytes?
>
> These tests are all closely related and can probably be macro-ised
> easily. They can go on the TODO list for now anyway: let's get this
> series settled in its current form first.
>
Ok
> In any case:
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>
Thanks
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
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