acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0

PATCH Superseded Jason Andryuk 2 participants 3 messages Oct 10, 2023 – Oct 13, 2023

Subsystems (4)

Stable branch x86 Xen hypervisor interface Xen hypervisor x86

lore

Share & tools

lore

Jason Andryuk

author

2023-10-10 17:39 UTC #0 lore raw

PATCH v3 1/1 acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0

From: Roger Pau Monne <[email protected]>

The Processor capability bits notify ACPI of the OS capabilities, and so ACPI can adjust the return of other Processor methods taking the OS capabilities into account.

When Linux is running as a Xen dom0, the hypervisor is the entity in charge of processor power management, and hence Xen needs to make sure the capabilities reported by _OSC/_PDC match the capabilities of the driver in Xen.

Introduce a small helper to sanitize the buffer when running as Xen dom0.

When Xen supports HWP, this serves as the equivalent of commit a21211672c9a ("ACPI / processor: Request native thermal interrupt handling via _OSC") to avoid SMM crashes. Xen will set bit ACPI_PROC_CAP_COLLAB_PROC_PERF (bit 12) in the capability bits and the _OSC/_PDC call will apply it.

[ jandryuk: Mention Xen HWP's need. Support _OSC & _PDC ]

Signed-off-by Roger Pau Monné <[email protected]>

Cc [email protected]

Signed-off-by Jason Andryuk <[email protected]>

---
v3:
Move xen_sanitize_pdc() call to arch_acpi_set_proc_cap_bits() to cover _OSC and _PDC.
drivers/xen/pcpu.c is CONFIG_DOM0 && CONFIG_X86

v2:
Move local variables in acpi_processor_eval_pdc() to reuse in both conditions.

---
 arch/x86/include/asm/acpi.h           | 13 +++++++++++++
 arch/x86/include/asm/xen/hypervisor.h |  9 +++++++++
 drivers/xen/pcpu.c                    | 21 +++++++++++++++++++++
 3 files changed, 43 insertions(+)

arch/x86/include/asm/acpi.h +13 -0

index c8a7fc23f63c..cc8d1669d6e8 100644
@@ -16,6 +16,9 @@
 #include <asm/x86_init.h>
 #include <asm/cpufeature.h>
 #include <asm/irq_vectors.h>
+#include <asm/xen/hypervisor.h>
+
+#include <xen/xen.h>
 
 #ifdef CONFIG_ACPI_APEI
 # include <asm/pgtable_types.h>

@@ -127,6 +130,16 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
 	if (!cpu_has(c, X86_FEATURE_MWAIT) ||
 	    boot_option_idle_override == IDLE_NOMWAIT)
 		*cap &= ~(ACPI_PROC_CAP_C_C1_FFH | ACPI_PROC_CAP_C_C2C3_FFH);
+
+	if (xen_initial_domain()) {
+		/*
+		 * When Linux is running as Xen dom0, the hypervisor is the
+		 * entity in charge of the processor power management, and so
+		 * Xen needs to check the OS capabilities reported in the _PDC
+		 * buffer matches what the hypervisor driver supports.
+		 */
+		xen_sanitize_pdc(cap);
+	}
 }
 
 static inline bool acpi_has_cpu_in_madt(void)

arch/x86/include/asm/xen/hypervisor.h +9 -0

index 7048dfacc04b..c6c2f174fa30 100644
@@ -100,4 +100,13 @@ static inline void leave_lazy(enum xen_lazy_mode mode)
 
 enum xen_lazy_mode xen_get_lazy_mode(void);
 
+#if defined(CONFIG_XEN_DOM0) && defined(CONFIG_ACPI)
+void xen_sanitize_pdc(uint32_t *buf);
+#else
+static inline void xen_sanitize_pdc(uint32_t *buf)
+{
+	BUG();
+}
+#endif
+
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */

drivers/xen/pcpu.c +21 -0

index b3e3d1bb37f3..859bb6027105 100644
@@ -47,6 +47,9 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+#endif
 
 /*
  * @cpu_id: Xen physical cpu logic number

@@ -400,4 +403,22 @@ bool __init xen_processor_present(uint32_t acpi_id)
 
 	return online;
 }
+
+void xen_sanitize_pdc(uint32_t *cap)
+{
+	struct xen_platform_op op = {
+		.cmd			= XENPF_set_processor_pminfo,
+		.u.set_pminfo.id	= -1,
+		.u.set_pminfo.type	= XEN_PM_PDC,
+	};
+	u32 buf[3] = { ACPI_PDC_REVISION_ID, 1, *cap };
+	int ret;
+
+	set_xen_guest_handle(op.u.set_pminfo.pdc, buf);
+	ret = HYPERVISOR_platform_op(&op);
+	if (ret)
+		pr_info("sanitize of _PDC buffer bits from Xen failed: %d\n",
+			ret);
+	*cap = buf[2];
+}
 #endif

Signature · 1 line

2.41.0

Wilczynski, Michal

2023-10-13 16:17 UTC #1 lore raw

Hi,

Quoted 53 lines · Jason Andryuk

From: Roger Pau Monne <[email protected]>
The Processor capability bits notify ACPI of the OS capabilities, and so ACPI can adjust the return of other Processor methods taking the OS capabilities into account.
When Linux is running as a Xen dom0, the hypervisor is the entity in charge of processor power management, and hence Xen needs to make sure the capabilities reported by _OSC/_PDC match the capabilities of the driver in Xen.
Introduce a small helper to sanitize the buffer when running as Xen dom0.
When Xen supports HWP, this serves as the equivalent of commit a21211672c9a ("ACPI / processor: Request native thermal interrupt handling via _OSC") to avoid SMM crashes. Xen will set bit ACPI_PROC_CAP_COLLAB_PROC_PERF (bit 12) in the capability bits and the _OSC/_PDC call will apply it.
[ jandryuk: Mention Xen HWP's need. Support _OSC & _PDC ]
Signed-off-by Roger Pau Monné <[email protected]>
Cc [email protected]
Signed-off-by Jason Andryuk <[email protected]>
---
v3:
Move xen_sanitize_pdc() call to arch_acpi_set_proc_cap_bits() to cover _OSC and _PDC.
drivers/xen/pcpu.c is CONFIG_DOM0 && CONFIG_X86
v2:
Move local variables in acpi_processor_eval_pdc() to reuse in both conditions.
---
 arch/x86/include/asm/acpi.h           | 13 +++++++++++++
 arch/x86/include/asm/xen/hypervisor.h |  9 +++++++++
 drivers/xen/pcpu.c                    | 21 +++++++++++++++++++++
 3 files changed, 43 insertions(+)
arch/x86/include/asm/acpi.h +3 -0
index c8a7fc23f63c..cc8d1669d6e8 100644
@@ -16,6 +16,9 @@
 #include <asm/x86_init.h>
 #include <asm/cpufeature.h>
 #include <asm/irq_vectors.h>
+#include <asm/xen/hypervisor.h>
+
+#include <xen/xen.h>
 
 #ifdef CONFIG_ACPI_APEI
 # include <asm/pgtable_types.h>

arch/x86/include/asm/acpi.h static inline void arch_acpi_set_proc_cap_bits(u32 *cap) · line 138

Quoted 9 lines

arch/x86/include/asm/acpi.h +6 -0

@@ -127,6 +130,16 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
 	if (!cpu_has(c, X86_FEATURE_MWAIT) ||
 	    boot_option_idle_override == IDLE_NOMWAIT)
 		*cap &= ~(ACPI_PROC_CAP_C_C1_FFH | ACPI_PROC_CAP_C_C2C3_FFH);
+
+	if (xen_initial_domain()) {
+		/*
+		 * When Linux is running as Xen dom0, the hypervisor is the
+		 * entity in charge of the processor power management, and so
+		 * Xen needs to check the OS capabilities reported in the _PDC

I would argue the phrasing here is unfortunate - it's not really _PDC buffer anymore, it's more processor capabilities buffer [1]. Your phrasing would indicate that this buffer is somehow _PDC specific.

BTW this file is x86 specific code - are you sure it's appropriate to involve Xen hypervisor here ? I understand this case if x86 specific, but still.

arch/x86/include/asm/acpi.h static inline void arch_acpi_set_proc_cap_bits(u32 *cap) · line 141

Quoted 3 lines

arch/x86/include/asm/acpi.h +3 -0

+		 * buffer matches what the hypervisor driver supports.
+		 */
+		xen_sanitize_pdc(cap);

Same here as in [1], I would call this function xen_sanitize_proc_cap_buffer(), or something along those lines for better readability and correctness.

Quoted 37 lines

arch/x86/include/asm/acpi.h +1 -0

+	}
 }
 
 static inline bool acpi_has_cpu_in_madt(void)

arch/x86/include/asm/xen/hypervisor.h +9 -0

index 7048dfacc04b..c6c2f174fa30 100644
@@ -100,4 +100,13 @@ static inline void leave_lazy(enum xen_lazy_mode mode)
 
 enum xen_lazy_mode xen_get_lazy_mode(void);
 
+#if defined(CONFIG_XEN_DOM0) && defined(CONFIG_ACPI)
+void xen_sanitize_pdc(uint32_t *buf);
+#else
+static inline void xen_sanitize_pdc(uint32_t *buf)
+{
+	BUG();
+}
+#endif
+
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */

drivers/xen/pcpu.c +3 -0

index b3e3d1bb37f3..859bb6027105 100644
@@ -47,6 +47,9 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+#endif
 
 /*
  * @cpu_id: Xen physical cpu logic number

drivers/xen/pcpu.c bool __init xen_processor_present(uint32_t acpi_id) · line 412

Quoted 10 lines

drivers/xen/pcpu.c +7 -0

@@ -400,4 +403,22 @@ bool __init xen_processor_present(uint32_t acpi_id)
 
 	return online;
 }
+
+void xen_sanitize_pdc(uint32_t *cap)
+{
+	struct xen_platform_op op = {
+		.cmd			= XENPF_set_processor_pminfo,
+		.u.set_pminfo.id	= -1,
+		.u.set_pminfo.type	= XEN_PM_PDC,

It would probably be best to rename this constant as well so it's not misleading.

drivers/xen/pcpu.c bool __init xen_processor_present(uint32_t acpi_id) · line 421

Quoted 9 lines

drivers/xen/pcpu.c +9 -0

+	};
+	u32 buf[3] = { ACPI_PDC_REVISION_ID, 1, *cap };
+	int ret;
+
+	set_xen_guest_handle(op.u.set_pminfo.pdc, buf);
+	ret = HYPERVISOR_platform_op(&op);
+	if (ret)
+		pr_info("sanitize of _PDC buffer bits from Xen failed: %d\n",
+			ret);

Shouldn't an error be pr_err ?

drivers/xen/pcpu.c bool __init xen_processor_present(uint32_t acpi_id) · line 424

Quoted 3 lines

drivers/xen/pcpu.c +2 -0
+	*cap = buf[2];
+}
 #endif

Regards,
Michał

Jason Andryuk

author

2023-10-13 18:20 UTC #2 lore raw

Thanks for taking a look, Michal.

Quoted 54 lines · Michal

Hi,
Quoted 50 lines · Jason Andryuk
From: Roger Pau Monne <[email protected]>
The Processor capability bits notify ACPI of the OS capabilities, and so ACPI can adjust the return of other Processor methods taking the OS capabilities into account.
When Linux is running as a Xen dom0, the hypervisor is the entity in charge of processor power management, and hence Xen needs to make sure the capabilities reported by _OSC/_PDC match the capabilities of the driver in Xen.
Introduce a small helper to sanitize the buffer when running as Xen dom0.
When Xen supports HWP, this serves as the equivalent of commit a21211672c9a ("ACPI / processor: Request native thermal interrupt handling via _OSC") to avoid SMM crashes. Xen will set bit ACPI_PROC_CAP_COLLAB_PROC_PERF (bit 12) in the capability bits and the _OSC/_PDC call will apply it.
[ jandryuk: Mention Xen HWP's need. Support _OSC & _PDC ]
Signed-off-by Roger Pau Monné <[email protected]>
Cc [email protected]
Signed-off-by Jason Andryuk <[email protected]>
---
v3:
Move xen_sanitize_pdc() call to arch_acpi_set_proc_cap_bits() to cover _OSC and _PDC.
drivers/xen/pcpu.c is CONFIG_DOM0 && CONFIG_X86
v2:
Move local variables in acpi_processor_eval_pdc() to reuse in both conditions.
---
 arch/x86/include/asm/acpi.h           | 13 +++++++++++++
 arch/x86/include/asm/xen/hypervisor.h |  9 +++++++++
 drivers/xen/pcpu.c                    | 21 +++++++++++++++++++++
 3 files changed, 43 insertions(+)
arch/x86/include/asm/acpi.h +3 -0
index c8a7fc23f63c..cc8d1669d6e8 100644
@@ -16,6 +16,9 @@
 #include <asm/x86_init.h>
 #include <asm/cpufeature.h>
 #include <asm/irq_vectors.h>
+#include <asm/xen/hypervisor.h>
+
+#include <xen/xen.h>
 

Quoted 16 lines

arch/x86/include/asm/acpi.h static inline void arch_acpi_set_proc_cap_bits(u32 *cap) · line 138

Quoted 12 lines

arch/x86/include/asm/acpi.h +6 -0

 #ifdef CONFIG_ACPI_APEI
 # include <asm/pgtable_types.h>

@@ -127,6 +130,16 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
      if (!cpu_has(c, X86_FEATURE_MWAIT) ||
          boot_option_idle_override == IDLE_NOMWAIT)
              *cap &= ~(ACPI_PROC_CAP_C_C1_FFH | ACPI_PROC_CAP_C_C2C3_FFH);
+
+     if (xen_initial_domain()) {
+             /*
+              * When Linux is running as Xen dom0, the hypervisor is the
+              * entity in charge of the processor power management, and so
+              * Xen needs to check the OS capabilities reported in the _PDC

Ok.

Quoted 2 lines

BTW this file is x86 specific code - are you sure it's appropriate to involve Xen hypervisor here ? I understand this case if x86 specific, but still.

The Xen hypercall is x86-specific. I see arch_acpi_set_proc_cap_bits() as a hook to set arch-specific bits. Looking at Xen/x86 as the arch, it makes sense. The other option would be a Xen conditional back in the acpi code. Keeping it with the x86 code therefore made more sense to me.

Quoted 6 lines

arch/x86/include/asm/acpi.h static inline void arch_acpi_set_proc_cap_bits(u32 *cap) · line 141
Quoted 3 lines
arch/x86/include/asm/acpi.h +3 -0
+              * buffer matches what the hypervisor driver supports.
+              */
+             xen_sanitize_pdc(cap);
Same here as in [1], I would call this function xen_sanitize_proc_cap_buffer(), or something along those lines for better readability and correctness.

Ok.

Quoted 38 lines

arch/x86/include/asm/acpi.h +1 -0

+     }
 }
 
 static inline bool acpi_has_cpu_in_madt(void)

arch/x86/include/asm/xen/hypervisor.h +9 -0

index 7048dfacc04b..c6c2f174fa30 100644
@@ -100,4 +100,13 @@ static inline void leave_lazy(enum xen_lazy_mode mode)
 
 enum xen_lazy_mode xen_get_lazy_mode(void);
 
+#if defined(CONFIG_XEN_DOM0) && defined(CONFIG_ACPI)
+void xen_sanitize_pdc(uint32_t *buf);
+#else
+static inline void xen_sanitize_pdc(uint32_t *buf)
+{
+     BUG();
+}
+#endif
+
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */

drivers/xen/pcpu.c +3 -0

index b3e3d1bb37f3..859bb6027105 100644
@@ -47,6 +47,9 @@
 #include <asm/xen/hypervisor.h>
 #include <asm/xen/hypercall.h>
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+#endif
 
 /*
  * @cpu_id: Xen physical cpu logic number

@@ -400,4 +403,22 @@ bool __init xen_processor_present(uint32_t acpi_id)

Quoted 12 lines

drivers/xen/pcpu.c bool __init xen_processor_present(uint32_t acpi_id) · line 412
Quoted 9 lines
drivers/xen/pcpu.c +7 -0
      return online;
 }
+
+void xen_sanitize_pdc(uint32_t *cap)
+{
+     struct xen_platform_op op = {
+             .cmd                    = XENPF_set_processor_pminfo,
+             .u.set_pminfo.id        = -1,
+             .u.set_pminfo.type      = XEN_PM_PDC,
It would probably be best to rename this constant as well so it's not misleading.

That is a Xen constant, so we can't change it.

Quoted 11 lines

drivers/xen/pcpu.c bool __init xen_processor_present(uint32_t acpi_id) · line 421

Quoted 9 lines

drivers/xen/pcpu.c +9 -0

+     };
+     u32 buf[3] = { ACPI_PDC_REVISION_ID, 1, *cap };
+     int ret;
+
+     set_xen_guest_handle(op.u.set_pminfo.pdc, buf);
+     ret = HYPERVISOR_platform_op(&op);
+     if (ret)
+             pr_info("sanitize of _PDC buffer bits from Xen failed: %d\n",
+                     ret);

Shouldn't an error be pr_err ?

Sure.

Quoted 3 lines

drivers/xen/pcpu.c bool __init xen_processor_present(uint32_t acpi_id) · line 424
Quoted 3 lines
drivers/xen/pcpu.c +2 -0
+     *cap = buf[2];
+}
 #endif

Thanks,
Jason