Skip to contentSkip to conversation

ACPI: disable extra P_LVLx access on KVM

RFC PATCH In review Michael S. Tsirkin 3 participants 3 messages Mar 30, 2018 – Apr 23, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 ACPI: disable extra P_LVLx access on KVM

As documented by commit b488f021 "ACPI: restore comment justifying 'extra' P_LVLx access", Linux does an extra IO read after entering idle because on (some) chipsets STPCLK# doesn't get asserted in time to prevent further instruction processing.

This can never be the case on KVM, and a timer read causes an expensive VM exit in turn causing useless load on host system. Detect KVM and skip the read. TODO: whitelist more hypervisors?

Note: very lightly tested. Pls don't apply this yet, I am working on a _CST implementation for KVM and will repost this without the RFC tag when it's been tested properly.

Posting now for early flames/feedback.

---
 drivers/acpi/processor_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/processor_idle.c +5 -2
index abb559c..8ae28dc 100644
@@ -33,6 +33,7 @@
 #include <linux/cpuidle.h>
 #include <linux/cpu.h>
 #include <acpi/processor.h>
+#include <linux/kvm_para.h>
 
 /*
  * Include the apic definitions for x86 to have the APIC timer related defines
@@ -665,7 +666,8 @@ static void __cpuidle acpi_idle_do_entry(struct acpi_processor_cx *cx)
 		/* Dummy wait op - must do something useless after P_LVL2 read
 		   because chipsets cannot guarantee that STPCLK# signal
 		   gets asserted in time to freeze execution properly. */
-		inl(acpi_gbl_FADT.xpm_timer_block.address);
+		if (!kvm_para_available())
+			inl(acpi_gbl_FADT.xpm_timer_block.address);
 	}
 }
 
@@ -687,7 +689,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 		else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
 			inb(cx->address);
 			/* See comment in acpi_idle_do_entry() */
-			inl(acpi_gbl_FADT.xpm_timer_block.address);
+			if (!kvm_para_available())
+				inl(acpi_gbl_FADT.xpm_timer_block.address);
 		} else
 			return -ENODEV;
 	}
Signature · 1 line
MST
#1 lore raw
Quoted 47 lines · CEST Michael S. Tsirkin

As documented by commit b488f021 "ACPI: restore comment justifying 'extra' P_LVLx access", Linux does an extra IO read after entering idle because on (some) chipsets STPCLK# doesn't get asserted in time to prevent further instruction processing.

This can never be the case on KVM, and a timer read causes an expensive VM exit in turn causing useless load on host system. Detect KVM and skip the read. TODO: whitelist more hypervisors?

Note: very lightly tested. Pls don't apply this yet, I am working on a _CST implementation for KVM and will repost this without the RFC tag when it's been tested properly.

Posting now for early flames/feedback.

---
 drivers/acpi/processor_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/processor_idle.c +3 -1
index abb559c..8ae28dc 100644
@@ -33,6 +33,7 @@
 #include <linux/cpuidle.h>
 #include <linux/cpu.h>
 #include <acpi/processor.h>
+#include <linux/kvm_para.h>
 
 /*
  * Include the apic definitions for x86 to have the APIC timer related defines
@@ -665,7 +666,8 @@ static void __cpuidle acpi_idle_do_entry(struct acpi_processor_cx *cx)
 		/* Dummy wait op - must do something useless after P_LVL2 read
 		   because chipsets cannot guarantee that STPCLK# signal
 		   gets asserted in time to freeze execution properly. */
-		inl(acpi_gbl_FADT.xpm_timer_block.address);
+		if (!kvm_para_available())
+			inl(acpi_gbl_FADT.xpm_timer_block.address);
 	}
 }
 

drivers/acpi/processor_idle.c static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) · line 696

Quoted 9 lines
drivers/acpi/processor_idle.c +2 -1
@@ -687,7 +689,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 		else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
 			inb(cx->address);
 			/* See comment in acpi_idle_do_entry() */
-			inl(acpi_gbl_FADT.xpm_timer_block.address);
+			if (!kvm_para_available())
+				inl(acpi_gbl_FADT.xpm_timer_block.address);
 		} else
 			return -ENODEV;
 	}

While I have no objections to this change from the ACPI side, I'd like someone from the KVM land to comment on this.

Thanks,
Rafael

maintainer
#2 lore raw
Quoted 46 lines · Rafael J. Wysocki
Quoted 45 lines · CEST Michael S. Tsirkin

As documented by commit b488f021 "ACPI: restore comment justifying 'extra' P_LVLx access", Linux does an extra IO read after entering idle because on (some) chipsets STPCLK# doesn't get asserted in time to prevent further instruction processing.

This can never be the case on KVM, and a timer read causes an expensive VM exit in turn causing useless load on host system. Detect KVM and skip the read. TODO: whitelist more hypervisors?

Note: very lightly tested. Pls don't apply this yet, I am working on a _CST implementation for KVM and will repost this without the RFC tag when it's been tested properly.

Posting now for early flames/feedback.

---
 drivers/acpi/processor_idle.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/processor_idle.c +3 -1
index abb559c..8ae28dc 100644
@@ -33,6 +33,7 @@
 #include <linux/cpuidle.h>
 #include <linux/cpu.h>
 #include <acpi/processor.h>
+#include <linux/kvm_para.h>
 
 /*
  * Include the apic definitions for x86 to have the APIC timer related defines
@@ -665,7 +666,8 @@ static void __cpuidle acpi_idle_do_entry(struct acpi_processor_cx *cx)
 		/* Dummy wait op - must do something useless after P_LVL2 read
 		   because chipsets cannot guarantee that STPCLK# signal
 		   gets asserted in time to freeze execution properly. */
-		inl(acpi_gbl_FADT.xpm_timer_block.address);
+		if (!kvm_para_available())
+			inl(acpi_gbl_FADT.xpm_timer_block.address);
 	}
 }
 
Quoted 14 lines

drivers/acpi/processor_idle.c static int acpi_idle_play_dead(struct cpuidle_device *dev, int index) · line 696

Quoted 11 lines
drivers/acpi/processor_idle.c +2 -1
@@ -687,7 +689,8 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
 		else if (cx->entry_method == ACPI_CSTATE_SYSTEMIO) {
 			inb(cx->address);
 			/* See comment in acpi_idle_do_entry() */
-			inl(acpi_gbl_FADT.xpm_timer_block.address);
+			if (!kvm_para_available())
+				inl(acpi_gbl_FADT.xpm_timer_block.address);
 		} else
 			return -ENODEV;
 	}

While I have no objections to this change from the ACPI side, I'd like someone from the KVM land to comment on this.

It would be nicer to blacklist the problematic chipsets, or as Michael said have a way to whitelist more hypervisors, but this is certainly okay from KVM land too.

Thanks,

paolo