Xen devel - Nov 2011 - [RFC PATCH] Exporting ACPI Pxx/Cxx states to other kernel subsystems (v1).

Hello,

The following patches are a solution to a problem we have encountered
when using the Xen hypervisor:
 - Need Pxx/Cxx states to save on power consumption when using Xen (we
   do want those datacenters to consume less power!),
 - Also need to figure out the Turbo mode so that the scheduler can properly
   boost a core for CPU bound guests.

In essence the Xen hypervisor requires that information to work efficiently.

There are some other ways of solving this problem as well that have
been tossed around. I am enumerating them here, but I don''t have the
full records of the disadvantages/advantges so hopefully some other people
can chime in.
 - Parse the information in userspace. I am not really sure how that would
   work, but one thought was export in SysFS the Pxx/Cxx states and other ones
   (hand-waving here) and the libvirt/xend/xl toolstack would parse those as
   needed and push them up (say when the guest is started). But it does have
   a disadvantage that it would not work well with CPU hotplug, nor with the
   physical CPU != virtual CPU discountinty (that is Linux sees only one CPU,
   and the ACPI Pxx says there are eight of them - with different values than
   the CPU0).
 - Make the ACPI subsystem export some of this functionality so that there
   can be multiple ACPI processor drivers and they can share common code.
   This is what this patch series follows. This does mean exporting some
   ACPI "stuff" outside drivers/acpi. 
 - Implement the ACPI DSDT parsing in the hypervisor. The couple of reasons
   that pop in my head for not going that route is foremost it seems an
   overkill just so that this specific information (Pxx/Cxx) can pushed in
   the hypervisor.  We already use the Linux ACPI to figure out the IRQ routing,
   or for ACPI states changes like button pushing, docking events, etc.
   Making it all be done in the hypervisor seems well, an overkill.

The original authors of the code are Intel folks. Liang has been working
on this to see if the abstraction can be improved, but I am by no means
an ACPI expert - so feedback, guidance or input would be much appreciated.


Liang:
 ACPI: processor: Don''t setup cpu idle driver and handler

I think it can be actually dropped - now that disable_cpuidle() functionality
exists and we use it? 

Kevin Tian (6):
      ACPI: processor: export necessary interfaces
      ACPI: processor: cache acpi_power_register in cx structure
      ACPI: processor: Don''t setup cpu idle driver and handler when we
do not want them.
      ACPI: add processor driver for Xen virtual CPUs.
      ACPI: xen processor: add PM notification interfaces.
      ACPI: xen processor: set ignore_ppc to handle PPC event for Xen vcpu.

Tang Liang (2):
      ACPI: processor: add __acpi_processor_[un]register_driver helpers.
      ACPI: processor: override the interface of register acpi processor handler
for Xen vcpu

 drivers/acpi/Makefile            |    1 +
 drivers/acpi/processor_driver.c  |   48 +++++--
 drivers/acpi/processor_idle.c    |   10 +-
 drivers/acpi/processor_perflib.c |    6 +-
 drivers/acpi/processor_xen.c     |  246 ++++++++++++++++++++++++++++++++++
 drivers/xen/Kconfig              |    5 +
 drivers/xen/Makefile             |    3 +
 drivers/xen/acpi_processor.c     |  269 ++++++++++++++++++++++++++++++++++++++
 include/acpi/processor.h         |   20 +++-
 include/xen/acpi.h               |  104 +++++++++++++++
 10 files changed, 690 insertions(+), 22 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Kevin Tian <kevin.tian@intel.com>

This patch export some necessary functions which parse processor
power management information. The Xen ACPI processor driver uses them.

Signed-off-by: Yu Ke <ke.yu@intel.com>
Signed-off-by: Tian Kevin <kevin.tian@intel.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/processor_driver.c  |   11 +++--------
 drivers/acpi/processor_perflib.c |    4 ++--
 include/acpi/processor.h         |   10 +++++++++-
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 9d7bc9f..211c078 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -79,9 +79,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Processor Driver");
 MODULE_LICENSE("GPL");
 
-static int acpi_processor_add(struct acpi_device *device);
-static int acpi_processor_remove(struct acpi_device *device, int type);
-static void acpi_processor_notify(struct acpi_device *device, u32 event);
 static acpi_status acpi_processor_hotadd_init(acpi_handle handle, int *p_cpu);
 static int acpi_processor_handle_eject(struct acpi_processor *pr);
 
@@ -378,7 +375,7 @@ static int acpi_processor_get_info(struct acpi_device
*device)
 
 static DEFINE_PER_CPU(void *, processor_device_array);
 
-static void acpi_processor_notify(struct acpi_device *device, u32 event)
+void acpi_processor_notify(struct acpi_device *device, u32 event)
 {
 	struct acpi_processor *pr = acpi_driver_data(device);
 	int saved;
@@ -442,7 +439,7 @@ static struct notifier_block acpi_cpu_notifier  	   
.notifier_call = acpi_cpu_soft_notify,
 };
 
-static int __cpuinit acpi_processor_add(struct acpi_device *device)
+int __cpuinit acpi_processor_add(struct acpi_device *device)
 {
 	struct acpi_processor *pr = NULL;
 	int result = 0;
@@ -545,7 +542,7 @@ err_free_cpumask:
 	return result;
 }
 
-static int acpi_processor_remove(struct acpi_device *device, int type)
+int acpi_processor_remove(struct acpi_device *device, int type)
 {
 	struct acpi_processor *pr = NULL;
 
@@ -758,7 +755,6 @@ static int acpi_processor_handle_eject(struct acpi_processor
*pr)
 }
 #endif
 
-static
 void acpi_processor_install_hotplug_notify(void)
 {
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
@@ -771,7 +767,6 @@ void acpi_processor_install_hotplug_notify(void)
 	register_hotcpu_notifier(&acpi_cpu_notifier);
 }
 
-static
 void acpi_processor_uninstall_hotplug_notify(void)
 {
 #ifdef CONFIG_ACPI_HOTPLUG_CPU
diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 85b3237..22c6195 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -386,7 +386,7 @@ static int acpi_processor_get_performance_states(struct
acpi_processor *pr)
 	return result;
 }
 
-static int acpi_processor_get_performance_info(struct acpi_processor *pr)
+int acpi_processor_get_performance_info(struct acpi_processor *pr)
 {
 	int result = 0;
 	acpi_status status = AE_OK;
@@ -492,7 +492,7 @@ int acpi_processor_notify_smm(struct module *calling_module)
 
 EXPORT_SYMBOL(acpi_processor_notify_smm);
 
-static int acpi_processor_get_psd(struct acpi_processor	*pr)
+int acpi_processor_get_psd(struct acpi_processor	*pr)
 {
 	int result = 0;
 	acpi_status status = AE_OK;
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 610f6fb..12657bb 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -239,6 +239,12 @@ extern void acpi_processor_unregister_performance(struct
          if a _PPC object exists, rmmod is disallowed then */
 int acpi_processor_notify_smm(struct module *calling_module);
 
+void acpi_processor_install_hotplug_notify(void);
+void acpi_processor_uninstall_hotplug_notify(void);
+
+int acpi_processor_add(struct acpi_device *device);
+int acpi_processor_remove(struct acpi_device *device, int type);
+void acpi_processor_notify(struct acpi_device *device, u32 event);
 /* for communication between multiple parts of the processor kernel module */
 DECLARE_PER_CPU(struct acpi_processor *, processors);
 extern struct acpi_processor_errata errata;
@@ -278,7 +284,9 @@ static inline void acpi_processor_ffh_cstate_enter(struct
acpi_processor_cx
 void acpi_processor_ppc_init(void);
 void acpi_processor_ppc_exit(void);
 int acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag);
-extern int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
+int acpi_processor_get_performance_info(struct acpi_processor *pr);
+int acpi_processor_get_psd(struct acpi_processor *pr);
+int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
 #else
 static inline void acpi_processor_ppc_init(void)
 {
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Kevin Tian <kevin.tian@intel.com>

This patch save acpi_power_register in cx structure because we need
pass this to the Xen ACPI processor driver.

Signed-off-by: Yu Ke <ke.yu@intel.com>
Signed-off-by: Tian Kevin <kevin.tian@intel.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/processor_idle.c |    2 +-
 include/acpi/processor.h      |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 0e8e2de..d88974a 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -418,7 +418,7 @@ static int acpi_processor_get_power_info_cst(struct
acpi_processor *pr)
 		if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO &&
 		    (reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE))
 			continue;
-
+		memcpy(&(cx.reg), reg, sizeof(*reg));
 		/* There should be an easy way to extract an integer... */
 		obj = &(element->package.elements[1]);
 		if (obj->type != ACPI_TYPE_INTEGER)
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 12657bb..bd99fb6 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -65,6 +65,7 @@ struct acpi_processor_cx {
 	u64 time;
 	u8 bm_sts_skip;
 	char desc[ACPI_CX_DESC_LEN];
+	struct acpi_power_register reg;
 };
 
 struct acpi_processor_power {
-- 
1.7.7.3

From: Tang Liang <liang.tang@oracle.com>

This patch implement __acpi_processor_[un]register_driver helper,
so we can registry override processor driver function. Specifically
the Xen processor driver.

By default the values are set to the native one.

Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/processor_driver.c |   35 +++++++++++++++++++++++++++++------
 include/acpi/processor.h        |    6 +++++-
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 211c078..55f0b89 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -90,6 +90,11 @@ static const struct acpi_device_id processor_device_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, processor_device_ids);
 
+int (*__acpi_processor_register_driver)(void) = acpi_processor_register_driver;
+void (*__acpi_processor_unregister_driver)(void) \
+	= acpi_processor_unregister_driver;
+
+
 static struct acpi_driver acpi_processor_driver = {
 	.name = "processor",
 	.class = ACPI_PROCESSOR_CLASS,
@@ -779,6 +784,22 @@ void acpi_processor_uninstall_hotplug_notify(void)
 	unregister_hotcpu_notifier(&acpi_cpu_notifier);
 }
 
+int acpi_processor_register_driver(void)
+{
+	int result = 0;
+
+	result = acpi_bus_register_driver(&acpi_processor_driver);
+	return result;
+}
+
+void acpi_processor_unregister_driver(void)
+{
+	acpi_bus_unregister_driver(&acpi_processor_driver);
+
+	cpuidle_unregister_driver(&acpi_idle_driver);
+
+	return;
+}
 /*
  * We keep the driver loaded even when ACPI is not running.
  * This is needed for the powernow-k8 driver, that works even without
@@ -794,9 +815,11 @@ static int __init acpi_processor_init(void)
 
 	memset(&errata, 0, sizeof(errata));
 
-	result = acpi_bus_register_driver(&acpi_processor_driver);
-	if (result < 0)
-		return result;
+	if (__acpi_processor_register_driver) {
+		result = __acpi_processor_register_driver();
+		if (result < 0)
+			return result;
+	}
 
 	acpi_processor_install_hotplug_notify();
 
@@ -809,6 +832,7 @@ static int __init acpi_processor_init(void)
 	return 0;
 }
 
+
 static void __exit acpi_processor_exit(void)
 {
 	if (acpi_disabled)
@@ -820,9 +844,8 @@ static void __exit acpi_processor_exit(void)
 
 	acpi_processor_uninstall_hotplug_notify();
 
-	acpi_bus_unregister_driver(&acpi_processor_driver);
-
-	cpuidle_unregister_driver(&acpi_idle_driver);
+	if (__acpi_processor_unregister_driver)
+		__acpi_processor_unregister_driver();
 
 	return;
 }
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index bd99fb6..969cbc9 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -225,6 +225,9 @@ struct acpi_processor_errata {
 	} piix4;
 };
 
+extern int (*__acpi_processor_register_driver)(void);
+extern void (*__acpi_processor_unregister_driver)(void);
+
 extern int acpi_processor_preregister_performance(struct
 						  acpi_processor_performance
 						  __percpu *performance);
@@ -242,7 +245,8 @@ int acpi_processor_notify_smm(struct module
*calling_module);
 
 void acpi_processor_install_hotplug_notify(void);
 void acpi_processor_uninstall_hotplug_notify(void);
-
+int acpi_processor_register_driver(void);
+void acpi_processor_unregister_driver(void);
 int acpi_processor_add(struct acpi_device *device);
 int acpi_processor_remove(struct acpi_device *device, int type);
 void acpi_processor_notify(struct acpi_device *device, u32 event);
-- 
1.7.7.3

From: Kevin Tian <kevin.tian@intel.com>

This patch inhibits processing of the CPU idle handler if it is not
set to the appropiate one. This is needed by the Xen processor driver
which, while still needing processor details, wants to use the default_idle
call (which makes a yield hypercall).

Signed-off-by: Yu Ke <ke.yu@intel.com>
Signed-off-by: Tian Kevin <kevin.tian@intel.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/processor_idle.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index d88974a..0ad347f 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1127,7 +1127,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
 	cpuidle_pause_and_lock();
 	cpuidle_disable_device(&pr->power.dev);
 	acpi_processor_get_power_info(pr);
-	if (pr->flags.power) {
+	if (pr->flags.power  && (cpuidle_get_driver() ==
&acpi_idle_driver)) {
 		acpi_processor_setup_cpuidle_cx(pr);
 		ret = cpuidle_enable_device(&pr->power.dev);
 	}
@@ -1183,7 +1183,8 @@ int acpi_processor_cst_has_changed(struct acpi_processor
*pr)
 			if (!_pr || !_pr->flags.power_setup_done)
 				continue;
 			acpi_processor_get_power_info(_pr);
-			if (_pr->flags.power) {
+			if (_pr->flags.power && (cpuidle_get_driver()
+						== &acpi_idle_driver)) {
 				acpi_processor_setup_cpuidle_cx(_pr);
 				cpuidle_enable_device(&_pr->power.dev);
 			}
@@ -1237,7 +1238,8 @@ int __cpuinit acpi_processor_power_init(struct
acpi_processor *pr,
 	 * Note that we use previously set idle handler will be used on
 	 * platforms that only support C1.
 	 */
-	if (pr->flags.power) {
+	if (pr->flags.power && (__acpi_processor_register_driver =+			
acpi_processor_register_driver)) {
 		/* Register acpi_idle_driver if not already registered */
 		if (!acpi_processor_registered) {
 			acpi_processor_setup_cpuidle_states(pr);
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Kevin Tian <kevin.tian@intel.com>

Because the processor is controlled by the VMM in xen,
we need new acpi processor driver for Xen virtual CPU.

Specifically we need to be able to pass the CXX/PXX states
to the hypervisor, and as well deal with the peculiarity
that the amount of CPUs that Linux parses in the ACPI
is different from the amount visible to the Linux kernel.

Signed-off-by: Yu Ke <ke.yu@intel.com>
Signed-off-by: Tian Kevin <kevin.tian@intel.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/Makefile        |    1 +
 drivers/acpi/processor_xen.c |  244 ++++++++++++++++++++++++++++++++++++++++++
 drivers/xen/Kconfig          |    5 +
 drivers/xen/Makefile         |    3 +
 drivers/xen/acpi_processor.c |   53 +++++++++
 include/xen/acpi.h           |  104 ++++++++++++++++++
 6 files changed, 410 insertions(+), 0 deletions(-)
 create mode 100644 drivers/acpi/processor_xen.c
 create mode 100644 drivers/xen/acpi_processor.c
 create mode 100644 include/xen/acpi.h

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index ecb26b4..1f39861f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o processor_throttling.o
 processor-y			+= processor_idle.o processor_thermal.o
+processor-y			+= processor_xen.o
 processor-$(CONFIG_CPU_FREQ)	+= processor_perflib.o
 
 obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
diff --git a/drivers/acpi/processor_xen.c b/drivers/acpi/processor_xen.c
new file mode 100644
index 0000000..fc3cc0b
--- /dev/null
+++ b/drivers/acpi/processor_xen.c
@@ -0,0 +1,244 @@
+/*
+ * processor_xen.c - ACPI Processor Driver for xen
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  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.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <acpi/acpi_drivers.h>
+#include <acpi/processor.h>
+#include <xen/acpi.h>
+#include <linux/export.h>
+
+#define PREFIX "ACPI: "
+
+#define ACPI_PROCESSOR_CLASS            "processor"
+#define ACPI_PROCESSOR_NOTIFY_PERFORMANCE 0x80
+#define ACPI_PROCESSOR_NOTIFY_POWER	0x81
+#define ACPI_PROCESSOR_NOTIFY_THROTTLING	0x82
+
+#define _COMPONENT              ACPI_PROCESSOR_COMPONENT
+ACPI_MODULE_NAME("processor_xen");
+
+#if defined(CONFIG_ACPI_PROCESSOR_XEN) || \
+defined(CONFIG_ACPI_PROCESSOR_XEN_MODULE)
+static const struct acpi_device_id processor_device_ids[] = {
+	{ACPI_PROCESSOR_OBJECT_HID, 0},
+	{"ACPI0007", 0},
+	{"", 0},
+};
+
+static int xen_acpi_processor_add(struct acpi_device *device);
+static void xen_acpi_processor_notify(struct acpi_device *device, u32 event);
+
+struct acpi_driver xen_acpi_processor_driver = {
+	.name = "processor",
+	.class = ACPI_PROCESSOR_CLASS,
+	.ids = processor_device_ids,
+	.ops = {
+		.add = xen_acpi_processor_add,
+		.remove = acpi_processor_remove,
+		.suspend = acpi_processor_suspend,
+		.resume = acpi_processor_resume,
+		.notify = xen_acpi_processor_notify,
+		},
+};
+
+#ifdef CONFIG_CPU_FREQ
+/*
+ * Existing ACPI module does parse performance states at some point,
+ * when acpi-cpufreq driver is loaded which however is something
+ * we''d like to disable to avoid confliction with xen PM
+ * logic. So we have to collect raw performance information here
+ * when ACPI processor object is found and started.
+ */
+static int xen_acpi_processor_get_performance(struct acpi_processor *pr)
+{
+	int ret;
+	struct acpi_processor_performance *perf;
+	struct acpi_psd_package *pdomain;
+
+	if (pr->performance)
+		return -EBUSY;
+
+	perf = kzalloc(sizeof(struct acpi_processor_performance), GFP_KERNEL);
+	if (!perf)
+		return -ENOMEM;
+
+	pr->performance = perf;
+	/* Get basic performance state information */
+	ret = acpi_processor_get_performance_info(pr);
+	if (ret < 0)
+		goto err_out;
+
+	/* invoke raw psd parse interface directly, as it''s useless to
+	 * construct a shared map around dom0''s vcpu ID.
+	 */
+	pdomain = &pr->performance->domain_info;
+	pdomain->num_processors = 0;
+	ret = acpi_processor_get_psd(pr);
+	if (ret < 0) {
+		/*
+		 * _PSD is optional - assume no coordination if absent (or
+		 * broken), matching native kernels'' behavior.
+		 */
+		pdomain->num_entries = ACPI_PSD_REV0_ENTRIES;
+		pdomain->revision = ACPI_PSD_REV0_REVISION;
+		pdomain->domain = pr->acpi_id;
+		pdomain->coord_type = DOMAIN_COORD_TYPE_SW_ALL;
+		pdomain->num_processors = 1;
+	}
+
+	processor_cntl_xen_notify(pr, PROCESSOR_PM_INIT, PM_TYPE_PERF);
+
+	/* Last step is to notify BIOS that xen exists */
+	acpi_processor_notify_smm(THIS_MODULE);
+
+	return 0;
+err_out:
+	pr->performance = NULL;
+	kfree(perf);
+	return ret;
+}
+#endif /* CONFIG_CPU_FREQ */
+
+static int __cpuinit xen_acpi_processor_add(struct acpi_device *device)
+{
+	struct acpi_processor *pr = NULL;
+	int result = 0;
+
+	result = acpi_processor_add(device);
+	if (result < 0)
+		return result;
+
+	pr = acpi_driver_data(device);
+	if (!pr)
+		return -EINVAL;
+
+	if (pr->id == -1) {
+		int device_declaration;
+		int apic_id = -1;
+
+		if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID))
+			device_declaration = 0;
+		else
+			device_declaration = 1;
+
+		apic_id = acpi_get_cpuid(pr->handle,
+			device_declaration, pr->acpi_id);
+		if (apic_id == -1) {
+			/* Processor is not present in MADT table */
+			return 0;
+		}
+
+		/*
+		 * It''s possible to have pr->id as ''-1'' even
when it''s actually
+		 * present in MADT table, e.g. due to limiting dom0 max vcpus
+		 * less than physical present number. In such case we still want
+		 * to parse ACPI processor object information, so mimic the
+		 * pr->id to CPU-0. This should be safe because we only care
+		 * about raw ACPI information, which only relies on pr->acpi_id.
+		 * For other information relying on pr->id and gathered through
+		 * SMP function call, it''s safe to let them run on CPU-0 since
+		 * underlying Xen will collect them. Only a valid pr->id can
+		 * make later invocations forward progress.
+		 */
+		pr->id = 0;
+	}
+
+	if (likely(!pr->flags.power_setup_done)) {
+		/* reset idle boot option which we don''t care */
+		boot_option_idle_override = IDLE_NO_OVERRIDE;
+		acpi_processor_power_init(pr, device);
+		/* set to IDLE_HALT for trapping into Xen */
+		boot_option_idle_override = IDLE_HALT;
+
+		if (pr->flags.power)
+			processor_cntl_xen_notify(pr,
+					PROCESSOR_PM_INIT, PM_TYPE_IDLE);
+	}
+
+#ifdef CONFIG_CPU_FREQ
+	if (likely(!pr->performance))
+		xen_acpi_processor_get_performance(pr);
+#endif
+
+	return 0;
+}
+
+static void xen_acpi_processor_notify(struct acpi_device *device, u32 event)
+{
+	struct acpi_processor *pr = acpi_driver_data(device);
+
+	if (!pr)
+		return;
+
+	acpi_processor_notify(device, event);
+
+	switch (event) {
+	case ACPI_PROCESSOR_NOTIFY_PERFORMANCE:
+#ifdef CONFIG_CPU_FREQ
+		processor_cntl_xen_notify(pr,
+				PROCESSOR_PM_CHANGE, PM_TYPE_PERF);
+#endif
+		break;
+	case ACPI_PROCESSOR_NOTIFY_POWER:
+		processor_cntl_xen_notify(pr,
+				PROCESSOR_PM_CHANGE, PM_TYPE_IDLE);
+		break;
+	default:
+		break;
+	}
+
+	return;
+}
+
+/* init and exit */
+
+/* we don''t install acpi cpuidle driver because dom0 itself is running
+ * as a guest which has no knowledge whether underlying is actually idle
+ */
+int xen_acpi_processor_init(void)
+{
+	int result = 0;
+
+	result = acpi_bus_register_driver(&xen_acpi_processor_driver);
+	if (result < 0)
+		return result;
+		/* mark ready for handling ppc */
+
+	return 0;
+}
+
+void xen_acpi_processor_exit(void)
+{
+
+	acpi_bus_unregister_driver(&xen_acpi_processor_driver);
+}
+#endif
+
+#if defined(CONFIG_ACPI_PROCESSOR_XEN) || \
+defined(CONFIG_ACPI_PROCESSOR_XEN_MODULE)
+void xen_processor_driver_register(void)
+{
+	if (xen_initial_domain()) {
+		__acpi_processor_register_driver = xen_acpi_processor_init;
+		__acpi_processor_unregister_driver = xen_acpi_processor_exit;
+	}
+}
+#else
+void xen_processor_driver_register(void)
+{
+}
+#endif
diff --git a/drivers/xen/Kconfig b/drivers/xen/Kconfig
index 8795480..640bbf5 100644
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -117,6 +117,11 @@ config XEN_SYS_HYPERVISOR
 	 virtual environment, /sys/hypervisor will still be present,
 	 but will have no xen contents.
 
+config ACPI_PROCESSOR_XEN
+	tristate
+	depends on XEN_DOM0 && ACPI_PROCESSOR && CPU_FREQ
+	default m
+
 config XEN_XENBUS_FRONTEND
 	tristate
 
diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
index 974fffd..f67450c 100644
--- a/drivers/xen/Makefile
+++ b/drivers/xen/Makefile
@@ -19,6 +19,9 @@ obj-$(CONFIG_XEN_TMEM)			+= tmem.o
 obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
 obj-$(CONFIG_XEN_DOM0)			+= pci.o
 obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
+ifdef CONFIG_ACPI_PROCESSOR_XEN
+obj-$(CONFIG_ACPI_PROCESSOR)		+= acpi_processor.o
+endif
 
 xen-evtchn-y				:= evtchn.o
 xen-gntdev-y				:= gntdev.o
diff --git a/drivers/xen/acpi_processor.c b/drivers/xen/acpi_processor.c
new file mode 100644
index 0000000..8fa7914
--- /dev/null
+++ b/drivers/xen/acpi_processor.c
@@ -0,0 +1,53 @@
+/*
+ *  acpi_processor.c - interface to notify Xen on acpi processor object
+ *                     info parsing
+ *
+ *  Copyright (C) 2008, Intel corporation
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  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.
+ *
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/cpufreq.h>
+#include <acpi/processor.h>
+#include <xen/acpi.h>
+
+#include <asm/xen/hypercall.h>
+#include <asm/xen/hypervisor.h>
+
+static struct processor_cntl_xen_ops xen_ops;
+int processor_cntl_xen_notify(struct acpi_processor *pr, int event, int type)
+{
+	int ret = -EINVAL;
+
+	switch (event) {
+	case PROCESSOR_PM_INIT:
+	case PROCESSOR_PM_CHANGE:
+		if ((type >= PM_TYPE_MAX) ||
+			!xen_ops.pm_ops[type])
+			break;
+
+		ret = xen_ops.pm_ops[type](pr, event);
+		break;
+	default:
+		printk(KERN_ERR "Unsupport processor events %d.\n", event);
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(processor_cntl_xen_notify);
+
+MODULE_LICENSE("GPL");
diff --git a/include/xen/acpi.h b/include/xen/acpi.h
new file mode 100644
index 0000000..99e1270
--- /dev/null
+++ b/include/xen/acpi.h
@@ -0,0 +1,104 @@
+/******************************************************************************
+ * acpi.h
+ * acpi file for domain 0 kernel
+ *
+ * Copyright (c) 2011 Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
+ * Copyright (c) 2011 Yu Ke <ke.yu@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation; or, when distributed
+ * separately from the Linux kernel or incorporated into other
+ * software packages, subject to the following license:
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this source file (the "Software"), to deal in the Software
without
+ * restriction, including without limitation the rights to use, copy, modify,
+ * merge, publish, distribute, sublicense, and/or sell copies of the Software,
+ * and to permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef _XEN_ACPI_H
+#define _XEN_ACPI_H
+
+#include <linux/types.h>
+#include <acpi/acpi_drivers.h>
+#include <acpi/processor.h>
+
+/*
+ * Following are interfaces for xen acpi processor control
+ */
+#if defined(CONFIG_ACPI_PROCESSOR_XEN) || \
+defined(CONFIG_ACPI_PROCESSOR_XEN_MODULE)
+/* Events notified to xen */
+#define PROCESSOR_PM_INIT	1
+#define PROCESSOR_PM_CHANGE	2
+#define PROCESSOR_HOTPLUG	3
+
+/* Objects for the PM events */
+#define PM_TYPE_IDLE		0
+#define PM_TYPE_PERF		1
+#define PM_TYPE_THR		2
+#define PM_TYPE_MAX		3
+
+#define XEN_MAX_ACPI_ID 255
+
+/* Processor hotplug events */
+#define HOTPLUG_TYPE_ADD	0
+#define HOTPLUG_TYPE_REMOVE	1
+
+extern int (*__acpi_processor_register_driver)(void);
+extern void (*__acpi_processor_unregister_driver)(void);
+#endif
+
+#ifndef CONFIG_CPU_FREQ
+static inline int xen_acpi_processor_get_performance(struct acpi_processor *pr)
+{
+	printk(KERN_WARNING
+		"Warning: xen_acpi_processor_get_performance not supported\n"
+		"Consider compiling CPUfreq support into your kernel.\n");
+	return 0;
+}
+#endif
+
+#if defined(CONFIG_ACPI_PROCESSOR_XEN) || \
+defined(CONFIG_ACPI_PROCESSOR_XEN_MODULE)
+
+struct processor_cntl_xen_ops {
+	/* Transfer processor PM events to xen */
+int (*pm_ops[PM_TYPE_MAX])(struct acpi_processor *pr, int event);
+	/* Notify physical processor status to xen */
+	int (*hotplug)(struct acpi_processor *pr, int type);
+};
+
+extern int processor_cntl_xen_notify(struct acpi_processor *pr,
+			int event, int type);
+extern int processor_cntl_xen_power_cache(int cpu, int cx,
+		struct acpi_power_register *reg);
+#else
+
+static inline int processor_cntl_xen_notify(struct acpi_processor *pr,
+			int event, int type)
+{
+	return 0;
+}
+static inline int processor_cntl_xen_power_cache(int cpu, int cx,
+		struct acpi_power_register *reg)
+{
+	return 0;
+}
+#endif /* CONFIG_ACPI_PROCESSOR_XEN */
+
+#endif	/* _XEN_ACPI_H */
-- 
1.7.7.3

From: Tang Liang <liang.tang@oracle.com>

This patch calls the check which detectes whether to override
the interface to register ACPI processor.

Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/processor_driver.c |    2 ++
 include/acpi/processor.h        |    4 ++++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 55f0b89..2fec82e 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -815,6 +815,8 @@ static int __init acpi_processor_init(void)
 
 	memset(&errata, 0, sizeof(errata));
 
+	xen_processor_driver_register();
+
 	if (__acpi_processor_register_driver) {
 		result = __acpi_processor_register_driver();
 		if (result < 0)
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 969cbc9..cf53ed8 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -283,6 +283,10 @@ static inline void acpi_processor_ffh_cstate_enter(struct
acpi_processor_cx
 }
 #endif
 
+/* in processor_xen.c */
+extern void xen_processor_driver_register(void);
+
+
 /* in processor_perflib.c */
 
 #ifdef CONFIG_CPU_FREQ
-- 
1.7.7.3

From: Kevin Tian <kevin.tian@intel.com>

Since cpu power is controlled by VMM in Xen, to provide
that information to the VMM, we have to use hypercall to exchange
power management state between domain with hypervisor.

Signed-off-by: Yu Ke <ke.yu@intel.com>
Signed-off-by: Tian Kevin <kevin.tian@intel.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/acpi_processor.c |  216 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 216 insertions(+), 0 deletions(-)

diff --git a/drivers/xen/acpi_processor.c b/drivers/xen/acpi_processor.c
index 8fa7914..23730d8 100644
--- a/drivers/xen/acpi_processor.c
+++ b/drivers/xen/acpi_processor.c
@@ -24,6 +24,7 @@
 #include <acpi/processor.h>
 #include <xen/acpi.h>
 
+#include <xen/interface/platform.h>
 #include <asm/xen/hypercall.h>
 #include <asm/xen/hypervisor.h>
 
@@ -50,4 +51,219 @@ int processor_cntl_xen_notify(struct acpi_processor *pr, int
event, int type)
 }
 EXPORT_SYMBOL(processor_cntl_xen_notify);
 
+static inline void xen_convert_pct_reg(struct xen_pct_register *xpct,
+	struct acpi_pct_register *apct)
+{
+	xpct->descriptor = apct->descriptor;
+	xpct->length     = apct->length;
+	xpct->space_id   = apct->space_id;
+	xpct->bit_width  = apct->bit_width;
+	xpct->bit_offset = apct->bit_offset;
+	xpct->reserved   = apct->reserved;
+	xpct->address    = apct->address;
+}
+
+static inline void xen_convert_pss_states(struct xen_processor_px *xpss,
+	struct acpi_processor_px *apss, int state_count)
+{
+	int i;
+	for (i = 0; i < state_count; i++) {
+		xpss->core_frequency     = apss->core_frequency;
+		xpss->power              = apss->power;
+		xpss->transition_latency = apss->transition_latency;
+		xpss->bus_master_latency = apss->bus_master_latency;
+		xpss->control            = apss->control;
+		xpss->status             = apss->status;
+		xpss++;
+		apss++;
+	}
+}
+
+static inline void xen_convert_psd_pack(struct xen_psd_package *xpsd,
+	struct acpi_psd_package *apsd)
+{
+	xpsd->num_entries    = apsd->num_entries;
+	xpsd->revision       = apsd->revision;
+	xpsd->domain         = apsd->domain;
+	xpsd->coord_type     = apsd->coord_type;
+	xpsd->num_processors = apsd->num_processors;
+}
+
+static int xen_cx_notifier(struct acpi_processor *pr, int action)
+{
+	int ret, count = 0, i;
+	struct xen_platform_op op = {
+		.cmd			= XENPF_set_processor_pminfo,
+		.interface_version	= XENPF_INTERFACE_VERSION,
+		.u.set_pminfo.id	= pr->acpi_id,
+		.u.set_pminfo.type	= XEN_PM_CX,
+	};
+	struct xen_processor_cx *data, *buf;
+	struct acpi_processor_cx *cx;
+	struct acpi_power_register *reg;
+
+	if (action == PROCESSOR_PM_CHANGE)
+		return -EINVAL;
+
+	/* Convert to Xen defined structure and hypercall */
+	buf = kzalloc(pr->power.count * sizeof(struct xen_processor_cx)\
+		, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	data = buf;
+	for (i = 1; i <= pr->power.count; i++) {
+		cx = &pr->power.states[i];
+		reg = &cx->reg;
+		/* Skip invalid cstate entry */
+		if (!cx->valid)
+			continue;
+
+		data->type = cx->type;
+		data->latency = cx->latency;
+		data->power = cx->power;
+		data->reg.space_id = reg->space_id;
+		data->reg.bit_width = reg->bit_width;
+		data->reg.bit_offset = reg->bit_offset;
+		data->reg.access_size = reg->access_size;
+		data->reg.address = reg->address;
+
+		/* Get dependency relationships, _CSD is not supported yet */
+		data->dpcnt = 0;
+		set_xen_guest_handle(data->dp, NULL);
+
+		data++;
+		count++;
+	}
+
+	if (!count) {
+		printk(KERN_ERR "No available Cx info for cpu %d\n",
+				pr->acpi_id);
+		kfree(buf);
+		return -EINVAL;
+	}
+
+	op.u.set_pminfo.power.count = count;
+	op.u.set_pminfo.power.flags.bm_control = pr->flags.bm_control;
+	op.u.set_pminfo.power.flags.bm_check = pr->flags.bm_check;
+	op.u.set_pminfo.power.flags.has_cst = pr->flags.has_cst;
+	op.u.set_pminfo.power.flags.power_setup_done +		pr->flags.power_setup_done;
+
+	set_xen_guest_handle(op.u.set_pminfo.power.states, buf);
+	ret = HYPERVISOR_dom0_op(&op);
+	kfree(buf);
+	return ret;
+}
+
+static int xen_px_notifier(struct acpi_processor *pr, int action)
+{
+	int ret = -EINVAL;
+	struct xen_platform_op op = {
+		.cmd			= XENPF_set_processor_pminfo,
+		.interface_version	= XENPF_INTERFACE_VERSION,
+		.u.set_pminfo.id	= pr->acpi_id,
+		.u.set_pminfo.type	= XEN_PM_PX,
+	};
+	struct xen_processor_performance *perf;
+	struct xen_processor_px *states = NULL;
+	struct acpi_processor_performance *px;
+	struct acpi_psd_package *pdomain;
+
+	if (!pr)
+		return -EINVAL;
+
+	perf = &op.u.set_pminfo.perf;
+	px = pr->performance;
+
+	switch (action) {
+	case PROCESSOR_PM_CHANGE:
+		/* ppc dynamic handle */
+		perf->flags = XEN_PX_PPC;
+		perf->platform_limit = pr->performance_platform_limit;
+
+		ret = HYPERVISOR_dom0_op(&op);
+		break;
+
+	case PROCESSOR_PM_INIT:
+		/* px normal init */
+		perf->flags = XEN_PX_PPC |
+			      XEN_PX_PCT |
+			      XEN_PX_PSS |
+			      XEN_PX_PSD;
+
+		/* ppc */
+		perf->platform_limit = pr->performance_platform_limit;
+
+		/* pct */
+		xen_convert_pct_reg(&perf->control_register,
+				&px->control_register);
+		xen_convert_pct_reg(&perf->status_register,
+				&px->status_register);
+
+		/* pss */
+		perf->state_count = px->state_count;
+		states = kzalloc(px->state_count*\
+			sizeof(struct xen_processor_px), GFP_KERNEL);
+		if (!states)
+			return -ENOMEM;
+		xen_convert_pss_states(states, px->states, px->state_count);
+		set_xen_guest_handle(perf->states, states);
+
+		/* psd */
+		pdomain = &px->domain_info;
+		/* Some sanity check */
+		if ((pdomain->revision != ACPI_PSD_REV0_REVISION) ||
+			(pdomain->num_entries != ACPI_PSD_REV0_ENTRIES) ||
+			((pdomain->coord_type != DOMAIN_COORD_TYPE_SW_ALL) &&
+			(pdomain->coord_type != DOMAIN_COORD_TYPE_SW_ANY) &&
+			(pdomain->coord_type != DOMAIN_COORD_TYPE_HW_ALL))) {
+			ret = -EINVAL;
+			kfree(states);
+			break;
+		}
+
+		xen_convert_psd_pack(&perf->domain_info, pdomain);
+		if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
+			perf->shared_type = CPUFREQ_SHARED_TYPE_ALL;
+		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY)
+			perf->shared_type = CPUFREQ_SHARED_TYPE_ANY;
+		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
+			perf->shared_type = CPUFREQ_SHARED_TYPE_HW;
+		else {
+			ret = -EINVAL;
+			kfree(states);
+			break;
+		}
+
+		ret = HYPERVISOR_dom0_op(&op);
+		kfree(states);
+		break;
+
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int __init xen_acpi_processor_extcntl_init(void)
+{
+	unsigned int pmbits;
+
+	/* Only xen dom0 is allowed to handle ACPI processor info */
+	if (!xen_initial_domain())
+		return 0;
+
+	pmbits = (xen_start_info->flags & SIF_PM_MASK) >> 8;
+
+	if (pmbits & XEN_PROCESSOR_PM_CX)
+		xen_ops.pm_ops[PM_TYPE_IDLE] = xen_cx_notifier;
+	if (pmbits & XEN_PROCESSOR_PM_PX)
+		xen_ops.pm_ops[PM_TYPE_PERF] = xen_px_notifier;
+
+	return 0;
+}
+
+subsys_initcall(xen_acpi_processor_extcntl_init);
 MODULE_LICENSE("GPL");
-- 
1.7.7.3

From: Kevin Tian <kevin.tian@intel.com>

Xen acpi processor does not CPUFREQ_START, hence we we need to set
ignore_ppc to handle PPC events.

Signed-off-by: Yu Ke <ke.yu@intel.com>
Signed-off-by: Tian Kevin <kevin.tian@intel.com>
Signed-off-by: Tang Liang <liang.tang@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/acpi/processor_perflib.c |    2 +-
 drivers/acpi/processor_xen.c     |    2 ++
 include/acpi/processor.h         |    1 +
 3 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 22c6195..e622a0d 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -65,7 +65,7 @@ static DEFINE_MUTEX(performance_mutex);
  *  0 -> cpufreq low level drivers initialized -> consider _PPC values
  *  1 -> ignore _PPC totally -> forced by user through boot param
  */
-static int ignore_ppc = -1;
+int ignore_ppc = -1;
 module_param(ignore_ppc, int, 0644);
 MODULE_PARM_DESC(ignore_ppc, "If the frequency of your machine gets
wrongly" \
 		 "limited by BIOS, this should help");
diff --git a/drivers/acpi/processor_xen.c b/drivers/acpi/processor_xen.c
index fc3cc0b..a87b222 100644
--- a/drivers/acpi/processor_xen.c
+++ b/drivers/acpi/processor_xen.c
@@ -217,12 +217,14 @@ int xen_acpi_processor_init(void)
 	if (result < 0)
 		return result;
 		/* mark ready for handling ppc */
+	ignore_ppc = 0;
 
 	return 0;
 }
 
 void xen_acpi_processor_exit(void)
 {
+	ignore_ppc = -1;
 
 	acpi_bus_unregister_driver(&xen_acpi_processor_driver);
 }
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index cf53ed8..1380bb7 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -290,6 +290,7 @@ extern void xen_processor_driver_register(void);
 /* in processor_perflib.c */
 
 #ifdef CONFIG_CPU_FREQ
+extern int ignore_ppc;
 void acpi_processor_ppc_init(void);
 void acpi_processor_ppc_exit(void);
 int acpi_processor_ppc_has_changed(struct acpi_processor *pr, int event_flag);
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

>>> On 30.11.11 at 18:21, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o processor_throttling.o
>  processor-y			+= processor_idle.o processor_thermal.o
> +processor-y			+= processor_xen.o
This should minimally be processor-$(CONFIG_XEN), with other things
adjusted as necessary.
>  processor-$(CONFIG_CPU_FREQ)	+= processor_perflib.o
>  
>  obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
>...
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -117,6 +117,11 @@ config XEN_SYS_HYPERVISOR
>  	 virtual environment, /sys/hypervisor will still be present,
>  	 but will have no xen contents.
>  
> +config ACPI_PROCESSOR_XEN
> +	tristate
> +	depends on XEN_DOM0 && ACPI_PROCESSOR && CPU_FREQ
> +	default m
default ACPI_PROCESSOR
> +
>  config XEN_XENBUS_FRONTEND
>  	tristate
>  
> diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> index 974fffd..f67450c 100644
> --- a/drivers/xen/Makefile
> +++ b/drivers/xen/Makefile
> @@ -19,6 +19,9 @@ obj-$(CONFIG_XEN_TMEM)			+= tmem.o
>  obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
>  obj-$(CONFIG_XEN_DOM0)			+= pci.o
>  obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
> +ifdef CONFIG_ACPI_PROCESSOR_XEN
> +obj-$(CONFIG_ACPI_PROCESSOR)		+= acpi_processor.o
> +endif
obj-$(CONFIG_ACPI_PPROCESSOR_XEN)

Jan

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Dec 01, 2011 at 09:24:23AM +0000, Jan Beulich
wrote:
> >>> On 30.11.11 at 18:21, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -66,6 +66,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+= custom_method.o
> >  # processor has its own "processor." module_param namespace
> >  processor-y			:= processor_driver.o processor_throttling.o
> >  processor-y			+= processor_idle.o processor_thermal.o
> > +processor-y			+= processor_xen.o
> 
> This should minimally be processor-$(CONFIG_XEN), with other things
> adjusted as necessary.
I was under the impression that this was required to get the processor_xen.ko
to be a module. Otherwise it would only compile as built-in.

Liang, can you chime in please?
> 
> >  processor-$(CONFIG_CPU_FREQ)	+= processor_perflib.o
> >  
> >  obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o
> >...
> > --- a/drivers/xen/Kconfig
> > +++ b/drivers/xen/Kconfig
> > @@ -117,6 +117,11 @@ config XEN_SYS_HYPERVISOR
> >  	 virtual environment, /sys/hypervisor will still be present,
> >  	 but will have no xen contents.
> >  
> > +config ACPI_PROCESSOR_XEN
> > +	tristate
> > +	depends on XEN_DOM0 && ACPI_PROCESSOR && CPU_FREQ
> > +	default m
> 
> default ACPI_PROCESSOR
> 
> > +
> >  config XEN_XENBUS_FRONTEND
> >  	tristate
> >  
> > diff --git a/drivers/xen/Makefile b/drivers/xen/Makefile
> > index 974fffd..f67450c 100644
> > --- a/drivers/xen/Makefile
> > +++ b/drivers/xen/Makefile
> > @@ -19,6 +19,9 @@ obj-$(CONFIG_XEN_TMEM)			+= tmem.o
> >  obj-$(CONFIG_SWIOTLB_XEN)		+= swiotlb-xen.o
> >  obj-$(CONFIG_XEN_DOM0)			+= pci.o
> >  obj-$(CONFIG_XEN_PCIDEV_BACKEND)	+= xen-pciback/
> > +ifdef CONFIG_ACPI_PROCESSOR_XEN
> > +obj-$(CONFIG_ACPI_PROCESSOR)		+= acpi_processor.o
> > +endif
> 
> obj-$(CONFIG_ACPI_PPROCESSOR_XEN)
> 
> Jan
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

>>> On 12.12.11 at 18:29, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Thu, Dec 01, 2011 at 09:24:23AM +0000, Jan Beulich wrote:
>> >>> On 30.11.11 at 18:21, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> > --- a/drivers/acpi/Makefile
>> > +++ b/drivers/acpi/Makefile
>> > @@ -66,6 +66,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+=
custom_method.o
>> >  # processor has its own "processor." module_param
namespace
>> >  processor-y			:= processor_driver.o processor_throttling.o
>> >  processor-y			+= processor_idle.o processor_thermal.o
>> > +processor-y			+= processor_xen.o
>> 
>> This should minimally be processor-$(CONFIG_XEN), with other things
>> adjusted as necessary.
> 
> I was under the impression that this was required to get the 
> processor_xen.ko
> to be a module. Otherwise it would only compile as built-in.
processor_xen.ko? Why not have all the code go into processor.ko?
(And the original construct didn''t aim in that direction either.)

Jan

On 2011-12-13 15:45, Jan Beulich wrote:
>>>> On 12.12.11 at 18:29, Konrad Rzeszutek
Wilk<konrad.wilk@oracle.com>  wrote:
>> On Thu, Dec 01, 2011 at 09:24:23AM +0000, Jan Beulich wrote:
>>>>>> On 30.11.11 at 18:21, Konrad Rzeszutek
Wilk<konrad.wilk@oracle.com>  wrote:
>>>> --- a/drivers/acpi/Makefile
>>>> +++ b/drivers/acpi/Makefile
>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+=
custom_method.o
>>>>   # processor has its own "processor." module_param
namespace
>>>>   processor-y			:= processor_driver.o processor_throttling.o
>>>>   processor-y			+= processor_idle.o processor_thermal.o
>>>> +processor-y			+= processor_xen.o
>>> This should minimally be processor-$(CONFIG_XEN), with other things
>>> adjusted as necessary.
>> I was under the impression that this was required to get the
>> processor_xen.ko
>> to be a module. Otherwise it would only compile as built-in.
> processor_xen.ko? Why not have all the code go into processor.ko?
> (And the original construct didn''t aim in that direction either.)
>
> Jan
>
the code about driver function which kernel 
require(drivers/acpi/processor_xen.c )  build into processor.ko.
the code which have more relation with xen 
(drivers/xen/acpi_processor.c)  did not build into processor.ko.
liang

On Wed, Nov 30, 2011 at 12:20:57PM -0500, Konrad Rzeszutek Wilk
wrote:
> From: Kevin Tian <kevin.tian@intel.com>
> 
> This patch export some necessary functions which parse processor
> power management information. The Xen ACPI processor driver uses them.
I was wondering if it could be done by moving a bunch of these
functions in the processor_perflib.c, but there is a lot of code
that would have to be moved. Way too much.

Perhaps another, and a nicer way would be to:

 1). Create a processor_driver_lib.c which would have the "generic"
code
 2). In the processor_driver just keep the essential notifications, and
     the hotplug code
 3). The introduce the other user of the acpi_processor_[add|remove|notify]
     calls as a seperate library?

Thoughts?
> 
> Signed-off-by: Yu Ke <ke.yu@intel.com>
> Signed-off-by: Tian Kevin <kevin.tian@intel.com>
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/acpi/processor_driver.c  |   11 +++--------
>  drivers/acpi/processor_perflib.c |    4 ++--
>  include/acpi/processor.h         |   10 +++++++++-
>  3 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c
b/drivers/acpi/processor_driver.c
> index 9d7bc9f..211c078 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -79,9 +79,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
>  MODULE_DESCRIPTION("ACPI Processor Driver");
>  MODULE_LICENSE("GPL");
>  
> -static int acpi_processor_add(struct acpi_device *device);
> -static int acpi_processor_remove(struct acpi_device *device, int type);
> -static void acpi_processor_notify(struct acpi_device *device, u32 event);
>  static acpi_status acpi_processor_hotadd_init(acpi_handle handle, int
*p_cpu);
>  static int acpi_processor_handle_eject(struct acpi_processor *pr);
>  
> @@ -378,7 +375,7 @@ static int acpi_processor_get_info(struct acpi_device
*device)
>  
>  static DEFINE_PER_CPU(void *, processor_device_array);
>  
> -static void acpi_processor_notify(struct acpi_device *device, u32 event)
> +void acpi_processor_notify(struct acpi_device *device, u32 event)
>  {
>  	struct acpi_processor *pr = acpi_driver_data(device);
>  	int saved;
> @@ -442,7 +439,7 @@ static struct notifier_block acpi_cpu_notifier >  	 
.notifier_call = acpi_cpu_soft_notify,
>  };
>  
> -static int __cpuinit acpi_processor_add(struct acpi_device *device)
> +int __cpuinit acpi_processor_add(struct acpi_device *device)
>  {
>  	struct acpi_processor *pr = NULL;
>  	int result = 0;
> @@ -545,7 +542,7 @@ err_free_cpumask:
>  	return result;
>  }
>  
> -static int acpi_processor_remove(struct acpi_device *device, int type)
> +int acpi_processor_remove(struct acpi_device *device, int type)
>  {
>  	struct acpi_processor *pr = NULL;
>  
> @@ -758,7 +755,6 @@ static int acpi_processor_handle_eject(struct
acpi_processor *pr)
>  }
>  #endif
>  
> -static
>  void acpi_processor_install_hotplug_notify(void)
>  {
>  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> @@ -771,7 +767,6 @@ void acpi_processor_install_hotplug_notify(void)
>  	register_hotcpu_notifier(&acpi_cpu_notifier);
>  }
>  
> -static
>  void acpi_processor_uninstall_hotplug_notify(void)
>  {
>  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> diff --git a/drivers/acpi/processor_perflib.c
b/drivers/acpi/processor_perflib.c
> index 85b3237..22c6195 100644
> --- a/drivers/acpi/processor_perflib.c
> +++ b/drivers/acpi/processor_perflib.c
> @@ -386,7 +386,7 @@ static int acpi_processor_get_performance_states(struct
acpi_processor *pr)
>  	return result;
>  }
>  
> -static int acpi_processor_get_performance_info(struct acpi_processor *pr)
> +int acpi_processor_get_performance_info(struct acpi_processor *pr)
>  {
>  	int result = 0;
>  	acpi_status status = AE_OK;
> @@ -492,7 +492,7 @@ int acpi_processor_notify_smm(struct module
*calling_module)
>  
>  EXPORT_SYMBOL(acpi_processor_notify_smm);
>  
> -static int acpi_processor_get_psd(struct acpi_processor	*pr)
> +int acpi_processor_get_psd(struct acpi_processor	*pr)
>  {
>  	int result = 0;
>  	acpi_status status = AE_OK;
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 610f6fb..12657bb 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -239,6 +239,12 @@ extern void
acpi_processor_unregister_performance(struct
>           if a _PPC object exists, rmmod is disallowed then */
>  int acpi_processor_notify_smm(struct module *calling_module);
>  
> +void acpi_processor_install_hotplug_notify(void);
> +void acpi_processor_uninstall_hotplug_notify(void);
> +
> +int acpi_processor_add(struct acpi_device *device);
> +int acpi_processor_remove(struct acpi_device *device, int type);
> +void acpi_processor_notify(struct acpi_device *device, u32 event);
>  /* for communication between multiple parts of the processor kernel module
*/
>  DECLARE_PER_CPU(struct acpi_processor *, processors);
>  extern struct acpi_processor_errata errata;
> @@ -278,7 +284,9 @@ static inline void
acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
>  void acpi_processor_ppc_init(void);
>  void acpi_processor_ppc_exit(void);
>  int acpi_processor_ppc_has_changed(struct acpi_processor *pr, int
event_flag);
> -extern int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
> +int acpi_processor_get_performance_info(struct acpi_processor *pr);
> +int acpi_processor_get_psd(struct acpi_processor *pr);
> +int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
>  #else
>  static inline void acpi_processor_ppc_init(void)
>  {
> -- 
> 1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Nov 30, 2011 at 12:21:00PM -0500, Konrad Rzeszutek Wilk
wrote:
> From: Kevin Tian <kevin.tian@intel.com>
> 
> This patch inhibits processing of the CPU idle handler if it is not
> set to the appropiate one. This is needed by the Xen processor driver
> which, while still needing processor details, wants to use the default_idle
> call (which makes a yield hypercall).
Which I think is not required anymore with the
d91ee5863b71e8c90eaf6035bff3078a85e2e7b5
(cpuidle: replace xen access to x86 pm_idle and default_idle) and
62027aea23fcd14478abdddd3b74a4e0f5fb2984
(cpuidle: create bootparam "cpuidle.off=1")

Liang, can you double-check that please?
> 
> Signed-off-by: Yu Ke <ke.yu@intel.com>
> Signed-off-by: Tian Kevin <kevin.tian@intel.com>
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/acpi/processor_idle.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index d88974a..0ad347f 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1127,7 +1127,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
>  	cpuidle_pause_and_lock();
>  	cpuidle_disable_device(&pr->power.dev);
>  	acpi_processor_get_power_info(pr);
> -	if (pr->flags.power) {
> +	if (pr->flags.power  && (cpuidle_get_driver() ==
&acpi_idle_driver)) {
>  		acpi_processor_setup_cpuidle_cx(pr);
>  		ret = cpuidle_enable_device(&pr->power.dev);
>  	}
> @@ -1183,7 +1183,8 @@ int acpi_processor_cst_has_changed(struct
acpi_processor *pr)
>  			if (!_pr || !_pr->flags.power_setup_done)
>  				continue;
>  			acpi_processor_get_power_info(_pr);
> -			if (_pr->flags.power) {
> +			if (_pr->flags.power && (cpuidle_get_driver()
> +						== &acpi_idle_driver)) {
>  				acpi_processor_setup_cpuidle_cx(_pr);
>  				cpuidle_enable_device(&_pr->power.dev);
>  			}
> @@ -1237,7 +1238,8 @@ int __cpuinit acpi_processor_power_init(struct
acpi_processor *pr,
>  	 * Note that we use previously set idle handler will be used on
>  	 * platforms that only support C1.
>  	 */
> -	if (pr->flags.power) {
> +	if (pr->flags.power && (__acpi_processor_register_driver =>
+				acpi_processor_register_driver)) {
>  		/* Register acpi_idle_driver if not already registered */
>  		if (!acpi_processor_registered) {
>  			acpi_processor_setup_cpuidle_states(pr);
> -- 
> 1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad Rzeszutek Wilk
wrote:
> From: Tang Liang <liang.tang@oracle.com>
> 
> This patch implement __acpi_processor_[un]register_driver helper,
> so we can registry override processor driver function. Specifically
> the Xen processor driver.
Liang,

Is the reason we are doing this b/c we need to call acpi_bus_register_driver
and inhibit the registration of ''acpi_processor_driver'' ?

And the reason we don''t want ''acpi_processor_driver''
to run is b/c of what?
If the cpuidle is disabled what is the harm of running the
''acpi_processor_driver''
driver?
> 
> By default the values are set to the native one.
> 
> Signed-off-by: Tang Liang <liang.tang@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/acpi/processor_driver.c |   35 +++++++++++++++++++++++++++++------
>  include/acpi/processor.h        |    6 +++++-
>  2 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c
b/drivers/acpi/processor_driver.c
> index 211c078..55f0b89 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -90,6 +90,11 @@ static const struct acpi_device_id
processor_device_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, processor_device_ids);
>  
> +int (*__acpi_processor_register_driver)(void) =
acpi_processor_register_driver;
> +void (*__acpi_processor_unregister_driver)(void) \
> +	= acpi_processor_unregister_driver;
> +
> +
>  static struct acpi_driver acpi_processor_driver = {
>  	.name = "processor",
>  	.class = ACPI_PROCESSOR_CLASS,
> @@ -779,6 +784,22 @@ void acpi_processor_uninstall_hotplug_notify(void)
>  	unregister_hotcpu_notifier(&acpi_cpu_notifier);
>  }
>  
> +int acpi_processor_register_driver(void)
> +{
> +	int result = 0;
> +
> +	result = acpi_bus_register_driver(&acpi_processor_driver);
> +	return result;
> +}
> +
> +void acpi_processor_unregister_driver(void)
> +{
> +	acpi_bus_unregister_driver(&acpi_processor_driver);
> +
> +	cpuidle_unregister_driver(&acpi_idle_driver);
> +
> +	return;
> +}
>  /*
>   * We keep the driver loaded even when ACPI is not running.
>   * This is needed for the powernow-k8 driver, that works even without
> @@ -794,9 +815,11 @@ static int __init acpi_processor_init(void)
>  
>  	memset(&errata, 0, sizeof(errata));
>  
> -	result = acpi_bus_register_driver(&acpi_processor_driver);
> -	if (result < 0)
> -		return result;
> +	if (__acpi_processor_register_driver) {
> +		result = __acpi_processor_register_driver();
> +		if (result < 0)
> +			return result;
> +	}
>  
>  	acpi_processor_install_hotplug_notify();
>  
> @@ -809,6 +832,7 @@ static int __init acpi_processor_init(void)
>  	return 0;
>  }
>  
> +
>  static void __exit acpi_processor_exit(void)
>  {
>  	if (acpi_disabled)
> @@ -820,9 +844,8 @@ static void __exit acpi_processor_exit(void)
>  
>  	acpi_processor_uninstall_hotplug_notify();
>  
> -	acpi_bus_unregister_driver(&acpi_processor_driver);
> -
> -	cpuidle_unregister_driver(&acpi_idle_driver);
> +	if (__acpi_processor_unregister_driver)
> +		__acpi_processor_unregister_driver();
>  
>  	return;
>  }
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index bd99fb6..969cbc9 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -225,6 +225,9 @@ struct acpi_processor_errata {
>  	} piix4;
>  };
>  
> +extern int (*__acpi_processor_register_driver)(void);
> +extern void (*__acpi_processor_unregister_driver)(void);
> +
>  extern int acpi_processor_preregister_performance(struct
>  						  acpi_processor_performance
>  						  __percpu *performance);
> @@ -242,7 +245,8 @@ int acpi_processor_notify_smm(struct module
*calling_module);
>  
>  void acpi_processor_install_hotplug_notify(void);
>  void acpi_processor_uninstall_hotplug_notify(void);
> -
> +int acpi_processor_register_driver(void);
> +void acpi_processor_unregister_driver(void);
>  int acpi_processor_add(struct acpi_device *device);
>  int acpi_processor_remove(struct acpi_device *device, int type);
>  void acpi_processor_notify(struct acpi_device *device, u32 event);
> -- 
> 1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Dec 13, 2011 at 05:26:58PM +0800, liang tang
wrote:
> On 2011-12-13 15:45, Jan Beulich wrote:
> >>>>On 12.12.11 at 18:29, Konrad Rzeszutek
Wilk<konrad.wilk@oracle.com>  wrote:
> >>On Thu, Dec 01, 2011 at 09:24:23AM +0000, Jan Beulich wrote:
> >>>>>>On 30.11.11 at 18:21, Konrad Rzeszutek
Wilk<konrad.wilk@oracle.com>  wrote:
> >>>>--- a/drivers/acpi/Makefile
> >>>>+++ b/drivers/acpi/Makefile
> >>>>@@ -66,6 +66,7 @@ obj-$(CONFIG_ACPI_CUSTOM_METHOD)+=
custom_method.o
> >>>>  # processor has its own "processor."
module_param namespace
> >>>>  processor-y			:= processor_driver.o
processor_throttling.o
> >>>>  processor-y			+= processor_idle.o processor_thermal.o
> >>>>+processor-y			+= processor_xen.o
> >>>This should minimally be processor-$(CONFIG_XEN), with other
things
> >>>adjusted as necessary.
> >>I was under the impression that this was required to get the
> >>processor_xen.ko
> >>to be a module. Otherwise it would only compile as built-in.
> >processor_xen.ko? Why not have all the code go into processor.ko?
> >(And the original construct didn''t aim in that direction
either.)
> >
> >Jan
> >
> the code about driver function which kernel
> require(drivers/acpi/processor_xen.c )  build into processor.ko.
> the code which have more relation with xen
> (drivers/xen/acpi_processor.c)  did not build into processor.ko.
I am not sure if I understand you. Are you saying that the bulk of
''acpi_processor'' (in the drivers/xen directory) should not be
built in
processor.o and that the config options will do that?

But the config option is for the drivers/acpi directory..

So if I enable CONFIG_ACPI_PROCESSOR and CONFIG_ACPI_PROCESSOR_XEN
then the build will stick processor_xen.o in to the processor.ko file.

And if I select CONFIG_ACPI_PROCESSOR and unselect CONFIG_ACPI_PROCESSOR_XEN
then I still get some of the processor_xen.o stuck in processor.ko file?
(Granted, there is not much that gets stuck in b/c the majority of the code
is guarded by a big #if defined(CONFIG_ACPI_PROCESSOR_XEN..) so the compiler
might not stick anything in there)


Is there a way to make it so there are _two_ modules:
 - processor.ko
 - processor-xen.ko [which uses some code from processor.ko]

And they both can work together? Well, .. such that processor.ko
does not really do anything since there are no cpuidle enabled, and
the processor-xen.ko just deals with the parsing of ACPI Pxx states.

And since cpuidle is disabled, there won''t be any notify events sent
anyhow
so that could be removed (ah wait. the processor_xen.c does call
processor_cntl_xen_notify(..PM_INIT) to pipe the data up.. so that is
needed).

In which case the processor-xen.ko only does one thing - parses the 
''struct acpi_processor'' data and pipes it up the hypervisor.

Would this be feasible?
> liang
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Saturday, December 17, 2011 5:34 AM
> 
> On Wed, Nov 30, 2011 at 12:20:57PM -0500, Konrad Rzeszutek Wilk wrote:
> > From: Kevin Tian <kevin.tian@intel.com>
> >
> > This patch export some necessary functions which parse processor
> > power management information. The Xen ACPI processor driver uses them.
> 
> I was wondering if it could be done by moving a bunch of these
> functions in the processor_perflib.c, but there is a lot of code
> that would have to be moved. Way too much.
> 
> Perhaps another, and a nicer way would be to:
> 
>  1). Create a processor_driver_lib.c which would have the
"generic" code
>  2). In the processor_driver just keep the essential notifications, andk
>      the hotplug code
>  3). The introduce the other user of the acpi_processor_[add|remove|notify]
>      calls as a seperate library?
> 
> Thoughts?
that''s a cleaner approach in the long term view IMO, though it incurs
more changes
into current code base. 

Thanks
Kevin
> >
> > Signed-off-by: Yu Ke <ke.yu@intel.com>
> > Signed-off-by: Tian Kevin <kevin.tian@intel.com>
> > Signed-off-by: Tang Liang <liang.tang@oracle.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/acpi/processor_driver.c  |   11 +++--------
> >  drivers/acpi/processor_perflib.c |    4 ++--
> >  include/acpi/processor.h         |   10 +++++++++-
> >  3 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_driver.c
b/drivers/acpi/processor_driver.c
> > index 9d7bc9f..211c078 100644
> > --- a/drivers/acpi/processor_driver.c
> > +++ b/drivers/acpi/processor_driver.c
> > @@ -79,9 +79,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> >  MODULE_DESCRIPTION("ACPI Processor Driver");
> >  MODULE_LICENSE("GPL");
> >
> > -static int acpi_processor_add(struct acpi_device *device);
> > -static int acpi_processor_remove(struct acpi_device *device, int
type);
> > -static void acpi_processor_notify(struct acpi_device *device, u32
event);
> >  static acpi_status acpi_processor_hotadd_init(acpi_handle handle, int
> *p_cpu);
> >  static int acpi_processor_handle_eject(struct acpi_processor *pr);
> >
> > @@ -378,7 +375,7 @@ static int acpi_processor_get_info(struct
acpi_device
> *device)
> >
> >  static DEFINE_PER_CPU(void *, processor_device_array);
> >
> > -static void acpi_processor_notify(struct acpi_device *device, u32
event)
> > +void acpi_processor_notify(struct acpi_device *device, u32 event)
> >  {
> >  	struct acpi_processor *pr = acpi_driver_data(device);
> >  	int saved;
> > @@ -442,7 +439,7 @@ static struct notifier_block acpi_cpu_notifier
> >  	    .notifier_call = acpi_cpu_soft_notify,
> >  };
> >
> > -static int __cpuinit acpi_processor_add(struct acpi_device *device)
> > +int __cpuinit acpi_processor_add(struct acpi_device *device)
> >  {
> >  	struct acpi_processor *pr = NULL;
> >  	int result = 0;
> > @@ -545,7 +542,7 @@ err_free_cpumask:
> >  	return result;
> >  }
> >
> > -static int acpi_processor_remove(struct acpi_device *device, int
type)
> > +int acpi_processor_remove(struct acpi_device *device, int type)
> >  {
> >  	struct acpi_processor *pr = NULL;
> >
> > @@ -758,7 +755,6 @@ static int acpi_processor_handle_eject(struct
> acpi_processor *pr)
> >  }
> >  #endif
> >
> > -static
> >  void acpi_processor_install_hotplug_notify(void)
> >  {
> >  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > @@ -771,7 +767,6 @@ void acpi_processor_install_hotplug_notify(void)
> >  	register_hotcpu_notifier(&acpi_cpu_notifier);
> >  }
> >
> > -static
> >  void acpi_processor_uninstall_hotplug_notify(void)
> >  {
> >  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > diff --git a/drivers/acpi/processor_perflib.c
b/drivers/acpi/processor_perflib.c
> > index 85b3237..22c6195 100644
> > --- a/drivers/acpi/processor_perflib.c
> > +++ b/drivers/acpi/processor_perflib.c
> > @@ -386,7 +386,7 @@ static int
> acpi_processor_get_performance_states(struct acpi_processor *pr)
> >  	return result;
> >  }
> >
> > -static int acpi_processor_get_performance_info(struct acpi_processor
*pr)
> > +int acpi_processor_get_performance_info(struct acpi_processor *pr)
> >  {
> >  	int result = 0;
> >  	acpi_status status = AE_OK;
> > @@ -492,7 +492,7 @@ int acpi_processor_notify_smm(struct module
> *calling_module)
> >
> >  EXPORT_SYMBOL(acpi_processor_notify_smm);
> >
> > -static int acpi_processor_get_psd(struct acpi_processor	*pr)
> > +int acpi_processor_get_psd(struct acpi_processor	*pr)
> >  {
> >  	int result = 0;
> >  	acpi_status status = AE_OK;
> > diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > index 610f6fb..12657bb 100644
> > --- a/include/acpi/processor.h
> > +++ b/include/acpi/processor.h
> > @@ -239,6 +239,12 @@ extern void
> acpi_processor_unregister_performance(struct
> >           if a _PPC object exists, rmmod is disallowed then */
> >  int acpi_processor_notify_smm(struct module *calling_module);
> >
> > +void acpi_processor_install_hotplug_notify(void);
> > +void acpi_processor_uninstall_hotplug_notify(void);
> > +
> > +int acpi_processor_add(struct acpi_device *device);
> > +int acpi_processor_remove(struct acpi_device *device, int type);
> > +void acpi_processor_notify(struct acpi_device *device, u32 event);
> >  /* for communication between multiple parts of the processor kernel
> module */
> >  DECLARE_PER_CPU(struct acpi_processor *, processors);
> >  extern struct acpi_processor_errata errata;
> > @@ -278,7 +284,9 @@ static inline void
> acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
> >  void acpi_processor_ppc_init(void);
> >  void acpi_processor_ppc_exit(void);
> >  int acpi_processor_ppc_has_changed(struct acpi_processor *pr, int
> event_flag);
> > -extern int acpi_processor_get_bios_limit(int cpu, unsigned int
*limit);
> > +int acpi_processor_get_performance_info(struct acpi_processor *pr);
> > +int acpi_processor_get_psd(struct acpi_processor *pr);
> > +int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
> >  #else
> >  static inline void acpi_processor_ppc_init(void)
> >  {
> > --
> > 1.7.7.3

> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Saturday, December 17, 2011 6:04 AM
> 
> On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad Rzeszutek Wilk wrote:
> > From: Tang Liang <liang.tang@oracle.com>
> >
> > This patch implement __acpi_processor_[un]register_driver helper,
> > so we can registry override processor driver function. Specifically
> > the Xen processor driver.
> 
> Liang,
> 
> Is the reason we are doing this b/c we need to call
acpi_bus_register_driver
> and inhibit the registration of ''acpi_processor_driver'' ?
> 
> And the reason we don''t want
''acpi_processor_driver'' to run is b/c of what?
> If the cpuidle is disabled what is the harm of running the
> ''acpi_processor_driver''
> driver?
IIRC, the reason why we want a Xen specific driver is because current driver
is integrated with OS logic too tightly, e.g. the various checks on VCPU related
structures. Long time ago the 1st version in Xen was to use same driver, by
adding various tweaking lines inside which makes it completely messed. Then
later it''s found that it''s clearer to create a Xen specific
wrapping driver, by
invoking some exported functions from existing one.

Thanks
Kevin
> 
> >
> > By default the values are set to the native one.
> >
> > Signed-off-by: Tang Liang <liang.tang@oracle.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  drivers/acpi/processor_driver.c |   35
> +++++++++++++++++++++++++++++------
> >  include/acpi/processor.h        |    6 +++++-
> >  2 files changed, 34 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_driver.c
b/drivers/acpi/processor_driver.c
> > index 211c078..55f0b89 100644
> > --- a/drivers/acpi/processor_driver.c
> > +++ b/drivers/acpi/processor_driver.c
> > @@ -90,6 +90,11 @@ static const struct acpi_device_id
> processor_device_ids[] = {
> >  };
> >  MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> >
> > +int (*__acpi_processor_register_driver)(void) >
acpi_processor_register_driver;
> > +void (*__acpi_processor_unregister_driver)(void) \
> > +	= acpi_processor_unregister_driver;
> > +
> > +
> >  static struct acpi_driver acpi_processor_driver = {
> >  	.name = "processor",
> >  	.class = ACPI_PROCESSOR_CLASS,
> > @@ -779,6 +784,22 @@ void
acpi_processor_uninstall_hotplug_notify(void)
> >  	unregister_hotcpu_notifier(&acpi_cpu_notifier);
> >  }
> >
> > +int acpi_processor_register_driver(void)
> > +{
> > +	int result = 0;
> > +
> > +	result = acpi_bus_register_driver(&acpi_processor_driver);
> > +	return result;
> > +}
> > +
> > +void acpi_processor_unregister_driver(void)
> > +{
> > +	acpi_bus_unregister_driver(&acpi_processor_driver);
> > +
> > +	cpuidle_unregister_driver(&acpi_idle_driver);
> > +
> > +	return;
> > +}
> >  /*
> >   * We keep the driver loaded even when ACPI is not running.
> >   * This is needed for the powernow-k8 driver, that works even without
> > @@ -794,9 +815,11 @@ static int __init acpi_processor_init(void)
> >
> >  	memset(&errata, 0, sizeof(errata));
> >
> > -	result = acpi_bus_register_driver(&acpi_processor_driver);
> > -	if (result < 0)
> > -		return result;
> > +	if (__acpi_processor_register_driver) {
> > +		result = __acpi_processor_register_driver();
> > +		if (result < 0)
> > +			return result;
> > +	}
> >
> >  	acpi_processor_install_hotplug_notify();
> >
> > @@ -809,6 +832,7 @@ static int __init acpi_processor_init(void)
> >  	return 0;
> >  }
> >
> > +
> >  static void __exit acpi_processor_exit(void)
> >  {
> >  	if (acpi_disabled)
> > @@ -820,9 +844,8 @@ static void __exit acpi_processor_exit(void)
> >
> >  	acpi_processor_uninstall_hotplug_notify();
> >
> > -	acpi_bus_unregister_driver(&acpi_processor_driver);
> > -
> > -	cpuidle_unregister_driver(&acpi_idle_driver);
> > +	if (__acpi_processor_unregister_driver)
> > +		__acpi_processor_unregister_driver();
> >
> >  	return;
> >  }
> > diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > index bd99fb6..969cbc9 100644
> > --- a/include/acpi/processor.h
> > +++ b/include/acpi/processor.h
> > @@ -225,6 +225,9 @@ struct acpi_processor_errata {
> >  	} piix4;
> >  };
> >
> > +extern int (*__acpi_processor_register_driver)(void);
> > +extern void (*__acpi_processor_unregister_driver)(void);
> > +
> >  extern int acpi_processor_preregister_performance(struct
> >  						  acpi_processor_performance
> >  						  __percpu *performance);
> > @@ -242,7 +245,8 @@ int acpi_processor_notify_smm(struct module
> *calling_module);
> >
> >  void acpi_processor_install_hotplug_notify(void);
> >  void acpi_processor_uninstall_hotplug_notify(void);
> > -
> > +int acpi_processor_register_driver(void);
> > +void acpi_processor_unregister_driver(void);
> >  int acpi_processor_add(struct acpi_device *device);
> >  int acpi_processor_remove(struct acpi_device *device, int type);
> >  void acpi_processor_notify(struct acpi_device *device, u32 event);
> > --
> > 1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2011-12-17 5:36, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 30, 2011 at 12:21:00PM -0500, Konrad Rzeszutek Wilk wrote:
>> From: Kevin Tian<kevin.tian@intel.com>
>>
>> This patch inhibits processing of the CPU idle handler if it is not
>> set to the appropiate one. This is needed by the Xen processor driver
>> which, while still needing processor details, wants to use the
default_idle
>> call (which makes a yield hypercall).
> Which I think is not required anymore with the
d91ee5863b71e8c90eaf6035bff3078a85e2e7b5
> (cpuidle: replace xen access to x86 pm_idle and default_idle) and
> 62027aea23fcd14478abdddd3b74a4e0f5fb2984
> (cpuidle: create bootparam "cpuidle.off=1")
>
> Liang, can you double-check that please?
>
Hi,Konrad
I just read the code ,I think  don''t need that code too. I will double 
check tomorrow.
thanks
liang
>> Signed-off-by: Yu Ke<ke.yu@intel.com>
>> Signed-off-by: Tian Kevin<kevin.tian@intel.com>
>> Signed-off-by: Tang Liang<liang.tang@oracle.com>
>> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
>> ---
>>   drivers/acpi/processor_idle.c |    8 +++++---
>>   1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c
b/drivers/acpi/processor_idle.c
>> index d88974a..0ad347f 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1127,7 +1127,7 @@ int acpi_processor_hotplug(struct acpi_processor
*pr)
>>   	cpuidle_pause_and_lock();
>>   	cpuidle_disable_device(&pr->power.dev);
>>   	acpi_processor_get_power_info(pr);
>> -	if (pr->flags.power) {
>> +	if (pr->flags.power&&  (cpuidle_get_driver()
==&acpi_idle_driver)) {
>>   		acpi_processor_setup_cpuidle_cx(pr);
>>   		ret = cpuidle_enable_device(&pr->power.dev);
>>   	}
>> @@ -1183,7 +1183,8 @@ int acpi_processor_cst_has_changed(struct
acpi_processor *pr)
>>   			if (!_pr || !_pr->flags.power_setup_done)
>>   				continue;
>>   			acpi_processor_get_power_info(_pr);
>> -			if (_pr->flags.power) {
>> +			if (_pr->flags.power&&  (cpuidle_get_driver()
>> +						==&acpi_idle_driver)) {
>>   				acpi_processor_setup_cpuidle_cx(_pr);
>>   				cpuidle_enable_device(&_pr->power.dev);
>>   			}
>> @@ -1237,7 +1238,8 @@ int __cpuinit acpi_processor_power_init(struct
acpi_processor *pr,
>>   	 * Note that we use previously set idle handler will be used on
>>   	 * platforms that only support C1.
>>   	 */
>> -	if (pr->flags.power) {
>> +	if (pr->flags.power&&  (__acpi_processor_register_driver
=>> +				acpi_processor_register_driver)) {
>>   		/* Register acpi_idle_driver if not already registered */
>>   		if (!acpi_processor_registered) {
>>   			acpi_processor_setup_cpuidle_states(pr);
>> -- 
>> 1.7.7.3
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Dec 19, 2011 at 01:43:01PM +0800, Tian, Kevin
wrote:
> > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > Sent: Saturday, December 17, 2011 5:34 AM
> > 
> > On Wed, Nov 30, 2011 at 12:20:57PM -0500, Konrad Rzeszutek Wilk wrote:
> > > From: Kevin Tian <kevin.tian@intel.com>
> > >
> > > This patch export some necessary functions which parse processor
> > > power management information. The Xen ACPI processor driver uses
them.
> > 
> > I was wondering if it could be done by moving a bunch of these
> > functions in the processor_perflib.c, but there is a lot of code
> > that would have to be moved. Way too much.
> > 
> > Perhaps another, and a nicer way would be to:
> > 
> >  1). Create a processor_driver_lib.c which would have the
"generic" code
> >  2). In the processor_driver just keep the essential notifications,
andk
> >      the hotplug code
> >  3). The introduce the other user of the
acpi_processor_[add|remove|notify]
> >      calls as a seperate library?
> > 
> > Thoughts?
> 
> that''s a cleaner approach in the long term view IMO, though it
incurs more changes
> into current code base. 
Well, upstream kernels are focused on the long term view. So that sounds
like agreement.
> 
> Thanks
> Kevin
> 
> > >
> > > Signed-off-by: Yu Ke <ke.yu@intel.com>
> > > Signed-off-by: Tian Kevin <kevin.tian@intel.com>
> > > Signed-off-by: Tang Liang <liang.tang@oracle.com>
> > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > > ---
> > >  drivers/acpi/processor_driver.c  |   11 +++--------
> > >  drivers/acpi/processor_perflib.c |    4 ++--
> > >  include/acpi/processor.h         |   10 +++++++++-
> > >  3 files changed, 14 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/acpi/processor_driver.c
b/drivers/acpi/processor_driver.c
> > > index 9d7bc9f..211c078 100644
> > > --- a/drivers/acpi/processor_driver.c
> > > +++ b/drivers/acpi/processor_driver.c
> > > @@ -79,9 +79,6 @@ MODULE_AUTHOR("Paul Diefenbaugh");
> > >  MODULE_DESCRIPTION("ACPI Processor Driver");
> > >  MODULE_LICENSE("GPL");
> > >
> > > -static int acpi_processor_add(struct acpi_device *device);
> > > -static int acpi_processor_remove(struct acpi_device *device, int
type);
> > > -static void acpi_processor_notify(struct acpi_device *device,
u32 event);
> > >  static acpi_status acpi_processor_hotadd_init(acpi_handle
handle, int
> > *p_cpu);
> > >  static int acpi_processor_handle_eject(struct acpi_processor
*pr);
> > >
> > > @@ -378,7 +375,7 @@ static int acpi_processor_get_info(struct
acpi_device
> > *device)
> > >
> > >  static DEFINE_PER_CPU(void *, processor_device_array);
> > >
> > > -static void acpi_processor_notify(struct acpi_device *device,
u32 event)
> > > +void acpi_processor_notify(struct acpi_device *device, u32
event)
> > >  {
> > >  	struct acpi_processor *pr = acpi_driver_data(device);
> > >  	int saved;
> > > @@ -442,7 +439,7 @@ static struct notifier_block
acpi_cpu_notifier > > >  	    .notifier_call = acpi_cpu_soft_notify,
> > >  };
> > >
> > > -static int __cpuinit acpi_processor_add(struct acpi_device
*device)
> > > +int __cpuinit acpi_processor_add(struct acpi_device *device)
> > >  {
> > >  	struct acpi_processor *pr = NULL;
> > >  	int result = 0;
> > > @@ -545,7 +542,7 @@ err_free_cpumask:
> > >  	return result;
> > >  }
> > >
> > > -static int acpi_processor_remove(struct acpi_device *device, int
type)
> > > +int acpi_processor_remove(struct acpi_device *device, int type)
> > >  {
> > >  	struct acpi_processor *pr = NULL;
> > >
> > > @@ -758,7 +755,6 @@ static int acpi_processor_handle_eject(struct
> > acpi_processor *pr)
> > >  }
> > >  #endif
> > >
> > > -static
> > >  void acpi_processor_install_hotplug_notify(void)
> > >  {
> > >  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > > @@ -771,7 +767,6 @@ void
acpi_processor_install_hotplug_notify(void)
> > >  	register_hotcpu_notifier(&acpi_cpu_notifier);
> > >  }
> > >
> > > -static
> > >  void acpi_processor_uninstall_hotplug_notify(void)
> > >  {
> > >  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > > diff --git a/drivers/acpi/processor_perflib.c
b/drivers/acpi/processor_perflib.c
> > > index 85b3237..22c6195 100644
> > > --- a/drivers/acpi/processor_perflib.c
> > > +++ b/drivers/acpi/processor_perflib.c
> > > @@ -386,7 +386,7 @@ static int
> > acpi_processor_get_performance_states(struct acpi_processor *pr)
> > >  	return result;
> > >  }
> > >
> > > -static int acpi_processor_get_performance_info(struct
acpi_processor *pr)
> > > +int acpi_processor_get_performance_info(struct acpi_processor
*pr)
> > >  {
> > >  	int result = 0;
> > >  	acpi_status status = AE_OK;
> > > @@ -492,7 +492,7 @@ int acpi_processor_notify_smm(struct module
> > *calling_module)
> > >
> > >  EXPORT_SYMBOL(acpi_processor_notify_smm);
> > >
> > > -static int acpi_processor_get_psd(struct acpi_processor	*pr)
> > > +int acpi_processor_get_psd(struct acpi_processor	*pr)
> > >  {
> > >  	int result = 0;
> > >  	acpi_status status = AE_OK;
> > > diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > > index 610f6fb..12657bb 100644
> > > --- a/include/acpi/processor.h
> > > +++ b/include/acpi/processor.h
> > > @@ -239,6 +239,12 @@ extern void
> > acpi_processor_unregister_performance(struct
> > >           if a _PPC object exists, rmmod is disallowed then */
> > >  int acpi_processor_notify_smm(struct module *calling_module);
> > >
> > > +void acpi_processor_install_hotplug_notify(void);
> > > +void acpi_processor_uninstall_hotplug_notify(void);
> > > +
> > > +int acpi_processor_add(struct acpi_device *device);
> > > +int acpi_processor_remove(struct acpi_device *device, int type);
> > > +void acpi_processor_notify(struct acpi_device *device, u32
event);
> > >  /* for communication between multiple parts of the processor
kernel
> > module */
> > >  DECLARE_PER_CPU(struct acpi_processor *, processors);
> > >  extern struct acpi_processor_errata errata;
> > > @@ -278,7 +284,9 @@ static inline void
> > acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
> > >  void acpi_processor_ppc_init(void);
> > >  void acpi_processor_ppc_exit(void);
> > >  int acpi_processor_ppc_has_changed(struct acpi_processor *pr,
int
> > event_flag);
> > > -extern int acpi_processor_get_bios_limit(int cpu, unsigned int
*limit);
> > > +int acpi_processor_get_performance_info(struct acpi_processor
*pr);
> > > +int acpi_processor_get_psd(struct acpi_processor *pr);
> > > +int acpi_processor_get_bios_limit(int cpu, unsigned int *limit);
> > >  #else
> > >  static inline void acpi_processor_ppc_init(void)
> > >  {
> > > --
> > > 1.7.7.3
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

On Mon, Dec 19, 2011 at 01:48:01PM +0800, Tian, Kevin
wrote:
> > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > Sent: Saturday, December 17, 2011 6:04 AM
> > 
> > On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad Rzeszutek Wilk wrote:
> > > From: Tang Liang <liang.tang@oracle.com>
> > >
> > > This patch implement __acpi_processor_[un]register_driver helper,
> > > so we can registry override processor driver function.
Specifically
> > > the Xen processor driver.
> > 
> > Liang,
> > 
> > Is the reason we are doing this b/c we need to call
acpi_bus_register_driver
> > and inhibit the registration of
''acpi_processor_driver'' ?
> > 
> > And the reason we don''t want
''acpi_processor_driver'' to run is b/c of what?
> > If the cpuidle is disabled what is the harm of running the
> > ''acpi_processor_driver''
> > driver?
> 
> IIRC, the reason why we want a Xen specific driver is because current
driver
> is integrated with OS logic too tightly, e.g. the various checks on VCPU
related
> structures. Long time ago the 1st version in Xen was to use same driver, by
> adding various tweaking lines inside which makes it completely messed. Then
> later it''s found that it''s clearer to create a Xen
specific wrapping driver, by
> invoking some exported functions from existing one.
What I am asking is does it matter "if the current driver is integrated
with OS logic to tighly" - as it is not running anyhow (b/c cpuidle is
disabled).

And if Xen specific driver can run along-side the generic one - since
the generic one is not doing any work (b/c cpuidle is disabled).

That is what I am trying to figure out - since this patch purpose is to
thwart the generic driver from running and allowing the xen one to
run.
> 
> Thanks
> Kevin
> 
> > 
> > >
> > > By default the values are set to the native one.
> > >
> > > Signed-off-by: Tang Liang <liang.tang@oracle.com>
> > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > > ---
> > >  drivers/acpi/processor_driver.c |   35
> > +++++++++++++++++++++++++++++------
> > >  include/acpi/processor.h        |    6 +++++-
> > >  2 files changed, 34 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/acpi/processor_driver.c
b/drivers/acpi/processor_driver.c
> > > index 211c078..55f0b89 100644
> > > --- a/drivers/acpi/processor_driver.c
> > > +++ b/drivers/acpi/processor_driver.c
> > > @@ -90,6 +90,11 @@ static const struct acpi_device_id
> > processor_device_ids[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> > >
> > > +int (*__acpi_processor_register_driver)(void) > >
acpi_processor_register_driver;
> > > +void (*__acpi_processor_unregister_driver)(void) \
> > > +	= acpi_processor_unregister_driver;
> > > +
> > > +
> > >  static struct acpi_driver acpi_processor_driver = {
> > >  	.name = "processor",
> > >  	.class = ACPI_PROCESSOR_CLASS,
> > > @@ -779,6 +784,22 @@ void
acpi_processor_uninstall_hotplug_notify(void)
> > >  	unregister_hotcpu_notifier(&acpi_cpu_notifier);
> > >  }
> > >
> > > +int acpi_processor_register_driver(void)
> > > +{
> > > +	int result = 0;
> > > +
> > > +	result = acpi_bus_register_driver(&acpi_processor_driver);
> > > +	return result;
> > > +}
> > > +
> > > +void acpi_processor_unregister_driver(void)
> > > +{
> > > +	acpi_bus_unregister_driver(&acpi_processor_driver);
> > > +
> > > +	cpuidle_unregister_driver(&acpi_idle_driver);
> > > +
> > > +	return;
> > > +}
> > >  /*
> > >   * We keep the driver loaded even when ACPI is not running.
> > >   * This is needed for the powernow-k8 driver, that works even
without
> > > @@ -794,9 +815,11 @@ static int __init acpi_processor_init(void)
> > >
> > >  	memset(&errata, 0, sizeof(errata));
> > >
> > > -	result = acpi_bus_register_driver(&acpi_processor_driver);
> > > -	if (result < 0)
> > > -		return result;
> > > +	if (__acpi_processor_register_driver) {
> > > +		result = __acpi_processor_register_driver();
> > > +		if (result < 0)
> > > +			return result;
> > > +	}
> > >
> > >  	acpi_processor_install_hotplug_notify();
> > >
> > > @@ -809,6 +832,7 @@ static int __init acpi_processor_init(void)
> > >  	return 0;
> > >  }
> > >
> > > +
> > >  static void __exit acpi_processor_exit(void)
> > >  {
> > >  	if (acpi_disabled)
> > > @@ -820,9 +844,8 @@ static void __exit acpi_processor_exit(void)
> > >
> > >  	acpi_processor_uninstall_hotplug_notify();
> > >
> > > -	acpi_bus_unregister_driver(&acpi_processor_driver);
> > > -
> > > -	cpuidle_unregister_driver(&acpi_idle_driver);
> > > +	if (__acpi_processor_unregister_driver)
> > > +		__acpi_processor_unregister_driver();
> > >
> > >  	return;
> > >  }
> > > diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> > > index bd99fb6..969cbc9 100644
> > > --- a/include/acpi/processor.h
> > > +++ b/include/acpi/processor.h
> > > @@ -225,6 +225,9 @@ struct acpi_processor_errata {
> > >  	} piix4;
> > >  };
> > >
> > > +extern int (*__acpi_processor_register_driver)(void);
> > > +extern void (*__acpi_processor_unregister_driver)(void);
> > > +
> > >  extern int acpi_processor_preregister_performance(struct
> > >  						  acpi_processor_performance
> > >  						  __percpu *performance);
> > > @@ -242,7 +245,8 @@ int acpi_processor_notify_smm(struct module
> > *calling_module);
> > >
> > >  void acpi_processor_install_hotplug_notify(void);
> > >  void acpi_processor_uninstall_hotplug_notify(void);
> > > -
> > > +int acpi_processor_register_driver(void);
> > > +void acpi_processor_unregister_driver(void);
> > >  int acpi_processor_add(struct acpi_device *device);
> > >  int acpi_processor_remove(struct acpi_device *device, int type);
> > >  void acpi_processor_notify(struct acpi_device *device, u32
event);
> > > --
> > > 1.7.7.3
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Dec 19, 2011 at 06:33:01PM +0800, liang tang
wrote:
> On 2011-12-17 5:36, Konrad Rzeszutek Wilk wrote:
> >On Wed, Nov 30, 2011 at 12:21:00PM -0500, Konrad Rzeszutek Wilk wrote:
> >>From: Kevin Tian<kevin.tian@intel.com>
> >>
> >>This patch inhibits processing of the CPU idle handler if it is not
> >>set to the appropiate one. This is needed by the Xen processor
driver
> >>which, while still needing processor details, wants to use the 
> >>default_idle
> >>call (which makes a yield hypercall).
> >Which I think is not required anymore with the 
> >d91ee5863b71e8c90eaf6035bff3078a85e2e7b5
> >(cpuidle: replace xen access to x86 pm_idle and default_idle) and
> >62027aea23fcd14478abdddd3b74a4e0f5fb2984
> >(cpuidle: create bootparam "cpuidle.off=1")
> >
> >Liang, can you double-check that please?
> >
> Hi,Konrad
> I just read the code ,I think  don''t need that code too. I will
double
> check tomorrow.
Great! Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> Sent: Monday, December 19, 2011 10:26 PM
> 
> On Mon, Dec 19, 2011 at 01:48:01PM +0800, Tian, Kevin wrote:
> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > Sent: Saturday, December 17, 2011 6:04 AM
> > >
> > > On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad Rzeszutek Wilk
wrote:
> > > > From: Tang Liang <liang.tang@oracle.com>
> > > >
> > > > This patch implement __acpi_processor_[un]register_driver
helper,
> > > > so we can registry override processor driver function.
Specifically
> > > > the Xen processor driver.
> > >
> > > Liang,
> > >
> > > Is the reason we are doing this b/c we need to call
acpi_bus_register_driver
> > > and inhibit the registration of
''acpi_processor_driver'' ?
> > >
> > > And the reason we don''t want
''acpi_processor_driver'' to run is b/c of what?
> > > If the cpuidle is disabled what is the harm of running the
> > > ''acpi_processor_driver''
> > > driver?
> >
> > IIRC, the reason why we want a Xen specific driver is because current
driver
> > is integrated with OS logic too tightly, e.g. the various checks on
VCPU related
> > structures. Long time ago the 1st version in Xen was to use same
driver, by
> > adding various tweaking lines inside which makes it completely messed.
Then
> > later it''s found that it''s clearer to create a Xen
specific wrapping driver, by
> > invoking some exported functions from existing one.
> 
> What I am asking is does it matter "if the current driver is
integrated
> with OS logic to tighly" - as it is not running anyhow (b/c cpuidle is
> disabled).
> 
> And if Xen specific driver can run along-side the generic one - since
> the generic one is not doing any work (b/c cpuidle is disabled).
> 
> That is what I am trying to figure out - since this patch purpose is to
> thwart the generic driver from running and allowing the xen one to run.
It''s a separate issue from cpuidle case. Here we''re talking
about acpi processor
driver, not the acpi cpuidle driver. ACPI processor driver is responsible for 
discovering ACPI processor projects, and then enumerate various methods 
such as _PPC, _CST, etc. under those objects. So far this driver depends on 
VCPU presence in various places, which causes trouble when dom0 is configured
with less VCPU number than physical present one.

One example you can see from acpi_processor_add:

        result = acpi_processor_get_info(device); // call acpi_get_cpuid
        if (result) {
                /* Processor is physically not present */
                return 0;
        }

#ifdef CONFIG_SMP
        if (pr->id >= setup_max_cpus && pr->id != 0)
                return 0;
#endif 

        BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));

The binding between ACPI processor objects and VCPU presence would only parse
partial objects to Xen. And there''re various places within driver
validating pr->id
making it messy to workaround for Xen within same driver.

That''s the major reason for coming up a Xen specific driver, which
always parses
all present objects regardless of VCPU presence. :-)

Thanks
Kevin
> >
> > Thanks
> > Kevin
> >
> > >
> > > >
> > > > By default the values are set to the native one.
> > > >
> > > > Signed-off-by: Tang Liang <liang.tang@oracle.com>
> > > > Signed-off-by: Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com>
> > > > ---
> > > >  drivers/acpi/processor_driver.c |   35
> > > +++++++++++++++++++++++++++++------
> > > >  include/acpi/processor.h        |    6 +++++-
> > > >  2 files changed, 34 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/processor_driver.c
> b/drivers/acpi/processor_driver.c
> > > > index 211c078..55f0b89 100644
> > > > --- a/drivers/acpi/processor_driver.c
> > > > +++ b/drivers/acpi/processor_driver.c
> > > > @@ -90,6 +90,11 @@ static const struct acpi_device_id
> > > processor_device_ids[] = {
> > > >  };
> > > >  MODULE_DEVICE_TABLE(acpi, processor_device_ids);
> > > >
> > > > +int (*__acpi_processor_register_driver)(void) > >
> acpi_processor_register_driver;
> > > > +void (*__acpi_processor_unregister_driver)(void) \
> > > > +	= acpi_processor_unregister_driver;
> > > > +
> > > > +
> > > >  static struct acpi_driver acpi_processor_driver = {
> > > >  	.name = "processor",
> > > >  	.class = ACPI_PROCESSOR_CLASS,
> > > > @@ -779,6 +784,22 @@ void
> acpi_processor_uninstall_hotplug_notify(void)
> > > >  	unregister_hotcpu_notifier(&acpi_cpu_notifier);
> > > >  }
> > > >
> > > > +int acpi_processor_register_driver(void)
> > > > +{
> > > > +	int result = 0;
> > > > +
> > > > +	result =
acpi_bus_register_driver(&acpi_processor_driver);
> > > > +	return result;
> > > > +}
> > > > +
> > > > +void acpi_processor_unregister_driver(void)
> > > > +{
> > > > +	acpi_bus_unregister_driver(&acpi_processor_driver);
> > > > +
> > > > +	cpuidle_unregister_driver(&acpi_idle_driver);
> > > > +
> > > > +	return;
> > > > +}
> > > >  /*
> > > >   * We keep the driver loaded even when ACPI is not running.
> > > >   * This is needed for the powernow-k8 driver, that works
even without
> > > > @@ -794,9 +815,11 @@ static int __init
acpi_processor_init(void)
> > > >
> > > >  	memset(&errata, 0, sizeof(errata));
> > > >
> > > > -	result =
acpi_bus_register_driver(&acpi_processor_driver);
> > > > -	if (result < 0)
> > > > -		return result;
> > > > +	if (__acpi_processor_register_driver) {
> > > > +		result = __acpi_processor_register_driver();
> > > > +		if (result < 0)
> > > > +			return result;
> > > > +	}
> > > >
> > > >  	acpi_processor_install_hotplug_notify();
> > > >
> > > > @@ -809,6 +832,7 @@ static int __init
acpi_processor_init(void)
> > > >  	return 0;
> > > >  }
> > > >
> > > > +
> > > >  static void __exit acpi_processor_exit(void)
> > > >  {
> > > >  	if (acpi_disabled)
> > > > @@ -820,9 +844,8 @@ static void __exit
acpi_processor_exit(void)
> > > >
> > > >  	acpi_processor_uninstall_hotplug_notify();
> > > >
> > > > -	acpi_bus_unregister_driver(&acpi_processor_driver);
> > > > -
> > > > -	cpuidle_unregister_driver(&acpi_idle_driver);
> > > > +	if (__acpi_processor_unregister_driver)
> > > > +		__acpi_processor_unregister_driver();
> > > >
> > > >  	return;
> > > >  }
> > > > diff --git a/include/acpi/processor.h
b/include/acpi/processor.h
> > > > index bd99fb6..969cbc9 100644
> > > > --- a/include/acpi/processor.h
> > > > +++ b/include/acpi/processor.h
> > > > @@ -225,6 +225,9 @@ struct acpi_processor_errata {
> > > >  	} piix4;
> > > >  };
> > > >
> > > > +extern int (*__acpi_processor_register_driver)(void);
> > > > +extern void (*__acpi_processor_unregister_driver)(void);
> > > > +
> > > >  extern int acpi_processor_preregister_performance(struct
> > > >  						  acpi_processor_performance
> > > >  						  __percpu *performance);
> > > > @@ -242,7 +245,8 @@ int acpi_processor_notify_smm(struct
module
> > > *calling_module);
> > > >
> > > >  void acpi_processor_install_hotplug_notify(void);
> > > >  void acpi_processor_uninstall_hotplug_notify(void);
> > > > -
> > > > +int acpi_processor_register_driver(void);
> > > > +void acpi_processor_unregister_driver(void);
> > > >  int acpi_processor_add(struct acpi_device *device);
> > > >  int acpi_processor_remove(struct acpi_device *device, int
type);
> > > >  void acpi_processor_notify(struct acpi_device *device, u32
event);
> > > > --
> > > > 1.7.7.3
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel

On 2011-12-19 22:26, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 19, 2011 at 06:33:01PM +0800, liang tang wrote:
>> On 2011-12-17 5:36, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Nov 30, 2011 at 12:21:00PM -0500, Konrad Rzeszutek Wilk
wrote:
>>>> From: Kevin Tian<kevin.tian@intel.com>
>>>>
>>>> This patch inhibits processing of the CPU idle handler if it is
not
>>>> set to the appropiate one. This is needed by the Xen processor
driver
>>>> which, while still needing processor details, wants to use the
>>>> default_idle
>>>> call (which makes a yield hypercall).
>>> Which I think is not required anymore with the
>>> d91ee5863b71e8c90eaf6035bff3078a85e2e7b5
>>> (cpuidle: replace xen access to x86 pm_idle and default_idle) and
>>> 62027aea23fcd14478abdddd3b74a4e0f5fb2984
>>> (cpuidle: create bootparam "cpuidle.off=1")
>>>
>>> Liang, can you double-check that please?
>>>
>> Hi,Konrad
>> I just read the code ,I think  don''t need that code too. I
will double
>> check tomorrow.
> Great! Thanks!
I have check the code, I think that will be not call. I think we can 
remove this patch .
liang
thanks !
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Dec 20, 2011 at 10:29:12AM +0800, Tian, Kevin
wrote:
> > From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> > Sent: Monday, December 19, 2011 10:26 PM
> > 
> > On Mon, Dec 19, 2011 at 01:48:01PM +0800, Tian, Kevin wrote:
> > > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> > > > Sent: Saturday, December 17, 2011 6:04 AM
> > > >
> > > > On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad Rzeszutek
Wilk wrote:
> > > > > From: Tang Liang <liang.tang@oracle.com>
> > > > >
> > > > > This patch implement
__acpi_processor_[un]register_driver helper,
> > > > > so we can registry override processor driver function.
Specifically
> > > > > the Xen processor driver.
> > > >
> > > > Liang,
> > > >
> > > > Is the reason we are doing this b/c we need to call
acpi_bus_register_driver
> > > > and inhibit the registration of
''acpi_processor_driver'' ?
> > > >
> > > > And the reason we don''t want
''acpi_processor_driver'' to run is b/c of what?
> > > > If the cpuidle is disabled what is the harm of running the
> > > > ''acpi_processor_driver''
> > > > driver?
> > >
> > > IIRC, the reason why we want a Xen specific driver is because
current driver
> > > is integrated with OS logic too tightly, e.g. the various checks
on VCPU related
> > > structures. Long time ago the 1st version in Xen was to use same
driver, by
> > > adding various tweaking lines inside which makes it completely
messed. Then
> > > later it''s found that it''s clearer to create a
Xen specific wrapping driver, by
> > > invoking some exported functions from existing one.
> > 
> > What I am asking is does it matter "if the current driver is
integrated
> > with OS logic to tighly" - as it is not running anyhow (b/c
cpuidle is
> > disabled).
> > 
> > And if Xen specific driver can run along-side the generic one - since
> > the generic one is not doing any work (b/c cpuidle is disabled).
> > 
> > That is what I am trying to figure out - since this patch purpose is
to
> > thwart the generic driver from running and allowing the xen one to
run.
> 
> It''s a separate issue from cpuidle case. Here we''re
talking about acpi processor
> driver, not the acpi cpuidle driver. ACPI processor driver is responsible
for
> discovering ACPI processor projects, and then enumerate various methods 
> such as _PPC, _CST, etc. under those objects. So far this driver depends on
> VCPU presence in various places, which causes trouble when dom0 is
configured
> with less VCPU number than physical present one.
> 
> One example you can see from acpi_processor_add:
> 
>         result = acpi_processor_get_info(device); // call acpi_get_cpuid
>         if (result) {
>                 /* Processor is physically not present */
>                 return 0;
>         }
> 
> #ifdef CONFIG_SMP
>         if (pr->id >= setup_max_cpus && pr->id != 0)
>                 return 0;
> #endif 
> 
>         BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
> 
> The binding between ACPI processor objects and VCPU presence would only
parse
> partial objects to Xen. And there''re various places within driver
validating pr->id
> making it messy to workaround for Xen within same driver.
> 
> That''s the major reason for coming up a Xen specific driver, which
always parses
> all present objects regardless of VCPU presence. :-)
OK. Lets put the # VCPU != PCPU aside. Say dom0 will boot with all
CPUs and then later on the admin starts unplugging them.

With that in mind, could we use a slim driver (like the one in: "ACPI:
 add processor driver for Xen virtual CPUs.") that would just
take the _Pxx, Cxx data and shove them to the hypervisor (handwaving
aside the checking for quirks and such). And lets assume that we use the
unmodified acpi_processor_[add|remove|notify] code that is present in
processor_driver.c.

Oh, and the processor-driver.c did try to load (with the understanding
that it would load, but won''t do much since cpuidle is turned off).

Would that still get the Pxx, Cxx data to the hypervisor?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> Sent: Tuesday, December 20, 2011 11:32 PM
> 
> On Tue, Dec 20, 2011 at 10:29:12AM +0800, Tian, Kevin wrote:
> > > From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> > > Sent: Monday, December 19, 2011 10:26 PM
> > >
> > > On Mon, Dec 19, 2011 at 01:48:01PM +0800, Tian, Kevin wrote:
> > > > > From: Konrad Rzeszutek Wilk
[mailto:konrad.wilk@oracle.com]
> > > > > Sent: Saturday, December 17, 2011 6:04 AM
> > > > >
> > > > > On Wed, Nov 30, 2011 at 12:20:59PM -0500, Konrad
Rzeszutek Wilk
> wrote:
> > > > > > From: Tang Liang <liang.tang@oracle.com>
> > > > > >
> > > > > > This patch implement
__acpi_processor_[un]register_driver helper,
> > > > > > so we can registry override processor driver
function. Specifically
> > > > > > the Xen processor driver.
> > > > >
> > > > > Liang,
> > > > >
> > > > > Is the reason we are doing this b/c we need to call
> acpi_bus_register_driver
> > > > > and inhibit the registration of
''acpi_processor_driver'' ?
> > > > >
> > > > > And the reason we don''t want
''acpi_processor_driver'' to run is b/c of
> what?
> > > > > If the cpuidle is disabled what is the harm of running
the
> > > > > ''acpi_processor_driver''
> > > > > driver?
> > > >
> > > > IIRC, the reason why we want a Xen specific driver is
because current
> driver
> > > > is integrated with OS logic too tightly, e.g. the various
checks on VCPU
> related
> > > > structures. Long time ago the 1st version in Xen was to use
same driver,
> by
> > > > adding various tweaking lines inside which makes it
completely messed.
> Then
> > > > later it''s found that it''s clearer to
create a Xen specific wrapping driver, by
> > > > invoking some exported functions from existing one.
> > >
> > > What I am asking is does it matter "if the current driver is
integrated
> > > with OS logic to tighly" - as it is not running anyhow (b/c
cpuidle is
> > > disabled).
> > >
> > > And if Xen specific driver can run along-side the generic one -
since
> > > the generic one is not doing any work (b/c cpuidle is disabled).
> > >
> > > That is what I am trying to figure out - since this patch purpose
is to
> > > thwart the generic driver from running and allowing the xen one
to run.
> >
> > It''s a separate issue from cpuidle case. Here we''re
talking about acpi
> processor
> > driver, not the acpi cpuidle driver. ACPI processor driver is
responsible for
> > discovering ACPI processor projects, and then enumerate various
methods
> > such as _PPC, _CST, etc. under those objects. So far this driver
depends on
> > VCPU presence in various places, which causes trouble when dom0 is
> configured
> > with less VCPU number than physical present one.
> >
> > One example you can see from acpi_processor_add:
> >
> >         result = acpi_processor_get_info(device); // call
acpi_get_cpuid
> >         if (result) {
> >                 /* Processor is physically not present */
> >                 return 0;
> >         }
> >
> > #ifdef CONFIG_SMP
> >         if (pr->id >= setup_max_cpus && pr->id != 0)
> >                 return 0;
> > #endif
> >
> >         BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
> >
> > The binding between ACPI processor objects and VCPU presence would
only
> parse
> > partial objects to Xen. And there''re various places within
driver validating
> pr->id
> > making it messy to workaround for Xen within same driver.
> >
> > That''s the major reason for coming up a Xen specific driver,
which always
> parses
> > all present objects regardless of VCPU presence. :-)
> 
> OK. Lets put the # VCPU != PCPU aside. Say dom0 will boot with all
> CPUs and then later on the admin starts unplugging them.
This should be communicated to major Xen based distributions, so that
it''s
an agreed approach since in majority case dom0 is configured as UP or
a few VCPUs.
> 
> With that in mind, could we use a slim driver (like the one in: "ACPI:
>  add processor driver for Xen virtual CPUs.") that would just
> take the _Pxx, Cxx data and shove them to the hypervisor (handwaving
> aside the checking for quirks and such). And lets assume that we use the
> unmodified acpi_processor_[add|remove|notify] code that is present in
> processor_driver.c.
There may still have other condition checks which may fail, but yes
it''s possible
to reuse the interfaces. From my memory #VCPU is the major blocker...
> 
> Oh, and the processor-driver.c did try to load (with the understanding
> that it would load, but won''t do much since cpuidle is turned
off).
you also need consider cpufreq. IIRC, Px related information may be only
parsed from loaded acpi cpufreq driver, which is another reason why a Xen
wrapper driver is created. But of course this could be relaxed from existing
code if you want to go that way.
> 
> Would that still get the Pxx, Cxx data to the hypervisor?
yes, it''s possible under the assumption you put in the start. But you
need
verify more code paths to make sure some conditional checks for native
Linux still hold true for dom0.

Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > OK. Lets put the # VCPU != PCPU aside. Say dom0 will boot with all
> > CPUs and then later on the admin starts unplugging them.
> 
> This should be communicated to major Xen based distributions, so that
it''s
> an agreed approach since in majority case dom0 is configured as UP or
> a few VCPUs.
I am not saying that is it the agreed approach. There has to be
flexibility in supporting both. But what I want to understand whether
the requirement for VCPU != PCPU can be put aside and put in the drivers
later on.

So that the first approach is not changing the generic drivers (much).
The reason I am asking about this is two-fold:
 1). For new distros (Ubuntu, Fedora), the default is all VCPUs.
     Enterprising users might use dom0_max_vcpus to limit the VCPU count,
     but most won''t.
     Which mean we can concentrate on bringing the _Pxx/_Cxx parsing
     up to the hypervisor. Which is really neccessary on any chipset
     which has the notion of TurboBoost (otherwise the Xen scheduler
     won''t pick this up and won''t engage this mode in certain
     workloads).
 2). The ACPI maintainers are busy with ACPI 5.0. I don''t know how
     much work this is, but it probably means tons of stuff with
     embedded platforms and tons of regression testing. So if there
     is a patch that does not impact the generic code much (or any)
     it will make their life easier. Which also means we can built
     on top that for the VCPU != PCPU case.

That is what I am trying to understand.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> Sent: Friday, December 23, 2011 11:01 AM
> 
> > > OK. Lets put the # VCPU != PCPU aside. Say dom0 will boot with
all
> > > CPUs and then later on the admin starts unplugging them.
> >
> > This should be communicated to major Xen based distributions, so that
it''s
> > an agreed approach since in majority case dom0 is configured as UP or
> > a few VCPUs.
> 
> I am not saying that is it the agreed approach. There has to be
> flexibility in supporting both. But what I want to understand whether
> the requirement for VCPU != PCPU can be put aside and put in the drivers
> later on.
sure. VCPU!=PCPU requirement is orthogonal to the basic part for gearing
ACPI information to Xen.
> 
> So that the first approach is not changing the generic drivers (much).
> The reason I am asking about this is two-fold:
>  1). For new distros (Ubuntu, Fedora), the default is all VCPUs.
good to know that.
>      Enterprising users might use dom0_max_vcpus to limit the VCPU count,
>      but most won''t.
>      Which mean we can concentrate on bringing the _Pxx/_Cxx parsing
>      up to the hypervisor. Which is really neccessary on any chipset
>      which has the notion of TurboBoost (otherwise the Xen scheduler
>      won''t pick this up and won''t engage this mode in
certain
>      workloads).
>  2). The ACPI maintainers are busy with ACPI 5.0. I don''t know how
>      much work this is, but it probably means tons of stuff with
>      embedded platforms and tons of regression testing. So if there
>      is a patch that does not impact the generic code much (or any)
>      it will make their life easier. Which also means we can built
>      on top that for the VCPU != PCPU case.
> 
> That is what I am trying to understand.
no problem. this incremental approach should work.

Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Dec 26, 2011 at 01:31:45AM +0000, Tian, Kevin
wrote:
> > From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> > Sent: Friday, December 23, 2011 11:01 AM
> > 
> > > > OK. Lets put the # VCPU != PCPU aside. Say dom0 will boot
with all
> > > > CPUs and then later on the admin starts unplugging them.
> > >
> > > This should be communicated to major Xen based distributions, so
that it''s
> > > an agreed approach since in majority case dom0 is configured as
UP or
> > > a few VCPUs.
> > 
> > I am not saying that is it the agreed approach. There has to be
> > flexibility in supporting both. But what I want to understand whether
> > the requirement for VCPU != PCPU can be put aside and put in the
drivers
> > later on.
> 
> sure. VCPU!=PCPU requirement is orthogonal to the basic part for gearing
> ACPI information to Xen.
> 
> > 
> > So that the first approach is not changing the generic drivers (much).
> > The reason I am asking about this is two-fold:
> >  1). For new distros (Ubuntu, Fedora), the default is all VCPUs.
> 
> good to know that.
> 
> >      Enterprising users might use dom0_max_vcpus to limit the VCPU
count,
> >      but most won''t.
> >      Which mean we can concentrate on bringing the _Pxx/_Cxx parsing
> >      up to the hypervisor. Which is really neccessary on any chipset
> >      which has the notion of TurboBoost (otherwise the Xen scheduler
> >      won''t pick this up and won''t engage this mode
in certain
> >      workloads).
> >  2). The ACPI maintainers are busy with ACPI 5.0. I don''t
know how
> >      much work this is, but it probably means tons of stuff with
> >      embedded platforms and tons of regression testing. So if there
> >      is a patch that does not impact the generic code much (or any)
> >      it will make their life easier. Which also means we can built
> >      on top that for the VCPU != PCPU case.
> > 
> > That is what I am trying to understand.
> 
> no problem. this incremental approach should work.
Excellent. So now the big question  - is this something you would have the
time to do?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Wednesday, January 04, 2012 4:59 AM
> 
> On Mon, Dec 26, 2011 at 01:31:45AM +0000, Tian, Kevin wrote:
> > > From: Konrad Rzeszutek Wilk [mailto:konrad@darnok.org]
> > > Sent: Friday, December 23, 2011 11:01 AM
> > >
> > > > > OK. Lets put the # VCPU != PCPU aside. Say dom0 will
boot with all
> > > > > CPUs and then later on the admin starts unplugging
them.
> > > >
> > > > This should be communicated to major Xen based
distributions, so that
> it''s
> > > > an agreed approach since in majority case dom0 is configured
as UP or
> > > > a few VCPUs.
> > >
> > > I am not saying that is it the agreed approach. There has to be
> > > flexibility in supporting both. But what I want to understand
whether
> > > the requirement for VCPU != PCPU can be put aside and put in the
drivers
> > > later on.
> >
> > sure. VCPU!=PCPU requirement is orthogonal to the basic part for
gearing
> > ACPI information to Xen.
> >
> > >
> > > So that the first approach is not changing the generic drivers
(much).
> > > The reason I am asking about this is two-fold:
> > >  1). For new distros (Ubuntu, Fedora), the default is all VCPUs.
> >
> > good to know that.
> >
> > >      Enterprising users might use dom0_max_vcpus to limit the
VCPU
> count,
> > >      but most won''t.
> > >      Which mean we can concentrate on bringing the _Pxx/_Cxx
parsing
> > >      up to the hypervisor. Which is really neccessary on any
chipset
> > >      which has the notion of TurboBoost (otherwise the Xen
scheduler
> > >      won''t pick this up and won''t engage this
mode in certain
> > >      workloads).
> > >  2). The ACPI maintainers are busy with ACPI 5.0. I
don''t know how
> > >      much work this is, but it probably means tons of stuff with
> > >      embedded platforms and tons of regression testing. So if
there
> > >      is a patch that does not impact the generic code much (or
any)
> > >      it will make their life easier. Which also means we can
built
> > >      on top that for the VCPU != PCPU case.
> > >
> > > That is what I am trying to understand.
> >
> > no problem. this incremental approach should work.
> 
> Excellent. So now the big question  - is this something you would have the
> time to do?
yes, this is a big question. First, I''d like to give my sincere thanks
to you and
Liang for help push this part to Linux upstream. You''ve done a great
job. :-)
Unfortunately I can''t afford the time in the short term, due to
extremely busy
tasks in other projects, at least in the whole Q1. Really sorry about that. :/

I would very appreciate your help if you could continue lending some time here,
since you''ve done plenty of works on the cleanup. The majority of the
tricky
part is related to VCPU/PCPU handling. If putting that part aside, the remaining
logic should be much simpler.

Thanks
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> > > sure. VCPU!=PCPU requirement is orthogonal to the basic part for
gearing
> > > ACPI information to Xen.
.. snip..
> > >
> > > >  1). For new distros (Ubuntu, Fedora), the default is all
VCPUs.
> > >
> > > good to know that.
> > >
> > > >      Enterprising users might use dom0_max_vcpus to limit
the VCPU
> > count,
> > > >      but most won''t.
> > > >      Which mean we can concentrate on bringing the _Pxx/_Cxx
parsing
> > > >      up to the hypervisor. Which is really neccessary on any
chipset
> > > >      which has the notion of TurboBoost (otherwise the Xen
scheduler
> > > >      won''t pick this up and won''t engage
this mode in certain
> > > >      workloads).
.. snip..
> yes, this is a big question. First, I''d like to give my sincere
thanks to you and
> Liang for help push this part to Linux upstream. You''ve done a
great job. :-)
Thanks!
> Unfortunately I can''t afford the time in the short term, due to
extremely busy
> tasks in other projects, at least in the whole Q1. Really sorry about that.
:/
Bummer.
> 
> I would very appreciate your help if you could continue lending some time
here,
> since you''ve done plenty of works on the cleanup. The majority of
the tricky
> part is related to VCPU/PCPU handling. If putting that part aside, the
remaining
> logic should be much simpler.
I was trying to figure out how difficult it would be to just bring Pxx states to
the Xen hypervisor using the existing ACPI interfaces. And while it did not pass
all the _Pxx states (seems that all the _PCT, _PSS, _PSD, _PPC flags need to
be enabled in the hypercall to make this work), it demonstrates what I had in
mind.


#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
#include <acpi/acpi_bus.h>
#include <acpi/acpi_drivers.h>
#include <acpi/processor.h>
#include <linux/cpumask.h>

#include <xen/interface/platform.h>
#include <asm/xen/hypercall.h>

#define DRV_NAME	"ACPI_PXX"
#define DRV_CLASS	"ACPI_PXX_CLASS"
MODULE_AUTHOR("Konrad Rzeszutek Wilk");
MODULE_DESCRIPTION("ACPI Processor Driver to send data to Xen
hypervisor");
MODULE_LICENSE("GPL");

static int parse_acpi_cxx(struct acpi_processor *_pr)
{
	struct acpi_processor_cx *cx;
	int i;

	for (i = 1; i <= _pr->power.count; i++) {
		cx = &_pr->power.states[i];
		if (!cx->valid)
			continue;
		pr_info("%s: %d %d %d 0x%x\n", __func__,
			cx->type, cx->latency, cx->power, (u32)cx->address);
	}
	/* TODO: Under Xen, the C-states information is not present.
 	 * Figure out why. */
	return 0;
}
static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
{
	int ret = -EINVAL;
	struct xen_platform_op op = {
		.cmd			= XENPF_set_processor_pminfo,
		.interface_version	= XENPF_INTERFACE_VERSION,
		.u.set_pminfo.id	= _pr->acpi_id,
		.u.set_pminfo.type	= XEN_PM_PX,
	};
	struct xen_processor_performance *xen_perf;
	struct xen_processor_px *xen_states, *xen_px = NULL;
	struct acpi_processor_px *px;
	unsigned i;

	xen_perf = &op.u.set_pminfo.perf;
	xen_perf->flags = XEN_PX_PSS;

	xen_perf->state_count = _pr->performance->state_count;
	xen_states = kzalloc(xen_perf->state_count *
			     sizeof(struct xen_processor_px), GFP_KERNEL);
	if (!xen_states)
		return -ENOMEM;

	for (i = 0; i < _pr->performance->state_count; i++) {
		xen_px = &(xen_states[i]);
		px = &(_pr->performance->states[i]);

		xen_px->core_frequency = px->core_frequency;
		xen_px->power = px->power;
		xen_px->transition_latency = px->transition_latency;
		xen_px->bus_master_latency = px->bus_master_latency;
		xen_px->control = px->control;
		xen_px->status = px->status;
	}
	set_xen_guest_handle(xen_perf->states, xen_states);
	ret = HYPERVISOR_dom0_op(&op);
	kfree(xen_states);
	return ret;
}
static int parse_acpi_pxx(struct acpi_processor *_pr)
{
	struct acpi_processor_px *px;
	int i;

	for (i = 0; i < _pr->performance->state_count;i++) {
		px = &(_pr->performance->states[i]);
		pr_info("%s: [%d]: %d, %d, %d, %d, %d, %d\n", __func__,
			i, (u32)px->core_frequency, (u32)px->power,
			(u32)px->transition_latency,
			(u32)px->bus_master_latency,
			(u32)px->control, (u32)px->status);
	}
	if (xen_initial_domain())
		return push_pxx_to_hypervisor(_pr);
	return 0;
}
static int parse_acpi_data(void)
{
	int cpu;
	int err = -ENODEV;
	struct acpi_processor *_pr;
	struct cpuinfo_x86 *c = &cpu_data(0);

	/* TODO: Under AMD, the information is populated
	 * using the powernow-k8 driver which does an MSR_PSTATE_CUR_LIMIT
	 * MSR which returns the wrong value so the population of
''processors''
	 * has bogus data. So only run this under Intel for right now. */
	if (!cpu_has(c, X86_FEATURE_EST))
		return -ENODEV;
	for_each_possible_cpu(cpu) {
		_pr = per_cpu(processors, cpu);
		if (!_pr)
			continue;

		if (_pr->flags.power)
			(void)parse_acpi_cxx(_pr);

		if (_pr->performance->states)
			err = parse_acpi_pxx(_pr);
		if (err)
			break;
	}
	return -ENODEV; /* force it to unload */
}
static int __init acpi_pxx_init(void)
{
	return parse_acpi_data();
}
static void __exit acpi_pxx_exit(void)
{
}
module_init(acpi_pxx_init);
module_exit(acpi_pxx_exit);
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Saturday, January 14, 2012 6:25 AM
> 
> > > > sure. VCPU!=PCPU requirement is orthogonal to the basic part
for gearing
> > > > ACPI information to Xen.
> 
> .. snip..
> > > >
> > > > >  1). For new distros (Ubuntu, Fedora), the default is
all VCPUs.
> > > >
> > > > good to know that.
> > > >
> > > > >      Enterprising users might use dom0_max_vcpus to
limit the VCPU
> > > count,
> > > > >      but most won''t.
> > > > >      Which mean we can concentrate on bringing the
_Pxx/_Cxx
> parsing
> > > > >      up to the hypervisor. Which is really neccessary
on any chipset
> > > > >      which has the notion of TurboBoost (otherwise the
Xen scheduler
> > > > >      won''t pick this up and won''t
engage this mode in certain
> > > > >      workloads).
> 
> .. snip..
> 
> > yes, this is a big question. First, I''d like to give my
sincere thanks to you and
> > Liang for help push this part to Linux upstream. You''ve done
a great job. :-)
> 
> Thanks!
> > Unfortunately I can''t afford the time in the short term, due
to extremely busy
> > tasks in other projects, at least in the whole Q1. Really sorry about
that. :/
> 
> Bummer.
> >
> > I would very appreciate your help if you could continue lending some
time
> here,
> > since you''ve done plenty of works on the cleanup. The
majority of the tricky
> > part is related to VCPU/PCPU handling. If putting that part aside, the
> remaining
> > logic should be much simpler.
> 
> I was trying to figure out how difficult it would be to just bring Pxx
states to
> the Xen hypervisor using the existing ACPI interfaces. And while it did not
pass
> all the _Pxx states (seems that all the _PCT, _PSS, _PSD, _PPC flags need
to
> be enabled in the hypercall to make this work), it demonstrates what I had
in
> mind.
> 
> 
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/types.h>
> #include <acpi/acpi_bus.h>
> #include <acpi/acpi_drivers.h>
> #include <acpi/processor.h>
> #include <linux/cpumask.h>
> 
> #include <xen/interface/platform.h>
> #include <asm/xen/hypercall.h>
> 
> #define DRV_NAME	"ACPI_PXX"
> #define DRV_CLASS	"ACPI_PXX_CLASS"
> MODULE_AUTHOR("Konrad Rzeszutek Wilk");
> MODULE_DESCRIPTION("ACPI Processor Driver to send data to Xen
> hypervisor");
> MODULE_LICENSE("GPL");
> 
> static int parse_acpi_cxx(struct acpi_processor *_pr)
> {
> 	struct acpi_processor_cx *cx;
> 	int i;
> 
> 	for (i = 1; i <= _pr->power.count; i++) {
> 		cx = &_pr->power.states[i];
> 		if (!cx->valid)
> 			continue;
> 		pr_info("%s: %d %d %d 0x%x\n", __func__,
> 			cx->type, cx->latency, cx->power, (u32)cx->address);
> 	}
> 	/* TODO: Under Xen, the C-states information is not present.
>  	 * Figure out why. */
it''s possible related to this long thread:

http://lists.xen.org/archives/html/xen-devel/2011-08/msg00511.html

IOW, Xen doesn''t export mwait capability to dom0, which impacts _PDC
setting.
Final solution is to have a para-virtualized PDC call for that.
> 	return 0;
> }
> static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
> {
> 	int ret = -EINVAL;
> 	struct xen_platform_op op = {
> 		.cmd			= XENPF_set_processor_pminfo,
> 		.interface_version	= XENPF_INTERFACE_VERSION,
> 		.u.set_pminfo.id	= _pr->acpi_id,
> 		.u.set_pminfo.type	= XEN_PM_PX,
> 	};
> 	struct xen_processor_performance *xen_perf;
> 	struct xen_processor_px *xen_states, *xen_px = NULL;
> 	struct acpi_processor_px *px;
> 	unsigned i;
> 
> 	xen_perf = &op.u.set_pminfo.perf;
> 	xen_perf->flags = XEN_PX_PSS;
> 
> 	xen_perf->state_count = _pr->performance->state_count;
> 	xen_states = kzalloc(xen_perf->state_count *
> 			     sizeof(struct xen_processor_px), GFP_KERNEL);
> 	if (!xen_states)
> 		return -ENOMEM;
> 
> 	for (i = 0; i < _pr->performance->state_count; i++) {
> 		xen_px = &(xen_states[i]);
> 		px = &(_pr->performance->states[i]);
> 
> 		xen_px->core_frequency = px->core_frequency;
> 		xen_px->power = px->power;
> 		xen_px->transition_latency = px->transition_latency;
> 		xen_px->bus_master_latency = px->bus_master_latency;
> 		xen_px->control = px->control;
> 		xen_px->status = px->status;
> 	}
> 	set_xen_guest_handle(xen_perf->states, xen_states);
> 	ret = HYPERVISOR_dom0_op(&op);
> 	kfree(xen_states);
> 	return ret;
> }
> static int parse_acpi_pxx(struct acpi_processor *_pr)
> {
> 	struct acpi_processor_px *px;
> 	int i;
> 
> 	for (i = 0; i < _pr->performance->state_count;i++) {
> 		px = &(_pr->performance->states[i]);
> 		pr_info("%s: [%d]: %d, %d, %d, %d, %d, %d\n", __func__,
> 			i, (u32)px->core_frequency, (u32)px->power,
> 			(u32)px->transition_latency,
> 			(u32)px->bus_master_latency,
> 			(u32)px->control, (u32)px->status);
> 	}
> 	if (xen_initial_domain())
> 		return push_pxx_to_hypervisor(_pr);
> 	return 0;
> }
> static int parse_acpi_data(void)
> {
> 	int cpu;
> 	int err = -ENODEV;
> 	struct acpi_processor *_pr;
> 	struct cpuinfo_x86 *c = &cpu_data(0);
> 
> 	/* TODO: Under AMD, the information is populated
> 	 * using the powernow-k8 driver which does an MSR_PSTATE_CUR_LIMIT
> 	 * MSR which returns the wrong value so the population of
''processors''
> 	 * has bogus data. So only run this under Intel for right now. */
> 	if (!cpu_has(c, X86_FEATURE_EST))
> 		return -ENODEV;
> 	for_each_possible_cpu(cpu) {
> 		_pr = per_cpu(processors, cpu);
> 		if (!_pr)
> 			continue;
> 
> 		if (_pr->flags.power)
> 			(void)parse_acpi_cxx(_pr);
> 
> 		if (_pr->performance->states)
> 			err = parse_acpi_pxx(_pr);
> 		if (err)
> 			break;
> 	}
> 	return -ENODEV; /* force it to unload */
> }
> static int __init acpi_pxx_init(void)
> {
> 	return parse_acpi_data();
> }
> static void __exit acpi_pxx_exit(void)
> {
> }
> module_init(acpi_pxx_init);
> module_exit(acpi_pxx_exit);
the prerequisites for this module to work correctly, is that dom0 has the right
configurations to have all necessary Cx/Px information ready before this 
module is loaded. That may mean enabling full CONFIG_CPU_IDLE and
CONFIG_CPUFREQ,
which in current form may add some negative impact, e.g. dom0 will try to
control
Px/Cx to conflict with Xen. So some tweaks may be required in that part.

given our purpose now, is to come up a cleaner approach which tolerate some
assumptions (e.g. #VCPU of dom0 == #PCPU), there''s another option
following this
trend (perhaps compensate your idea). We can register a Xen-cpuidle and 
xen-cpufreq driver to current Linux cpuidle and cpufreq framework, which plays 
mainly two roles:
	- a dummy driver to prevent dom0 touching actual Px/Cx
	- parse ACPI Cx/Px information to Xen, in a similar way you did above

there may have some other trickiness, but the majority code will be
self-contained.

Thanks
Kevin

> > I was trying to figure out how difficult it would be to just bring Pxx
states to
> > the Xen hypervisor using the existing ACPI interfaces. And while it
did not pass
> > all the _Pxx states (seems that all the _PCT, _PSS, _PSD, _PPC flags
need to
> > be enabled in the hypercall to make this work), it demonstrates what I
had in
> > mind. 
.. snip..
> > 	/* TODO: Under Xen, the C-states information is not present.
> >  	 * Figure out why. */
> 
> it''s possible related to this long thread:
> 
> http://lists.xen.org/archives/html/xen-devel/2011-08/msg00511.html
> 
> IOW, Xen doesn''t export mwait capability to dom0, which impacts
_PDC setting.
> Final solution is to have a para-virtualized PDC call for that.
Aaah. Let me play with that a bit. Thanks for the pointer.

.. snip..
> the prerequisites for this module to work correctly, is that dom0 has the
right
> configurations to have all necessary Cx/Px information ready before this 
> module is loaded. That may mean enabling full CONFIG_CPU_IDLE and
CONFIG_CPUFREQ,
Right.
> which in current form may add some negative impact, e.g. dom0 will try to
control
> Px/Cx to conflict with Xen. So some tweaks may be required in that part.
Yup. Hadn''t even looked at the cpufreq tries to do
yet.
> 
> given our purpose now, is to come up a cleaner approach which tolerate some
> assumptions (e.g. #VCPU of dom0 == #PCPU), there''s another option
following this
> trend (perhaps compensate your idea). We can register a Xen-cpuidle and 
> xen-cpufreq driver to current Linux cpuidle and cpufreq framework, which
plays
> mainly two roles:
> 	- a dummy driver to prevent dom0 touching actual Px/Cx
> 	- parse ACPI Cx/Px information to Xen, in a similar way you did above
Yeah, I like where you are heading.
> 
> there may have some other trickiness, but the majority code will be
self-contained.
<nods>
> 
> Thanks
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jan 17, 2012 at 12:13:14PM -0500, Konrad Rzeszutek Wilk
wrote:
> > > I was trying to figure out how difficult it would be to just
bring Pxx states to
> > > the Xen hypervisor using the existing ACPI interfaces. And while
it did not pass
> > > all the _Pxx states (seems that all the _PCT, _PSS, _PSD, _PPC
flags need to
> > > be enabled in the hypercall to make this work), it demonstrates
what I had in
> > > mind. 
> 
> .. snip..
> > > 	/* TODO: Under Xen, the C-states information is not present.
> > >  	 * Figure out why. */
> > 
> > it''s possible related to this long thread:
> > 
> > http://lists.xen.org/archives/html/xen-devel/2011-08/msg00511.html
> > 
> > IOW, Xen doesn''t export mwait capability to dom0, which
impacts _PDC setting.
> > Final solution is to have a para-virtualized PDC call for that.
> 
> Aaah. Let me play with that a bit. Thanks for the pointer.
> 
> .. snip..
> > the prerequisites for this module to work correctly, is that dom0 has
the right
> > configurations to have all necessary Cx/Px information ready before
this
> > module is loaded. That may mean enabling full CONFIG_CPU_IDLE and
CONFIG_CPUFREQ,
> 
> Right.
> > which in current form may add some negative impact, e.g. dom0 will try
to control
> > Px/Cx to conflict with Xen. So some tweaks may be required in that
part.
> 
> Yup. Hadn''t even looked at the cpufreq tries to do yet.
> > 
> > given our purpose now, is to come up a cleaner approach which tolerate
some
> > assumptions (e.g. #VCPU of dom0 == #PCPU), there''s another
option following this
> > trend (perhaps compensate your idea). We can register a Xen-cpuidle
and
> > xen-cpufreq driver to current Linux cpuidle and cpufreq framework,
which plays
> > mainly two roles:
> > 	- a dummy driver to prevent dom0 touching actual Px/Cx
> > 	- parse ACPI Cx/Px information to Xen, in a similar way you did above
> 
> Yeah, I like where you are heading.
> > 
> > there may have some other trickiness, but the majority code will be
self-contained.
> 
> <nods>
For reference, the attached module does end up programming the Pxx states in the
hypervisor. The issues that I hit on a Core i3 box (some MSI motherboard) it
would fail
on the PCT, but I hadn''t really dug into this. And did not look any
further in the
Cxx states issue either. On a old Core 2 Duo it looked to have programmed the
hypervisor
fine, but the machine afterwards started to act very weird so I am sure there is
something extra that needs to be done (like maybe not using memcpy in this
module).

#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/types.h>
#include <acpi/acpi_bus.h>
#include <acpi/acpi_drivers.h>
#include <acpi/processor.h>
#include <linux/cpumask.h>

#include <xen/interface/platform.h>
#include <asm/xen/hypercall.h>

#define DRV_NAME	"ACPI_PXX"
#define DRV_CLASS	"ACPI_PXX_CLASS"
MODULE_AUTHOR("Konrad Rzeszutek Wilk");
MODULE_DESCRIPTION("ACPI Processor Driver to send data to Xen
hypervisor");
MODULE_LICENSE("GPL");

static int parse_acpi_cxx(struct acpi_processor *_pr)
{
	struct acpi_processor_cx *cx;
	int i;

	for (i = 1; i <= _pr->power.count; i++) {
		cx = &_pr->power.states[i];
		if (!cx->valid)
			continue;
		pr_info("%s: %d %d %d 0x%x\n", __func__,
			cx->type, cx->latency, cx->power, (u32)cx->address);
	}
	/* TODO: Under Xen, the C-states information is not present.
 	 * Figure out why.
 	 * Kevin thinks it might be:
http://lists.xen.org/archives/html/xen-devel/2011-08/msg00511.html
 	 * But perhaps it is
http://lists.xen.org/archives/html/xen-devel/2011-08/msg00521.html?
 	 */
	return 0;
}
static struct xen_processor_px *xen_copy_pss_data(struct acpi_processor *_pr,
		  				  struct xen_processor_performance *xen_perf)
{
	struct xen_processor_px *xen_states = NULL;
	int i;

	xen_states = kzalloc(_pr->performance->state_count *
			     sizeof(struct xen_processor_px), GFP_KERNEL);
	if (!xen_states)
		return ERR_PTR(-ENOMEM);

	xen_perf->state_count = _pr->performance->state_count;
	for (i = 0; i < _pr->performance->state_count; i++) {
		/* Figure out if the lack of __packed is bad */
		memcpy(&(xen_states[i]), &(_pr->performance->states[i]),
		       sizeof(struct acpi_processor_px));
	}
	return xen_states;
}
static int
xen_copy_psd_data(struct acpi_processor *_pr,
		  struct xen_processor_performance *xen_perf)
{
	/* Figure out if the lack of __packed is bad */
	printk(KERN_INFO "psd: %ld\n",
		offsetof(struct xen_processor_performance, domain_info.num_entries));

	xen_perf->shared_type = _pr->performance->shared_type;
	memcpy(&(xen_perf->domain_info),
&(_pr->performance->domain_info),
	       sizeof(struct acpi_psd_package));

	return 0;
}
static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
{
	int ret = -EINVAL;
	struct xen_platform_op op = {
		.cmd			= XENPF_set_processor_pminfo,
		.interface_version	= XENPF_INTERFACE_VERSION,
		.u.set_pminfo.id	= _pr->acpi_id,
		.u.set_pminfo.type	= XEN_PM_PX,
	};
	struct xen_processor_performance *xen_perf;
	struct xen_processor_px *xen_states = NULL;

	if (!_pr->performance)
		return -ENODEV;

	xen_perf = &op.u.set_pminfo.perf;

	/* PPC */
	xen_perf->platform_limit = _pr->performance_platform_limit;
	xen_perf->flags |= XEN_PX_PPC;
	/* PCT */
	/* Mmight need to copy them individually as there are no __packed
	 * so the offset might be wrong on a 32-bit host with 64-bit hypervisor. */
	printk(KERN_INFO "address: %ld\n", offsetof(struct
xen_processor_performance, control_register.address));
	printk(KERN_INFO "address: %ld\n", offsetof(struct
xen_processor_performance, status_register.address));
	printk(KERN_INFO "state_count: %ld\n", offsetof(struct
xen_processor_performance, state_count));
	memcpy(&xen_perf->control_register,
&(_pr->performance->control_register),
	       sizeof(struct acpi_pct_register));
	memcpy(&xen_perf->status_register,
&(_pr->performance->status_register),
	       sizeof(struct acpi_pct_register));
	xen_perf->flags |= XEN_PX_PCT;
	/* PSS */
	xen_states = xen_copy_pss_data(_pr, xen_perf);
	if (!IS_ERR_OR_NULL(xen_states)) {
		set_xen_guest_handle(xen_perf->states, xen_states);
		xen_perf->flags |= XEN_PX_PSS;
	}
	/* PSD */
	if (!xen_copy_psd_data(_pr, xen_perf)) {
		xen_perf->flags |= XEN_PX_PSD;
	}
	printk(KERN_INFO "Sending %x\n", xen_perf->flags);

	ret = HYPERVISOR_dom0_op(&op);
	if (!IS_ERR_OR_NULL(xen_states))
		kfree(xen_states);
	return ret;
}
static int parse_acpi_pxx(struct acpi_processor *_pr)
{
/*
	struct acpi_processor_px *px;
	int i;

	for (i = 0; i < _pr->performance->state_count;i++) {
		px = &(_pr->performance->states[i]);
		pr_info("%s: [%d]: %d, %d, %d, %d, %d, %d\n", __func__,
			i, (u32)px->core_frequency, (u32)px->power,
			(u32)px->transition_latency,
			(u32)px->bus_master_latency,
			(u32)px->control, (u32)px->status);
	}
*/
	if (xen_initial_domain())
		return push_pxx_to_hypervisor(_pr);
	return 0;
}
static int parse_acpi_data(void)
{
	int cpu;
	int err = -ENODEV;
	struct acpi_processor *_pr;
	struct cpuinfo_x86 *c = &cpu_data(0);

	/* TODO: Under AMD, the information is populated
	 * using the powernow-k8 driver which does an MSR_PSTATE_CUR_LIMIT
	 * MSR which returns the wrong value so the population of
''processors''
	 * has bogus data. So only run this under Intel for right now. */
	if (!cpu_has(c, X86_FEATURE_EST))
		return -ENODEV;
	for_each_possible_cpu(cpu) {
		_pr = per_cpu(processors, cpu);
		if (!_pr)
			continue;

		if (_pr->flags.power)
			(void)parse_acpi_cxx(_pr);

		if (_pr->performance->states)
			err = parse_acpi_pxx(_pr);
		if (err)
			break;
	}
	return -ENODEV; /* force it to unload */
}
static int __init acpi_pxx_init(void)
{
	return parse_acpi_data();
}
static void __exit acpi_pxx_exit(void)
{
}
module_init(acpi_pxx_init);
module_exit(acpi_pxx_exit);
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jan 17, 2012 at 01:19:22PM -0500, Konrad Rzeszutek Wilk
wrote:
> On Tue, Jan 17, 2012 at 12:13:14PM -0500, Konrad Rzeszutek Wilk wrote:
> > > > I was trying to figure out how difficult it would be to just
bring Pxx states to
> > > > the Xen hypervisor using the existing ACPI interfaces. And
while it did not pass
> > > > all the _Pxx states (seems that all the _PCT, _PSS, _PSD,
_PPC flags need to
> > > > be enabled in the hypercall to make this work), it
demonstrates what I had in
> > > > mind. 
> > 
> > .. snip..
> > > > 	/* TODO: Under Xen, the C-states information is not
present.
> > > >  	 * Figure out why. */
> > > 
> > > it''s possible related to this long thread:
> > > 
> > >
http://lists.xen.org/archives/html/xen-devel/2011-08/msg00511.html
> > > 
> > > IOW, Xen doesn''t export mwait capability to dom0, which
impacts _PDC setting.
> > > Final solution is to have a para-virtualized PDC call for that.
> > 
> > Aaah. Let me play with that a bit. Thanks for the pointer.
Found out the reason. It was that the hypervisor did not expose the MWAIT
bit and that dom0 was setting boot_option_idle...

The #1 patch has the fix for that.

.. snip.
> > > which in current form may add some negative impact, e.g. dom0
will try to control
> > > Px/Cx to conflict with Xen. So some tweaks may be required in
that part.
> > 
> > Yup. Hadn''t even looked at the cpufreq tries to do yet.
> > > 
> > > given our purpose now, is to come up a cleaner approach which
tolerate some
> > > assumptions (e.g. #VCPU of dom0 == #PCPU), there''s
another option following this
> > > trend (perhaps compensate your idea). We can register a
Xen-cpuidle and
> > > xen-cpufreq driver to current Linux cpuidle and cpufreq
framework, which plays
> > > mainly two roles:
> > > 	- a dummy driver to prevent dom0 touching actual Px/Cx
This is still TODO - I hadn''t really looked to see what dom0 does and
if the hypervisor ignores the dom0 (I sure it does so!). 

But interestigly enough, the cpuidle driver is not doing anything
b/c the cpuidle_disable() call which inhibits it from running.
So we might not a dummy driver for cpuidle. Not so sure about cpufreq.
> > > 	- parse ACPI Cx/Px information to Xen, in a similar way you did
above
The attached #2 patch does that - and it works at least on Intel machines.
I hadn''t done any extensive testing, like doing ''xl vcpu-set 0
X'' as that
seems to crash on 3.3 - irregardless of these patches :-)

But ''xenpm'' and running some guests seems to work just fine so
I am
hopefull.

There are still some TODOs with this:
 - which is how to make the module be autoloaded after the processor.ko
   (or rather acpi_cpufreq_cpu_init) has been loaded. As right now you
   have to manually load the driver.
 - make it work under AMD. I think that requires trapping the MSR call.
 - check the cpufreq notification calls.
 - double check that cpuidle is indeed not called.
 - play with dom0_max_vcpus= or ''xl vcpu-set 0 1''

> +	if (pr->id == -1) {
> +		int device_declaration;
> +		int apic_id = -1;
> +
> +		if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID))
> +			device_declaration = 0;
> +		else
> +			device_declaration = 1;
> +
> +		apic_id = acpi_get_cpuid(pr->handle,
> +			device_declaration, pr->acpi_id);
> +		if (apic_id == -1) {
> +			/* Processor is not present in MADT table */
So I was struggling to find an easy way to make the cases below (where
VCPU != physical CPU) work with using the driver that iterates over the
''processor'' and was mystified to why it would not work, even
with this
patchset. Found out that the acpi_get_cpuid does this:


201 #ifdef CONFIG_SMP
202         for_each_possible_cpu(i) {
203                 if (cpu_physical_id(i) == apic_id)
204                         return i;
205         }

and since not-online vCPUs (so dom0_max_vcpus) are not in the
"possible"
bitmask, we never get to check line 203 and end up returning -1 for
offline/not-present/not-possible vCPUs.

Which means that we end up here:
> +			return 0;
> +		}
> +
instead of going through the pr->id = 0.

By the end of this, the information that the hypervisor gets is
actually limited to the amount of CPUs that we specified in
dom0_max_vcpus
> +		/*
> +		 * It''s possible to have pr->id as ''-1''
even when it''s actually
> +		 * present in MADT table, e.g. due to limiting dom0 max vcpus
> +		 * less than physical present number. In such case we still want
> +		 * to parse ACPI processor object information, so mimic the
> +		 * pr->id to CPU-0. This should be safe because we only care
> +		 * about raw ACPI information, which only relies on pr->acpi_id.
> +		 * For other information relying on pr->id and gathered through
> +		 * SMP function call, it''s safe to let them run on CPU-0 since
> +		 * underlying Xen will collect them. Only a valid pr->id can
> +		 * make later invocations forward progress.
> +		 */
> +		pr->id = 0;
> +	}
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html