linux-kernel - Re: [PATCH v8 10/18] RISC-V: Init and Halt Code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <799b75ef-f512-3182-bc6d-cc79afedf282@linaro.org>
Date:   Wed, 13 Sep 2017 20:15:12 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Palmer Dabbelt <palmer@...belt.com>, peterz@...radead.org,
        tglx@...utronix.de, jason@...edaemon.net, marc.zyngier@....com,
        Arnd Bergmann <arnd@...db.de>, dmitriy@...-tech.org
Cc:     yamada.masahiro@...ionext.com, mmarek@...e.com, albert@...ive.com,
        will.deacon@....com, boqun.feng@...il.com, oleg@...hat.com,
        mingo@...hat.com, gregkh@...uxfoundation.org, jslaby@...e.com,
        davem@...emloft.net, mchehab@...nel.org, hverkuil@...all.nl,
        rdunlap@...radead.org, viro@...iv.linux.org.uk,
        mhiramat@...nel.org, fweisbec@...il.com, mcgrof@...nel.org,
        dledford@...hat.com, bart.vanassche@...disk.com,
        sstabellini@...nel.org, mpe@...erman.id.au,
        rmk+kernel@...linux.org.uk, paul.gortmaker@...driver.com,
        nicolas.dichtel@...nd.com, linux@...ck-us.net,
        heiko.carstens@...ibm.com, schwidefsky@...ibm.com,
        geert@...ux-m68k.org, akpm@...ux-foundation.org,
        andriy.shevchenko@...ux.intel.com, jiri@...lanox.com,
        vgupta@...opsys.com, airlied@...hat.com, jk@...abs.org,
        chris@...is-wilson.co.uk, Jason@...c4.com,
        paulmck@...ux.vnet.ibm.com, ncardwell@...gle.com,
        linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
        patches@...ups.riscv.org
Subject: Re: [PATCH v8 10/18] RISC-V: Init and Halt Code

On 12/09/2017 23:57, Palmer Dabbelt wrote:
> This contains the various __init C functions, the initial assembly
> kernel entry point, and the code to reset the system.  When a file was
> init-related this patch contains the entire file.
> 
> Signed-off-by: Palmer Dabbelt <palmer@...belt.com>
> ---

[ ... ]

> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
> new file mode 100644
> index 000000000000..8362a0e17a1a
> --- /dev/null
> +++ b/arch/riscv/kernel/time.c
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2017 SiFive
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of the GNU General Public License
> + *   as published by the Free Software Foundation, version 2.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + */
> +
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/delay.h>
> +#include <linux/timer_riscv.h>
> +
> +#include <asm/sbi.h>
> +
> +unsigned long riscv_timebase;
> +
> +DECLARE_PER_CPU(struct clock_event_device, riscv_clock_event);
> +
> +static int next_event(unsigned long delta, struct clock_event_device *ce)
> +{
> +	/*
> +	 * time_init() allocates a timer for each CPU.  Since we're writing the
> +	 * timer comparison register here we can't allow the timers to cross
> +	 * harts.
> +	 */
> +	BUG_ON(ce != this_cpu_ptr(&riscv_clock_event));
> +	sbi_set_timer(get_cycles64() + delta);
> +	return 0;
> +}
> +
> +static unsigned long long rdtime(struct clocksource *cs)
> +{
> +	/*
> +	 * It's guarnteed that all the timers across all the harts are
> +	 * synchronized within one tick of each other, so while this could
> +	 * technically go backwards when hopping between CPUs, practically it
> +	 * won't happen.
> +	 */
> +	return get_cycles64();
> +}
> +
> +void riscv_timer_interrupt(void)
> +{
> +	struct clock_event_device *evdev = this_cpu_ptr(&riscv_clock_event);
> +
> +	evdev->event_handler(evdev);
> +}
> +
> +void __init init_clockevent(void)
> +{
> +	int cpu_id = smp_processor_id();
> +
> +	timer_riscv_init(cpu_id, riscv_timebase, &next_event);
> +	csr_set(sie, SIE_STIE);
> +}
> +
> +void __init time_init(void)
> +{
> +	struct device_node *cpu;
> +	u32 prop;
> +
> +	cpu = of_find_node_by_path("/cpus");
> +	if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop))
> +		panic(KERN_WARNING "RISC-V system with no 'timebase-frequency' in DTS\n");
> +	riscv_timebase = prop;
> +
> +	lpj_fine = riscv_timebase / HZ;
> +
> +	clocksource_riscv_init(&rdtime);
> +	init_clockevent();
> +}

All this code must go in the timer side and use the TIMER_OF_DECLARE
macro with the proper wrappers.

Where is the request_per_cpu_interrupt()?

What is this riscv_timer_interrupt() signature?

Where is get_cycles64() ?

The timer driver should be self-contained and not spread across
different places, it is very difficult to review it.

[ ... ]

-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog