Re: [PATCH v14 21/23] x86/virt/tdx: Handle TDX interaction with ACPI S3 and deeper states

[Huang, Kai]

> > 
> > > > 
> > > > -/* Low-level suspend routine. */
> > > > -extern int (*acpi_suspend_lowlevel)(void);
> > > > +typedef int (*acpi_suspend_lowlevel_t)(void);
> > > > +
> > > > +/* Set up low-level suspend routine. */
> > > > +void acpi_set_suspend_lowlevel(acpi_suspend_lowlevel_t func);
> > > 
> > > I'm not sure about the typededf, but I have no strong opinion against it either.
> > > 
> > > > 
> > > >  /* Physical address to resume after wakeup */
> > > >  unsigned long acpi_get_wakeup_address(void);
> > > > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > > > index 2a0ea38955df..95be371305c6 100644
> > > > --- a/arch/x86/kernel/acpi/boot.c
> > > > +++ b/arch/x86/kernel/acpi/boot.c
> > > > @@ -779,11 +779,17 @@ int (*__acpi_register_gsi)(struct device *dev, u32 gsi,
> > > >  void (*__acpi_unregister_gsi)(u32 gsi) = NULL;
> > > > 
> > > >  #ifdef CONFIG_ACPI_SLEEP
> > > > -int (*acpi_suspend_lowlevel)(void) = x86_acpi_suspend_lowlevel;
> > > > +static int (*acpi_suspend_lowlevel)(void) = x86_acpi_suspend_lowlevel;
> > > >  #else
> > > > -int (*acpi_suspend_lowlevel)(void);
> > > > +static int (*acpi_suspend_lowlevel)(void);
> > > 
> > > For the sake of consistency, either use the typedef here, or don't use
> > > it at all.
> > 
> > Ah right.
> > 
> > Since you don't prefer the typedef, I'll abandon it:
> > 
> > E.g,:
> > 
> > void acpi_set_suspend_lowlevel(int (*suspend_lowlevel)(void))
> > {
> >         acpi_suspend_lowlevel = suspend_lowlevel;
> > }
> > 
> > Let me know whether this looks good to you?
> 
> Yes, this is fine with me.
> 

Hi Rafael,

Sorry for the back and forth on this.

With the patch which provides the helper, LKP reported build error:

   drivers/acpi/sleep.c: In function 'acpi_suspend_enter':
>> drivers/acpi/sleep.c:600:22: error: 'acpi_suspend_lowlevel' undeclared (first
use in this function); did you mean 'acpi_set_suspend_lowlevel'?
     600 |                 if (!acpi_suspend_lowlevel)
         |                      ^~~~~~~~~~~~~~~~~~~~~
         |                      acpi_set_suspend_lowlevel

Turns out I disabled both suspend/hibernation in my own kernel build test, sigh.

The common ACPI sleep code requires the ARCH to declare 'acpi_suspend_lowlevel'
in <asm/acpi.h>, and define it somewhere in the ARCH code  too.

So sadly I cannot remove the acpi_suspend_lowlevel variable declaration in
<asm/acpi.h>.  And the ending patch would have both below in <asm/acpi.h>:

   /* Low-level suspend routine. */
   extern int (*acpi_suspend_lowlevel)(void);

  +/* To override @acpi_suspend_lowlevel at early boot */
  +void acpi_set_suspend_lowlevel(int (*suspend_lowlevel)(void));
  +

Thus I am not sure whether it still a good idea to have the helper?

Any comments?  Thanks!

Below is the full patch that I end up for your reference:

From 9372e0278e2a14419d08e8df36e474dc72d93784 Mon Sep 17 00:00:00 2001
From: Kai Huang <kai.huang@xxxxxxxxx>
Date: Thu, 19 Oct 2023 14:37:19 +1300
Subject: [PATCH] x86/acpi: Add a helper to override ACPI lowlevel suspend
 function

ACPI S3 suspend code requires a valid 'acpi_suspend_lowlevel' function
pointer to work.  Each ARCH needs to set the acpi_suspend_lowlevel to
its own implementation to make ACPI S3 suspend work on that ARCH.  X86
implements a default function for that, and Xen PV dom0 overrides it
with its own version during early kernel boot.

Intel Trusted Domain Extensions (TDX) doesn't play nice with ACPI S3.
ACPI S3 suspend will gets disabled during kernel early boot if TDX is
enabled.

Add a helper function to override the acpi_suspend_lowlevel at kernel
early boot, so that the callers don't manipulate the function pointer
directly.  Change the Xen code to use the helper.  It will be used by
TDX code to disable ACPI S3 suspend too.

No functional change is intended.

Signed-off-by: Kai Huang <kai.huang@xxxxxxxxx>
---
 arch/x86/include/asm/acpi.h | 3 +++
 arch/x86/kernel/acpi/boot.c | 9 +++++++--
 include/xen/acpi.h          | 2 +-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index c8a7fc23f63c..6001df87526e 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -63,6 +63,9 @@ static inline void acpi_disable_pci(void)
 /* Low-level suspend routine. */
 extern int (*acpi_suspend_lowlevel)(void);

+/* To override @acpi_suspend_lowlevel at early boot */
+void acpi_set_suspend_lowlevel(int (*suspend_lowlevel)(void));
+
 /* Physical address to resume after wakeup */
 unsigned long acpi_get_wakeup_address(void);

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 2a0ea38955df..e9143a8a0350 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -779,11 +779,16 @@ int (*__acpi_register_gsi)(struct device *dev, u32 gsi,
 void (*__acpi_unregister_gsi)(u32 gsi) = NULL;

 #ifdef CONFIG_ACPI_SLEEP
-int (*acpi_suspend_lowlevel)(void) = x86_acpi_suspend_lowlevel;
+static int (*acpi_suspend_lowlevel)(void) = x86_acpi_suspend_lowlevel;
 #else
-int (*acpi_suspend_lowlevel)(void);
+static int (*acpi_suspend_lowlevel)(void);
 #endif

+void __init acpi_set_suspend_lowlevel(int (*suspend_lowlevel)(void))
+{
+       acpi_suspend_lowlevel = suspend_lowlevel;
+}
+
 /*
  * success: return IRQ number (>=0)
  * failure: return < 0
diff --git a/include/xen/acpi.h b/include/xen/acpi.h
index b1e11863144d..81a1b6ee8fc2 100644
--- a/include/xen/acpi.h
+++ b/include/xen/acpi.h
@@ -64,7 +64,7 @@ static inline void xen_acpi_sleep_register(void)
                acpi_os_set_prepare_extended_sleep(
                        &xen_acpi_notify_hypervisor_extended_sleep);

-               acpi_suspend_lowlevel = xen_acpi_suspend_lowlevel;
+               acpi_set_suspend_lowlevel(xen_acpi_suspend_lowlevel);
        }
 }
 #else
-- 
2.41.0