Re: [Xen-devel] [RFC PATCH 05/31] pmstat: make pmstat functions more generalizable

[Stefano Stabellini]

On Mon, 4 Dec 2017, Oleksandr Tyshchenko wrote:
> Hi Stefano
> 
> On Sat, Dec 2, 2017 at 3:21 AM, Stefano Stabellini
>  wrote:
> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Dmytryshyn 
> >>
> >> ACPI-specific parts are moved under appropriate ifdefs.
> >> Now pmstat functions can be used in ARM platform.
> >>
> >> This is a rebased version of the original patch:
> >> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00941.html
> >
> > My first maybe naive question is: why do we want to disable the C-states
> > and not the P-states? After all, they are both defined in ACPI?
> 
> Good question. Xen CPUFreq infrastructure based on ACPI P-states. We
> have to either
> completely rework generic code/existing drivers or integrate into
> "current environment" (so, the CPUFreq driver,
> this patch series adds, is pretending that it does understand what the
> P-states are). The second option requires much less
> developing & upstreaming (I hope) efforts. BTW, with the current
> solution you don't have to modify public sysctl & xenpm.
> And looking through all previous discussions [1] I got a feeling that
> the original author of this patch had had similar opinion.
> 
> [1]
> /* RFC v0 */
> https://lists.xen.org/archives/html/xen-devel/2014-08/msg02919.html
> /* RFC v1 */
> https://lists.xenproject.org/archives/html/xen-devel/2014-10/msg00787.html
> /* RFC v2 */
> https://lists.xenproject.org/archives/html/xen-devel/2014-10/msg01879.html
> /* RFC v3 */
> https://marc.info/?l=xen-devel=141407701110860=2
> /* RFC v4 */
> https://marc.info/?l=xen-devel=141510663108037=2
> /* RFC v5 */
> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00940.html

thank you, it makes sense


> >
> > The second question is: instead of #ifdef'ing everything C-states,
> > couldn't we just rely on XEN_PROCESSOR_PM_CX not being available?
> 
> I am afraid that relying on XEN_PROCESSOR_PM_CX not being available is
> not enough.
> A few functions, which were #ifdef'd by original author of the patch,
> are located at arch/x86 path.
> So, I think, the question was to get pmstat.c compilable on ARM.
> 
> But completely agree that a scope of #ifdef's can be reduced.
> 
> 1. For next functions we will be able to omit #ifdef CONFIG_ACPI if we
> create corresponding stubs.
> - pmstat_get_cx_nr()
> - pmstat_get_cx_stat()
> - pmstat_reset_cx_stat()
> They won't never be called if XEN_PROCESSOR_PM_CX is not set.

sounds good


> 2. For next functions we, probably, may omit #ifdef CONFIG_ACPI, since
> the corresponding stubs already present (see !CONFIG_ACPI_CSTATE in
> acpi.h)
> - acpi_get_cstate_limit()
> - acpi_set_cstate_limit()

it looks like it, yes


> But acpi_set_pdc_bits() I would leave under #ifdef CONFIG_ACPI
> (CONFIG_X86 ?) or move it to arch/x86.
> It is called from arch/x86/platform_hypercall.c and pulls a bunch of
> #define-s from pdc_intel.h

Yes, I would move it to arch/x86.


> Something like that:
> 
> diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
> index 133e64d..353d0ab 100644
> --- a/xen/drivers/pm/stat.c
> +++ b/xen/drivers/pm/stat.c
> @@ -500,6 +500,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>  return ret;
>  }
> 
> +#ifdef CONFIG_ACPI /* or CONFIG_X86 ? */
>  int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
>  {
>  u32 bits[3];
> @@ -530,3 +531,4 @@ int acpi_set_pdc_bits(u32 acpi_id,
> XEN_GUEST_HANDLE_PARAM(uint32) pdc)
> 
>  return ret;
>  }
> +#endif /* CONFIG_ACPI */
> diff --git a/xen/include/xen/pmstat.h b/xen/include/xen/pmstat.h
> index 266bc16..05d6b7b 100644
> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -6,10 +6,17 @@
>  #include/* for struct pm_cx_stat */
> 
>  int set_px_pminfo(uint32_t cpu, struct xen_processor_performance *perf);
> +#ifdef CONFIG_ACPI /* or CONFIG_X86 ? */
>  long set_cx_pminfo(uint32_t cpu, struct xen_processor_power *power);
>  uint32_t pmstat_get_cx_nr(uint32_t cpuid);
>  int pmstat_get_cx_stat(uint32_t cpuid, struct pm_cx_stat *stat);
>  int pmstat_reset_cx_stat(uint32_t cpuid);
> +#else
> +static inline long set_cx_pminfo(uint32_t cpu, struct
> xen_processor_power *power) { return 0; }
> +static inline uint32_t pmstat_get_cx_nr(uint32_t cpuid) { return 0; }
> +static inline int pmstat_get_cx_stat(uint32_t cpuid, struct
> pm_cx_stat *stat) { return 0; }
> +static inline int pmstat_reset_cx_stat(uint32_t cpuid) { return 0; }
> +#endif
> 
>  int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
>  int do_pm_op(struct xen_sysctl_pm_op *op);
> 
> What do you think?

much better


> >
> >
> >> Signed-off-by: Oleksandr Dmytryshyn 
> >> Signed-off-by: Oleksandr Tyshchenko 
> >> CC: Jan Beulich 
> >> CC: Andrew Cooper 
> >> CC: Stefano Stabellini 
> >> CC: Julien Grall 

Re: [Xen-devel] [RFC PATCH 05/31] pmstat: make pmstat functions more generalizable

[Oleksandr Tyshchenko]

Hi Stefano

On Sat, Dec 2, 2017 at 3:21 AM, Stefano Stabellini
 wrote:
> On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Dmytryshyn 
>>
>> ACPI-specific parts are moved under appropriate ifdefs.
>> Now pmstat functions can be used in ARM platform.
>>
>> This is a rebased version of the original patch:
>> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00941.html
>
> My first maybe naive question is: why do we want to disable the C-states
> and not the P-states? After all, they are both defined in ACPI?

Good question. Xen CPUFreq infrastructure based on ACPI P-states. We
have to either
completely rework generic code/existing drivers or integrate into
"current environment" (so, the CPUFreq driver,
this patch series adds, is pretending that it does understand what the
P-states are). The second option requires much less
developing & upstreaming (I hope) efforts. BTW, with the current
solution you don't have to modify public sysctl & xenpm.
And looking through all previous discussions [1] I got a feeling that
the original author of this patch had had similar opinion.

[1]
/* RFC v0 */
https://lists.xen.org/archives/html/xen-devel/2014-08/msg02919.html
/* RFC v1 */
https://lists.xenproject.org/archives/html/xen-devel/2014-10/msg00787.html
/* RFC v2 */
https://lists.xenproject.org/archives/html/xen-devel/2014-10/msg01879.html
/* RFC v3 */
https://marc.info/?l=xen-devel=141407701110860=2
/* RFC v4 */
https://marc.info/?l=xen-devel=141510663108037=2
/* RFC v5 */
https://lists.xen.org/archives/html/xen-devel/2014-11/msg00940.html

>
> The second question is: instead of #ifdef'ing everything C-states,
> couldn't we just rely on XEN_PROCESSOR_PM_CX not being available?

I am afraid that relying on XEN_PROCESSOR_PM_CX not being available is
not enough.
A few functions, which were #ifdef'd by original author of the patch,
are located at arch/x86 path.
So, I think, the question was to get pmstat.c compilable on ARM.

But completely agree that a scope of #ifdef's can be reduced.

1. For next functions we will be able to omit #ifdef CONFIG_ACPI if we
create corresponding stubs.
- pmstat_get_cx_nr()
- pmstat_get_cx_stat()
- pmstat_reset_cx_stat()
They won't never be called if XEN_PROCESSOR_PM_CX is not set.

2. For next functions we, probably, may omit #ifdef CONFIG_ACPI, since
the corresponding stubs already present (see !CONFIG_ACPI_CSTATE in
acpi.h)
- acpi_get_cstate_limit()
- acpi_set_cstate_limit()

But acpi_set_pdc_bits() I would leave under #ifdef CONFIG_ACPI
(CONFIG_X86 ?) or move it to arch/x86.
It is called from arch/x86/platform_hypercall.c and pulls a bunch of
#define-s from pdc_intel.h

Something like that:

diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
index 133e64d..353d0ab 100644
--- a/xen/drivers/pm/stat.c
+++ b/xen/drivers/pm/stat.c
@@ -500,6 +500,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
 return ret;
 }

+#ifdef CONFIG_ACPI /* or CONFIG_X86 ? */
 int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
 {
 u32 bits[3];
@@ -530,3 +531,4 @@ int acpi_set_pdc_bits(u32 acpi_id,
XEN_GUEST_HANDLE_PARAM(uint32) pdc)

 return ret;
 }
+#endif /* CONFIG_ACPI */
diff --git a/xen/include/xen/pmstat.h b/xen/include/xen/pmstat.h
index 266bc16..05d6b7b 100644
--- a/xen/include/xen/pmstat.h
+++ b/xen/include/xen/pmstat.h
@@ -6,10 +6,17 @@
 #include/* for struct pm_cx_stat */

 int set_px_pminfo(uint32_t cpu, struct xen_processor_performance *perf);
+#ifdef CONFIG_ACPI /* or CONFIG_X86 ? */
 long set_cx_pminfo(uint32_t cpu, struct xen_processor_power *power);
 uint32_t pmstat_get_cx_nr(uint32_t cpuid);
 int pmstat_get_cx_stat(uint32_t cpuid, struct pm_cx_stat *stat);
 int pmstat_reset_cx_stat(uint32_t cpuid);
+#else
+static inline long set_cx_pminfo(uint32_t cpu, struct
xen_processor_power *power) { return 0; }
+static inline uint32_t pmstat_get_cx_nr(uint32_t cpuid) { return 0; }
+static inline int pmstat_get_cx_stat(uint32_t cpuid, struct
pm_cx_stat *stat) { return 0; }
+static inline int pmstat_reset_cx_stat(uint32_t cpuid) { return 0; }
+#endif

 int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
 int do_pm_op(struct xen_sysctl_pm_op *op);

What do you think?

>
>
>> Signed-off-by: Oleksandr Dmytryshyn 
>> Signed-off-by: Oleksandr Tyshchenko 
>> CC: Jan Beulich 
>> CC: Andrew Cooper 
>> CC: Stefano Stabellini 
>> CC: Julien Grall 
>> ---
>>  xen/drivers/pm/stat.c| 8 +++-
>>  xen/include/xen/pmstat.h | 2 ++
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
>> index 133e64d..986ba41 100644
>> --- a/xen/drivers/pm/stat.c
>> +++ b/xen/drivers/pm/stat.c
>> @@ -35,7 +35,6 @@
>>  #include 
>>  #include 
>>  #include 
>> -#include 
>>
>>  #include 
>>