[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0



On Wed, Nov 01, 2023 at 09:41:52AM -0400, Jason Andryuk wrote:
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> ---
> 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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.