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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi Stefano

On Tue, Dec 5, 2017 at 12:46 AM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Mon, 4 Dec 2017, Oleksandr Tyshchenko wrote:
>> 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.
>
> I agree
>
>
>> >
>> >
>> >>      }
>> >>
>> >>      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.
>
> Makes sense
>
>
>> 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
>
> Regarding [2]:
>
> This is something that needs to be agreed with the x86 maintainers.
> However, I would move the copy_from_guest (and everything related to
> parsing caller provided arguments) to
> xen/arch/x86/platform_hypercall.c:do_platform_op.
>
> Then, I would make set_px_pminfo look like a regular function that
> takes regular arguments (no XEN_GUEST_HANDLEs), so that it can be called
> on ARM without having to "fake" an hypercall.

Just to clarify:

The current function interface is:
int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance
*dom0_px_info)
where "dom0_px_info" argument contains XEN_GUEST_HANDLE we would like
to avoid playing with in case of ARM.

The idea to move operation over XEN_GUEST_HANDLE (copy_from_guest) out
of the function sounds reasonable.
But what function interface we will end up with?

Looks like we need either to pass each structure field as a separate
argument, so "new" function interface will be the following:
int set_px_pminfo(uint32_t acpi_id, uint32_t flags, ... , struct
xen_processor_px *states, ... , uint32_t shared_type)
or to reuse "struct processor_performance" somehow in order to reduce
a scope of possible arguments...

Or I missed something?

>
>
>> >
>> >
>> >>  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);
>
> Yes, I think that's OK.
>
>
>> >
>> >
>> >>      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.
>



-- 
Regards,

Oleksandr Tyshchenko

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