Yu, Ke
2009-Nov-25 11:55 UTC
[Xen-devel] [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup
Base on git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git branch:
xen/dom0/acpi-parser
===
Xen acpi processor logic cleanup
From: Yu Ke <ke.yu@intel.com>
current xen acpi processor logic scattered in several places and mixed with
other acpi_processor_driver logic. this patch try to split these logic out and
consolidate them into a new xen acpi_processor_driver. this will make xen acpi
processor logic more clean
Signed-off-by: Yu Ke <ke.yu@intel.com>
---
arch/x86/kernel/acpi/processor.c | 16 +-
drivers/acpi/processor_core.c | 344 ++++++++++++++++++++++++++++++++++++--
drivers/acpi/processor_idle.c | 94 ++++++++++
drivers/acpi/processor_perflib.c | 21 ++
include/acpi/processor.h | 9 +
5 files changed, 456 insertions(+), 28 deletions(-)
diff --git a/arch/x86/kernel/acpi/processor.c b/arch/x86/kernel/acpi/processor.c
index 721272b..05ef6a6 100644
--- a/arch/x86/kernel/acpi/processor.c
+++ b/arch/x86/kernel/acpi/processor.c
@@ -79,9 +79,6 @@ void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
{
struct cpuinfo_x86 *c = &cpu_data(pr->id);
- if (xen_pv_domain())
- c = &cpu_data(0);
-
pr->pdc = NULL;
if (c->x86_vendor == X86_VENDOR_INTEL)
init_intel_pdc(pr, c);
@@ -91,6 +88,19 @@ void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
EXPORT_SYMBOL(arch_acpi_processor_init_pdc);
+/* Initialize _PDC data based on the CPU vendor */
+void xen_arch_acpi_processor_init_pdc(struct acpi_processor *pr)
+{
+ struct cpuinfo_x86 *c = &cpu_data(0);
+
+ pr->pdc = NULL;
+ if (c->x86_vendor == X86_VENDOR_INTEL)
+ init_intel_pdc(pr, c);
+
+ return;
+}
+EXPORT_SYMBOL(xen_arch_acpi_processor_init_pdc);
+
void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr)
{
if (pr->pdc) {
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 98010d5..6a35118 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -87,6 +87,8 @@ 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);
+static int xen_acpi_processor_start(struct acpi_device *device);
+static void xen_acpi_processor_notify(struct acpi_device *device, u32 event);
static const struct acpi_device_id processor_device_ids[] = {
{ACPI_PROCESSOR_OBJECT_HID, 0},
@@ -109,6 +111,20 @@ static struct acpi_driver acpi_processor_driver = {
},
};
+static struct acpi_driver xen_acpi_processor_driver = {
+ .name = "processor",
+ .class = ACPI_PROCESSOR_CLASS,
+ .ids = processor_device_ids,
+ .ops = {
+ .add = acpi_processor_add,
+ .remove = acpi_processor_remove,
+ .start = xen_acpi_processor_start,
+ .suspend = acpi_processor_suspend,
+ .resume = acpi_processor_resume,
+ .notify = xen_acpi_processor_notify,
+ },
+};
+
#define INSTALL_NOTIFY_HANDLER 1
#define UNINSTALL_NOTIFY_HANDLER 2
@@ -411,6 +427,10 @@ static int acpi_processor_remove_fs(struct acpi_device
*device)
#ifndef CONFIG_SMP
static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; }
+static int is_processor_apic_enabled(struct acpi_processor *pr, int type)
+{
+ return 0;
+}
#else
static struct acpi_table_madt *madt;
@@ -561,6 +581,26 @@ static int get_cpu_id(acpi_handle handle, int type, u32
acpi_id)
}
return -1;
}
+
+/*
+ * check the MADT entry to see if its apic is enabled or not.
+ *
+ * in Xen domain 0 case, one acpi processor may be physically present
+ * but not enumerated due to #vCPU != #pCPU
+ * this function can be used to check this case.
+ */
+static int is_processor_apic_enabled(struct acpi_processor *pr, int type)
+{
+
+ int apic_id;
+
+ apic_id = map_mat_entry(pr->handle, type, pr->acpi_id);
+ if (apic_id == -1)
+ apic_id = map_madt_entry(type, pr->acpi_id);
+
+ return (apic_id != -1);
+}
+
#endif
/* --------------------------------------------------------------------------
@@ -639,9 +679,6 @@ static int acpi_processor_get_info(struct acpi_device
*device)
pr->id = cpu_index;
- if (xen_pv_domain())
- pr->id = pr->acpi_id;
-
/*
* Extra Processor objects may be enumerated on MP systems with
* less than the max # of CPUs. They should be ignored _iff
@@ -716,17 +753,15 @@ static int __cpuinit acpi_processor_start(struct
acpi_device *device)
return 0;
}
- if (!xen_pv_domain())
- BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
+ BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0));
/*
* Buggy BIOS check
* ACPI id of processors can be reported wrongly by the BIOS.
* Don''t trust it blindly
*/
- if (!xen_pv_domain() &&
- per_cpu(processor_device_array, pr->id) != NULL &&
- per_cpu(processor_device_array, pr->id) != device) {
+ if (per_cpu(processor_device_array, pr->id) != NULL &&
+ per_cpu(processor_device_array, pr->id) != device) {
printk(KERN_WARNING "BIOS reported wrong ACPI id "
"for the processor\n");
return -ENODEV;
@@ -758,10 +793,6 @@ static int __cpuinit acpi_processor_start(struct
acpi_device *device)
acpi_processor_power_init(pr, device);
- result = processor_cntl_xen_prepare(pr);
- if (result)
- goto end;
-
pr->cdev = thermal_cooling_device_register("Processor",
device,
&processor_cooling_ops);
if (IS_ERR(pr->cdev)) {
@@ -1165,6 +1196,284 @@ void acpi_processor_uninstall_hotplug_notify(void)
}
/*
+ * xen specific ACPI processor driver
+ */
+
+static int xen_acpi_processor_get_info(struct acpi_device *device)
+{
+ acpi_status status = 0;
+ union acpi_object object = { 0 };
+ struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
+ struct acpi_processor *pr;
+ int cpu_index, device_declaration = 0;
+ static int cpu0_initialized;
+
+ pr = acpi_driver_data(device);
+ if (!pr)
+ return -EINVAL;
+
+ if (num_online_cpus() > 1)
+ errata.smp = TRUE;
+
+ acpi_processor_errata(pr);
+
+ /*
+ * Check to see if we have bus mastering arbitration control. This
+ * is required for proper C3 usage (to maintain cache coherency).
+ */
+ if (acpi_gbl_FADT.pm2_control_block &&
+ acpi_gbl_FADT.pm2_control_length) {
+ pr->flags.bm_control = 1;
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Bus mastering arbitration control
present\n"));
+ } else
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "No bus mastering arbitration control\n"));
+
+ if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
+ /* Declared with "Processor" statement; match
ProcessorID */
+ status = acpi_evaluate_object(pr->handle, NULL, NULL,
&buffer);
+ if (ACPI_FAILURE(status)) {
+ printk(KERN_ERR PREFIX "Evaluating processor
object\n");
+ return -ENODEV;
+ }
+
+ /*
+ * TBD: Synch processor ID (via LAPIC/LSAPIC structures) on SMP.
+ * >>> ''acpi_get_processor_id(acpi_id,
&id)'' in
+ * arch/xxx/acpi.c
+ */
+ pr->acpi_id = object.processor.proc_id;
+ } else {
+ /*
+ * Declared with "Device" statement; match _UID.
+ * Note that we don''t handle string _UIDs yet.
+ */
+ unsigned long long value;
+ status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
+ NULL, &value);
+ if (ACPI_FAILURE(status)) {
+ printk(KERN_ERR PREFIX
+ "Evaluating processor _UID
[%#x]\n", status);
+ return -ENODEV;
+ }
+ device_declaration = 1;
+ pr->acpi_id = value;
+ }
+ cpu_index = get_cpu_id(pr->handle, device_declaration,
pr->acpi_id);
+
+ /* Handle UP system running SMP kernel, with no LAPIC in MADT */
+ if (!cpu0_initialized && (cpu_index == -1) &&
+ (num_online_cpus() == 1)) {
+ cpu_index = 0;
+ }
+
+ cpu0_initialized = 1;
+
+ pr->id = cpu_index;
+
+ /*
+ * Extra Processor objects may be enumerated on MP systems with
+ * less than the max # of CPUs, or Xen vCPU < pCPU.
+ * They should be ignored _iff they are physically not present.
+ *
+ */
+ if (pr->id == -1) {
+ if (ACPI_FAILURE(acpi_processor_hotadd_init(pr->handle,
&pr->id))
+ && !is_processor_apic_enabled(pr,
device_declaration))
+ return -ENODEV;
+ }
+
+ /*
+ * On some boxes several processors use the same processor bus id.
+ * But they are located in different scope. For example:
+ * \_SB.SCK0.CPU0
+ * \_SB.SCK1.CPU0
+ * Rename the processor device bus id. And the new bus id will be
+ * generated as the following format:
+ * CPU+CPU ID.
+ */
+ sprintf(acpi_device_bid(device), "CPU%X", pr->id);
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Processor [%d:%d]\n",
pr->id,
+ pr->acpi_id));
+
+ if (!object.processor.pblk_address)
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No PBLK (NULL
address)\n"));
+ else if (object.processor.pblk_length != 6)
+ printk(KERN_ERR PREFIX "Invalid PBLK length [%d]\n",
+ object.processor.pblk_length);
+ else {
+ pr->throttling.address = object.processor.pblk_address;
+ pr->throttling.duty_offset = acpi_gbl_FADT.duty_offset;
+ pr->throttling.duty_width = acpi_gbl_FADT.duty_width;
+
+ pr->pblk = object.processor.pblk_address;
+
+ /*
+ * We don''t care about error returns - we just try to
mark
+ * these reserved so that nobody else is confused into thinking
+ * that this region might be unused..
+ *
+ * (In particular, allocating the IO range for Cardbus)
+ */
+ request_region(pr->throttling.address, 6, "ACPI CPU
throttle");
+ }
+
+ /*
+ * If ACPI describes a slot number for this CPU, we can use it
+ * ensure we get the right value in the "physical id" field
+ * of /proc/cpuinfo
+ */
+ status = acpi_evaluate_object(pr->handle, "_SUN", NULL,
&buffer);
+ if (ACPI_SUCCESS(status))
+ arch_fix_phys_package_id(pr->id, object.integer.value);
+
+ return 0;
+}
+
+#define MAX_ACPI_ID 255
+
+static struct acpi_device *processor_device_array[MAX_ACPI_ID + 1];
+
+static int __cpuinit xen_acpi_processor_start(struct acpi_device *device)
+{
+ int result = 0;
+ struct acpi_processor *pr;
+
+ pr = acpi_driver_data(device);
+
+ result = xen_acpi_processor_get_info(device);
+ if (result) {
+ /* Processor is physically not present */
+ return 0;
+ }
+
+ if (pr->acpi_id > MAX_ACPI_ID)
+ return 0;
+ /*
+ * Buggy BIOS check
+ * ACPI id of processors can be reported wrongly by the BIOS.
+ * Don''t trust it blindly
+ */
+ if (processor_device_array[pr->acpi_id] != NULL &&
+ processor_device_array[pr->acpi_id] != device) {
+ printk(KERN_WARNING "BIOS reported wrong ACPI id "
+ "for the processor\n");
+ return -ENODEV;
+ }
+
+ processor_device_array[pr->acpi_id] = device;
+
+ if (pr->id != -1) {
+ per_cpu(processors, pr->id) = pr;
+
+ result = acpi_processor_add_fs(device);
+ if (result)
+ goto end;
+ }
+
+ /* _PDC call should be done before doing anything else (if reqd.). */
+ xen_arch_acpi_processor_init_pdc(pr);
+ acpi_processor_set_pdc(pr);
+ arch_acpi_processor_cleanup_pdc(pr);
+
+#ifdef CONFIG_CPU_FREQ
+ xen_acpi_processor_ppc_has_changed(pr);
+#endif
+
+ /*
+ * pr->id may equal to -1 while Xen enabled.
+ * throttle and thermal module don''t support this case.
+ * Tx only works when dom0 vcpu == pcpu num by far, as we give
+ * control to dom0.
+ */
+ if (pr->id != -1) {
+ acpi_processor_get_throttling_info(pr);
+ acpi_processor_get_limit_info(pr);
+ }
+
+ xen_acpi_processor_power_init(pr, device);
+
+ result = processor_cntl_xen_prepare(pr);
+ if (result)
+ goto end;
+
+ if (pr->id != -1) {
+ pr->cdev =
thermal_cooling_device_register("Processor", device,
+ &processor_cooling_ops);
+ if (IS_ERR(pr->cdev)) {
+ result = PTR_ERR(pr->cdev);
+ goto end;
+ }
+
+ dev_info(&device->dev, "registered as
cooling_device%d\n",
+ pr->cdev->id);
+
+ result = sysfs_create_link(&device->dev.kobj,
+ &pr->cdev->device.kobj,
+ "thermal_cooling");
+ if (result)
+ printk(KERN_ERR PREFIX "Create sysfs link\n");
+ result = sysfs_create_link(&pr->cdev->device.kobj,
+ &device->dev.kobj,
+ "device");
+ if (result)
+ printk(KERN_ERR PREFIX "Create sysfs link\n");
+
+ if (pr->flags.throttling) {
+ printk(KERN_INFO PREFIX "%s [%s] (supports",
+ acpi_device_name(device),
acpi_device_bid(device));
+ printk(" %d throttling states",
pr->throttling.state_count);
+ printk(")\n");
+ }
+ }
+
+end:
+
+ return result;
+}
+
+static void xen_acpi_processor_notify(struct acpi_device *device, u32 event)
+{
+ struct acpi_processor *pr = acpi_driver_data(device);
+ int saved;
+
+ if (!pr)
+ return;
+
+ switch (event) {
+ case ACPI_PROCESSOR_NOTIFY_PERFORMANCE:
+ saved = pr->performance_platform_limit;
+ xen_acpi_processor_ppc_has_changed(pr);
+ if (saved == pr->performance_platform_limit)
+ break;
+ acpi_bus_generate_proc_event(device, event,
+ pr->performance_platform_limit);
+ acpi_bus_generate_netlink_event(device->pnp.device_class,
+ dev_name(&device->dev),
event,
+
pr->performance_platform_limit);
+ break;
+ case ACPI_PROCESSOR_NOTIFY_POWER:
+ xen_acpi_processor_cst_has_changed(pr);
+ acpi_bus_generate_proc_event(device, event, 0);
+ acpi_bus_generate_netlink_event(device->pnp.device_class,
+ dev_name(&device->dev),
event, 0);
+ break;
+ case ACPI_PROCESSOR_NOTIFY_THROTTLING:
+ acpi_processor_tstate_has_changed(pr);
+ acpi_bus_generate_proc_event(device, event, 0);
+ acpi_bus_generate_netlink_event(device->pnp.device_class,
+ dev_name(&device->dev),
event, 0);
+ default:
+ ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+ "Unsupported event [0x%x]\n",
event));
+ break;
+ }
+
+ return;
+}
+
+/*
* We keep the driver loaded even when ACPI is not running.
* This is needed for the powernow-k8 driver, that works even without
* ACPI, but needs symbols from this driver
@@ -1198,7 +1507,11 @@ static int __init acpi_processor_init(void)
if (result < 0)
goto out_proc;
- result = acpi_bus_register_driver(&acpi_processor_driver);
+ if (xen_initial_domain())
+ result =
acpi_bus_register_driver(&xen_acpi_processor_driver);
+ else
+ result = acpi_bus_register_driver(&acpi_processor_driver);
+
if (result < 0)
goto out_cpuidle;
@@ -1232,7 +1545,10 @@ static void __exit acpi_processor_exit(void)
acpi_processor_uninstall_hotplug_notify();
- acpi_bus_unregister_driver(&acpi_processor_driver);
+ if (xen_initial_domain())
+ acpi_bus_unregister_driver(&xen_acpi_processor_driver);
+ else
+ acpi_bus_unregister_driver(&acpi_processor_driver);
cpuidle_unregister_driver(&acpi_idle_driver);
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 25680c6..1b382e2 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1148,13 +1148,6 @@ int acpi_processor_cst_has_changed(struct acpi_processor
*pr)
if (!pr->flags.power_setup_done)
return -ENODEV;
- if (xen_initial_domain()) {
- acpi_processor_get_power_info(pr);
- processor_cntl_xen_notify(pr,
- PROCESSOR_PM_CHANGE, PM_TYPE_IDLE);
- return ret;
- }
-
cpuidle_pause_and_lock();
cpuidle_disable_device(&pr->power.dev);
acpi_processor_get_power_info(pr);
@@ -1167,6 +1160,44 @@ int acpi_processor_cst_has_changed(struct acpi_processor
*pr)
return ret;
}
+static int xen_acpi_processor_get_power_info(struct acpi_processor *pr)
+{
+ int ret;
+ int invalid_pr_id = 0;
+
+ /*
+ * acpi_processor_get_power_info need valid pr->id
+ * so set pr->id=0 temporarily
+ */
+ if (pr->id == -1) {
+ invalid_pr_id = 1;
+ pr->id = 0;
+ }
+
+ ret = acpi_processor_get_power_info(pr);
+
+ if (invalid_pr_id)
+ pr->id = -1;
+
+ return ret;
+}
+
+int xen_acpi_processor_cst_has_changed(struct acpi_processor *pr)
+{
+ if (!pr)
+ return -EINVAL;
+
+ if (!pr->flags.power_setup_done)
+ return -ENODEV;
+
+ xen_acpi_processor_get_power_info(pr);
+
+ processor_cntl_xen_notify(pr,
+ PROCESSOR_PM_CHANGE, PM_TYPE_IDLE);
+
+ return 0;
+}
+
int __cpuinit acpi_processor_power_init(struct acpi_processor *pr,
struct acpi_device *device)
{
@@ -1245,6 +1276,55 @@ int __cpuinit acpi_processor_power_init(struct
acpi_processor *pr,
return 0;
}
+int __cpuinit xen_acpi_processor_power_init(struct acpi_processor *pr,
+ struct acpi_device *device)
+{
+ acpi_status status = 0;
+ struct proc_dir_entry *entry = NULL;
+ unsigned int i;
+
+ if (!pr)
+ return -EINVAL;
+
+ if (acpi_gbl_FADT.cst_control) {
+ status = acpi_os_write_port(acpi_gbl_FADT.smi_command,
+ acpi_gbl_FADT.cst_control, 8);
+ if (ACPI_FAILURE(status)) {
+ ACPI_EXCEPTION((AE_INFO, status,
+ "Notifying BIOS of _CST ability
failed"));
+ }
+ }
+
+ xen_acpi_processor_get_power_info(pr);
+
+ pr->flags.power_setup_done = 1;
+
+ if (pr->flags.power) {
+ processor_cntl_xen_notify(pr,
+ PROCESSOR_PM_INIT, PM_TYPE_IDLE);
+
+ printk(KERN_INFO PREFIX "CPU%d (power states:",
pr->id);
+ for (i = 1; i <= pr->power.count; i++)
+ if (pr->power.states[i].valid)
+ printk(" C%d[C%d]", i,
+ pr->power.states[i].type);
+ printk(")\n");
+ }
+
+ if (pr->id != -1) {
+ /* ''power'' [R]
+ */
+ entry = proc_create_data(ACPI_PROCESSOR_FILE_POWER,
+ S_IRUGO, acpi_device_dir(device),
+ &acpi_processor_power_fops,
+ acpi_driver_data(device));
+ if (!entry)
+ return -EIO;
+ }
+
+ return 0;
+}
+
int acpi_processor_power_exit(struct acpi_processor *pr,
struct acpi_device *device)
{
diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
index 8375075..f17e740 100644
--- a/drivers/acpi/processor_perflib.c
+++ b/drivers/acpi/processor_perflib.c
@@ -155,20 +155,33 @@ int acpi_processor_ppc_has_changed(struct acpi_processor
*pr)
{
int ret;
- if (ignore_ppc && !processor_cntl_xen_pmperf())
+ if (ignore_ppc)
return 0;
ret = acpi_processor_get_platform_limit(pr);
if (ret < 0)
return (ret);
- else if (processor_cntl_xen_pmperf())
- return processor_cntl_xen_notify(pr,
- PROCESSOR_PM_CHANGE, PM_TYPE_PERF);
else
return cpufreq_update_policy(pr->id);
}
+int xen_acpi_processor_ppc_has_changed(struct acpi_processor *pr)
+{
+ int ret;
+
+ if (!processor_cntl_xen_pmperf())
+ return 0;
+
+ ret = acpi_processor_get_platform_limit(pr);
+
+ if (ret < 0)
+ return ret;
+ else
+ return processor_cntl_xen_notify(pr,
+ PROCESSOR_PM_CHANGE, PM_TYPE_PERF);
+}
+
void acpi_processor_ppc_init(void)
{
if (!cpufreq_register_notifier
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 221d30d..c806ecf 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -259,6 +259,7 @@ extern struct acpi_processor_errata errata;
void arch_acpi_processor_init_pdc(struct acpi_processor *pr);
void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr);
+void xen_arch_acpi_processor_init_pdc(struct acpi_processor *pr);
#ifdef ARCH_HAS_POWER_INIT
void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
@@ -297,6 +298,7 @@ void acpi_processor_ppc_exit(void);
int acpi_processor_ppc_has_changed(struct acpi_processor *pr);
int acpi_processor_get_performance_info(struct acpi_processor *pr);
int acpi_processor_get_psd(struct acpi_processor *pr);
+int xen_acpi_processor_ppc_has_changed(struct acpi_processor *pr);
#else
static inline void acpi_processor_ppc_init(void)
{
@@ -318,6 +320,10 @@ static inline int acpi_processor_ppc_has_changed(struct
acpi_processor *pr)
}
return 0;
}
+static inline int xen_acpi_processor_ppc_has_changed(struct acpi_processor *pr)
+{
+ return acpi_processor_ppc_has_changed(pr);
+}
#endif /* CONFIG_CPU_FREQ */
/* in processor_throttling.c */
@@ -330,7 +336,10 @@ extern void acpi_processor_throttling_init(void);
/* in processor_idle.c */
int acpi_processor_power_init(struct acpi_processor *pr,
struct acpi_device *device);
+int xen_acpi_processor_power_init(struct acpi_processor *pr,
+ struct acpi_device *device);
int acpi_processor_cst_has_changed(struct acpi_processor *pr);
+int xen_acpi_processor_cst_has_changed(struct acpi_processor *pr);
int acpi_processor_power_exit(struct acpi_processor *pr,
struct acpi_device *device);
int acpi_processor_suspend(struct acpi_device * device, pm_message_t state);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-Nov-25 18:48 UTC
[Xen-devel] Re: [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup
On 11/25/09 03:55, Yu, Ke wrote:> Base on git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git branch: xen/dom0/acpi-parser > > ===> > Xen acpi processor logic cleanup > > From: Yu Ke <ke.yu@intel.com> > > current xen acpi processor logic scattered in several places and mixed with other acpi_processor_driver logic. this patch try to split these logic out and consolidate them into a new xen acpi_processor_driver. this will make xen acpi processor logic more clean > > Signed-off-by: Yu Ke <ke.yu@intel.com> >Thanks, this looks like a big improvement. I have some comments about the details though: * Split this into 3 patches: o revert the old Xen-specific changes o add the new common code (which may just be is_processor_apic_enabled()) o add the new Xen-specific code in a new file * Make acpi_processor_driver a pointer to the preferred driver structure which defaults to the normal driver, and have some Xen code re-point it to the Xen one during Xen setup, rather than having an explicit Xen test in the core ACPI code Thanks, J> --- > > arch/x86/kernel/acpi/processor.c | 16 +- > drivers/acpi/processor_core.c | 344 ++++++++++++++++++++++++++++++++++++-- > drivers/acpi/processor_idle.c | 94 ++++++++++ > drivers/acpi/processor_perflib.c | 21 ++ > include/acpi/processor.h | 9 + > 5 files changed, 456 insertions(+), 28 deletions(-) > > > diff --git a/arch/x86/kernel/acpi/processor.c b/arch/x86/kernel/acpi/processor.c > index 721272b..05ef6a6 100644 > --- a/arch/x86/kernel/acpi/processor.c > +++ b/arch/x86/kernel/acpi/processor.c > @@ -79,9 +79,6 @@ void arch_acpi_processor_init_pdc(struct acpi_processor *pr) > { > struct cpuinfo_x86 *c = &cpu_data(pr->id); > > - if (xen_pv_domain()) > - c = &cpu_data(0); > - > pr->pdc = NULL; > if (c->x86_vendor == X86_VENDOR_INTEL) > init_intel_pdc(pr, c); > @@ -91,6 +88,19 @@ void arch_acpi_processor_init_pdc(struct acpi_processor *pr) > > EXPORT_SYMBOL(arch_acpi_processor_init_pdc); > > +/* Initialize _PDC data based on the CPU vendor */ > +void xen_arch_acpi_processor_init_pdc(struct acpi_processor *pr) > +{ > + struct cpuinfo_x86 *c = &cpu_data(0); > + > + pr->pdc = NULL; > + if (c->x86_vendor == X86_VENDOR_INTEL) > + init_intel_pdc(pr, c); > + > + return; > +} > +EXPORT_SYMBOL(xen_arch_acpi_processor_init_pdc); > + > void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr) > { > if (pr->pdc) { > diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c > index 98010d5..6a35118 100644 > --- a/drivers/acpi/processor_core.c > +++ b/drivers/acpi/processor_core.c > @@ -87,6 +87,8 @@ 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); > > +static int xen_acpi_processor_start(struct acpi_device *device); > +static void xen_acpi_processor_notify(struct acpi_device *device, u32 event); > > static const struct acpi_device_id processor_device_ids[] = { > {ACPI_PROCESSOR_OBJECT_HID, 0}, > @@ -109,6 +111,20 @@ static struct acpi_driver acpi_processor_driver = { > }, > }; > > +static struct acpi_driver xen_acpi_processor_driver = { > + .name = "processor", > + .class = ACPI_PROCESSOR_CLASS, > + .ids = processor_device_ids, > + .ops = { > + .add = acpi_processor_add, > + .remove = acpi_processor_remove, > + .start = xen_acpi_processor_start, > + .suspend = acpi_processor_suspend, > + .resume = acpi_processor_resume, > + .notify = xen_acpi_processor_notify, > + }, > +}; > + > #define INSTALL_NOTIFY_HANDLER 1 > #define UNINSTALL_NOTIFY_HANDLER 2 > > @@ -411,6 +427,10 @@ static int acpi_processor_remove_fs(struct acpi_device *device) > > #ifndef CONFIG_SMP > static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) { return -1; } > +static int is_processor_apic_enabled(struct acpi_processor *pr, int type) > +{ > + return 0; > +} > #else > > static struct acpi_table_madt *madt; > @@ -561,6 +581,26 @@ static int get_cpu_id(acpi_handle handle, int type, u32 acpi_id) > } > return -1; > } > + > +/* > + * check the MADT entry to see if its apic is enabled or not. > + * > + * in Xen domain 0 case, one acpi processor may be physically present > + * but not enumerated due to #vCPU != #pCPU > + * this function can be used to check this case. > + */ > +static int is_processor_apic_enabled(struct acpi_processor *pr, int type) > +{ > + > + int apic_id; > + > + apic_id = map_mat_entry(pr->handle, type, pr->acpi_id); > + if (apic_id == -1) > + apic_id = map_madt_entry(type, pr->acpi_id); > + > + return (apic_id != -1); > +} > + > #endif > > /* -------------------------------------------------------------------------- > @@ -639,9 +679,6 @@ static int acpi_processor_get_info(struct acpi_device *device) > > pr->id = cpu_index; > > - if (xen_pv_domain()) > - pr->id = pr->acpi_id; > - > /* > * Extra Processor objects may be enumerated on MP systems with > * less than the max # of CPUs. They should be ignored _iff > @@ -716,17 +753,15 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device) > return 0; > } > > - if (!xen_pv_domain()) > - BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0)); > + BUG_ON((pr->id >= nr_cpu_ids) || (pr->id < 0)); > > /* > * Buggy BIOS check > * ACPI id of processors can be reported wrongly by the BIOS. > * Don''t trust it blindly > */ > - if (!xen_pv_domain() && > - per_cpu(processor_device_array, pr->id) != NULL && > - per_cpu(processor_device_array, pr->id) != device) { > + if (per_cpu(processor_device_array, pr->id) != NULL && > + per_cpu(processor_device_array, pr->id) != device) { > printk(KERN_WARNING "BIOS reported wrong ACPI id " > "for the processor\n"); > return -ENODEV; > @@ -758,10 +793,6 @@ static int __cpuinit acpi_processor_start(struct acpi_device *device) > > acpi_processor_power_init(pr, device); > > - result = processor_cntl_xen_prepare(pr); > - if (result) > - goto end; > - > pr->cdev = thermal_cooling_device_register("Processor", device, > &processor_cooling_ops); > if (IS_ERR(pr->cdev)) { > @@ -1165,6 +1196,284 @@ void acpi_processor_uninstall_hotplug_notify(void) > } > > /* > + * xen specific ACPI processor driver > + */ > + > +static int xen_acpi_processor_get_info(struct acpi_device *device) > +{ > + acpi_status status = 0; > + union acpi_object object = { 0 }; > + struct acpi_buffer buffer = { sizeof(union acpi_object), &object }; > + struct acpi_processor *pr; > + int cpu_index, device_declaration = 0; > + static int cpu0_initialized; > + > + pr = acpi_driver_data(device); > + if (!pr) > + return -EINVAL; > + > + if (num_online_cpus() > 1) > + errata.smp = TRUE; > + > + acpi_processor_errata(pr); > + > + /* > + * Check to see if we have bus mastering arbitration control. This > + * is required for proper C3 usage (to maintain cache coherency). > + */ > + if (acpi_gbl_FADT.pm2_control_block && > + acpi_gbl_FADT.pm2_control_length) { > + pr->flags.bm_control = 1; > + ACPI_DEBUG_PRINT((ACPI_DB_INFO, > + "Bus mastering arbitration control present\n")); > + } else > + ACPI_DEBUG_PRINT((ACPI_DB_INFO, > + "No bus mastering arbitration control\n")); > + > + if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) { > + /* Declared with "Processor" statement; match ProcessorID */ > + status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer); > + if (ACPI_FAILURE(status)) { > + printk(KERN_ERR PREFIX "Evaluating processor object\n"); > + return -ENODEV; > + } > + > + /* > + * TBD: Synch processor ID (via LAPIC/LSAPIC structures) on SMP. > + * >>> ''acpi_get_processor_id(acpi_id, &id)'' in > + * arch/xxx/acpi.c > + */ > + pr->acpi_id = object.processor.proc_id; > + } else { > + /* > + * Declared with "Device" statement; match _UID. > + * Note that we don''t handle string _UIDs yet. > + */ > + unsigned long long value; > + status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID, > + NULL, &value); > + if (ACPI_FAILURE(status)) { > + printk(KERN_ERR PREFIX > + "Evaluating processor _UID [%#x]\n", status); > + return -ENODEV; > + } > + device_declaration = 1; > + pr->acpi_id = value; > + } > + cpu_index = get_cpu_id(pr->handle, device_declaration, pr->acpi_id); > + > + /* Handle UP system running SMP kernel, with no LAPIC in MADT */ > + if (!cpu0_initialized && (cpu_index == -1) && > + (num_online_cpus() == 1)) { > + cpu_index = 0; > + } > + > + cpu0_initialized = 1; > + > + pr->id = cpu_index; > + > + /* > + * Extra Processor objects may be enumerated on MP systems with > + * less than the max # of CPUs, or Xen vCPU < pCPU. > + * They should be ignored _iff they are physically not present. > + * > + */ > + if (pr->id == -1) { > + if (ACPI_FAILURE(acpi_processor_hotadd_init(pr->handle, &pr->id)) > + && !is_processor_apic_enabled(pr, device_declaration)) > + return -ENODEV; > + } > + > + /* > + * On some boxes several processors use the same processor bus id. > + * But they are located in different scope. For example: > + * \_SB.SCK0.CPU0 > + * \_SB.SCK1.CPU0 > + * Rename the processor device bus id. And the new bus id will be > + * generated as the following format: > + * CPU+CPU ID. > + */ > + sprintf(acpi_device_bid(device), "CPU%X", pr->id); > + ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Processor [%d:%d]\n", pr->id, > + pr->acpi_id)); > + > + if (!object.processor.pblk_address) > + ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No PBLK (NULL address)\n")); > + else if (object.processor.pblk_length != 6) > + printk(KERN_ERR PREFIX "Invalid PBLK length [%d]\n", > + object.processor.pblk_length); > + else { > + pr->throttling.address = object.processor.pblk_address; > + pr->throttling.duty_offset = acpi_gbl_FADT.duty_offset; > + pr->throttling.duty_width = acpi_gbl_FADT.duty_width; > + > + pr->pblk = object.processor.pblk_address; > + > + /* > + * We don''t care about error returns - we just try to mark > + * these reserved so that nobody else is confused into thinking > + * that this region might be unused.. > + * > + * (In particular, allocating the IO range for Cardbus) > + */ > + request_region(pr->throttling.address, 6, "ACPI CPU throttle"); > + } > + > + /* > + * If ACPI describes a slot number for this CPU, we can use it > + * ensure we get the right value in the "physical id" field > + * of /proc/cpuinfo > + */ > + status = acpi_evaluate_object(pr->handle, "_SUN", NULL, &buffer); > + if (ACPI_SUCCESS(status)) > + arch_fix_phys_package_id(pr->id, object.integer.value); > + > + return 0; > +} > + > +#define MAX_ACPI_ID 255 > + > +static struct acpi_device *processor_device_array[MAX_ACPI_ID + 1]; > + > +static int __cpuinit xen_acpi_processor_start(struct acpi_device *device) > +{ > + int result = 0; > + struct acpi_processor *pr; > + > + pr = acpi_driver_data(device); > + > + result = xen_acpi_processor_get_info(device); > + if (result) { > + /* Processor is physically not present */ > + return 0; > + } > + > + if (pr->acpi_id > MAX_ACPI_ID) > + return 0; > + /* > + * Buggy BIOS check > + * ACPI id of processors can be reported wrongly by the BIOS. > + * Don''t trust it blindly > + */ > + if (processor_device_array[pr->acpi_id] != NULL && > + processor_device_array[pr->acpi_id] != device) { > + printk(KERN_WARNING "BIOS reported wrong ACPI id " > + "for the processor\n"); > + return -ENODEV; > + } > + > + processor_device_array[pr->acpi_id] = device; > + > + if (pr->id != -1) { > + per_cpu(processors, pr->id) = pr; > + > + result = acpi_processor_add_fs(device); > + if (result) > + goto end; > + } > + > + /* _PDC call should be done before doing anything else (if reqd.). */ > + xen_arch_acpi_processor_init_pdc(pr); > + acpi_processor_set_pdc(pr); > + arch_acpi_processor_cleanup_pdc(pr); > + > +#ifdef CONFIG_CPU_FREQ > + xen_acpi_processor_ppc_has_changed(pr); > +#endif > + > + /* > + * pr->id may equal to -1 while Xen enabled. > + * throttle and thermal module don''t support this case. > + * Tx only works when dom0 vcpu == pcpu num by far, as we give > + * control to dom0. > + */ > + if (pr->id != -1) { > + acpi_processor_get_throttling_info(pr); > + acpi_processor_get_limit_info(pr); > + } > + > + xen_acpi_processor_power_init(pr, device); > + > + result = processor_cntl_xen_prepare(pr); > + if (result) > + goto end; > + > + if (pr->id != -1) { > + pr->cdev = thermal_cooling_device_register("Processor", device, > + &processor_cooling_ops); > + if (IS_ERR(pr->cdev)) { > + result = PTR_ERR(pr->cdev); > + goto end; > + } > + > + dev_info(&device->dev, "registered as cooling_device%d\n", > + pr->cdev->id); > + > + result = sysfs_create_link(&device->dev.kobj, > + &pr->cdev->device.kobj, > + "thermal_cooling"); > + if (result) > + printk(KERN_ERR PREFIX "Create sysfs link\n"); > + result = sysfs_create_link(&pr->cdev->device.kobj, > + &device->dev.kobj, > + "device"); > + if (result) > + printk(KERN_ERR PREFIX "Create sysfs link\n"); > + > + if (pr->flags.throttling) { > + printk(KERN_INFO PREFIX "%s [%s] (supports", > + acpi_device_name(device), acpi_device_bid(device)); > + printk(" %d throttling states", pr->throttling.state_count); > + printk(")\n"); > + } > + } > + > +end: > + > + return result; > +} > + > +static void xen_acpi_processor_notify(struct acpi_device *device, u32 event) > +{ > + struct acpi_processor *pr = acpi_driver_data(device); > + int saved; > + > + if (!pr) > + return; > + > + switch (event) { > + case ACPI_PROCESSOR_NOTIFY_PERFORMANCE: > + saved = pr->performance_platform_limit; > + xen_acpi_processor_ppc_has_changed(pr); > + if (saved == pr->performance_platform_limit) > + break; > + acpi_bus_generate_proc_event(device, event, > + pr->performance_platform_limit); > + acpi_bus_generate_netlink_event(device->pnp.device_class, > + dev_name(&device->dev), event, > + pr->performance_platform_limit); > + break; > + case ACPI_PROCESSOR_NOTIFY_POWER: > + xen_acpi_processor_cst_has_changed(pr); > + acpi_bus_generate_proc_event(device, event, 0); > + acpi_bus_generate_netlink_event(device->pnp.device_class, > + dev_name(&device->dev), event, 0); > + break; > + case ACPI_PROCESSOR_NOTIFY_THROTTLING: > + acpi_processor_tstate_has_changed(pr); > + acpi_bus_generate_proc_event(device, event, 0); > + acpi_bus_generate_netlink_event(device->pnp.device_class, > + dev_name(&device->dev), event, 0); > + default: > + ACPI_DEBUG_PRINT((ACPI_DB_INFO, > + "Unsupported event [0x%x]\n", event)); > + break; > + } > + > + return; > +} > + > +/* > * We keep the driver loaded even when ACPI is not running. > * This is needed for the powernow-k8 driver, that works even without > * ACPI, but needs symbols from this driver > @@ -1198,7 +1507,11 @@ static int __init acpi_processor_init(void) > if (result < 0) > goto out_proc; > > - result = acpi_bus_register_driver(&acpi_processor_driver); > + if (xen_initial_domain()) > + result = acpi_bus_register_driver(&xen_acpi_processor_driver); > + else > + result = acpi_bus_register_driver(&acpi_processor_driver); > + > if (result < 0) > goto out_cpuidle; > > @@ -1232,7 +1545,10 @@ static void __exit acpi_processor_exit(void) > > acpi_processor_uninstall_hotplug_notify(); > > - acpi_bus_unregister_driver(&acpi_processor_driver); > + if (xen_initial_domain()) > + acpi_bus_unregister_driver(&xen_acpi_processor_driver); > + else > + acpi_bus_unregister_driver(&acpi_processor_driver); > > cpuidle_unregister_driver(&acpi_idle_driver); > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 25680c6..1b382e2 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -1148,13 +1148,6 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) > if (!pr->flags.power_setup_done) > return -ENODEV; > > - if (xen_initial_domain()) { > - acpi_processor_get_power_info(pr); > - processor_cntl_xen_notify(pr, > - PROCESSOR_PM_CHANGE, PM_TYPE_IDLE); > - return ret; > - } > - > cpuidle_pause_and_lock(); > cpuidle_disable_device(&pr->power.dev); > acpi_processor_get_power_info(pr); > @@ -1167,6 +1160,44 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr) > return ret; > } > > +static int xen_acpi_processor_get_power_info(struct acpi_processor *pr) > +{ > + int ret; > + int invalid_pr_id = 0; > + > + /* > + * acpi_processor_get_power_info need valid pr->id > + * so set pr->id=0 temporarily > + */ > + if (pr->id == -1) { > + invalid_pr_id = 1; > + pr->id = 0; > + } > + > + ret = acpi_processor_get_power_info(pr); > + > + if (invalid_pr_id) > + pr->id = -1; > + > + return ret; > +} > + > +int xen_acpi_processor_cst_has_changed(struct acpi_processor *pr) > +{ > + if (!pr) > + return -EINVAL; > + > + if (!pr->flags.power_setup_done) > + return -ENODEV; > + > + xen_acpi_processor_get_power_info(pr); > + > + processor_cntl_xen_notify(pr, > + PROCESSOR_PM_CHANGE, PM_TYPE_IDLE); > + > + return 0; > +} > + > int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, > struct acpi_device *device) > { > @@ -1245,6 +1276,55 @@ int __cpuinit acpi_processor_power_init(struct acpi_processor *pr, > return 0; > } > > +int __cpuinit xen_acpi_processor_power_init(struct acpi_processor *pr, > + struct acpi_device *device) > +{ > + acpi_status status = 0; > + struct proc_dir_entry *entry = NULL; > + unsigned int i; > + > + if (!pr) > + return -EINVAL; > + > + if (acpi_gbl_FADT.cst_control) { > + status = acpi_os_write_port(acpi_gbl_FADT.smi_command, > + acpi_gbl_FADT.cst_control, 8); > + if (ACPI_FAILURE(status)) { > + ACPI_EXCEPTION((AE_INFO, status, > + "Notifying BIOS of _CST ability failed")); > + } > + } > + > + xen_acpi_processor_get_power_info(pr); > + > + pr->flags.power_setup_done = 1; > + > + if (pr->flags.power) { > + processor_cntl_xen_notify(pr, > + PROCESSOR_PM_INIT, PM_TYPE_IDLE); > + > + printk(KERN_INFO PREFIX "CPU%d (power states:", pr->id); > + for (i = 1; i <= pr->power.count; i++) > + if (pr->power.states[i].valid) > + printk(" C%d[C%d]", i, > + pr->power.states[i].type); > + printk(")\n"); > + } > + > + if (pr->id != -1) { > + /* ''power'' [R] > + */ > + entry = proc_create_data(ACPI_PROCESSOR_FILE_POWER, > + S_IRUGO, acpi_device_dir(device), > + &acpi_processor_power_fops, > + acpi_driver_data(device)); > + if (!entry) > + return -EIO; > + } > + > + return 0; > +} > + > int acpi_processor_power_exit(struct acpi_processor *pr, > struct acpi_device *device) > { > diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c > index 8375075..f17e740 100644 > --- a/drivers/acpi/processor_perflib.c > +++ b/drivers/acpi/processor_perflib.c > @@ -155,20 +155,33 @@ int acpi_processor_ppc_has_changed(struct acpi_processor *pr) > { > int ret; > > - if (ignore_ppc && !processor_cntl_xen_pmperf()) > + if (ignore_ppc) > return 0; > > ret = acpi_processor_get_platform_limit(pr); > > if (ret < 0) > return (ret); > - else if (processor_cntl_xen_pmperf()) > - return processor_cntl_xen_notify(pr, > - PROCESSOR_PM_CHANGE, PM_TYPE_PERF); > else > return cpufreq_update_policy(pr->id); > } > > +int xen_acpi_processor_ppc_has_changed(struct acpi_processor *pr) > +{ > + int ret; > + > + if (!processor_cntl_xen_pmperf()) > + return 0; > + > + ret = acpi_processor_get_platform_limit(pr); > + > + if (ret < 0) > + return ret; > + else > + return processor_cntl_xen_notify(pr, > + PROCESSOR_PM_CHANGE, PM_TYPE_PERF); > +} > + > void acpi_processor_ppc_init(void) > { > if (!cpufreq_register_notifier > diff --git a/include/acpi/processor.h b/include/acpi/processor.h > index 221d30d..c806ecf 100644 > --- a/include/acpi/processor.h > +++ b/include/acpi/processor.h > @@ -259,6 +259,7 @@ extern struct acpi_processor_errata errata; > > void arch_acpi_processor_init_pdc(struct acpi_processor *pr); > void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr); > +void xen_arch_acpi_processor_init_pdc(struct acpi_processor *pr); > > #ifdef ARCH_HAS_POWER_INIT > void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags, > @@ -297,6 +298,7 @@ void acpi_processor_ppc_exit(void); > int acpi_processor_ppc_has_changed(struct acpi_processor *pr); > int acpi_processor_get_performance_info(struct acpi_processor *pr); > int acpi_processor_get_psd(struct acpi_processor *pr); > +int xen_acpi_processor_ppc_has_changed(struct acpi_processor *pr); > #else > static inline void acpi_processor_ppc_init(void) > { > @@ -318,6 +320,10 @@ static inline int acpi_processor_ppc_has_changed(struct acpi_processor *pr) > } > return 0; > } > +static inline int xen_acpi_processor_ppc_has_changed(struct acpi_processor *pr) > +{ > + return acpi_processor_ppc_has_changed(pr); > +} > #endif /* CONFIG_CPU_FREQ */ > > /* in processor_throttling.c */ > @@ -330,7 +336,10 @@ extern void acpi_processor_throttling_init(void); > /* in processor_idle.c */ > int acpi_processor_power_init(struct acpi_processor *pr, > struct acpi_device *device); > +int xen_acpi_processor_power_init(struct acpi_processor *pr, > + struct acpi_device *device); > int acpi_processor_cst_has_changed(struct acpi_processor *pr); > +int xen_acpi_processor_cst_has_changed(struct acpi_processor *pr); > int acpi_processor_power_exit(struct acpi_processor *pr, > struct acpi_device *device); > int acpi_processor_suspend(struct acpi_device * device, pm_message_t state); >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu, Ke
2009-Nov-26 08:29 UTC
[Xen-devel] RE: [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup
>Thanks, this looks like a big improvement. I have some comments about >the details though: > > * Split this into 3 patches: > o revert the old Xen-specific changes > o add the new common code (which may just be > is_processor_apic_enabled()) > o add the new Xen-specific code in a new fileThanks for the comments. I split the original patch into three patch as you suggested. However, after it is done, I notice the original patch has already been checked in. so I just attach the three patch for your information, in case you still need that.> * Make acpi_processor_driver a pointer to the preferred driver > structure which defaults to the normal driver, and have some Xen > code re-point it to the Xen one during Xen setup, rather than > having an explicit Xen test in the core ACPI codeThis approach will introduce another dependency between xen setup code and acpi processor driver module, and again cannot pass compiling when CONFIG_ACPI_PROCESSOR=m. so I would suggest to keep it as it is. How do you think? Best Regards Ke _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-Nov-26 08:44 UTC
[Xen-devel] Re: [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup
On 11/26/09 00:29, Yu, Ke wrote:>> * Split this into 3 patches: >> o revert the old Xen-specific changes >> o add the new common code (which may just be >> is_processor_apic_enabled()) >> o add the new Xen-specific code in a new file >> > Thanks for the comments. I split the original patch into three patch as you suggested. However, after it is done, I notice the original patch has already been checked in. so I just attach the three patch for your information, in case you still need that. >Yes, that''s why I''d like to see the revert patch separate. Later on we can fold together the patches to clean them up, but for now lots of incremental delta patches is best. And if we can avoid mixing Xen and non-Xen changes into the one file, then its easier for subsystem maintainers to see what we''re doing to their code without getting stuck in the details of the Xen-specific parts. And can we split all the Xen-specific code into a new file? I don''t want it mixed in with the generic ACPI code.>> * Make acpi_processor_driver a pointer to the preferred driver >> structure which defaults to the normal driver, and have some Xen >> code re-point it to the Xen one during Xen setup, rather than >> having an explicit Xen test in the core ACPI code >> > This approach will introduce another dependency between xen setup code and acpi processor driver module, and again cannot pass compiling when CONFIG_ACPI_PROCESSOR=m. so I would suggest to keep it as it is. How do you think? >Well, if ACPI_PROCESSOR=m then the corresponding Xen code would also need to be modular. But aside from that, it would work OK, wouldn''t it? Rather than exposing the variable directly, it might be better to wrap it with acpi_set_processor_driver() or something. Or am I missing something? J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yu, Ke
2009-Nov-27 02:23 UTC
[Xen-devel] RE: [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup
>-----Original Message----- >From: Jeremy Fitzhardinge [mailto:jeremy@goop.org] >Sent: Thursday, November 26, 2009 4:45 PM >To: Yu, Ke >Cc: ''xen-devel@lists.xensource.com''; Brown, Len >Subject: Re: [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup > >On 11/26/09 00:29, Yu, Ke wrote: >>> * Split this into 3 patches: >>> o revert the old Xen-specific changes >>> o add the new common code (which may just be >>> is_processor_apic_enabled()) >>> o add the new Xen-specific code in a new file >>> >> Thanks for the comments. I split the original patch into three patch as you >suggested. However, after it is done, I notice the original patch has already been >checked in. so I just attach the three patch for your information, in case you still >need that. >> > >Yes, that''s why I''d like to see the revert patch separate. Later on we >can fold together the patches to clean them up, but for now lots of >incremental delta patches is best. And if we can avoid mixing Xen and >non-Xen changes into the one file, then its easier for subsystem >maintainers to see what we''re doing to their code without getting stuck >in the details of the Xen-specific parts.Yes, this is more easy for maintenance. Will follow this way in later patches.> >And can we split all the Xen-specific code into a new file? I don''t >want it mixed in with the generic ACPI code.Yes. This is what I plan to do next step.> >>> * Make acpi_processor_driver a pointer to the preferred driver >>> structure which defaults to the normal driver, and have some Xen >>> code re-point it to the Xen one during Xen setup, rather than >>> having an explicit Xen test in the core ACPI code >>> >> This approach will introduce another dependency between xen setup code and >acpi processor driver module, and again cannot pass compiling when >CONFIG_ACPI_PROCESSOR=m. so I would suggest to keep it as it is. How do you >think? >> > >Well, if ACPI_PROCESSOR=m then the corresponding Xen code would also >need to be modular. But aside from that, it would work OK, wouldn''t >it? Rather than exposing the variable directly, it might be better to >wrap it with acpi_set_processor_driver() or something. Or am I missing >something? > > JI tried making xen code modular before, the issue here is the two-way dependency among acpi processor driver (driver/acpi/processor_core.c) and xen driver (driver/xen/acpi_processor.c). firstly, acpi processor driver depend on xen driver routine processor_cntl_xen_notify(). Secondly, if xen driver depend on some routine of acpi processor driver (e.g. acpi_set_processor_driver as you mentioned), then there is two way dependency between these two modules, which cannot pass compilation. so what I have done in last patch is moving related code from xen part to acpi processor driver part, to eliminate the xen->acpi processor dependency. Best Regards Ke _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jeremy Fitzhardinge
2009-Nov-30 20:05 UTC
[Xen-devel] Re: [PATCH][1/2] pvops-dom0: Xen acpi processor logic cleanup
On 11/26/09 18:23, Yu, Ke wrote:> I tried making xen code modular before, the issue here is the two-way dependency among acpi processor driver (driver/acpi/processor_core.c) and xen driver (driver/xen/acpi_processor.c). firstly, acpi processor driver depend on xen driver routine processor_cntl_xen_notify(). Secondly, if xen driver depend on some routine of acpi processor driver (e.g. acpi_set_processor_driver as you mentioned), then there is two way dependency between these two modules, which cannot pass compilation. so what I have done in last patch is moving related code from xen part to acpi processor driver part, to eliminate the xen->acpi processor dependency.Well, you can either make it a looser coupling by making the acpi->xen call via a function pointer, or by using Kconfig to make sure that the Xen code and the ACPI code are always compiled in a compatible way (ie, both builtin or both modules). J _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel