Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced hardware sleep path

[Ben Guthro <benjamin.guthro@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7844 bytes --]

I'm afraid this is well outside of the scope of the bug I was trying to fix.
Given the interactions with the acpi code I have had so far - I am somewhat
disinclined to make such sweeping changes.

I guess any distro supporting Xen, or tboot will have to carry a patch to
avoid such a bug.


On Wed, Jul 24, 2013 at 9:28 PM, Zheng, Lv <lv.zheng@intel.com> wrote:

> 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@vger.kernel.org
> > [mailto:linux-acpi-owner@vger.kernel.org] 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@vger.kernel.org; linux-acpi@vger.kernel.org;
> > xen-devel@lists.xen.org
> > 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@intel.com>
> > 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@intel.com>
> >     Signed-off-by: Shane Wang <shane.wang@intel.com>
> >     Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> >
> > 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@vger.kernel.org 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[-- Attachment #1.2: Type: text/html, Size: 10337 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel