[PATCH v2] arm64: acpi/gtdt: validate CNTFRQ after having enabled the frame

[PATCH v2] arm64: acpi/gtdt: validate CNTFRQ after having enabled the frame

[Ard Biesheuvel]

The ACPI GTDT code validates the CNTFRQ field of each MMIO timer
frame against the CNTFRQ system register of the current CPU, to
ensure that they are equal, which is mandated by the architecture.

However, reading the CNTFRQ field of a frame is not possible until
the RFRQ bit in the frame's CNTACRn register is set, and doing so
before that willl produce the following error:

  arch_timer: [Firmware Bug]: CNTFRQ mismatch: frame @ 0x00000000e0be0000: (0x00000000), CPU: (0x0ee6b280)
  arch_timer: Disabling MMIO timers due to CNTFRQ mismatch
  arch_timer: Failed to initialize memory-mapped timer.

The reason is that the CNTFRQ field is RES0 if access is not enabled.

So move the validation of CNTFRQ into the loop that iterates over the
timers to find the best frame, and disregard the frame if its CNTFRQ
field differs from the CPU timer's CNTFRQ value. Even though the
architecture suggests that the CNTFRQ field of each frame should simply
reflect the CNTFRQ value of the base frame, in reality we cannot rule
out the possibility that there is another frame available that does
have the correct value, and so it makes sense to inspect any remaining
frames in this case.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 47 +++++++-----------------------------
 1 file changed, 9 insertions(+), 38 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index aae87c4c546e..c16fa694a47b 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1217,7 +1217,8 @@ arch_timer_mem_frame_get_cntfrq(struct arch_timer_mem_frame *frame)
 }
 
 static struct arch_timer_mem_frame * __init
-arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
+arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem,
+			       bool verify_freq)
 {
 	struct arch_timer_mem_frame *frame, *best_frame = NULL;
 	void __iomem *cntctlbase;
@@ -1259,6 +1260,11 @@ arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
 		if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
 			continue;
 
+		if (verify_freq &&
+		    arch_timer_mem_frame_get_cntfrq(frame) != arch_timer_rate) {
+			pr_err(FW_BUG "Ignoring MMIO timer frame with incorrect CNTFRQ\n");
+			continue;
+		}
 		best_frame = frame;
 	}
 
@@ -1366,7 +1372,7 @@ static int __init arch_timer_mem_of_init(struct device_node *np)
 		frame->valid = true;
 	}
 
-	frame = arch_timer_mem_find_best_frame(timer_mem);
+	frame = arch_timer_mem_find_best_frame(timer_mem, false);
 	if (!frame) {
 		ret = -EINVAL;
 		goto out;
@@ -1386,33 +1392,6 @@ TIMER_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
 		       arch_timer_mem_of_init);
 
 #ifdef CONFIG_ACPI_GTDT
-static int __init
-arch_timer_mem_verify_cntfrq(struct arch_timer_mem *timer_mem)
-{
-	struct arch_timer_mem_frame *frame;
-	u32 rate;
-	int i;
-
-	for (i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
-		frame = &timer_mem->frame[i];
-
-		if (!frame->valid)
-			continue;
-
-		rate = arch_timer_mem_frame_get_cntfrq(frame);
-		if (rate == arch_timer_rate)
-			continue;
-
-		pr_err(FW_BUG "CNTFRQ mismatch: frame @ %pa: (0x%08lx), CPU: (0x%08lx)\n",
-			&frame->cntbase,
-			(unsigned long)rate, (unsigned long)arch_timer_rate);
-
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int __init arch_timer_mem_acpi_init(int platform_timer_count)
 {
 	struct arch_timer_mem *timers, *timer;
@@ -1428,14 +1407,6 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
 	if (ret || !timer_count)
 		goto out;
 
-	for (i = 0; i < timer_count; i++) {
-		ret = arch_timer_mem_verify_cntfrq(&timers[i]);
-		if (ret) {
-			pr_err("Disabling MMIO timers due to CNTFRQ mismatch\n");
-			goto out;
-		}
-	}
-
 	/*
 	 * While unlikely, it's theoretically possible that none of the frames
 	 * in a timer expose the combination of feature we want.
@@ -1443,7 +1414,7 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
 	for (i = i; i < timer_count; i++) {
 		timer = &timers[i];
 
-		frame = arch_timer_mem_find_best_frame(timer);
+		frame = arch_timer_mem_find_best_frame(timer, true);
 		if (frame)
 			break;
 	}
-- 
2.11.0

[PATCH v2] arm64: acpi/gtdt: validate CNTFRQ after having enabled the frame

[Ard Biesheuvel]

On 21 August 2017 at 10:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The ACPI GTDT code validates the CNTFRQ field of each MMIO timer
> frame against the CNTFRQ system register of the current CPU, to
> ensure that they are equal, which is mandated by the architecture.
>
> However, reading the CNTFRQ field of a frame is not possible until
> the RFRQ bit in the frame's CNTACRn register is set, and doing so
> before that willl produce the following error:
>
>   arch_timer: [Firmware Bug]: CNTFRQ mismatch: frame @ 0x00000000e0be0000: (0x00000000), CPU: (0x0ee6b280)
>   arch_timer: Disabling MMIO timers due to CNTFRQ mismatch
>   arch_timer: Failed to initialize memory-mapped timer.
>
> The reason is that the CNTFRQ field is RES0 if access is not enabled.
>
> So move the validation of CNTFRQ into the loop that iterates over the
> timers to find the best frame, and disregard the frame if its CNTFRQ
> field differs from the CPU timer's CNTFRQ value. Even though the
> architecture suggests that the CNTFRQ field of each frame should simply
> reflect the CNTFRQ value of the base frame, in reality we cannot rule
> out the possibility that there is another frame available that does
> have the correct value, and so it makes sense to inspect any remaining
> frames in this case.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Ping?

> ---
>  drivers/clocksource/arm_arch_timer.c | 47 +++++++-----------------------------
>  1 file changed, 9 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index aae87c4c546e..c16fa694a47b 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1217,7 +1217,8 @@ arch_timer_mem_frame_get_cntfrq(struct arch_timer_mem_frame *frame)
>  }
>
>  static struct arch_timer_mem_frame * __init
> -arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
> +arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem,
> +                              bool verify_freq)
>  {
>         struct arch_timer_mem_frame *frame, *best_frame = NULL;
>         void __iomem *cntctlbase;
> @@ -1259,6 +1260,11 @@ arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
>                 if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
>                         continue;
>
> +               if (verify_freq &&
> +                   arch_timer_mem_frame_get_cntfrq(frame) != arch_timer_rate) {
> +                       pr_err(FW_BUG "Ignoring MMIO timer frame with incorrect CNTFRQ\n");
> +                       continue;
> +               }
>                 best_frame = frame;
>         }
>
> @@ -1366,7 +1372,7 @@ static int __init arch_timer_mem_of_init(struct device_node *np)
>                 frame->valid = true;
>         }
>
> -       frame = arch_timer_mem_find_best_frame(timer_mem);
> +       frame = arch_timer_mem_find_best_frame(timer_mem, false);
>         if (!frame) {
>                 ret = -EINVAL;
>                 goto out;
> @@ -1386,33 +1392,6 @@ TIMER_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
>                        arch_timer_mem_of_init);
>
>  #ifdef CONFIG_ACPI_GTDT
> -static int __init
> -arch_timer_mem_verify_cntfrq(struct arch_timer_mem *timer_mem)
> -{
> -       struct arch_timer_mem_frame *frame;
> -       u32 rate;
> -       int i;
> -
> -       for (i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
> -               frame = &timer_mem->frame[i];
> -
> -               if (!frame->valid)
> -                       continue;
> -
> -               rate = arch_timer_mem_frame_get_cntfrq(frame);
> -               if (rate == arch_timer_rate)
> -                       continue;
> -
> -               pr_err(FW_BUG "CNTFRQ mismatch: frame @ %pa: (0x%08lx), CPU: (0x%08lx)\n",
> -                       &frame->cntbase,
> -                       (unsigned long)rate, (unsigned long)arch_timer_rate);
> -
> -               return -EINVAL;
> -       }
> -
> -       return 0;
> -}
> -
>  static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>  {
>         struct arch_timer_mem *timers, *timer;
> @@ -1428,14 +1407,6 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>         if (ret || !timer_count)
>                 goto out;
>
> -       for (i = 0; i < timer_count; i++) {
> -               ret = arch_timer_mem_verify_cntfrq(&timers[i]);
> -               if (ret) {
> -                       pr_err("Disabling MMIO timers due to CNTFRQ mismatch\n");
> -                       goto out;
> -               }
> -       }
> -
>         /*
>          * While unlikely, it's theoretically possible that none of the frames
>          * in a timer expose the combination of feature we want.
> @@ -1443,7 +1414,7 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>         for (i = i; i < timer_count; i++) {
>                 timer = &timers[i];
>
> -               frame = arch_timer_mem_find_best_frame(timer);
> +               frame = arch_timer_mem_find_best_frame(timer, true);
>                 if (frame)
>                         break;
>         }
> --
> 2.11.0
>

[PATCH v2] arm64: acpi/gtdt: validate CNTFRQ after having enabled the frame

[Mark Rutland]

Hi Ard,

Thanks for respinning this, and apologies for the delay.

On Mon, Aug 21, 2017 at 10:56:23AM +0100, Ard Biesheuvel wrote:
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index aae87c4c546e..c16fa694a47b 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -1217,7 +1217,8 @@ arch_timer_mem_frame_get_cntfrq(struct arch_timer_mem_frame *frame)
>  }
>  
>  static struct arch_timer_mem_frame * __init
> -arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
> +arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem,
> +			       bool verify_freq)
>  {
>  	struct arch_timer_mem_frame *frame, *best_frame = NULL;
>  	void __iomem *cntctlbase;
> @@ -1259,6 +1260,11 @@ arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
>  		if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
>  			continue;
>  
> +		if (verify_freq &&
> +		    arch_timer_mem_frame_get_cntfrq(frame) != arch_timer_rate) {
> +			pr_err(FW_BUG "Ignoring MMIO timer frame with incorrect CNTFRQ\n");
> +			continue;
> +		}

I don't think this is quite right, since we only check this after
skipping over frames (which are valid, but don't match our preferences
w.r.t. the timer and counter).

Can we move this before checking the CNTACR_RWPT and CNTACR_RPCT bits?

I's still prefer that we give up (as we previously did), rather than
skipping timers with bad CNTFRQ values, but I guess that really depends
on whether we think we'll use more than one MMIO timer in future. Maybe
we'll want to use one per socket or something like that.

> @@ -1443,7 +1414,7 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>  	for (i = i; i < timer_count; i++) {
>  		timer = &timers[i];
>  
> -		frame = arch_timer_mem_find_best_frame(timer);
> +		frame = arch_timer_mem_find_best_frame(timer, true);
>  		if (frame)
>  			break;

Previously we'd have checked all timers, then chosen the best one,
whereas now we bail out once we find a timer with a usable frame.

Is there any way we can make this check the remaining timers?

Thanks,
Mark.

[PATCH v2] arm64: acpi/gtdt: validate CNTFRQ after having enabled the frame

[Ard Biesheuvel]

On 31 August 2017 at 18:47, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> Thanks for respinning this, and apologies for the delay.
>
> On Mon, Aug 21, 2017 at 10:56:23AM +0100, Ard Biesheuvel wrote:
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index aae87c4c546e..c16fa694a47b 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -1217,7 +1217,8 @@ arch_timer_mem_frame_get_cntfrq(struct arch_timer_mem_frame *frame)
>>  }
>>
>>  static struct arch_timer_mem_frame * __init
>> -arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
>> +arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem,
>> +                            bool verify_freq)
>>  {
>>       struct arch_timer_mem_frame *frame, *best_frame = NULL;
>>       void __iomem *cntctlbase;
>> @@ -1259,6 +1260,11 @@ arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
>>               if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
>>                       continue;
>>
>> +             if (verify_freq &&
>> +                 arch_timer_mem_frame_get_cntfrq(frame) != arch_timer_rate) {
>> +                     pr_err(FW_BUG "Ignoring MMIO timer frame with incorrect CNTFRQ\n");
>> +                     continue;
>> +             }
>
> I don't think this is quite right, since we only check this after
> skipping over frames (which are valid, but don't match our preferences
> w.r.t. the timer and counter).
>
> Can we move this before checking the CNTACR_RWPT and CNTACR_RPCT bits?
>

Yes.

> I's still prefer that we give up (as we previously did), rather than
> skipping timers with bad CNTFRQ values, but I guess that really depends
> on whether we think we'll use more than one MMIO timer in future. Maybe
> we'll want to use one per socket or something like that.
>

Not sure I follow, since we skip frames, not timers in this case.

>> @@ -1443,7 +1414,7 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count)
>>       for (i = i; i < timer_count; i++) {
>>               timer = &timers[i];
>>
>> -             frame = arch_timer_mem_find_best_frame(timer);
>> +             frame = arch_timer_mem_find_best_frame(timer, true);
>>               if (frame)
>>                       break;
>
> Previously we'd have checked all timers, then chosen the best one,
> whereas now we bail out once we find a timer with a usable frame.
>
> Is there any way we can make this check the remaining timers?
>

You mean as a diagnostic? We don't really care if timer frames we are
not interested in using have invalid CNTFRQ values, right?