Re: [Xen-devel] [RFC PATCH 06/31] cpufreq: make cpufreq driver more generalizable

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> 
> First implementation of the cpufreq driver has been
> written with x86 in mind. This patch makes possible
> the cpufreq driver be working on both x86 and arm
> architectures.
> 
> This is a rebased version of the original patch:
> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00932.html
> 
> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxxxxx>
> ---
>  xen/drivers/cpufreq/cpufreq.c    | 81 
> +++++++++++++++++++++++++++++++++++++---
>  xen/include/public/platform.h    |  1 +
>  xen/include/xen/processor_perf.h |  6 +++
>  3 files changed, 82 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
> index ab909e2..64e1ae7 100644
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -42,7 +42,6 @@
>  #include <asm/io.h>
>  #include <asm/processor.h>
>  #include <asm/percpu.h>
> -#include <acpi/acpi.h>
>  #include <xen/cpufreq.h>
>  
>  static unsigned int __read_mostly usr_min_freq;
> @@ -206,6 +205,7 @@ int cpufreq_add_cpu(unsigned int cpu)
>      } else {
>          /* domain sanity check under whatever coordination type */
>          firstcpu = cpumask_first(cpufreq_dom->map);
> +#ifdef CONFIG_ACPI
>          if ((perf->domain_info.coord_type !=
>              processor_pminfo[firstcpu]->perf.domain_info.coord_type) ||
>              (perf->domain_info.num_processors !=
> @@ -221,6 +221,19 @@ int cpufreq_add_cpu(unsigned int cpu)
>                  );
>              return -EINVAL;
>          }
> +#else /* !CONFIG_ACPI */
> +        if ((perf->domain_info.num_processors !=
> +            processor_pminfo[firstcpu]->perf.domain_info.num_processors)) {
> +
> +            printk(KERN_WARNING "cpufreq fail to add CPU%d:"
> +                   "incorrect num processors (%"PRIu64"), "
> +                   "expect(%"PRIu64")\n",
> +                   cpu, perf->domain_info.num_processors,
> +                   
> processor_pminfo[firstcpu]->perf.domain_info.num_processors
> +                );
> +            return -EINVAL;
> +        }
> +#endif /* CONFIG_ACPI */

Why is this necessary? I am asking this question, because I think it
would be best to avoid more #ifdef's if we can avoid them, and some of
the code #ifdef'ed doesn't look very acpi specific (at least at first
sight). It doesn't look like this change is very beneficial. What am I
missing?


>      }
>  
>      if (!domexist || hw_all) {
> @@ -380,6 +393,7 @@ int cpufreq_del_cpu(unsigned int cpu)
>      return 0;
>  }
>  
> +#ifdef CONFIG_ACPI
>  static void print_PCT(struct xen_pct_register *ptr)
>  {
>      printk("\t_PCT: descriptor=%d, length=%d, space_id=%d, "
> @@ -387,12 +401,14 @@ static void print_PCT(struct xen_pct_register *ptr)
>             ptr->descriptor, ptr->length, ptr->space_id, ptr->bit_width,
>             ptr->bit_offset, ptr->reserved, ptr->address);
>  }
> +#endif /* CONFIG_ACPI */

same question


>  static void print_PSS(struct xen_processor_px *ptr, int count)
>  {
>      int i;
>      printk("\t_PSS: state_count=%d\n", count);
>      for (i=0; i<count; i++){
> +#ifdef CONFIG_ACPI
>          printk("\tState%d: %"PRId64"MHz %"PRId64"mW %"PRId64"us "
>                 "%"PRId64"us %#"PRIx64" %#"PRIx64"\n",
>                 i,
> @@ -402,15 +418,26 @@ static void print_PSS(struct xen_processor_px *ptr, int 
> count)
>                 ptr[i].bus_master_latency,
>                 ptr[i].control,
>                 ptr[i].status);
> +#else /* !CONFIG_ACPI */
> +        printk("\tState%d: %"PRId64"MHz %"PRId64"us\n",
> +               i,
> +               ptr[i].core_frequency,
> +               ptr[i].transition_latency);
> +#endif /* CONFIG_ACPI */
>      }
>  }
  
same question


>  static void print_PSD( struct xen_psd_package *ptr)
>  {
> +#ifdef CONFIG_ACPI
>      printk("\t_PSD: num_entries=%"PRId64" rev=%"PRId64
>             " domain=%"PRId64" coord_type=%"PRId64" 
> num_processors=%"PRId64"\n",
>             ptr->num_entries, ptr->revision, ptr->domain, ptr->coord_type,
>             ptr->num_processors);
> +#else /* !CONFIG_ACPI */
> +    printk("\t_PSD:  domain=%"PRId64" num_processors=%"PRId64"\n",
> +           ptr->domain, ptr->num_processors);
> +#endif /* CONFIG_ACPI */
>  }

same question


>  static void print_PPC(unsigned int platform_limit)
> @@ -418,13 +445,53 @@ static void print_PPC(unsigned int platform_limit)
>      printk("\t_PPC: %d\n", platform_limit);
>  }
>  
> +static inline bool is_pss_data(struct xen_processor_performance *px)
> +{
> +#ifdef CONFIG_ACPI
> +    return px->flags & XEN_PX_PSS;
> +#else
> +    return px->flags == XEN_PX_DATA;
> +#endif
> +}
> +
> +static inline bool is_psd_data(struct xen_processor_performance *px)
> +{
> +#ifdef CONFIG_ACPI
> +    return px->flags & XEN_PX_PSD;
> +#else
> +    return px->flags == XEN_PX_DATA;
> +#endif
> +}
> +
> +static inline bool is_ppc_data(struct xen_processor_performance *px)
> +{
> +#ifdef CONFIG_ACPI
> +    return px->flags & XEN_PX_PPC;
> +#else
> +    return px->flags == XEN_PX_DATA;
> +#endif
> +}
> +
> +static inline bool is_all_data(struct xen_processor_performance *px)
> +{
> +#ifdef CONFIG_ACPI
> +    return px->flags == ( XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC 
> );
> +#else
> +    return px->flags == XEN_PX_DATA;
> +#endif
> +}

Could you please explain here and in the commit message the idea behind
this? It looks like we want to get rid of the different flags on
non-ACPI systems? Why can't we reuse the same flags?


>  int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance 
> *dom0_px_info)
>  {
>      int ret=0, cpuid;
>      struct processor_pminfo *pmpt;
>      struct processor_performance *pxpt;
>  
> +#ifdef CONFIG_ACPI
>      cpuid = get_cpu_id(acpi_id);
> +#else
> +    cpuid = acpi_id;
> +#endif

Rather than an #ifdef here, I would probably generalize the get_cpu_id
function.


>      if ( cpuid < 0 || !dom0_px_info)
>      {
>          ret = -EINVAL;
> @@ -446,6 +513,8 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>          processor_pminfo[cpuid] = pmpt;
>      }
>      pxpt = &pmpt->perf;
> +
> +#ifdef CONFIG_ACPI
>      pmpt->acpi_id = acpi_id;
>      pmpt->id = cpuid;
>  
> @@ -472,8 +541,9 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>              print_PCT(&pxpt->status_register);
>          }
>      }
> +#endif /* CONFIG_ACPI */
>  
> -    if ( dom0_px_info->flags & XEN_PX_PSS ) 
> +    if ( is_pss_data(dom0_px_info) )
>      {
>          /* capability check */
>          if (dom0_px_info->state_count <= 1)
> @@ -500,7 +570,7 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>              print_PSS(pxpt->states,pxpt->state_count);
>      }
>  
> -    if ( dom0_px_info->flags & XEN_PX_PSD )
> +    if ( is_psd_data(dom0_px_info) )
>      {
>          /* check domain coordination */
>          if (dom0_px_info->shared_type != CPUFREQ_SHARED_TYPE_ALL &&
> @@ -520,7 +590,7 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>              print_PSD(&pxpt->domain_info);
>      }
>  
> -    if ( dom0_px_info->flags & XEN_PX_PPC )
> +    if ( is_ppc_data(dom0_px_info) )
>      {
>          pxpt->platform_limit = dom0_px_info->platform_limit;
>  
> @@ -534,8 +604,7 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *dom0_px_in
>          }
>      }
>  
> -    if ( dom0_px_info->flags == ( XEN_PX_PCT | XEN_PX_PSS |
> -                XEN_PX_PSD | XEN_PX_PPC ) )
> +    if ( is_all_data(dom0_px_info) )
>      {
>          pxpt->init = XEN_PX_INIT;
>  
> diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> index 94dbc3f..328579c 100644
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -384,6 +384,7 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
>  #define XEN_PX_PSS   2
>  #define XEN_PX_PPC   4
>  #define XEN_PX_PSD   8
> +#define XEN_PX_DATA  16
>  
>  struct xen_power_register {
>      uint32_t     space_id;
> diff --git a/xen/include/xen/processor_perf.h 
> b/xen/include/xen/processor_perf.h
> index d8a1ba6..afdccf2 100644
> --- a/xen/include/xen/processor_perf.h
> +++ b/xen/include/xen/processor_perf.h
> @@ -3,7 +3,9 @@
>  
>  #include <public/platform.h>
>  #include <public/sysctl.h>
> +#ifdef CONFIG_ACPI
>  #include <xen/acpi.h>
> +#endif
>  
>  #define XEN_PX_INIT 0x80000000
>  
> @@ -24,8 +26,10 @@ int  cpufreq_del_cpu(unsigned int);
>  struct processor_performance {
>      uint32_t state;
>      uint32_t platform_limit;
> +#ifdef CONFIG_ACPI
>      struct xen_pct_register control_register;
>      struct xen_pct_register status_register;
> +#endif
>      uint32_t state_count;
>      struct xen_processor_px *states;
>      struct xen_psd_package domain_info;
> @@ -35,8 +39,10 @@ struct processor_performance {
>  };
>  
>  struct processor_pminfo {
> +#ifdef CONFIG_ACPI
>      uint32_t acpi_id;
>      uint32_t id;
> +#endif
>      struct processor_performance    perf;
>  };
>  
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel