From: Boqun Feng <boqun.feng@gmail.com>
To: Michael Kelley <mikelley@microsoft.com>
Cc: sthemmin@microsoft.com, kys@microsoft.com, wei.liu@kernel.org,
tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
hpa@zytor.com, daniel.lezcano@linaro.org, arnd@arndb.de,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
x86@kernel.org, linux-arch@vger.kernel.org
Subject: Re: [PATCH 10/10] clocksource/drivers/hyper-v: Move handling of STIMER0 interrupts
Date: Tue, 23 Feb 2021 14:47:10 +0800 [thread overview]
Message-ID: <YDSk7scPgUZdwyMd@boqun-archlinux> (raw)
In-Reply-To: <1611779025-21503-11-git-send-email-mikelley@microsoft.com>
On Wed, Jan 27, 2021 at 12:23:45PM -0800, Michael Kelley wrote:
> STIMER0 interrupts are most naturally modeled as per-cpu IRQs. But
> because x86/x64 doesn't have per-cpu IRQs, the core STIMER0 interrupt
> handling machinery is done in code under arch/x86 and Linux IRQs are
> not used. Adding support for ARM64 means adding equivalent code
> using per-cpu IRQs under arch/arm64.
>
> A better model is to treat per-cpu IRQs as the normal path (which it is
> for modern architectures), and the x86/x64 path as the exception. Do this
> by incorporating standard Linux per-cpu IRQ allocation into the main
> SITMER0 driver code, and bypass it in the x86/x64 exception case. For
> x86/x64, special case code is retained under arch/x86, but no STIMER0
> interrupt handling code is needed under arch/arm64.
>
> No functional change.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
> arch/x86/hyperv/hv_init.c | 2 +-
> arch/x86/include/asm/mshyperv.h | 4 -
> arch/x86/kernel/cpu/mshyperv.c | 10 +--
> drivers/clocksource/hyperv_timer.c | 170 +++++++++++++++++++++++++------------
> include/asm-generic/mshyperv.h | 5 --
> include/clocksource/hyperv_timer.h | 3 +-
> 6 files changed, 123 insertions(+), 71 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 22e9557..fe37546 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -371,7 +371,7 @@ void __init hyperv_init(void)
> * Ignore any errors in setting up stimer clockevents
> * as we can run with the LAPIC timer as a fallback.
> */
> - (void)hv_stimer_alloc();
> + (void)hv_stimer_alloc(false);
>
> hv_apic_init();
>
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 5ccbba8..941dd55 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -31,10 +31,6 @@ static inline u64 hv_get_register(unsigned int reg)
>
> void hyperv_vector_handler(struct pt_regs *regs);
>
> -static inline void hv_enable_stimer0_percpu_irq(int irq) {}
> -static inline void hv_disable_stimer0_percpu_irq(int irq) {}
> -
> -
> #if IS_ENABLED(CONFIG_HYPERV)
> extern void *hv_hypercall_pg;
> extern void __percpu **hyperv_pcpu_input_arg;
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 5679100a1..440507e 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -85,21 +85,17 @@ void hv_remove_vmbus_handler(void)
> set_irq_regs(old_regs);
> }
>
> -int hv_setup_stimer0_irq(int *irq, int *vector, void (*handler)(void))
> +/* For x86/x64, override weak placeholders in hyperv_timer.c */
> +void hv_setup_stimer0_handler(void (*handler)(void))
> {
> - *vector = HYPERV_STIMER0_VECTOR;
> - *irq = -1; /* Unused on x86/x64 */
> hv_stimer0_handler = handler;
> - return 0;
> }
> -EXPORT_SYMBOL_GPL(hv_setup_stimer0_irq);
>
> -void hv_remove_stimer0_irq(int irq)
> +void hv_remove_stimer0_handler(void)
> {
> /* We have no way to deallocate the interrupt gate */
> hv_stimer0_handler = NULL;
> }
> -EXPORT_SYMBOL_GPL(hv_remove_stimer0_irq);
>
> void hv_setup_kexec_handler(void (*handler)(void))
> {
> diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c
> index edf2d43..c553b8c 100644
> --- a/drivers/clocksource/hyperv_timer.c
> +++ b/drivers/clocksource/hyperv_timer.c
> @@ -18,6 +18,9 @@
> #include <linux/sched_clock.h>
> #include <linux/mm.h>
> #include <linux/cpuhotplug.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/acpi.h>
> #include <clocksource/hyperv_timer.h>
> #include <asm/hyperv-tlfs.h>
> #include <asm/mshyperv.h>
> @@ -43,14 +46,13 @@
> */
> static bool direct_mode_enabled;
>
> -static int stimer0_irq;
> -static int stimer0_vector;
> +static int stimer0_irq = -1;
> +static long __percpu *stimer0_evt;
> static int stimer0_message_sint;
>
> /*
> - * ISR for when stimer0 is operating in Direct Mode. Direct Mode
> - * does not use VMbus or any VMbus messages, so process here and not
> - * in the VMbus driver code.
> + * Common code for stimer0 interrupts coming via Direct Mode or
> + * as a VMbus message.
> */
> void hv_stimer0_isr(void)
> {
> @@ -61,6 +63,16 @@ void hv_stimer0_isr(void)
> }
> EXPORT_SYMBOL_GPL(hv_stimer0_isr);
>
> +/*
> + * stimer0 interrupt handler for architectures that support
> + * per-cpu interrupts, which also implies Direct Mode.
> + */
> +static irqreturn_t hv_stimer0_percpu_isr(int irq, void *dev_id)
> +{
> + hv_stimer0_isr();
> + return IRQ_HANDLED;
> +}
> +
> static int hv_ce_set_next_event(unsigned long delta,
> struct clock_event_device *evt)
> {
> @@ -76,8 +88,8 @@ static int hv_ce_shutdown(struct clock_event_device *evt)
> {
> hv_set_register(HV_REGISTER_STIMER0_COUNT, 0);
> hv_set_register(HV_REGISTER_STIMER0_CONFIG, 0);
> - if (direct_mode_enabled)
> - hv_disable_stimer0_percpu_irq(stimer0_irq);
> + if (direct_mode_enabled && stimer0_irq >= 0)
> + disable_percpu_irq(stimer0_irq);
>
> return 0;
> }
> @@ -95,8 +107,9 @@ static int hv_ce_set_oneshot(struct clock_event_device *evt)
> * on the specified hardware vector/IRQ.
> */
> timer_cfg.direct_mode = 1;
> - timer_cfg.apic_vector = stimer0_vector;
> - hv_enable_stimer0_percpu_irq(stimer0_irq);
> + timer_cfg.apic_vector = HYPERV_STIMER0_VECTOR;
> + if (stimer0_irq >= 0)
> + enable_percpu_irq(stimer0_irq, IRQ_TYPE_NONE);
> } else {
> /*
> * When it expires, the timer will generate a VMbus message,
> @@ -169,10 +182,67 @@ int hv_stimer_cleanup(unsigned int cpu)
> }
> EXPORT_SYMBOL_GPL(hv_stimer_cleanup);
>
> +/*
> + * These placeholders are overridden by arch specific code on
> + * architectures that need special setup of the stimer0 IRQ because
> + * they don't support per-cpu IRQs (such as x86/x64).
> + */
> +void __weak hv_setup_stimer0_handler(void (*handler)(void))
> +{
> +};
> +
> +void __weak hv_remove_stimer0_handler(void)
> +{
> +};
> +
> +static int hv_setup_stimer0_irq(void)
> +{
> + int ret;
> +
> + ret = acpi_register_gsi(NULL, HYPERV_STIMER0_VECTOR,
> + ACPI_EDGE_SENSITIVE, ACPI_ACTIVE_HIGH);
> + if (ret < 0) {
> + pr_err("Can't register Hyper-V stimer0 GSI. Error %d", ret);
> + return ret;
> + }
> + stimer0_irq = ret;
> +
> + stimer0_evt = alloc_percpu(long);
> + if (!stimer0_evt) {
> + ret = -ENOMEM;
> + goto unregister_gsi;
> + }
> + ret = request_percpu_irq(stimer0_irq, hv_stimer0_percpu_isr,
> + "Hyper-V stimer0", stimer0_evt);
> + if (ret) {
> + pr_err("Can't request Hyper-V stimer0 IRQ %d. Error %d",
> + stimer0_irq, ret);
> + goto free_stimer0_evt;
> + }
> + return ret;
> +
> +free_stimer0_evt:
> + free_percpu(stimer0_evt);
> +unregister_gsi:
> + acpi_unregister_gsi(stimer0_irq);
> + stimer0_irq = -1;
> + return ret;
> +}
> +
> +static void hv_remove_stimer0_irq(void)
> +{
> + if (stimer0_irq != -1) {
> + free_percpu_irq(stimer0_irq, stimer0_evt);
> + free_percpu(stimer0_evt);
> + acpi_unregister_gsi(stimer0_irq);
> + stimer0_irq = -1;
> + }
I think we need:
else {
hv_remove_stimer0_handler();
}
here?
Because previously, on x86 we set hv_stimer0_handler to NULL in
hv_remove_stimer0_irq(), however, this patch doesn't keep this behavior
any more.
Thoughts?
Regards,
Boqun
> +}
> +
> /* hv_stimer_alloc - Global initialization of the clockevent and stimer0 */
> -int hv_stimer_alloc(void)
> +int hv_stimer_alloc(bool have_percpu_irqs)
> {
> - int ret = 0;
> + int ret;
>
> /*
> * Synthetic timers are always available except on old versions of
> @@ -188,29 +258,37 @@ int hv_stimer_alloc(void)
>
> direct_mode_enabled = ms_hyperv.misc_features &
> HV_STIMER_DIRECT_MODE_AVAILABLE;
> - if (direct_mode_enabled) {
> - ret = hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
> - hv_stimer0_isr);
> +
> + /*
> + * If Direct Mode isn't enabled, the remainder of the initialization
> + * is done later by hv_stimer_legacy_init()
> + */
> + if (!direct_mode_enabled)
> + return 0;
> +
> + if (have_percpu_irqs) {
> + ret = hv_setup_stimer0_irq();
> if (ret)
> - goto free_percpu;
> + goto free_clock_event;
> + } else {
> + hv_setup_stimer0_handler(hv_stimer0_isr);
> + }
>
> - /*
> - * Since we are in Direct Mode, stimer initialization
> - * can be done now with a CPUHP value in the same range
> - * as other clockevent devices.
> - */
> - ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
> - "clockevents/hyperv/stimer:starting",
> - hv_stimer_init, hv_stimer_cleanup);
> - if (ret < 0)
> - goto free_stimer0_irq;
> + /*
> + * Since we are in Direct Mode, stimer initialization
> + * can be done now with a CPUHP value in the same range
> + * as other clockevent devices.
> + */
> + ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
> + "clockevents/hyperv/stimer:starting",
> + hv_stimer_init, hv_stimer_cleanup);
> + if (ret < 0) {
> + hv_remove_stimer0_irq();
> + goto free_clock_event;
> }
> return ret;
>
> -free_stimer0_irq:
> - hv_remove_stimer0_irq(stimer0_irq);
> - stimer0_irq = 0;
> -free_percpu:
> +free_clock_event:
> free_percpu(hv_clock_event);
> hv_clock_event = NULL;
> return ret;
> @@ -254,23 +332,6 @@ void hv_stimer_legacy_cleanup(unsigned int cpu)
> }
> EXPORT_SYMBOL_GPL(hv_stimer_legacy_cleanup);
>
> -
> -/* hv_stimer_free - Free global resources allocated by hv_stimer_alloc() */
> -void hv_stimer_free(void)
> -{
> - if (!hv_clock_event)
> - return;
> -
> - if (direct_mode_enabled) {
> - cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);
> - hv_remove_stimer0_irq(stimer0_irq);
> - stimer0_irq = 0;
> - }
> - free_percpu(hv_clock_event);
> - hv_clock_event = NULL;
> -}
> -EXPORT_SYMBOL_GPL(hv_stimer_free);
> -
> /*
> * Do a global cleanup of clockevents for the cases of kexec and
> * vmbus exit
> @@ -287,12 +348,17 @@ void hv_stimer_global_cleanup(void)
> hv_stimer_legacy_cleanup(cpu);
> }
>
> - /*
> - * If Direct Mode is enabled, the cpuhp teardown callback
> - * (hv_stimer_cleanup) will be run on all CPUs to stop the
> - * stimers.
> - */
> - hv_stimer_free();
> + if (!hv_clock_event)
> + return;
> +
> + if (direct_mode_enabled) {
> + cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);
> + hv_remove_stimer0_irq();
> + stimer0_irq = -1;
> + }
> + free_percpu(hv_clock_event);
> + hv_clock_event = NULL;
> +
> }
> EXPORT_SYMBOL_GPL(hv_stimer_global_cleanup);
>
> diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
> index 9f4089b..c271870 100644
> --- a/include/asm-generic/mshyperv.h
> +++ b/include/asm-generic/mshyperv.h
> @@ -178,9 +178,4 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
> static inline void hyperv_cleanup(void) {}
> #endif /* CONFIG_HYPERV */
>
> -#if IS_ENABLED(CONFIG_HYPERV)
> -extern int hv_setup_stimer0_irq(int *irq, int *vector, void (*handler)(void));
> -extern void hv_remove_stimer0_irq(int irq);
> -#endif
> -
> #endif
> diff --git a/include/clocksource/hyperv_timer.h b/include/clocksource/hyperv_timer.h
> index 34eef083..b6774aa 100644
> --- a/include/clocksource/hyperv_timer.h
> +++ b/include/clocksource/hyperv_timer.h
> @@ -21,8 +21,7 @@
> #define HV_MIN_DELTA_TICKS 1
>
> /* Routines called by the VMbus driver */
> -extern int hv_stimer_alloc(void);
> -extern void hv_stimer_free(void);
> +extern int hv_stimer_alloc(bool have_percpu_irqs);
> extern int hv_stimer_cleanup(unsigned int cpu);
> extern void hv_stimer_legacy_init(unsigned int cpu, int sint);
> extern void hv_stimer_legacy_cleanup(unsigned int cpu);
> --
> 1.8.3.1
>
next prev parent reply other threads:[~2021-02-23 6:48 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-27 20:23 [PATCH 00/10] Refactor arch specific Hyper-V code Michael Kelley
2021-01-27 20:23 ` [PATCH 01/10] Drivers: hv: vmbus: Move Hyper-V page allocator to arch neutral code Michael Kelley
2021-02-22 3:04 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 02/10] x86/hyper-v: Move hv_message_type to architecture neutral module Michael Kelley
2021-02-22 3:19 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 03/10] Drivers: hv: Redo Hyper-V synthetic MSR get/set functions Michael Kelley
2021-02-22 3:25 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 04/10] Drivers: hv: vmbus: Move hyperv_report_panic_msg to arch neutral code Michael Kelley
2021-02-22 3:27 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 05/10] Drivers: hv: vmbus: Handle auto EOI quirk inline Michael Kelley
2021-02-22 3:30 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 06/10] Drivers: hv: vmbus: Move handling of VMbus interrupts Michael Kelley
2021-02-22 3:54 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 07/10] clocksource/drivers/hyper-v: Handle vDSO differences inline Michael Kelley
2021-02-22 4:07 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 08/10] clocksource/drivers/hyper-v: Handle sched_clock " Michael Kelley
2021-02-01 18:55 ` Wei Liu
2021-02-04 16:28 ` Michael Kelley
2021-02-04 16:31 ` Wei Liu
2021-02-22 15:17 ` Boqun Feng
2021-01-27 20:23 ` [PATCH 09/10] clocksource/drivers/hyper-v: Set clocksource rating based on Hyper-V feature Michael Kelley
2021-02-22 16:01 ` Boqun Feng
2021-02-22 22:48 ` Michael Kelley
2021-01-27 20:23 ` [PATCH 10/10] clocksource/drivers/hyper-v: Move handling of STIMER0 interrupts Michael Kelley
2021-02-01 19:53 ` Wei Liu
2021-02-04 16:30 ` Michael Kelley
2021-02-23 6:47 ` Boqun Feng [this message]
2021-02-25 18:56 ` Michael Kelley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YDSk7scPgUZdwyMd@boqun-archlinux \
--to=boqun.feng@gmail.com \
--cc=arnd@arndb.de \
--cc=bp@alien8.de \
--cc=daniel.lezcano@linaro.org \
--cc=hpa@zytor.com \
--cc=kys@microsoft.com \
--cc=linux-arch@vger.kernel.org \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mikelley@microsoft.com \
--cc=mingo@redhat.com \
--cc=sthemmin@microsoft.com \
--cc=tglx@linutronix.de \
--cc=wei.liu@kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).