[PATCH 1/2] RFC: Prepare PAD for native and xen platform

[PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 4018 bytes --]

>From 9a12d7c610bffb900015e8947a57e4e428058abf Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Fri, 24 Feb 2012 01:51:18 +0800
Subject: [PATCH 1/2] Prepare PAD for native and xen platform

This patch is to prepare PAD (Processor Aggregator Device) for native and xen platform.
When working under baremetal it initializes native acpi_pad, while working under
xen platform it would hook to xen acpi_pad (would implement at later patch).

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
---
 drivers/acpi/Kconfig        |    3 ++-
 drivers/acpi/acpi_pad.c     |   21 +++++++++------------
 include/acpi/acpi_drivers.h |   15 +++++++++++++++
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7556913..2653a98 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
 	default y
 
 config ACPI_PROCESSOR_AGGREGATOR
-	tristate "Processor Aggregator"
+	bool "Processor Aggregator"
 	depends on ACPI_PROCESSOR
 	depends on EXPERIMENTAL
 	depends on X86
+	default n
 	help
 	  ACPI 4.0 defines processor Aggregator, which enables OS to perform
 	  specific processor configuration and control that applies to all
diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index a43fa1a..8aebbe5 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -2,6 +2,7 @@
  * acpi_pad.c ACPI Processor Aggregator Driver
  *
  * Copyright (c) 2009, Intel Corporation.
+ *    Author: Li, Shaohua <shaohua.li@intel.com>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -32,9 +33,6 @@
 #include <acpi/acpi_drivers.h>
 #include <asm/mwait.h>
 
-#define ACPI_PROCESSOR_AGGREGATOR_CLASS	"acpi_pad"
-#define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
-#define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
 static DEFINE_MUTEX(isolated_cpus_lock);
 
 static unsigned long power_saving_mwait_eax;
@@ -510,7 +508,7 @@ static struct acpi_driver acpi_pad_driver = {
 	},
 };
 
-static int __init acpi_pad_init(void)
+static int __init native_acpi_pad_init(void)
 {
 	power_saving_mwait_init();
 	if (power_saving_mwait_eax == 0)
@@ -519,13 +517,12 @@ static int __init acpi_pad_init(void)
 	return acpi_bus_register_driver(&acpi_pad_driver);
 }
 
-static void __exit acpi_pad_exit(void)
+struct acpi_pad_ops acpi_pad_ops = {
+	.init = native_acpi_pad_init,
+};
+
+static int __init acpi_pad_init(void)
 {
-	acpi_bus_unregister_driver(&acpi_pad_driver);
+	return acpi_pad_ops.init();
 }
-
-module_init(acpi_pad_init);
-module_exit(acpi_pad_exit);
-MODULE_AUTHOR("Shaohua Li<shaohua.li@intel.com>");
-MODULE_DESCRIPTION("ACPI Processor Aggregator Driver");
-MODULE_LICENSE("GPL");
+device_initcall(acpi_pad_init);
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index bb145e4..d40abbc 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -115,6 +115,21 @@ void pci_acpi_crs_quirks(void);
 #define ACPI_PROCESSOR_LIMIT_INCREMENT	0x01
 #define ACPI_PROCESSOR_LIMIT_DECREMENT	0x02
 
+/* --------------------------------------------------------------------------
+			PAD (Processor Aggregator Device)
+   -------------------------------------------------------------------------- */
+
+#define ACPI_PROCESSOR_AGGREGATOR_CLASS	"acpi_pad"
+#define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
+#define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
+
+struct acpi_pad_ops {
+	int (*init)(void);
+};
+#ifdef CONFIG_ACPI_PROCESSOR_AGGREGATOR
+extern struct acpi_pad_ops acpi_pad_ops;
+#endif
+
 /*--------------------------------------------------------------------------
                                   Dock Station
   -------------------------------------------------------------------------- */
-- 
1.7.1

[-- Attachment #2: 0001-Prepare-PAD-for-native-and-xen-platform.patch --]
[-- Type: application/octet-stream, Size: 3905 bytes --]

From 9a12d7c610bffb900015e8947a57e4e428058abf Mon Sep 17 00:00:00 2001
From: Liu, Jinsong <jinsong.liu@intel.com>
Date: Fri, 24 Feb 2012 01:51:18 +0800
Subject: [PATCH 1/2] Prepare PAD for native and xen platform

This patch is to prepare PAD (Processor Aggregator Device) for native and xen platform.
When working under baremetal it initializes native acpi_pad, while working under
xen platform it would hook to xen acpi_pad (would implement at later patch).

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
---
 drivers/acpi/Kconfig        |    3 ++-
 drivers/acpi/acpi_pad.c     |   21 +++++++++------------
 include/acpi/acpi_drivers.h |   15 +++++++++++++++
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 7556913..2653a98 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
 	default y
 
 config ACPI_PROCESSOR_AGGREGATOR
-	tristate "Processor Aggregator"
+	bool "Processor Aggregator"
 	depends on ACPI_PROCESSOR
 	depends on EXPERIMENTAL
 	depends on X86
+	default n
 	help
 	  ACPI 4.0 defines processor Aggregator, which enables OS to perform
 	  specific processor configuration and control that applies to all
diff --git a/drivers/acpi/acpi_pad.c b/drivers/acpi/acpi_pad.c
index a43fa1a..8aebbe5 100644
--- a/drivers/acpi/acpi_pad.c
+++ b/drivers/acpi/acpi_pad.c
@@ -2,6 +2,7 @@
  * acpi_pad.c ACPI Processor Aggregator Driver
  *
  * Copyright (c) 2009, Intel Corporation.
+ *    Author: Li, Shaohua <shaohua.li@intel.com>
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -32,9 +33,6 @@
 #include <acpi/acpi_drivers.h>
 #include <asm/mwait.h>
 
-#define ACPI_PROCESSOR_AGGREGATOR_CLASS	"acpi_pad"
-#define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
-#define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
 static DEFINE_MUTEX(isolated_cpus_lock);
 
 static unsigned long power_saving_mwait_eax;
@@ -510,7 +508,7 @@ static struct acpi_driver acpi_pad_driver = {
 	},
 };
 
-static int __init acpi_pad_init(void)
+static int __init native_acpi_pad_init(void)
 {
 	power_saving_mwait_init();
 	if (power_saving_mwait_eax == 0)
@@ -519,13 +517,12 @@ static int __init acpi_pad_init(void)
 	return acpi_bus_register_driver(&acpi_pad_driver);
 }
 
-static void __exit acpi_pad_exit(void)
+struct acpi_pad_ops acpi_pad_ops = {
+	.init = native_acpi_pad_init,
+};
+
+static int __init acpi_pad_init(void)
 {
-	acpi_bus_unregister_driver(&acpi_pad_driver);
+	return acpi_pad_ops.init();
 }
-
-module_init(acpi_pad_init);
-module_exit(acpi_pad_exit);
-MODULE_AUTHOR("Shaohua Li<shaohua.li@intel.com>");
-MODULE_DESCRIPTION("ACPI Processor Aggregator Driver");
-MODULE_LICENSE("GPL");
+device_initcall(acpi_pad_init);
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index bb145e4..d40abbc 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -115,6 +115,21 @@ void pci_acpi_crs_quirks(void);
 #define ACPI_PROCESSOR_LIMIT_INCREMENT	0x01
 #define ACPI_PROCESSOR_LIMIT_DECREMENT	0x02
 
+/* --------------------------------------------------------------------------
+			PAD (Processor Aggregator Device)
+   -------------------------------------------------------------------------- */
+
+#define ACPI_PROCESSOR_AGGREGATOR_CLASS	"acpi_pad"
+#define ACPI_PROCESSOR_AGGREGATOR_DEVICE_NAME "Processor Aggregator"
+#define ACPI_PROCESSOR_AGGREGATOR_NOTIFY 0x80
+
+struct acpi_pad_ops {
+	int (*init)(void);
+};
+#ifdef CONFIG_ACPI_PROCESSOR_AGGREGATOR
+extern struct acpi_pad_ops acpi_pad_ops;
+#endif
+
 /*--------------------------------------------------------------------------
                                   Dock Station
   -------------------------------------------------------------------------- */
-- 
1.7.1

Re: [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Jan Beulich]

>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>--- a/drivers/acpi/Kconfig
>+++ b/drivers/acpi/Kconfig
>@@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
>    default y
 >
>config ACPI_PROCESSOR_AGGREGATOR
>-    tristate "Processor Aggregator"
>+    bool "Processor Aggregator"

There must be ways to address whatever strange problem you see without
making this piece of code non-modular.

>    depends on ACPI_PROCESSOR
>    depends on EXPERIMENTAL
>    depends on X86
>+    default n

This is pointless.

>    help
>      ACPI 4.0 defines processor Aggregator, which enables OS to perform
>      specific processor configuration and control that applies to all

Jan

RE: [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Jan Beulich wrote:
>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
>>    default y
>  >
>> config ACPI_PROCESSOR_AGGREGATOR
>> -    tristate "Processor Aggregator"
>> +    bool "Processor Aggregator"
> 
> There must be ways to address whatever strange problem you see without
> making this piece of code non-modular.
> 

Yes, another approach is x86_init approach, defining acpi_pad_ops at x86_init.c and overwritten when xen_start_kernel.
This patch is just a RFC patch, to evaluate which approch is more reasonable :-)

>>    depends on ACPI_PROCESSOR
>>    depends on EXPERIMENTAL
>>    depends on X86
>> +    default n
> 
> This is pointless.

Elaborate more? just a little curious why Kconfig has so many default n.

> 
>>    help
>>      ACPI 4.0 defines processor Aggregator, which enables OS to
>>      perform specific processor configuration and control that
>> applies to all 
> 
> Jan

RE: [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Jan Beulich]

>>> On 23.02.12 at 17:58, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> Jan Beulich wrote:
>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>>>    depends on ACPI_PROCESSOR
>>>    depends on EXPERIMENTAL
>>>    depends on X86
>>> +    default n
>> 
>> This is pointless.
> 
> Elaborate more? just a little curious why Kconfig has so many default n.

Which are all pointless afaict (and I did already get a couple of
patches merged to remove some of them) - the tool defaults
them to 'n' anyway.

Jan

RE: [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Liu, Jinsong wrote:
> Jan Beulich wrote:
>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
>>>    default y
>>  >
>>> config ACPI_PROCESSOR_AGGREGATOR
>>> -    tristate "Processor Aggregator"
>>> +    bool "Processor Aggregator"
>> 
>> There must be ways to address whatever strange problem you see
>> without making this piece of code non-modular.
>> 
> 
> Yes, another approach is x86_init approach, defining acpi_pad_ops at
> x86_init.c and overwritten when xen_start_kernel. This patch is just
> a RFC patch, to evaluate which approch is more reasonable :-) 
> 

Have a more think about it, x86_init approach still need to disable acpi_pad module.
Seems we have to set acpi_pad as bool, as long as it need to hook to native acpi_pad fucs/variables.

Thanks,
Jinsong

Re: [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Konrad Rzeszutek Wilk]

On Sun, Feb 26, 2012 at 08:25:41AM +0000, Liu, Jinsong wrote:
> Liu, Jinsong wrote:
> > Jan Beulich wrote:
> >>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
> >>> --- a/drivers/acpi/Kconfig
> >>> +++ b/drivers/acpi/Kconfig
> >>> @@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
> >>>    default y
> >>  >
> >>> config ACPI_PROCESSOR_AGGREGATOR
> >>> -    tristate "Processor Aggregator"
> >>> +    bool "Processor Aggregator"
> >> 
> >> There must be ways to address whatever strange problem you see
> >> without making this piece of code non-modular.
> >> 
> > 
> > Yes, another approach is x86_init approach, defining acpi_pad_ops at
> > x86_init.c and overwritten when xen_start_kernel. This patch is just
> > a RFC patch, to evaluate which approch is more reasonable :-) 
> > 
> 
> Have a more think about it, x86_init approach still need to disable acpi_pad module.
> Seems we have to set acpi_pad as bool, as long as it need to hook to native acpi_pad fucs/variables.

What about the other approach I suggested where there are some function
overrides in osl.c? Something similar to https://lkml.org/lkml/2012/1/17/401,
specifically https://lkml.org/lkml/2012/1/17/403 - that way you are not turning
the modules into being built in, but intead have the function table already in
the kernel (as some form of EXPORT_SYMBOL_GPL or a registration function).

Instead of just one function being over-ridden it could have some more. However
I am not sure if the osl.c is the place for this either. Perhaps Len might
have some better ideas?

RE: [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Konrad Rzeszutek Wilk wrote:
> On Sun, Feb 26, 2012 at 08:25:41AM +0000, Liu, Jinsong wrote:
>> Liu, Jinsong wrote:
>>> Jan Beulich wrote:
>>>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>>>>> --- a/drivers/acpi/Kconfig
>>>>> +++ b/drivers/acpi/Kconfig
>>>>> @@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
>>>>>    default y
>>>>  >
>>>>> config ACPI_PROCESSOR_AGGREGATOR
>>>>> -    tristate "Processor Aggregator"
>>>>> +    bool "Processor Aggregator"
>>>> 
>>>> There must be ways to address whatever strange problem you see
>>>> without making this piece of code non-modular.
>>>> 
>>> 
>>> Yes, another approach is x86_init approach, defining acpi_pad_ops at
>>> x86_init.c and overwritten when xen_start_kernel. This patch is just
>>> a RFC patch, to evaluate which approch is more reasonable :-)
>>> 
>> 
>> Have a more think about it, x86_init approach still need to disable
>> acpi_pad module. 
>> Seems we have to set acpi_pad as bool, as long as it need to hook to
>> native acpi_pad fucs/variables. 
> 
> What about the other approach I suggested where there are some
> function overrides in osl.c? Something similar to
> https://lkml.org/lkml/2012/1/17/401, specifically
> https://lkml.org/lkml/2012/1/17/403 - that way you are not turning
> the modules into being built in, but intead have the function table
> already in the kernel (as some form of EXPORT_SYMBOL_GPL or a
> registration function). 
> 

Thanks for the example (it's good itself :-), but per my understanding they are different cases.

In the osl example case, tboot_late_init call acpi_os_set_prepare_sleep to register func, so it works no matter tboot.c built as y/n/m (through currently it's bool).

However, in our case, we just cannot do so. We need
1. predefine a hook to native acpi pad funcs, take effect when static compile stage;
2. when xen init, redirect the hook to xen acpi pad funcs, take effect at running time;
(The point is, we cannot do init twice for native and xen acpi pad, so we have to statically predefine a hook to native acpi_pad)

For the reason above, I think we have to remove acpi_pad module, as long as we need to hook to native acpi_pad funcs. Thoughts?

Regards,
Jinsong

RE: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
>> On Sun, Feb 26, 2012 at 08:25:41AM +0000, Liu, Jinsong wrote:
>>> Liu, Jinsong wrote:
>>>> Jan Beulich wrote:
>>>>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>>>>>> --- a/drivers/acpi/Kconfig
>>>>>> +++ b/drivers/acpi/Kconfig
>>>>>> @@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
>>>>>>    default y
>>>>>  >
>>>>>> config ACPI_PROCESSOR_AGGREGATOR
>>>>>> -    tristate "Processor Aggregator"
>>>>>> +    bool "Processor Aggregator"
>>>>> 
>>>>> There must be ways to address whatever strange problem you see
>>>>> without making this piece of code non-modular.
>>>>> 
>>>> 
>>>> Yes, another approach is x86_init approach, defining acpi_pad_ops
>>>> at x86_init.c and overwritten when xen_start_kernel. This patch is
>>>> just a RFC patch, to evaluate which approch is more reasonable :-)
>>>> 
>>> 
>>> Have a more think about it, x86_init approach still need to disable
>>> acpi_pad module. Seems we have to set acpi_pad as bool, as long as
>>> it need to hook to native acpi_pad fucs/variables.
>> 
>> What about the other approach I suggested where there are some
>> function overrides in osl.c? Something similar to
>> https://lkml.org/lkml/2012/1/17/401, specifically
>> https://lkml.org/lkml/2012/1/17/403 - that way you are not turning
>> the modules into being built in, but intead have the function table
>> already in the kernel (as some form of EXPORT_SYMBOL_GPL or a
>> registration function). 
>> 
> 
> Thanks for the example (it's good itself :-), but per my
> understanding they are different cases. 
> 
> In the osl example case, tboot_late_init call
> acpi_os_set_prepare_sleep to register func, so it works no matter
> tboot.c built as y/n/m (through currently it's bool).  
> 
> However, in our case, we just cannot do so. We need
> 1. predefine a hook to native acpi pad funcs, take effect when static
> compile stage; 
> 2. when xen init, redirect the hook to xen acpi pad funcs, take
> effect at running time; (The point is, we cannot do init twice for
> native and xen acpi pad, so we have to statically predefine a hook to
> native acpi_pad)  
> 
> For the reason above, I think we have to remove acpi_pad module, as
> long as we need to hook to native acpi_pad funcs. Thoughts? 
> 

Compare approaches:

1. xen overwritten approach (patches V2, x86_init, osl approach)
    Pros:
        a little simpler code
    Cons:
        1). specific to xen, cannot extend to other virt platform;
        2). need to change natvie acpi_pad as modular;

2. paravirt interface approach (original patches V1)
    Pros:
        1). standard hypervisor-agnostic interface (USENIX conference 2006), can easily hook to Xen/lguest/... on demand;
        2). arch independent;
        3). no need to change native acpi_pad as non-modular;
    Cons:
        a little complicated code, and code patching is some overkilled for this case (but no harm);

(BTW, in the future we need add more and more pv ops, like pv_pm_ops, pv_cpu_hotplug_ops, pv_mem_hotplug_ops, etc. So how about add a pv_misc_ops template to handle them all? seems it's a common issue).

Your opinion?

Thanks,
Jinsong

RE: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Liu, Jinsong wrote:
> Liu, Jinsong wrote:
>> Konrad Rzeszutek Wilk wrote:
>>> On Sun, Feb 26, 2012 at 08:25:41AM +0000, Liu, Jinsong wrote:
>>>> Liu, Jinsong wrote:
>>>>> Jan Beulich wrote:
>>>>>>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 02/23/12 2:29 PM >>>
>>>>>>> --- a/drivers/acpi/Kconfig
>>>>>>> +++ b/drivers/acpi/Kconfig
>>>>>>> @@ -213,10 +213,11 @@ config ACPI_HOTPLUG_CPU
>>>>>>>    default y
>>>>>>  >
>>>>>>> config ACPI_PROCESSOR_AGGREGATOR
>>>>>>> -    tristate "Processor Aggregator"
>>>>>>> +    bool "Processor Aggregator"
>>>>>> 
>>>>>> There must be ways to address whatever strange problem you see
>>>>>> without making this piece of code non-modular.
>>>>>> 
>>>>> 
>>>>> Yes, another approach is x86_init approach, defining acpi_pad_ops
>>>>> at x86_init.c and overwritten when xen_start_kernel. This patch is
>>>>> just a RFC patch, to evaluate which approch is more reasonable :-)
>>>>> 
>>>> 
>>>> Have a more think about it, x86_init approach still need to disable
>>>> acpi_pad module. Seems we have to set acpi_pad as bool, as long as
>>>> it need to hook to native acpi_pad fucs/variables.
>>> 
>>> What about the other approach I suggested where there are some
>>> function overrides in osl.c? Something similar to
>>> https://lkml.org/lkml/2012/1/17/401, specifically
>>> https://lkml.org/lkml/2012/1/17/403 - that way you are not turning
>>> the modules into being built in, but intead have the function table
>>> already in the kernel (as some form of EXPORT_SYMBOL_GPL or a
>>> registration function). 
>>> 
>> 
>> Thanks for the example (it's good itself :-), but per my
>> understanding they are different cases.
>> 
>> In the osl example case, tboot_late_init call
>> acpi_os_set_prepare_sleep to register func, so it works no matter
>> tboot.c built as y/n/m (through currently it's bool).
>> 
>> However, in our case, we just cannot do so. We need
>> 1. predefine a hook to native acpi pad funcs, take effect when
>> static compile stage; 
>> 2. when xen init, redirect the hook to xen acpi pad funcs, take
>> effect at running time; (The point is, we cannot do init twice for
>> native and xen acpi pad, so we have to statically predefine a hook
>> to native acpi_pad) 
>> 
>> For the reason above, I think we have to remove acpi_pad module, as
>> long as we need to hook to native acpi_pad funcs. Thoughts?
>> 
> 
> Compare approaches:
> 
> 1. xen overwritten approach (patches V2, x86_init, osl approach)
>     Pros:
>         a little simpler code
>     Cons:
>         1). specific to xen, cannot extend to other virt platform;
>         2). need to change natvie acpi_pad as modular;
> 
> 2. paravirt interface approach (original patches V1)
>     Pros:
>         1). standard hypervisor-agnostic interface (USENIX conference
>         2006), can easily hook to Xen/lguest/... on demand; 2). arch
>         independent; 3). no need to change native acpi_pad as
>     non-modular; Cons:
>         a little complicated code, and code patching is some
> overkilled for this case (but no harm); 
> 
> (BTW, in the future we need add more and more pv ops, like pv_pm_ops,
> pv_cpu_hotplug_ops, pv_mem_hotplug_ops, etc. So how about add a
> pv_misc_ops template to handle them all? seems it's a common issue).  
> 
> Your opinion?
> 

Konrad,

Comments? we need make sure which approach we choose (patches for both approaches are basically done and tested). IMO I prefer paravirt interface approach (it need slightly update for the purpose of 'not change acpi_pad as non-modular'. If choose it I will update ASAP).

Thanks,
Jinsong

Re: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Konrad Rzeszutek Wilk]

> > Compare approaches:
> > 
> > 1. xen overwritten approach (patches V2, x86_init, osl approach)
> >     Pros:
> >         a little simpler code
> >     Cons:
> >         1). specific to xen, cannot extend to other virt platform;
> >         2). need to change natvie acpi_pad as modular;
> > 
> > 2. paravirt interface approach (original patches V1)
> >     Pros:
> >         1). standard hypervisor-agnostic interface (USENIX conference
> >         2006), can easily hook to Xen/lguest/... on demand; 2). arch
> >         independent; 3). no need to change native acpi_pad as
> >     non-modular; Cons:
> >         a little complicated code, and code patching is some
> > overkilled for this case (but no harm); 
> > 
> > (BTW, in the future we need add more and more pv ops, like pv_pm_ops,
> > pv_cpu_hotplug_ops, pv_mem_hotplug_ops, etc. So how about add a
> > pv_misc_ops template to handle them all? seems it's a common issue).  
> > 

I think (and you probabaly have a better idea) is that the maintainer of 
drivers/acpi/* is not very open to adding in code that only benefits Xen.

If it benefits other architectures (say ARM) then adding in hooks there
(in osl for example) makes sense - but I am not sure if ARM has a form
of _PUR code/calls it needs to do.

So with that in mind, neither of those options seems proper - as all of them
depend on changing something in drivers/acpi/*.

I've one or two suggestions of what could be done to still make this
work, but I need you to first see what happens if the native acpi_pad
runs under Xen with the latest upstream code (along with three patches
that are in a BZ I pointed you too). 

RE: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Konrad Rzeszutek Wilk wrote:
>>> Compare approaches:
>>> 
>>> 1. xen overwritten approach (patches V2, x86_init, osl approach)   
>>>         Pros: a little simpler code
>>>     Cons:
>>>         1). specific to xen, cannot extend to other virt platform;
>>>         2). need to change natvie acpi_pad as modular;
>>> 
>>> 2. paravirt interface approach (original patches V1)     Pros:
>>>         1). standard hypervisor-agnostic interface (USENIX
>>>         conference 2006), can easily hook to Xen/lguest/... on
>>>         demand; 2). arch independent; 3). no need to change native
>>>         acpi_pad as     non-modular; Cons: a little complicated
>>> code, and code patching is some 
>>> overkilled for this case (but no harm);
>>> 
>>> (BTW, in the future we need add more and more pv ops, like
>>> pv_pm_ops, pv_cpu_hotplug_ops, pv_mem_hotplug_ops, etc. So how
>>> about add a pv_misc_ops template to handle them all? seems it's a
>>> common issue). 
>>> 
> 
> I think (and you probabaly have a better idea) is that the maintainer
> of drivers/acpi/* is not very open to adding in code that only
> benefits Xen. 

Take paravirt interface approach as example. We only change a little about native acpi_pad_init/acpi_pad_exit, intercept it and *implicitly* hook to native/paravirt platform (it didn't appear any 'xen' 'pv' word in native pad logic). This is what I can find out the least change to native pad logic, and it in fact benefits (extensiable) to all pv. If this is still not acceptable we have to find other way (but I'm not sure) :-)

> 
> If it benefits other architectures (say ARM) then adding in hooks
> there (in osl for example) makes sense - but I am not sure if ARM has
> a form 
> of _PUR code/calls it needs to do.
> 
> So with that in mind, neither of those options seems proper - as all
> of them depend on changing something in drivers/acpi/*.
> 
> I've one or two suggestions of what could be done to still make this
> work, but I need you to first see what happens if the native acpi_pad
> runs under Xen with the latest upstream code (along with three patches
> that are in a BZ I pointed you too).

Do you mean test the patch http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blobdiff;f=arch/x86/xen/enlighten.c;h=b132ade26f778f2cfec7c2d5c7b6db48afe424d5;hp=4172af8ceeb363d06912af15bf89e8508752b794;hb=d4c6fa73fe984e504d52f3d6bba291fd76fe49f7;hpb=aab008db8063364dc3c8ccf4981c21124866b395 ?

I also don't have proper machine to test native pad w/ _PUR :(
But if your idea based on exposing mwait, I worry it may bring other troubles as I replied another thread yesterday. We can discuss it.

Thanks,
Jinsong

RE: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
>>>> Compare approaches:
>>>> 
>>>> 1. xen overwritten approach (patches V2, x86_init, osl approach)
>>>>         Pros: a little simpler code
>>>>     Cons:
>>>>         1). specific to xen, cannot extend to other virt platform;
>>>>         2). need to change natvie acpi_pad as modular;
>>>> 
>>>> 2. paravirt interface approach (original patches V1)     Pros:
>>>>         1). standard hypervisor-agnostic interface (USENIX
>>>>         conference 2006), can easily hook to Xen/lguest/... on
>>>>         demand; 2). arch independent; 3). no need to change native
>>>>         acpi_pad as     non-modular; Cons: a little complicated
>>>> code, and code patching is some
>>>> overkilled for this case (but no harm);
>>>> 
>>>> (BTW, in the future we need add more and more pv ops, like
>>>> pv_pm_ops, pv_cpu_hotplug_ops, pv_mem_hotplug_ops, etc. So how
>>>> about add a pv_misc_ops template to handle them all? seems it's a
>>>> common issue). 
>>>> 
>> 
>> I think (and you probabaly have a better idea) is that the maintainer
>> of drivers/acpi/* is not very open to adding in code that only
>> benefits Xen.
> 
> Take paravirt interface approach as example. We only change a little
> about native acpi_pad_init/acpi_pad_exit, intercept it and
> *implicitly* hook to native/paravirt platform (it didn't appear any
> 'xen' 'pv' word in native pad logic). This is what I can find out the
> least change to native pad logic, and it in fact benefits
> (extensiable) to all pv. If this is still not acceptable we have to
> find other way (but I'm not sure) :-)      
> 
>> 
>> If it benefits other architectures (say ARM) then adding in hooks
>> there (in osl for example) makes sense - but I am not sure if ARM
>> has a form of _PUR code/calls it needs to do.
>> 
>> So with that in mind, neither of those options seems proper - as all
>> of them depend on changing something in drivers/acpi/*.
>> 
>> I've one or two suggestions of what could be done to still make this
>> work, but I need you to first see what happens if the native acpi_pad
>> runs under Xen with the latest upstream code (along with three
>> patches that are in a BZ I pointed you too).
> 
> Do you mean test the patch
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blobdiff;f=arch/x86/xen/enlighten.c;h=b132ade26f778f2cfec7c2d5c7b6db48afe424d5;hp=4172af8ceeb363d06912af15bf89e8508752b794;hb=d4c6fa73fe984e504d52f3d6bba291fd76fe49f7;hpb=aab008db8063364dc3c8ccf4981c21124866b395
> ?  

Ah, you want to test https://bugzilla.redhat.com/show_bug.cgi?id=804347
Anyway, I didn't have proper h/w platform, but seems the bug (ioapic) is irrelated to pad thread we are talking?

Thanks,
Jinsong

Re: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Konrad Rzeszutek Wilk]

> >> If it benefits other architectures (say ARM) then adding in hooks
> >> there (in osl for example) makes sense - but I am not sure if ARM
> >> has a form of _PUR code/calls it needs to do.
> >> 
> >> So with that in mind, neither of those options seems proper - as all
> >> of them depend on changing something in drivers/acpi/*.
> >> 
> >> I've one or two suggestions of what could be done to still make this
> >> work, but I need you to first see what happens if the native acpi_pad
> >> runs under Xen with the latest upstream code (along with three
> >> patches that are in a BZ I pointed you too).
> > 
> > Do you mean test the patch
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blobdiff;f=arch/x86/xen/enlighten.c;h=b132ade26f778f2cfec7c2d5c7b6db48afe424d5;hp=4172af8ceeb363d06912af15bf89e8508752b794;hb=d4c6fa73fe984e504d52f3d6bba291fd76fe49f7;hpb=aab008db8063364dc3c8ccf4981c21124866b395

Right.
> > ?  
> 
> Ah, you want to test https://bugzilla.redhat.com/show_bug.cgi?id=804347

No. That is a fix on that is required to boot v3.4-rc0 - if you are using
that version.

> Anyway, I didn't have proper h/w platform, but seems the bug (ioapic) is irrelated to pad thread we are talking?

Correct (it is irrelevant).

RE: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Konrad Rzeszutek Wilk wrote:
>>>> If it benefits other architectures (say ARM) then adding in hooks
>>>> there (in osl for example) makes sense - but I am not sure if ARM
>>>> has a form of _PUR code/calls it needs to do.
>>>> 
>>>> So with that in mind, neither of those options seems proper - as
>>>> all of them depend on changing something in drivers/acpi/*.
>>>> 
>>>> I've one or two suggestions of what could be done to still make
>>>> this work, but I need you to first see what happens if the native
>>>> acpi_pad runs under Xen with the latest upstream code (along with
>>>> three patches that are in a BZ I pointed you too).
>>> 
>>> Do you mean test the patch
>>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blobdiff;f=arch/x86/xen/enlighten.c;h=b132ade26f778f2cfec7c2d5c7b6db48afe424d5;hp=4172af8ceeb363d06912af15bf89e8508752b794;hb=d4c6fa73fe984e504d52f3d6bba291fd76fe49f7;hpb=aab008db8063364dc3c8ccf4981c21124866b395
> 
> Right.

OK, I test by simulated method (I don't have platform w/ _PUR), executed __monitor at dom0 kernel.
In the test, it in fact no need to use platform w/ _PUR since it only care mwait, and __monitor would generated UD no matter xen hypervisor exposing mwait or not (break cpuid law or CPL law).

As expected, UD not handled by hypervisor, then bounce back to the created bounce frame of dom0, then

[   82.282152] invalid opcode: 0000 [#1] SMP ^M
[   82.282170] CPU 0 ^M
[   82.282175] Modules linked in: monitor(O+) xen_gntdev [last unloaded: scsi_wait_scan]^M
[   82.282196] ^M
[   82.282203] Pid: 5315, comm: insmod Tainted: G           O 3.3.0-rc3+ #22 Intel Corporation S2600CP/S2600CP^M
[   82.282222] RIP: e030:[<ffffffffa0005019>]  [<ffffffffa0005019>] init_module+0x19/0x20 [monitor]^M
[   82.282243] RSP: e02b:ffff8807a6357e68  EFLAGS: 00010246^M
[   82.282251] RAX: ffffffffa0005068 RBX: 0000000001ab4010 RCX: 0000000000000000^M
[   82.282261] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffa0005000^M
[   82.282271] RBP: ffff8807a6357e68 R08: ffff8807bdee2c60 R09: ffff8807b9802700^M
[   82.282280] R10: ffffffff8111bd9c R11: 0000000000000000 R12: 0000000000000000^M
[   82.282289] R13: ffffffffa0005000 R14: 0000000000000000 R15: ffff8807a6357ee8^M
[   82.282307] FS:  00007fb88c33c700(0000) GS:ffff8807bdecd000(0000) knlGS:0000000000000000^M
[   82.282318] CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b^M
[   82.282327] CR2: 00000035044d9380 CR3: 00000007a637d000 CR4: 0000000000042660^M
[   82.282370] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000^M
[   82.282389] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400^M
[   82.282394] Process insmod (pid: 5315, threadinfo ffff8807a6356000, task ffff8807b22aba80)^M
[   82.282399] Stack:^M
[   82.282402]  ffff8807a6357e98 ffffffff8100203d 0000000001ab4010 ffffffffa0005080^M
[   82.282411]  ffffffffa0007210 0000000000000000 ffff8807a6357f78 ffffffff8109f298^M
[   82.282420]  ffff8807b3815300 ffffc90013f29000 ffff880700000005 ffff880700000003^M
[   82.282429] Call Trace:^M
[   82.282440]  [<ffffffff8100203d>] do_one_initcall+0x3d/0x170^M
[   82.282450]  [<ffffffff8109f298>] sys_init_module+0x138/0x1720^M
[   82.282462]  [<ffffffff8174f039>] system_call_fastpath+0x16/0x1b^M
[   82.282466] Code: <0f> 01 c8 31 c0 c9 c3 55 48 c7 c7 58 50 00 a0 31 c0 48 89 e5 e8 44 ^M
[   82.282500] RIP  [<ffffffffa0005019>] init_module+0x19/0x20 [monitor]^M
[   82.282526]  RSP <ffff8807a6357e68>^M

Re: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Konrad Rzeszutek Wilk]

On Fri, Mar 30, 2012 at 07:05:13AM +0000, Liu, Jinsong wrote:
> Konrad Rzeszutek Wilk wrote:
> >>>> If it benefits other architectures (say ARM) then adding in hooks
> >>>> there (in osl for example) makes sense - but I am not sure if ARM
> >>>> has a form of _PUR code/calls it needs to do.
> >>>> 
> >>>> So with that in mind, neither of those options seems proper - as
> >>>> all of them depend on changing something in drivers/acpi/*.
> >>>> 
> >>>> I've one or two suggestions of what could be done to still make
> >>>> this work, but I need you to first see what happens if the native
> >>>> acpi_pad runs under Xen with the latest upstream code (along with
> >>>> three patches that are in a BZ I pointed you too).
> >>> 
> >>> Do you mean test the patch
> >>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blobdiff;f=arch/x86/xen/enlighten.c;h=b132ade26f778f2cfec7c2d5c7b6db48afe424d5;hp=4172af8ceeb363d06912af15bf89e8508752b794;hb=d4c6fa73fe984e504d52f3d6bba291fd76fe49f7;hpb=aab008db8063364dc3c8ccf4981c21124866b395
> > 
> > Right.
> 
> OK, I test by simulated method (I don't have platform w/ _PUR), executed __monitor at dom0 kernel.
> In the test, it in fact no need to use platform w/ _PUR since it only care mwait, and __monitor would generated UD no matter xen hypervisor exposing mwait or not (break cpuid law or CPL law).
> 
> As expected, UD not handled by hypervisor, then bounce back to the created bounce frame of dom0, then
> 
> [   82.282152] invalid opcode: 0000 [#1] SMP ^M
> [   82.282170] CPU 0 ^M
> [   82.282175] Modules linked in: monitor(O+) xen_gntdev [last unloaded: scsi_wait_scan]^M
> [   82.282196] ^M
> [   82.282203] Pid: 5315, comm: insmod Tainted: G           O 3.3.0-rc3+ #22 Intel Corporation S2600CP/S2600CP^M
> [   82.282222] RIP: e030:[<ffffffffa0005019>]  [<ffffffffa0005019>] init_module+0x19/0x20 [monitor]^M
> [   82.282243] RSP: e02b:ffff8807a6357e68  EFLAGS: 00010246^M
> [   82.282251] RAX: ffffffffa0005068 RBX: 0000000001ab4010 RCX: 0000000000000000^M
> [   82.282261] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffa0005000^M
> [   82.282271] RBP: ffff8807a6357e68 R08: ffff8807bdee2c60 R09: ffff8807b9802700^M
> [   82.282280] R10: ffffffff8111bd9c R11: 0000000000000000 R12: 0000000000000000^M
> [   82.282289] R13: ffffffffa0005000 R14: 0000000000000000 R15: ffff8807a6357ee8^M
> [   82.282307] FS:  00007fb88c33c700(0000) GS:ffff8807bdecd000(0000) knlGS:0000000000000000^M
> [   82.282318] CS:  e033 DS: 0000 ES: 0000 CR0: 000000008005003b^M
> [   82.282327] CR2: 00000035044d9380 CR3: 00000007a637d000 CR4: 0000000000042660^M
> [   82.282370] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000^M
> [   82.282389] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400^M
> [   82.282394] Process insmod (pid: 5315, threadinfo ffff8807a6356000, task ffff8807b22aba80)^M
> [   82.282399] Stack:^M
> [   82.282402]  ffff8807a6357e98 ffffffff8100203d 0000000001ab4010 ffffffffa0005080^M
> [   82.282411]  ffffffffa0007210 0000000000000000 ffff8807a6357f78 ffffffff8109f298^M
> [   82.282420]  ffff8807b3815300 ffffc90013f29000 ffff880700000005 ffff880700000003^M
> [   82.282429] Call Trace:^M
> [   82.282440]  [<ffffffff8100203d>] do_one_initcall+0x3d/0x170^M
> [   82.282450]  [<ffffffff8109f298>] sys_init_module+0x138/0x1720^M
> [   82.282462]  [<ffffffff8174f039>] system_call_fastpath+0x16/0x1b^M
> [   82.282466] Code: <0f> 01 c8 31 c0 c9 c3 55 48 c7 c7 58 50 00 a0 31 c0 48 89 e5 e8 44 ^M
> [   82.282500] RIP  [<ffffffffa0005019>] init_module+0x19/0x20 [monitor]^M
> [   82.282526]  RSP <ffff8807a6357e68>^M


Eww. Thanks for testing it out!
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

RE: [Xen-devel] [PATCH 1/2] RFC: Prepare PAD for native and xen platform

[Liu, Jinsong]

Konrad Rzeszutek Wilk wrote:
>>> Compare approaches:
>>> 
>>> 1. xen overwritten approach (patches V2, x86_init, osl approach)   
>>>         Pros: a little simpler code
>>>     Cons:
>>>         1). specific to xen, cannot extend to other virt platform;
>>>         2). need to change natvie acpi_pad as modular;
>>> 
>>> 2. paravirt interface approach (original patches V1)     Pros:
>>>         1). standard hypervisor-agnostic interface (USENIX
>>>         conference 2006), can easily hook to Xen/lguest/... on
>>>         demand; 2). arch independent; 3). no need to change native
>>>         acpi_pad as     non-modular; Cons: a little complicated
>>> code, and code patching is some 
>>> overkilled for this case (but no harm);
>>> 
>>> (BTW, in the future we need add more and more pv ops, like
>>> pv_pm_ops, pv_cpu_hotplug_ops, pv_mem_hotplug_ops, etc. So how
>>> about add a pv_misc_ops template to handle them all? seems it's a
>>> common issue). 
>>> 
> 
> I think (and you probabaly have a better idea) is that the maintainer
> of drivers/acpi/* is not very open to adding in code that only
> benefits Xen. 
> 
> If it benefits other architectures (say ARM) then adding in hooks
> there (in osl for example) makes sense - but I am not sure if ARM has
> a form 
> of _PUR code/calls it needs to do.
> 
> So with that in mind, neither of those options seems proper - as all
> of them depend on changing something in drivers/acpi/*.
> 
> I've one or two suggestions of what could be done to still make this
> work, but I need you to first see what happens if the native acpi_pad
> runs under Xen with the latest upstream code (along with three patches
> that are in a BZ I pointed you too).

Konrad, any new idea? seems we hardly totally walk around acpi staff. Thanks, Jinsong