Hi Stefano
On Tue, Dec 5, 2017 at 12:46 AM, Stefano Stabellini
<[email protected]> wrote:
> 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.
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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
