On Mon, 4 Dec 2017, Oleksandr Tyshchenko wrote:
> Hi, Stefano
>
> On Sat, Dec 2, 2017 at 3:37 AM, Stefano Stabellini
> <[email protected]> wrote:
> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Dmytryshyn <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Oleksandr Tyshchenko <[email protected]>
> >> CC: Jan Beulich <[email protected]>
> >> CC: Andrew Cooper <[email protected]>
> >> CC: Stefano Stabellini <[email protected]>
> >> CC: Julien Grall <[email protected]>
> >> ---
> >> 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/[email protected]/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.
> >
> >
> >> 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.
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
