Let me just give an example to let you know the difficulties for ACPICA developers to merge Xen's acpi_os_prepare_sleep.
The original logic in the acpi_hw_legacy_sleep is:
111 /* Get current value of PM1A control */
112
113 status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
114 &pm1a_control);
115 if (ACPI_FAILURE(status)) {
116 return_ACPI_STATUS(status);
117 }
118 ACPI_DEBUG_PRINT((ACPI_DB_INIT,
119 "Entering sleep state [S%u]\n", sleep_state));
120
121 /* Clear the SLP_EN and SLP_TYP fields */
122
123 pm1a_control &= ~(sleep_type_reg_info->access_bit_mask |
124 sleep_enable_reg_info->access_bit_mask);
125 pm1b_control = pm1a_control;
126
127 /* Insert the SLP_TYP bits */
128
129 pm1a_control |=
130 (acpi_gbl_sleep_type_a << sleep_type_reg_info->bit_position);
131 pm1b_control |=
132 (acpi_gbl_sleep_type_b << sleep_type_reg_info->bit_position);
133
134 /*
135 * We split the writes of SLP_TYP and SLP_EN to workaround
136 * poorly implemented hardware.
137 */
138
139 /* Write #1: write the SLP_TYP data to the PM1 Control registers */
140
141 status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
142 if (ACPI_FAILURE(status)) {
143 return_ACPI_STATUS(status);
144 }
145
146 /* Insert the sleep enable (SLP_EN) bit */
147
148 pm1a_control |= sleep_enable_reg_info->access_bit_mask;
149 pm1b_control |= sleep_enable_reg_info->access_bit_mask;
150
151 /* Flush caches, as per ACPI specification */
152
153 ACPI_FLUSH_CPU_CACHE();
154
=======
[Now Xen's hook appears here)
=======
161 /* Write #2: Write both SLP_TYP + SLP_EN */
162
163 status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
164 if (ACPI_FAILURE(status)) {
165 return_ACPI_STATUS(status);
166 }
If the whole block is re-implemented by ACPICA in the future:
Acpi_hw_write_field_register(ACPI_SLEEP_REGISTER, ACPI_SLP_TYPE | ACPI_SLP_EN, slp_type | slp_en);
Then where should ACPICA put this hook under the new design?
Can it go inside acpi_hw_write_field_register?
If the hook is in the current position, then future ACPICA development work on the suspend/resume codes are likely broken.
IMO,
1. acpi_os_preapre_sleep() should be put before Line 111
2. acpi_os_preapre_sleep()'s parameters should be re-designed
3. Xen only register hacking logic should be put inside acpi_os_prepare_sleep().
Hope the above example can make my concern clearer now. :-)
Thanks
-Lv
> -----Original Message-----
> From: linux-acpi-owner@xxxxxxxxxxxxxxx
> [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] On Behalf Of Konrad Rzeszutek Wilk
> Sent: Thursday, July 25, 2013 12:32 AM
> To: Ben Guthro
> Cc: Moore, Robert; Zheng, Lv; Jan Beulich; Rafael J . Wysocki;
> linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx;
> xen-devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced
> hardware sleep path
>
> On Wed, Jul 24, 2013 at 11:14:06AM -0400, Ben Guthro wrote:
> >
> >
> > 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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
