LinuxLists.cc - [PATCH v1 1/1] x86: Skip WBINVD instruction for VM guest

[permalink] [raw]

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

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

2021-12-02 22:39:27

[permalink] [raw]

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

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?

2021-12-02 23:48:49

by Thomas Gleixner

[permalink] [raw]

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

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

2021-12-03 23:49:15

[permalink] [raw]

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

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

2021-12-04 00:20:39

[permalink] [raw]

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

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.

2021-12-04 00:54:29

[permalink] [raw]

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

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

2021-12-04 20:27:36

[permalink] [raw]

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

On Sat, Dec 4, 2021 at 12:49 AM Kirill A. Shutemov
<[email protected]> 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.

2021-12-06 12:30:12

[permalink] [raw]

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

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

2021-12-06 12:30:20

[permalink] [raw]

Subject: [PATCH 2/4] 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. 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 <[email protected]>
---
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

2021-12-06 12:30:22

[permalink] [raw]

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

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 <[email protected]>
---
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

2021-12-06 15:49:48

[permalink] [raw]

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

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.

2021-12-06 16:40:08

by Dan Williams

[permalink] [raw]

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

On Mon, Dec 6, 2021 at 7:35 AM Dave Hansen <[email protected]> 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.

2021-12-06 17:05:18

[permalink] [raw]

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

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?

2021-12-06 17:51:20

by Dan Williams

[permalink] [raw]

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

On Mon, Dec 6, 2021 at 8:54 AM Dave Hansen <[email protected]> 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.

2021-12-07 16:35:58

[permalink] [raw]

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

On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
<[email protected]> 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 <[email protected]>
> ---
> 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
>

2021-12-08 15:11:06

[permalink] [raw]

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

On Mon, Dec 6, 2021 at 1:30 PM Kirill A. Shutemov
<[email protected]> 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 <[email protected]>
> ---
> 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!

2021-12-08 16:04:58

[permalink] [raw]

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

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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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

2021-12-08 16:16:24

[permalink] [raw]

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

On Wed, Dec 8, 2021 at 5:04 PM Kirill A. Shutemov
<[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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].

2021-12-09 13:32:17

[permalink] [raw]

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

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" <[email protected]>
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 <[email protected]>
---
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

2021-12-17 18:04:45