All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor
@ 2014-10-16 11:26 Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 01/12] cpufreq: move cpufreq.h file to the xen/include/xen location Oleksandr Dmytryshyn
                   ` (11 more replies)
  0 siblings, 12 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Hi to all.

Next series of patches implements xen-cpufreq driver in Xen hypervisor.

Cpufreq core and registered cpufreq governors are located in xen. Dom0 has CPU
driver which can only change frequency of the physical CPUs. In addition this
driver can change CPUs regulator voltage. At start time xen-cpufreq driver
in kernel uploads to Xen information about physical cpus.
Xen notifies Dom0 kernel using VIRQ_CPUFREQ interrupt. Then xen-cpufreq driver
in kernel uses XEN_SYSCTL_cpufreq_op operation from HYPERVISOR_sysctl hypercall
to get some parameters from Xen (frequency, relation and cpu number).
Then xen-cpufreq changes frequency on physical cpu and uses the same
XEN_SYSCTL_cpufreq_op operation ti give the result to Xen.

Changed since v1:
 * use /xen/include/xen/ instead of the  /xen/include/cpufreq/
   for included files
 * move pmstat.c file to the xen/drivers/pm/stat.c instead of the
   xen/drivers/pm/pmstat.c
 * updated ./MAINTAINERS accordingly to new files location
 * introduce HAS_CPU_TURBO config and use it
 * move ACPI-specific pmstat functions under the CONFIG_ACPI config
   instead of the CONFIG_X86 config
 * correct info message in cpufreq_add_cpu() function (remove _PSD
   prefix for NON ACPI configuration)
 * dropped patch "[RFC PATCH 07/13] xen/arm: enable cpu hotplug"
 * dropped patch "[RFC PATCH 08/13] xen/dts: make the dt_find_property
   function to be global"
 * create PCPUs device tree node in /hypervisor/pcpus node instead
   of the /cpus/cpu@0/private_date/ node
 * reworked platform hypercall implementation (used XSM check
   for ARM architecture) and moved common code to the common
   place.
 * xen-cpufreq driver to the dom0-cpufreq

Oleksandr Dmytryshyn (12):
  cpufreq: move cpufreq.h file to the xen/include/xen location
  pm: move processor_perf.h file to the xen/include/xen location
  pmstat: move pmstat.c file to the xen/drivers/pm/stat.c location
  cpufreq: make turbo settings to be configurable
  pmstat: make pmstat functions more generalizable
  cpufreq: make cpufreq driver more generalizable
  arch/arm: create device tree nodes for Dom0 cpufreq cpu driver
  xsm: enable xsm_platform_op hook for all architectures
  xen: arm: implement platform hypercall
  cpufreq: add dom0-cpufreq driver
  xen: arm: implement XEN_SYSCTL_cpufreq_op
  xen/arm: enable cpufreq functionality for ARM

 MAINTAINERS                                        |   3 +-
 xen/Rules.mk                                       |   4 +
 xen/arch/arm/Makefile                              |   1 +
 xen/arch/arm/Rules.mk                              |   3 +
 xen/arch/arm/domain_build.c                        |  67 ++++++
 xen/arch/arm/platform_hypercall.c                  |  57 +++++
 xen/arch/arm/traps.c                               |   1 +
 xen/arch/x86/Rules.mk                              |   2 +
 xen/arch/x86/acpi/cpu_idle.c                       |   2 +-
 xen/arch/x86/acpi/cpufreq/cpufreq.c                |   2 +-
 xen/arch/x86/acpi/cpufreq/powernow.c               |   2 +-
 xen/arch/x86/acpi/power.c                          |   2 +-
 xen/arch/x86/cpu/mwait-idle.c                      |   2 +-
 xen/arch/x86/platform_hypercall.c                  |  31 +--
 xen/arch/x86/x86_64/platform_hypercall.c           |   2 +
 xen/common/Makefile                                |   1 +
 xen/common/platform_hypercall.c                    |  70 ++++++
 xen/common/sysctl.c                                |  10 +-
 xen/drivers/Makefile                               |   1 +
 xen/drivers/acpi/Makefile                          |   1 -
 xen/drivers/cpufreq/Makefile                       |   1 +
 xen/drivers/cpufreq/cpufreq.c                      |  48 +++-
 xen/drivers/cpufreq/cpufreq_misc_governors.c       |   2 +-
 xen/drivers/cpufreq/cpufreq_ondemand.c             |   4 +-
 xen/drivers/cpufreq/dom0-cpufreq.c                 | 267 +++++++++++++++++++++
 xen/drivers/cpufreq/utility.c                      |  13 +-
 xen/drivers/pm/Makefile                            |   1 +
 xen/drivers/{acpi/pmstat.c => pm/stat.c}           |  17 +-
 xen/include/public/sysctl.h                        |  19 ++
 xen/include/public/xen.h                           |   1 +
 xen/include/{acpi/cpufreq => xen}/cpufreq.h        |  18 +-
 xen/include/{acpi/cpufreq => xen}/processor_perf.h |   7 +
 xen/include/xsm/dummy.h                            |  12 +-
 xen/include/xsm/xsm.h                              |  10 +-
 xen/xsm/flask/hooks.c                              |   3 +-
 35 files changed, 624 insertions(+), 63 deletions(-)
 create mode 100644 xen/arch/arm/platform_hypercall.c
 create mode 100644 xen/common/platform_hypercall.c
 create mode 100644 xen/drivers/cpufreq/dom0-cpufreq.c
 create mode 100644 xen/drivers/pm/Makefile
 rename xen/drivers/{acpi/pmstat.c => pm/stat.c} (97%)
 rename xen/include/{acpi/cpufreq => xen}/cpufreq.h (96%)
 rename xen/include/{acpi/cpufreq => xen}/processor_perf.h (95%)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 01/12] cpufreq: move cpufreq.h file to the xen/include/xen location
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
@ 2014-10-16 11:26 ` Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 02/12] pm: move processor_perf.h " Oleksandr Dmytryshyn
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Cpufreq driver should be more generalizable (not ACPI-specific).
Thus this file should be placed to more convenient location.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 MAINTAINERS                                  | 1 +
 xen/arch/x86/acpi/cpu_idle.c                 | 2 +-
 xen/arch/x86/acpi/cpufreq/cpufreq.c          | 2 +-
 xen/arch/x86/acpi/cpufreq/powernow.c         | 2 +-
 xen/arch/x86/acpi/power.c                    | 2 +-
 xen/arch/x86/cpu/mwait-idle.c                | 2 +-
 xen/drivers/acpi/pmstat.c                    | 2 +-
 xen/drivers/cpufreq/cpufreq.c                | 2 +-
 xen/drivers/cpufreq/cpufreq_misc_governors.c | 2 +-
 xen/drivers/cpufreq/cpufreq_ondemand.c       | 4 ++--
 xen/drivers/cpufreq/utility.c                | 2 +-
 xen/include/{acpi/cpufreq => xen}/cpufreq.h  | 7 +++++--
 12 files changed, 17 insertions(+), 13 deletions(-)
 rename xen/include/{acpi/cpufreq => xen}/cpufreq.h (98%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7757cdd..49f56a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -235,6 +235,7 @@ X:	xen/arch/x86/acpi/boot.c
 X:	xen/arch/x86/acpi/lib.c
 F:	xen/drivers/cpufreq/
 F:	xen/include/acpi/cpufreq/
+F:	xen/include/xen/cpufreq.h
 
 QEMU-DM
 M:	Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index 597befa..d773955 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -51,7 +51,7 @@
 #include <xen/softirq.h>
 #include <public/platform.h>
 #include <public/sysctl.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 #include <asm/apic.h>
 #include <asm/cpuidle.h>
 #include <asm/mwait.h>
diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c b/xen/arch/x86/acpi/cpufreq/cpufreq.c
index 4a6aeb3..5d22257 100644
--- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
+++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
@@ -42,7 +42,7 @@
 #include <asm/percpu.h>
 #include <asm/cpufeature.h>
 #include <acpi/acpi.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 enum {
     UNDEFINED_CAPABLE = 0,
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c
index 2c9fea2..4961d55 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -36,7 +36,7 @@
 #include <asm/percpu.h>
 #include <asm/cpufeature.h>
 #include <acpi/acpi.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 #define CPUID_6_ECX_APERFMPERF_CAPABILITY       (0x1)
 #define CPUID_FREQ_VOLT_CAPABILITIES    0x80000007
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index f41f0de..f4a87e3 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -29,7 +29,7 @@
 #include <asm/tboot.h>
 #include <asm/apic.h>
 #include <asm/io_apic.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 uint32_t system_reset_counter = 1;
 
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 85179f2..c72219a 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -59,7 +59,7 @@
 #include <asm/hpet.h>
 #include <asm/mwait.h>
 #include <asm/msr.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 #define MWAIT_IDLE_VERSION "0.4"
 #undef PREFIX
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index daac2da..3486148 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -40,7 +40,7 @@
 #include <xen/acpi.h>
 
 #include <public/sysctl.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 #include <xen/pmstat.h>
 
 DEFINE_PER_CPU_READ_MOSTLY(struct pm_px *, cpufreq_statistic_data);
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index ab66884..f5f4d75 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -44,7 +44,7 @@
 #include <asm/processor.h>
 #include <asm/percpu.h>
 #include <acpi/acpi.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 static unsigned int __read_mostly usr_min_freq;
 static unsigned int __read_mostly usr_max_freq;
diff --git a/xen/drivers/cpufreq/cpufreq_misc_governors.c b/xen/drivers/cpufreq/cpufreq_misc_governors.c
index 746bbcd..4a5510c 100644
--- a/xen/drivers/cpufreq/cpufreq_misc_governors.c
+++ b/xen/drivers/cpufreq/cpufreq_misc_governors.c
@@ -18,7 +18,7 @@
 #include <xen/init.h>
 #include <xen/percpu.h>
 #include <xen/sched.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 /*
  * cpufreq userspace governor
diff --git a/xen/drivers/cpufreq/cpufreq_ondemand.c b/xen/drivers/cpufreq/cpufreq_ondemand.c
index 7fdba03..d490c8a 100644
--- a/xen/drivers/cpufreq/cpufreq_ondemand.c
+++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
@@ -1,5 +1,5 @@
 /*
- *  xen/arch/x86/acpi/cpufreq/cpufreq_ondemand.c
+ *  xen/drivers/cpufreq/cpufreq_ondemand.c
  *
  *  Copyright (C)  2001 Russell King
  *            (C)  2003 Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>.
@@ -18,7 +18,7 @@
 #include <xen/types.h>
 #include <xen/sched.h>
 #include <xen/timer.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 
 #define DEF_FREQUENCY_UP_THRESHOLD              (80)
 #define MIN_FREQUENCY_UP_THRESHOLD              (11)
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index 519f862..3cb0b3e 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -28,7 +28,7 @@
 #include <xen/sched.h>
 #include <xen/timer.h>
 #include <xen/trace.h>
-#include <acpi/cpufreq/cpufreq.h>
+#include <xen/cpufreq.h>
 #include <public/sysctl.h>
 
 struct cpufreq_driver   *cpufreq_driver;
diff --git a/xen/include/acpi/cpufreq/cpufreq.h b/xen/include/xen/cpufreq.h
similarity index 98%
rename from xen/include/acpi/cpufreq/cpufreq.h
rename to xen/include/xen/cpufreq.h
index f96c3e4..fa653ef 100644
--- a/xen/include/acpi/cpufreq/cpufreq.h
+++ b/xen/include/xen/cpufreq.h
@@ -1,5 +1,5 @@
 /*
- *  xen/include/acpi/cpufreq/cpufreq.h
+ *  xen/include/xen/cpufreq.h
  *
  *  Copyright (C) 2001 Russell King
  *            (C) 2002 - 2003 Dominik Brodowski <linux@brodo.de>
@@ -16,9 +16,12 @@
 
 #include <xen/types.h>
 #include <xen/list.h>
+#include <xen/percpu.h>
+#include <xen/spinlock.h>
+#include <xen/errno.h>
 #include <xen/cpumask.h>
 
-#include "processor_perf.h"
+#include <acpi/cpufreq/processor_perf.h>
 
 DECLARE_PER_CPU(spinlock_t, cpufreq_statistic_lock);
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 02/12] pm: move processor_perf.h file to the xen/include/xen location
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 01/12] cpufreq: move cpufreq.h file to the xen/include/xen location Oleksandr Dmytryshyn
@ 2014-10-16 11:26 ` Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 03/12] pmstat: move pmstat.c file to the xen/drivers/pm/stat.c location Oleksandr Dmytryshyn
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Cpufreq driver should be more generalizable (not ACPI-specific).
Thus this file should be placed to more convenient location.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 MAINTAINERS                                        | 2 +-
 xen/arch/x86/platform_hypercall.c                  | 2 +-
 xen/include/xen/cpufreq.h                          | 2 +-
 xen/include/{acpi/cpufreq => xen}/processor_perf.h | 0
 4 files changed, 3 insertions(+), 3 deletions(-)
 rename xen/include/{acpi/cpufreq => xen}/processor_perf.h (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 49f56a1..f4d916e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -234,8 +234,8 @@ F:	xen/arch/x86/acpi/
 X:	xen/arch/x86/acpi/boot.c
 X:	xen/arch/x86/acpi/lib.c
 F:	xen/drivers/cpufreq/
-F:	xen/include/acpi/cpufreq/
 F:	xen/include/xen/cpufreq.h
+F:	xen/include/xen/processor_perf.h
 
 QEMU-DM
 M:	Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 2162811..7ce8592 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -25,7 +25,7 @@
 #include <xen/irq.h>
 #include <asm/current.h>
 #include <public/platform.h>
-#include <acpi/cpufreq/processor_perf.h>
+#include <xen/processor_perf.h>
 #include <asm/edd.h>
 #include <asm/mtrr.h>
 #include <asm/io_apic.h>
diff --git a/xen/include/xen/cpufreq.h b/xen/include/xen/cpufreq.h
index fa653ef..82dc4dc 100644
--- a/xen/include/xen/cpufreq.h
+++ b/xen/include/xen/cpufreq.h
@@ -21,7 +21,7 @@
 #include <xen/errno.h>
 #include <xen/cpumask.h>
 
-#include <acpi/cpufreq/processor_perf.h>
+#include <xen/processor_perf.h>
 
 DECLARE_PER_CPU(spinlock_t, cpufreq_statistic_lock);
 
diff --git a/xen/include/acpi/cpufreq/processor_perf.h b/xen/include/xen/processor_perf.h
similarity index 100%
rename from xen/include/acpi/cpufreq/processor_perf.h
rename to xen/include/xen/processor_perf.h
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 03/12] pmstat: move pmstat.c file to the xen/drivers/pm/stat.c location
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 01/12] cpufreq: move cpufreq.h file to the xen/include/xen location Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 02/12] pm: move processor_perf.h " Oleksandr Dmytryshyn
@ 2014-10-16 11:26 ` Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 04/12] cpufreq: make turbo settings to be configurable Oleksandr Dmytryshyn
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Cpufreq driver should be more generalizable (not ACPI-specific).
Thus this file should be placed to more convenient location.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/Rules.mk                             | 1 +
 xen/arch/x86/Rules.mk                    | 1 +
 xen/common/sysctl.c                      | 2 +-
 xen/drivers/Makefile                     | 1 +
 xen/drivers/acpi/Makefile                | 1 -
 xen/drivers/pm/Makefile                  | 1 +
 xen/drivers/{acpi/pmstat.c => pm/stat.c} | 0
 7 files changed, 5 insertions(+), 2 deletions(-)
 create mode 100644 xen/drivers/pm/Makefile
 rename xen/drivers/{acpi/pmstat.c => pm/stat.c} (100%)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 3a6cec5..b7caab6 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -55,6 +55,7 @@ CFLAGS-$(perfc)         += -DPERF_COUNTERS
 CFLAGS-$(perfc_arrays)  += -DPERF_ARRAYS
 CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
 CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
+CFLAGS-$(HAS_PM)        += -DHAS_PM
 CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
 CFLAGS-$(HAS_PASSTHROUGH) += -DHAS_PASSTHROUGH
 CFLAGS-$(HAS_DEVICE_TREE) += -DHAS_DEVICE_TREE
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 576985e..9e9fbf1 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -3,6 +3,7 @@
 
 HAS_IOPORTS := y
 HAS_ACPI := y
+HAS_PM := y
 HAS_VGA  := y
 HAS_VIDEO  := y
 HAS_CPUFREQ := y
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 0cb6ee1..0dcf06a 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -170,7 +170,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         op->u.availheap.avail_bytes <<= PAGE_SHIFT;
         break;
 
-#ifdef HAS_ACPI
+#ifdef HAS_PM
     case XEN_SYSCTL_get_pmstat:
         ret = do_get_pm_info(&op->u.get_pmstat);
         break;
diff --git a/xen/drivers/Makefile b/xen/drivers/Makefile
index 9c70f20..ee4fd4d 100644
--- a/xen/drivers/Makefile
+++ b/xen/drivers/Makefile
@@ -4,3 +4,4 @@ subdir-$(HAS_PCI) += pci
 subdir-$(HAS_PASSTHROUGH) += passthrough
 subdir-$(HAS_ACPI) += acpi
 subdir-$(HAS_VIDEO) += video
+subdir-$(HAS_PM) += pm
diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
index bbb06a7..0505742 100644
--- a/xen/drivers/acpi/Makefile
+++ b/xen/drivers/acpi/Makefile
@@ -5,7 +5,6 @@ subdir-$(x86) += apei
 obj-bin-y += tables.init.o
 obj-y += numa.o
 obj-y += osl.o
-obj-y += pmstat.o
 
 obj-$(x86) += hwregs.o
 obj-$(x86) += reboot.o
diff --git a/xen/drivers/pm/Makefile b/xen/drivers/pm/Makefile
new file mode 100644
index 0000000..2073683
--- /dev/null
+++ b/xen/drivers/pm/Makefile
@@ -0,0 +1 @@
+obj-y += stat.o
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/pm/stat.c
similarity index 100%
rename from xen/drivers/acpi/pmstat.c
rename to xen/drivers/pm/stat.c
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 04/12] cpufreq: make turbo settings to be configurable
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (2 preceding siblings ...)
  2014-10-16 11:26 ` [RFC PATCH v2 03/12] pmstat: move pmstat.c file to the xen/drivers/pm/stat.c location Oleksandr Dmytryshyn
@ 2014-10-16 11:26 ` Oleksandr Dmytryshyn
  2014-10-16 11:26 ` [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable Oleksandr Dmytryshyn
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

This settings is not needed for some architectures.
So make it to be configurable and use it for x86
architecture.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/Rules.mk                  |  1 +
 xen/arch/x86/Rules.mk         |  1 +
 xen/drivers/cpufreq/utility.c | 11 ++++++++++-
 xen/drivers/pm/stat.c         |  6 ++++++
 xen/include/xen/cpufreq.h     |  6 ++++++
 5 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index b7caab6..5953152 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -56,6 +56,7 @@ CFLAGS-$(perfc_arrays)  += -DPERF_ARRAYS
 CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
 CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
 CFLAGS-$(HAS_PM)        += -DHAS_PM
+CFLAGS-$(HAS_CPU_TURBO) += -DHAS_CPU_TURBO
 CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
 CFLAGS-$(HAS_PASSTHROUGH) += -DHAS_PASSTHROUGH
 CFLAGS-$(HAS_DEVICE_TREE) += -DHAS_DEVICE_TREE
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 9e9fbf1..cfe4f90 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -4,6 +4,7 @@
 HAS_IOPORTS := y
 HAS_ACPI := y
 HAS_PM := y
+HAS_CPU_TURBO := y
 HAS_VGA  := y
 HAS_VIDEO  := y
 HAS_CPUFREQ := y
diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c
index 3cb0b3e..e92cf17 100644
--- a/xen/drivers/cpufreq/utility.c
+++ b/xen/drivers/cpufreq/utility.c
@@ -209,7 +209,9 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
 {
     unsigned int min_freq = ~0;
     unsigned int max_freq = 0;
+#ifdef HAS_CPU_TURBO
     unsigned int second_max_freq = 0;
+#endif
     unsigned int i;
 
     for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
@@ -221,6 +223,7 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
         if (freq > max_freq)
             max_freq = freq;
     }
+#ifdef HAS_CPU_TURBO
     for (i=0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
         unsigned int freq = table[i].frequency;
         if (freq == CPUFREQ_ENTRY_INVALID || freq == max_freq)
@@ -234,9 +237,13 @@ int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
         printk("max_freq: %u    second_max_freq: %u\n",
                max_freq, second_max_freq);
 
+    policy->cpuinfo.second_max_freq = second_max_freq;
+#else
+    if (cpufreq_verbose)
+        printk("max_freq: %u\n", max_freq);
+#endif
     policy->min = policy->cpuinfo.min_freq = min_freq;
     policy->max = policy->cpuinfo.max_freq = max_freq;
-    policy->cpuinfo.second_max_freq = second_max_freq;
 
     if (policy->min == ~0)
         return -EINVAL;
@@ -390,6 +397,7 @@ int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag)
     return policy->cur;
 }
 
+#ifdef HAS_CPU_TURBO
 int cpufreq_update_turbo(int cpuid, int new_state)
 {
     struct cpufreq_policy *policy;
@@ -430,6 +438,7 @@ int cpufreq_get_turbo_status(int cpuid)
     policy = per_cpu(cpufreq_cpu_policy, cpuid);
     return policy && policy->turbo == CPUFREQ_TURBO_ENABLED;
 }
+#endif /* HAS_CPU_TURBO */
 
 /*********************************************************************
  *                 POLICY                                            *
diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
index 3486148..3154051 100644
--- a/xen/drivers/pm/stat.c
+++ b/xen/drivers/pm/stat.c
@@ -292,7 +292,11 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op)
             &op->u.get_para.u.ondemand.sampling_rate,
             &op->u.get_para.u.ondemand.up_threshold);
     }
+#ifdef HAS_CPU_TURBO
     op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid);
+#else
+    op->u.get_para.turbo_enabled = 0;
+#endif
 
     return ret;
 }
@@ -475,6 +479,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
         break;
     }
 
+#ifdef HAS_CPU_TURBO
     case XEN_SYSCTL_pm_op_enable_turbo:
     {
         ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_ENABLED);
@@ -486,6 +491,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
         ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_DISABLED);
         break;
     }
+#endif /* HAS_CPU_TURBO */
 
     default:
         printk("not defined sub-hypercall @ do_pm_op\n");
diff --git a/xen/include/xen/cpufreq.h b/xen/include/xen/cpufreq.h
index 82dc4dc..d7b6c34 100644
--- a/xen/include/xen/cpufreq.h
+++ b/xen/include/xen/cpufreq.h
@@ -39,7 +39,9 @@ extern struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];
 
 struct cpufreq_cpuinfo {
     unsigned int        max_freq;
+#ifdef HAS_CPU_TURBO
     unsigned int        second_max_freq;    /* P1 if Turbo Mode is on */
+#endif
     unsigned int        min_freq;
     unsigned int        transition_latency; /* in 10^(-9) s = nanoseconds */
 };
@@ -59,10 +61,12 @@ struct cpufreq_policy {
 
     bool_t              resume; /* flag for cpufreq 1st run
                                  * S3 wakeup, hotplug cpu, etc */
+#ifdef HAS_CPU_TURBO
     s8                  turbo;  /* tristate flag: 0 for unsupported
                                  * -1 for disable, 1 for enabled
                                  * See CPUFREQ_TURBO_* below for defines */
     bool_t              aperf_mperf; /* CPU has APERF/MPERF MSRs */
+#endif
 };
 DECLARE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_policy);
 
@@ -127,8 +131,10 @@ extern int cpufreq_driver_getavg(unsigned int cpu, unsigned int flag);
 #define CPUFREQ_TURBO_UNSUPPORTED   0
 #define CPUFREQ_TURBO_ENABLED       1
 
+#ifdef HAS_CPU_TURBO
 extern int cpufreq_update_turbo(int cpuid, int new_state);
 extern int cpufreq_get_turbo_status(int cpuid);
+#endif
 
 static __inline__ int 
 __cpufreq_governor(struct cpufreq_policy *policy, unsigned int event)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (3 preceding siblings ...)
  2014-10-16 11:26 ` [RFC PATCH v2 04/12] cpufreq: make turbo settings to be configurable Oleksandr Dmytryshyn
@ 2014-10-16 11:26 ` Oleksandr Dmytryshyn
  2014-10-17 12:26   ` Jan Beulich
  2014-10-16 11:27 ` [RFC PATCH v2 06/12] cpufreq: make cpufreq driver " Oleksandr Dmytryshyn
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:26 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

ACPI-specific parts are moved under appropriate ifdefs.
Now pmstat functions can be used in ARM platform.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/drivers/pm/stat.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
index 3154051..530a365 100644
--- a/xen/drivers/pm/stat.c
+++ b/xen/drivers/pm/stat.c
@@ -37,7 +37,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>
@@ -133,7 +132,8 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
         cpufreq_statistic_reset(op->cpuid);
         break;
     }
-
+/* For now those operations can be used when ACPI is enabled */
+#ifdef CONFIG_ACPI
     case PMSTAT_get_max_cx:
     {
         op->u.getcx.nr = pmstat_get_cx_nr(op->cpuid);
@@ -152,6 +152,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");
@@ -467,6 +468,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();
@@ -478,6 +480,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 HAS_CPU_TURBO
     case XEN_SYSCTL_pm_op_enable_turbo:
@@ -502,6 +505,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];
@@ -532,3 +536,4 @@ int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
 
     return ret;
 }
+#endif /* CONFIG_ACPI */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (4 preceding siblings ...)
  2014-10-16 11:26 ` [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  2014-10-17 12:31   ` Jan Beulich
  2014-10-16 11:27 ` [RFC PATCH v2 07/12] arch/arm: create device tree nodes for Dom0 cpufreq cpu driver Oleksandr Dmytryshyn
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

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.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/Rules.mk                     |  1 +
 xen/drivers/cpufreq/cpufreq.c    | 46 +++++++++++++++++++++++++++++++++++-----
 xen/include/xen/processor_perf.h |  7 ++++++
 3 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 5953152..3b0b89b 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -55,6 +55,7 @@ CFLAGS-$(perfc)         += -DPERF_COUNTERS
 CFLAGS-$(perfc_arrays)  += -DPERF_ARRAYS
 CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
 CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
+CFLAGS-$(HAS_CPUFREQ)   += -DHAS_CPUFREQ
 CFLAGS-$(HAS_PM)        += -DHAS_PM
 CFLAGS-$(HAS_CPU_TURBO) += -DHAS_CPU_TURBO
 CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index f5f4d75..1cf9119 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -43,9 +43,14 @@
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/percpu.h>
-#include <acpi/acpi.h>
 #include <xen/cpufreq.h>
 
+#ifdef CONFIG_ACPI
+    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
+#else
+    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
+#endif
+
 static unsigned int __read_mostly usr_min_freq;
 static unsigned int __read_mostly usr_max_freq;
 static void cpufreq_cmdline_common_para(struct cpufreq_policy *new_policy);
@@ -192,6 +197,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 !=
@@ -207,6 +213,18 @@ 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 */
     }
 
     if (!domexist || hw_all) {
@@ -363,6 +381,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, "
@@ -370,12 +389,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
 
 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,
@@ -385,15 +406,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 */
     }
 }
 
 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 */
 }
 
 static void print_PPC(unsigned int platform_limit)
@@ -407,7 +439,11 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *dom0_px_in
     struct processor_pminfo *pmpt;
     struct processor_performance *pxpt;
 
+#ifdef CONFIG_ACPI
     cpuid = get_cpu_id(acpi_id);
+#else
+    cpuid = acpi_id;
+#endif
     if ( cpuid < 0 || !dom0_px_info)
     {
         ret = -EINVAL;
@@ -429,6 +465,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;
 
@@ -455,7 +493,7 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *dom0_px_in
             print_PCT(&pxpt->status_register);
         }
     }
-
+#endif /* CONFIG_ACPI */
     if ( dom0_px_info->flags & XEN_PX_PSS ) 
     {
         /* capability check */
@@ -517,11 +555,9 @@ 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 ( dom0_px_info->flags == XEN_PX_FULL_INIT )
     {
         pxpt->init = XEN_PX_INIT;
-
         ret = cpufreq_cpu_init(cpuid);
         goto out;
     }
diff --git a/xen/include/xen/processor_perf.h b/xen/include/xen/processor_perf.h
index d8a1ba6..6c1279d 100644
--- a/xen/include/xen/processor_perf.h
+++ b/xen/include/xen/processor_perf.h
@@ -3,7 +3,10 @@
 
 #include <public/platform.h>
 #include <public/sysctl.h>
+
+#ifdef CONFIG_ACPI
 #include <xen/acpi.h>
+#endif
 
 #define XEN_PX_INIT 0x80000000
 
@@ -24,8 +27,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 +40,10 @@ struct processor_performance {
 };
 
 struct processor_pminfo {
+#ifdef CONFIG_ACPI
     uint32_t acpi_id;
     uint32_t id;
+#endif
     struct processor_performance    perf;
 };
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 07/12] arch/arm: create device tree nodes for Dom0 cpufreq cpu driver
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (5 preceding siblings ...)
  2014-10-16 11:27 ` [RFC PATCH v2 06/12] cpufreq: make cpufreq driver " Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  2014-10-16 11:27 ` [RFC PATCH v2 08/12] xsm: enable xsm_platform_op hook for all architectures Oleksandr Dmytryshyn
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

This patch copies all cpu@0..cpu@N nodes (from input
device tree) with properties to /hypervisor/pcpus
node (device tree for Dom0). Thus we can give all
information about all physical CPUs in the pcpus node.
Driver in Dom0 should parse /hypervisor/pcpus path
instead of the /cpus path in the device tree.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/arch/arm/domain_build.c | 67 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2db0236..2186514 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -326,6 +326,69 @@ static int make_memory_node(const struct domain *d,
     return res;
 }
 
+#ifdef HAS_CPUFREQ
+static int fdt_copy_phys_cpus_nodes(void *fdt)
+{
+    int res;
+    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
+    const struct dt_device_node *npcpu;
+    const struct dt_property *pp;
+    char *node_name;
+
+    if ( !cpus )
+    {
+        dprintk(XENLOG_ERR, "Missing /cpus node in the device tree?\n");
+        return -ENOENT;
+    }
+
+    /*
+     * create pcpus node and copy to it
+     * original cpu@0..cpu@N nodes with its properties.
+     * This is needed for the cpufreq cpu driver in Dom0
+     */
+    DPRINT("Create pcpus node\n");
+
+    res = fdt_begin_node(fdt, "pcpus");
+    if ( res )
+        return res;
+
+    dt_for_each_child_node( cpus, npcpu )
+    {
+        if ( dt_device_type_is_equal(npcpu, "cpu") )
+        {
+            node_name = strrchr(dt_node_full_name(npcpu), '/');
+            node_name++;
+
+            ASSERT(node_name && *node_name);
+
+            DPRINT("Copy %s node to the pcpus\n", node_name);
+
+            res = fdt_begin_node(fdt, node_name);
+            if ( res )
+                return res;
+
+            dt_for_each_property_node( npcpu, pp )
+            {
+                if ( pp->length )
+                {
+                    res = fdt_property(fdt, pp->name, pp->value,
+                                        pp->length);
+                    if ( res )
+                        return res;
+                }
+            }
+
+            res = fdt_end_node(fdt);
+            if ( res )
+                return res;
+        }
+    }
+
+    res = fdt_end_node(fdt);
+    return res;
+}
+#endif /* HAS_CPUFREQ */
+
 static int make_hypervisor_node(struct domain *d,
                                 void *fdt, const struct dt_device_node *parent)
 {
@@ -386,6 +449,10 @@ static int make_hypervisor_node(struct domain *d,
     if ( res )
         return res;
 
+    #ifdef HAS_CPUFREQ
+    fdt_copy_phys_cpus_nodes(fdt);
+    #endif
+
     res = fdt_end_node(fdt);
 
     return res;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 08/12] xsm: enable xsm_platform_op hook for all architectures
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (6 preceding siblings ...)
  2014-10-16 11:27 ` [RFC PATCH v2 07/12] arch/arm: create device tree nodes for Dom0 cpufreq cpu driver Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  2014-10-16 11:27 ` [RFC PATCH v2 09/12] xen: arm: implement platform hypercall Oleksandr Dmytryshyn
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

This hook is used by platform hypercall which will
be implemented for all architectures.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/include/xsm/dummy.h | 12 ++++++------
 xen/include/xsm/xsm.h   | 10 +++++-----
 xen/xsm/flask/hooks.c   |  3 ++-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index eb9e1a1..911fb5d 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -491,6 +491,12 @@ static XSM_INLINE int xsm_hvm_param_nested(XSM_DEFAULT_ARG struct domain *d)
     return xsm_default_action(action, current->domain, d);
 }
 
+static XSM_INLINE int xsm_platform_op(XSM_DEFAULT_ARG uint32_t op)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    return xsm_default_action(action, current->domain, NULL);
+}
+
 #ifdef CONFIG_X86
 static XSM_INLINE int xsm_shadow_control(XSM_DEFAULT_ARG struct domain *d, uint32_t op)
 {
@@ -546,12 +552,6 @@ static XSM_INLINE int xsm_apic(XSM_DEFAULT_ARG struct domain *d, int cmd)
     return xsm_default_action(action, d, NULL);
 }
 
-static XSM_INLINE int xsm_platform_op(XSM_DEFAULT_ARG uint32_t op)
-{
-    XSM_ASSERT_ACTION(XSM_PRIV);
-    return xsm_default_action(action, current->domain, NULL);
-}
-
 static XSM_INLINE int xsm_machine_memory_map(XSM_DEFAULT_VOID)
 {
     XSM_ASSERT_ACTION(XSM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 1939453..5cb1e0d 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -509,6 +509,11 @@ static inline int xsm_hvm_param_nested (xsm_default_t def, struct domain *d)
     return xsm_ops->hvm_param_nested(d);
 }
 
+static inline int xsm_platform_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->platform_op(op);
+}
+
 #ifdef CONFIG_X86
 static inline int xsm_shadow_control (xsm_default_t def, struct domain *d, uint32_t op)
 {
@@ -560,11 +565,6 @@ static inline int xsm_memtype (xsm_default_t def, uint32_t access)
     return xsm_ops->memtype(access);
 }
 
-static inline int xsm_platform_op (xsm_default_t def, uint32_t op)
-{
-    return xsm_ops->platform_op(op);
-}
-
 static inline int xsm_machine_memory_map(xsm_default_t def)
 {
     return xsm_ops->machine_memory_map();
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index d94ab77..29126ec 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1542,6 +1542,8 @@ static struct xsm_operations flask_ops = {
     .add_to_physmap = flask_add_to_physmap,
     .remove_from_physmap = flask_remove_from_physmap,
 
+    .platform_op = flask_platform_op,
+
 #ifdef CONFIG_X86
     .shadow_control = flask_shadow_control,
     .hvm_set_pci_intx_level = flask_hvm_set_pci_intx_level,
@@ -1552,7 +1554,6 @@ static struct xsm_operations flask_ops = {
     .mem_event_op = flask_mem_event_op,
     .mem_sharing_op = flask_mem_sharing_op,
     .apic = flask_apic,
-    .platform_op = flask_platform_op,
     .machine_memory_map = flask_machine_memory_map,
     .domain_memory_map = flask_domain_memory_map,
     .mmu_update = flask_mmu_update,
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 09/12] xen: arm: implement platform hypercall
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (7 preceding siblings ...)
  2014-10-16 11:27 ` [RFC PATCH v2 08/12] xsm: enable xsm_platform_op hook for all architectures Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  2014-10-17 12:44   ` Jan Beulich
  2014-10-16 11:27 ` [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver Oleksandr Dmytryshyn
                   ` (2 subsequent siblings)
  11 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/arch/arm/Makefile                    |  1 +
 xen/arch/arm/platform_hypercall.c        | 57 ++++++++++++++++++++++++++
 xen/arch/arm/traps.c                     |  1 +
 xen/arch/x86/platform_hypercall.c        | 29 ++-----------
 xen/arch/x86/x86_64/platform_hypercall.c |  2 +
 xen/common/Makefile                      |  1 +
 xen/common/platform_hypercall.c          | 70 ++++++++++++++++++++++++++++++++
 7 files changed, 135 insertions(+), 26 deletions(-)
 create mode 100644 xen/arch/arm/platform_hypercall.c
 create mode 100644 xen/common/platform_hypercall.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index d70f6d5..54d8258 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -32,6 +32,7 @@ obj-y += vuart.o
 obj-y += hvm.o
 obj-y += device.o
 obj-y += decode.o
+obj-y += platform_hypercall.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
new file mode 100644
index 0000000..aa870b4
--- /dev/null
+++ b/xen/arch/arm/platform_hypercall.c
@@ -0,0 +1,57 @@
+/******************************************************************************
+ * platform_hypercall.c
+ *
+ * Hardware platform operations. Intended for use by domain-0 kernel.
+ *
+ * Copyright (c) 2014 GlobalLogic Inc.
+ */
+
+#include <xen/config.h>
+#include <xen/types.h>
+#include <xen/sched.h>
+#include <xen/pmstat.h>
+#include <public/platform.h>
+
+long arch_do_platform_op(struct xen_platform_op *platform_op,
+                          XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
+{
+    long ret = 0;
+    struct xen_platform_op *op = platform_op;
+
+    switch ( op->cmd )
+    {
+    case XENPF_set_processor_pminfo:
+        switch ( op->u.set_pminfo.type )
+        {
+        case XEN_PM_PX:
+            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
+            {
+                ret = -ENOSYS;
+                break;
+           }
+#ifdef HAS_CPUFREQ
+            ret = set_px_pminfo(op->u.set_pminfo.id, &op->u.set_pminfo.u.perf);
+#else
+            ret = -EINVAL;
+#endif
+            break;
+
+        default:
+            ret = -EINVAL;
+            break;
+        }
+        break;
+    }
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 21c7b26..d1b0014 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1012,6 +1012,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(hvm_op, 2),
     HYPERCALL(grant_table_op, 3),
     HYPERCALL_ARM(vcpu_op, 3),
+    HYPERCALL(platform_op, 1),
 };
 
 typedef int (*arm_psci_fn_t)(uint32_t, register_t);
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 7ce8592..98956ff 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -35,15 +35,12 @@
 
 #ifndef COMPAT
 typedef long ret_t;
-DEFINE_SPINLOCK(xenpf_lock);
 # undef copy_from_compat
 # define copy_from_compat copy_from_guest
 # undef copy_to_compat
 # define copy_to_compat copy_to_guest
 # undef guest_from_compat_handle
 # define guest_from_compat_handle(x,y) ((x)=(y))
-#else
-extern spinlock_t xenpf_lock;
 #endif
 
 static DEFINE_PER_CPU(uint64_t, freq);
@@ -61,30 +58,11 @@ long cpu_down_helper(void *data);
 long core_parking_helper(void *data);
 uint32_t get_cur_idle_nums(void);
 
-ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
+ret_t arch_do_platform_op(struct xen_platform_op *platform_op,
+                          XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
 {
     ret_t ret = 0;
-    struct xen_platform_op curop, *op = &curop;
-
-    if ( copy_from_guest(op, u_xenpf_op, 1) )
-        return -EFAULT;
-
-    if ( op->interface_version != XENPF_INTERFACE_VERSION )
-        return -EACCES;
-
-    ret = xsm_platform_op(XSM_PRIV, op->cmd);
-    if ( ret )
-        return ret;
-
-    /*
-     * Trylock here avoids deadlock with an existing platform critical section
-     * which might (for some current or future reason) want to synchronise
-     * with this vcpu.
-     */
-    while ( !spin_trylock(&xenpf_lock) )
-        if ( hypercall_preempt_check() )
-            return hypercall_create_continuation(
-                __HYPERVISOR_platform_op, "h", u_xenpf_op);
+    struct xen_platform_op *op = platform_op;
 
     switch ( op->cmd )
     {
@@ -607,7 +585,6 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
     }
 
  out:
-    spin_unlock(&xenpf_lock);
 
     return ret;
 }
diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c
index b6f380e..655fbb2 100644
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -10,6 +10,7 @@ DEFINE_XEN_GUEST_HANDLE(compat_platform_op_t);
 #define xen_platform_op     compat_platform_op
 #define xen_platform_op_t   compat_platform_op_t
 #define do_platform_op(x)   compat_platform_op(_##x)
+#define arch_do_platform_op(x, y)   arch_compat_platform_op(x, y)
 
 #define efi_get_info        efi_compat_get_info
 #define efi_runtime_call(x) efi_compat_runtime_call(x)
@@ -37,6 +38,7 @@ CHECK_pf_enter_acpi_sleep;
 #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
 typedef int ret_t;
 
+#include "../../../common/platform_hypercall.c"
 #include "../platform_hypercall.c"
 
 /*
diff --git a/xen/common/Makefile b/xen/common/Makefile
index 3683ae3..455bda6 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -51,6 +51,7 @@ obj-y += tmem_xen.o
 obj-y += radix-tree.o
 obj-y += rbtree.o
 obj-y += lzo.o
+obj-y += platform_hypercall.o
 
 obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma unlzo unlz4 earlycpio,$(n).init.o)
 
diff --git a/xen/common/platform_hypercall.c b/xen/common/platform_hypercall.c
new file mode 100644
index 0000000..e562660
--- /dev/null
+++ b/xen/common/platform_hypercall.c
@@ -0,0 +1,70 @@
+/******************************************************************************
+ * platform_hypercall.c
+ *
+ * Hardware platform operations. Intended for use by domain-0 kernel.
+ *
+ * Copyright (c) 2002-2006, K Fraser
+ */
+
+#include <xen/config.h>
+#include <xen/types.h>
+#include <xen/lib.h>
+#include <xen/event.h>
+#include <xen/iocap.h>
+#include <xen/guest_access.h>
+#include <public/platform.h>
+#include <xsm/xsm.h>
+
+#ifndef COMPAT
+typedef long ret_t;
+DEFINE_SPINLOCK(xenpf_lock);
+#else
+extern spinlock_t xenpf_lock;
+#endif
+
+extern ret_t
+arch_do_platform_op(
+    struct xen_platform_op *platform_op,
+    XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op);
+
+ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
+{
+    ret_t ret = 0;
+    struct xen_platform_op curop, *op = &curop;
+
+    if ( copy_from_guest(op, u_xenpf_op, 1) )
+        return -EFAULT;
+
+    if ( op->interface_version != XENPF_INTERFACE_VERSION )
+        return -EACCES;
+
+    ret = xsm_platform_op(XSM_PRIV, op->cmd);
+    if ( ret )
+        return ret;
+
+    /*
+     * Trylock here avoids deadlock with an existing platform critical section
+     * which might (for some current or future reason) want to synchronise
+     * with this vcpu.
+     */
+    while ( !spin_trylock(&xenpf_lock) )
+        if ( hypercall_preempt_check() )
+            return hypercall_create_continuation(
+                __HYPERVISOR_platform_op, "h", u_xenpf_op);
+
+    ret = arch_do_platform_op(op, u_xenpf_op);
+
+    spin_unlock(&xenpf_lock);
+
+    return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (8 preceding siblings ...)
  2014-10-16 11:27 ` [RFC PATCH v2 09/12] xen: arm: implement platform hypercall Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  2014-10-17 13:03   ` Jan Beulich
  2014-10-16 11:27 ` [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op Oleksandr Dmytryshyn
  2014-10-16 11:27 ` [RFC PATCH v2 12/12] xen/arm: enable cpufreq functionality for ARM Oleksandr Dmytryshyn
  11 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

This driver uses Dom0 to change frequencies on CPUs

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/Rules.mk                       |   1 +
 xen/drivers/cpufreq/Makefile       |   1 +
 xen/drivers/cpufreq/dom0-cpufreq.c | 241 +++++++++++++++++++++++++++++++++++++
 xen/include/public/xen.h           |   1 +
 4 files changed, 244 insertions(+)
 create mode 100644 xen/drivers/cpufreq/dom0-cpufreq.c

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 3b0b89b..8104f7b 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -56,6 +56,7 @@ CFLAGS-$(perfc_arrays)  += -DPERF_ARRAYS
 CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
 CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
 CFLAGS-$(HAS_CPUFREQ)   += -DHAS_CPUFREQ
+CFLAGS-$(HAS_DOM0_CPUFREQ) += -DHAS_DOM0_CPUFREQ
 CFLAGS-$(HAS_PM)        += -DHAS_PM
 CFLAGS-$(HAS_CPU_TURBO) += -DHAS_CPU_TURBO
 CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
diff --git a/xen/drivers/cpufreq/Makefile b/xen/drivers/cpufreq/Makefile
index b87d127..172bd51 100644
--- a/xen/drivers/cpufreq/Makefile
+++ b/xen/drivers/cpufreq/Makefile
@@ -2,3 +2,4 @@ obj-y += cpufreq.o
 obj-y += cpufreq_ondemand.o
 obj-y += cpufreq_misc_governors.o
 obj-y += utility.o
+obj-$(HAS_DOM0_CPUFREQ) += dom0-cpufreq.o
diff --git a/xen/drivers/cpufreq/dom0-cpufreq.c b/xen/drivers/cpufreq/dom0-cpufreq.c
new file mode 100644
index 0000000..f4274d7
--- /dev/null
+++ b/xen/drivers/cpufreq/dom0-cpufreq.c
@@ -0,0 +1,241 @@
+/*
+ *  Copyright (C) 2014 GlobalLogic Inc.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <xen/types.h>
+#include <xen/errno.h>
+#include <xen/sched.h>
+#include <xen/event.h>
+#include <xen/irq.h>
+#include <xen/spinlock.h>
+#include <xen/cpufreq.h>
+#include <asm/current.h>
+
+struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];
+static DEFINE_SPINLOCK(sysctl_cpufreq_lock);
+
+struct sysctl_cpufreq_data {
+    uint32_t cpu;
+    uint32_t freq;
+    uint32_t relation;
+    int32_t result;
+};
+
+static struct sysctl_cpufreq_data sysctl_cpufreq_data;
+
+int cpufreq_cpu_init(unsigned int cpuid)
+{
+    return cpufreq_add_cpu(cpuid);
+}
+
+static void notify_cpufreq_domains(void)
+{
+    send_global_virq(VIRQ_CPUFREQ);
+}
+
+static int dom0_cpufreq_verify(struct cpufreq_policy *policy)
+{
+    struct acpi_cpufreq_data *data;
+    struct processor_performance *perf;
+
+    if ( !policy || !(data = cpufreq_drv_data[policy->cpu]) ||
+         !processor_pminfo[policy->cpu] )
+        return -EINVAL;
+
+    perf = &processor_pminfo[policy->cpu]->perf;
+
+    cpufreq_verify_within_limits(policy, 0,
+        perf->states[perf->platform_limit].core_frequency * 1000);
+
+    return cpufreq_frequency_table_verify(policy, data->freq_table);
+}
+
+static int dom0_cpufreq_target(struct cpufreq_policy *policy,
+                               unsigned int target_freq, unsigned int relation)
+{
+    struct acpi_cpufreq_data *data = cpufreq_drv_data[policy->cpu];
+    struct processor_performance *perf;
+    struct cpufreq_freqs freqs;
+    cpumask_t online_policy_cpus;
+    unsigned int next_state = 0; /* Index into freq_table */
+    unsigned int next_perf_state = 0; /* Index into perf table */
+    unsigned int j;
+    int ret = 0;
+
+    if ( unlikely(data == NULL ||
+         data->acpi_data == NULL || data->freq_table == NULL) )
+    {
+        return -ENODEV;
+    }
+
+    perf = data->acpi_data;
+    ret = cpufreq_frequency_table_target(policy,
+                                            data->freq_table,
+                                            target_freq,
+                                            relation, &next_state);
+    if ( unlikely(ret) )
+        return -ENODEV;
+
+    cpumask_and(&online_policy_cpus, &cpu_online_map, policy->cpus);
+
+    next_perf_state = data->freq_table[next_state].index;
+    if ( perf->state == next_perf_state )
+    {
+        if ( unlikely(policy->resume) )
+            policy->resume = 0;
+        else
+            return 0;
+    }
+
+    freqs.old = perf->states[perf->state].core_frequency * 1000;
+    freqs.new = data->freq_table[next_state].frequency;
+
+    /* Do send cmd for Dom0 */
+    spin_lock(&sysctl_cpufreq_lock);
+    /* return previous result */
+    ret = sysctl_cpufreq_data.result;
+
+    sysctl_cpufreq_data.cpu = policy->cpu;
+    sysctl_cpufreq_data.freq = freqs.new;
+    sysctl_cpufreq_data.relation = (uint32_t)relation;
+    spin_unlock(&sysctl_cpufreq_lock);
+    notify_cpufreq_domains();
+
+    for_each_cpu( j, &online_policy_cpus )
+        cpufreq_statistic_update(j, perf->state, next_perf_state);
+
+    perf->state = next_perf_state;
+    policy->cur = freqs.new;
+
+    return ret;
+}
+
+static int
+dom0_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+    struct processor_performance *perf;
+    struct acpi_cpufreq_data *data;
+    unsigned int cpu = policy->cpu;
+    unsigned int valid_states = 0;
+    int i;
+    int ret = 0;
+
+    data = xzalloc(struct acpi_cpufreq_data);
+    if ( !data )
+        return -ENOMEM;
+
+    cpufreq_drv_data[cpu] = data;
+
+    data->acpi_data = &processor_pminfo[cpu]->perf;
+
+    perf = data->acpi_data;
+    policy->shared_type = perf->shared_type;
+
+    data->freq_table = xmalloc_array(struct cpufreq_frequency_table,
+                                    (perf->state_count+1));
+    if ( !data->freq_table )
+    {
+        ret = -ENOMEM;
+        goto err_unreg;
+    }
+
+    /* detect transition latency */
+    policy->cpuinfo.transition_latency = 0;
+    for ( i=0; i<perf->state_count; i++ )
+    {
+        if ( (perf->states[i].transition_latency * 1000) >
+             policy->cpuinfo.transition_latency )
+            policy->cpuinfo.transition_latency =
+                perf->states[i].transition_latency * 1000;
+    }
+
+    policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR;
+
+    /* table init */
+    for ( i=0; i<perf->state_count; i++ )
+    {
+        if ( i>0 && perf->states[i].core_frequency >=
+            data->freq_table[valid_states-1].frequency / 1000 )
+            continue;
+
+        data->freq_table[valid_states].index = i;
+        data->freq_table[valid_states].frequency =
+            perf->states[i].core_frequency * 1000;
+        valid_states++;
+    }
+    data->freq_table[valid_states].frequency = CPUFREQ_TABLE_END;
+    perf->state = 0;
+
+    ret = cpufreq_frequency_table_cpuinfo(policy, data->freq_table);
+    if ( ret )
+        goto err_freqfree;
+
+
+    /*
+     * the first call to ->target() should result in us actually
+     * send command to the Dom0 to set frequency.
+     */
+    policy->resume = 1;
+
+    /* Set the minimal frequency */
+    return dom0_cpufreq_target(policy, policy->min, CPUFREQ_RELATION_L);
+
+err_freqfree:
+    xfree(data->freq_table);
+err_unreg:
+    xfree(data);
+    cpufreq_drv_data[cpu] = NULL;
+
+    return ret;
+}
+
+static int dom0_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+    struct acpi_cpufreq_data *data = cpufreq_drv_data[policy->cpu];
+
+    if ( data )
+    {
+        cpufreq_drv_data[policy->cpu] = NULL;
+        xfree(data->freq_table);
+        xfree(data);
+    }
+
+    return 0;
+}
+
+static struct cpufreq_driver dom0_cpufreq_driver = {
+    .name   = "dom0-cpufreq",
+    .verify = dom0_cpufreq_verify,
+    .target = dom0_cpufreq_target,
+    .init   = dom0_cpufreq_cpu_init,
+    .exit   = dom0_cpufreq_cpu_exit,
+};
+
+int __init dom0_cpufreq_driver_init(void)
+{
+    int ret = 0;
+
+    if ( cpufreq_controller == FREQCTL_xen )
+        ret = cpufreq_register_driver(&dom0_cpufreq_driver);
+
+    return ret;
+}
+
+__initcall(dom0_cpufreq_driver_init);
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 8c5697e..14a593d 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -160,6 +160,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #define VIRQ_MEM_EVENT  10 /* G. (DOM0) A memory event has occured           */
 #define VIRQ_XC_RESERVED 11 /* G. Reserved for XenClient                     */
 #define VIRQ_ENOMEM     12 /* G. (DOM0) Low on heap memory       */
+#define VIRQ_CPUFREQ    13 /* G. (DOM0) Notify dom0-cpufreq driver.           */
 
 /* Architecture-specific VIRQ definitions. */
 #define VIRQ_ARCH_0    16
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (9 preceding siblings ...)
  2014-10-16 11:27 ` [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  2014-10-17 13:21   ` Jan Beulich
  2014-10-16 11:27 ` [RFC PATCH v2 12/12] xen/arm: enable cpufreq functionality for ARM Oleksandr Dmytryshyn
  11 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Kernel uses this op to get some parameters for the
xen-cpufreq driver to change CPUs frequency.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/common/sysctl.c                |  8 ++++++++
 xen/drivers/cpufreq/dom0-cpufreq.c | 26 ++++++++++++++++++++++++++
 xen/include/public/sysctl.h        | 19 +++++++++++++++++++
 xen/include/xen/cpufreq.h          |  5 +++++
 4 files changed, 58 insertions(+)

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 0dcf06a..d90b3c0 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -27,6 +27,7 @@
 #include <xsm/xsm.h>
 #include <xen/pmstat.h>
 #include <xen/gcov.h>
+#include <xen/cpufreq.h>
 
 long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 {
@@ -362,6 +363,13 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
         break;
 #endif
 
+#ifdef HAS_DOM0_CPUFREQ
+    case XEN_SYSCTL_cpufreq_op:
+        ret = sysctl_cpufreq_op(&op->u.cpufreq_op);
+        copyback = 1;
+        break;
+#endif
+
     default:
         ret = arch_do_sysctl(op, u_sysctl);
         copyback = 0;
diff --git a/xen/drivers/cpufreq/dom0-cpufreq.c b/xen/drivers/cpufreq/dom0-cpufreq.c
index f4274d7..a55cd64 100644
--- a/xen/drivers/cpufreq/dom0-cpufreq.c
+++ b/xen/drivers/cpufreq/dom0-cpufreq.c
@@ -50,6 +50,32 @@ static void notify_cpufreq_domains(void)
     send_global_virq(VIRQ_CPUFREQ);
 }
 
+int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
+{
+    int ret = 0;
+    switch ( op->cmd )
+    {
+    case XEN_SYSCTL_CPUFREQ_get_target:
+        spin_lock(&sysctl_cpufreq_lock);
+        op->u.target.cpu = sysctl_cpufreq_data.cpu;
+        op->u.target.freq = sysctl_cpufreq_data.freq;
+        op->u.target.relation = sysctl_cpufreq_data.relation;
+        spin_unlock(&sysctl_cpufreq_lock);
+        break;
+
+    case XEN_SYSCTL_CPUFREQ_set_result:
+        spin_lock(&sysctl_cpufreq_lock);
+        sysctl_cpufreq_data.result = op->u.result;
+        spin_unlock(&sysctl_cpufreq_lock);
+        break;
+
+    default:
+        return -ENOSYS;
+        break;
+    }
+    return ret;
+}
+
 static int dom0_cpufreq_verify(struct cpufreq_policy *policy)
 {
     struct acpi_cpufreq_data *data;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 8437d31..ecd4674 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -632,6 +632,23 @@ struct xen_sysctl_coverage_op {
 typedef struct xen_sysctl_coverage_op xen_sysctl_coverage_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
 
+#define XEN_SYSCTL_CPUFREQ_get_target      0
+#define XEN_SYSCTL_CPUFREQ_set_result      1
+
+struct xen_sysctl_cpufreq_op {
+    uint32_t cmd;
+    union {
+        struct {
+            uint32_t cpu;
+            uint32_t freq;
+            uint32_t relation;
+        } target;
+        uint32_t result;
+    } u;
+};
+typedef struct xen_sysctl_cpufreq_op xen_sysctl_cpufreq_op_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpufreq_op_t);
+
 
 struct xen_sysctl {
     uint32_t cmd;
@@ -654,6 +671,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_cpupool_op                    18
 #define XEN_SYSCTL_scheduler_op                  19
 #define XEN_SYSCTL_coverage_op                   20
+#define XEN_SYSCTL_cpufreq_op                    21
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -675,6 +693,7 @@ struct xen_sysctl {
         struct xen_sysctl_cpupool_op        cpupool_op;
         struct xen_sysctl_scheduler_op      scheduler_op;
         struct xen_sysctl_coverage_op       coverage_op;
+        struct xen_sysctl_cpufreq_op        cpufreq_op;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/cpufreq.h b/xen/include/xen/cpufreq.h
index d7b6c34..2094b29 100644
--- a/xen/include/xen/cpufreq.h
+++ b/xen/include/xen/cpufreq.h
@@ -20,6 +20,7 @@
 #include <xen/spinlock.h>
 #include <xen/errno.h>
 #include <xen/cpumask.h>
+#include <public/sysctl.h>
 
 #include <xen/processor_perf.h>
 
@@ -264,4 +265,8 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq);
 void cpufreq_dbs_timer_suspend(void);
 void cpufreq_dbs_timer_resume(void);
 
+#ifdef HAS_DOM0_CPUFREQ
+int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op);
+#endif
+
 #endif /* __XEN_CPUFREQ_PM_H__ */
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [RFC PATCH v2 12/12] xen/arm: enable cpufreq functionality for ARM
  2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
                   ` (10 preceding siblings ...)
  2014-10-16 11:27 ` [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op Oleksandr Dmytryshyn
@ 2014-10-16 11:27 ` Oleksandr Dmytryshyn
  11 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-16 11:27 UTC (permalink / raw)
  To: xen-devel; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
---
 xen/arch/arm/Rules.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 718cd8a..8d150d5 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -9,6 +9,9 @@
 HAS_DEVICE_TREE := y
 HAS_VIDEO := y
 HAS_ARM_HDLCD := y
+HAS_CPUFREQ := y
+HAS_DOM0_CPUFREQ := y
+HAS_PM := y
 
 CFLAGS += -I$(BASEDIR)/include
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable
  2014-10-16 11:26 ` [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable Oleksandr Dmytryshyn
@ 2014-10-17 12:26   ` Jan Beulich
  2014-10-22  8:38     ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-17 12:26 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 16.10.14 at 13:26, <oleksandr.dmytryshyn@globallogic.com> wrote:
> @@ -133,7 +132,8 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
>          cpufreq_statistic_reset(op->cpuid);
>          break;
>      }
> -
> +/* For now those operations can be used when ACPI is enabled */
> +#ifdef CONFIG_ACPI
>      case PMSTAT_get_max_cx:
>      {
>          op->u.getcx.nr = pmstat_get_cx_nr(op->cpuid);

Don't remove blank lines like this please. Also I'm not sure the comment
makes sense as is - is there perhaps an "only" missing?

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-16 11:27 ` [RFC PATCH v2 06/12] cpufreq: make cpufreq driver " Oleksandr Dmytryshyn
@ 2014-10-17 12:31   ` Jan Beulich
  2014-10-22  8:39     ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-17 12:31 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -43,9 +43,14 @@
>  #include <asm/io.h>
>  #include <asm/processor.h>
>  #include <asm/percpu.h>
> -#include <acpi/acpi.h>
>  #include <xen/cpufreq.h>

I can see this being moved into a #ifdef CONFIG_ACPI section, but
removing it altogether seems kind of wrong.

> +#ifdef CONFIG_ACPI
> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
> +#else
> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
> +#endif

I'm not really understanding what is ACPI-specific about PCT but not
any of the other three. Please give some explanation in the commit
description.

Also if you want to use any kind of indentation for preprocessor
directives, please use

# define

(i.e. keep the # at the first column).

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 09/12] xen: arm: implement platform hypercall
  2014-10-16 11:27 ` [RFC PATCH v2 09/12] xen: arm: implement platform hypercall Oleksandr Dmytryshyn
@ 2014-10-17 12:44   ` Jan Beulich
  2014-10-22  8:40     ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-17 12:44 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn, xen-devel
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell

>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> +long arch_do_platform_op(struct xen_platform_op *platform_op,
> +                          XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> +{
> +    long ret = 0;
> +    struct xen_platform_op *op = platform_op;
> +
> +    switch ( op->cmd )
> +    {
> +    case XENPF_set_processor_pminfo:
> +        switch ( op->u.set_pminfo.type )
> +        {
> +        case XEN_PM_PX:
> +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
> +            {
> +                ret = -ENOSYS;
> +                break;
> +           }
> +#ifdef HAS_CPUFREQ
> +            ret = set_px_pminfo(op->u.set_pminfo.id, &op->u.set_pminfo.u.perf);

This cannot possibly be HAS_CPUFREQ, since ->u.set_pminfo
specifically uses ACPI notation for e.g. the CPU numbering. And
with that it becomes questionable whether the whole series is
doing right in abstracting away ACPI.

> @@ -37,6 +38,7 @@ CHECK_pf_enter_acpi_sleep;
>  #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
>  typedef int ret_t;
>  
> +#include "../../../common/platform_hypercall.c"
>  #include "../platform_hypercall.c"

This needs to be done properly in common/.

> +extern ret_t
> +arch_do_platform_op(
> +    struct xen_platform_op *platform_op,
> +    XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op);

Declarations belong in header files except in very special cases
(which this is none of).

> +ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> +{
> +    ret_t ret = 0;
> +    struct xen_platform_op curop, *op = &curop;
> +
> +    if ( copy_from_guest(op, u_xenpf_op, 1) )
> +        return -EFAULT;
> +
> +    if ( op->interface_version != XENPF_INTERFACE_VERSION )
> +        return -EACCES;
> +
> +    ret = xsm_platform_op(XSM_PRIV, op->cmd);
> +    if ( ret )
> +        return ret;
> +
> +    /*
> +     * Trylock here avoids deadlock with an existing platform critical section
> +     * which might (for some current or future reason) want to synchronise
> +     * with this vcpu.
> +     */
> +    while ( !spin_trylock(&xenpf_lock) )
> +        if ( hypercall_preempt_check() )
> +            return hypercall_create_continuation(
> +                __HYPERVISOR_platform_op, "h", u_xenpf_op);
> +
> +    ret = arch_do_platform_op(op, u_xenpf_op);
> +
> +    spin_unlock(&xenpf_lock);

And seeing this I really wonder whether making this common is
really worthwhile.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-16 11:27 ` [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver Oleksandr Dmytryshyn
@ 2014-10-17 13:03   ` Jan Beulich
  2014-10-22  8:43     ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-17 13:03 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> This driver uses Dom0 to change frequencies on CPUs

This surely is too little of an explanation, not the least because this
approach is still being argued about. Regardless of me not being in
favor of the approach, a couple of comments:

> +#include <xen/types.h>
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/event.h>
> +#include <xen/irq.h>
> +#include <xen/spinlock.h>
> +#include <xen/cpufreq.h>
> +#include <asm/current.h>
> +
> +struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];

static (and renamed) or you need to abstract out the variable from
the other driver. In no case should there be two definitions of the
same variable.

> +static void notify_cpufreq_domains(void)

Why "domains", not "hwdom"? You don't really want to send this
to other than the hardware domain I hope.

> +    /* Do send cmd for Dom0 */
> +    spin_lock(&sysctl_cpufreq_lock);
> +    /* return previous result */
> +    ret = sysctl_cpufreq_data.result;
> +
> +    sysctl_cpufreq_data.cpu = policy->cpu;
> +    sysctl_cpufreq_data.freq = freqs.new;
> +    sysctl_cpufreq_data.relation = (uint32_t)relation;
> +    spin_unlock(&sysctl_cpufreq_lock);

sysctl_cpufreq_data fields appear to be accessed only here. Is the
patch incomplete? It is certainly not possible to understand how this
is intended to work without the other sides being there.

> +static int
> +dom0_cpufreq_cpu_init(struct cpufreq_policy *policy)

Please avoid the term dom0 where possible, and where not truly
meaning dom0. The tree got switched to use hwdom instead a
while ago.

> +    data->freq_table = xmalloc_array(struct cpufreq_frequency_table,
> +                                    (perf->state_count+1));

Why count+1 here but ...

> +    if ( !data->freq_table )
> +    {
> +        ret = -ENOMEM;
> +        goto err_unreg;
> +    }
> +
> +    /* detect transition latency */
> +    policy->cpuinfo.transition_latency = 0;
> +    for ( i=0; i<perf->state_count; i++ )

... iterating up to count only here? Also there are a number of blanks
missing in this for() (also again below).

> +err_freqfree:
> +    xfree(data->freq_table);
> +err_unreg:

Labels indented by at least one space please.

> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -160,6 +160,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>  #define VIRQ_MEM_EVENT  10 /* G. (DOM0) A memory event has occured           */
>  #define VIRQ_XC_RESERVED 11 /* G. Reserved for XenClient                     */
>  #define VIRQ_ENOMEM     12 /* G. (DOM0) Low on heap memory       */
> +#define VIRQ_CPUFREQ    13 /* G. (DOM0) Notify dom0-cpufreq driver.           */

I think with vPMU already wanting to use 13 you should pick 14 right
away so that your Dom0 side code doesn't become incompatible later.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-16 11:27 ` [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op Oleksandr Dmytryshyn
@ 2014-10-17 13:21   ` Jan Beulich
  2014-10-22  8:46     ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-17 13:21 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
> +{
> +    int ret = 0;
> +    switch ( op->cmd )
> +    {
> +    case XEN_SYSCTL_CPUFREQ_get_target:
> +        spin_lock(&sysctl_cpufreq_lock);
> +        op->u.target.cpu = sysctl_cpufreq_data.cpu;
> +        op->u.target.freq = sysctl_cpufreq_data.freq;
> +        op->u.target.relation = sysctl_cpufreq_data.relation;
> +        spin_unlock(&sysctl_cpufreq_lock);
> +        break;
> +
> +    case XEN_SYSCTL_CPUFREQ_set_result:
> +        spin_lock(&sysctl_cpufreq_lock);
> +        sysctl_cpufreq_data.result = op->u.result;
> +        spin_unlock(&sysctl_cpufreq_lock);
> +        break;

Seeing this and taking it together with the previous patch I still
can't really see how all this is intended to fit together, and
where the frequency change really takes place. I'm particularly
lacking understanding of how the result of the operation is
intended to be made available for the right .target handler
invocation - right now it looks like the next invocation will get
to see the result of the previous one.

> +    default:
> +        return -ENOSYS;

-EINVAL or -EOPNOTSUPP please.

> --- a/xen/include/xen/cpufreq.h
> +++ b/xen/include/xen/cpufreq.h
> @@ -20,6 +20,7 @@
>  #include <xen/spinlock.h>
>  #include <xen/errno.h>
>  #include <xen/cpumask.h>
> +#include <public/sysctl.h>

Please don't add new dependencies that aren't needed (not even
on ARM).

> @@ -264,4 +265,8 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq);
>  void cpufreq_dbs_timer_suspend(void);
>  void cpufreq_dbs_timer_resume(void);
>  
> +#ifdef HAS_DOM0_CPUFREQ
> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op);
> +#endif

Simply forward declare and use struct xen_sysctl_cpufreq_op here.

And framing a declaration in #ifdef is relatively pointless too.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable
  2014-10-17 12:26   ` Jan Beulich
@ 2014-10-22  8:38     ` Oleksandr Dmytryshyn
  0 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  8:38 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Fri, Oct 17, 2014 at 3:26 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.10.14 at 13:26, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> @@ -133,7 +132,8 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
>>          cpufreq_statistic_reset(op->cpuid);
>>          break;
>>      }
>> -
>> +/* For now those operations can be used when ACPI is enabled */
>> +#ifdef CONFIG_ACPI
>>      case PMSTAT_get_max_cx:
>>      {
>>          op->u.getcx.nr = pmstat_get_cx_nr(op->cpuid);
>
> Don't remove blank lines like this please. Also I'm not sure the comment
> makes sense as is - is there perhaps an "only" missing?
I'll fix this in the next patch-set

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-17 12:31   ` Jan Beulich
@ 2014-10-22  8:39     ` Oleksandr Dmytryshyn
  2014-10-22  8:44       ` Oleksandr Dmytryshyn
  2014-10-22  9:21       ` Jan Beulich
  0 siblings, 2 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  8:39 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Fri, Oct 17, 2014 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> --- a/xen/drivers/cpufreq/cpufreq.c
>> +++ b/xen/drivers/cpufreq/cpufreq.c
>> @@ -43,9 +43,14 @@
>>  #include <asm/io.h>
>>  #include <asm/processor.h>
>>  #include <asm/percpu.h>
>> -#include <acpi/acpi.h>
>>  #include <xen/cpufreq.h>
>
> I can see this being moved into a #ifdef CONFIG_ACPI section, but
> removing it altogether seems kind of wrong.
>
>> +#ifdef CONFIG_ACPI
>> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>> +#else
>> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>> +#endif
>
> I'm not really understanding what is ACPI-specific about PCT but not
> any of the other three. Please give some explanation in the commit
> description.
XEN_PX_PPC - 'platform_limit' setting is included to the processor_performance
structure which can be used in ARM meaning

XEN_PX_PCT - 'control_register' and 'status_register' settings (MSR and MCR
registers) are included to the processor_performance structure - this is
ACPI-specific settings because MSR and MCR registers are ACPI-specific

XEN_PX_PSS - information about P-states is included to the processor_performance
structure which can be used in ARM meaning (frequency in each state,
state_count etc.)

XEN_PX_PPC - inforamtion about pomer domain info and shared_type is included
to the processor_performance structure which can be used in ARM meaning
(shared_type and power domain number)


> Also if you want to use any kind of indentation for preprocessor
> directives, please use
>
> # define
>
> (i.e. keep the # at the first column).
I'll ашч this in the next patch-set

> Jan
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 09/12] xen: arm: implement platform hypercall
  2014-10-17 12:44   ` Jan Beulich
@ 2014-10-22  8:40     ` Oleksandr Dmytryshyn
  2014-10-22  9:24       ` Jan Beulich
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  8:40 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Fri, Oct 17, 2014 at 3:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> +long arch_do_platform_op(struct xen_platform_op *platform_op,
>> +                          XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>> +{
>> +    long ret = 0;
>> +    struct xen_platform_op *op = platform_op;
>> +
>> +    switch ( op->cmd )
>> +    {
>> +    case XENPF_set_processor_pminfo:
>> +        switch ( op->u.set_pminfo.type )
>> +        {
>> +        case XEN_PM_PX:
>> +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
>> +            {
>> +                ret = -ENOSYS;
>> +                break;
>> +           }
>> +#ifdef HAS_CPUFREQ
>> +            ret = set_px_pminfo(op->u.set_pminfo.id, &op->u.set_pminfo.u.perf);
>
> This cannot possibly be HAS_CPUFREQ, since ->u.set_pminfo
> specifically uses ACPI notation for e.g. the CPU numbering. And
> with that it becomes questionable whether the whole series is
> doing right in abstracting away ACPI.
set_px_pminfo() function is implemented in the cpufreq.c file and this file
will be compiled only if HAS_CPUFREQ is defined. Thus I've used HAS_CPUFREQ in
this case. If HAS_CPUFREQ is not defined then set_px_pminfo() will not be
compiled and there will be errors on compilation without this definition.
->u.set_pminfo uses ACPI notation for the CPU numbering but
patch "cpufreq: make cpufreq driver more generalizable" skips ACPI
notation for the CPU numbering if CONFIG_ACPI is not defined.

>> @@ -37,6 +38,7 @@ CHECK_pf_enter_acpi_sleep;
>>  #define _XEN_GUEST_HANDLE_PARAM(t) XEN_GUEST_HANDLE_PARAM(t)
>>  typedef int ret_t;
>>
>> +#include "../../../common/platform_hypercall.c"
>>  #include "../platform_hypercall.c"
>
> This needs to be done properly in common/.
>
>> +extern ret_t
>> +arch_do_platform_op(
>> +    struct xen_platform_op *platform_op,
>> +    XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op);
>
> Declarations belong in header files except in very special cases
> (which this is none of).
>
>> +ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>> +{
>> +    ret_t ret = 0;
>> +    struct xen_platform_op curop, *op = &curop;
>> +
>> +    if ( copy_from_guest(op, u_xenpf_op, 1) )
>> +        return -EFAULT;
>> +
>> +    if ( op->interface_version != XENPF_INTERFACE_VERSION )
>> +        return -EACCES;
>> +
>> +    ret = xsm_platform_op(XSM_PRIV, op->cmd);
>> +    if ( ret )
>> +        return ret;
>> +
>> +    /*
>> +     * Trylock here avoids deadlock with an existing platform critical section
>> +     * which might (for some current or future reason) want to synchronise
>> +     * with this vcpu.
>> +     */
>> +    while ( !spin_trylock(&xenpf_lock) )
>> +        if ( hypercall_preempt_check() )
>> +            return hypercall_create_continuation(
>> +                __HYPERVISOR_platform_op, "h", u_xenpf_op);
>> +
>> +    ret = arch_do_platform_op(op, u_xenpf_op);
>> +
>> +    spin_unlock(&xenpf_lock);
>
> And seeing this I really wonder whether making this common is
> really worthwhile.
I'll create separate file for platform_hypercall without common code
(as it was done for physdev_op)

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-17 13:03   ` Jan Beulich
@ 2014-10-22  8:43     ` Oleksandr Dmytryshyn
  2014-10-22  9:30       ` Jan Beulich
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  8:43 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> This driver uses Dom0 to change frequencies on CPUs
>
> This surely is too little of an explanation, not the least because this
> approach is still being argued about. Regardless of me not being in
> favor of the approach, a couple of comments:
>
>> +#include <xen/types.h>
>> +#include <xen/errno.h>
>> +#include <xen/sched.h>
>> +#include <xen/event.h>
>> +#include <xen/irq.h>
>> +#include <xen/spinlock.h>
>> +#include <xen/cpufreq.h>
>> +#include <asm/current.h>
>> +
>> +struct acpi_cpufreq_data *cpufreq_drv_data[NR_CPUS];
>
> static (and renamed) or you need to abstract out the variable from
> the other driver. In no case should there be two definitions of the
> same variable.
>
>> +static void notify_cpufreq_domains(void)
I'll do this in the next patch-set

> Why "domains", not "hwdom"? You don't really want to send this
> to other than the hardware domain I hope.
All domains (not only hwdomain) should receive this interrupt.
In case is hwdomain is Linux kernel it can automaticaly recalculate
jiffies. But other domains should recalculate jiffies too. I'll
implement this mechanism a bit later.

>> +    /* Do send cmd for Dom0 */
>> +    spin_lock(&sysctl_cpufreq_lock);
>> +    /* return previous result */
>> +    ret = sysctl_cpufreq_data.result;
>> +
>> +    sysctl_cpufreq_data.cpu = policy->cpu;
>> +    sysctl_cpufreq_data.freq = freqs.new;
>> +    sysctl_cpufreq_data.relation = (uint32_t)relation;
>> +    spin_unlock(&sysctl_cpufreq_lock);
>
> sysctl_cpufreq_data fields appear to be accessed only here. Is the
> patch incomplete? It is certainly not possible to understand how this
> is intended to work without the other sides being there.
Now sysctl_cpufreq_data fields is accessed only here. But it will be accessed
in XEN_SYSCTL_cpufreq_op handler (in the next patch in this series).

>> +static int
>> +dom0_cpufreq_cpu_init(struct cpufreq_policy *policy)
I'll fix this in the next patch-set

> Please avoid the term dom0 where possible, and where not truly
> meaning dom0. The tree got switched to use hwdom instead a
> while ago.
>
>> +    data->freq_table = xmalloc_array(struct cpufreq_frequency_table,
>> +                                    (perf->state_count+1));
>
> Why count+1 here but ...
count+1 because last field of the freq table should be reserved for
CPUFREQ_TABLE_END.

>> +    if ( !data->freq_table )
>> +    {
>> +        ret = -ENOMEM;
>> +        goto err_unreg;
>> +    }
>> +
>> +    /* detect transition latency */
>> +    policy->cpuinfo.transition_latency = 0;
>> +    for ( i=0; i<perf->state_count; i++ )
>
> ... iterating up to count only here? Also there are a number of blanks
> missing in this for() (also again below).
Iterating up to count because we have maximum 'count' valid states + additional
field for CPUFREQ_TABLE_END.

>> +err_freqfree:
>> +    xfree(data->freq_table);
>> +err_unreg:
>
> Labels indented by at least one space please.
I'll fix this in the next patch-set

>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -160,6 +160,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>>  #define VIRQ_MEM_EVENT  10 /* G. (DOM0) A memory event has occured           */
>>  #define VIRQ_XC_RESERVED 11 /* G. Reserved for XenClient                     */
>>  #define VIRQ_ENOMEM     12 /* G. (DOM0) Low on heap memory       */
>> +#define VIRQ_CPUFREQ    13 /* G. (DOM0) Notify dom0-cpufreq driver.           */
>
> I think with vPMU already wanting to use 13 you should pick 14 right
> away so that your Dom0 side code doesn't become incompatible later.
I'll fix this in the next patch-set

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-22  8:39     ` Oleksandr Dmytryshyn
@ 2014-10-22  8:44       ` Oleksandr Dmytryshyn
  2014-10-22  9:21       ` Jan Beulich
  1 sibling, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  8:44 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

http://www.globallogic.com/email_disclaimer.txt


On Wed, Oct 22, 2014 at 11:39 AM, Oleksandr Dmytryshyn
<oleksandr.dmytryshyn@globallogic.com> wrote:
> On Fri, Oct 17, 2014 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>> @@ -43,9 +43,14 @@
>>>  #include <asm/io.h>
>>>  #include <asm/processor.h>
>>>  #include <asm/percpu.h>
>>> -#include <acpi/acpi.h>
>>>  #include <xen/cpufreq.h>
>>
>> I can see this being moved into a #ifdef CONFIG_ACPI section, but
>> removing it altogether seems kind of wrong.
>>
>>> +#ifdef CONFIG_ACPI
>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>>> +#else
>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>>> +#endif
>>
>> I'm not really understanding what is ACPI-specific about PCT but not
>> any of the other three. Please give some explanation in the commit
>> description.
> XEN_PX_PPC - 'platform_limit' setting is included to the processor_performance
> structure which can be used in ARM meaning
>
> XEN_PX_PCT - 'control_register' and 'status_register' settings (MSR and MCR
> registers) are included to the processor_performance structure - this is
> ACPI-specific settings because MSR and MCR registers are ACPI-specific
>
> XEN_PX_PSS - information about P-states is included to the processor_performance
> structure which can be used in ARM meaning (frequency in each state,
> state_count etc.)
>
> XEN_PX_PPC - inforamtion about pomer domain info and shared_type is included
> to the processor_performance structure which can be used in ARM meaning
> (shared_type and power domain number)
>
>
>> Also if you want to use any kind of indentation for preprocessor
>> directives, please use
>>
>> # define
>>
>> (i.e. keep the # at the first column).
> I'll ашч this in the next patch-set
I'll fix this in the next patch-set

>
>> Jan
>>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-17 13:21   ` Jan Beulich
@ 2014-10-22  8:46     ` Oleksandr Dmytryshyn
  2014-10-22  9:34       ` Jan Beulich
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  8:46 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Fri, Oct 17, 2014 at 4:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
>> +{
>> +    int ret = 0;
>> +    switch ( op->cmd )
>> +    {
>> +    case XEN_SYSCTL_CPUFREQ_get_target:
>> +        spin_lock(&sysctl_cpufreq_lock);
>> +        op->u.target.cpu = sysctl_cpufreq_data.cpu;
>> +        op->u.target.freq = sysctl_cpufreq_data.freq;
>> +        op->u.target.relation = sysctl_cpufreq_data.relation;
>> +        spin_unlock(&sysctl_cpufreq_lock);
>> +        break;
>> +
>> +    case XEN_SYSCTL_CPUFREQ_set_result:
>> +        spin_lock(&sysctl_cpufreq_lock);
>> +        sysctl_cpufreq_data.result = op->u.result;
>> +        spin_unlock(&sysctl_cpufreq_lock);
>> +        break;
>
> Seeing this and taking it together with the previous patch I still
> can't really see how all this is intended to fit together, and
> where the frequency change really takes place. I'm particularly
> lacking understanding of how the result of the operation is
> intended to be made available for the right .target handler
> invocation - right now it looks like the next invocation will get
> to see the result of the previous one.
Some requests to change frequency can be issued from the hypercall.
All hypercalls are locked by spinlocks. Thus the frequency changing
should be done in the atomic contex. But we can not send command to the
hwdom and wait answer in the atomic context. So we may use the result
of the previous command and send current command.

>> +    default:
>> +        return -ENOSYS;
>
> -EINVAL or -EOPNOTSUPP please.
I'll fix this in the next patch-set

>> --- a/xen/include/xen/cpufreq.h
>> +++ b/xen/include/xen/cpufreq.h
>> @@ -20,6 +20,7 @@
>>  #include <xen/spinlock.h>
>>  #include <xen/errno.h>
>>  #include <xen/cpumask.h>
>> +#include <public/sysctl.h>
>
> Please don't add new dependencies that aren't needed (not even
> on ARM).
I'll create a separate file hwdom-cpufreq.h and I'll use it.

>> @@ -264,4 +265,8 @@ int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq);
>>  void cpufreq_dbs_timer_suspend(void);
>>  void cpufreq_dbs_timer_resume(void);
>>
>> +#ifdef HAS_DOM0_CPUFREQ
>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op);
>> +#endif
>
> Simply forward declare and use struct xen_sysctl_cpufreq_op here.
>
> And framing a declaration in #ifdef is relatively pointless too.
I'll create a separate file hwdom-cpufreq.h and I'll use it (as I've
written above).

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-22  8:39     ` Oleksandr Dmytryshyn
  2014-10-22  8:44       ` Oleksandr Dmytryshyn
@ 2014-10-22  9:21       ` Jan Beulich
  2014-10-22  9:51         ` Oleksandr Dmytryshyn
  1 sibling, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-22  9:21 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 22.10.14 at 10:39, <oleksandr.dmytryshyn@globallogic.com> wrote:
> On Fri, Oct 17, 2014 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>> @@ -43,9 +43,14 @@
>>>  #include <asm/io.h>
>>>  #include <asm/processor.h>
>>>  #include <asm/percpu.h>
>>> -#include <acpi/acpi.h>
>>>  #include <xen/cpufreq.h>
>>
>> I can see this being moved into a #ifdef CONFIG_ACPI section, but
>> removing it altogether seems kind of wrong.
>>
>>> +#ifdef CONFIG_ACPI
>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD | 
> XEN_PX_PPC)
>>> +#else
>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>>> +#endif
>>
>> I'm not really understanding what is ACPI-specific about PCT but not
>> any of the other three. Please give some explanation in the commit
>> description.
> XEN_PX_PPC - 'platform_limit' setting is included to the 
> processor_performance
> structure which can be used in ARM meaning
> 
> XEN_PX_PCT - 'control_register' and 'status_register' settings (MSR and MCR
> registers) are included to the processor_performance structure - this is
> ACPI-specific settings because MSR and MCR registers are ACPI-specific
> 
> XEN_PX_PSS - information about P-states is included to the 
> processor_performance
> structure which can be used in ARM meaning (frequency in each state,
> state_count etc.)
> 
> XEN_PX_PPC - inforamtion about pomer domain info and shared_type is included
> to the processor_performance structure which can be used in ARM meaning
> (shared_type and power domain number)

Not sure why you listed XEN_PX_PPC twice, but ignored PSD (I guess
one of the two was just a typo). In any event I suggest no
overloading ACPI things with ARM non-ACPI ones (re-using interface
structures may be okay if they're truly not ACPI specific, but ACPI
naming shouldn't be applied to non-ACPI items).

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 09/12] xen: arm: implement platform hypercall
  2014-10-22  8:40     ` Oleksandr Dmytryshyn
@ 2014-10-22  9:24       ` Jan Beulich
  2014-10-22 10:36         ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-22  9:24 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 22.10.14 at 10:40, <oleksandr.dmytryshyn@globallogic.com> wrote:
> On Fri, Oct 17, 2014 at 3:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> +long arch_do_platform_op(struct xen_platform_op *platform_op,
>>> +                          XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) 
> u_xenpf_op)
>>> +{
>>> +    long ret = 0;
>>> +    struct xen_platform_op *op = platform_op;
>>> +
>>> +    switch ( op->cmd )
>>> +    {
>>> +    case XENPF_set_processor_pminfo:
>>> +        switch ( op->u.set_pminfo.type )
>>> +        {
>>> +        case XEN_PM_PX:
>>> +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
>>> +            {
>>> +                ret = -ENOSYS;
>>> +                break;
>>> +           }
>>> +#ifdef HAS_CPUFREQ
>>> +            ret = set_px_pminfo(op->u.set_pminfo.id, 
> &op->u.set_pminfo.u.perf);
>>
>> This cannot possibly be HAS_CPUFREQ, since ->u.set_pminfo
>> specifically uses ACPI notation for e.g. the CPU numbering. And
>> with that it becomes questionable whether the whole series is
>> doing right in abstracting away ACPI.
> set_px_pminfo() function is implemented in the cpufreq.c file and this file
> will be compiled only if HAS_CPUFREQ is defined. Thus I've used HAS_CPUFREQ 
> in
> this case. If HAS_CPUFREQ is not defined then set_px_pminfo() will not be
> compiled and there will be errors on compilation without this definition.
> ->u.set_pminfo uses ACPI notation for the CPU numbering but
> patch "cpufreq: make cpufreq driver more generalizable" skips ACPI
> notation for the CPU numbering if CONFIG_ACPI is not defined.

This all looks very kludgy and hence is likely going to become a
maintenance nightmare. Please separate things properly that are
separate by design.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-22  8:43     ` Oleksandr Dmytryshyn
@ 2014-10-22  9:30       ` Jan Beulich
  2014-10-22 11:00         ` Ian Campbell
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-22  9:30 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
> On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> +static void notify_cpufreq_domains(void)
>> Why "domains", not "hwdom"? You don't really want to send this
>> to other than the hardware domain I hope.
> All domains (not only hwdomain) should receive this interrupt.
> In case is hwdomain is Linux kernel it can automaticaly recalculate
> jiffies. But other domains should recalculate jiffies too. I'll
> implement this mechanism a bit later.

This needs more explanation then: If all domains need to do
adjustments when the frequency changes, there's something
wrong here imo. Such changes are supposed to be visible to
the hypervisor only (with, in the case here, assistance - but
nothing else - by the hardware domain).

>>> +    /* Do send cmd for Dom0 */
>>> +    spin_lock(&sysctl_cpufreq_lock);
>>> +    /* return previous result */
>>> +    ret = sysctl_cpufreq_data.result;
>>> +
>>> +    sysctl_cpufreq_data.cpu = policy->cpu;
>>> +    sysctl_cpufreq_data.freq = freqs.new;
>>> +    sysctl_cpufreq_data.relation = (uint32_t)relation;
>>> +    spin_unlock(&sysctl_cpufreq_lock);
>>
>> sysctl_cpufreq_data fields appear to be accessed only here. Is the
>> patch incomplete? It is certainly not possible to understand how this
>> is intended to work without the other sides being there.
> Now sysctl_cpufreq_data fields is accessed only here. But it will be 
> accessed
> in XEN_SYSCTL_cpufreq_op handler (in the next patch in this series).

I.e. the split of changes among patches is suboptimal.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-22  8:46     ` Oleksandr Dmytryshyn
@ 2014-10-22  9:34       ` Jan Beulich
  2014-10-22 10:09         ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-22  9:34 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 22.10.14 at 10:46, <oleksandr.dmytryshyn@globallogic.com> wrote:
> On Fri, Oct 17, 2014 at 4:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
>>> +{
>>> +    int ret = 0;
>>> +    switch ( op->cmd )
>>> +    {
>>> +    case XEN_SYSCTL_CPUFREQ_get_target:
>>> +        spin_lock(&sysctl_cpufreq_lock);
>>> +        op->u.target.cpu = sysctl_cpufreq_data.cpu;
>>> +        op->u.target.freq = sysctl_cpufreq_data.freq;
>>> +        op->u.target.relation = sysctl_cpufreq_data.relation;
>>> +        spin_unlock(&sysctl_cpufreq_lock);
>>> +        break;
>>> +
>>> +    case XEN_SYSCTL_CPUFREQ_set_result:
>>> +        spin_lock(&sysctl_cpufreq_lock);
>>> +        sysctl_cpufreq_data.result = op->u.result;
>>> +        spin_unlock(&sysctl_cpufreq_lock);
>>> +        break;
>>
>> Seeing this and taking it together with the previous patch I still
>> can't really see how all this is intended to fit together, and
>> where the frequency change really takes place. I'm particularly
>> lacking understanding of how the result of the operation is
>> intended to be made available for the right .target handler
>> invocation - right now it looks like the next invocation will get
>> to see the result of the previous one.
> Some requests to change frequency can be issued from the hypercall.
> All hypercalls are locked by spinlocks. Thus the frequency changing
> should be done in the atomic contex. But we can not send command to the
> hwdom and wait answer in the atomic context. So we may use the result
> of the previous command and send current command.

You just restate what was known already. What you fail to do is
explain how this can end up being correct. To me this looks like a
flawed design (acceptable for a PoC, but certainly not for
something proposed for upstream inclusion).

>>> --- a/xen/include/xen/cpufreq.h
>>> +++ b/xen/include/xen/cpufreq.h
>>> @@ -20,6 +20,7 @@
>>>  #include <xen/spinlock.h>
>>>  #include <xen/errno.h>
>>>  #include <xen/cpumask.h>
>>> +#include <public/sysctl.h>
>>
>> Please don't add new dependencies that aren't needed (not even
>> on ARM).
> I'll create a separate file hwdom-cpufreq.h and I'll use it.

I don't follow - you just don't need the #include above; no need to
create a new header.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-22  9:21       ` Jan Beulich
@ 2014-10-22  9:51         ` Oleksandr Dmytryshyn
  2014-10-22  9:57           ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  9:51 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Wed, Oct 22, 2014 at 12:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.10.14 at 10:39, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> On Fri, Oct 17, 2014 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>>> @@ -43,9 +43,14 @@
>>>>  #include <asm/io.h>
>>>>  #include <asm/processor.h>
>>>>  #include <asm/percpu.h>
>>>> -#include <acpi/acpi.h>
>>>>  #include <xen/cpufreq.h>
>>>
>>> I can see this being moved into a #ifdef CONFIG_ACPI section, but
>>> removing it altogether seems kind of wrong.
>>>
>>>> +#ifdef CONFIG_ACPI
>>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD |
>> XEN_PX_PPC)
>>>> +#else
>>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>>>> +#endif
>>>
>>> I'm not really understanding what is ACPI-specific about PCT but not
>>> any of the other three. Please give some explanation in the commit
>>> description.
>> XEN_PX_PPC - 'platform_limit' setting is included to the
>> processor_performance
>> structure which can be used in ARM meaning
>>
>> XEN_PX_PCT - 'control_register' and 'status_register' settings (MSR and MCR
>> registers) are included to the processor_performance structure - this is
>> ACPI-specific settings because MSR and MCR registers are ACPI-specific
>>
>> XEN_PX_PSS - information about P-states is included to the
>> processor_performance
>> structure which can be used in ARM meaning (frequency in each state,
>> state_count etc.)

It was a typo. It should be XEN_PX_PSD in last paragraph
>> XEN_PX_PPC - inforamtion about pomer domain info and shared_type is included
>> to the processor_performance structure which can be used in ARM meaning
>> (shared_type and power domain number)
>
> Not sure why you listed XEN_PX_PPC twice, but ignored PSD (I guess
> one of the two was just a typo). In any event I suggest no
> overloading ACPI things with ARM non-ACPI ones (re-using interface
> structures may be okay if they're truly not ACPI specific, but ACPI
> naming shouldn't be applied to non-ACPI items).
>
> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-22  9:51         ` Oleksandr Dmytryshyn
@ 2014-10-22  9:57           ` Oleksandr Dmytryshyn
  2014-10-22 10:09             ` Jan Beulich
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22  9:57 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Wed, Oct 22, 2014 at 12:51 PM, Oleksandr Dmytryshyn
<oleksandr.dmytryshyn@globallogic.com> wrote:
> On Wed, Oct 22, 2014 at 12:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.10.14 at 10:39, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> On Fri, Oct 17, 2014 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>>>> @@ -43,9 +43,14 @@
>>>>>  #include <asm/io.h>
>>>>>  #include <asm/processor.h>
>>>>>  #include <asm/percpu.h>
>>>>> -#include <acpi/acpi.h>
>>>>>  #include <xen/cpufreq.h>
>>>>
>>>> I can see this being moved into a #ifdef CONFIG_ACPI section, but
>>>> removing it altogether seems kind of wrong.
>>>>
>>>>> +#ifdef CONFIG_ACPI
>>>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD |
>>> XEN_PX_PPC)
>>>>> +#else
>>>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>>>>> +#endif
>>>>
>>>> I'm not really understanding what is ACPI-specific about PCT but not
>>>> any of the other three. Please give some explanation in the commit
>>>> description.
>>> XEN_PX_PPC - 'platform_limit' setting is included to the
>>> processor_performance
>>> structure which can be used in ARM meaning
>>>
>>> XEN_PX_PCT - 'control_register' and 'status_register' settings (MSR and MCR
>>> registers) are included to the processor_performance structure - this is
>>> ACPI-specific settings because MSR and MCR registers are ACPI-specific
>>>
>>> XEN_PX_PSS - information about P-states is included to the
>>> processor_performance
>>> structure which can be used in ARM meaning (frequency in each state,
>>> state_count etc.)
>
> It was a typo. It should be XEN_PX_PSD in last paragraph
>>> XEN_PX_PPC - inforamtion about pomer domain info and shared_type is included
>>> to the processor_performance structure which can be used in ARM meaning
>>> (shared_type and power domain number)
>>
>> Not sure why you listed XEN_PX_PPC twice, but ignored PSD (I guess
>> one of the two was just a typo). In any event I suggest no
>> overloading ACPI things with ARM non-ACPI ones (re-using interface
>> structures may be okay if they're truly not ACPI specific, but ACPI
>> naming shouldn't be applied to non-ACPI items).
May be it will be better to introduce a new flag (XEN_PX_DATA) for
non-ACPI case?

>> Jan
>>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-22  9:34       ` Jan Beulich
@ 2014-10-22 10:09         ` Oleksandr Dmytryshyn
  2014-10-22 10:24           ` Jan Beulich
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22 10:09 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Wed, Oct 22, 2014 at 12:34 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.10.14 at 10:46, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> On Fri, Oct 17, 2014 at 4:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>> +int sysctl_cpufreq_op(xen_sysctl_cpufreq_op_t *op)
>>>> +{
>>>> +    int ret = 0;
>>>> +    switch ( op->cmd )
>>>> +    {
>>>> +    case XEN_SYSCTL_CPUFREQ_get_target:
>>>> +        spin_lock(&sysctl_cpufreq_lock);
>>>> +        op->u.target.cpu = sysctl_cpufreq_data.cpu;
>>>> +        op->u.target.freq = sysctl_cpufreq_data.freq;
>>>> +        op->u.target.relation = sysctl_cpufreq_data.relation;
>>>> +        spin_unlock(&sysctl_cpufreq_lock);
>>>> +        break;
>>>> +
>>>> +    case XEN_SYSCTL_CPUFREQ_set_result:
>>>> +        spin_lock(&sysctl_cpufreq_lock);
>>>> +        sysctl_cpufreq_data.result = op->u.result;
>>>> +        spin_unlock(&sysctl_cpufreq_lock);
>>>> +        break;
>>>
>>> Seeing this and taking it together with the previous patch I still
>>> can't really see how all this is intended to fit together, and
>>> where the frequency change really takes place. I'm particularly
>>> lacking understanding of how the result of the operation is
>>> intended to be made available for the right .target handler
>>> invocation - right now it looks like the next invocation will get
>>> to see the result of the previous one.
>> Some requests to change frequency can be issued from the hypercall.
>> All hypercalls are locked by spinlocks. Thus the frequency changing
>> should be done in the atomic contex. But we can not send command to the
>> hwdom and wait answer in the atomic context. So we may use the result
>> of the previous command and send current command.
>
> You just restate what was known already. What you fail to do is
> explain how this can end up being correct. To me this looks like a
> flawed design (acceptable for a PoC, but certainly not for
> something proposed for upstream inclusion).
I'll try to implement it in the better way but now it is difficult to do.

>>>> --- a/xen/include/xen/cpufreq.h
>>>> +++ b/xen/include/xen/cpufreq.h
>>>> @@ -20,6 +20,7 @@
>>>>  #include <xen/spinlock.h>
>>>>  #include <xen/errno.h>
>>>>  #include <xen/cpumask.h>
>>>> +#include <public/sysctl.h>
>>>
>>> Please don't add new dependencies that aren't needed (not even
>>> on ARM).
>> I'll create a separate file hwdom-cpufreq.h and I'll use it.
>
> I don't follow - you just don't need the #include above; no need to
> create a new header.
The structure 'xen_sysctl_cpufreq_op_t' could not be defined in the
original file cpufreq.h
because all sysctl-related structures are defined in the
xen/include/public/sysctl.h.
And if this structure will be defined in the cpufreq.h than this file
should be included to the sysctl.h (and it is not an acceptable solution).

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 06/12] cpufreq: make cpufreq driver more generalizable
  2014-10-22  9:57           ` Oleksandr Dmytryshyn
@ 2014-10-22 10:09             ` Jan Beulich
  0 siblings, 0 replies; 42+ messages in thread
From: Jan Beulich @ 2014-10-22 10:09 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 22.10.14 at 11:57, <oleksandr.dmytryshyn@globallogic.com> wrote:
> On Wed, Oct 22, 2014 at 12:51 PM, Oleksandr Dmytryshyn
> <oleksandr.dmytryshyn@globallogic.com> wrote:
>> On Wed, Oct 22, 2014 at 12:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 22.10.14 at 10:39, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>> On Fri, Oct 17, 2014 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>>>> --- a/xen/drivers/cpufreq/cpufreq.c
>>>>>> +++ b/xen/drivers/cpufreq/cpufreq.c
>>>>>> @@ -43,9 +43,14 @@
>>>>>>  #include <asm/io.h>
>>>>>>  #include <asm/processor.h>
>>>>>>  #include <asm/percpu.h>
>>>>>> -#include <acpi/acpi.h>
>>>>>>  #include <xen/cpufreq.h>
>>>>>
>>>>> I can see this being moved into a #ifdef CONFIG_ACPI section, but
>>>>> removing it altogether seems kind of wrong.
>>>>>
>>>>>> +#ifdef CONFIG_ACPI
>>>>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PCT | XEN_PX_PSS | XEN_PX_PSD |
>>>> XEN_PX_PPC)
>>>>>> +#else
>>>>>> +    #define XEN_PX_FULL_INIT (XEN_PX_PSS | XEN_PX_PSD | XEN_PX_PPC)
>>>>>> +#endif
>>>>>
>>>>> I'm not really understanding what is ACPI-specific about PCT but not
>>>>> any of the other three. Please give some explanation in the commit
>>>>> description.
>>>> XEN_PX_PPC - 'platform_limit' setting is included to the
>>>> processor_performance
>>>> structure which can be used in ARM meaning
>>>>
>>>> XEN_PX_PCT - 'control_register' and 'status_register' settings (MSR and MCR
>>>> registers) are included to the processor_performance structure - this is
>>>> ACPI-specific settings because MSR and MCR registers are ACPI-specific
>>>>
>>>> XEN_PX_PSS - information about P-states is included to the
>>>> processor_performance
>>>> structure which can be used in ARM meaning (frequency in each state,
>>>> state_count etc.)
>>
>> It was a typo. It should be XEN_PX_PSD in last paragraph
>>>> XEN_PX_PPC - inforamtion about pomer domain info and shared_type is included
>>>> to the processor_performance structure which can be used in ARM meaning
>>>> (shared_type and power domain number)
>>>
>>> Not sure why you listed XEN_PX_PPC twice, but ignored PSD (I guess
>>> one of the two was just a typo). In any event I suggest no
>>> overloading ACPI things with ARM non-ACPI ones (re-using interface
>>> structures may be okay if they're truly not ACPI specific, but ACPI
>>> naming shouldn't be applied to non-ACPI items).
> May be it will be better to introduce a new flag (XEN_PX_DATA) for
> non-ACPI case?

Exactly.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-22 10:09         ` Oleksandr Dmytryshyn
@ 2014-10-22 10:24           ` Jan Beulich
  2014-10-22 10:39             ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Jan Beulich @ 2014-10-22 10:24 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

>>> On 22.10.14 at 12:09, <oleksandr.dmytryshyn@globallogic.com> wrote:
> On Wed, Oct 22, 2014 at 12:34 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 22.10.14 at 10:46, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>> On Fri, Oct 17, 2014 at 4:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>>> --- a/xen/include/xen/cpufreq.h
>>>>> +++ b/xen/include/xen/cpufreq.h
>>>>> @@ -20,6 +20,7 @@
>>>>>  #include <xen/spinlock.h>
>>>>>  #include <xen/errno.h>
>>>>>  #include <xen/cpumask.h>
>>>>> +#include <public/sysctl.h>
>>>>
>>>> Please don't add new dependencies that aren't needed (not even
>>>> on ARM).
>>> I'll create a separate file hwdom-cpufreq.h and I'll use it.
>>
>> I don't follow - you just don't need the #include above; no need to
>> create a new header.
> The structure 'xen_sysctl_cpufreq_op_t' could not be defined in the
> original file cpufreq.h
> because all sysctl-related structures are defined in the
> xen/include/public/sysctl.h.
> And if this structure will be defined in the cpufreq.h than this file
> should be included to the sysctl.h (and it is not an acceptable solution).

No. It is sufficient for the structure to be forward declared when
all you need is declaring a function argument as a pointer to it.

Jan

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 09/12] xen: arm: implement platform hypercall
  2014-10-22  9:24       ` Jan Beulich
@ 2014-10-22 10:36         ` Oleksandr Dmytryshyn
  0 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22 10:36 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Wed, Oct 22, 2014 at 12:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.10.14 at 10:40, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> On Fri, Oct 17, 2014 at 3:44 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>> +long arch_do_platform_op(struct xen_platform_op *platform_op,
>>>> +                          XEN_GUEST_HANDLE_PARAM(xen_platform_op_t)
>> u_xenpf_op)
>>>> +{
>>>> +    long ret = 0;
>>>> +    struct xen_platform_op *op = platform_op;
>>>> +
>>>> +    switch ( op->cmd )
>>>> +    {
>>>> +    case XENPF_set_processor_pminfo:
>>>> +        switch ( op->u.set_pminfo.type )
>>>> +        {
>>>> +        case XEN_PM_PX:
>>>> +            if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
>>>> +            {
>>>> +                ret = -ENOSYS;
>>>> +                break;
>>>> +           }
>>>> +#ifdef HAS_CPUFREQ
>>>> +            ret = set_px_pminfo(op->u.set_pminfo.id,
>> &op->u.set_pminfo.u.perf);
>>>
>>> This cannot possibly be HAS_CPUFREQ, since ->u.set_pminfo
>>> specifically uses ACPI notation for e.g. the CPU numbering. And
>>> with that it becomes questionable whether the whole series is
>>> doing right in abstracting away ACPI.
>> set_px_pminfo() function is implemented in the cpufreq.c file and this file
>> will be compiled only if HAS_CPUFREQ is defined. Thus I've used HAS_CPUFREQ
>> in
>> this case. If HAS_CPUFREQ is not defined then set_px_pminfo() will not be
>> compiled and there will be errors on compilation without this definition.
>> ->u.set_pminfo uses ACPI notation for the CPU numbering but
>> patch "cpufreq: make cpufreq driver more generalizable" skips ACPI
>> notation for the CPU numbering if CONFIG_ACPI is not defined.
>
> This all looks very kludgy and hence is likely going to become a
> maintenance nightmare. Please separate things properly that are
> separate by design.
Yes, It all looks very kludgy.
But now ->u.set_pminfo can be reused for non-ACPI cases.
set_px_pminfo() function can be reused for non-ACPI cases too.
Thus this function is not only ACPI-specific. This function can be
CPUFREQ-specific in this context.

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op
  2014-10-22 10:24           ` Jan Beulich
@ 2014-10-22 10:39             ` Oleksandr Dmytryshyn
  0 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22 10:39 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Tim Deegan, Stefano Stabellini, Ian Campbell, xen-devel

On Wed, Oct 22, 2014 at 1:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.10.14 at 12:09, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> On Wed, Oct 22, 2014 at 12:34 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 22.10.14 at 10:46, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>> On Fri, Oct 17, 2014 at 4:21 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>>>>>> --- a/xen/include/xen/cpufreq.h
>>>>>> +++ b/xen/include/xen/cpufreq.h
>>>>>> @@ -20,6 +20,7 @@
>>>>>>  #include <xen/spinlock.h>
>>>>>>  #include <xen/errno.h>
>>>>>>  #include <xen/cpumask.h>
>>>>>> +#include <public/sysctl.h>
>>>>>
>>>>> Please don't add new dependencies that aren't needed (not even
>>>>> on ARM).
>>>> I'll create a separate file hwdom-cpufreq.h and I'll use it.
>>>
>>> I don't follow - you just don't need the #include above; no need to
>>> create a new header.
>> The structure 'xen_sysctl_cpufreq_op_t' could not be defined in the
>> original file cpufreq.h
>> because all sysctl-related structures are defined in the
>> xen/include/public/sysctl.h.
>> And if this structure will be defined in the cpufreq.h than this file
>> should be included to the sysctl.h (and it is not an acceptable solution).
>
> No. It is sufficient for the structure to be forward declared when
> all you need is declaring a function argument as a pointer to it.
I'll try to do this.

> Jan
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-22  9:30       ` Jan Beulich
@ 2014-10-22 11:00         ` Ian Campbell
  2014-10-22 13:24           ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Ian Campbell @ 2014-10-22 11:00 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Tim Deegan, Oleksandr Dmytryshyn, Stefano Stabellini, xen-devel

On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> >>> +static void notify_cpufreq_domains(void)
> >> Why "domains", not "hwdom"? You don't really want to send this
> >> to other than the hardware domain I hope.
> > All domains (not only hwdomain) should receive this interrupt.
> > In case is hwdomain is Linux kernel it can automaticaly recalculate
> > jiffies. But other domains should recalculate jiffies too. I'll
> > implement this mechanism a bit later.
> 
> This needs more explanation then: If all domains need to do
> adjustments when the frequency changes, there's something
> wrong here imo. Such changes are supposed to be visible to
> the hypervisor only (with, in the case here, assistance - but
> nothing else - by the hardware domain).

Guests should be using the arch/generic timers as their time source.
This is required by the hardware to increment at a fixed frequency, so
guests shouldn't need to be aware of the change to the underlying
processor clock, at least not for timekeeping/jiffies purposes.

The timers are allowed to increment by larger amounts less frequency in
lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
adjust anything for this though IMHO.

Ian.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-22 11:00         ` Ian Campbell
@ 2014-10-22 13:24           ` Oleksandr Dmytryshyn
  2014-10-22 14:00             ` Ian Campbell
  0 siblings, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22 13:24 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Tim Deegan, Stefano Stabellini, Jan Beulich, xen-devel

On Wed, Oct 22, 2014 at 2:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
>> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >>> +static void notify_cpufreq_domains(void)
>> >> Why "domains", not "hwdom"? You don't really want to send this
>> >> to other than the hardware domain I hope.
>> > All domains (not only hwdomain) should receive this interrupt.
>> > In case is hwdomain is Linux kernel it can automaticaly recalculate
>> > jiffies. But other domains should recalculate jiffies too. I'll
>> > implement this mechanism a bit later.
>>
>> This needs more explanation then: If all domains need to do
>> adjustments when the frequency changes, there's something
>> wrong here imo. Such changes are supposed to be visible to
>> the hypervisor only (with, in the case here, assistance - but
>> nothing else - by the hardware domain).
>
> Guests should be using the arch/generic timers as their time source.
> This is required by the hardware to increment at a fixed frequency, so
> guests shouldn't need to be aware of the change to the underlying
> processor clock, at least not for timekeeping/jiffies purposes.
>
> The timers are allowed to increment by larger amounts less frequency in
> lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
> adjust anything for this though IMHO.
There are many functions which are using jiffies. What about them?
All those functions will measure time not correctly if frequency
of the microprocessor will change.

> Ian.
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-22 13:24           ` Oleksandr Dmytryshyn
@ 2014-10-22 14:00             ` Ian Campbell
  2014-10-22 14:16               ` Oleksandr Dmytryshyn
  2014-10-23  9:49               ` Oleksandr Dmytryshyn
  0 siblings, 2 replies; 42+ messages in thread
From: Ian Campbell @ 2014-10-22 14:00 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Jan Beulich, xen-devel

On Wed, 2014-10-22 at 16:24 +0300, Oleksandr Dmytryshyn wrote:
> On Wed, Oct 22, 2014 at 2:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
> >> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
> >> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> >> >>> +static void notify_cpufreq_domains(void)
> >> >> Why "domains", not "hwdom"? You don't really want to send this
> >> >> to other than the hardware domain I hope.
> >> > All domains (not only hwdomain) should receive this interrupt.
> >> > In case is hwdomain is Linux kernel it can automaticaly recalculate
> >> > jiffies. But other domains should recalculate jiffies too. I'll
> >> > implement this mechanism a bit later.
> >>
> >> This needs more explanation then: If all domains need to do
> >> adjustments when the frequency changes, there's something
> >> wrong here imo. Such changes are supposed to be visible to
> >> the hypervisor only (with, in the case here, assistance - but
> >> nothing else - by the hardware domain).
> >
> > Guests should be using the arch/generic timers as their time source.
> > This is required by the hardware to increment at a fixed frequency, so
> > guests shouldn't need to be aware of the change to the underlying
> > processor clock, at least not for timekeeping/jiffies purposes.
> >
> > The timers are allowed to increment by larger amounts less frequency in
> > lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
> > adjust anything for this though IMHO.
> There are many functions which are using jiffies. What about them?
> All those functions will measure time not correctly if frequency
> of the microprocessor will change.

Jiffies is based on the timer tick, isn't it? Therefore it is ultimately
based on the arch timer.

Ian.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-22 14:00             ` Ian Campbell
@ 2014-10-22 14:16               ` Oleksandr Dmytryshyn
  2014-10-23  9:49               ` Oleksandr Dmytryshyn
  1 sibling, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-22 14:16 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Tim Deegan, Stefano Stabellini, Jan Beulich, xen-devel

On Wed, Oct 22, 2014 at 5:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-10-22 at 16:24 +0300, Oleksandr Dmytryshyn wrote:
>> On Wed, Oct 22, 2014 at 2:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
>> >> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >> >>> +static void notify_cpufreq_domains(void)
>> >> >> Why "domains", not "hwdom"? You don't really want to send this
>> >> >> to other than the hardware domain I hope.
>> >> > All domains (not only hwdomain) should receive this interrupt.
>> >> > In case is hwdomain is Linux kernel it can automaticaly recalculate
>> >> > jiffies. But other domains should recalculate jiffies too. I'll
>> >> > implement this mechanism a bit later.
>> >>
>> >> This needs more explanation then: If all domains need to do
>> >> adjustments when the frequency changes, there's something
>> >> wrong here imo. Such changes are supposed to be visible to
>> >> the hypervisor only (with, in the case here, assistance - but
>> >> nothing else - by the hardware domain).
>> >
>> > Guests should be using the arch/generic timers as their time source.
>> > This is required by the hardware to increment at a fixed frequency, so
>> > guests shouldn't need to be aware of the change to the underlying
>> > processor clock, at least not for timekeeping/jiffies purposes.
>> >
>> > The timers are allowed to increment by larger amounts less frequency in
>> > lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
>> > adjust anything for this though IMHO.
>> There are many functions which are using jiffies. What about them?
>> All those functions will measure time not correctly if frequency
>> of the microprocessor will change.
>
> Jiffies is based on the timer tick, isn't it? Therefore it is ultimately
> based on the arch timer.
Thanks for the clarification.

> Ian.
>
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-22 14:00             ` Ian Campbell
  2014-10-22 14:16               ` Oleksandr Dmytryshyn
@ 2014-10-23  9:49               ` Oleksandr Dmytryshyn
  2014-10-23 10:56                 ` Ian Campbell
  1 sibling, 1 reply; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-23  9:49 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Tim Deegan, Stefano Stabellini, Jan Beulich, xen-devel

On Wed, Oct 22, 2014 at 5:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-10-22 at 16:24 +0300, Oleksandr Dmytryshyn wrote:
>> On Wed, Oct 22, 2014 at 2:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
>> >> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >> >>> +static void notify_cpufreq_domains(void)
>> >> >> Why "domains", not "hwdom"? You don't really want to send this
>> >> >> to other than the hardware domain I hope.
>> >> > All domains (not only hwdomain) should receive this interrupt.
>> >> > In case is hwdomain is Linux kernel it can automaticaly recalculate
>> >> > jiffies. But other domains should recalculate jiffies too. I'll
>> >> > implement this mechanism a bit later.
>> >>
>> >> This needs more explanation then: If all domains need to do
>> >> adjustments when the frequency changes, there's something
>> >> wrong here imo. Such changes are supposed to be visible to
>> >> the hypervisor only (with, in the case here, assistance - but
>> >> nothing else - by the hardware domain).
>> >
>> > Guests should be using the arch/generic timers as their time source.
>> > This is required by the hardware to increment at a fixed frequency, so
>> > guests shouldn't need to be aware of the change to the underlying
>> > processor clock, at least not for timekeeping/jiffies purposes.
>> >
>> > The timers are allowed to increment by larger amounts less frequency in
>> > lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
>> > adjust anything for this though IMHO.
>> There are many functions which are using jiffies. What about them?
>> All those functions will measure time not correctly if frequency
>> of the microprocessor will change.
>
> Jiffies is based on the timer tick, isn't it? Therefore it is ultimately
> based on the arch timer.
Even if jiffies is ticked from the timer and what about
'loops_per_jiffy' variable(s)? This variable is declared for ARM and it
should be recalculated in every change of frequency.

> Ian.
>
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-23  9:49               ` Oleksandr Dmytryshyn
@ 2014-10-23 10:56                 ` Ian Campbell
  2014-10-23 11:25                   ` Oleksandr Dmytryshyn
  0 siblings, 1 reply; 42+ messages in thread
From: Ian Campbell @ 2014-10-23 10:56 UTC (permalink / raw)
  To: Oleksandr Dmytryshyn
  Cc: Tim Deegan, Stefano Stabellini, Jan Beulich, xen-devel

On Thu, 2014-10-23 at 12:49 +0300, Oleksandr Dmytryshyn wrote:
> On Wed, Oct 22, 2014 at 5:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2014-10-22 at 16:24 +0300, Oleksandr Dmytryshyn wrote:
> >> On Wed, Oct 22, 2014 at 2:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> > On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
> >> >> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
> >> >> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
> >> >> >>> +static void notify_cpufreq_domains(void)
> >> >> >> Why "domains", not "hwdom"? You don't really want to send this
> >> >> >> to other than the hardware domain I hope.
> >> >> > All domains (not only hwdomain) should receive this interrupt.
> >> >> > In case is hwdomain is Linux kernel it can automaticaly recalculate
> >> >> > jiffies. But other domains should recalculate jiffies too. I'll
> >> >> > implement this mechanism a bit later.
> >> >>
> >> >> This needs more explanation then: If all domains need to do
> >> >> adjustments when the frequency changes, there's something
> >> >> wrong here imo. Such changes are supposed to be visible to
> >> >> the hypervisor only (with, in the case here, assistance - but
> >> >> nothing else - by the hardware domain).
> >> >
> >> > Guests should be using the arch/generic timers as their time source.
> >> > This is required by the hardware to increment at a fixed frequency, so
> >> > guests shouldn't need to be aware of the change to the underlying
> >> > processor clock, at least not for timekeeping/jiffies purposes.
> >> >
> >> > The timers are allowed to increment by larger amounts less frequency in
> >> > lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
> >> > adjust anything for this though IMHO.
> >> There are many functions which are using jiffies. What about them?
> >> All those functions will measure time not correctly if frequency
> >> of the microprocessor will change.
> >
> > Jiffies is based on the timer tick, isn't it? Therefore it is ultimately
> > based on the arch timer.
> Even if jiffies is ticked from the timer and what about
> 'loops_per_jiffy' variable(s)? This variable is declared for ARM and it
> should be recalculated in every change of frequency.

AFAICT it is unused if arch_timers are in use, via the call to
register_current_timer_delay which takes the loops_per_jiffy based delay
function out of the equation.

Rather than making me run around pointing out why each potential use of
LPJ is not relevant in practice one by one please instead point out an
actual case where you see it being used in practice in a Xen guest using
arch timers.

Ian.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver
  2014-10-23 10:56                 ` Ian Campbell
@ 2014-10-23 11:25                   ` Oleksandr Dmytryshyn
  0 siblings, 0 replies; 42+ messages in thread
From: Oleksandr Dmytryshyn @ 2014-10-23 11:25 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Tim Deegan, Stefano Stabellini, Jan Beulich, xen-devel

On Thu, Oct 23, 2014 at 1:56 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-10-23 at 12:49 +0300, Oleksandr Dmytryshyn wrote:
>> On Wed, Oct 22, 2014 at 5:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Wed, 2014-10-22 at 16:24 +0300, Oleksandr Dmytryshyn wrote:
>> >> On Wed, Oct 22, 2014 at 2:00 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> >> > On Wed, 2014-10-22 at 10:30 +0100, Jan Beulich wrote:
>> >> >> >>> On 22.10.14 at 10:43, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >> >> > On Fri, Oct 17, 2014 at 4:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> >> >> >>>>> On 16.10.14 at 13:27, <oleksandr.dmytryshyn@globallogic.com> wrote:
>> >> >> >>> +static void notify_cpufreq_domains(void)
>> >> >> >> Why "domains", not "hwdom"? You don't really want to send this
>> >> >> >> to other than the hardware domain I hope.
>> >> >> > All domains (not only hwdomain) should receive this interrupt.
>> >> >> > In case is hwdomain is Linux kernel it can automaticaly recalculate
>> >> >> > jiffies. But other domains should recalculate jiffies too. I'll
>> >> >> > implement this mechanism a bit later.
>> >> >>
>> >> >> This needs more explanation then: If all domains need to do
>> >> >> adjustments when the frequency changes, there's something
>> >> >> wrong here imo. Such changes are supposed to be visible to
>> >> >> the hypervisor only (with, in the case here, assistance - but
>> >> >> nothing else - by the hardware domain).
>> >> >
>> >> > Guests should be using the arch/generic timers as their time source.
>> >> > This is required by the hardware to increment at a fixed frequency, so
>> >> > guests shouldn't need to be aware of the change to the underlying
>> >> > processor clock, at least not for timekeeping/jiffies purposes.
>> >> >
>> >> > The timers are allowed to increment by larger amounts less frequency in
>> >> > lower power modes, i.e. +=4 every 4 ticks. Guests shouldn't need to
>> >> > adjust anything for this though IMHO.
>> >> There are many functions which are using jiffies. What about them?
>> >> All those functions will measure time not correctly if frequency
>> >> of the microprocessor will change.
>> >
>> > Jiffies is based on the timer tick, isn't it? Therefore it is ultimately
>> > based on the arch timer.
>> Even if jiffies is ticked from the timer and what about
>> 'loops_per_jiffy' variable(s)? This variable is declared for ARM and it
>> should be recalculated in every change of frequency.
>
> AFAICT it is unused if arch_timers are in use, via the call to
> register_current_timer_delay which takes the loops_per_jiffy based delay
> function out of the equation.
>
> Rather than making me run around pointing out why each potential use of
> LPJ is not relevant in practice one by one please instead point out an
> actual case where you see it being used in practice in a Xen guest using
> arch timers.
We are using Linux kernel 3.8 (as Dom0) and Android +  Linux kernel 3.8
and both kernels contain arch_timers (and function
register_current_timer_delay()).
So I will not recalculate LPJ in DomU even if some legacy code is present
which may use LPJ. I'll use VIRQ_CPUFREQ only to notify hwdom to
change frequency.

> Ian.
>

^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2014-10-23 11:25 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-16 11:26 [RFC PATCH v2 00/12] xen_cpufreq implementation in Xen hypervisor Oleksandr Dmytryshyn
2014-10-16 11:26 ` [RFC PATCH v2 01/12] cpufreq: move cpufreq.h file to the xen/include/xen location Oleksandr Dmytryshyn
2014-10-16 11:26 ` [RFC PATCH v2 02/12] pm: move processor_perf.h " Oleksandr Dmytryshyn
2014-10-16 11:26 ` [RFC PATCH v2 03/12] pmstat: move pmstat.c file to the xen/drivers/pm/stat.c location Oleksandr Dmytryshyn
2014-10-16 11:26 ` [RFC PATCH v2 04/12] cpufreq: make turbo settings to be configurable Oleksandr Dmytryshyn
2014-10-16 11:26 ` [RFC PATCH v2 05/12] pmstat: make pmstat functions more generalizable Oleksandr Dmytryshyn
2014-10-17 12:26   ` Jan Beulich
2014-10-22  8:38     ` Oleksandr Dmytryshyn
2014-10-16 11:27 ` [RFC PATCH v2 06/12] cpufreq: make cpufreq driver " Oleksandr Dmytryshyn
2014-10-17 12:31   ` Jan Beulich
2014-10-22  8:39     ` Oleksandr Dmytryshyn
2014-10-22  8:44       ` Oleksandr Dmytryshyn
2014-10-22  9:21       ` Jan Beulich
2014-10-22  9:51         ` Oleksandr Dmytryshyn
2014-10-22  9:57           ` Oleksandr Dmytryshyn
2014-10-22 10:09             ` Jan Beulich
2014-10-16 11:27 ` [RFC PATCH v2 07/12] arch/arm: create device tree nodes for Dom0 cpufreq cpu driver Oleksandr Dmytryshyn
2014-10-16 11:27 ` [RFC PATCH v2 08/12] xsm: enable xsm_platform_op hook for all architectures Oleksandr Dmytryshyn
2014-10-16 11:27 ` [RFC PATCH v2 09/12] xen: arm: implement platform hypercall Oleksandr Dmytryshyn
2014-10-17 12:44   ` Jan Beulich
2014-10-22  8:40     ` Oleksandr Dmytryshyn
2014-10-22  9:24       ` Jan Beulich
2014-10-22 10:36         ` Oleksandr Dmytryshyn
2014-10-16 11:27 ` [RFC PATCH v2 10/12] cpufreq: add dom0-cpufreq driver Oleksandr Dmytryshyn
2014-10-17 13:03   ` Jan Beulich
2014-10-22  8:43     ` Oleksandr Dmytryshyn
2014-10-22  9:30       ` Jan Beulich
2014-10-22 11:00         ` Ian Campbell
2014-10-22 13:24           ` Oleksandr Dmytryshyn
2014-10-22 14:00             ` Ian Campbell
2014-10-22 14:16               ` Oleksandr Dmytryshyn
2014-10-23  9:49               ` Oleksandr Dmytryshyn
2014-10-23 10:56                 ` Ian Campbell
2014-10-23 11:25                   ` Oleksandr Dmytryshyn
2014-10-16 11:27 ` [RFC PATCH v2 11/12] xen: arm: implement XEN_SYSCTL_cpufreq_op Oleksandr Dmytryshyn
2014-10-17 13:21   ` Jan Beulich
2014-10-22  8:46     ` Oleksandr Dmytryshyn
2014-10-22  9:34       ` Jan Beulich
2014-10-22 10:09         ` Oleksandr Dmytryshyn
2014-10-22 10:24           ` Jan Beulich
2014-10-22 10:39             ` Oleksandr Dmytryshyn
2014-10-16 11:27 ` [RFC PATCH v2 12/12] xen/arm: enable cpufreq functionality for ARM Oleksandr Dmytryshyn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.