* Re: [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0
2023-11-01 13:41 [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0 Jason Andryuk
@ 2023-11-02 8:29 ` Wilczynski, Michal
2023-11-02 12:39 ` Juergen Gross
2023-11-08 17:02 ` Roger Pau Monné
2 siblings, 0 replies; 4+ messages in thread
From: Wilczynski, Michal @ 2023-11-02 8:29 UTC (permalink / raw)
To: Jason Andryuk, Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
Oleksandr Tyshchenko
Cc: Roger Pau Monne, stable, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, linux-kernel,
xen-devel
On 11/1/2023 2:41 PM, Jason Andryuk wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> 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é <roger.pau@citrix.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> v4:
> Use xen_santize_proc_cap_bits() name - Michal
> Rephrase comment - Michal
>
> 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 | 14 ++++++++++++++
> arch/x86/include/asm/xen/hypervisor.h | 9 +++++++++
> drivers/xen/pcpu.c | 21 +++++++++++++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index c8a7fc23f63c..f896eed4516c 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -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,17 @@ 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
> + * processor capabilities buffer matches what the hypervisor
> + * driver supports.
> + */
> + xen_sanitize_proc_cap_bits(cap);
> + }
> }
>
> static inline bool acpi_has_cpu_in_madt(void)
> diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
> index 7048dfacc04b..a9088250770f 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -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_proc_cap_bits(uint32_t *buf);
> +#else
> +static inline void xen_sanitize_proc_cap_bits(uint32_t *buf)
> +{
> + BUG();
> +}
> +#endif
> +
> #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
> index b3e3d1bb37f3..7000701dff8f 100644
> --- a/drivers/xen/pcpu.c
> +++ b/drivers/xen/pcpu.c
> @@ -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_proc_cap_bits(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_err("sanitize of _PDC buffer bits from Xen failed: %d\n",
> + ret);
> + *cap = buf[2];
> +}
> #endif
Looks good to me.
For what it's worth -
Reviewed-by: Michal Wilczynski <michal.wilczynski@intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0
2023-11-01 13:41 [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0 Jason Andryuk
2023-11-02 8:29 ` Wilczynski, Michal
@ 2023-11-02 12:39 ` Juergen Gross
2023-11-08 17:02 ` Roger Pau Monné
2 siblings, 0 replies; 4+ messages in thread
From: Juergen Gross @ 2023-11-02 12:39 UTC (permalink / raw)
To: Jason Andryuk, Boris Ostrovsky, Stefano Stabellini,
Oleksandr Tyshchenko
Cc: michal.wilczynski, Roger Pau Monne, stable, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
linux-kernel, xen-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 1173 bytes --]
On 01.11.23 14:41, Jason Andryuk wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> 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é <roger.pau@citrix.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0
2023-11-01 13:41 [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0 Jason Andryuk
2023-11-02 8:29 ` Wilczynski, Michal
2023-11-02 12:39 ` Juergen Gross
@ 2023-11-08 17:02 ` Roger Pau Monné
2 siblings, 0 replies; 4+ messages in thread
From: Roger Pau Monné @ 2023-11-08 17:02 UTC (permalink / raw)
To: Jason Andryuk
Cc: Juergen Gross, Boris Ostrovsky, Stefano Stabellini,
Oleksandr Tyshchenko, michal.wilczynski, stable, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
linux-kernel, xen-devel
On Wed, Nov 01, 2023 at 09:41:52AM -0400, Jason Andryuk wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
>
> 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é <roger.pau@citrix.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
> v4:
> Use xen_santize_proc_cap_bits() name - Michal
> Rephrase comment - Michal
>
> 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 | 14 ++++++++++++++
> arch/x86/include/asm/xen/hypervisor.h | 9 +++++++++
> drivers/xen/pcpu.c | 21 +++++++++++++++++++++
> 3 files changed, 44 insertions(+)
>
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index c8a7fc23f63c..f896eed4516c 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -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,17 @@ 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
> + * processor capabilities buffer matches what the hypervisor
> + * driver supports.
> + */
> + xen_sanitize_proc_cap_bits(cap);
> + }
> }
>
> static inline bool acpi_has_cpu_in_madt(void)
> diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
> index 7048dfacc04b..a9088250770f 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -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_proc_cap_bits(uint32_t *buf);
> +#else
> +static inline void xen_sanitize_proc_cap_bits(uint32_t *buf)
> +{
> + BUG();
> +}
> +#endif
> +
> #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
> index b3e3d1bb37f3..7000701dff8f 100644
> --- a/drivers/xen/pcpu.c
> +++ b/drivers/xen/pcpu.c
> @@ -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_proc_cap_bits(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_err("sanitize of _PDC buffer bits from Xen failed: %d\n",
> + ret);
> + *cap = buf[2];
FWIW, we might want to only update cap if the hypercall has been
successful, otherwise there's no guarantee of what's in the buffer, so
I would recommend to put the updating of cap in an else branch.
Anyway, not a strong opinion, as I think in practice even if the
hypercall fails it wouldn't corrupt the data in the buffer, but seems
more robust.
Thanks, Roger.
^ permalink raw reply [flat|nested] 4+ messages in thread