Re: [PATCH] xen/arm: Validate generic timer frequency

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 27 Sep 2023, Michal Orzel wrote:
> Generic timer dt node property "clock-frequency" (refer Linux dt binding
> Documentation/devicetree/bindings/timer/arm,arch_timer.yaml) is used to
> override the incorrect value set by firmware in CNTFRQ_EL0. If the value
> of this property is 0 (i.e. by mistake), Xen would continue to work and
> use the value from the sysreg instead. The logic is thus incorrect and
> results in inconsistency when creating timer node for domUs:
>  - libxl domUs: clock_frequency member of struct xen_arch_domainconfig
>    is set to 0 and libxl ignores setting the "clock-frequency" property,
>  - dom0less domUs: "clock-frequency" property is taken from dt_host and
>    thus set to 0.
> 
> Property "clock-frequency" is used to override the value from sysreg,
> so if it is also invalid, there is nothing we can do and we shall panic
> to let user know about incorrect setting. Going even further, we operate
> under assumption that the frequency must be at least 1KHz (i.e. cpu_khz
> not 0) in order for Xen to boot. Therefore, introduce a new helper
> validate_timer_frequency() to verify this assumption and use it to check
> the frequency obtained either from dt property or from sysreg.
> 
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
>  xen/arch/arm/time.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 3535bd8ac7c7..04b07096b165 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -101,6 +101,17 @@ static void __init preinit_acpi_xen_time(void)
>  static void __init preinit_acpi_xen_time(void) { }
>  #endif
>  
> +static void __init validate_timer_frequency(void)
> +{
> +    /*
> +     * ARM ARM does not impose any strict limit on the range of allowable
> +     * system counter frequencies. However, we operate under the assumption
> +     * that cpu_khz must not be 0.
> +     */
> +    if ( !cpu_khz )
> +        panic("Timer frequency is less than 1 KHz\n");
> +}
> +
>  /* Set up the timer on the boot CPU (early init function) */
>  static void __init preinit_dt_xen_time(void)
>  {
> @@ -122,6 +133,7 @@ static void __init preinit_dt_xen_time(void)
>      if ( res )
>      {
>          cpu_khz = rate / 1000;
> +        validate_timer_frequency();
>          timer_dt_clock_frequency = rate;
>      }
>  }
> @@ -137,7 +149,10 @@ void __init preinit_xen_time(void)
>          preinit_acpi_xen_time();
>  
>      if ( !cpu_khz )
> +    {
>          cpu_khz = (READ_SYSREG(CNTFRQ_EL0) & CNTFRQ_MASK) / 1000;
> +        validate_timer_frequency();
> +    }
>  
>      res = platform_init_time();
>      if ( res )
> -- 
> 2.25.1
>