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

[Oleksandr Tyshchenko]

Hi Jan

On Fri, Dec 8, 2017 at 10:07 AM, Jan Beulich  wrote:
 On 07.12.17 at 21:31,  wrote:
>> Have questions which need to be clarified:
>>
>> If I understood correctly, new variant of set_px_pminfo is going to
>> have an extra "flag" argument, since
>> struct processor_performance doesn't have "flag" field (it contains
>> "state" field instead, which has yet another meaning).
>> Something like that:
>> int set_px_pminfo(uint32_t acpi_id, uint32_t flag, struct
>> processor_performance *dom0_px_info)
>> Is my understanding correct?
>
> Well, you obviously must not lose information, so having that
> extra parameter is unavoidable. Please use common sense
> when dealing with such re-structuring. And btw, please also be
> precise: There's no "flag" field, but there is a "flags" one. Such
> should also be the name of the new parameter - we're talking
> about multiple bits here, after all.
Indeed "flags", sorry for being unclear.

>
>> As for set_cx_pminfo(). To what struct we should do translation from
>> struct xen_processor_power? (struct acpi_processor_power?)
>
> Yes, of course.
>
>> Briefly looking at set_cx_pminfo(), I got a feeling, that in order to
>> modify it in a "set_px_pminfo() manner"
>> we need to rework print_cx_pminfo(),  set_cx(), check_cx(),
>> acpi_processor_ffh_cstate_probe() too, since
>> all these function have arguments which contain XEN_GUEST_HANDLE. I am
>> wondering is it worth
>> doing such rework taking into the account that set_cx_pminfo() is not
>> going to be called from the non-hypercall context.
>> Or I missed something?
>
> Without looking at the details of this, please again use common
> sense. If there are good reasons for the two functions to not
> follow the same model, please simply state so in the overview
> mail of the patch series and/or (briefly, but concisely) in the
> specific patch's description. A good reason for example would
> be if overly large amounts of other code would need touching.
Agree.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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

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

[Oleksandr Tyshchenko]

Hi, Stefano, Jan

On Thu, Dec 7, 2017 at 10:45 AM, Jan Beulich  wrote:
 On 07.12.17 at 00:44,  wrote:
>> Oleksandr would like to call set_px_pminfo from a non-hypercall context,
>> meaning that there are no XEN_GUEST_HANDLE parameters. Today, struct
>> xen_processor_performance contains a
>>
>>   XEN_GUEST_HANDLE(xen_processor_px_t) states;
>>
>> field. Instead of "faking" the XEN_GUEST_HANDLE field from Xen, I
>> suggested to modify set_px_pminfo to take a different struct, one
>> without any XEN_GUEST_HANDLE field. 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;   < this is the interesting change
>>  struct xen_psd_package domain_info;
>>  uint32_t shared_type; /* coordination type of this processor */
>>  };
>>
>> The caller, in the x86 case is
>> xen/arch/x86/platform_hypercall.c:do_platform_op, would be resposible
>> for issuing the copy_from_guest.
Stefano, thank you for the detailed clarification.

>
> I think we don't want yet another variant of the structure: I'd
> then prefer to have a function doing the translation from struct
> xen_processor_performance to struct processor_performance,
> and hand the result to set_px_pminfo(). For consistency I'd then
> like to ask though that the same be done for set_cx_pminfo().

Jan, Stefano, thank you for suggestions.

Have questions which need to be clarified:

If I understood correctly, new variant of set_px_pminfo is going to
have an extra "flag" argument, since
struct processor_performance doesn't have "flag" field (it contains
"state" field instead, which has yet another meaning).
Something like that:
int set_px_pminfo(uint32_t acpi_id, uint32_t flag, struct
processor_performance *dom0_px_info)
Is my understanding correct?

As for set_cx_pminfo(). To what struct we should do translation from
struct xen_processor_power? (struct acpi_processor_power?)

Briefly looking at set_cx_pminfo(), I got a feeling, that in order to
modify it in a "set_px_pminfo() manner"
we need to rework print_cx_pminfo(),  set_cx(), check_cx(),
acpi_processor_ffh_cstate_probe() too, since
all these function have arguments which contain XEN_GUEST_HANDLE. I am
wondering is it worth
doing such rework taking into the account that set_cx_pminfo() is not
going to be called from the non-hypercall context.
Or I missed something?

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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

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

[Jan Beulich]

>>> On 07.12.17 at 00:44,  wrote:
> Oleksandr would like to call set_px_pminfo from a non-hypercall context,
> meaning that there are no XEN_GUEST_HANDLE parameters. Today, struct
> xen_processor_performance contains a
> 
>   XEN_GUEST_HANDLE(xen_processor_px_t) states;
> 
> field. Instead of "faking" the XEN_GUEST_HANDLE field from Xen, I
> suggested to modify set_px_pminfo to take a different struct, one
> without any XEN_GUEST_HANDLE field. 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;   < this is the interesting change
>  struct xen_psd_package domain_info;
>  uint32_t shared_type; /* coordination type of this processor */
>  };
> 
> The caller, in the x86 case is
> xen/arch/x86/platform_hypercall.c:do_platform_op, would be resposible
> for issuing the copy_from_guest.

I think we don't want yet another variant of the structure: I'd
then prefer to have a function doing the translation from struct
xen_processor_performance to struct processor_performance,
and hand the result to set_px_pminfo(). For consistency I'd then
like to ask though that the same be done for set_cx_pminfo().

Jan


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

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

[Jan Beulich]

>>> On 05.12.17 at 21:48,  wrote:
> 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?

I'm afraid I don't have the time to go through this discussion (and
the original patch) in detail to figure out the full context in which
you raise the question. IOW please summarize things alongside
the proposed structure, or alternatively Oleksandr could simply
submit an updated patch to allow seeing the actual context
(albeit in any case I can't promise timely feedback, given the
number of pending patches plus all the work I still hope to be
able to get done myself eventually.

From a brief check, I can't really figure much of a difference to
the already existing (and internal) struct processor_performance.

Jan


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

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

[Oleksandr Tyshchenko]

Hi Stefano

On Tue, Dec 5, 2017 at 12:46 AM, Stefano Stabellini
 wrote:
> On Mon, 4 Dec 2017, Oleksandr Tyshchenko wrote:
>> Hi, Stefano
>>
>> On Sat, Dec 2, 2017 at 3:37 AM, Stefano Stabellini
>>  wrote:
>> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> >> From: Oleksandr Dmytryshyn 
>> >>
>> >> 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 
>> >> Signed-off-by: Oleksandr Tyshchenko 
>> >> CC: Jan Beulich 
>> >> CC: Andrew Cooper 
>> >> CC: Stefano Stabellini 
>> >> CC: Julien Grall 
>> >> ---
>> >>  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 
>> >>  #include 
>> >>  #include 
>> >> -#include 
>> >>  #include 
>> >>
>> >>  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> >> +#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,
>> >> +   

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

[Oleksandr Tyshchenko]

Hi, Stefano

On Sat, Dec 2, 2017 at 3:37 AM, Stefano Stabellini
 wrote:
> On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Dmytryshyn 
>>
>> 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 
>> Signed-off-by: Oleksandr Tyshchenko 
>> CC: Jan Beulich 
>> CC: Andrew Cooper 
>> CC: Stefano Stabellini 
>> CC: Julien Grall 
>> ---
>>  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 
>>  #include 
>>  #include 
>> -#include 
>>  #include 
>>
>>  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> +#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 /*