* [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode
From: Thomas Garnier @ 2017-04-27 14:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427064917.phwo6yl4v4q43fql@gmail.com>
On Wed, Apr 26, 2017 at 11:49 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Thomas Garnier <thgarnie@google.com> wrote:
>
>> +
>> +/*
>> + * Called before coming back to user-mode. Returning to user-mode with an
>> + * address limit different than USER_DS can allow to overwrite kernel memory.
>> + */
>> +static inline void addr_limit_check_syscall(void)
>> +{
>> + BUG_ON(!segment_eq(get_fs(), USER_DS));
>> +}
>> +
>> +#ifndef CONFIG_ADDR_LIMIT_CHECK
>> +#define __CHECK_USERMODE_SYSCALL() \
>> + bool user_caller = segment_eq(get_fs(), USER_DS)
>> +#define __VERIFY_ADDR_LIMIT() \
>> + if (user_caller) addr_limit_check_syscall()
>> +#else
>> +#define __CHECK_USERMODE_SYSCALL()
>> +#define __VERIFY_ADDR_LIMIT()
>> +asmlinkage void addr_limit_check_failed(void) __noreturn;
>> +#endif
>
> _Please_ harmonize all the externally exposed names and symbols.
>
> There's no reason for this mismash of names:
>
> CONFIG_ADDR_LIMIT_CHECK
>
> __CHECK_USERMODE_SYSCALL
> __VERIFY_ADDR_LIMIT
>
> When we could just as easily name them consistently, along the existing pattern:
>
> CONFIG_ADDR_LIMIT_CHECK
>
> __SYSCALL_ADDR_LIMIT_CHECK
> __ADDR_LIMIT_CHECK
>
> which should fit into existing nomenclature:
>
>> #define __SYSCALL_DEFINEx(x, name, ...) \
>
> But even with that fixed, the whole construct still looks pretty weird:
>
>> { \
>> - long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + long ret; \
>> + __CHECK_USERMODE_SYSCALL(); \
>> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + __ADDR_LIMIT_CHECK(); \
>> __MAP(x,__SC_TEST,__VA_ARGS__); \
>> __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
>> return ret; \
>
> I think something like this would be more natural to read:
>
>> + ADDR_LIMIT_CHECK_PRE(); \
>> + ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
>> + ADDR_LIMIT_CHECK_POST(); \
>
> it's a clear pre/post construct. Also note the lack of double underscores.
I think this construct makes more sense because the first macro check
if the syscall was called by user-mode. I will send an update for this
on this thread.
>
> BTW., a further simplification would be:
>
> #ifndef ADDR_LIMIT_CHECK_PRE
> # define ADDR_LIMIT_CHECK_PRE ...
> #endif
>
> This way architectures could override this generic functionality simply by
> defining the helpers. Architectures that don't do that get the generic version.
I don't think architectures need to do that. The optimizations are
embedding the checks on their architecture-specific code to make it
faster and remove the size impact. The pre/post is fine for the rest.
>
> Thanks,
>
> Ingo
--
Thomas
^ permalink raw reply
* stable/linux-3.18.y build: 204 builds: 5 failed, 199 passed, 35 errors, 212 warnings (v3.18.49)
From: gregkh @ 2017-04-27 14:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAK8P3a3QsSMtc7AWjVjtM+tW8ARt1Ygw53=CSjgbG6Pvpq0QvQ@mail.gmail.com>
On Fri, Apr 21, 2017 at 04:27:14PM +0200, Arnd Bergmann wrote:
> On Thu, Apr 20, 2017 at 7:04 PM, kernelci.org bot <bot@kernelci.org> wrote:
> > efm32_defconfig (arm) ? PASS, 0 errors, 1 warning, 0 section mismatches
> >
> > Warnings:
> > arch/arm/kernel/head-nommu.S:167: Warning: Use of r13 as a source register
> > is deprecated when r15 is the destination register.
>
> 970d96f9a81b ("ARM: 8383/1: nommu: avoid deprecated source register on mov")
This doesn't apply to the 3.18-stable tree, and I don't know ARM
assembly good enough to backport it properly. Any hints?
> > imx_v6_v7_defconfig (arm) ? PASS, 0 errors, 2 warnings, 0 section mismatches
> >
> > Warnings:
> > drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c:1478:8: warning: 'skb'
> > may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> 22f44150aad7 ("brcmfmac: avoid gcc-5.1 warning")
Applied.
> > ip27_defconfig (mips) ? PASS, 0 errors, 6 warnings, 0 section mismatches
> >
> > Warnings:
> > lib/cpumask.c:194:25: warning: the address of 'cpu_all_bits' will always
> > evaluate as 'true' [-Waddress]
>
> f36963c9d3f6f4 cpumask_set_cpu_local_first => cpumask_local_spread, lament
Doesn't apply, and I couldn't figure out the backport :(
> > drivers/tty/nozomi.c:857:9: warning: 'size' may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
>
> a4f642a8a3c2 ("tty: nozomi: avoid a harmless gcc warning")
Applied to 3.18 and 4.4-stable, as it was missing in 4.4
> > drivers/net/ethernet/neterion/vxge/vxge-main.c:2149:13: warning:
> > 'adaptive_coalesce_rx_interrupts' defined but not used [-Wunused-function]
> > drivers/net/ethernet/neterion/vxge/vxge-main.c:2121:13: warning:
> > 'adaptive_coalesce_tx_interrupts' defined but not used [-Wunused-function]
>
> 57e7c8cef224 ("net: vxge: avoid unused function warnings")
Applied.
> > ip32_defconfig (mips) ? PASS, 0 errors, 2 warnings, 0 section mismatches
> >
> > Warnings:
> > drivers/misc/ioc4.c:194:16: warning: 'start' may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
>
> 769105aa740d ("misc: ioc4: simplify wave period measurement in clock_calibrate")
Applied.
> > malta_defconfig (mips) ? PASS, 0 errors, 3 warnings, 0 section mismatches
> >
> > Warnings:
> > drivers/net/wireless/hostap/hostap_hw.c:842:5: warning: 'rec' may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
>
> 48dc5fb3ba53b2 hostap: avoid uninitialized variable use in hfa384x_get_rid
Applied to 4.4 and 3.18 trees.
> > msp71xx_defconfig (mips) ? PASS, 0 errors, 3 warnings, 0 section mismatches
> >
> > Warnings:
> > arch/mips/pci/ops-pmcmsp.c:196:24: warning: 'bpci_lock' defined but not used
> > [-Wunused-variable]
>
> c4a305374bbf ("MIPS: MSP71xx: remove odd locking in PCI config space
> access code")
Applied.
> > mtx1_defconfig (mips) ? PASS, 0 errors, 4 warnings, 0 section mismatches
> >
> > Warnings:
> > drivers/net/ethernet/dec/tulip/winbond-840.c:910:2: warning: #warning
> > Processor architecture undefined [-Wcpp]
>
> de92718883dd ("net: tulip: turn compile-time warning into dev_warn()")
Applied.
> > multi_v7_defconfig+CONFIG_ARM_LPAE=y (arm) ? PASS, 0 errors, 3 warnings, 0
> > section mismatches
> >
> > Warnings:
> > drivers/mmc/host/sunxi-mmc.c:297:35: warning: cast to pointer from integer
> > of different size [-Wint-to-pointer-cast]
>
> d34712d2e3db ("mmc: sunxi: avoid invalid pointer calculation")
Applied.
> > nlm_xlp_defconfig (mips) ? FAIL, 1 error, 3 warnings, 0 section mismatches
> >
> > Errors:
> > fs/gfs2/dir.c:768:9: warning: 'leaf_no' may be used uninitialized in this
> > function [-Wmaybe-uninitialized]
> > fs/gfs2/dir.c:987:8: warning: 'leaf_no' may be used uninitialized in this
> > function [-Wmaybe-uninitialized]
>
> 67893f12e537 ("gfs2: avoid uninitialized variable warning")
>
> (also needed on v4.4.y)
Applied to both.
> > fs/btrfs/extent_io.c:2166:13: warning: cast to pointer from integer of
> > different size [-Wint-to-pointer-cast]
>
> 6e1103a6e9b1 ("btrfs: fix state->private cast on 32 bit machines")
Applied.
> > realview-smp_defconfig
> >
> > Warnings:
> > arch/arm/mm/cache-l2x0.c:168:13: warning: 'l2x0_cache_sync' defined but not
> > used [-Wunused-function]
> > arch/arm/mm/cache-l2x0.c:185:13: warning: 'l2x0_flush_all' defined but not
> > used [-Wunused-function]
> > arch/arm/mm/cache-l2x0.c:195:13: warning: 'l2x0_disable' defined but not
> > used [-Wunused-function]
>
> 20e783e39e55 ARM: 8296/1: cache-l2x0: clean up aurora cache handling
Applied.
> > rm200_defconfig (mips) ? FAIL, 0 errors, 5 warnings, 0 section mismatches
> >
> > Warnings:
> > drivers/scsi/aic94xx/aic94xx_sds.c:597:2: warning: 'offs' may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
>
> 36dd5acd196574d4 aic94xx: Skip reading user settings if flash is not found
Applied.
> > arch/mips/boot/elf2ecoff.c:270:8: warning: variable 'shstrtab' set but not
> > used [-Wunused-but-set-variable]
>
> > Warnings:
> > arch/mips/ralink/prom.c:64:2: warning: 'argc' is used uninitialized in this
> > function [-Wuninitialized]
> > arch/mips/ralink/prom.c:64:2: warning: 'argv' is used uninitialized in this
> > function [-Wuninitialized]
>
> 9c48568b3692 ("MIPS: ralink: Cosmetic change to prom_init().")
Applied.
> > tinyconfig (arm) ? PASS, 0 errors, 3 warnings, 0 section mismatches
> >
> > Warnings:
> > .config:807:warning: override: KERNEL_XZ changes choice state
> > .config:809:warning: override: SLOB changes choice state
>
> 236dec051078 ("kconfig: tinyconfig: provide whole choice blocks to
> avoid warnings")
Applied.
Thanks so much for the list, I think I'm all caught up now, hopefully
the next release will have almost no warnings.
greg k-h
^ permalink raw reply
* [PATCH v5 1/4] printk/nmi: generic solution for safe printk in NMI
From: Steven Rostedt @ 2017-04-27 14:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427133819.GW3452@pathway.suse.cz>
On Thu, 27 Apr 2017 15:38:19 +0200
Petr Mladek <pmladek@suse.com> wrote:
> > by the way,
> > does this `nmi_print_seq' bypass even fix anything for Steven?
>
> I think that this is the most important question.
>
> Steven, does the patch from
> https://lkml.kernel.org/r/20170420131154.GL3452 at pathway.suse.cz
> help you to see the debug messages, please?
You'll have to wait for a bit. The box that I was debugging takes 45
minutes to reboot. And I don't have much more time to play on it before
I have to give it back. I already found the bug I was looking for and
I'm trying not to crash it again (due to the huge bring up time).
When I get a chance, I'll see if I can insert a trigger to crash the
kernel from NMI on another box and see if this patch helps.
Thanks,
-- Steve
^ permalink raw reply
* [PATCH] mtd: spi-nor: stm32: remove broken MODULE_ALIAS
From: Cyrille Pitchen @ 2017-04-27 14:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <dc7d8b5e-0f0e-1593-4c6b-0c93a828818c@st.com>
Le 27/04/2017 ? 14:17, Ludovic BARRE a ?crit :
> thanks a lot Arnd
>
Indeed, thanks Arnd!
Since commit ("mtd: spi-nor: add driver for STM32 quad spi flash
controller") was included with the spi-nor PR for 4.12 I've sent
yesterday to Brian, I will check with him to know how he wants us to
proceed:
- If Brian has not started yet to review the PR and if he agrees on
that, I can rework a little bit the history in github/spi-nor to squash
Arnd's patch into Ludovic's one.
Ludovic's patch is already in linux-next but not yet in any release
candiate.
- or I can send an new PR keeping the fixup patch aside.
Best regards,
Cyrille
> Oops, I've not tested in Module :-(.
> Just tested in built-in, and used bind/unbind and device tree overlays.
>
> BR
> Ludo
> On 04/27/2017 01:09 PM, Arnd Bergmann wrote:
>> The MODULE_ALIAS statement refers to a macro that has never been defined
>> in this driver, causing a build error:
>>
>> drivers/mtd/spi-nor/stm32-quadspi.c:694:150: error: expected ',' or
>> ';' before 'DRIVER_NAME'
>>
>> Unless there is a specific alias we need other then the driver name,
>> we don't need an alias here and can simply remove the line to fix the
>> build.
>>
>> Fixes: dc5e400611c5 ("mtd: spi-nor: add driver for STM32 quad spi
>> flash controller")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> drivers/mtd/spi-nor/stm32-quadspi.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c
>> b/drivers/mtd/spi-nor/stm32-quadspi.c
>> index 90d9152ddf98..1056e7408d2a 100644
>> --- a/drivers/mtd/spi-nor/stm32-quadspi.c
>> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c
>> @@ -691,7 +691,6 @@ static struct platform_driver stm32_qspi_driver = {
>> };
>> module_platform_driver(stm32_qspi_driver);
>> -MODULE_ALIAS("platform:" DRIVER_NAME);
>> MODULE_AUTHOR("Ludovic Barre <ludovic.barre@st.com>");
>> MODULE_DESCRIPTION("STMicroelectronics STM32 quad spi driver");
>> MODULE_LICENSE("GPL v2");
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
^ permalink raw reply
* [PATCH] perf evsel: Fix to perf-stat malloc corruption on arm64 platforms
From: Mark Rutland @ 2017-04-27 14:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFpQJXX7HaKQ8t85mB2Fqn-6wHhb_GR+2sOi23HQZnRgUw-Y4g@mail.gmail.com>
On Wed, Apr 26, 2017 at 11:49:46PM +0530, Ganapatrao Kulkarni wrote:
> On Wed, Apr 26, 2017 at 10:42 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 13b5499..638aefa 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -346,6 +346,28 @@ static void read_counters(void)
> > }
> > }
> >
> > +/*
> > + * Close all evnt FDs we open in __run_perf_stat() and
> > + * create_perf_stat_counter(), taking care to match the number of threads and CPUs.
> > + *
> > + * Note that perf_evlist__close(evsel_list) is not equivalent, as it doesn't
> > + * take the target into account.
> > + */
> > +static void close_counters(void)
> > +{
> > + bool per_cpu = target__has_cpu(&target);
> > + struct perf_evsel *evsel;
> > +
> > + evlist__for_each_entry(evsel_list, evsel) {
> > + if (per_cpu)
> > + perf_evsel__close_per_cpu(evsel,
> > + perf_evsel__cpus(evsel));
> > + else
> > + perf_evsel__close_per_thread(evsel,
> > + evsel_list->threads);
> > + }
> > +}
> > +
> > static void process_interval(void)
> > {
> > struct timespec ts, rs;
> > @@ -686,7 +708,7 @@ static int __run_perf_stat(int argc, const char **argv)
> > * group leaders.
> > */
> > read_counters();
> > - perf_evlist__close(evsel_list);
> > + close_counters();
> >
> > return WEXITSTATUS(status);
> > }
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index ac59710..726ceca 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1670,6 +1670,18 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
> > return err;
> > }
> >
> > +int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
> > + struct cpu_map *cpus)
> > +{
> > + return perf_evsel__open(evsel, cpus, NULL);
> > +}
> > +
> > +int perf_evsel__open_per_thread(struct perf_evsel *evsel,
> > + struct thread_map *threads)
> > +{
> > + return perf_evsel__open(evsel, NULL, threads);
> > +}
> > +
> > void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads)
> > {
> > if (evsel->fd == NULL)
> > @@ -1679,16 +1691,18 @@ void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads)
> > perf_evsel__free_fd(evsel);
> > }
> >
> > -int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
> > - struct cpu_map *cpus)
> > +void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
> > + struct cpu_map *cpus)
> > {
> > - return perf_evsel__open(evsel, cpus, NULL);
> > + int ncpus = cpus ? cpus->nr : 1;
> > + perf_evsel__close(evsel, ncpus, 1);
> > }
> >
> > -int perf_evsel__open_per_thread(struct perf_evsel *evsel,
> > - struct thread_map *threads)
> > +void perf_evsel__close_per_thread(struct perf_evsel *evsel,
> > + struct thread_map *threads)
> > {
> > - return perf_evsel__open(evsel, NULL, threads);
> > + int nthreads = threads ? threads->nr : 1;
> > + perf_evsel__close(evsel, 1, nthreads);
> > }
> >
> > static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel,
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index 06ef6f2..02bea43 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -252,6 +252,10 @@ int perf_evsel__open_per_thread(struct perf_evsel *evsel,
> > struct thread_map *threads);
> > int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
> > struct thread_map *threads);
> > +void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
> > + struct cpu_map *cpus);
> > +void perf_evsel__close_per_thread(struct perf_evsel *evsel,
> > + struct thread_map *threads);
> > void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads);
> >
> > struct perf_sample;
>
> this diff looks to me doing same as mine.
Be careful when reading the diff above; the open functions have been
moved, but have not changed.
I've only changed the close path, whereas your proposal changed the open
path. Those are not equivalent changes.
> i think below diff should be more appropriate fix to this issue?
>
> when open allocates and uses dummy cpus, there is no point in holding
> old unused one. instead it should free and link to dummy cpus which
> is created with 1 CPU. same will be used by close.
>
> i did quick testing on both x86 and arm64. testing looks ok, may need
> more testing!
>
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ac59710..b1aab0a 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1466,9 +1466,13 @@ int perf_evsel__open(struct perf_evsel *evsel,
> struct cpu_map *cpus,
> empty_cpu_map = cpu_map__dummy_new();
> if (empty_cpu_map == NULL)
> return -ENOMEM;
> + } else {
> + cpu_map__get(empty_cpu_map);
> }
>
> cpus = empty_cpu_map;
> + cpu_map__put(evsel->cpus);
> + evsel->cpus = cpus;
> }
>
> if (threads == NULL) {
Unfortunately, I believe that might break the logic added in commit:
9f21b815be863218 ("perf evlist: Only open events on CPUs an evsel permits")
... since the evsel->cpus would now not represent the PMUs CPUs.
As I'd mentioned in my prior reply, I think in order to use the cpu_maps
consistently we need to do a bigger rework of the way cpu_maps are used,
in order to separate the PMU CPUs from the requested event CPUs, etc.
while taking all of these into account.
Could you please give my diff a go?
Thanks,
Mark.
^ permalink raw reply
* [PATCH 1/3] ARM: at91: pm: Add sama5d2 backup mode
From: Alexandre Belloni @ 2017-04-27 14:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMiSF3AZ9i2ywtTMYxjK8amaLEuez_0H_Jzy_pPS15CAe_FMRg@mail.gmail.com>
On 27/04/2017 at 15:34:07 +0200, Romain Izard wrote:
> Hello Alexandre,
>
> This series might also be of interest for the linux-pm mailing list.
>
I don't think they care enough to review that.
> 2017-04-26 18:04 GMT+02:00 Alexandre Belloni
> > diff --git a/arch/arm/mach-at91/Makefile b/arch/arm/mach-at91/Makefile
> > index cfd8f60a9268..87fe17dbdb56 100644
> > --- a/arch/arm/mach-at91/Makefile
> > +++ b/arch/arm/mach-at91/Makefile
> > @@ -14,6 +14,10 @@ obj-$(CONFIG_PM) += pm_suspend.o
> > ifeq ($(CONFIG_CPU_V7),y)
> > AFLAGS_pm_suspend.o := -march=armv7-a
> > endif
> > +# Backup mode will not compile for ARMv5 because of movt
> > +ifeq ($(CONFIG_SOC_SAMA5D2),y)
> > +AFLAGS_pm_suspend.o += -DBACKUP_MODE
> > +endif
> > ifeq ($(CONFIG_PM_DEBUG),y)
> > CFLAGS_pm.o += -DDEBUG
> > endif
>
> We can rewrite the assembly to avoid using movt, and remove some ifdefs
> from the code.
>
I'm kind of balanced there because I'm wondering whether we should
better separate what is in that assembly file because there are part of
it that have no chance to run on some platforms anyway.
But your solution is correct, I'll remove that.
> > +static void __init at91_pm_bu_sram_init(void)
> > +{
> > + struct gen_pool *sram_pool;
> > + struct device_node *node;
> > + struct platform_device *pdev = NULL;
> > +
> > + pm_bu = NULL;
> > +
> > + for_each_compatible_node(node, NULL, "atmel,sama5d2-securam") {
> > + pdev = of_find_device_by_node(node);
> > + if (pdev) {
> > + of_node_put(node);
> > + break;
> > + }
> > + }
> > +
>
> Do we really need to iterate over compatible nodes ?
>
You're right, this can probably be avoided.
> > + if (!pdev) {
> > + pr_warn("%s: failed to find securam device!\n", __func__);
> > + return;
> > + }
> > +
> > + sram_pool = gen_pool_get(&pdev->dev, NULL);
> > + if (!sram_pool) {
> > + pr_warn("%s: securam pool unavailable!\n", __func__);
> > + return;
> > + }
> > +
> > + pm_bu = (void *)gen_pool_alloc(sram_pool, sizeof(struct at91_pm_bu));
> > + if (!pm_bu) {
> > + pr_warn("%s: unable to alloc securam!\n", __func__);
> > + return;
> > + }
> > +
> > + pm_bu->suspended = 0;
> > + pm_bu->canary = virt_to_phys(&canary);
> > + pm_bu->resume = virt_to_phys(cpu_resume);
> > +}
> > +
>
> at91_pm_bu_sram_init and at91_dt_shdwc are necessary to use backup mode.
> But those functions do not return error codes, and do no cleanup in case
> of error. I believe that it would be simpler if we only had a single
> function.
>
Yeah, this is kind of solved by adding the fallback in a latter patch
but I agree it can be done better.
> > diff --git a/arch/arm/mach-at91/pm_suspend.S b/arch/arm/mach-at91/pm_suspend.S
> > index 96781daa671a..b5ffa8e1f203 100644
> > --- a/arch/arm/mach-at91/pm_suspend.S
> > +++ b/arch/arm/mach-at91/pm_suspend.S
> > @@ -97,15 +97,74 @@ ENTRY(at91_pm_suspend_in_sram)
> > str tmp1, .memtype
> > ldr tmp1, [r0, #PM_DATA_MODE]
> > str tmp1, .pm_mode
> > + ldr tmp1, [r0, #PM_DATA_SHDWC]
> > +#if defined(BACKUP_MODE)
> > + str tmp1, .shdwc
> > + cmp tmp1, #0
> > + ldrne tmp2, [tmp1, #0]
> > + ldr tmp1, [r0, #PM_DATA_SFRBU]
> > + str tmp1, .sfr
> > + cmp tmp1, #0
> > + ldrne tmp2, [tmp1, #0x10]
> > +#endif
>
> If I understand this well, we are doing this to fill the TLB in advance
> before the external RAM is put in self-refresh. It might be worthy of a
> comment. Moreover, .pm_mode and .memtype do not need to be protected as
> they are accessed during the at91_sramc_self_refresh, but .pmc_base
> may need to be loaded in the TLB as well.
We never had issue with .pmc_base because it is used in the C part of
the code, right before calling the assembly.
I'll add a comment.
>
> > +#if defined(BACKUP_MODE)
> > +ENTRY(at91_backup_mode)
> > + #if 0
> > + /* Read LPR */
> > + ldr r2, .sramc_base
> > + ldr r3, [r2, #AT91_DDRSDRC_LPR]
> > + #endif
> > +
>
> Do we need to keep this commented code ?
>
Nope, leftover from development
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH] [media] mtk-mdp: Fix g_/s_selection capture/compose logic
From: Stanimir Varbanov @ 2017-04-27 14:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492057130-1194-1-git-send-email-minghsiu.tsai@mediatek.com>
Hi,
On 04/13/2017 07:18 AM, Minghsiu Tsai wrote:
> From: Daniel Kurtz <djkurtz@chromium.org>
>
> Experiments show that the:
> (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
> (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
>
> ---
> drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 13afe48..8ab7ca0 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -837,12 +837,12 @@ static int mtk_mdp_m2m_g_selection(struct file *file, void *fh,
> struct mtk_mdp_ctx *ctx = fh_to_ctx(fh);
> bool valid = false;
>
> - if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> - if (mtk_mdp_is_target_compose(s->target))
> - valid = true;
> - } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> + if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> if (mtk_mdp_is_target_crop(s->target))
> valid = true;
> + } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> + if (mtk_mdp_is_target_compose(s->target))
> + valid = true;
> }
Using MPLANE formats in g/s_selection violates the v4l2 spec. See [1].
<snip>
--
regards,
Stan
[1]
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-g-selection.html
^ permalink raw reply
* [PATCH 1/3] mm: Silence vmap() allocation failures based on caller gfp_flags
From: Michal Hocko @ 2017-04-27 14:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170425223332.6999-4-f.fainelli@gmail.com>
On Tue 25-04-17 15:33:29, Florian Fainelli wrote:
> If the caller has set __GFP_NOWARN don't print the following message:
> vmap allocation for size 15736832 failed: use vmalloc=<size> to increase
> size.
>
> This can happen with the ARM/Linux module loader built with
> CONFIG_ARM_MODULE_PLTS=y which does a first attempt at loading a large
> module from module space, then falls back to vmalloc space.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> mm/vmalloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 0b057628a7ba..5a788eb58741 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -521,7 +521,7 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
> }
> }
>
> - if (printk_ratelimit())
> + if (printk_ratelimit() && !(gfp_mask & __GFP_NOWARN))
Are you sure about this ordering? Should NOWARN requests alter the
ratelimit state?
> pr_warn("vmap allocation for size %lu failed: use vmalloc=<size> to increase size\n",
> size);
> kfree(va);
> --
> 2.9.3
>
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH v8 1/4] syscalls: Verify address limit before returning to user-mode
From: Thomas Garnier @ 2017-04-27 14:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJcbSZENjCgn3TNDGzCBBSOXMvbTX8mBO7Trugdb5VRzhxwmHg@mail.gmail.com>
Ensure that a syscall does not return to user-mode with a kernel address
limit. If that happens, a process can corrupt kernel-mode memory and
elevate privileges [1].
The CONFIG_ADDR_LIMIT_CHECK option disables the generic check so each
architecture can create optimized versions.
[1] https://bugs.chromium.org/p/project-zero/issues/detail?id=990
Signed-off-by: Thomas Garnier <thgarnie@google.com>
Tested-by: Kees Cook <keescook@chromium.org>
---
Based on next-20170426
---
arch/s390/Kconfig | 1 +
include/linux/syscalls.h | 27 ++++++++++++++++++++++++++-
init/Kconfig | 6 ++++++
kernel/sys.c | 13 +++++++++++++
4 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index d25435d94b6e..164de1d24e92 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -103,6 +103,7 @@ config S390
select ARCH_INLINE_WRITE_UNLOCK_BH
select ARCH_INLINE_WRITE_UNLOCK_IRQ
select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
+ select ADDR_LIMIT_CHECK
select ARCH_SAVE_PAGE_KEYS if HIBERNATION
select ARCH_SUPPORTS_ATOMIC_RMW
select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 980c3c9b06f8..ebde64f1622c 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -191,6 +191,28 @@ extern struct trace_event_functions exit_syscall_print_funcs;
SYSCALL_METADATA(sname, x, __VA_ARGS__) \
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
+
+/*
+ * Called before coming back to user-mode. Returning to user-mode with an
+ * address limit different than USER_DS can allow to overwrite kernel memory.
+ */
+static inline void addr_limit_check_syscall(void)
+{
+ BUG_ON(!segment_eq(get_fs(), USER_DS));
+}
+
+#ifndef CONFIG_ADDR_LIMIT_CHECK
+#define ADDR_LIMIT_CHECK_PRE() \
+ bool user_caller = segment_eq(get_fs(), USER_DS)
+#define ADDR_LIMIT_CHECK_POST() \
+ if (user_caller) addr_limit_check_syscall()
+#else
+#define ADDR_LIMIT_CHECK_PRE()
+#define ADDR_LIMIT_CHECK_POST()
+asmlinkage void addr_limit_check_failed(void) __noreturn;
+#endif
+
+
#define __PROTECT(...) asmlinkage_protect(__VA_ARGS__)
#define __SYSCALL_DEFINEx(x, name, ...) \
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
@@ -199,7 +221,10 @@ extern struct trace_event_functions exit_syscall_print_funcs;
asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
asmlinkage long SyS##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
{ \
- long ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
+ long ret; \
+ ADDR_LIMIT_CHECK_PRE(); \
+ ret = SYSC##name(__MAP(x,__SC_CAST,__VA_ARGS__)); \
+ ADDR_LIMIT_CHECK_POST(); \
__MAP(x,__SC_TEST,__VA_ARGS__); \
__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
return ret; \
diff --git a/init/Kconfig b/init/Kconfig
index 42a346b0df43..599d9fe30703 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1961,6 +1961,12 @@ config PROFILING
config TRACEPOINTS
bool
+config ADDR_LIMIT_CHECK
+ bool
+ help
+ Disable the generic address limit check. Allow each architecture to
+ optimize how and when the verification is done.
+
source "arch/Kconfig"
endmenu # General setup
diff --git a/kernel/sys.c b/kernel/sys.c
index 8a94b4eabcaa..a1cbcd715d62 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2458,3 +2458,16 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info)
return 0;
}
#endif /* CONFIG_COMPAT */
+
+#ifdef CONFIG_ADDR_LIMIT_CHECK
+/*
+ * Used when an architecture specific implementation detects an invalid address
+ * limit. This function does not return.
+ */
+asmlinkage void addr_limit_check_failed(void)
+{
+ /* Try to fail on the generic address limit check */
+ addr_limit_check_syscall();
+ panic("Invalid address limit before returning to user-mode");
+}
+#endif
--
2.13.0.rc0.306.g87b477812d-goog
^ permalink raw reply related
* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Christoffer Dall @ 2017-04-27 14:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d0aa6771-a8c3-276e-0f70-d709cdf678d6@redhat.com>
Hi Eric,
On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:
> On 27/04/2017 13:02, Christoffer Dall wrote:
> > On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:
> >> On 27/04/2017 10:57, Christoffer Dall wrote:
> >>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
> >>>> On 26/04/2017 14:31, Christoffer Dall wrote:
> >>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:
> >>>>>> Add description for how to access ITS registers and how to save/restore
> >>>>>> ITS tables into/from memory.
> >>>>>>
> >>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>>>>>
> >>>>>> ---
> >>>>>> v4 -> v5:
> >>>>>> - take into account Christoffer's comments
> >>>>>> - pending table save on GICV3 side now
> >>>>>>
> >>>>>> v3 -> v4:
> >>>>>> - take into account Peter's comments:
> >>>>>> - typos
> >>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0
> >>>>>> - add a validity bit in DTE
> >>>>>> - document all fields in CTE and ITE
> >>>>>> - document ABI revision
> >>>>>> - take into account Andre's comments:
> >>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR
> >>>>>> - document -EBUSY error if one or more VCPUS are runnning
> >>>>>> - document 64b registers only can be accessed with 64b access
> >>>>>> - itt_addr field matches bits [51:8] of the itt_addr
> >>>>>>
> >>>>>> v1 -> v2:
> >>>>>> - DTE and ITE now are 8 bytes
> >>>>>> - DTE and ITE now indexed by deviceid/eventid
> >>>>>> - use ITE name instead of ITTE
> >>>>>> - mentions ITT_addr matches bits [51:8] of the actual address
> >>>>>> - mentions LE layout
> >>>>>> ---
> >>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++
> >>>>>> 1 file changed, 99 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >>>>>> index 6081a5b..b5f010d 100644
> >>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> >>>>>> @@ -32,7 +32,106 @@ Groups:
> >>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT
> >>>>>> request the initialization of the ITS, no additional parameter in
> >>>>>> kvm_device_attr.addr.
> >>>>>> +
> >>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES
> >>>>>> + save the ITS table data into guest RAM, at the location provisioned
> >>>>>> + by the guest in corresponding registers/table entries.
> >>>>>> +
> >>>>>> + The layout of the tables in guest memory defines an ABI. The entries
> >>>>>> + are laid out in little endian format as described in the last paragraph.
> >>>>>> +
> >>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES
> >>>>>> + restore the ITS tables from guest RAM to ITS internal structures.
> >>>>>> +
> >>>>>> + The GICV3 must be restored before the ITS and all ITS registers but
> >>>>>> + the GITS_CTLR must be restored before restoring the ITS tables.
> >>>>>> +
> >>>>>> + The GITS_IIDR read-only register must also be restored before
> >>>>>> + the table restore as the IIDR revision field encodes the ABI revision.
> >>>>>> +
> >>>>>
> >>>>> what is the expected sequence of operations. For example, to restore
> >>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all
> >>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES?
> >>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers
> >>>> except GITS_CTLR, then table restore, then GITS_CTLR
> >>>>>
> >>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES
> >>>>> and restore GITS_CTLR (which enables the ITS)?
> >>>>
> >>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event
> >>>> that the pending table is read. But the whole pending table is not read
> >>>> as we only iterate on registered LPIs. So the ITT must have been
> >>>> restored previously.
> >>>>
> >>>> I became aware that the pending table sync is done twice, once in the
> >>>> pending table restore, and once in the GITS_CTLR restore. So if we
> >>>> leave this order specification, I should be able to remove the sync on
> >>>> table restore. This was the original reason why GITS_CTLR restore has
> >>>> been done at the very end.
> >>>
> >>> I'm sorry, I'm a bit confused. Do we not need
> >>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then?
> >>
> >> Yes you do. I was talking about the RDIST pending table sync. The save
> >> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.
> >> However the sync is implicit on GITS_CTLR restore if LPIs are enabled.
> >> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES
> >> which is not requested I think since GITS_CTLR restore does it already.
> >
> > Shouldn't restoring the pending tables happen when restoring some
> > redeistributor state and not anything related to the ITS?
>
> Marc wrote:
> "
> I don't think you necessarily need a coarse map. When restoring the ITS
> tables, you can always read the pending bit when creating the LPI
> structure (it has been written to RAM at save time). Note that we
> already do something like this in vgic_enable_lpis().
> "
>
> This is currently what is implemented I think. the pending tables are
> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously
> also on on ITS table restore
>
> The problematic is: Either you know in advance which LPI INTIDare used
> or you need to parse the whole pending table (possibly using the 1st kB
> as coarse mapping).
>
> If you don't know the LPI INTIDs in advance it is only possible to
> restore the pending bit of pending LPIs. At that time you would
> re-allocate those pending LPI (vgic_add_lpi) and when you restore the
> ITS ITT you would do the same for those which were not pending. Looks
> really heavy to me: coarse mapping + dual vgic_add_lpi path.
>
> Otherwise we would need to add another dependency between RDIST pending
> table restore and ITS table restore but this looks even more weird, no?
>
>
So I just sat down with Andre and Marc and we tried to work through this
and came up with the best scheme. I apologize in advance for the
one-way nature of this e-mail, and I am of course open to discussing the
following proposal again if you do not agree.
What I think this document should say, is that the following ordering
must be followed when restoring the GIC and the ITS:
First, restore all guest memory
Second, restore ALL redistributors
Third, restore the ITS, in the following order:
1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
2. Restore GITS_CBASER
3. Restore all other GITS_ registers, except GITS_CTLR!
4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
5. Restore GITS_CTLR
The rationale is that we really want the redistributor and the ITS
restore to be independent and follow the architecture. This means that
our ABI for the redistributor should still work without restoring an ITS
(if we ever decide to support LPIs for KVM without the ITS).
In terms of our current implementation this means that vgic_add_lpi()
should ask the redistributor what the state of the LPI is (priority,
enabled, pending). I suggest you do the pending check by adding a
function called something like vgic_v3_lpi_is_pending() which scans the
bit in memory, clears the memory bit, and returns the value. Clearing
the pending bit in memory when moving it to the struct irq is nice,
because you then don't have to clear out the entire pending table later
and we don't keep 'consumed' data lying around. This change should be
implemented in its_sync_lpi_pending_table() as well, but note that you
need never call that function in the normal restore path using this
design.
I hope this makes sense.
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH] mtd: spi-nor: stm32: remove broken MODULE_ALIAS
From: Cyrille Pitchen @ 2017-04-27 14:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427111001.2587551-1-arnd@arndb.de>
Le 27/04/2017 ? 13:09, Arnd Bergmann a ?crit :
> The MODULE_ALIAS statement refers to a macro that has never been defined
> in this driver, causing a build error:
>
> drivers/mtd/spi-nor/stm32-quadspi.c:694:150: error: expected ',' or ';' before 'DRIVER_NAME'
>
> Unless there is a specific alias we need other then the driver name,
> we don't need an alias here and can simply remove the line to fix the
> build.
>
> Fixes: dc5e400611c5 ("mtd: spi-nor: add driver for STM32 quad spi flash controller")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Thanks!
> ---
> drivers/mtd/spi-nor/stm32-quadspi.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c b/drivers/mtd/spi-nor/stm32-quadspi.c
> index 90d9152ddf98..1056e7408d2a 100644
> --- a/drivers/mtd/spi-nor/stm32-quadspi.c
> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c
> @@ -691,7 +691,6 @@ static struct platform_driver stm32_qspi_driver = {
> };
> module_platform_driver(stm32_qspi_driver);
>
> -MODULE_ALIAS("platform:" DRIVER_NAME);
> MODULE_AUTHOR("Ludovic Barre <ludovic.barre@st.com>");
> MODULE_DESCRIPTION("STMicroelectronics STM32 quad spi driver");
> MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* [PATCH v5 10/22] KVM: arm64: vgic-its: Implement vgic_mmio_uaccess_write_its_iidr
From: Christoffer Dall @ 2017-04-27 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1492164934-988-11-git-send-email-eric.auger@redhat.com>
On Fri, Apr 14, 2017 at 12:15:22PM +0200, Eric Auger wrote:
> The GITS_IIDR revision field is used to encode the migration ABI
> revision. So we need to restore it to check the table layout is
> readable by the destination.
>
> By writing the IIDR, userspace thus force the ABI revision to be
forces
> used and this msut be less or equal than the max revision KVM supports.
must
less than or equal to
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
> v4 -> v5
> - rename user_revision into abi_rev and REV into MAX_ABI_REV
> - IIDR reports abi_rev set by userspace if any.
> - If value set by userspace exceeds the max supported revision, an
> error is reported.
> - add some defines
>
> v4: creation
> ---
> include/linux/irqchip/arm-gic-v3.h | 4 ++++
> virt/kvm/arm/vgic/vgic-its.c | 23 ++++++++++++++++++++---
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 9648bad..54c20bd 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -241,6 +241,10 @@
> #define GITS_TYPER_PTA (1UL << 19)
> #define GITS_TYPER_HWCOLLCNT_SHIFT 24
>
> +#define GITS_IIDR_REV_SHIFT 12
> +#define GITS_IIDR_REV(r) (((r) >> GITS_IIDR_REV_SHIFT) & 0xf)
> +#define GITS_IIDR_PRODUCTID_SHIFT 24
> +
> #define GITS_CBASER_VALID (1ULL << 63)
> #define GITS_CBASER_SHAREABILITY_SHIFT (10)
> #define GITS_CBASER_INNER_CACHEABILITY_SHIFT (59)
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 69ecfe4..1b5797e 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -401,7 +401,23 @@ static unsigned long vgic_mmio_read_its_iidr(struct kvm *kvm,
> struct vgic_its *its,
> gpa_t addr, unsigned int len)
> {
> - return (PRODUCT_ID_KVM << 24) | (IMPLEMENTER_ARM << 0);
> + return (PRODUCT_ID_KVM << GITS_IIDR_PRODUCTID_SHIFT) |
> + (its->abi_rev << GITS_IIDR_REV_SHIFT) | IMPLEMENTER_ARM;
> +}
> +
> +static int vgic_mmio_uaccess_write_its_iidr(struct kvm *kvm,
> + struct vgic_its *its,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u64 tmp = 0;
> +
> + tmp = update_64bit_reg(tmp, addr & 3, len, val);
but the IIDR is not a 64bit register?
> + tmp = GITS_IIDR_REV(tmp);
You should be able to call rev = GITS_IIDR_REV(val) and be done.
> +
> + if (tmp > MAX_ABI_REV)
> + return -EINVAL;
> + return vgic_its_set_abi(its, tmp);
> }
>
> static unsigned long vgic_mmio_read_its_idregs(struct kvm *kvm,
> @@ -1382,8 +1398,9 @@ static struct vgic_register_region its_registers[] = {
> REGISTER_ITS_DESC(GITS_CTLR,
> vgic_mmio_read_its_ctlr, vgic_mmio_write_its_ctlr, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_ITS_DESC(GITS_IIDR,
> - vgic_mmio_read_its_iidr, its_mmio_write_wi, 4,
> + REGISTER_ITS_DESC_UACCESS(GITS_IIDR,
> + vgic_mmio_read_its_iidr, its_mmio_write_wi,
> + vgic_mmio_uaccess_write_its_iidr, 4,
> VGIC_ACCESS_32bit),
> REGISTER_ITS_DESC(GITS_TYPER,
> vgic_mmio_read_its_typer, its_mmio_write_wi, 8,
> --
> 2.5.5
>
Besides these smaller details, this looks ok to me.
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH] [media] atmel-isc: Set the default DMA memory burst size
From: Nicolas Ferre @ 2017-04-27 15:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170420085130.24326-1-songjun.wu@microchip.com>
Le 20/04/2017 ? 10:51, Songjun Wu a ?crit :
> Sometimes 'DMA single access' is not enough to transfer
> a frame of image, '8-beat burst access' is set as the
> default DMA memory burst size.
>
> Signed-off-by: Songjun Wu <songjun.wu@microchip.com>
Seems okay:
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
>
> drivers/media/platform/atmel/atmel-isc.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index c4b2115559a5..78d966233f80 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -239,13 +239,11 @@ static struct isc_format isc_formats[] = {
>
> { V4L2_PIX_FMT_YUV420, 0x0, 12,
> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
> - ISC_DCFG_IMODE_YC420P | ISC_DCFG_YMBSIZE_BEATS8 |
> - ISC_DCFG_CMBSIZE_BEATS8, ISC_DCTRL_DVIEW_PLANAR, 0x7fb,
> + ISC_DCFG_IMODE_YC420P, ISC_DCTRL_DVIEW_PLANAR, 0x7fb,
> false, false },
> { V4L2_PIX_FMT_YUV422P, 0x0, 16,
> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
> - ISC_DCFG_IMODE_YC422P | ISC_DCFG_YMBSIZE_BEATS8 |
> - ISC_DCFG_CMBSIZE_BEATS8, ISC_DCTRL_DVIEW_PLANAR, 0x3fb,
> + ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb,
> false, false },
> { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16,
> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_RGB565,
> @@ -700,8 +698,10 @@ static void isc_set_histogram(struct isc_device *isc)
> }
>
> static inline void isc_get_param(const struct isc_format *fmt,
> - u32 *rlp_mode, u32 *dcfg_imode)
> + u32 *rlp_mode, u32 *dcfg)
> {
> + *dcfg = ISC_DCFG_YMBSIZE_BEATS8;
> +
> switch (fmt->fourcc) {
> case V4L2_PIX_FMT_SBGGR10:
> case V4L2_PIX_FMT_SGBRG10:
> @@ -712,11 +712,11 @@ static inline void isc_get_param(const struct isc_format *fmt,
> case V4L2_PIX_FMT_SGRBG12:
> case V4L2_PIX_FMT_SRGGB12:
> *rlp_mode = fmt->reg_rlp_mode;
> - *dcfg_imode = fmt->reg_dcfg_imode;
> + *dcfg |= fmt->reg_dcfg_imode;
> break;
> default:
> *rlp_mode = ISC_RLP_CFG_MODE_DAT8;
> - *dcfg_imode = ISC_DCFG_IMODE_PACKED8;
> + *dcfg |= ISC_DCFG_IMODE_PACKED8;
> break;
> }
> }
> @@ -726,18 +726,19 @@ static int isc_configure(struct isc_device *isc)
> struct regmap *regmap = isc->regmap;
> const struct isc_format *current_fmt = isc->current_fmt;
> struct isc_subdev_entity *subdev = isc->current_subdev;
> - u32 pfe_cfg0, rlp_mode, dcfg_imode, mask, pipeline;
> + u32 pfe_cfg0, rlp_mode, dcfg, mask, pipeline;
>
> if (sensor_is_preferred(current_fmt)) {
> pfe_cfg0 = current_fmt->reg_bps;
> pipeline = 0x0;
> - isc_get_param(current_fmt, &rlp_mode, &dcfg_imode);
> + isc_get_param(current_fmt, &rlp_mode, &dcfg);
> isc->ctrls.hist_stat = HIST_INIT;
> } else {
> pfe_cfg0 = isc->raw_fmt->reg_bps;
> pipeline = current_fmt->pipeline;
> rlp_mode = current_fmt->reg_rlp_mode;
> - dcfg_imode = current_fmt->reg_dcfg_imode;
> + dcfg = current_fmt->reg_dcfg_imode | ISC_DCFG_YMBSIZE_BEATS8 |
> + ISC_DCFG_CMBSIZE_BEATS8;
> }
>
> pfe_cfg0 |= subdev->pfe_cfg0 | ISC_PFE_CFG0_MODE_PROGRESSIVE;
> @@ -750,7 +751,7 @@ static int isc_configure(struct isc_device *isc)
> regmap_update_bits(regmap, ISC_RLP_CFG, ISC_RLP_CFG_MODE_MASK,
> rlp_mode);
>
> - regmap_update_bits(regmap, ISC_DCFG, ISC_DCFG_IMODE_MASK, dcfg_imode);
> + regmap_write(regmap, ISC_DCFG, dcfg);
>
> /* Set the pipeline */
> isc_set_pipeline(isc, pipeline);
>
--
Nicolas Ferre
^ permalink raw reply
* [PATCH v5 1/4] printk/nmi: generic solution for safe printk in NMI
From: Petr Mladek @ 2017-04-27 15:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427103118.56351d30@gandalf.local.home>
On Thu 2017-04-27 10:31:18, Steven Rostedt wrote:
> On Thu, 27 Apr 2017 15:38:19 +0200
> Petr Mladek <pmladek@suse.com> wrote:
>
> > > by the way,
> > > does this `nmi_print_seq' bypass even fix anything for Steven?
> >
> > I think that this is the most important question.
> >
> > Steven, does the patch from
> > https://lkml.kernel.org/r/20170420131154.GL3452 at pathway.suse.cz
> > help you to see the debug messages, please?
>
> You'll have to wait for a bit. The box that I was debugging takes 45
> minutes to reboot. And I don't have much more time to play on it before
> I have to give it back. I already found the bug I was looking for and
> I'm trying not to crash it again (due to the huge bring up time).
I see.
> When I get a chance, I'll see if I can insert a trigger to crash the
> kernel from NMI on another box and see if this patch helps.
I actually tested it here using this hack:
diff --cc lib/nmi_backtrace.c
index d531f85c0c9b,0bc0a3535a8a..000000000000
--- a/lib/nmi_backtrace.c
+++ b/lib/nmi_backtrace.c
@@@ -89,8 -90,7 +90,9 @@@ bool nmi_cpu_backtrace(struct pt_regs *
int cpu = smp_processor_id();
if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
+ if (in_nmi())
+ panic("Simulating panic in NMI\n");
+ arch_spin_lock(&lock);
if (regs && cpu_in_idle(instruction_pointer(regs))) {
pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
cpu, instruction_pointer(regs));
and triggered by:
echo l > /proc/sysrq-trigger
The patch really helped to see much more (all) messages from the ftrace
buffers in NMI mode.
But the test is a bit artifical. The patch might not help when there
is a big printk() activity on the system when the panic() is
triggered. We might wrongly use the small per-CPU buffer when
the logbuf_lock is tested and taken on another CPU at the same time.
It means that it will not always help.
I personally think that the patch might be good enough. I am not sure
if a perfect (more comlpex) solution is worth it.
Best Regards,
Petr
^ permalink raw reply
* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Auger Eric @ 2017-04-27 15:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427144516.GL50776@lvm>
On 27/04/2017 16:45, Christoffer Dall wrote:
> Hi Eric,
>
> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:
>> On 27/04/2017 13:02, Christoffer Dall wrote:
>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:
>>>> On 27/04/2017 10:57, Christoffer Dall wrote:
>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote:
>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:
>>>>>>>> Add description for how to access ITS registers and how to save/restore
>>>>>>>> ITS tables into/from memory.
>>>>>>>>
>>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> v4 -> v5:
>>>>>>>> - take into account Christoffer's comments
>>>>>>>> - pending table save on GICV3 side now
>>>>>>>>
>>>>>>>> v3 -> v4:
>>>>>>>> - take into account Peter's comments:
>>>>>>>> - typos
>>>>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0
>>>>>>>> - add a validity bit in DTE
>>>>>>>> - document all fields in CTE and ITE
>>>>>>>> - document ABI revision
>>>>>>>> - take into account Andre's comments:
>>>>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR
>>>>>>>> - document -EBUSY error if one or more VCPUS are runnning
>>>>>>>> - document 64b registers only can be accessed with 64b access
>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr
>>>>>>>>
>>>>>>>> v1 -> v2:
>>>>>>>> - DTE and ITE now are 8 bytes
>>>>>>>> - DTE and ITE now indexed by deviceid/eventid
>>>>>>>> - use ITE name instead of ITTE
>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address
>>>>>>>> - mentions LE layout
>>>>>>>> ---
>>>>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++
>>>>>>>> 1 file changed, 99 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>> index 6081a5b..b5f010d 100644
>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>> @@ -32,7 +32,106 @@ Groups:
>>>>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT
>>>>>>>> request the initialization of the ITS, no additional parameter in
>>>>>>>> kvm_device_attr.addr.
>>>>>>>> +
>>>>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES
>>>>>>>> + save the ITS table data into guest RAM, at the location provisioned
>>>>>>>> + by the guest in corresponding registers/table entries.
>>>>>>>> +
>>>>>>>> + The layout of the tables in guest memory defines an ABI. The entries
>>>>>>>> + are laid out in little endian format as described in the last paragraph.
>>>>>>>> +
>>>>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>> + restore the ITS tables from guest RAM to ITS internal structures.
>>>>>>>> +
>>>>>>>> + The GICV3 must be restored before the ITS and all ITS registers but
>>>>>>>> + the GITS_CTLR must be restored before restoring the ITS tables.
>>>>>>>> +
>>>>>>>> + The GITS_IIDR read-only register must also be restored before
>>>>>>>> + the table restore as the IIDR revision field encodes the ABI revision.
>>>>>>>> +
>>>>>>>
>>>>>>> what is the expected sequence of operations. For example, to restore
>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all
>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES?
>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers
>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR
>>>>>>>
>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>> and restore GITS_CTLR (which enables the ITS)?
>>>>>>
>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event
>>>>>> that the pending table is read. But the whole pending table is not read
>>>>>> as we only iterate on registered LPIs. So the ITT must have been
>>>>>> restored previously.
>>>>>>
>>>>>> I became aware that the pending table sync is done twice, once in the
>>>>>> pending table restore, and once in the GITS_CTLR restore. So if we
>>>>>> leave this order specification, I should be able to remove the sync on
>>>>>> table restore. This was the original reason why GITS_CTLR restore has
>>>>>> been done at the very end.
>>>>>
>>>>> I'm sorry, I'm a bit confused. Do we not need
>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then?
>>>>
>>>> Yes you do. I was talking about the RDIST pending table sync. The save
>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.
>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled.
>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>> which is not requested I think since GITS_CTLR restore does it already.
>>>
>>> Shouldn't restoring the pending tables happen when restoring some
>>> redeistributor state and not anything related to the ITS?
>>
>> Marc wrote:
>> "
>> I don't think you necessarily need a coarse map. When restoring the ITS
>> tables, you can always read the pending bit when creating the LPI
>> structure (it has been written to RAM at save time). Note that we
>> already do something like this in vgic_enable_lpis().
>> "
>>
>> This is currently what is implemented I think. the pending tables are
>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously
>> also on on ITS table restore
>>
>> The problematic is: Either you know in advance which LPI INTIDare used
>> or you need to parse the whole pending table (possibly using the 1st kB
>> as coarse mapping).
>>
>> If you don't know the LPI INTIDs in advance it is only possible to
>> restore the pending bit of pending LPIs. At that time you would
>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the
>> ITS ITT you would do the same for those which were not pending. Looks
>> really heavy to me: coarse mapping + dual vgic_add_lpi path.
>>
>> Otherwise we would need to add another dependency between RDIST pending
>> table restore and ITS table restore but this looks even more weird, no?
>>
>>
> So I just sat down with Andre and Marc and we tried to work through this
> and came up with the best scheme. I apologize in advance for the
> one-way nature of this e-mail, and I am of course open to discussing the
> following proposal again if you do not agree.
>
> What I think this document should say, is that the following ordering
> must be followed when restoring the GIC and the ITS:
>
> First, restore all guest memory
>
> Second, restore ALL redistributors
>
> Third, restore the ITS, in the following order:
> 1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
> 2. Restore GITS_CBASER
> 3. Restore all other GITS_ registers, except GITS_CTLR!
> 4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
> 5. Restore GITS_CTLR
>
> The rationale is that we really want the redistributor and the ITS
> restore to be independent and follow the architecture. This means that
> our ABI for the redistributor should still work without restoring an ITS
> (if we ever decide to support LPIs for KVM without the ITS).
OK. Note I already mentioned that GICv3 must be restored before the ITS.
To me this comprised the RDIST.
I understand the above description of the ordering comes in addition to
the existing text, right? in other words I keep the GITS_READR,
GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES
section.
>
> In terms of our current implementation this means that vgic_add_lpi()
> should ask the redistributor what the state of the LPI is (priority,
> enabled, pending).
this practically means I move update_lpi_config call from
vgic_its_restore_ite to vgic_add_lpi(). OK
However for getting the LPI pending state I must know which RDIST the
LPI is attached to. This is not known at LPI allocation time. Do I
misunderstand something?
Thanks
Eric
I suggest you do the pending check by adding a
> function called something like vgic_v3_lpi_is_pending() which scans the
> bit in memory, clears the memory bit, and returns the value. Clearing
> the pending bit in memory when moving it to the struct irq is nice,
> because you then don't have to clear out the entire pending table later
> and we don't keep 'consumed' data lying around. This change should be
> implemented in its_sync_lpi_pending_table() as well, but note that you
> need never call that function in the normal restore path using this
> design.
>
> I hope this makes sense.
>
> Thanks,
> -Christoffer
>
^ permalink raw reply
* [PULL 00/79] KVM/ARM Changes for v4.12
From: Paolo Bonzini @ 2017-04-27 15:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170423170929.27334-1-cdall@linaro.org>
On 23/04/2017 19:08, Christoffer Dall wrote:
> git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git kvm-arm-for-v4.12
Pulled, thanks.
Paolo
^ permalink raw reply
* [PATCH v5 1/4] printk/nmi: generic solution for safe printk in NMI
From: Steven Rostedt @ 2017-04-27 15:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427152807.GY3452@pathway.suse.cz>
On Thu, 27 Apr 2017 17:28:07 +0200
Petr Mladek <pmladek@suse.com> wrote:
> > When I get a chance, I'll see if I can insert a trigger to crash the
> > kernel from NMI on another box and see if this patch helps.
>
> I actually tested it here using this hack:
>
> diff --cc lib/nmi_backtrace.c
> index d531f85c0c9b,0bc0a3535a8a..000000000000
> --- a/lib/nmi_backtrace.c
> +++ b/lib/nmi_backtrace.c
> @@@ -89,8 -90,7 +90,9 @@@ bool nmi_cpu_backtrace(struct pt_regs *
> int cpu = smp_processor_id();
>
> if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
> + if (in_nmi())
> + panic("Simulating panic in NMI\n");
> + arch_spin_lock(&lock);
I was going to create a ftrace trigger, to crash on demand, but this
may do as well.
> if (regs && cpu_in_idle(instruction_pointer(regs))) {
> pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
> cpu, instruction_pointer(regs));
>
> and triggered by:
>
> echo l > /proc/sysrq-trigger
>
> The patch really helped to see much more (all) messages from the ftrace
> buffers in NMI mode.
>
> But the test is a bit artifical. The patch might not help when there
> is a big printk() activity on the system when the panic() is
> triggered. We might wrongly use the small per-CPU buffer when
> the logbuf_lock is tested and taken on another CPU at the same time.
> It means that it will not always help.
>
> I personally think that the patch might be good enough. I am not sure
> if a perfect (more comlpex) solution is worth it.
I wasn't asking for perfect, as the previous solutions never were
either. I just want an optimistic dump if possible.
I'll try to get some time today to test this, and let you know. But it
wont be on the machine that I originally had the issue with.
Thanks,
-- Steve
^ permalink raw reply
* [PATCH] perf evsel: Fix to perf-stat malloc corruption on arm64 platforms
From: Ganapatrao Kulkarni @ 2017-04-27 15:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427143438.GE31337@leverpostej>
On Thu, Apr 27, 2017 at 8:04 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Apr 26, 2017 at 11:49:46PM +0530, Ganapatrao Kulkarni wrote:
>> On Wed, Apr 26, 2017 at 10:42 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
>> > index 13b5499..638aefa 100644
>> > --- a/tools/perf/builtin-stat.c
>> > +++ b/tools/perf/builtin-stat.c
>> > @@ -346,6 +346,28 @@ static void read_counters(void)
>> > }
>> > }
>> >
>> > +/*
>> > + * Close all evnt FDs we open in __run_perf_stat() and
>> > + * create_perf_stat_counter(), taking care to match the number of threads and CPUs.
>> > + *
>> > + * Note that perf_evlist__close(evsel_list) is not equivalent, as it doesn't
>> > + * take the target into account.
>> > + */
>> > +static void close_counters(void)
>> > +{
>> > + bool per_cpu = target__has_cpu(&target);
>> > + struct perf_evsel *evsel;
>> > +
>> > + evlist__for_each_entry(evsel_list, evsel) {
>> > + if (per_cpu)
>> > + perf_evsel__close_per_cpu(evsel,
>> > + perf_evsel__cpus(evsel));
>> > + else
>> > + perf_evsel__close_per_thread(evsel,
>> > + evsel_list->threads);
>> > + }
>> > +}
>> > +
>> > static void process_interval(void)
>> > {
>> > struct timespec ts, rs;
>> > @@ -686,7 +708,7 @@ static int __run_perf_stat(int argc, const char **argv)
>> > * group leaders.
>> > */
>> > read_counters();
>> > - perf_evlist__close(evsel_list);
>> > + close_counters();
>> >
>> > return WEXITSTATUS(status);
>> > }
>> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> > index ac59710..726ceca 100644
>> > --- a/tools/perf/util/evsel.c
>> > +++ b/tools/perf/util/evsel.c
>> > @@ -1670,6 +1670,18 @@ int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>> > return err;
>> > }
>> >
>> > +int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
>> > + struct cpu_map *cpus)
>> > +{
>> > + return perf_evsel__open(evsel, cpus, NULL);
>> > +}
>> > +
>> > +int perf_evsel__open_per_thread(struct perf_evsel *evsel,
>> > + struct thread_map *threads)
>> > +{
>> > + return perf_evsel__open(evsel, NULL, threads);
>> > +}
>> > +
>> > void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads)
>> > {
>> > if (evsel->fd == NULL)
>> > @@ -1679,16 +1691,18 @@ void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads)
>> > perf_evsel__free_fd(evsel);
>> > }
>> >
>> > -int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
>> > - struct cpu_map *cpus)
>> > +void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
>> > + struct cpu_map *cpus)
>> > {
>> > - return perf_evsel__open(evsel, cpus, NULL);
>> > + int ncpus = cpus ? cpus->nr : 1;
>> > + perf_evsel__close(evsel, ncpus, 1);
>> > }
>> >
>> > -int perf_evsel__open_per_thread(struct perf_evsel *evsel,
>> > - struct thread_map *threads)
>> > +void perf_evsel__close_per_thread(struct perf_evsel *evsel,
>> > + struct thread_map *threads)
>> > {
>> > - return perf_evsel__open(evsel, NULL, threads);
>> > + int nthreads = threads ? threads->nr : 1;
>> > + perf_evsel__close(evsel, 1, nthreads);
>> > }
>> >
>> > static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel,
>> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
>> > index 06ef6f2..02bea43 100644
>> > --- a/tools/perf/util/evsel.h
>> > +++ b/tools/perf/util/evsel.h
>> > @@ -252,6 +252,10 @@ int perf_evsel__open_per_thread(struct perf_evsel *evsel,
>> > struct thread_map *threads);
>> > int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
>> > struct thread_map *threads);
>> > +void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
>> > + struct cpu_map *cpus);
>> > +void perf_evsel__close_per_thread(struct perf_evsel *evsel,
>> > + struct thread_map *threads);
>> > void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads);
>> >
>> > struct perf_sample;
>>
>> this diff looks to me doing same as mine.
>
> Be careful when reading the diff above; the open functions have been
> moved, but have not changed.
>
> I've only changed the close path, whereas your proposal changed the open
> path. Those are not equivalent changes.
>
>> i think below diff should be more appropriate fix to this issue?
>>
>> when open allocates and uses dummy cpus, there is no point in holding
>> old unused one. instead it should free and link to dummy cpus which
>> is created with 1 CPU. same will be used by close.
>>
>> i did quick testing on both x86 and arm64. testing looks ok, may need
>> more testing!
>>
>> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
>> index ac59710..b1aab0a 100644
>> --- a/tools/perf/util/evsel.c
>> +++ b/tools/perf/util/evsel.c
>> @@ -1466,9 +1466,13 @@ int perf_evsel__open(struct perf_evsel *evsel,
>> struct cpu_map *cpus,
>> empty_cpu_map = cpu_map__dummy_new();
>> if (empty_cpu_map == NULL)
>> return -ENOMEM;
>> + } else {
>> + cpu_map__get(empty_cpu_map);
>> }
>>
>> cpus = empty_cpu_map;
>> + cpu_map__put(evsel->cpus);
>> + evsel->cpus = cpus;
>> }
>>
>> if (threads == NULL) {
>
> Unfortunately, I believe that might break the logic added in commit:
>
> 9f21b815be863218 ("perf evlist: Only open events on CPUs an evsel permits")
>
> ... since the evsel->cpus would now not represent the PMUs CPUs.
>
> As I'd mentioned in my prior reply, I think in order to use the cpu_maps
> consistently we need to do a bigger rework of the way cpu_maps are used,
> in order to separate the PMU CPUs from the requested event CPUs, etc.
> while taking all of these into account.
>
> Could you please give my diff a go?
i tried your diff, and testing looks ok.
below is the cleanly merged diff on top of latest commit
f832460 Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 13b5499..4be2980 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -346,6 +346,28 @@ static void read_counters(void)
}
}
+/*
+ * Close all evnt FDs we open in __run_perf_stat() and
+ * create_perf_stat_counter(), taking care to match the number of
threads and CPUs.
+ *
+ * Note that perf_evlist__close(evsel_list) is not equivalent, as it doesn't
+ * take the target into account.
+ */
+static void close_counters(void)
+{
+ bool per_cpu = target__has_cpu(&target);
+ struct perf_evsel *evsel;
+
+ evlist__for_each_entry(evsel_list, evsel) {
+ if (per_cpu)
+ perf_evsel__close_per_cpu(evsel,
+ perf_evsel__cpus(evsel));
+ else
+ perf_evsel__close_per_thread(evsel,
+ evsel_list->threads);
+ }
+}
+
static void process_interval(void)
{
struct timespec ts, rs;
@@ -686,7 +708,7 @@ static int __run_perf_stat(int argc, const char **argv)
* group leaders.
*/
read_counters();
- perf_evlist__close(evsel_list);
+ close_counters();
return WEXITSTATUS(status);
}
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ac59710..ecd9778 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1691,6 +1691,20 @@ int perf_evsel__open_per_thread(struct perf_evsel *evsel,
return perf_evsel__open(evsel, NULL, threads);
}
+void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
+ struct cpu_map *cpus)
+ {
+ int ncpus = cpus ? cpus->nr : 1;
+ perf_evsel__close(evsel, ncpus, 1);
+ }
+
+void perf_evsel__close_per_thread(struct perf_evsel *evsel,
+ struct thread_map *threads)
+ {
+ int nthreads = threads ? threads->nr : 1;
+ perf_evsel__close(evsel, 1, nthreads);
+ }
+
static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel,
const union perf_event *event,
struct perf_sample *sample)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index 06ef6f2..6779bd2 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -250,6 +250,10 @@ int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
struct cpu_map *cpus);
int perf_evsel__open_per_thread(struct perf_evsel *evsel,
struct thread_map *threads);
+void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
+ struct cpu_map *cpus);
+void perf_evsel__close_per_thread(struct perf_evsel *evsel,
+ struct thread_map *threads);
int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
struct thread_map *threads);
void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads);
>
> Thanks,
> Mark.
thanks
Ganapat
^ permalink raw reply related
* [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() (was: Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem)
From: Mark Rutland @ 2017-04-27 15:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170427123056.GD31337@leverpostej>
Hi Catalin/Will,
The below addresses a boot failure Catalin spotted in next-20170424,
based on Sebastian's patch [1]. I've given it a spin on Juno R1, where I
can reproduce the issue prior to applying this patch.
I believe this would need to go via tip, as the issue is a result of
change in the tip smp/hotplug branch, and the fix depends on
infrastructure introduced there.
Are you happy with the fix, and for it to go via the tip tree?
Thanks,
Mark.
[1] https://lkml.kernel.org/r/20170425172838.mr3kyccsdteyjso5 at linutronix.de
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/log/?h=smp/hotplug
---->8----
>From 6cdb503b060f74743769c9f601c35f985d3c58eb Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 26 Apr 2017 09:46:47 +0100
Subject: [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked()
Recently, the hotplug locking was conveted to use a percpu rwsem. Unlike
the existing {get,put}_online_cpus() logic, this can't nest.
Unfortunately, in arm64's secondary boot path we can end up nesting via
static_branch_enable() in cpus_set_cap() when we detect an erratum.
This leads to a stream of messages as below, where the secondary
attempts to schedule before it has been fully onlined. As the CPU
orchestrating the onlining holds the rswem, this hangs the system.
[ 0.250334] BUG: scheduling while atomic: swapper/1/0/0x00000002
[ 0.250337] Modules linked in:
[ 0.250346] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc7-next-20170424 #2
[ 0.250349] Hardware name: ARM Juno development board (r1) (DT)
[ 0.250353] Call trace:
[ 0.250365] [<ffff000008088510>] dump_backtrace+0x0/0x238
[ 0.250371] [<ffff00000808880c>] show_stack+0x14/0x20
[ 0.250377] [<ffff00000839d854>] dump_stack+0x9c/0xc0
[ 0.250384] [<ffff0000080e3540>] __schedule_bug+0x50/0x70
[ 0.250391] [<ffff000008932ecc>] __schedule+0x52c/0x5a8
[ 0.250395] [<ffff000008932f80>] schedule+0x38/0xa0
[ 0.250400] [<ffff000008935e8c>] rwsem_down_read_failed+0xc4/0x108
[ 0.250407] [<ffff0000080fe8e0>] __percpu_down_read+0x100/0x118
[ 0.250414] [<ffff0000080c0b60>] get_online_cpus+0x70/0x78
[ 0.250420] [<ffff0000081749e8>] static_key_enable+0x28/0x48
[ 0.250425] [<ffff00000808de90>] update_cpu_capabilities+0x78/0xf8
[ 0.250430] [<ffff00000808d14c>] update_cpu_errata_workarounds+0x1c/0x28
[ 0.250435] [<ffff00000808e004>] check_local_cpu_capabilities+0xf4/0x128
[ 0.250440] [<ffff00000808e894>] secondary_start_kernel+0x8c/0x118
[ 0.250444] [<000000008093d1b4>] 0x8093d1b4
We call cpus_set_cap() from update_cpu_capabilities(), which is called
from the secondary boot path (where the CPU orchestrating the onlining
holds the hotplug rwsem), and in the primary boot path, where this is
not held.
This patch makes cpus_set_cap() use static_branch_enable_cpuslocked(),
and updates the primary CPU boot path to hold the rwsem so as to keep
the *_cpuslocked() code happy.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Catalin Marinas <catalin.marinas@arm.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Suzuki Poulose <suzuki,poulose@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
arch/arm64/include/asm/cpufeature.h | 2 +-
arch/arm64/kernel/smp.c | 8 +++++---
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index f31c48d..349b5cd 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -145,7 +145,7 @@ static inline void cpus_set_cap(unsigned int num)
num, ARM64_NCAPS);
} else {
__set_bit(num, cpu_hwcaps);
- static_branch_enable(&cpu_hwcap_keys[num]);
+ static_branch_enable_cpuslocked(&cpu_hwcap_keys[num]);
}
}
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 9b10365..c2ce9aa 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -447,11 +447,13 @@ void __init smp_prepare_boot_cpu(void)
cpuinfo_store_boot_cpu();
save_boot_cpu_run_el();
/*
- * Run the errata work around checks on the boot CPU, once we have
- * initialised the cpu feature infrastructure from
- * cpuinfo_store_boot_cpu() above.
+ * Run the errata work around checks on the boot CPU, now that
+ * cpuinfo_store_boot_cpu() has set things up. We hold the percpu rwsem
+ * to keep the workaround setup code happy.
*/
+ get_online_cpus();
update_cpu_errata_workarounds();
+ put_online_cpus();
}
static u64 __init of_get_cpu_mpidr(struct device_node *dn)
--
1.9.1
^ permalink raw reply related
* [PATCH] perf evsel: Fix to perf-stat malloc corruption on arm64 platforms
From: Mark Rutland @ 2017-04-27 15:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFpQJXWjx3uVG-6UNiJzmdc4FFw8kea2A5aJZquxVRXq8BR1VQ@mail.gmail.com>
On Thu, Apr 27, 2017 at 09:16:41PM +0530, Ganapatrao Kulkarni wrote:
> > Could you please give my diff a go?
>
> i tried your diff, and testing looks ok.
Can I take that as a Tested-by when I post this as a proper patch?
> below is the cleanly merged diff on top of latest commit
> f832460 Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc
Thanks for the rebase.
Mark.
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 13b5499..4be2980 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -346,6 +346,28 @@ static void read_counters(void)
> }
> }
>
> +/*
> + * Close all evnt FDs we open in __run_perf_stat() and
> + * create_perf_stat_counter(), taking care to match the number of
> threads and CPUs.
> + *
> + * Note that perf_evlist__close(evsel_list) is not equivalent, as it doesn't
> + * take the target into account.
> + */
> +static void close_counters(void)
> +{
> + bool per_cpu = target__has_cpu(&target);
> + struct perf_evsel *evsel;
> +
> + evlist__for_each_entry(evsel_list, evsel) {
> + if (per_cpu)
> + perf_evsel__close_per_cpu(evsel,
> + perf_evsel__cpus(evsel));
> + else
> + perf_evsel__close_per_thread(evsel,
> + evsel_list->threads);
> + }
> +}
> +
> static void process_interval(void)
> {
> struct timespec ts, rs;
> @@ -686,7 +708,7 @@ static int __run_perf_stat(int argc, const char **argv)
> * group leaders.
> */
> read_counters();
> - perf_evlist__close(evsel_list);
> + close_counters();
>
> return WEXITSTATUS(status);
> }
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ac59710..ecd9778 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1691,6 +1691,20 @@ int perf_evsel__open_per_thread(struct perf_evsel *evsel,
> return perf_evsel__open(evsel, NULL, threads);
> }
>
> +void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
> + struct cpu_map *cpus)
> + {
> + int ncpus = cpus ? cpus->nr : 1;
> + perf_evsel__close(evsel, ncpus, 1);
> + }
> +
> +void perf_evsel__close_per_thread(struct perf_evsel *evsel,
> + struct thread_map *threads)
> + {
> + int nthreads = threads ? threads->nr : 1;
> + perf_evsel__close(evsel, 1, nthreads);
> + }
> +
> static int perf_evsel__parse_id_sample(const struct perf_evsel *evsel,
> const union perf_event *event,
> struct perf_sample *sample)
> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> index 06ef6f2..6779bd2 100644
> --- a/tools/perf/util/evsel.h
> +++ b/tools/perf/util/evsel.h
> @@ -250,6 +250,10 @@ int perf_evsel__open_per_cpu(struct perf_evsel *evsel,
> struct cpu_map *cpus);
> int perf_evsel__open_per_thread(struct perf_evsel *evsel,
> struct thread_map *threads);
> +void perf_evsel__close_per_cpu(struct perf_evsel *evsel,
> + struct cpu_map *cpus);
> +void perf_evsel__close_per_thread(struct perf_evsel *evsel,
> + struct thread_map *threads);
> int perf_evsel__open(struct perf_evsel *evsel, struct cpu_map *cpus,
> struct thread_map *threads);
> void perf_evsel__close(struct perf_evsel *evsel, int ncpus, int nthreads);
^ permalink raw reply
* [RFC kvm-unit-tests PATCH 4/8] Makefiles: use explicit path for including sub-Makefiles
From: Paolo Bonzini @ 2017-04-27 15:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170407122010.umftrqr2qqd6g3q4@kamzik.brq.redhat.com>
On 07/04/2017 14:20, Andrew Jones wrote:
>> -LIBFDT_objdir = lib/libfdt
>> -LIBFDT_srcdir = lib/libfdt
>> +LIBFDT_objdir = $(SRCDIR)/lib/libfdt
>> +LIBFDT_srcdir = $(SRCDIR)/lib/libfdt
>> LIBFDT_archive = $(LIBFDT_objdir)/libfdt.a
>> LIBFDT_include = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_INCLUDES))
>> LIBFDT_version = $(addprefix $(LIBFDT_srcdir)/,$(LIBFDT_VERSION))
> Can't we just do VPATH += $LIBFDT_srcdir instead?
That would search for every file in the lib/libfdt subdirectory of the
source directory, while here the desired prefix is just $(SRCDIR).
Paolo
^ permalink raw reply
* [RFC kvm-unit-tests PATCH 5/8] Makefile: add explicit mkdir for .o targets
From: Paolo Bonzini @ 2017-04-27 15:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170406190727.5624-6-alex.bennee@linaro.org>
On 06/04/2017 21:07, Alex Benn?e wrote:
> This is fairly direct way of ensuring the target build directories are
> created before we build a binary blob. mkdir -p fails gracefully if
> the directory is already there.
>
> Signed-off-by: Alex Benn?e <alex.bennee@linaro.org>
> ---
> Makefile | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 781186e..56598df 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -79,8 +79,13 @@ $(LIBFDT_archive): $(addprefix $(LIBFDT_objdir)/,$(LIBFDT_OBJS))
> $(AR) rcs $@ $^
>
> %.o: %.S
> + mkdir -p $(dir $@)
Should this use @ for cleanliness?
Paolo
> $(CC) $(CFLAGS) -c -nostdlib -o $@ $<
>
> +%.o: %.c
> + mkdir -p $(dir $@)
> + $(CC) $(CFLAGS) -c -o $@ $<
> +
> -include */.*.d */*/.*.d
>
> all: $(shell git -C $(SRCDIR) rev-parse --verify --short=8 HEAD >build-head 2>/dev/null)
>
^ permalink raw reply
* [PATCH 1/2] dt-bindings: Document STM32 I2S bindings
From: Olivier MOYSAN @ 2017-04-27 15:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170426161553.oc5pozga53ttypyq@sirena.org.uk>
Hello Mark,
On 04/26/2017 06:15 PM, Mark Brown wrote:
> On Thu, Apr 13, 2017 at 08:01:34AM +0000, Olivier MOYSAN wrote:
>
>> 1) 2 static dais NOT exclusive
>> - dai tx
>> - dai rx
>> The IP exhibits a mode register, where you select mode TX, RX or FD.
>> There are 2 two options to manage this register.
>> option 1:
>> start first channel with mode RX or TX
>> when second channel is started, mode has to be changed to FD.
>> Transfers have to be stopped before changing configuration
>> registers, so this leads to cuts in audio stream.
>> option 2:
>> start a first channel with mode FD.
>> In this case, we may have unpredictable behavior for the stream
>> which is not already started. probably underrun/overrun.
>> So, this solution rises problem for full-duplex management.
>
>> 2) 3 static dais exclusive
>> - dai tx
>> - dai rx
>> - dai rx-tx (fd)
>> This is the current implementation.
>> The choice of the dai is done at probe time. It is provided by DT
>> through sound-dai parameter.
>> When dai fd is selected, after starting first stream, we assume that
>> second stream will be started. In this case we wait for second stream
>> to be available before enabling IP and starting transfers.
>
>> 3) 1 dynamic dai
>> - dai rx or tx or fd (according to dma conf in IP node)
>> Here the driver exposes only a single dai constructed from dma
>> configuration provided by IP DT node.
>> This allows to get ride of sound-dai parameter.
>
> None of these options reflect how normal I2S controllers present
> themselves in DT. To repeat, you should present a single bidirectional
> DAI for the single physical bidirectional I2S controller that your
> hardware has.
>
> If it's not possible to figure out a way to make the controller support
> simultaneous playback and record with the two started independently then
> the driver should just return an error if userspace tries to start the
> second direction up. This will severely limit the utility of the driver
> as Linux generally treats playback and record independently but that's
> going to apply just as much with any of the options involving multiple
> DAIs or configuration in DT. You might be able to do something with
> feeding it dummy data I guess?
>
Ok. I will implement a single bidirectional DAI in v2.
Best regards
Olivier
^ permalink raw reply
* [PATCH v5 1/4] printk/nmi: generic solution for safe printk in NMI
From: Steven Rostedt @ 2017-04-27 16:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20170420131154.GL3452@pathway.suse.cz>
On Thu, 20 Apr 2017 15:11:54 +0200
Petr Mladek <pmladek@suse.com> wrote:
>
> >From c530d9dee91c74db5e6a198479e2e63b24cb84a2 Mon Sep 17 00:00:00 2001
> From: Petr Mladek <pmladek@suse.com>
> Date: Thu, 20 Apr 2017 10:52:31 +0200
> Subject: [PATCH] printk: Use the main logbuf in NMI when logbuf_lock is
> available
I tried this patch. It's better because I get the end of the trace, but
I do lose the beginning of it:
** 196358 printk messages dropped ** [ 102.321182] perf-5981 0.... 12983650us : d_path <-seq_path
The way I tested it was by adding this:
Index: linux-trace.git/kernel/trace/trace_functions.c
===================================================================
--- linux-trace.git.orig/kernel/trace/trace_functions.c
+++ linux-trace.git/kernel/trace/trace_functions.c
@@ -469,8 +469,11 @@ ftrace_cpudump_probe(unsigned long ip, u
struct trace_array *tr, struct ftrace_probe_ops *ops,
void *data)
{
- if (update_count(ops, ip, data))
- ftrace_dump(DUMP_ORIG);
+ char *killer = NULL;
+
+ panic_on_oops = 1; /* force panic */
+ wmb();
+ *killer = 1;
}
static int
Then doing the following:
# echo 1 > /proc/sys/kernel/ftrace_dump_on_oops
# trace-cmd start -p function
# echo nmi_handle:cpudump > /debug/tracing/set_ftrace_filter
# perf record -c 100 -a sleep 1
And that triggers the crash.
-- Steve
^ permalink raw reply
* [PATCH v5 01/22] KVM: arm/arm64: Add ITS save/restore API documentation
From: Marc Zyngier @ 2017-04-27 16:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c3dd2ad2-b068-f158-10d7-53e27bd2b1bf@redhat.com>
On 27/04/17 16:29, Auger Eric wrote:
>
>
> On 27/04/2017 16:45, Christoffer Dall wrote:
>> Hi Eric,
>>
>> On Thu, Apr 27, 2017 at 02:51:00PM +0200, Auger Eric wrote:
>>> On 27/04/2017 13:02, Christoffer Dall wrote:
>>>> On Thu, Apr 27, 2017 at 11:33:39AM +0200, Auger Eric wrote:
>>>>> On 27/04/2017 10:57, Christoffer Dall wrote:
>>>>>> On Wed, Apr 26, 2017 at 05:48:32PM +0200, Auger Eric wrote:
>>>>>>> On 26/04/2017 14:31, Christoffer Dall wrote:
>>>>>>>> On Fri, Apr 14, 2017 at 12:15:13PM +0200, Eric Auger wrote:
>>>>>>>>> Add description for how to access ITS registers and how to save/restore
>>>>>>>>> ITS tables into/from memory.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> v4 -> v5:
>>>>>>>>> - take into account Christoffer's comments
>>>>>>>>> - pending table save on GICV3 side now
>>>>>>>>>
>>>>>>>>> v3 -> v4:
>>>>>>>>> - take into account Peter's comments:
>>>>>>>>> - typos
>>>>>>>>> - KVM_DEV_ARM_VGIC_GRP_ITS_TABLES kvm_device_attr = 0
>>>>>>>>> - add a validity bit in DTE
>>>>>>>>> - document all fields in CTE and ITE
>>>>>>>>> - document ABI revision
>>>>>>>>> - take into account Andre's comments:
>>>>>>>>> - document restrictions about GITS_CREADR writing and GITS_IIDR
>>>>>>>>> - document -EBUSY error if one or more VCPUS are runnning
>>>>>>>>> - document 64b registers only can be accessed with 64b access
>>>>>>>>> - itt_addr field matches bits [51:8] of the itt_addr
>>>>>>>>>
>>>>>>>>> v1 -> v2:
>>>>>>>>> - DTE and ITE now are 8 bytes
>>>>>>>>> - DTE and ITE now indexed by deviceid/eventid
>>>>>>>>> - use ITE name instead of ITTE
>>>>>>>>> - mentions ITT_addr matches bits [51:8] of the actual address
>>>>>>>>> - mentions LE layout
>>>>>>>>> ---
>>>>>>>>> Documentation/virtual/kvm/devices/arm-vgic-its.txt | 99 ++++++++++++++++++++++
>>>>>>>>> 1 file changed, 99 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>> index 6081a5b..b5f010d 100644
>>>>>>>>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>>>>>>>>> @@ -32,7 +32,106 @@ Groups:
>>>>>>>>> KVM_DEV_ARM_VGIC_CTRL_INIT
>>>>>>>>> request the initialization of the ITS, no additional parameter in
>>>>>>>>> kvm_device_attr.addr.
>>>>>>>>> +
>>>>>>>>> + KVM_DEV_ARM_ITS_SAVE_TABLES
>>>>>>>>> + save the ITS table data into guest RAM, at the location provisioned
>>>>>>>>> + by the guest in corresponding registers/table entries.
>>>>>>>>> +
>>>>>>>>> + The layout of the tables in guest memory defines an ABI. The entries
>>>>>>>>> + are laid out in little endian format as described in the last paragraph.
>>>>>>>>> +
>>>>>>>>> + KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>>> + restore the ITS tables from guest RAM to ITS internal structures.
>>>>>>>>> +
>>>>>>>>> + The GICV3 must be restored before the ITS and all ITS registers but
>>>>>>>>> + the GITS_CTLR must be restored before restoring the ITS tables.
>>>>>>>>> +
>>>>>>>>> + The GITS_IIDR read-only register must also be restored before
>>>>>>>>> + the table restore as the IIDR revision field encodes the ABI revision.
>>>>>>>>> +
>>>>>>>>
>>>>>>>> what is the expected sequence of operations. For example, to restore
>>>>>>>> the ITS, do I call KVM_DEV_ARM_VGIC_CTRL_INIT first, then restore all
>>>>>>>> the memory and registers, and finally call KVM_DEV_ARM_ITS_RESTORE_TABLES?
>>>>>>> Yes KVM_DEV_ARM_VGIC_CTRL_INIT comes first, then restore all registers
>>>>>>> except GITS_CTLR, then table restore, then GITS_CTLR
>>>>>>>>
>>>>>>>> Is there any interaction between when you call KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>>>>> and restore GITS_CTLR (which enables the ITS)?
>>>>>>>
>>>>>>> Yep, when GITS_CTLR is set, LPIs may be enabled and this on that event
>>>>>>> that the pending table is read. But the whole pending table is not read
>>>>>>> as we only iterate on registered LPIs. So the ITT must have been
>>>>>>> restored previously.
>>>>>>>
>>>>>>> I became aware that the pending table sync is done twice, once in the
>>>>>>> pending table restore, and once in the GITS_CTLR restore. So if we
>>>>>>> leave this order specification, I should be able to remove the sync on
>>>>>>> table restore. This was the original reason why GITS_CTLR restore has
>>>>>>> been done at the very end.
>>>>>>
>>>>>> I'm sorry, I'm a bit confused. Do we not need
>>>>>> KVM_DEV_ARM_ITS_RESTORE_TABLES at all then?
>>>>>
>>>>> Yes you do. I was talking about the RDIST pending table sync. The save
>>>>> is explicit using GICV3 device KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES.
>>>>> However the sync is implicit on GITS_CTLR restore if LPIs are enabled.
>>>>> and today I do it also on ITS device KVM_DEV_ARM_ITS_RESTORE_TABLES
>>>>> which is not requested I think since GITS_CTLR restore does it already.
>>>>
>>>> Shouldn't restoring the pending tables happen when restoring some
>>>> redeistributor state and not anything related to the ITS?
>>>
>>> Marc wrote:
>>> "
>>> I don't think you necessarily need a coarse map. When restoring the ITS
>>> tables, you can always read the pending bit when creating the LPI
>>> structure (it has been written to RAM at save time). Note that we
>>> already do something like this in vgic_enable_lpis().
>>> "
>>>
>>> This is currently what is implemented I think. the pending tables are
>>> currently sync'ed on GITS_CTLR set (if LPI are enabled) + erroneously
>>> also on on ITS table restore
>>>
>>> The problematic is: Either you know in advance which LPI INTIDare used
>>> or you need to parse the whole pending table (possibly using the 1st kB
>>> as coarse mapping).
>>>
>>> If you don't know the LPI INTIDs in advance it is only possible to
>>> restore the pending bit of pending LPIs. At that time you would
>>> re-allocate those pending LPI (vgic_add_lpi) and when you restore the
>>> ITS ITT you would do the same for those which were not pending. Looks
>>> really heavy to me: coarse mapping + dual vgic_add_lpi path.
>>>
>>> Otherwise we would need to add another dependency between RDIST pending
>>> table restore and ITS table restore but this looks even more weird, no?
>>>
>>>
>> So I just sat down with Andre and Marc and we tried to work through this
>> and came up with the best scheme. I apologize in advance for the
>> one-way nature of this e-mail, and I am of course open to discussing the
>> following proposal again if you do not agree.
>>
>> What I think this document should say, is that the following ordering
>> must be followed when restoring the GIC and the ITS:
>>
>> First, restore all guest memory
>>
>> Second, restore ALL redistributors
>>
>> Third, restore the ITS, in the following order:
>> 1. Initialize the ITS (KVM_DEV_ARM_VGIC_CTRL_INIT)
>> 2. Restore GITS_CBASER
>> 3. Restore all other GITS_ registers, except GITS_CTLR!
>> 4. Load the ITS table data (KVM_DEV_ARM_ITS_RESTORE_TABLES)
>> 5. Restore GITS_CTLR
>>
>> The rationale is that we really want the redistributor and the ITS
>> restore to be independent and follow the architecture. This means that
>> our ABI for the redistributor should still work without restoring an ITS
>> (if we ever decide to support LPIs for KVM without the ITS).
>
> OK. Note I already mentioned that GICv3 must be restored before the ITS.
> To me this comprised the RDIST.
>
> I understand the above description of the ordering comes in addition to
> the existing text, right? in other words I keep the GITS_READR,
> GITS_IIDR specific text as well as KVM_DEV_ARM_ITS_SAVE/RESTORE_TABLES
> section.
>
>>
>> In terms of our current implementation this means that vgic_add_lpi()
>> should ask the redistributor what the state of the LPI is (priority,
>> enabled, pending).
> this practically means I move update_lpi_config call from
> vgic_its_restore_ite to vgic_add_lpi(). OK
>
> However for getting the LPI pending state I must know which RDIST the
> LPI is attached to. This is not known at LPI allocation time. Do I
> misunderstand something?
Once you have rebuilt the ITS data structures and allocated the IRQ
structures, you should have a target_cpu field pointing to the right
vcpu. From there, you can surely find the corresponding redistributor
and the pending table.
BTW, we should document the fact that vcpus must have been created
before reloading the ITS (that's not completely obvious).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ 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