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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Stefano

On Sat, Dec 2, 2017 at 3:37 AM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> 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?

Probably, the original author of this patch wanted to avoid playing
with some stuff (code & variables) which didn't make sense/wouldn't be
used on non-ACPI systems.

Agree here, we are able to avoid this #ifdef as well as many others. I
don't see an issue, for example, to print something defaulting for
coord_type/num_entries/revision/etc.

>
>
>>      }
>>
>>      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

definitely omit #ifdef

>
>
>>  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

same answer)

>
>
>>  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

same answer)

>
>
>>  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?

You are right. Indeed looks redundant.
I will drop all these helpers and reuse existing flags. If we are
pretending to be an P-state driver and uploading the same P-state data
which [1] uploads
then I will just reuse existing flags. It will cost me nothing.

May I ask you to take a look at this patch [2]? It looks like a hack
right now, but how to make it in a proper way?

[1] 
https://github.com/torvalds/linux/blob/master/drivers/xen/xen-acpi-processor.c#L210
[2] https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxx/msg128410.html

>
>
>>  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.

Would a following stub be enough?

diff --git a/xen/include/xen/acpi.h b/xen/include/xen/acpi.h
index 9409350..4aab41e 100644
--- a/xen/include/xen/acpi.h
+++ b/xen/include/xen/acpi.h
@@ -123,7 +123,11 @@ static inline int acpi_boot_table_init(void)

 #endif         /*!CONFIG_ACPI*/

+#ifdef CONFIG_ACPI
 int get_cpu_id(u32 acpi_id);
+#else
+static inline int get_cpu_id(u32 acpi_id) { return acpi_id; }
+#endif

 unsigned int acpi_register_gsi (u32 gsi, int edge_level, int active_high_low);
 int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);

>
>
>>      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 */

BTW, at the first sight we could omit this #ifdef too with being taken
care of space_id check to pass successfully.

>>
>> -    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;
>>  };

There will be no changes here as well.

>>
>> --
>> 2.7.4
>>


-- 
Regards,

Oleksandr Tyshchenko

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