[PATCH v1 1/1] x86: Skip WBINVD instruction for VM guest

[PATCH v1 1/1] x86: Skip WBINVD instruction for VM guest

[Kuppuswamy Sathyanarayanan]

ACPI mandates that CPU caches be flushed before entering any sleep
state. This ensures that the CPU and its caches can be powered down
without losing data.

ACPI-based VMs have maintained this sleep-state-entry behavior.
However, cache flushing for VM sleep state entry is useless. Unlike on
bare metal, guest sleep states are not correlated with potential data
loss of any kind; the host is responsible for data preservation. In
fact, some KVM configurations simply skip the cache flushing
instruction (see need_emulate_wbinvd()).

Further, on TDX systems, the WBINVD instruction causes an
unconditional #VE exception.  If this cache flushing remained, it would
need extra code in the form of a #VE handler.

All use of ACPI_FLUSH_CPU_CACHE() appears to be in sleep-state-related
code.

This means that the ACPI use of WBINVD is at *best* superfluous.

Disable ACPI CPU cache flushing on all X86_FEATURE_HYPERVISOR systems,
which includes TDX.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: linux-acpi@vger.kernel.org
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/include/asm/acenv.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/acenv.h b/arch/x86/include/asm/acenv.h
index 9aff97f0de7f..d4162e94bee8 100644
--- a/arch/x86/include/asm/acenv.h
+++ b/arch/x86/include/asm/acenv.h
@@ -10,10 +10,15 @@
 #define _ASM_X86_ACENV_H
 
 #include <asm/special_insns.h>
+#include <asm/cpu.h>
 
 /* Asm macros */
 
-#define ACPI_FLUSH_CPU_CACHE()	wbinvd()
+#define ACPI_FLUSH_CPU_CACHE()				\
+do {							\
+	if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))	\
+		wbinvd();				\
+} while (0)
 
 int __acpi_acquire_global_lock(unsigned int *lock);
 int __acpi_release_global_lock(unsigned int *lock);
-- 
2.25.1

Re: [PATCH v1 1/1] x86: Skip WBINVD instruction for VM guest

[Borislav Petkov]

On Mon, Nov 15, 2021 at 04:50:27PM -0800, Kuppuswamy Sathyanarayanan wrote:
> -#define ACPI_FLUSH_CPU_CACHE()	wbinvd()
> +#define ACPI_FLUSH_CPU_CACHE()				\
> +do {							\
> +	if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))	\

cpu_feature_enabled()

If you wanna query a X86_FEATURE_* bit, from now on, use only this
function.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v1 1/1] x86: Skip WBINVD instruction for VM guest

[Sathyanarayanan Kuppuswamy]

Hi,

On 11/16/21 8:24 AM, Borislav Petkov wrote:
> On Mon, Nov 15, 2021 at 04:50:27PM -0800, Kuppuswamy Sathyanarayanan wrote:
>> -#define ACPI_FLUSH_CPU_CACHE()	wbinvd()
>> +#define ACPI_FLUSH_CPU_CACHE()				\
>> +do {							\
>> +	if (!boot_cpu_has(X86_FEATURE_HYPERVISOR))	\
> 
> cpu_feature_enabled()
> 
> If you wanna query a X86_FEATURE_* bit, from now on, use only this
> function.
> 

Ok. I will change it in next version.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

[PATCH v2] x86: Skip WBINVD instruction for VM guest

[Kuppuswamy Sathyanarayanan]

ACPI mandates that CPU caches be flushed before entering any sleep
state. This ensures that the CPU and its caches can be powered down
without losing data.

ACPI-based VMs have maintained this sleep-state-entry behavior.
However, cache flushing for VM sleep state entry is useless. Unlike on
bare metal, guest sleep states are not correlated with potential data
loss of any kind; the host is responsible for data preservation. In
fact, some KVM configurations simply skip the cache flushing
instruction (see need_emulate_wbinvd()).

Further, on TDX systems, the WBINVD instruction causes an
unconditional #VE exception.  If this cache flushing remained, it would
need extra code in the form of a #VE handler.

All use of ACPI_FLUSH_CPU_CACHE() appears to be in sleep-state-related
code.

This means that the ACPI use of WBINVD is at *best* superfluous.

Disable ACPI CPU cache flushing on all X86_FEATURE_HYPERVISOR systems,
which includes TDX.

Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: linux-acpi@vger.kernel.org
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v1:
 * Used cpu_feature_enabled() instead of boot_cpu_has().

 arch/x86/include/asm/acenv.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/acenv.h b/arch/x86/include/asm/acenv.h
index 9aff97f0de7f..dba05c74bd7e 100644
--- a/arch/x86/include/asm/acenv.h
+++ b/arch/x86/include/asm/acenv.h
@@ -10,10 +10,15 @@
 #define _ASM_X86_ACENV_H
 
 #include <asm/special_insns.h>
+#include <asm/cpu.h>
 
 /* Asm macros */
 
-#define ACPI_FLUSH_CPU_CACHE()	wbinvd()
+#define ACPI_FLUSH_CPU_CACHE()					\
+do {								\
+	if (!cpu_feature_enabled(X86_FEATURE_HYPERVISOR))	\
+		wbinvd();					\
+} while (0)
 
 int __acpi_acquire_global_lock(unsigned int *lock);
 int __acpi_release_global_lock(unsigned int *lock);
-- 
2.25.1

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Thomas Gleixner]

Kuppuswamy,

On Thu, Nov 18 2021 at 20:03, Kuppuswamy Sathyanarayanan wrote:
> ACPI mandates that CPU caches be flushed before entering any sleep
> state. This ensures that the CPU and its caches can be powered down
> without losing data.
>
> ACPI-based VMs have maintained this sleep-state-entry behavior.
> However, cache flushing for VM sleep state entry is useless. Unlike on
> bare metal, guest sleep states are not correlated with potential data
> loss of any kind; the host is responsible for data preservation. In
> fact, some KVM configurations simply skip the cache flushing
> instruction (see need_emulate_wbinvd()).

KVM starts out with kvm->arch.noncoherent_dma_count = 0 which makes
need_emulate_wbinvd() skip WBINVD emulation. So far so good.

VFIO has code to invoke kvm_arch_register_noncoherent_dma() which
increments the count which will subsequently cause WBINVD emulation to
be enabled. What now?

> Further, on TDX systems, the WBINVD instruction causes an
> unconditional #VE exception.  If this cache flushing remained, it would
> need extra code in the form of a #VE handler.
>
> All use of ACPI_FLUSH_CPU_CACHE() appears to be in sleep-state-related
> code.

C3 is considered a sleep state nowadays? Also ACPI_FLUSH_CPU_CACHE() is
used in other places which have nothing to do with sleep states.

git grep is not rocket science to use.

> This means that the ACPI use of WBINVD is at *best* superfluous.

Really? You probably meant to say:

  This means that the ACPI usage of WBINVD from within a guest is at
  best superfluous.

No?

But aside of that this does not give any reasonable answers why
disabling WBINVD for guests unconditionally in ACPI_FLUSH_CPU_CACHE()
and the argumentation vs. need_emulate_wbinvd() are actually correct
under all circumstances.

I'm neither going to do that analysis nor am I going to accept a patch
which comes with 'appears' based arguments and some handwavy references
to disabled WBINVD emulation code which can obviously be enabled for a
reason.

The even more interesting question for me is how a TDX guest is dealing
with all other potential invocations of WBINVD all over the place. Are
they all going to get the same treatment or are those magically going to
be never executed in TDX guests?

I really have to ask why SEV can deal with WBINVD and other things just
nicely by implementing trivial #VC handler functions, but TDX has to
prematurely optimize the kernel tree based on half baken arguments?

Having a few trivial #VE handlers is not the end of the world. You can
revisit that once basic support for TDX is merged in order to gain
performance or whatever.

Either that or you provide patches with arguments which are based on
proper analysis and not on 'appears to' observations.

Thanks,

        tglx

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Kirill A. Shutemov]

On Thu, Nov 25, 2021 at 01:40:24AM +0100, Thomas Gleixner wrote:
> Kuppuswamy,
> 
> On Thu, Nov 18 2021 at 20:03, Kuppuswamy Sathyanarayanan wrote:
> > ACPI mandates that CPU caches be flushed before entering any sleep
> > state. This ensures that the CPU and its caches can be powered down
> > without losing data.
> >
> > ACPI-based VMs have maintained this sleep-state-entry behavior.
> > However, cache flushing for VM sleep state entry is useless. Unlike on
> > bare metal, guest sleep states are not correlated with potential data
> > loss of any kind; the host is responsible for data preservation. In
> > fact, some KVM configurations simply skip the cache flushing
> > instruction (see need_emulate_wbinvd()).
> 
> KVM starts out with kvm->arch.noncoherent_dma_count = 0 which makes
> need_emulate_wbinvd() skip WBINVD emulation. So far so good.
> 
> VFIO has code to invoke kvm_arch_register_noncoherent_dma() which
> increments the count which will subsequently cause WBINVD emulation to
> be enabled. What now?
> 
> > Further, on TDX systems, the WBINVD instruction causes an
> > unconditional #VE exception.  If this cache flushing remained, it would
> > need extra code in the form of a #VE handler.
> >
> > All use of ACPI_FLUSH_CPU_CACHE() appears to be in sleep-state-related
> > code.
> 
> C3 is considered a sleep state nowadays? Also ACPI_FLUSH_CPU_CACHE() is
> used in other places which have nothing to do with sleep states.
> 
> git grep is not rocket science to use.
> 
> > This means that the ACPI use of WBINVD is at *best* superfluous.
> 
> Really? You probably meant to say:
> 
>   This means that the ACPI usage of WBINVD from within a guest is at
>   best superfluous.
> 
> No?
> 
> But aside of that this does not give any reasonable answers why
> disabling WBINVD for guests unconditionally in ACPI_FLUSH_CPU_CACHE()
> and the argumentation vs. need_emulate_wbinvd() are actually correct
> under all circumstances.
> 
> I'm neither going to do that analysis nor am I going to accept a patch
> which comes with 'appears' based arguments and some handwavy references
> to disabled WBINVD emulation code which can obviously be enabled for a
> reason.
> 
> The even more interesting question for me is how a TDX guest is dealing
> with all other potential invocations of WBINVD all over the place. Are
> they all going to get the same treatment or are those magically going to
> be never executed in TDX guests?
> 
> I really have to ask why SEV can deal with WBINVD and other things just
> nicely by implementing trivial #VC handler functions, but TDX has to
> prematurely optimize the kernel tree based on half baken arguments?
> 
> Having a few trivial #VE handlers is not the end of the world. You can
> revisit that once basic support for TDX is merged in order to gain
> performance or whatever.
> 
> Either that or you provide patches with arguments which are based on
> proper analysis and not on 'appears to' observations.

I think the right solution to the WBINVD would be to add a #VE handler
that does nothing. We don't have a reasonable way to handle it from within
the guest. We can call the VMM in hope that it would handle it, but VMM is
untrusted and it can ignore the request.

Dave suggested that we need to do code audit to make sure that there's no
user inside TDX guest environment that relies on WBINVD to work correctly.

Below is full call tree of WBINVD. It is substantially larger than I
anticipated from initial grep.

Conclusions:

  - Most of callers are in ACPI code on changing S-states. Ignoring cache
    flush for S-state change on virtual machine should be safe.

  - The only WBINVD I was able to trigger is on poweroff from ACPI code.
    Reboot also should trigger it, but for some reason I don't see it.

  - Few caller in CPU offline code. TDX does not allowed to offline CPU as
    we cannot bring it back -- we don't have SIPI. And even if offline
    works for vCPU it should be safe to ignore WBINVD there.

  - NVDIMMs are not supported inside TDX. If it will change we would need
    to deal with cache flushing for this case. Hopefully, we would be able
    to avoid WBINVD.

  - Cache QoS and MTRR use WBINVD. They are disabled in TDX, but it is
    controlled by VMM if the feature is advertised. We would need to
    filter CPUID/MSRs to make sure VMM would not mess with them.

Is it good enough justification for do-nothing #VE WBINVD handler?

WBINVD
  native_wbinvd()
    wbinvd()
      ACPI_FLUSH_CPU_CACHE()
        acpi_hw_extended_sleep()
          acpi_enter_sleep_state()
            x86_acpi_enter_sleep_state()
              do_suspend_lowlevel()
                x86_acpi_suspend_lowlevel()
                  acpi_suspend_enter()
                    >>> On S3: No suspend-to-ram -- no problem
            acpi_db_do_one_sleep_state()
              acpi_db_sleep()
                acpi_db_command_dispatch()
                  >>> "SLEEP" command of ACPI debugger. I guess can trigger poweroff. WBINVD doesn't make any difference in TDX.
            acpi_hibernation_enter()
              >>> On S4. No hibernate -- no problem.
            acpi_power_off()
              >>> On S5. Triggirable on poweroff, but safe to ignore WBINVD here on TDX
            acpi_suspend_enter()
              >>> On S1. No S1 -- no problem.
            xen_acpi_suspend_lowlevel()
              >>> N/A to TDX.
        acpi_hw_legacy_sleep()
          acpi_enter_sleep_state()
            >>> See above. For ACPI_REDUCED_HARDWARE.
        acpi_enter_sleep_state_s4bios()
          No users? Or I failed to decypther ACPI code.
        acpi_idle_enter()
          acpi_processor_setup_cstates()
            acpi_processor_setup_cpuidle_states()
              acpi_processor_power_state_has_changed()
                acpi_processor_notify()
                  >>> Looks like the driver going to get event in case the number of power state will change. But I can be mistaken. Anyway skipping WBINVD is safe.
              acpi_processor_power_init()
                >>> Only applicable if acpi_idle_driver is in use. N/A to TDX.
        acpi_idle_enter_s2idle()
          acpi_processor_setup_cstates()
            >>> See above.
        acpi_idle_play_dead()
          acpi_processor_setup_cstates()
            >>> See above.
        acpi_sleep_prepare()
          >>> On the way to S3/S4/S5. Safe to ignore WBINVD
        acpi_suspend_enter()
          >>> On the way to S3/S4/S5. Safe to ignore WBINVD
        acpi_hibernation_enter()
          >>> On S4, No S4 -- no problem.
        <Bunch of callers in cpufreq/longhaul.c>
          >>> CPU frequency driver for VIA Cyrix CPU. N/A to TDX.
      flush_agp_cache()
        ipi_handler()
          global_cache_flush()
            >>> Used by bunch of random AGP drivers. N/A to TDX: device passthrough is not supported.
      wbinvd_on_cpu()
        amd_l3_disable_index()
          >>> N/A to TDX
      gart_iommu_init()
        >>> N/A to TDX
      init_amd_k6()
        >>> N/A to TDX
      amd_set_mtrr()
        >>> N/A to TDX
      prepare_set() in mtrr/cyrix.c
        >>> N/A to TDX
      post_set() in mtrr/cyrix.c
        >>> N/A to TDX
      prepare_set() mtrr/generic.c
        >>> MTRR is disabled, but it is in control of VMM.
      mwait_play_dead()
        native_play_dead()
          sev_es_play_dead()
            >>> N/A to TDX.
          play_dead()
            arch_cpu_idle_enter()
              do_idle()
                >>> Only for offline CPUs. Offlining is disabled on TDX.
      hlt_play_dead()
        native_play_dead()
          >>> See above
        resume_play_dead()
          hibernate_resume_nonboot_cpu_disable()
            >>> No hipernate -- no problem.
      pseudo_lock_fn()
        rdtgroup_pseudo_lock_create()
          rdtgroup_schemata_write()
            res_common_files[]
              rdtgroup_init()
                resctrl_late_init()
                  >>> Depends on Cache QoS features that configured by VMM.
      wbinvd_ipi() in kvm/x86.c
        >>> KVM emulation of WBINVD. N/A for TDX guest.
      __wbinvd()
        wbinvd_on_cpu()
          >>> See above
        wbinvd_on_all_cpus()
          sev_flush_asids() and other users in kvm/svm/sev.c
            >>> N/A to TDX
          nvdimm_invalidate_cache()
            >>> No NVDIMMs in TDX
          i830_chipset_flush()
            >>> N/A to TDX
          __sev_platform_init_locked()
            >>> N/A to TDX
          drm_clflush_virt_range(), drm_clflush_pages(), drm_clflush_sg()
            >>> Only for !X86_FEATURE_CLFLUSH, N/A to TDX.
          Few callers in i915
            >>> N/A to TDX
      __sme_early_enc_dec()
        >>> N/A to TDX
      __cpa_flush_all()
        cpa_flush_all()
          cpa_flush()
            >>> Only for !X86_FEATURE_CLFLUSH. N/A to TDX.
      powernow_k6_set_cpu_multiplier()
        >>> N/A to TDX
      disable_caches()
        inject_write_store() in amd64_edac.c
          >>> N/A to TDX
      drm_ati_pcigart_init()
        >>> N/A to TDX
      nettel_init() and other nettel users
        >>> N/A to TDX
      atomisp_acc_start() and other atomisp users
        >>> N/A to TDX
    apply_microcode_intel()/apply_microcode_early()
      >>> N/A to TDX
  identity_mapped()
    >>> Only for AMD SME
  __enc_copy in /mem_encrypt_boot.S
    >>> N/A to TDX
  wakeup_start in platform/olpc/xo1-wakeup.S
    >>> N/A to TDX
  machine_real_restart_asm16 in realmode/rm/reboot.S
    >>> Safe to ignore WBINVD on TDX
  trampoline_start in realmode/rm/trampoline_64.S
    >>> TDX doesn't use realmode trampoline
  flush_cache() in i810_main.h
    >>> N/A to TDX
-- 
 Kirill A. Shutemov

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Dave Hansen]

On 12/2/21 2:21 PM, Kirill A. Shutemov wrote:
>   - NVDIMMs are not supported inside TDX. If it will change we would need
>     to deal with cache flushing for this case. Hopefully, we would be able
>     to avoid WBINVD.

Maybe we can use this as an example since we have our friendly NVDIMM
developers on cc already.

Let's say that tomorrow Intel decides that NVDIMMs are OK to use in TDX.
 It might not be a good idea, but Intel could arbitrarily start
supporting them immediately.  Further, someone could take today's kernel
and stick it on some future, fancy platform which does support TDX and
NVDIMMs.  In other words, there are multiple reasons we can't just say
"TDX doesn't support NVDIMMs" and forget about it.

If either of those happened, we'd have a NVDIMM driver which uses
WBINVD, expects cache flushing and subsequently loses data.  I think we
can all agree that's a bad idea.

So, we've got two different cases that land in the #VE handler:

	1. Silly ACPI code that doesn't need WBINVD behavior

	2. Less silly NVDIMM code that badly needs WBINVD behavior

... but we have a #VE handler that can't tell the difference.

To me, that says we need to do _something_ different than just papering
over the WBINVD in the #VE handler.

Does anyone have a different take on it?

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Thomas Gleixner]

Kirill,

On Fri, Dec 03 2021 at 01:21, Kirill A. Shutemov wrote:
> On Thu, Nov 25, 2021 at 01:40:24AM +0100, Thomas Gleixner wrote:
>> Kuppuswamy,
>> Either that or you provide patches with arguments which are based on
>> proper analysis and not on 'appears to' observations.
>
> I think the right solution to the WBINVD would be to add a #VE handler
> that does nothing. We don't have a reasonable way to handle it from within
> the guest. We can call the VMM in hope that it would handle it, but VMM is
> untrusted and it can ignore the request.
>
> Dave suggested that we need to do code audit to make sure that there's no
> user inside TDX guest environment that relies on WBINVD to work correctly.
>
> Below is full call tree of WBINVD. It is substantially larger than I
> anticipated from initial grep.
>
> Conclusions:
>
>   - Most of callers are in ACPI code on changing S-states. Ignoring cache
>     flush for S-state change on virtual machine should be safe.
>
>   - The only WBINVD I was able to trigger is on poweroff from ACPI code.
>     Reboot also should trigger it, but for some reason I don't see it.
>
>   - Few caller in CPU offline code. TDX does not allowed to offline CPU as
>     we cannot bring it back -- we don't have SIPI. And even if offline
>     works for vCPU it should be safe to ignore WBINVD there.
>
>   - NVDIMMs are not supported inside TDX. If it will change we would need
>     to deal with cache flushing for this case. Hopefully, we would be able
>     to avoid WBINVD.
>
>   - Cache QoS and MTRR use WBINVD. They are disabled in TDX, but it is
>     controlled by VMM if the feature is advertised. We would need to
>     filter CPUID/MSRs to make sure VMM would not mess with them.
>
> Is it good enough justification for do-nothing #VE WBINVD handler?

first of all thank you very much for this very profound analysis.

This is really what I was asking for and you probably went even a step
deeper than that. Very appreciated.

What we should do instead of doing a wholesale let's ignore WBINVD is to
have a separate function/macro:

 ACPI_FLUSH_CPU_CACHE_PHYS()

and invoke that from the functions which are considered to be safe.

That would default to ACPI_FLUSH_CPU_CACHE() for other architecures
obviously.

Then you can rightfully do:

#define ACPI_FLUSH_CPU_CACHE_PHYS()     \
        if (!cpu_feature_enabled(XXX))	\
        	wbinvd();               \              
                
where $XXX might be FEATURE_TDX_GUEST for paranoia sake and then
extended to X86_FEATURE_HYPERVISOR if everyone agrees.

Then you have the #VE handler which just acts on any other wbinvd
invocation via warn, panic, whatever, no?

Thanks,

        tglx

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Kirill A. Shutemov]

On Fri, Dec 03, 2021 at 12:48:43AM +0100, Thomas Gleixner wrote:
> Kirill,
> 
> On Fri, Dec 03 2021 at 01:21, Kirill A. Shutemov wrote:
> > On Thu, Nov 25, 2021 at 01:40:24AM +0100, Thomas Gleixner wrote:
> >> Kuppuswamy,
> >> Either that or you provide patches with arguments which are based on
> >> proper analysis and not on 'appears to' observations.
> >
> > I think the right solution to the WBINVD would be to add a #VE handler
> > that does nothing. We don't have a reasonable way to handle it from within
> > the guest. We can call the VMM in hope that it would handle it, but VMM is
> > untrusted and it can ignore the request.
> >
> > Dave suggested that we need to do code audit to make sure that there's no
> > user inside TDX guest environment that relies on WBINVD to work correctly.
> >
> > Below is full call tree of WBINVD. It is substantially larger than I
> > anticipated from initial grep.
> >
> > Conclusions:
> >
> >   - Most of callers are in ACPI code on changing S-states. Ignoring cache
> >     flush for S-state change on virtual machine should be safe.
> >
> >   - The only WBINVD I was able to trigger is on poweroff from ACPI code.
> >     Reboot also should trigger it, but for some reason I don't see it.
> >
> >   - Few caller in CPU offline code. TDX does not allowed to offline CPU as
> >     we cannot bring it back -- we don't have SIPI. And even if offline
> >     works for vCPU it should be safe to ignore WBINVD there.
> >
> >   - NVDIMMs are not supported inside TDX. If it will change we would need
> >     to deal with cache flushing for this case. Hopefully, we would be able
> >     to avoid WBINVD.
> >
> >   - Cache QoS and MTRR use WBINVD. They are disabled in TDX, but it is
> >     controlled by VMM if the feature is advertised. We would need to
> >     filter CPUID/MSRs to make sure VMM would not mess with them.
> >
> > Is it good enough justification for do-nothing #VE WBINVD handler?
> 
> first of all thank you very much for this very profound analysis.
> 
> This is really what I was asking for and you probably went even a step
> deeper than that. Very appreciated.
> 
> What we should do instead of doing a wholesale let's ignore WBINVD is to
> have a separate function/macro:
> 
>  ACPI_FLUSH_CPU_CACHE_PHYS()
> 
> and invoke that from the functions which are considered to be safe.
> 
> That would default to ACPI_FLUSH_CPU_CACHE() for other architecures
> obviously.
> 
> Then you can rightfully do:
> 
> #define ACPI_FLUSH_CPU_CACHE_PHYS()     \
>         if (!cpu_feature_enabled(XXX))	\
>         	wbinvd();               \              
>                 
> where $XXX might be FEATURE_TDX_GUEST for paranoia sake and then
> extended to X86_FEATURE_HYPERVISOR if everyone agrees.
> 
> Then you have the #VE handler which just acts on any other wbinvd
> invocation via warn, panic, whatever, no?

I found another angle at the problem. According to the ACPI spec v6.4
section 16.2 cache flushing is required on the way to S1, S2 and S3.
And according to 8.2 it also is required on the way to C3.

TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.

Adjusting code to match the spec would make TDX work automagically.

Any opinions on the patch below?

I didn't touch ACPI_FLUSH_CPU_CACHE() users in cpufreq/longhaul.c because
it might be outside of ACPI spec, I donno.

diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
index 808fdf54aeeb..b004a72a426e 100644
--- a/drivers/acpi/acpica/hwesleep.c
+++ b/drivers/acpi/acpica/hwesleep.c
@@ -104,7 +104,8 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state)
 
 	/* Flush caches, as per ACPI specification */
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (sleep_state >= ACPI_STATE_S1 && sleep_state <= ACPI_STATE_S3)
+		ACPI_FLUSH_CPU_CACHE();
 
 	status = acpi_os_enter_sleep(sleep_state, sleep_control, 0);
 	if (status == AE_CTRL_TERMINATE) {
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index 34a3825f25d3..bfcd66efeb48 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -110,7 +110,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
 
 	/* Flush caches, as per ACPI specification */
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (sleep_state >= ACPI_STATE_S1 && sleep_state <= ACPI_STATE_S3)
+		ACPI_FLUSH_CPU_CACHE();
 
 	status = acpi_os_enter_sleep(sleep_state, pm1a_control, pm1b_control);
 	if (status == AE_CTRL_TERMINATE) {
diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
index e4cde23a2906..ba77598ee43e 100644
--- a/drivers/acpi/acpica/hwxfsleep.c
+++ b/drivers/acpi/acpica/hwxfsleep.c
@@ -162,8 +162,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
 		return_ACPI_STATUS(status);
 	}
 
-	ACPI_FLUSH_CPU_CACHE();
-
 	status = acpi_hw_write_port(acpi_gbl_FADT.smi_command,
 				    (u32)acpi_gbl_FADT.s4_bios_request, 8);
 	if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 76ef1bcc8848..01495aca850e 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 {
 	struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (cx->type == ACPI_STATE_C3)
+		ACPI_FLUSH_CPU_CACHE();
 
 	while (1) {
 
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index eaa47753b758..a81d08b762c2 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -73,7 +73,9 @@ static int acpi_sleep_prepare(u32 acpi_state)
 		acpi_set_waking_vector(acpi_wakeup_address);
 
 	}
-	ACPI_FLUSH_CPU_CACHE();
+
+	if (acpi_state >= ACPI_STATE_S1 && acpi_state <= ACPI_STATE_S3)
+		ACPI_FLUSH_CPU_CACHE();
 #endif
 	pr_info("Preparing to enter system sleep state S%d\n", acpi_state);
 	acpi_enable_wakeup_devices(acpi_state);
@@ -566,7 +568,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	u32 acpi_state = acpi_target_sleep_state;
 	int error;
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (acpi_state >= ACPI_STATE_S1 && acpi_state <= ACPI_STATE_S3)
+		ACPI_FLUSH_CPU_CACHE();
 
 	trace_suspend_resume(TPS("acpi_suspend"), acpi_state, true);
 	switch (acpi_state) {
@@ -903,8 +906,6 @@ static int acpi_hibernation_enter(void)
 {
 	acpi_status status = AE_OK;
 
-	ACPI_FLUSH_CPU_CACHE();
-
 	/* This shouldn't return.  If it returns, we have a problem */
 	status = acpi_enter_sleep_state(ACPI_STATE_S4);
 	/* Reprogram control registers */
-- 
 Kirill A. Shutemov

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Dave Hansen]

On 12/3/21 3:49 PM, Kirill A. Shutemov wrote:
> -	ACPI_FLUSH_CPU_CACHE();
> +	if (acpi_state >= ACPI_STATE_S1 && acpi_state <= ACPI_STATE_S3)
> +		ACPI_FLUSH_CPU_CACHE();

It's a bit of a bummer that this per-sleep-state logic has to be
repeated so many time.

If you pass acpi_state into ACPI_FLUSH_CPU_CACHE() can you centralize
the set of places where that knowledge about which sleep states require
flushing?

> TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.

This makes me a bit nervous.  Is this "the first TDX implementation
supports..." or "the TDX architecture *prohibits* supporting S1 (or
whatever"?

I really think we need some kind of architecture guarantee.  Without
that, we risk breaking things if someone at our employer simply changes
their mind.

The:

> #define ACPI_FLUSH_CPU_CACHE_PHYS()     \
>         if (!cpu_feature_enabled(XXX))	\
>         	wbinvd();               \  

does seem simpler and less error-prone than this, though.

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Kirill A. Shutemov]

On Fri, Dec 03, 2021 at 04:20:34PM -0800, Dave Hansen wrote:
> On 12/3/21 3:49 PM, Kirill A. Shutemov wrote:
> > -	ACPI_FLUSH_CPU_CACHE();
> > +	if (acpi_state >= ACPI_STATE_S1 && acpi_state <= ACPI_STATE_S3)
> > +		ACPI_FLUSH_CPU_CACHE();
> 
> It's a bit of a bummer that this per-sleep-state logic has to be
> repeated so many time.
> 
> If you pass acpi_state into ACPI_FLUSH_CPU_CACHE() can you centralize
> the set of places where that knowledge about which sleep states require
> flushing?

Yes, sure, it is doable. It we decide that it is the way to go.

> > TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.
> 
> This makes me a bit nervous.  Is this "the first TDX implementation
> supports..." or "the TDX architecture *prohibits* supporting S1 (or
> whatever"?

TDX Virtual Firmware Design Guide only states that "ACPI S3 (not supported
by TDX guests)".

Kernel reports in dmesg "ACPI: PM: (supports S0 S5)".

But I don't see how any state beyond S0 and S5 make sense in TDX context.
Do you?

I find it neat that adjusting ACPI code to conform the spec makes TDX
work.

> I really think we need some kind of architecture guarantee.  Without
> that, we risk breaking things if someone at our employer simply changes
> their mind.

Guarantees are hard.

If somebody change their mind we will get unexpected #VE and crash.
I think it is acceptable way to handle unexpected change in confidential
computing environment.

> The:
> 
> > #define ACPI_FLUSH_CPU_CACHE_PHYS()     \
> >         if (!cpu_feature_enabled(XXX))	\
> >         	wbinvd();               \  
> 
> does seem simpler and less error-prone than this, though.

If it it the way to go, I can make a patch.

But there's no reason to have ACPI_FLUSH_CPU_CACHE_PHYS in addition to
ACPI_FLUSH_CPU_CACHE. All ACPI_FLUSH_CPU_CACHE can skip cache flush on
TDX.

-- 
 Kirill A. Shutemov

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Rafael J. Wysocki]

On Sat, Dec 4, 2021 at 12:49 AM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Fri, Dec 03, 2021 at 12:48:43AM +0100, Thomas Gleixner wrote:
> > Kirill,
> >
> > On Fri, Dec 03 2021 at 01:21, Kirill A. Shutemov wrote:
> > > On Thu, Nov 25, 2021 at 01:40:24AM +0100, Thomas Gleixner wrote:
> > >> Kuppuswamy,
> > >> Either that or you provide patches with arguments which are based on
> > >> proper analysis and not on 'appears to' observations.
> > >
> > > I think the right solution to the WBINVD would be to add a #VE handler
> > > that does nothing. We don't have a reasonable way to handle it from within
> > > the guest. We can call the VMM in hope that it would handle it, but VMM is
> > > untrusted and it can ignore the request.
> > >
> > > Dave suggested that we need to do code audit to make sure that there's no
> > > user inside TDX guest environment that relies on WBINVD to work correctly.
> > >
> > > Below is full call tree of WBINVD. It is substantially larger than I
> > > anticipated from initial grep.
> > >
> > > Conclusions:
> > >
> > >   - Most of callers are in ACPI code on changing S-states. Ignoring cache
> > >     flush for S-state change on virtual machine should be safe.
> > >
> > >   - The only WBINVD I was able to trigger is on poweroff from ACPI code.
> > >     Reboot also should trigger it, but for some reason I don't see it.
> > >
> > >   - Few caller in CPU offline code. TDX does not allowed to offline CPU as
> > >     we cannot bring it back -- we don't have SIPI. And even if offline
> > >     works for vCPU it should be safe to ignore WBINVD there.
> > >
> > >   - NVDIMMs are not supported inside TDX. If it will change we would need
> > >     to deal with cache flushing for this case. Hopefully, we would be able
> > >     to avoid WBINVD.
> > >
> > >   - Cache QoS and MTRR use WBINVD. They are disabled in TDX, but it is
> > >     controlled by VMM if the feature is advertised. We would need to
> > >     filter CPUID/MSRs to make sure VMM would not mess with them.
> > >
> > > Is it good enough justification for do-nothing #VE WBINVD handler?
> >
> > first of all thank you very much for this very profound analysis.
> >
> > This is really what I was asking for and you probably went even a step
> > deeper than that. Very appreciated.
> >
> > What we should do instead of doing a wholesale let's ignore WBINVD is to
> > have a separate function/macro:
> >
> >  ACPI_FLUSH_CPU_CACHE_PHYS()
> >
> > and invoke that from the functions which are considered to be safe.
> >
> > That would default to ACPI_FLUSH_CPU_CACHE() for other architecures
> > obviously.
> >
> > Then you can rightfully do:
> >
> > #define ACPI_FLUSH_CPU_CACHE_PHYS()     \
> >         if (!cpu_feature_enabled(XXX))        \
> >               wbinvd();               \
> >
> > where $XXX might be FEATURE_TDX_GUEST for paranoia sake and then
> > extended to X86_FEATURE_HYPERVISOR if everyone agrees.
> >
> > Then you have the #VE handler which just acts on any other wbinvd
> > invocation via warn, panic, whatever, no?
>
> I found another angle at the problem. According to the ACPI spec v6.4
> section 16.2 cache flushing is required on the way to S1, S2 and S3.
> And according to 8.2 it also is required on the way to C3.
>
> TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.
>
> Adjusting code to match the spec would make TDX work automagically.
>
> Any opinions on the patch below?
>
> I didn't touch ACPI_FLUSH_CPU_CACHE() users in cpufreq/longhaul.c because
> it might be outside of ACPI spec, I donno.
>
> diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
> index 808fdf54aeeb..b004a72a426e 100644
> --- a/drivers/acpi/acpica/hwesleep.c
> +++ b/drivers/acpi/acpica/hwesleep.c
> @@ -104,7 +104,8 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state)
>
>         /* Flush caches, as per ACPI specification */
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (sleep_state >= ACPI_STATE_S1 && sleep_state <= ACPI_STATE_S3)
> +               ACPI_FLUSH_CPU_CACHE();

So this basically means

if (sleep_state < ACPI_STATE_S14)
        ACPI_FLUSH_CPU_CACHE();

and analogously below.

This is fine with me, but it is an ACPICA patch, so it needs to be
submitted to the upstream project.  I think I can take care of this,
but not urgently.

>
>         status = acpi_os_enter_sleep(sleep_state, sleep_control, 0);
>         if (status == AE_CTRL_TERMINATE) {
> diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> index 34a3825f25d3..bfcd66efeb48 100644
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -110,7 +110,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
>
>         /* Flush caches, as per ACPI specification */
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (sleep_state >= ACPI_STATE_S1 && sleep_state <= ACPI_STATE_S3)
> +               ACPI_FLUSH_CPU_CACHE();
>
>         status = acpi_os_enter_sleep(sleep_state, pm1a_control, pm1b_control);
>         if (status == AE_CTRL_TERMINATE) {
> diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
> index e4cde23a2906..ba77598ee43e 100644
> --- a/drivers/acpi/acpica/hwxfsleep.c
> +++ b/drivers/acpi/acpica/hwxfsleep.c
> @@ -162,8 +162,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
>                 return_ACPI_STATUS(status);
>         }
>
> -       ACPI_FLUSH_CPU_CACHE();
> -
>         status = acpi_hw_write_port(acpi_gbl_FADT.smi_command,
>                                     (u32)acpi_gbl_FADT.s4_bios_request, 8);
>         if (ACPI_FAILURE(status)) {
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 76ef1bcc8848..01495aca850e 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
>  {
>         struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (cx->type == ACPI_STATE_C3)
> +               ACPI_FLUSH_CPU_CACHE();

And this is independent of the ACPICA changes above, so it can be made
in a separate patch.

This one is somewhat risky, though, because there is no guarantee that
all of the platforms in the field follow the spec.

>
>         while (1) {
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index eaa47753b758..a81d08b762c2 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -73,7 +73,9 @@ static int acpi_sleep_prepare(u32 acpi_state)
>                 acpi_set_waking_vector(acpi_wakeup_address);
>
>         }
> -       ACPI_FLUSH_CPU_CACHE();
> +
> +       if (acpi_state >= ACPI_STATE_S1 && acpi_state <= ACPI_STATE_S3)
> +               ACPI_FLUSH_CPU_CACHE();

This flushing and the one below looks like it may be redundant,
because the cache will be flushed again in the ACPICA code above
anyway.

However, this needs to be double-checked.

>  #endif
>         pr_info("Preparing to enter system sleep state S%d\n", acpi_state);
>         acpi_enable_wakeup_devices(acpi_state);
> @@ -566,7 +568,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>         u32 acpi_state = acpi_target_sleep_state;
>         int error;
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (acpi_state >= ACPI_STATE_S1 && acpi_state <= ACPI_STATE_S3)
> +               ACPI_FLUSH_CPU_CACHE();
>
>         trace_suspend_resume(TPS("acpi_suspend"), acpi_state, true);
>         switch (acpi_state) {
> @@ -903,8 +906,6 @@ static int acpi_hibernation_enter(void)
>  {
>         acpi_status status = AE_OK;
>
> -       ACPI_FLUSH_CPU_CACHE();
> -
>         /* This shouldn't return.  If it returns, we have a problem */
>         status = acpi_enter_sleep_state(ACPI_STATE_S4);
>         /* Reprogram control registers */

This one is OK and can be done in a separate patch.

[PATCH 0/4] ACPI/ACPICA: Only flush caches on S1/S2/S3 and C3

[Kirill A. Shutemov]

Does it look like you want?

Kirill A. Shutemov (4):
  ACPICA: Do not flush cache for on entering S4 and S5
  ACPI: PM: Remove redundant cache flushing
  ACPI: processor idle: Only flush cache on entering C3
  ACPI: PM: Avoid cache flush on entering S4

 drivers/acpi/acpica/hwesleep.c  | 3 ++-
 drivers/acpi/acpica/hwsleep.c   | 3 ++-
 drivers/acpi/acpica/hwxfsleep.c | 2 --
 drivers/acpi/processor_idle.c   | 3 ++-
 drivers/acpi/sleep.c            | 9 +++------
 5 files changed, 9 insertions(+), 11 deletions(-)

-- 
2.32.0

[PATCH 1/4] ACPICA: Do not flush cache for on entering S4 and S5

[Kirill A. Shutemov]

According to the ACPI spec v6.4, section 16.2 the cache flushing is
required on entering to S1, S2, and S3. ACPICA code flushes cache
regardless of the sleep state.

Blind cache flush on entering S5 causes problems for TDX. Flushing
happens with WBINVD that is not supported in the TDX environment.

TDX only supports S5 and adjusting ACPICA code to conform to the spec
fixes the issue.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/acpi/acpica/hwesleep.c  | 3 ++-
 drivers/acpi/acpica/hwsleep.c   | 3 ++-
 drivers/acpi/acpica/hwxfsleep.c | 2 --
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
index 808fdf54aeeb..ceb5a4292efa 100644
--- a/drivers/acpi/acpica/hwesleep.c
+++ b/drivers/acpi/acpica/hwesleep.c
@@ -104,7 +104,8 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state)
 
 	/* Flush caches, as per ACPI specification */
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (sleep_state < ACPI_STATE_S4)
+		ACPI_FLUSH_CPU_CACHE();
 
 	status = acpi_os_enter_sleep(sleep_state, sleep_control, 0);
 	if (status == AE_CTRL_TERMINATE) {
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index 34a3825f25d3..ee094a3aaaab 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -110,7 +110,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
 
 	/* Flush caches, as per ACPI specification */
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (sleep_state < ACPI_STATE_S4)
+		ACPI_FLUSH_CPU_CACHE();
 
 	status = acpi_os_enter_sleep(sleep_state, pm1a_control, pm1b_control);
 	if (status == AE_CTRL_TERMINATE) {
diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
index e4cde23a2906..ba77598ee43e 100644
--- a/drivers/acpi/acpica/hwxfsleep.c
+++ b/drivers/acpi/acpica/hwxfsleep.c
@@ -162,8 +162,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
 		return_ACPI_STATUS(status);
 	}
 
-	ACPI_FLUSH_CPU_CACHE();
-
 	status = acpi_hw_write_port(acpi_gbl_FADT.smi_command,
 				    (u32)acpi_gbl_FADT.s4_bios_request, 8);
 	if (ACPI_FAILURE(status)) {
-- 
2.32.0

[PATCH 2/4] ACPI: PM: Remove redundant cache flushing

[Kirill A. Shutemov]

ACPICA code takes care about cache flushing on S1/S2/S3 in
acpi_hw_extended_sleep() and acpi_hw_legacy_sleep().

acpi_suspend_enter() calls into ACPICA code via acpi_enter_sleep_state()
for S1 or x86_acpi_suspend_lowlevel() for S3. It only need to flush
cache for S2 (not sure if this call path is ever used for S2).

acpi_sleep_prepare() call tree:
  __acpi_pm_prepare()
    acpi_pm_prepare()
      acpi_suspend_ops::prepare_late()
      acpi_hibernation_ops::pre_snapshot()
      acpi_hibernation_ops::prepare()
    acpi_suspend_begin_old()
      acpi_suspend_begin_old::begin()
  acpi_hibernation_begin_old()
    acpi_hibernation_ops_old::acpi_hibernation_begin_old()
  acpi_power_off_prepare()
    pm_power_off_prepare()

Hibernation (S4) and Power Off (S5) don't require cache flushing. So,
the only interesting callsites are acpi_suspend_ops::prepare_late() and
acpi_suspend_begin_old::begin(). Both of them have cache flush on
->enter() operation in acpi_suspend_enter().

Remove redundant ACPI_FLUSH_CPU_CACHE() in acpi_sleep_prepare() and
acpi_suspend_enter().

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/acpi/sleep.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index eaa47753b758..14e8df0ac762 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -73,7 +73,6 @@ static int acpi_sleep_prepare(u32 acpi_state)
 		acpi_set_waking_vector(acpi_wakeup_address);
 
 	}
-	ACPI_FLUSH_CPU_CACHE();
 #endif
 	pr_info("Preparing to enter system sleep state S%d\n", acpi_state);
 	acpi_enable_wakeup_devices(acpi_state);
@@ -566,15 +565,15 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	u32 acpi_state = acpi_target_sleep_state;
 	int error;
 
-	ACPI_FLUSH_CPU_CACHE();
-
 	trace_suspend_resume(TPS("acpi_suspend"), acpi_state, true);
 	switch (acpi_state) {
 	case ACPI_STATE_S1:
 		barrier();
 		status = acpi_enter_sleep_state(acpi_state);
 		break;
-
+	case ACPI_STATE_S2:
+		ACPI_FLUSH_CPU_CACHE();
+		break;
 	case ACPI_STATE_S3:
 		if (!acpi_suspend_lowlevel)
 			return -ENOSYS;
-- 
2.32.0

[PATCH 3/4] ACPI: processor idle: Only flush cache on entering C3

[Kirill A. Shutemov]

According to the ACPI spec v6.4, section 8.2, cache flushing required
on entering C3 power state.

Avoid flushing cache on entering other power states.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/acpi/processor_idle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 76ef1bcc8848..01495aca850e 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 {
 	struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (cx->type == ACPI_STATE_C3)
+		ACPI_FLUSH_CPU_CACHE();
 
 	while (1) {
 
-- 
2.32.0

[PATCH 4/4] ACPI: PM: Avoid cache flush on entering S4

[Kirill A. Shutemov]

According to the ACPI spec v6.4, section 16.2 the cache flushing
required on entering to S1, S2, and S3.

No need to flush caches on hibernation (S4).

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/acpi/sleep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 14e8df0ac762..8166d863ed6b 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -902,8 +902,6 @@ static int acpi_hibernation_enter(void)
 {
 	acpi_status status = AE_OK;
 
-	ACPI_FLUSH_CPU_CACHE();
-
 	/* This shouldn't return.  If it returns, we have a problem */
 	status = acpi_enter_sleep_state(ACPI_STATE_S4);
 	/* Reprogram control registers */
-- 
2.32.0

Re: [PATCH 3/4] ACPI: processor idle: Only flush cache on entering C3

[Peter Zijlstra]

On Mon, Dec 06, 2021 at 03:29:51PM +0300, Kirill A. Shutemov wrote:
> According to the ACPI spec v6.4, section 8.2, cache flushing required
> on entering C3 power state.
> 
> Avoid flushing cache on entering other power states.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/acpi/processor_idle.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 76ef1bcc8848..01495aca850e 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
>  {
>  	struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
>  
> -	ACPI_FLUSH_CPU_CACHE();
> +	if (cx->type == ACPI_STATE_C3)
> +		ACPI_FLUSH_CPU_CACHE();
>  

acpi_idle_enter() already does this, acpi_idle_enter_s2idle() has it
confused again,

Also, I think acpi_idle_enter() does it too late; consider
acpi_idle_enter_mb(). Either that or the BM crud needs more comments.

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Dave Hansen]

On 12/3/21 4:54 PM, Kirill A. Shutemov wrote:
> On Fri, Dec 03, 2021 at 04:20:34PM -0800, Dave Hansen wrote:
>>> TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.
>>
>> This makes me a bit nervous.  Is this "the first TDX implementation
>> supports..." or "the TDX architecture *prohibits* supporting S1 (or
>> whatever"?
> 
> TDX Virtual Firmware Design Guide only states that "ACPI S3 (not supported
> by TDX guests)".
> 
> Kernel reports in dmesg "ACPI: PM: (supports S0 S5)".

Those describe the current firmware implementation, not a guarantee
provided by the TDX architecture forever.

> But I don't see how any state beyond S0 and S5 make sense in TDX context.
> Do you?

Do existing (non-TDX) VMs use anything other than S0 and S5?  If so, I'd
say yes.

>> I really think we need some kind of architecture guarantee.  Without
>> that, we risk breaking things if someone at our employer simply changes
>> their mind.
> 
> Guarantees are hard.
> 
> If somebody change their mind we will get unexpected #VE and crash.
> I think it is acceptable way to handle unexpected change in confidential
> computing environment.

Architectural guarantees are quite easy, actually.  They're just a
contract that two parties agree to.  In this case, the contract would be
that TDX firmware *PROMISES* not to enumerate support for additional
sleep states over what the implementation does today.  If future
firmware breaks that promise (and the kernel crashes) we get to come
after them with torches and pitchforks to fix the firmware.

The contract let's us do things in the OS like:

	WARN_ON(sleep_states[ACPI_STATE_S3]);

We also don't need *formal* documentation of such things.  We really
just need to have a chat.

It would be perfectly sufficient if we go bug Intel's TDX architecture
folks and say, "Hey, Linux is going to crash if you ever implement any
actual sleep states.  The current implementation is fine here, but is it
OK if future implementations are restricted from doing this?"

But, the trick is that we need a contract.  A contract requires a
"meeting of the minds" first.

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Dan Williams]

On Mon, Dec 6, 2021 at 7:35 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 12/3/21 4:54 PM, Kirill A. Shutemov wrote:
> > On Fri, Dec 03, 2021 at 04:20:34PM -0800, Dave Hansen wrote:
> >>> TDX doesn't support these S- and C-states. TDX is only supports S0 and S5.
> >>
> >> This makes me a bit nervous.  Is this "the first TDX implementation
> >> supports..." or "the TDX architecture *prohibits* supporting S1 (or
> >> whatever"?
> >
> > TDX Virtual Firmware Design Guide only states that "ACPI S3 (not supported
> > by TDX guests)".
> >
> > Kernel reports in dmesg "ACPI: PM: (supports S0 S5)".
>
> Those describe the current firmware implementation, not a guarantee
> provided by the TDX architecture forever.
>
> > But I don't see how any state beyond S0 and S5 make sense in TDX context.
> > Do you?
>
> Do existing (non-TDX) VMs use anything other than S0 and S5?  If so, I'd
> say yes.
>
> >> I really think we need some kind of architecture guarantee.  Without
> >> that, we risk breaking things if someone at our employer simply changes
> >> their mind.
> >
> > Guarantees are hard.
> >
> > If somebody change their mind we will get unexpected #VE and crash.
> > I think it is acceptable way to handle unexpected change in confidential
> > computing environment.
>
> Architectural guarantees are quite easy, actually.  They're just a
> contract that two parties agree to.  In this case, the contract would be
> that TDX firmware *PROMISES* not to enumerate support for additional
> sleep states over what the implementation does today.  If future
> firmware breaks that promise (and the kernel crashes) we get to come
> after them with torches and pitchforks to fix the firmware.
>
> The contract let's us do things in the OS like:
>
>         WARN_ON(sleep_states[ACPI_STATE_S3]);
>
> We also don't need *formal* documentation of such things.  We really
> just need to have a chat.
>
> It would be perfectly sufficient if we go bug Intel's TDX architecture
> folks and say, "Hey, Linux is going to crash if you ever implement any
> actual sleep states.  The current implementation is fine here, but is it
> OK if future implementations are restricted from doing this?"
>
> But, the trick is that we need a contract.  A contract requires a
> "meeting of the minds" first.

The WBINVD requirement in sleep states is about getting cache contents
out to to power preserved domain before the CPU turns off. The bare
metal host handles that requirement. The conversation that needs to be
had is with the ACPI specification committee to clarify that virtual
machines have no responsibility to flush caches. We can do that as a
Code First proposal to the ACPI Specification Working Group.

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Dave Hansen]

On 12/6/21 8:39 AM, Dan Williams wrote:
>> But, the trick is that we need a contract.  A contract requires a
>> "meeting of the minds" first.
> The WBINVD requirement in sleep states is about getting cache contents
> out to to power preserved domain before the CPU turns off. The bare
> metal host handles that requirement. The conversation that needs to be
> had is with the ACPI specification committee to clarify that virtual
> machines have no responsibility to flush caches. We can do that as a
> Code First proposal to the ACPI Specification Working Group.

Sounds sane to me.  So, we effectively go to the ACPI folks and say that
Linux isn't going to do WBINVD in virtualized environments any more.
That was effectively the approach that the first patch in this thread did:

> https://lore.kernel.org/all/20211116005027.2929297-1-sathyanarayanan.kuppuswamy@linux.intel.com/

Right?

Re: [PATCH v2] x86: Skip WBINVD instruction for VM guest

[Dan Williams]

On Mon, Dec 6, 2021 at 8:54 AM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 12/6/21 8:39 AM, Dan Williams wrote:
> >> But, the trick is that we need a contract.  A contract requires a
> >> "meeting of the minds" first.
> > The WBINVD requirement in sleep states is about getting cache contents
> > out to to power preserved domain before the CPU turns off. The bare
> > metal host handles that requirement. The conversation that needs to be
> > had is with the ACPI specification committee to clarify that virtual
> > machines have no responsibility to flush caches. We can do that as a
> > Code First proposal to the ACPI Specification Working Group.
>
> Sounds sane to me.  So, we effectively go to the ACPI folks and say that
> Linux isn't going to do WBINVD in virtualized environments any more.
> That was effectively the approach that the first patch in this thread did:
>
> > https://lore.kernel.org/all/20211116005027.2929297-1-sathyanarayanan.kuppuswamy@linux.intel.com/
>
> Right?

Correct, my reviewed-by was based on that observation, and now we can
close the loop by proposing the specification change.

Re: [PATCH 2/4] ACPI: PM: Remove redundant cache flushing

[Rafael J. Wysocki]

On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> ACPICA code takes care about cache flushing on S1/S2/S3 in
> acpi_hw_extended_sleep() and acpi_hw_legacy_sleep().
>
> acpi_suspend_enter() calls into ACPICA code via acpi_enter_sleep_state()
> for S1 or x86_acpi_suspend_lowlevel() for S3. It only need to flush
> cache for S2 (not sure if this call path is ever used for S2).
>
> acpi_sleep_prepare() call tree:
>   __acpi_pm_prepare()
>     acpi_pm_prepare()
>       acpi_suspend_ops::prepare_late()
>       acpi_hibernation_ops::pre_snapshot()
>       acpi_hibernation_ops::prepare()
>     acpi_suspend_begin_old()
>       acpi_suspend_begin_old::begin()
>   acpi_hibernation_begin_old()
>     acpi_hibernation_ops_old::acpi_hibernation_begin_old()
>   acpi_power_off_prepare()
>     pm_power_off_prepare()
>
> Hibernation (S4) and Power Off (S5) don't require cache flushing. So,
> the only interesting callsites are acpi_suspend_ops::prepare_late() and
> acpi_suspend_begin_old::begin(). Both of them have cache flush on
> ->enter() operation in acpi_suspend_enter().
>
> Remove redundant ACPI_FLUSH_CPU_CACHE() in acpi_sleep_prepare() and
> acpi_suspend_enter().
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/acpi/sleep.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index eaa47753b758..14e8df0ac762 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -73,7 +73,6 @@ static int acpi_sleep_prepare(u32 acpi_state)
>                 acpi_set_waking_vector(acpi_wakeup_address);
>
>         }
> -       ACPI_FLUSH_CPU_CACHE();
>  #endif
>         pr_info("Preparing to enter system sleep state S%d\n", acpi_state);
>         acpi_enable_wakeup_devices(acpi_state);
> @@ -566,15 +565,15 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>         u32 acpi_state = acpi_target_sleep_state;
>         int error;
>
> -       ACPI_FLUSH_CPU_CACHE();
> -
>         trace_suspend_resume(TPS("acpi_suspend"), acpi_state, true);
>         switch (acpi_state) {
>         case ACPI_STATE_S1:
>                 barrier();
>                 status = acpi_enter_sleep_state(acpi_state);
>                 break;
> -
> +       case ACPI_STATE_S2:
> +               ACPI_FLUSH_CPU_CACHE();
> +               break;

I don't think this is needed for S2, because the function doesn't do
anything low-level in that case and simply returns (IOW, S2 isn't
really supported).

>         case ACPI_STATE_S3:
>                 if (!acpi_suspend_lowlevel)
>                         return -ENOSYS;
> --
> 2.32.0
>

Re: [PATCH 1/4] ACPICA: Do not flush cache for on entering S4 and S5

[Rafael J. Wysocki]

On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> According to the ACPI spec v6.4, section 16.2 the cache flushing is
> required on entering to S1, S2, and S3. ACPICA code flushes cache
> regardless of the sleep state.
>
> Blind cache flush on entering S5 causes problems for TDX. Flushing
> happens with WBINVD that is not supported in the TDX environment.
>
> TDX only supports S5 and adjusting ACPICA code to conform to the spec
> fixes the issue.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

I've converted this patch to the upstream ACPICA code base format and
submitted a pull request with it to the upstream project.

Thanks!

> ---
>  drivers/acpi/acpica/hwesleep.c  | 3 ++-
>  drivers/acpi/acpica/hwsleep.c   | 3 ++-
>  drivers/acpi/acpica/hwxfsleep.c | 2 --
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
> index 808fdf54aeeb..ceb5a4292efa 100644
> --- a/drivers/acpi/acpica/hwesleep.c
> +++ b/drivers/acpi/acpica/hwesleep.c
> @@ -104,7 +104,8 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state)
>
>         /* Flush caches, as per ACPI specification */
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (sleep_state < ACPI_STATE_S4)
> +               ACPI_FLUSH_CPU_CACHE();
>
>         status = acpi_os_enter_sleep(sleep_state, sleep_control, 0);
>         if (status == AE_CTRL_TERMINATE) {
> diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> index 34a3825f25d3..ee094a3aaaab 100644
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -110,7 +110,8 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
>
>         /* Flush caches, as per ACPI specification */
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (sleep_state < ACPI_STATE_S4)
> +               ACPI_FLUSH_CPU_CACHE();
>
>         status = acpi_os_enter_sleep(sleep_state, pm1a_control, pm1b_control);
>         if (status == AE_CTRL_TERMINATE) {
> diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
> index e4cde23a2906..ba77598ee43e 100644
> --- a/drivers/acpi/acpica/hwxfsleep.c
> +++ b/drivers/acpi/acpica/hwxfsleep.c
> @@ -162,8 +162,6 @@ acpi_status acpi_enter_sleep_state_s4bios(void)
>                 return_ACPI_STATUS(status);
>         }
>
> -       ACPI_FLUSH_CPU_CACHE();
> -
>         status = acpi_hw_write_port(acpi_gbl_FADT.smi_command,
>                                     (u32)acpi_gbl_FADT.s4_bios_request, 8);
>         if (ACPI_FAILURE(status)) {
> --
> 2.32.0
>

Re: [PATCH 4/4] ACPI: PM: Avoid cache flush on entering S4

[Rafael J. Wysocki]

On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> According to the ACPI spec v6.4, section 16.2 the cache flushing
> required on entering to S1, S2, and S3.
>
> No need to flush caches on hibernation (S4).
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/acpi/sleep.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 14e8df0ac762..8166d863ed6b 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -902,8 +902,6 @@ static int acpi_hibernation_enter(void)
>  {
>         acpi_status status = AE_OK;
>
> -       ACPI_FLUSH_CPU_CACHE();
> -
>         /* This shouldn't return.  If it returns, we have a problem */
>         status = acpi_enter_sleep_state(ACPI_STATE_S4);
>         /* Reprogram control registers */
> --

Applied (with some edits in the subject and changelog) as 5.17 material, thanks!

Re: [PATCH 4/4] ACPI: PM: Avoid cache flush on entering S4

[Kirill A. Shutemov]

On Wed, Dec 08, 2021 at 04:10:52PM +0100, Rafael J. Wysocki wrote:
> On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> >
> > According to the ACPI spec v6.4, section 16.2 the cache flushing
> > required on entering to S1, S2, and S3.
> >
> > No need to flush caches on hibernation (S4).
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  drivers/acpi/sleep.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 14e8df0ac762..8166d863ed6b 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -902,8 +902,6 @@ static int acpi_hibernation_enter(void)
> >  {
> >         acpi_status status = AE_OK;
> >
> > -       ACPI_FLUSH_CPU_CACHE();
> > -
> >         /* This shouldn't return.  If it returns, we have a problem */
> >         status = acpi_enter_sleep_state(ACPI_STATE_S4);
> >         /* Reprogram control registers */
> > --
> 
> Applied (with some edits in the subject and changelog) as 5.17 material, thanks!

Is it for the series or only 4/4? Do I need to do something for 2/4 and
3/4?

-- 
 Kirill A. Shutemov

Re: [PATCH 4/4] ACPI: PM: Avoid cache flush on entering S4

[Rafael J. Wysocki]

On Wed, Dec 8, 2021 at 5:04 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Wed, Dec 08, 2021 at 04:10:52PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> > >
> > > According to the ACPI spec v6.4, section 16.2 the cache flushing
> > > required on entering to S1, S2, and S3.
> > >
> > > No need to flush caches on hibernation (S4).
> > >
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > ---
> > >  drivers/acpi/sleep.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > > index 14e8df0ac762..8166d863ed6b 100644
> > > --- a/drivers/acpi/sleep.c
> > > +++ b/drivers/acpi/sleep.c
> > > @@ -902,8 +902,6 @@ static int acpi_hibernation_enter(void)
> > >  {
> > >         acpi_status status = AE_OK;
> > >
> > > -       ACPI_FLUSH_CPU_CACHE();
> > > -
> > >         /* This shouldn't return.  If it returns, we have a problem */
> > >         status = acpi_enter_sleep_state(ACPI_STATE_S4);
> > >         /* Reprogram control registers */
> > > --
> >
> > Applied (with some edits in the subject and changelog) as 5.17 material, thanks!
>
> Is it for the series or only 4/4?

Just for the [4/4].

> Do I need to do something for 2/4 and 3/4?

For [2/4] you do as per the comment and let me reply to the [3/4].

Re: [PATCH 3/4] ACPI: processor idle: Only flush cache on entering C3

[Rafael J. Wysocki]

On Mon, Dec 6, 2021 at 4:03 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Dec 06, 2021 at 03:29:51PM +0300, Kirill A. Shutemov wrote:
> > According to the ACPI spec v6.4, section 8.2, cache flushing required
> > on entering C3 power state.
> >
> > Avoid flushing cache on entering other power states.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  drivers/acpi/processor_idle.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index 76ef1bcc8848..01495aca850e 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
> >  {
> >       struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
> >
> > -     ACPI_FLUSH_CPU_CACHE();
> > +     if (cx->type == ACPI_STATE_C3)
> > +             ACPI_FLUSH_CPU_CACHE();
> >
>
> acpi_idle_enter() already does this, acpi_idle_enter_s2idle() has it
> confused again,

No, they do the same thing: acpi_idle_enter_bm() if flags.bm_check is set.

> Also, I think acpi_idle_enter() does it too late; consider
> acpi_idle_enter_mb(). Either that or the BM crud needs more comments.

I think the latter.

Evidently, acpi_idle_play_dead(() doesn't support FFH and the BM
thing, so it is only necessary to flush the cache when using
ACPI_CSTATE_SYSTEMIO and when cx->type is C3.

Re: [PATCH 2/4] ACPI: PM: Remove redundant cache flushing

[Kirill A. Shutemov]

On Tue, Dec 07, 2021 at 05:35:38PM +0100, Rafael J. Wysocki wrote:
> I don't think this is needed for S2, because the function doesn't do
> anything low-level in that case and simply returns (IOW, S2 isn't
> really supported).

Updated patch is below. Does it look good?

From 5eb4ec7d8dd463ba186b779dcef2a802d999c59c Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Thu, 9 Dec 2021 16:08:02 +0300
Subject: [PATCH 1/2] ACPI: PM: Remove redundant cache flushing

ACPICA code takes care about cache flushing on S1/S2/S3 in
acpi_hw_extended_sleep() and acpi_hw_legacy_sleep().

acpi_suspend_enter() calls into ACPICA code via acpi_enter_sleep_state()
for S1 or x86_acpi_suspend_lowlevel() for S3.

acpi_sleep_prepare() call tree:
  __acpi_pm_prepare()
    acpi_pm_prepare()
      acpi_suspend_ops::prepare_late()
      acpi_hibernation_ops::pre_snapshot()
      acpi_hibernation_ops::prepare()
    acpi_suspend_begin_old()
      acpi_suspend_begin_old::begin()
  acpi_hibernation_begin_old()
    acpi_hibernation_ops_old::acpi_hibernation_begin_old()
  acpi_power_off_prepare()
    pm_power_off_prepare()

Hibernation (S4) and Power Off (S5) don't require cache flushing. So,
the only interesting callsites are acpi_suspend_ops::prepare_late() and
acpi_suspend_begin_old::begin(). Both of them have cache flush on
->enter() operation in acpi_suspend_enter().

Remove redundant ACPI_FLUSH_CPU_CACHE() in acpi_sleep_prepare() and
acpi_suspend_enter().

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/acpi/sleep.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index eaa47753b758..5ca6c223ba3d 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -73,7 +73,6 @@ static int acpi_sleep_prepare(u32 acpi_state)
 		acpi_set_waking_vector(acpi_wakeup_address);
 
 	}
-	ACPI_FLUSH_CPU_CACHE();
 #endif
 	pr_info("Preparing to enter system sleep state S%d\n", acpi_state);
 	acpi_enable_wakeup_devices(acpi_state);
@@ -566,8 +565,6 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	u32 acpi_state = acpi_target_sleep_state;
 	int error;
 
-	ACPI_FLUSH_CPU_CACHE();
-
 	trace_suspend_resume(TPS("acpi_suspend"), acpi_state, true);
 	switch (acpi_state) {
 	case ACPI_STATE_S1:
-- 
 Kirill A. Shutemov

Re: [PATCH 3/4] ACPI: processor idle: Only flush cache on entering C3

[Kirill A. Shutemov]

On Wed, Dec 08, 2021 at 05:26:12PM +0100, Rafael J. Wysocki wrote:
> On Mon, Dec 6, 2021 at 4:03 PM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Dec 06, 2021 at 03:29:51PM +0300, Kirill A. Shutemov wrote:
> > > According to the ACPI spec v6.4, section 8.2, cache flushing required
> > > on entering C3 power state.
> > >
> > > Avoid flushing cache on entering other power states.
> > >
> > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > ---
> > >  drivers/acpi/processor_idle.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > > index 76ef1bcc8848..01495aca850e 100644
> > > --- a/drivers/acpi/processor_idle.c
> > > +++ b/drivers/acpi/processor_idle.c
> > > @@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
> > >  {
> > >       struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
> > >
> > > -     ACPI_FLUSH_CPU_CACHE();
> > > +     if (cx->type == ACPI_STATE_C3)
> > > +             ACPI_FLUSH_CPU_CACHE();
> > >
> >
> > acpi_idle_enter() already does this, acpi_idle_enter_s2idle() has it
> > confused again,
> 
> No, they do the same thing: acpi_idle_enter_bm() if flags.bm_check is set.
> 
> > Also, I think acpi_idle_enter() does it too late; consider
> > acpi_idle_enter_mb(). Either that or the BM crud needs more comments.
> 
> I think the latter.
> 
> Evidently, acpi_idle_play_dead(() doesn't support FFH and the BM
> thing, so it is only necessary to flush the cache when using
> ACPI_CSTATE_SYSTEMIO and when cx->type is C3.

I'm new to this and not completely follow what I need to change.

Does it look correct?

From 3c544bc95a16d6a23dcb0aa50ee905d5e97c9ce5 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Thu, 9 Dec 2021 16:24:44 +0300
Subject: [PATCH] ACPI: processor idle: Only flush cache on entering C3

According to the ACPI spec v6.4, section 8.2, cache flushing required
on entering C3 power state.

Avoid flushing cache on entering other power states.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 drivers/acpi/processor_idle.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 76ef1bcc8848..d2a4d4446eff 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -567,7 +567,9 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 {
 	struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
 
-	ACPI_FLUSH_CPU_CACHE();
+	if (cx->entry_method == ACPI_CSTATE_SYSTEMIO &&
+	    cx->type == ACPI_STATE_C3)
+		ACPI_FLUSH_CPU_CACHE();
 
 	while (1) {
 
-- 
 Kirill A. Shutemov

Re: [PATCH 3/4] ACPI: processor idle: Only flush cache on entering C3

[Rafael J. Wysocki]

On Thu, Dec 9, 2021 at 2:33 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Wed, Dec 08, 2021 at 05:26:12PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Dec 6, 2021 at 4:03 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Mon, Dec 06, 2021 at 03:29:51PM +0300, Kirill A. Shutemov wrote:
> > > > According to the ACPI spec v6.4, section 8.2, cache flushing required
> > > > on entering C3 power state.
> > > >
> > > > Avoid flushing cache on entering other power states.
> > > >
> > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > > > ---
> > > >  drivers/acpi/processor_idle.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > > > index 76ef1bcc8848..01495aca850e 100644
> > > > --- a/drivers/acpi/processor_idle.c
> > > > +++ b/drivers/acpi/processor_idle.c
> > > > @@ -567,7 +567,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
> > > >  {
> > > >       struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
> > > >
> > > > -     ACPI_FLUSH_CPU_CACHE();
> > > > +     if (cx->type == ACPI_STATE_C3)
> > > > +             ACPI_FLUSH_CPU_CACHE();
> > > >
> > >
> > > acpi_idle_enter() already does this, acpi_idle_enter_s2idle() has it
> > > confused again,
> >
> > No, they do the same thing: acpi_idle_enter_bm() if flags.bm_check is set.
> >
> > > Also, I think acpi_idle_enter() does it too late; consider
> > > acpi_idle_enter_mb(). Either that or the BM crud needs more comments.
> >
> > I think the latter.
> >
> > Evidently, acpi_idle_play_dead(() doesn't support FFH and the BM
> > thing, so it is only necessary to flush the cache when using
> > ACPI_CSTATE_SYSTEMIO and when cx->type is C3.
>
> I'm new to this and not completely follow what I need to change.
>
> Does it look correct?

It does, but I liked the original one more and so that one has been
applied as 5.17 material (with some edits in the changelog).

Thanks!

> From 3c544bc95a16d6a23dcb0aa50ee905d5e97c9ce5 Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Thu, 9 Dec 2021 16:24:44 +0300
> Subject: [PATCH] ACPI: processor idle: Only flush cache on entering C3
>
> According to the ACPI spec v6.4, section 8.2, cache flushing required
> on entering C3 power state.
>
> Avoid flushing cache on entering other power states.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/acpi/processor_idle.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 76ef1bcc8848..d2a4d4446eff 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -567,7 +567,9 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
>  {
>         struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
>
> -       ACPI_FLUSH_CPU_CACHE();
> +       if (cx->entry_method == ACPI_CSTATE_SYSTEMIO &&
> +           cx->type == ACPI_STATE_C3)
> +               ACPI_FLUSH_CPU_CACHE();
>
>         while (1) {
>
> --
>  Kirill A. Shutemov

Re: [PATCH 2/4] ACPI: PM: Remove redundant cache flushing

[Rafael J. Wysocki]

On Thu, Dec 9, 2021 at 2:32 PM Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
>
> On Tue, Dec 07, 2021 at 05:35:38PM +0100, Rafael J. Wysocki wrote:
> > I don't think this is needed for S2, because the function doesn't do
> > anything low-level in that case and simply returns (IOW, S2 isn't
> > really supported).
>
> Updated patch is below. Does it look good?

It does, and so applied as 5.17 material with some minor edits in the changelog.

Thanks!

> From 5eb4ec7d8dd463ba186b779dcef2a802d999c59c Mon Sep 17 00:00:00 2001
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Date: Thu, 9 Dec 2021 16:08:02 +0300
> Subject: [PATCH 1/2] ACPI: PM: Remove redundant cache flushing
>
> ACPICA code takes care about cache flushing on S1/S2/S3 in
> acpi_hw_extended_sleep() and acpi_hw_legacy_sleep().
>
> acpi_suspend_enter() calls into ACPICA code via acpi_enter_sleep_state()
> for S1 or x86_acpi_suspend_lowlevel() for S3.
>
> acpi_sleep_prepare() call tree:
>   __acpi_pm_prepare()
>     acpi_pm_prepare()
>       acpi_suspend_ops::prepare_late()
>       acpi_hibernation_ops::pre_snapshot()
>       acpi_hibernation_ops::prepare()
>     acpi_suspend_begin_old()
>       acpi_suspend_begin_old::begin()
>   acpi_hibernation_begin_old()
>     acpi_hibernation_ops_old::acpi_hibernation_begin_old()
>   acpi_power_off_prepare()
>     pm_power_off_prepare()
>
> Hibernation (S4) and Power Off (S5) don't require cache flushing. So,
> the only interesting callsites are acpi_suspend_ops::prepare_late() and
> acpi_suspend_begin_old::begin(). Both of them have cache flush on
> ->enter() operation in acpi_suspend_enter().
>
> Remove redundant ACPI_FLUSH_CPU_CACHE() in acpi_sleep_prepare() and
> acpi_suspend_enter().
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  drivers/acpi/sleep.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index eaa47753b758..5ca6c223ba3d 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -73,7 +73,6 @@ static int acpi_sleep_prepare(u32 acpi_state)
>                 acpi_set_waking_vector(acpi_wakeup_address);
>
>         }
> -       ACPI_FLUSH_CPU_CACHE();
>  #endif
>         pr_info("Preparing to enter system sleep state S%d\n", acpi_state);
>         acpi_enable_wakeup_devices(acpi_state);
> @@ -566,8 +565,6 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>         u32 acpi_state = acpi_target_sleep_state;
>         int error;
>
> -       ACPI_FLUSH_CPU_CACHE();
> -
>         trace_suspend_resume(TPS("acpi_suspend"), acpi_state, true);
>         switch (acpi_state) {
>         case ACPI_STATE_S1:
> --
>  Kirill A. Shutemov