|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 06/31] cpufreq: make cpufreq driver more generalizable
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. > > > > > >> 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 Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |