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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 5 Dec 2017, Oleksandr Tyshchenko wrote:
> 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?

You are right. We need to define a new struct for internal usage, for
example:

struct xen_processor_performance_internal {
    uint32_t flags;     /* flag for Px sub info type */
    uint32_t platform_limit;  /* Platform limitation on freq usage */
    struct xen_pct_register control_register;
    struct xen_pct_register status_register;
    uint32_t state_count;     /* total available performance states */
    struct xen_processor_px states;
    struct xen_psd_package domain_info;
    uint32_t shared_type;     /* coordination type of this processor */
};

Jan, Andrew, does this sound like a good approach to you?

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