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.
>
>
>> }
>>
>> 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/[email protected]/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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
