| Message ID | 20161221064603.11830-6-fu.wei@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Dec 21, 2016 at 02:45:53PM +0800, fu.wei@linaro.org wrote: [...] > - if (is_hyp_mode_available() || !arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) { > - bool has_ppi; > + if (is_hyp_mode_available() && is_kernel_in_hyp_mode()) > + return ARCH_TIMER_HYP_PPI; > > - if (is_kernel_in_hyp_mode()) { > - arch_timer_uses_ppi = ARCH_TIMER_HYP_PPI; > - has_ppi = !!arch_timer_ppi[ARCH_TIMER_HYP_PPI]; > - } else { > - arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI; > - has_ppi = (!!arch_timer_ppi[ARCH_TIMER_PHYS_SECURE_PPI] || > - !!arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]); > - } > + if (arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) > + return ARCH_TIMER_VIRT_PPI; > > - if (!has_ppi) { > - pr_warn("No interrupt available, giving up\n"); > - return -EINVAL; > - } > - } > + if (IS_ENABLED(CONFIG_ARM64)) > + return ARCH_TIMER_PHYS_NONSECURE_PPI; > + > + return ARCH_TIMER_PHYS_SECURE_PPI; For a 32-bit platform booted at hyp (with a virt PPI available), the new logic will select ARCH_TIMER_VIRT_PPI. I beleive that will break KVM. I think the logic should be: if (is_kernel_in_hyp_mode()) return ARCH_TIMER_HYP_PPI; if (!is_hyp_mode_available() && arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) return ARCH_TIMER_VIRT_PPI; if (IS_ENABLED(CONFIG_ARM64)) return ARCH_TIMER_PHYS_NONSECURE_PPI; return ARCH_TIMER_PHYS_SECURE_PPI; Please use that instead (keeping the comment you retained). > +static int __init arch_timer_init(void) > +{ > + int ret; > > ret = arch_timer_register(); > if (ret) > @@ -904,6 +906,13 @@ static int __init arch_timer_of_init(struct device_node *np) > if (IS_ENABLED(CONFIG_ARM) && > of_property_read_bool(np, "arm,cpu-registers-not-fw-configured")) > arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI; > + else > + arch_timer_uses_ppi = arch_timer_select_ppi(); > + > + if (!arch_timer_ppi[arch_timer_uses_ppi]) { > + pr_err("No interrupt available, giving up\n"); > + return -EINVAL; > + } > > /* On some systems, the counter stops ticking when in suspend. */ > arch_counter_suspend_stop = of_property_read_bool(np, > @@ -1049,6 +1058,12 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table) > /* Get the frequency from CNTFRQ */ > arch_timer_detect_rate(NULL, NULL); > > + arch_timer_uses_ppi = arch_timer_select_ppi(); > + if (!arch_timer_ppi[arch_timer_uses_ppi]) { > + pr_err("No interrupt available, giving up\n"); > + return -EINVAL; > + } I see that we have to duplicate this so we can special-case the DT-specific behaviour, so that's fine by me. If you can fix the arch_timer_select_ppi() logic as above, this should be fine. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On 17 January 2017 at 01:29, Mark Rutland <mark.rutland@arm.com> wrote: > On Wed, Dec 21, 2016 at 02:45:53PM +0800, fu.wei@linaro.org wrote: > [...] > >> - if (is_hyp_mode_available() || !arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) { >> - bool has_ppi; >> + if (is_hyp_mode_available() && is_kernel_in_hyp_mode()) >> + return ARCH_TIMER_HYP_PPI; >> >> - if (is_kernel_in_hyp_mode()) { >> - arch_timer_uses_ppi = ARCH_TIMER_HYP_PPI; >> - has_ppi = !!arch_timer_ppi[ARCH_TIMER_HYP_PPI]; >> - } else { >> - arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI; >> - has_ppi = (!!arch_timer_ppi[ARCH_TIMER_PHYS_SECURE_PPI] || >> - !!arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]); >> - } >> + if (arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) >> + return ARCH_TIMER_VIRT_PPI; >> >> - if (!has_ppi) { >> - pr_warn("No interrupt available, giving up\n"); >> - return -EINVAL; >> - } >> - } >> + if (IS_ENABLED(CONFIG_ARM64)) >> + return ARCH_TIMER_PHYS_NONSECURE_PPI; >> + >> + return ARCH_TIMER_PHYS_SECURE_PPI; > > For a 32-bit platform booted at hyp (with a virt PPI available), the new > logic will select ARCH_TIMER_VIRT_PPI. I beleive that will break KVM. > > I think the logic should be: > > if (is_kernel_in_hyp_mode()) > return ARCH_TIMER_HYP_PPI; > > if (!is_hyp_mode_available() && > arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) > return ARCH_TIMER_VIRT_PPI; > > if (IS_ENABLED(CONFIG_ARM64)) > return ARCH_TIMER_PHYS_NONSECURE_PPI; > > return ARCH_TIMER_PHYS_SECURE_PPI; > > Please use that instead (keeping the comment you retained). Great thanks for pointing it out, that is bug. also got this bug report from Huawei engineer. I have fixed it using your example code, thanks! > >> +static int __init arch_timer_init(void) >> +{ >> + int ret; >> >> ret = arch_timer_register(); >> if (ret) >> @@ -904,6 +906,13 @@ static int __init arch_timer_of_init(struct device_node *np) >> if (IS_ENABLED(CONFIG_ARM) && >> of_property_read_bool(np, "arm,cpu-registers-not-fw-configured")) >> arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI; >> + else >> + arch_timer_uses_ppi = arch_timer_select_ppi(); >> + >> + if (!arch_timer_ppi[arch_timer_uses_ppi]) { >> + pr_err("No interrupt available, giving up\n"); >> + return -EINVAL; >> + } >> >> /* On some systems, the counter stops ticking when in suspend. */ >> arch_counter_suspend_stop = of_property_read_bool(np, >> @@ -1049,6 +1058,12 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table) >> /* Get the frequency from CNTFRQ */ >> arch_timer_detect_rate(NULL, NULL); >> >> + arch_timer_uses_ppi = arch_timer_select_ppi(); >> + if (!arch_timer_ppi[arch_timer_uses_ppi]) { >> + pr_err("No interrupt available, giving up\n"); >> + return -EINVAL; >> + } > > I see that we have to duplicate this so we can special-case the > DT-specific behaviour, so that's fine by me. Yes, that is the reason of the duplication :-) > > If you can fix the arch_timer_select_ppi() logic as above, this should > be fine. Done, thanks :-) > > Thanks, > Mark.
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index c7b4e50..c7b4482 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -702,7 +702,7 @@ static int __init arch_timer_register(void) case ARCH_TIMER_PHYS_NONSECURE_PPI: err = request_percpu_irq(ppi, arch_timer_handler_phys, "arch_timer", arch_timer_evt); - if (!err && arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]) { + if (!err && arch_timer_has_nonsecure_ppi()) { ppi = arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]; err = request_percpu_irq(ppi, arch_timer_handler_phys, "arch_timer", arch_timer_evt); @@ -824,39 +824,41 @@ static int __init arch_timer_common_init(void) return arch_timer_arch_init(); } -static int __init arch_timer_init(void) +/** + * arch_timer_select_ppi() - Select suitable PPI for the current system. + * + * If HYP mode is available, we know that the physical timer + * has been configured to be accessible from PL1. Use it, so + * that a guest can use the virtual timer instead. + * + * On ARMv8.1 with VH extensions, the kernel runs in HYP. VHE + * accesses to CNTP_*_EL1 registers are silently redirected to + * their CNTHP_*_EL2 counterparts, and use a different PPI + * number. + * + * If no interrupt provided for virtual timer, we'll have to + * stick to the physical timer. It'd better be accessible... + * For arm64 we never use the secure interrupt. + * + * Return: a suitable PPI type for the current system. + */ +static enum arch_timer_ppi_nr __init arch_timer_select_ppi(void) { - int ret; - /* - * If HYP mode is available, we know that the physical timer - * has been configured to be accessible from PL1. Use it, so - * that a guest can use the virtual timer instead. - * - * If no interrupt provided for virtual timer, we'll have to - * stick to the physical timer. It'd better be accessible... - * - * On ARMv8.1 with VH extensions, the kernel runs in HYP. VHE - * accesses to CNTP_*_EL1 registers are silently redirected to - * their CNTHP_*_EL2 counterparts, and use a different PPI - * number. - */ - if (is_hyp_mode_available() || !arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) { - bool has_ppi; + if (is_hyp_mode_available() && is_kernel_in_hyp_mode()) + return ARCH_TIMER_HYP_PPI; - if (is_kernel_in_hyp_mode()) { - arch_timer_uses_ppi = ARCH_TIMER_HYP_PPI; - has_ppi = !!arch_timer_ppi[ARCH_TIMER_HYP_PPI]; - } else { - arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI; - has_ppi = (!!arch_timer_ppi[ARCH_TIMER_PHYS_SECURE_PPI] || - !!arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI]); - } + if (arch_timer_ppi[ARCH_TIMER_VIRT_PPI]) + return ARCH_TIMER_VIRT_PPI; - if (!has_ppi) { - pr_warn("No interrupt available, giving up\n"); - return -EINVAL; - } - } + if (IS_ENABLED(CONFIG_ARM64)) + return ARCH_TIMER_PHYS_NONSECURE_PPI; + + return ARCH_TIMER_PHYS_SECURE_PPI; +} + +static int __init arch_timer_init(void) +{ + int ret; ret = arch_timer_register(); if (ret) @@ -904,6 +906,13 @@ static int __init arch_timer_of_init(struct device_node *np) if (IS_ENABLED(CONFIG_ARM) && of_property_read_bool(np, "arm,cpu-registers-not-fw-configured")) arch_timer_uses_ppi = ARCH_TIMER_PHYS_SECURE_PPI; + else + arch_timer_uses_ppi = arch_timer_select_ppi(); + + if (!arch_timer_ppi[arch_timer_uses_ppi]) { + pr_err("No interrupt available, giving up\n"); + return -EINVAL; + } /* On some systems, the counter stops ticking when in suspend. */ arch_counter_suspend_stop = of_property_read_bool(np, @@ -1049,6 +1058,12 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table) /* Get the frequency from CNTFRQ */ arch_timer_detect_rate(NULL, NULL); + arch_timer_uses_ppi = arch_timer_select_ppi(); + if (!arch_timer_ppi[arch_timer_uses_ppi]) { + pr_err("No interrupt available, giving up\n"); + return -EINVAL; + } + /* Always-on capability */ arch_timer_c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);