In version 3.4 acpi_os_prepare_sleep() got introduced in parallel with
reduced hardware sleep support, and the two changes didn't get
synchronized: The new code doesn't call the hook function (if so
requested). Fix this, requiring a boolean parameter to be added to the
hook function to distinguish "extended" from "legacy" sleep.
This requires adjusting TXT, but the adjustments only go as far as
failing the extended mode call (since, looking at the TXT interface,
there doesn't even appear to be precautions to deal with that
alternative interface).
The hypervisor change underlying this is commit 62d1a69 ("ACPI: support
v5 (reduced HW) sleep interface") on the master branch of
git://xenbits.xen.org/xen.git.
Signed-off-by: Ben Guthro <[email protected]>
Signed-off-by: Jan Beulich <[email protected]>
Cc: Richard L Maliszewski <[email protected]>
Cc: Gang Wei <[email protected]>
Cc: Shane Wang <[email protected]>
Cc: Bob Moore <[email protected]>
Cc: Rafaell J. Wysocki <[email protected]>
Cc: [email protected]
Cc: [email protected]
v2: Extend description to include reference to hypervisor side change
v3: Split into multiple patches, separating subsystems
Remove bool parameters, in favor of u8
v4: Remove linux/acpi.h dependencies
Further patch split to break out acpica from OSL
More bool vs u8 fixes
Ben Guthro (5):
acpi: Remove need to include linux/acpi.h in common acpica code
acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path
acpi: Adjust linux acpi OS functions to new extended parameter
x86/tboot: Fail extended mode reduced hardware sleep
xen/acpi: notify xen when reduced hardware sleep is available
arch/x86/kernel/tboot.c | 6 +++++-
drivers/acpi/acpica/hwesleep.c | 7 +++++++
drivers/acpi/acpica/hwsleep.c | 3 +--
drivers/acpi/osl.c | 16 ++++++++--------
drivers/xen/acpi.c | 26 +++++++++++++-------------
include/acpi/acpiosxf.h | 6 ++++++
include/linux/acpi.h | 9 +++------
include/xen/acpi.h | 4 ++--
include/xen/interface/platform.h | 7 ++++---
9 files changed, 49 insertions(+), 35 deletions(-)
--
1.7.9.5
Move the definition of acpi_os_prepare_sleep into the OS services layer header,
and remove the include of linux/acpi.h from common acpica code.
Signed-off-by: Ben Guthro <[email protected]>
Cc: Rafaell J. Wysocki <[email protected]>
Cc: Bob Moore <[email protected]>
---
drivers/acpi/acpica/hwsleep.c | 1 -
include/acpi/acpiosxf.h | 6 ++++++
include/linux/acpi.h | 3 ---
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index e3828cc..867b947 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -43,7 +43,6 @@
*/
#include <acpi/acpi.h>
-#include <linux/acpi.h>
#include "accommon.h"
#define _COMPONENT ACPI_HARDWARE
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 5b3d2bd..c68b779 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -276,4 +276,10 @@ char *acpi_os_get_next_filename(void *dir_handle);
void acpi_os_close_directory(void *dir_handle);
+/*
+ * ACPI sleep preparation
+ */
+acpi_status acpi_os_prepare_sleep(u8 sleep_state,
+ u32 pm1a_control, u32 pm1b_control);
+
#endif /* __ACPIOSXF_H__ */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 17b5b59..709a2f2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -479,9 +479,6 @@ static inline bool acpi_driver_match_device(struct device *dev,
#ifdef CONFIG_ACPI
void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
u32 pm1a_ctrl, u32 pm1b_ctrl));
-
-acpi_status acpi_os_prepare_sleep(u8 sleep_state,
- u32 pm1a_control, u32 pm1b_control);
#ifdef CONFIG_X86
void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
#else
--
1.7.9.5
As tboot currently does not support the reduced hardware sleep
interface, fail this extended call.
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Ben Guthro <[email protected]>
Cc: [email protected]
Cc: Gang Wei <[email protected]>
---
arch/x86/kernel/tboot.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index f84fe00..57383b2 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -273,7 +273,8 @@ static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)
offsetof(struct acpi_table_facs, firmware_waking_vector);
}
-static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
+static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control,
+ bool extended)
{
static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
/* S0,1,2: */ -1, -1, -1,
@@ -284,6 +285,9 @@ static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
if (!tboot_enabled())
return 0;
+ if (extended)
+ return -1;
+
tboot_copy_fadt(&acpi_gbl_FADT);
tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control;
tboot->acpi_sinfo.pm1b_cnt_val = pm1b_control;
--
1.7.9.5
Make use of acpi_os_prepare_sleep extended parameter to notify xen
to make use of the reduced hardware sleep functionality
The hypervisor change underlying this is commit 62d1a69 ("ACPI: support
v5 (reduced HW) sleep interface") on the master branch of
git://xenbits.xen.org/xen.git.
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Ben Guthro <[email protected]>
Cc: Konrad Wilk <[email protected]>
---
drivers/xen/acpi.c | 26 +++++++++++++-------------
include/xen/acpi.h | 4 ++--
include/xen/interface/platform.h | 7 ++++---
3 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/xen/acpi.c b/drivers/xen/acpi.c
index 119d42a..371dade 100644
--- a/drivers/xen/acpi.c
+++ b/drivers/xen/acpi.c
@@ -35,27 +35,27 @@
#include <asm/xen/hypercall.h>
#include <asm/xen/hypervisor.h>
-int xen_acpi_notify_hypervisor_state(u8 sleep_state,
- u32 pm1a_cnt, u32 pm1b_cnt)
+int xen_acpi_notify_hypervisor_state(u8 sleep_state, u32 val_a, u32 val_b,
+ bool extended)
{
+ unsigned int bits = extended ? 8 : 16;
+
struct xen_platform_op op = {
.cmd = XENPF_enter_acpi_sleep,
.interface_version = XENPF_INTERFACE_VERSION,
- .u = {
- .enter_acpi_sleep = {
- .pm1a_cnt_val = (u16)pm1a_cnt,
- .pm1b_cnt_val = (u16)pm1b_cnt,
- .sleep_state = sleep_state,
- },
+ .u.enter_acpi_sleep = {
+ .val_a = (u16)val_a,
+ .val_b = (u16)val_b,
+ .sleep_state = sleep_state,
+ .flags = extended ? XENPF_ACPI_SLEEP_EXTENDED : 0,
},
};
- if ((pm1a_cnt & 0xffff0000) || (pm1b_cnt & 0xffff0000)) {
- WARN(1, "Using more than 16bits of PM1A/B 0x%x/0x%x!"
- "Email [email protected] Thank you.\n", \
- pm1a_cnt, pm1b_cnt);
+ if (WARN((val_a & (~0 << bits)) || (val_b & (~0 << bits)),
+ "Using more than %u bits of sleep control values %#x/%#x!"
+ "Email [email protected] - Thank you.\n", \
+ bits, val_a, val_b))
return -1;
- }
HYPERVISOR_dom0_op(&op);
return 1;
diff --git a/include/xen/acpi.h b/include/xen/acpi.h
index 68d73d0..a2d5667 100644
--- a/include/xen/acpi.h
+++ b/include/xen/acpi.h
@@ -75,8 +75,8 @@ static inline int xen_acpi_get_pxm(acpi_handle h)
return -ENXIO;
}
-int xen_acpi_notify_hypervisor_state(u8 sleep_state,
- u32 pm1a_cnt, u32 pm1b_cnd);
+int xen_acpi_notify_hypervisor_state(u8 sleep_state, u32 val_a, u32 val_b,
+ bool extended);
static inline void xen_acpi_sleep_register(void)
{
diff --git a/include/xen/interface/platform.h b/include/xen/interface/platform.h
index c57d5f6..f1331e3 100644
--- a/include/xen/interface/platform.h
+++ b/include/xen/interface/platform.h
@@ -152,10 +152,11 @@ DEFINE_GUEST_HANDLE_STRUCT(xenpf_firmware_info_t);
#define XENPF_enter_acpi_sleep 51
struct xenpf_enter_acpi_sleep {
/* IN variables */
- uint16_t pm1a_cnt_val; /* PM1a control value. */
- uint16_t pm1b_cnt_val; /* PM1b control value. */
+ uint16_t val_a; /* PM1a control / sleep type A. */
+ uint16_t val_b; /* PM1b control / sleep type B. */
uint32_t sleep_state; /* Which state to enter (Sn). */
- uint32_t flags; /* Must be zero. */
+#define XENPF_ACPI_SLEEP_EXTENDED 0x00000001
+ uint32_t flags; /* XENPF_ACPI_SLEEP_*. */
};
DEFINE_GUEST_HANDLE_STRUCT(xenpf_enter_acpi_sleep_t);
--
1.7.9.5
In version 3.4 acpi_os_prepare_sleep() got introduced in parallel with
reduced hardware sleep support, and the two changes didn't get
synchronized: The new code doesn't call the hook function (if so
requested). Fix this, requiring a parameter to be added to the
hook function to distinguish "extended" from "legacy" sleep.
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Ben Guthro <[email protected]>
Cc: Bob Moore <[email protected]>
Cc: Rafaell J. Wysocki <[email protected]>
Cc: [email protected]
---
drivers/acpi/acpica/hwesleep.c | 7 +++++++
drivers/acpi/acpica/hwsleep.c | 2 +-
include/acpi/acpiosxf.h | 4 ++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
index 5e5f762..69b3e15 100644
--- a/drivers/acpi/acpica/hwesleep.c
+++ b/drivers/acpi/acpica/hwesleep.c
@@ -128,6 +128,13 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state)
ACPI_FLUSH_CPU_CACHE();
+ status = acpi_os_prepare_sleep(sleep_state, acpi_gbl_sleep_type_a,
+ acpi_gbl_sleep_type_b, TRUE);
+ if (ACPI_SKIP(status))
+ return_ACPI_STATUS(AE_OK);
+ if (ACPI_FAILURE(status))
+ return_ACPI_STATUS(status);
+
/*
* Set the SLP_TYP and SLP_EN bits.
*
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index 867b947..cf78157 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -152,7 +152,7 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
ACPI_FLUSH_CPU_CACHE();
status = acpi_os_prepare_sleep(sleep_state, pm1a_control,
- pm1b_control);
+ pm1b_control, FALSE);
if (ACPI_SKIP(status))
return_ACPI_STATUS(AE_OK);
if (ACPI_FAILURE(status))
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index c68b779..51d0f78 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -279,7 +279,7 @@ void acpi_os_close_directory(void *dir_handle);
/*
* ACPI sleep preparation
*/
-acpi_status acpi_os_prepare_sleep(u8 sleep_state,
- u32 pm1a_control, u32 pm1b_control);
+acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32 val_b,
+ u8 extended);
#endif /* __ACPIOSXF_H__ */
--
1.7.9.5
Change the function definitions of acpi_os_prepare_sleep() and
acpi_os_set_prepare_sleep() to pass along the new extended sleep
parameter.
Signed-off-by: Jan Beulich <[email protected]>
Signed-off-by: Ben Guthro <[email protected]>
Cc: Bob Moore <[email protected]>
Cc: Rafaell J. Wysocki <[email protected]>
Cc: [email protected]
---
drivers/acpi/osl.c | 16 ++++++++--------
include/linux/acpi.h | 6 +++---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e721863..0251c9b 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger);
extern char line_buf[80];
#endif /*ENABLE_DEBUGGER */
-static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
- u32 pm1b_ctrl);
+static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32 val_b,
+ bool extended);
static acpi_osd_handler acpi_irq_handler;
static void *acpi_irq_context;
@@ -1757,13 +1757,13 @@ acpi_status acpi_os_terminate(void)
return AE_OK;
}
-acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
- u32 pm1b_control)
+acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32 val_b,
+ u8 extended)
{
int rc = 0;
if (__acpi_os_prepare_sleep)
- rc = __acpi_os_prepare_sleep(sleep_state,
- pm1a_control, pm1b_control);
+ rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
+ extended);
if (rc < 0)
return AE_ERROR;
else if (rc > 0)
@@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
return AE_OK;
}
-void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
- u32 pm1a_ctrl, u32 pm1b_ctrl))
+void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
+ u32 val_b, bool extended))
{
__acpi_os_prepare_sleep = func;
}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 709a2f2..26f9996 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -477,8 +477,8 @@ static inline bool acpi_driver_match_device(struct device *dev,
#endif /* !CONFIG_ACPI */
#ifdef CONFIG_ACPI
-void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
- u32 pm1a_ctrl, u32 pm1b_ctrl));
+void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
+ u32 val_b, bool extended));
#ifdef CONFIG_X86
void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
#else
@@ -488,7 +488,7 @@ static inline void arch_reserve_mem_area(acpi_physical_address addr,
}
#endif /* CONFIG_X86 */
#else
-#define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0)
+#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { } while (0)
#endif
#if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
--
1.7.9.5
>>> On 27.06.13 at 17:02, Ben Guthro <[email protected]> wrote:
> Change the function definitions of acpi_os_prepare_sleep() and
> acpi_os_set_prepare_sleep() to pass along the new extended sleep
> parameter.
>
> Signed-off-by: Jan Beulich <[email protected]>
> Signed-off-by: Ben Guthro <[email protected]>
> Cc: Bob Moore <[email protected]>
> Cc: Rafaell J. Wysocki <[email protected]>
> Cc: [email protected]
> ---
> drivers/acpi/osl.c | 16 ++++++++--------
> include/linux/acpi.h | 6 +++---
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index e721863..0251c9b 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger);
> extern char line_buf[80];
> #endif /*ENABLE_DEBUGGER */
>
> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
> - u32 pm1b_ctrl);
> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32 val_b,
> + bool extended);
So from here till patch 5 the build will be half broken because of
the type mismatches? I think at least the types of the consumers
need to be changed in this patch; leaving the meat of the Xen
change to patch 4 is perhaps fine.
Jan
>
> static acpi_osd_handler acpi_irq_handler;
> static void *acpi_irq_context;
> @@ -1757,13 +1757,13 @@ acpi_status acpi_os_terminate(void)
> return AE_OK;
> }
>
> -acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
> - u32 pm1b_control)
> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32 val_b,
> + u8 extended)
> {
> int rc = 0;
> if (__acpi_os_prepare_sleep)
> - rc = __acpi_os_prepare_sleep(sleep_state,
> - pm1a_control, pm1b_control);
> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
> + extended);
> if (rc < 0)
> return AE_ERROR;
> else if (rc > 0)
> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32
> pm1a_control,
> return AE_OK;
> }
>
> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> - u32 pm1a_ctrl, u32 pm1b_ctrl))
> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
> + u32 val_b, bool extended))
> {
> __acpi_os_prepare_sleep = func;
> }
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 709a2f2..26f9996 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -477,8 +477,8 @@ static inline bool acpi_driver_match_device(struct device
> *dev,
> #endif /* !CONFIG_ACPI */
>
> #ifdef CONFIG_ACPI
> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> - u32 pm1a_ctrl, u32 pm1b_ctrl));
> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
> + u32 val_b, bool extended));
> #ifdef CONFIG_X86
> void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
> #else
> @@ -488,7 +488,7 @@ static inline void
> arch_reserve_mem_area(acpi_physical_address addr,
> }
> #endif /* CONFIG_X86 */
> #else
> -#define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while
> (0)
> +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { } while (0)
> #endif
>
> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
> --
> 1.7.9.5
I have not seen a discussion of the details on this, so can someone explain to me just why acpi_os_prepare_sleep is needed, what does it do, and why these changes are being made to ACPICA code?
Thanks,
Bob
> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: Thursday, June 27, 2013 8:12 AM
> To: Ben Guthro
> Cc: Moore, Robert; [email protected]; Konrad Rzeszutek Wilk; Rafaell
> J . Wysocki; [email protected]; [email protected]
> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new
> extended parameter
>
> >>> On 27.06.13 at 17:02, Ben Guthro <[email protected]> wrote:
> > Change the function definitions of acpi_os_prepare_sleep() and
> > acpi_os_set_prepare_sleep() to pass along the new extended sleep
> > parameter.
> >
> > Signed-off-by: Jan Beulich <[email protected]>
> > Signed-off-by: Ben Guthro <[email protected]>
> > Cc: Bob Moore <[email protected]>
> > Cc: Rafaell J. Wysocki <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/acpi/osl.c | 16 ++++++++--------
> > include/linux/acpi.h | 6 +++---
> > 2 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > e721863..0251c9b 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char
> > line_buf[80];
> > #endif /*ENABLE_DEBUGGER */
> >
> > -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
> > - u32 pm1b_ctrl);
> > +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32
> val_b,
> > + bool extended);
>
> So from here till patch 5 the build will be half broken because of the
> type mismatches? I think at least the types of the consumers need to be
> changed in this patch; leaving the meat of the Xen change to patch 4 is
> perhaps fine.
>
> Jan
>
> >
> > static acpi_osd_handler acpi_irq_handler; static void
> > *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status
> > acpi_os_terminate(void)
> > return AE_OK;
> > }
> >
> > -acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
> > - u32 pm1b_control)
> > +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32 val_b,
> > + u8 extended)
> > {
> > int rc = 0;
> > if (__acpi_os_prepare_sleep)
> > - rc = __acpi_os_prepare_sleep(sleep_state,
> > - pm1a_control, pm1b_control);
> > + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
> > + extended);
> > if (rc < 0)
> > return AE_ERROR;
> > else if (rc > 0)
> > @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8
> > sleep_state, u32 pm1a_control,
> > return AE_OK;
> > }
> >
> > -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> > - u32 pm1a_ctrl, u32 pm1b_ctrl))
> > +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
> > + u32 val_b, bool extended))
> > {
> > __acpi_os_prepare_sleep = func;
> > }
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h index
> > 709a2f2..26f9996 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -477,8 +477,8 @@ static inline bool acpi_driver_match_device(struct
> > device *dev,
> > #endif /* !CONFIG_ACPI */
> >
> > #ifdef CONFIG_ACPI
> > -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> > - u32 pm1a_ctrl, u32 pm1b_ctrl));
> > +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
> > + u32 val_b, bool extended));
> > #ifdef CONFIG_X86
> > void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
> > #else @@ -488,7 +488,7 @@ static inline void
> > arch_reserve_mem_area(acpi_physical_address addr, } #endif /*
> > CONFIG_X86 */ #else -#define acpi_os_set_prepare_sleep(func,
> > pm1a_ctrl, pm1b_ctrl) do { } while
> > (0)
> > +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { }
> > +while (0)
> > #endif
> >
> > #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
> > --
> > 1.7.9.5
>
>
On 06/27/2013 11:59 AM, Moore, Robert wrote:
> I have not seen a discussion of the details on this, so can someone explain to me just why acpi_os_prepare_sleep is needed, what does it do, and why these changes are being made to ACPICA code?
Hi Bob,
I'm not familiar with your background here, so I apologize if I am stating obvious things below.
acpi_os_prepare_sleep() has been in the acpica code for some time now, allowing for OS specific hooks to account for differences between OS architectures.
Specifically, it has been in since:
commit 09f98a825a821f7a3f1b162f9ed023f37213a63b
Author: Tang Liang <[email protected]>
Date: Fri Dec 9 10:05:54 2011 +0800
x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling tboot_sleep.
The ACPI suspend path makes a call to tboot_sleep right before
it writes the PM1A, PM1B values. We replace the direct call to
tboot via an registration callback similar to __acpi_register_gsi.
CC: Len Brown <[email protected]>
Acked-by: Joseph Cihula <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
[v1: Added __attribute__ ((unused))]
[v2: Introduced a wrapper instead of changing tboot_sleep return values]
[v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare]
Signed-off-by: Tang Liang <[email protected]>
[v1: Fix compile issues on IA64 and PPC64]
[v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep properly]
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
In this case, the Xen hypervisor, and not linux needs to actually put the system into S3, so this hook gives the architecture to do so.
This change does not introduce this functionality, but simply moves the code around into the proper locations, such that the acpica code no longer need to include linux specific headers.
I hope this helps.
If you have specific questions, please let me know.
Regards,
Ben
>
> Thanks,
> Bob
>
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:[email protected]]
>> Sent: Thursday, June 27, 2013 8:12 AM
>> To: Ben Guthro
>> Cc: Moore, Robert; [email protected]; Konrad Rzeszutek Wilk; Rafaell
>> J . Wysocki; [email protected]; [email protected]
>> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new
>> extended parameter
>>
>>>>> On 27.06.13 at 17:02, Ben Guthro <[email protected]> wrote:
>>> Change the function definitions of acpi_os_prepare_sleep() and
>>> acpi_os_set_prepare_sleep() to pass along the new extended sleep
>>> parameter.
>>>
>>> Signed-off-by: Jan Beulich <[email protected]>
>>> Signed-off-by: Ben Guthro <[email protected]>
>>> Cc: Bob Moore <[email protected]>
>>> Cc: Rafaell J. Wysocki <[email protected]>
>>> Cc: [email protected]
>>> ---
>>> drivers/acpi/osl.c | 16 ++++++++--------
>>> include/linux/acpi.h | 6 +++---
>>> 2 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
>>> e721863..0251c9b 100644
>>> --- a/drivers/acpi/osl.c
>>> +++ b/drivers/acpi/osl.c
>>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char
>>> line_buf[80];
>>> #endif /*ENABLE_DEBUGGER */
>>>
>>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
>>> - u32 pm1b_ctrl);
>>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32
>> val_b,
>>> + bool extended);
>>
>> So from here till patch 5 the build will be half broken because of the
>> type mismatches? I think at least the types of the consumers need to be
>> changed in this patch; leaving the meat of the Xen change to patch 4 is
>> perhaps fine.
>>
>> Jan
>>
>>>
>>> static acpi_osd_handler acpi_irq_handler; static void
>>> *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status
>>> acpi_os_terminate(void)
>>> return AE_OK;
>>> }
>>>
>>> -acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
>>> - u32 pm1b_control)
>>> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32 val_b,
>>> + u8 extended)
>>> {
>>> int rc = 0;
>>> if (__acpi_os_prepare_sleep)
>>> - rc = __acpi_os_prepare_sleep(sleep_state,
>>> - pm1a_control, pm1b_control);
>>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
>>> + extended);
>>> if (rc < 0)
>>> return AE_ERROR;
>>> else if (rc > 0)
>>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8
>>> sleep_state, u32 pm1a_control,
>>> return AE_OK;
>>> }
>>>
>>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>>> - u32 pm1a_ctrl, u32 pm1b_ctrl))
>>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
>>> + u32 val_b, bool extended))
>>> {
>>> __acpi_os_prepare_sleep = func;
>>> }
>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index
>>> 709a2f2..26f9996 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -477,8 +477,8 @@ static inline bool acpi_driver_match_device(struct
>>> device *dev,
>>> #endif /* !CONFIG_ACPI */
>>>
>>> #ifdef CONFIG_ACPI
>>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>>> - u32 pm1a_ctrl, u32 pm1b_ctrl));
>>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
>>> + u32 val_b, bool extended));
>>> #ifdef CONFIG_X86
>>> void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
>>> #else @@ -488,7 +488,7 @@ static inline void
>>> arch_reserve_mem_area(acpi_physical_address addr, } #endif /*
>>> CONFIG_X86 */ #else -#define acpi_os_set_prepare_sleep(func,
>>> pm1a_ctrl, pm1b_ctrl) do { } while
>>> (0)
>>> +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { }
>>> +while (0)
>>> #endif
>>>
>>> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
>>> --
>>> 1.7.9.5
>>
>>
>
I'm the ACPICA owner.
The issue I have is that acpi_os_prepare_sleep causes a divergence between the raw ACPICA source code and the "linux" version of the ACPICA code.
If it is possible to implement the functionality that is needed without changes to ACPICA, then that would be best. If not, we are willing to integrate this interface into ACPICA. However, this has an impact on all operating systems that use ACPICA, so this is always the last resort.
Bob
> -----Original Message-----
> From: Ben Guthro [mailto:[email protected]]
> Sent: Thursday, June 27, 2013 9:13 AM
> To: Moore, Robert
> Cc: Jan Beulich; Zheng, Lv; Box, David E; Brown, Len; xen-
> [email protected]; Konrad Rzeszutek Wilk; Rafaell J . Wysocki; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new
> extended parameter
>
>
>
> On 06/27/2013 11:59 AM, Moore, Robert wrote:
> > I have not seen a discussion of the details on this, so can someone
> explain to me just why acpi_os_prepare_sleep is needed, what does it do,
> and why these changes are being made to ACPICA code?
>
> Hi Bob,
>
> I'm not familiar with your background here, so I apologize if I am stating
> obvious things below.
>
> acpi_os_prepare_sleep() has been in the acpica code for some time now,
> allowing for OS specific hooks to account for differences between OS
> architectures.
>
> Specifically, it has been in since:
>
> commit 09f98a825a821f7a3f1b162f9ed023f37213a63b
> Author: Tang Liang <[email protected]>
> Date: Fri Dec 9 10:05:54 2011 +0800
>
> x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling
> tboot_sleep.
>
> The ACPI suspend path makes a call to tboot_sleep right before
> it writes the PM1A, PM1B values. We replace the direct call to
> tboot via an registration callback similar to __acpi_register_gsi.
>
> CC: Len Brown <[email protected]>
> Acked-by: Joseph Cihula <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> [v1: Added __attribute__ ((unused))]
> [v2: Introduced a wrapper instead of changing tboot_sleep return
> values]
> [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare]
> Signed-off-by: Tang Liang <[email protected]>
> [v1: Fix compile issues on IA64 and PPC64]
> [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep
> properly]
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
>
>
>
> In this case, the Xen hypervisor, and not linux needs to actually put the
> system into S3, so this hook gives the architecture to do so.
>
> This change does not introduce this functionality, but simply moves the
> code around into the proper locations, such that the acpica code no longer
> need to include linux specific headers.
>
> I hope this helps.
> If you have specific questions, please let me know.
>
> Regards,
> Ben
>
> >
> > Thanks,
> > Bob
> >
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:[email protected]]
> >> Sent: Thursday, June 27, 2013 8:12 AM
> >> To: Ben Guthro
> >> Cc: Moore, Robert; [email protected]; Konrad Rzeszutek Wilk;
> >> Rafaell J . Wysocki; [email protected];
> >> [email protected]
> >> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to
> >> new extended parameter
> >>
> >>>>> On 27.06.13 at 17:02, Ben Guthro <[email protected]> wrote:
> >>> Change the function definitions of acpi_os_prepare_sleep() and
> >>> acpi_os_set_prepare_sleep() to pass along the new extended sleep
> >>> parameter.
> >>>
> >>> Signed-off-by: Jan Beulich <[email protected]>
> >>> Signed-off-by: Ben Guthro <[email protected]>
> >>> Cc: Bob Moore <[email protected]>
> >>> Cc: Rafaell J. Wysocki <[email protected]>
> >>> Cc: [email protected]
> >>> ---
> >>> drivers/acpi/osl.c | 16 ++++++++--------
> >>> include/linux/acpi.h | 6 +++---
> >>> 2 files changed, 11 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> >>> e721863..0251c9b 100644
> >>> --- a/drivers/acpi/osl.c
> >>> +++ b/drivers/acpi/osl.c
> >>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char
> >>> line_buf[80];
> >>> #endif /*ENABLE_DEBUGGER */
> >>>
> >>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
> >>> - u32 pm1b_ctrl);
> >>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a,
> >>> +u32
> >> val_b,
> >>> + bool extended);
> >>
> >> So from here till patch 5 the build will be half broken because of
> >> the type mismatches? I think at least the types of the consumers need
> >> to be changed in this patch; leaving the meat of the Xen change to
> >> patch 4 is perhaps fine.
> >>
> >> Jan
> >>
> >>>
> >>> static acpi_osd_handler acpi_irq_handler; static void
> >>> *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status
> >>> acpi_os_terminate(void)
> >>> return AE_OK;
> >>> }
> >>>
> >>> -acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
> >>> - u32 pm1b_control)
> >>> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32
> val_b,
> >>> + u8 extended)
> >>> {
> >>> int rc = 0;
> >>> if (__acpi_os_prepare_sleep)
> >>> - rc = __acpi_os_prepare_sleep(sleep_state,
> >>> - pm1a_control, pm1b_control);
> >>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
> >>> + extended);
> >>> if (rc < 0)
> >>> return AE_ERROR;
> >>> else if (rc > 0)
> >>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8
> >>> sleep_state, u32 pm1a_control,
> >>> return AE_OK;
> >>> }
> >>>
> >>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> >>> - u32 pm1a_ctrl, u32 pm1b_ctrl))
> >>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
> >>> + u32 val_b, bool extended))
> >>> {
> >>> __acpi_os_prepare_sleep = func;
> >>> }
> >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index
> >>> 709a2f2..26f9996 100644
> >>> --- a/include/linux/acpi.h
> >>> +++ b/include/linux/acpi.h
> >>> @@ -477,8 +477,8 @@ static inline bool
> >>> acpi_driver_match_device(struct device *dev,
> >>> #endif /* !CONFIG_ACPI */
> >>>
> >>> #ifdef CONFIG_ACPI
> >>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
> >>> - u32 pm1a_ctrl, u32 pm1b_ctrl));
> >>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
> >>> + u32 val_b, bool extended));
> >>> #ifdef CONFIG_X86
> >>> void arch_reserve_mem_area(acpi_physical_address addr, size_t
> >>> size); #else @@ -488,7 +488,7 @@ static inline void
> >>> arch_reserve_mem_area(acpi_physical_address addr, } #endif /*
> >>> CONFIG_X86 */ #else -#define acpi_os_set_prepare_sleep(func,
> >>> pm1a_ctrl, pm1b_ctrl) do { } while
> >>> (0)
> >>> +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { }
> >>> +while (0)
> >>> #endif
> >>>
> >>> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
> >>> --
> >>> 1.7.9.5
> >>
> >>
> >
On 06/27/2013 04:19 PM, Moore, Robert wrote:
> I'm the ACPICA owner.
>
Apologies, I should have looked that up when Rafael asked for you to be
CC'ed
> The issue I have is that acpi_os_prepare_sleep causes a divergence between the raw ACPICA source code and the "linux" version of the ACPICA code.
>
Understood. The v2 version of this patchset was discussed with Jan
Beulich here:
http://www.gossamer-threads.com/lists/xen/devel/274507
In this thread, Rafael suggests that these changes be made, and
submitted through linux-acpi, with he, and you CC'ed.
This is more of a cleanup effort to existing functionality, that did not
get coordinated. That is, as the comment indicates, in linux 3.4,
reduced hardware sleep was introduced as part of ACPIv5. In parallel,
acpi_os_prepare_sleep() was also introduced, to allow for subsystems
such as Xen, and tboot to take advantage of the vast majority of ACPICA,
but hook some low level capabilities via function pointer, to do the
specific things for those subsystems.
Rafael rightly pointed out that inclusion of linux/acpi.h in ACPICA code
is wrong, and we should make efforts to avoid doing so. This patch is to
clean this up, and also to extend the functionality to also go down the
same path for the reduced hardware sleep path, passing that "extended"
flag down to that subsystem, so that it might make use of it at the very
low levels.
All efforts are being made to reduce the amount of churn in ACPICA,
since it is an upstream project to linux.
> If it is possible to implement the functionality that is needed without changes to ACPICA, then that would be best. If not, we are willing to integrate this interface into ACPICA. However, this has an impact on all operating systems that use ACPICA, so this is always the last resort.
>
I am unaware of a way to take advantage of the infrastructure that
ACPICA provides, and also do that which is necessary to make it work in
Xen / tboot without these hooks.
It would be greatly appreciated if you would consider this new interface
for inclusion.
Thanks for your time.
Ben
> Bob
>
>
>> -----Original Message-----
>> From: Ben Guthro [mailto:[email protected]]
>> Sent: Thursday, June 27, 2013 9:13 AM
>> To: Moore, Robert
>> Cc: Jan Beulich; Zheng, Lv; Box, David E; Brown, Len; xen-
>> [email protected]; Konrad Rzeszutek Wilk; Rafaell J . Wysocki; linux-
>> [email protected]; [email protected]
>> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new
>> extended parameter
>>
>>
>>
>> On 06/27/2013 11:59 AM, Moore, Robert wrote:
>>> I have not seen a discussion of the details on this, so can someone
>> explain to me just why acpi_os_prepare_sleep is needed, what does it do,
>> and why these changes are being made to ACPICA code?
>>
>> Hi Bob,
>>
>> I'm not familiar with your background here, so I apologize if I am stating
>> obvious things below.
>>
>> acpi_os_prepare_sleep() has been in the acpica code for some time now,
>> allowing for OS specific hooks to account for differences between OS
>> architectures.
>>
>> Specifically, it has been in since:
>>
>> commit 09f98a825a821f7a3f1b162f9ed023f37213a63b
>> Author: Tang Liang <[email protected]>
>> Date: Fri Dec 9 10:05:54 2011 +0800
>>
>> x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling
>> tboot_sleep.
>>
>> The ACPI suspend path makes a call to tboot_sleep right before
>> it writes the PM1A, PM1B values. We replace the direct call to
>> tboot via an registration callback similar to __acpi_register_gsi.
>>
>> CC: Len Brown <[email protected]>
>> Acked-by: Joseph Cihula <[email protected]>
>> Acked-by: Rafael J. Wysocki <[email protected]>
>> [v1: Added __attribute__ ((unused))]
>> [v2: Introduced a wrapper instead of changing tboot_sleep return
>> values]
>> [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare]
>> Signed-off-by: Tang Liang <[email protected]>
>> [v1: Fix compile issues on IA64 and PPC64]
>> [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep
>> properly]
>> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
>>
>>
>>
>> In this case, the Xen hypervisor, and not linux needs to actually put the
>> system into S3, so this hook gives the architecture to do so.
>>
>> This change does not introduce this functionality, but simply moves the
>> code around into the proper locations, such that the acpica code no longer
>> need to include linux specific headers.
>>
>> I hope this helps.
>> If you have specific questions, please let me know.
>>
>> Regards,
>> Ben
>>
>>>
>>> Thanks,
>>> Bob
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich [mailto:[email protected]]
>>>> Sent: Thursday, June 27, 2013 8:12 AM
>>>> To: Ben Guthro
>>>> Cc: Moore, Robert; [email protected]; Konrad Rzeszutek Wilk;
>>>> Rafaell J . Wysocki; [email protected];
>>>> [email protected]
>>>> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to
>>>> new extended parameter
>>>>
>>>>>>> On 27.06.13 at 17:02, Ben Guthro <[email protected]> wrote:
>>>>> Change the function definitions of acpi_os_prepare_sleep() and
>>>>> acpi_os_set_prepare_sleep() to pass along the new extended sleep
>>>>> parameter.
>>>>>
>>>>> Signed-off-by: Jan Beulich <[email protected]>
>>>>> Signed-off-by: Ben Guthro <[email protected]>
>>>>> Cc: Bob Moore <[email protected]>
>>>>> Cc: Rafaell J. Wysocki <[email protected]>
>>>>> Cc: [email protected]
>>>>> ---
>>>>> drivers/acpi/osl.c | 16 ++++++++--------
>>>>> include/linux/acpi.h | 6 +++---
>>>>> 2 files changed, 11 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
>>>>> e721863..0251c9b 100644
>>>>> --- a/drivers/acpi/osl.c
>>>>> +++ b/drivers/acpi/osl.c
>>>>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char
>>>>> line_buf[80];
>>>>> #endif /*ENABLE_DEBUGGER */
>>>>>
>>>>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
>>>>> - u32 pm1b_ctrl);
>>>>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a,
>>>>> +u32
>>>> val_b,
>>>>> + bool extended);
>>>>
>>>> So from here till patch 5 the build will be half broken because of
>>>> the type mismatches? I think at least the types of the consumers need
>>>> to be changed in this patch; leaving the meat of the Xen change to
>>>> patch 4 is perhaps fine.
>>>>
>>>> Jan
>>>>
>>>>>
>>>>> static acpi_osd_handler acpi_irq_handler; static void
>>>>> *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status
>>>>> acpi_os_terminate(void)
>>>>> return AE_OK;
>>>>> }
>>>>>
>>>>> -acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
>>>>> - u32 pm1b_control)
>>>>> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32
>> val_b,
>>>>> + u8 extended)
>>>>> {
>>>>> int rc = 0;
>>>>> if (__acpi_os_prepare_sleep)
>>>>> - rc = __acpi_os_prepare_sleep(sleep_state,
>>>>> - pm1a_control, pm1b_control);
>>>>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
>>>>> + extended);
>>>>> if (rc < 0)
>>>>> return AE_ERROR;
>>>>> else if (rc > 0)
>>>>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8
>>>>> sleep_state, u32 pm1a_control,
>>>>> return AE_OK;
>>>>> }
>>>>>
>>>>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>>>>> - u32 pm1a_ctrl, u32 pm1b_ctrl))
>>>>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
>>>>> + u32 val_b, bool extended))
>>>>> {
>>>>> __acpi_os_prepare_sleep = func;
>>>>> }
>>>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index
>>>>> 709a2f2..26f9996 100644
>>>>> --- a/include/linux/acpi.h
>>>>> +++ b/include/linux/acpi.h
>>>>> @@ -477,8 +477,8 @@ static inline bool
>>>>> acpi_driver_match_device(struct device *dev,
>>>>> #endif /* !CONFIG_ACPI */
>>>>>
>>>>> #ifdef CONFIG_ACPI
>>>>> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>>>>> - u32 pm1a_ctrl, u32 pm1b_ctrl));
>>>>> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a,
>>>>> + u32 val_b, bool extended));
>>>>> #ifdef CONFIG_X86
>>>>> void arch_reserve_mem_area(acpi_physical_address addr, size_t
>>>>> size); #else @@ -488,7 +488,7 @@ static inline void
>>>>> arch_reserve_mem_area(acpi_physical_address addr, } #endif /*
>>>>> CONFIG_X86 */ #else -#define acpi_os_set_prepare_sleep(func,
>>>>> pm1a_ctrl, pm1b_ctrl) do { } while
>>>>> (0)
>>>>> +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { }
>>>>> +while (0)
>>>>> #endif
>>>>>
>>>>> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME)
>>>>> --
>>>>> 1.7.9.5
>>>>
>>>>
>>>
On Thursday, June 27, 2013 11:01:58 AM Ben Guthro wrote:
> In version 3.4 acpi_os_prepare_sleep() got introduced in parallel with
> reduced hardware sleep support, and the two changes didn't get
> synchronized: The new code doesn't call the hook function (if so
> requested). Fix this, requiring a boolean parameter to be added to the
> hook function to distinguish "extended" from "legacy" sleep.
>
> This requires adjusting TXT, but the adjustments only go as far as
> failing the extended mode call (since, looking at the TXT interface,
> there doesn't even appear to be precautions to deal with that
> alternative interface).
>
> The hypervisor change underlying this is commit 62d1a69 ("ACPI: support
> v5 (reduced HW) sleep interface") on the master branch of
> git://xenbits.xen.org/xen.git.
>
> Signed-off-by: Ben Guthro <[email protected]>
> Signed-off-by: Jan Beulich <[email protected]>
> Cc: Richard L Maliszewski <[email protected]>
> Cc: Gang Wei <[email protected]>
> Cc: Shane Wang <[email protected]>
> Cc: Bob Moore <[email protected]>
> Cc: Rafaell J. Wysocki <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
>
> v2: Extend description to include reference to hypervisor side change
> v3: Split into multiple patches, separating subsystems
> Remove bool parameters, in favor of u8
> v4: Remove linux/acpi.h dependencies
> Further patch split to break out acpica from OSL
> More bool vs u8 fixes
>
> Ben Guthro (5):
> acpi: Remove need to include linux/acpi.h in common acpica code
> acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path
> acpi: Adjust linux acpi OS functions to new extended parameter
> x86/tboot: Fail extended mode reduced hardware sleep
> xen/acpi: notify xen when reduced hardware sleep is available
The ongoing discussion means to me that the ACPICA maintainers don't want
acpi_os_prepare_sleep() and quite frankly I understand them, because ACPICA
is about implementing the spec and not about things beyond it.
This means that patch [1/5] goes away.
That said, at the same time we need to address the problem at hand, which
is to make Xen work with the reduced HW sleep.
For that, I don't honestly think that modifying acpi_os_prepare_sleep() the
way the patchset is doing it is appropriate and the change of the meaning of
the arguments is simply disgusting.
To me, it would be much cleaner to add acpi_os_prepare_extended_sleep()
specifically to be called by acpi_hw_extended_sleep() and make tboot and Xen
use that.
This way or another, we'll need to live with one more divergence between the
upstream ACPICA and the Linux ACPICA code because of that, but that'd be just
a few added lines in acpi_hw_extended_sleep(), so I suppose it wouldn't be
such a big deal.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Jul 27, 2013, at 9:51 AM, "Rafael J. Wysocki" <[email protected]> wrote:
> On Thursday, June 27, 2013 11:01:58 AM Ben Guthro wrote:
>> In version 3.4 acpi_os_prepare_sleep() got introduced in parallel with
>> reduced hardware sleep support, and the two changes didn't get
>> synchronized: The new code doesn't call the hook function (if so
>> requested). Fix this, requiring a boolean parameter to be added to the
>> hook function to distinguish "extended" from "legacy" sleep.
>>
>> This requires adjusting TXT, but the adjustments only go as far as
>> failing the extended mode call (since, looking at the TXT interface,
>> there doesn't even appear to be precautions to deal with that
>> alternative interface).
>>
>> The hypervisor change underlying this is commit 62d1a69 ("ACPI: support
>> v5 (reduced HW) sleep interface") on the master branch of
>> git://xenbits.xen.org/xen.git.
>>
>> Signed-off-by: Ben Guthro <[email protected]>
>> Signed-off-by: Jan Beulich <[email protected]>
>> Cc: Richard L Maliszewski <[email protected]>
>> Cc: Gang Wei <[email protected]>
>> Cc: Shane Wang <[email protected]>
>> Cc: Bob Moore <[email protected]>
>> Cc: Rafaell J. Wysocki <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>>
>> v2: Extend description to include reference to hypervisor side change
>> v3: Split into multiple patches, separating subsystems
>> Remove bool parameters, in favor of u8
>> v4: Remove linux/acpi.h dependencies
>> Further patch split to break out acpica from OSL
>> More bool vs u8 fixes
>>
>> Ben Guthro (5):
>> acpi: Remove need to include linux/acpi.h in common acpica code
>> acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path
>> acpi: Adjust linux acpi OS functions to new extended parameter
>> x86/tboot: Fail extended mode reduced hardware sleep
>> xen/acpi: notify xen when reduced hardware sleep is available
>
> The ongoing discussion means to me that the ACPICA maintainers don't want
> acpi_os_prepare_sleep() and quite frankly I understand them, because ACPICA
> is about implementing the spec and not about things beyond it.
>
> This means that patch [1/5] goes away.
>
> That said, at the same time we need to address the problem at hand, which
> is to make Xen work with the reduced HW sleep.
>
> For that, I don't honestly think that modifying acpi_os_prepare_sleep() the
> way the patchset is doing it is appropriate and the change of the meaning of
> the arguments is simply disgusting.
>
> To me, it would be much cleaner to add acpi_os_prepare_extended_sleep()
> specifically to be called by acpi_hw_extended_sleep() and make tboot and Xen
> use that.
>
> This way or another, we'll need to live with one more divergence between the
> upstream ACPICA and the Linux ACPICA code because of that, but that'd be just
> a few added lines in acpi_hw_extended_sleep(), so I suppose it wouldn't be
> such a big deal.
>
Ok, thank you for the review, and being open to addressing the problem at hand, without a full architecture rework (not to say that that discussion is not also needed)
I will try to make some time next week to rework the patch set to address these concerns, and submit a new series.
Thanks
Ben
> Thanks,
> Rafael
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
On Saturday, July 27, 2013 03:33:31 PM Ben Guthro wrote:
>
> On Jul 27, 2013, at 9:51 AM, "Rafael J. Wysocki" <[email protected]> wrote:
>
> > On Thursday, June 27, 2013 11:01:58 AM Ben Guthro wrote:
> >> In version 3.4 acpi_os_prepare_sleep() got introduced in parallel with
> >> reduced hardware sleep support, and the two changes didn't get
> >> synchronized: The new code doesn't call the hook function (if so
> >> requested). Fix this, requiring a boolean parameter to be added to the
> >> hook function to distinguish "extended" from "legacy" sleep.
> >>
> >> This requires adjusting TXT, but the adjustments only go as far as
> >> failing the extended mode call (since, looking at the TXT interface,
> >> there doesn't even appear to be precautions to deal with that
> >> alternative interface).
> >>
> >> The hypervisor change underlying this is commit 62d1a69 ("ACPI: support
> >> v5 (reduced HW) sleep interface") on the master branch of
> >> git://xenbits.xen.org/xen.git.
> >>
> >> Signed-off-by: Ben Guthro <[email protected]>
> >> Signed-off-by: Jan Beulich <[email protected]>
> >> Cc: Richard L Maliszewski <[email protected]>
> >> Cc: Gang Wei <[email protected]>
> >> Cc: Shane Wang <[email protected]>
> >> Cc: Bob Moore <[email protected]>
> >> Cc: Rafaell J. Wysocki <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >>
> >> v2: Extend description to include reference to hypervisor side change
> >> v3: Split into multiple patches, separating subsystems
> >> Remove bool parameters, in favor of u8
> >> v4: Remove linux/acpi.h dependencies
> >> Further patch split to break out acpica from OSL
> >> More bool vs u8 fixes
> >>
> >> Ben Guthro (5):
> >> acpi: Remove need to include linux/acpi.h in common acpica code
> >> acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path
> >> acpi: Adjust linux acpi OS functions to new extended parameter
> >> x86/tboot: Fail extended mode reduced hardware sleep
> >> xen/acpi: notify xen when reduced hardware sleep is available
> >
> > The ongoing discussion means to me that the ACPICA maintainers don't want
> > acpi_os_prepare_sleep() and quite frankly I understand them, because ACPICA
> > is about implementing the spec and not about things beyond it.
> >
> > This means that patch [1/5] goes away.
> >
> > That said, at the same time we need to address the problem at hand, which
> > is to make Xen work with the reduced HW sleep.
> >
> > For that, I don't honestly think that modifying acpi_os_prepare_sleep() the
> > way the patchset is doing it is appropriate and the change of the meaning of
> > the arguments is simply disgusting.
> >
> > To me, it would be much cleaner to add acpi_os_prepare_extended_sleep()
> > specifically to be called by acpi_hw_extended_sleep() and make tboot and Xen
> > use that.
> >
> > This way or another, we'll need to live with one more divergence between the
> > upstream ACPICA and the Linux ACPICA code because of that, but that'd be just
> > a few added lines in acpi_hw_extended_sleep(), so I suppose it wouldn't be
> > such a big deal.
> >
>
> Ok, thank you for the review, and being open to addressing the problem at
> hand,
No problem, although I'm not exactly happy with it.
> without a full architecture rework (not to say that that discussion is not
> also needed)
Sure, it is needed.
> I will try to make some time next week to rework the patch set to address
> these concerns, and submit a new series.
Thanks!
Rafael