[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path
- To: "Zheng, Lv" <lv.zheng@xxxxxxxxx>
- From: konrad wilk <konrad.wilk@xxxxxxxxxx>
- Date: Fri, 26 Jul 2013 14:03:39 -0400
- Cc: Ben Guthro <benjamin.guthro@xxxxxxxxxx>, "Moore, Robert" <robert.moore@xxxxxxxxx>, "linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>, "Rafael J . Wysocki" <rjw@xxxxxxx>, "linux-acpi@xxxxxxxxxxxxxxx" <linux-acpi@xxxxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxx>
- Delivery-date: Fri, 26 Jul 2013 18:04:39 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 7/25/2013 10:51 PM, Zheng, Lv wrote:
From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx]
Sent: Thursday, July 25, 2013 8:04 PM
CC-ing some of the tboot maintainers.
As what I've said, it's up to the others to determine if the patch is OK.
I just need to make my concerns visible in the community. :-)
If I understand your concerns you don't want the hook to depend on any
of the bit manipulations the existing code does for the pm1 values. The
hook should do it itself case it needs to tweak them or what not.
And it also frees you from altering the ACPICA code without having to
worry about being dependent on what the input values the hook requires?
Is this what you had in mind? (not compile tested nor tested).
Actually I've drafted such a patch that had conflicts with the Xen/tboot hooks.
Ok, so the idea patch is not yet what you had in mind I surmise?
Looking at the patch "ACPICA: Hardware: Modify sleep hardware
interfaces" from said
bugzilla I believe you could still retain the hook. You could even add
it as part of the
state machine (before the values are written to the respective area).
Then the patchset has to first delete the hooks to make test possible for
testers.
It is here:
https://bugzilla.kernel.org/show_bug.cgi?id=54181
I must be missing something obvious but the patches just say "Removes
the hook for debugging purpose."
I think the reason you did that is b/c you were not sure were it would
fit in your "ACPICA: Hardware: Modify sleep hardware interfaces" patch.
But as I said - I am probably missing something obvious.
I am not even sure if outside the drivers/acpi you can call
acpi_hw_get_bit_register_info ..
If we want this patch to be accepted without modification, then someone can
help to do such cleanup in the future when ACPICA change happens.
How would you expose said functions to non-ACPICA drivers?
And since the Xen bits would do the same exact bit manipulation it
probably could use a library to do pm1* stuff so both tboot and Xen
can use it.
This sounds better.
I think Xen and tboot will need such a library to atomically accessing PM
register fields.
The sad part is that it would mirror the same exact code paths that the
existing ACPICA code does. Which is why
it was just passing in the pm* registers. But I understand that you
prefer to have a generic API so that you don't have to worry about
low-level implementations of platform hooks.
What if this hook was allowed to be turned in the state machine? That is
the acpi_sleep_dispatch and
it over-wrote part of the "write to PM1" state? The goal of the hook is
to redirect the write using a different mechanism - so if the 'write to
PM1' is accomplished then that is good.
Thanks and best regards
-Lv
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index f84fe00..59570b1 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -273,20 +273,75 @@ 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_get_pm_control(bool legacy)
+{
+ u32 pm1a_control;
+ u32 pm1b_control;
+ u32 in_value;
+ acpi_status status;
+ struct acpi_bit_register_info *sleep_type_reg_info;
+ struct acpi_bit_register_info *sleep_enable_reg_info;
+
+ if (!legacy)
+ return -ENOSPC;
+
+ status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
+ &pm1a_control);
+ if (ACPI_FAILURE(status)) {
+ return -EXXX /* something */;
+ }
+ sleep_type_reg_info =
acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_TYPE);
+ sleep_enable_reg_info =
acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_ENABLE);
+ /* Clear the SLP_EN and SLP_TYP fields */
+
+ pm1a_control &= ~(sleep_type_reg_info->access_bit_mask |
+ sleep_enable_reg_info->access_bit_mask);
+ pm1b_control = pm1a_control;
+
+ /* Insert the SLP_TYP bits */
+
+ pm1a_control |=
+ (acpi_gbl_sleep_type_a << sleep_type_reg_info->bit_position);
+ pm1b_control |=
+ (acpi_gbl_sleep_type_b << sleep_type_reg_info->bit_position);
+
+ /*
+ * We split the writes of SLP_TYP and SLP_EN to workaround
+ * poorly implemented hardware.
+ */
+
+ /* Write #1: write the SLP_TYP data to the PM1 Control registers */
+
+ status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
+ if (ACPI_FAILURE(status)) {
+ return -EXXX /* something */;
+ }
+
+ /* Insert the sleep enable (SLP_EN) bit */
+
+ pm1a_control |= sleep_enable_reg_info->access_bit_mask;
+ pm1b_control |= sleep_enable_reg_info->access_bit_mask;
+ tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control;
+ tboot->acpi_sinfo.pm1b_cnt_val = pm1b_control;
+ return 0;
+}
+static int tboot_sleep(u8 sleep_state);
{
static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
/* S0,1,2: */ -1, -1, -1,
/* S3: */ TB_SHUTDOWN_S3,
/* S4: */ TB_SHUTDOWN_S4,
/* S5: */ TB_SHUTDOWN_S5 };
+ int rc;
if (!tboot_enabled())
return 0;
tboot_copy_fadt(&acpi_gbl_FADT);
- tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control;
- tboot->acpi_sinfo.pm1b_cnt_val = pm1b_control;
+
+ rc = tboot_get_pm_control();
+ if (rc < 0)
+ return -1;
/* we always use the 32b wakeup vector */
tboot->acpi_sinfo.vector_width = 32;
diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
index 5e5f762..a8e98f9 100644
--- a/drivers/acpi/acpica/hwesleep.c
+++ b/drivers/acpi/acpica/hwesleep.c
@@ -113,6 +113,15 @@ acpi_status acpi_hw_extended_sleep(u8
sleep_state)
!acpi_gbl_FADT.sleep_status.address) {
return_ACPI_STATUS(AE_NOT_EXIST);
}
+ /*
+ * If using tboot or other platforms that need tweaks then
+ * do them here, and also bail out if neccessary.
+ */
+ status = acpi_os_prepare_sleep(sleep_state);
+ if (ACPI_SKIP(status))
+ return_ACPI_STATUS(AE_OK);
+ if (ACPI_FAILURE(status))
+ return_ACPI_STATUS(status);
/* Clear wake status (WAK_STS) */
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index e3828cc..909b23b 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -108,6 +108,16 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
return_ACPI_STATUS(status);
}
+ /*
+ * If using tboot or other platforms that need tweaks then
+ * do them here, and also bail out if neccessary.
+ */
+ status = acpi_os_prepare_sleep(sleep_state);
+ if (ACPI_SKIP(status))
+ return_ACPI_STATUS(AE_OK);
+ if (ACPI_FAILURE(status))
+ return_ACPI_STATUS(status);
+
/* Get current value of PM1A control */
status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
@@ -152,12 +162,6 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
ACPI_FLUSH_CPU_CACHE();
- status = acpi_os_prepare_sleep(sleep_state, pm1a_control,
- pm1b_control);
- if (ACPI_SKIP(status))
- return_ACPI_STATUS(AE_OK);
- if (ACPI_FAILURE(status))
- return_ACPI_STATUS(status);
/* Write #2: Write both SLP_TYP + SLP_EN */
status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e721863..ffcc364 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -77,8 +77,7 @@ 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);
static acpi_osd_handler acpi_irq_handler;
static void *acpi_irq_context;
@@ -1757,13 +1756,11 @@ 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)
{
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);
if (rc < 0)
return AE_ERROR;
else if (rc > 0)
@@ -1772,8 +1769,7 @@ 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))
{
__acpi_os_prepare_sleep = func;
}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 17b5b59..8de1043 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -480,8 +480,7 @@ static inline bool acpi_driver_match_device(struct
device *dev,
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);
+acpi_status acpi_os_prepare_sleep(u8 sleep_state);
#ifdef CONFIG_X86
void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
#else
.. massive snip..
On 07/24/2013 10:38 AM, Moore, Robert wrote:
I haven't found a high-level description of "acpi_os_prepare_sleep",
perhaps I missed it.
Can someone point me to the overall description of this change and why
it is
being considered?
Hi Bob,
For this series, the v6 of this series does a decent job of what it is
trying to accomplish:
https://lkml.org/lkml/2013/7/1/205
However, I recognize that this does not really describe *why*
acpi_os_prepare_sleep is necessary to begin with. For that, we need to
go back a little more.
The summary for the series that introduced it is a good description,
of the reasons it is necessary:
http://lkml.indiana.edu/hypermail/linux/kernel/1112.2/00450.html
In summary though - in the case of Xen (and I believe this is also
true in tboot) a value inappropriate for a VM (which dom0 is a special
case
of) was being written to cr3, and the physical machine would never
come out of S3.
This mechanism gives an os specific hook to do something else down at
the lower levels, while still being able to take advantage of the
large amount of OS independent code in ACPICA.
It might be also prudent to look at original 'hook' that was added by Intel in
the
Linux code to support TXT:
commit 86886e55b273f565935491816c7c96b82469d4f8
Author: Joseph Cihula <joseph.cihula@xxxxxxxxx>
Date: Tue Jun 30 19:31:07 2009 -0700
x86, intel_txt: Intel TXT Sx shutdown support
Support for graceful handling of sleep states (S3/S4/S5) after an
Intel(R)
TXT launch.
Without this patch, attempting to place the system in one of the ACPI
sleep
states (S3/S4/S5) will cause the TXT hardware to treat this as an
attack
and
will cause a system reset, with memory locked. Not only may the
subsequent
memory scrub take some time, but the platform will be unable to
enter
the
requested power state.
This patch calls back into the tboot so that it may properly and
securely
clean
up system state and clear the secrets-in-memory flag, after which it
will
place
the system into the requested sleep state using ACPI information
passed
by the kernel.
arch/x86/kernel/smpboot.c | 2 ++
drivers/acpi/acpica/hwsleep.c | 3 +++
kernel/cpu.c | 7 ++++++-
3 files changed, 11 insertions(+), 1 deletion(-)
Signed-off-by: Joseph Cihula <joseph.cihula@xxxxxxxxx>
Signed-off-by: Shane Wang <shane.wang@xxxxxxxxx>
Signed-off-by: H. Peter Anvin <hpa@xxxxxxxxx>
I suspect that if tboot was used with a different OS (Solaris?) it would hit the
same case and a similar hook would be needed.
Said 'hook' (which was a call to tboot_sleep) was converted to be a more
neutral 'acpi_os_prepare_sleep' which tboot can use (and incidently Xen
too).
I think what Bob is saying that if said hook is neccessary (and I believe it is
-
and
Intel TXT maintainer thinks so too since he added it in the first place), then
the
right way of adding it is via the ACPICA tree.
Should the discussion for this be moved there ?
(https://acpica.org/community)
and an generic 'os_prepare_sleep' patch added in
git://github.com/acpica/acpica.git?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the
body
of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
