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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi Stefano

On Sat, Dec 2, 2017 at 3:21 AM, Stefano Stabellini
<sstabellini@xxxxxxxxxx> wrote:
> On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
>>
>> 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&m=141407701110860&w=2
/* RFC v4 */
https://marc.info/?l=xen-devel&m=141510663108037&w=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 <public/sysctl.h>   /* 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 <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/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 <asm/processor.h>
>>  #include <xen/percpu.h>
>>  #include <xen/domain.h>
>> -#include <xen/acpi.h>
>>
>>  #include <public/sysctl.h>
>>  #include <xen/cpufreq.h>
>> @@ -132,6 +131,8 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
>>          break;
>>      }
>>
>> +/* For now those operations can be used only when ACPI is enabled */
>> +#ifdef CONFIG_ACPI
>>      case PMSTAT_get_max_cx:
>>      {
>>          op->u.getcx.nr = pmstat_get_cx_nr(op->cpuid);
>> @@ -150,6 +151,7 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
>>          ret = pmstat_reset_cx_stat(op->cpuid);
>>          break;
>>      }
>> +#endif /* CONFIG_ACPI */
>>
>>      default:
>>          printk("not defined sub-hypercall @ do_get_pm_info\n");
>> @@ -465,6 +467,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>>          break;
>>      }
>>
>> +#ifdef CONFIG_ACPI
>>      case XEN_SYSCTL_pm_op_get_max_cstate:
>>      {
>>          op->u.get_max_cstate = acpi_get_cstate_limit();
>> @@ -476,6 +479,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>>          acpi_set_cstate_limit(op->u.set_max_cstate);
>>          break;
>>      }
>> +#endif /* CONFIG_ACPI */
>>
>>  #ifdef CONFIG_HAS_CPU_TURBO
>>      case XEN_SYSCTL_pm_op_enable_turbo:
>> @@ -500,6 +504,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>>      return ret;
>>  }
>>
>> +#ifdef CONFIG_ACPI
>>  int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
>>  {
>>      u32 bits[3];
>> @@ -530,3 +535,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..a870c8a 100644
>> --- a/xen/include/xen/pmstat.h
>> +++ b/xen/include/xen/pmstat.h
>> @@ -6,10 +6,12 @@
>>  #include <public/sysctl.h>   /* for struct pm_cx_stat */
>>
>>  int set_px_pminfo(uint32_t cpu, struct xen_processor_performance *perf);
>> +#ifdef CONFIG_ACPI
>>  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);
>> +#endif
>>
>>  int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
>>  int do_pm_op(struct xen_sysctl_pm_op *op);
>> --
>> 2.7.4
>>



-- 
Regards,

Oleksandr Tyshchenko

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