| Message ID | 20230315121031.22450-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] acpi/processor: fix evaluating _PDC method when running as Xen dom0 | expand |
On 15.03.23 13:10, Roger Pau Monne wrote: > In ACPI systems, the OS can direct power management, as opposed to the > firmware. This OS-directed Power Management is called OSPM. Part of > telling the firmware that the OS going to direct power management is > making ACPI "_PDC" (Processor Driver Capabilities) calls. These _PDC > methods must be evaluated for every processor object. If these _PDC > calls are not completed for every processor it can lead to > inconsistency and later failures in things like the CPU frequency > driver. > > In a Xen system, the dom0 kernel is responsible for system-wide power > management. The dom0 kernel is in charge of OSPM. However, the > number of CPUs available to dom0 can be different than the number of > CPUs physically present on the system. > > This leads to a problem: the dom0 kernel needs to evaluate _PDC for > all the processors, but it can't always see them. > > In dom0 kernels, ignore the existing ACPI method for determining if a > processor is physically present because it might not be accurate. > Instead, ask the hypervisor for this information. > > Fix this by introducing a custom function to use when running as Xen > dom0 in order to check whether a processor object matches a CPU that's > online. > > This ensures that _PDC method gets evaluated for all physically online > CPUs, regardless of the number of CPUs made available to dom0. > > Fixes: 5d554a7bb064 ('ACPI: processor: add internal processor_physically_present()') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - Reword commit message. > --- > arch/x86/include/asm/xen/hypervisor.h | 10 ++++++++++ > arch/x86/xen/enlighten.c | 27 +++++++++++++++++++++++++++ > drivers/acpi/processor_pdc.c | 11 +++++++++++ > 3 files changed, 48 insertions(+) > > diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h > index 5fc35f889cd1..f14e39bce2cb 100644 > --- a/arch/x86/include/asm/xen/hypervisor.h > +++ b/arch/x86/include/asm/xen/hypervisor.h > @@ -63,4 +63,14 @@ void __init xen_pvh_init(struct boot_params *boot_params); > void __init mem_map_via_hcall(struct boot_params *boot_params_p); > #endif > > +#ifdef CONFIG_XEN_DOM0 > +bool __init xen_processor_present(uint32_t acpi_id); > +#else > +static inline bool xen_processor_present(uint32_t acpi_id) > +{ > + BUG(); > + return false; > +} > +#endif > + > #endif /* _ASM_X86_XEN_HYPERVISOR_H */ > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c > index b8db2148c07d..d4c44361a26c 100644 > --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -346,3 +346,30 @@ void xen_arch_unregister_cpu(int num) > } > EXPORT_SYMBOL(xen_arch_unregister_cpu); > #endif > + > +#ifdef CONFIG_XEN_DOM0 > +bool __init xen_processor_present(uint32_t acpi_id) > +{ > + unsigned int i, maxid; > + struct xen_platform_op op = { > + .cmd = XENPF_get_cpuinfo, > + .interface_version = XENPF_INTERFACE_VERSION, > + }; > + int ret = HYPERVISOR_platform_op(&op); > + > + if (ret) > + return false; > + > + maxid = op.u.pcpu_info.max_present; > + for (i = 0; i <= maxid; i++) { > + op.u.pcpu_info.xen_cpuid = i; > + ret = HYPERVISOR_platform_op(&op); > + if (ret) > + continue; > + if (op.u.pcpu_info.acpi_id == acpi_id) > + return op.u.pcpu_info.flags & XEN_PCPU_FLAGS_ONLINE; > + } > + > + return false; > +} > +#endif Did you consider not to do the hypercall again and again, but to reuse the pcpu list from drivers/xen/pcpu.c instead? You'd need to store the acpi_id in this list, of course. Juergen
On Wed, Mar 15, 2023 at 01:38:18PM +0100, Juergen Gross wrote: > On 15.03.23 13:10, Roger Pau Monne wrote: > > In ACPI systems, the OS can direct power management, as opposed to the > > firmware. This OS-directed Power Management is called OSPM. Part of > > telling the firmware that the OS going to direct power management is > > making ACPI "_PDC" (Processor Driver Capabilities) calls. These _PDC > > methods must be evaluated for every processor object. If these _PDC > > calls are not completed for every processor it can lead to > > inconsistency and later failures in things like the CPU frequency > > driver. > > > > In a Xen system, the dom0 kernel is responsible for system-wide power > > management. The dom0 kernel is in charge of OSPM. However, the > > number of CPUs available to dom0 can be different than the number of > > CPUs physically present on the system. > > > > This leads to a problem: the dom0 kernel needs to evaluate _PDC for > > all the processors, but it can't always see them. > > > > In dom0 kernels, ignore the existing ACPI method for determining if a > > processor is physically present because it might not be accurate. > > Instead, ask the hypervisor for this information. > > > > Fix this by introducing a custom function to use when running as Xen > > dom0 in order to check whether a processor object matches a CPU that's > > online. > > > > This ensures that _PDC method gets evaluated for all physically online > > CPUs, regardless of the number of CPUs made available to dom0. > > > > Fixes: 5d554a7bb064 ('ACPI: processor: add internal processor_physically_present()') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v1: > > - Reword commit message. > > --- > > arch/x86/include/asm/xen/hypervisor.h | 10 ++++++++++ > > arch/x86/xen/enlighten.c | 27 +++++++++++++++++++++++++++ > > drivers/acpi/processor_pdc.c | 11 +++++++++++ > > 3 files changed, 48 insertions(+) > > > > diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h > > index 5fc35f889cd1..f14e39bce2cb 100644 > > --- a/arch/x86/include/asm/xen/hypervisor.h > > +++ b/arch/x86/include/asm/xen/hypervisor.h > > @@ -63,4 +63,14 @@ void __init xen_pvh_init(struct boot_params *boot_params); > > void __init mem_map_via_hcall(struct boot_params *boot_params_p); > > #endif > > +#ifdef CONFIG_XEN_DOM0 > > +bool __init xen_processor_present(uint32_t acpi_id); > > +#else > > +static inline bool xen_processor_present(uint32_t acpi_id) > > +{ > > + BUG(); > > + return false; > > +} > > +#endif > > + > > #endif /* _ASM_X86_XEN_HYPERVISOR_H */ > > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c > > index b8db2148c07d..d4c44361a26c 100644 > > --- a/arch/x86/xen/enlighten.c > > +++ b/arch/x86/xen/enlighten.c > > @@ -346,3 +346,30 @@ void xen_arch_unregister_cpu(int num) > > } > > EXPORT_SYMBOL(xen_arch_unregister_cpu); > > #endif > > + > > +#ifdef CONFIG_XEN_DOM0 > > +bool __init xen_processor_present(uint32_t acpi_id) > > +{ > > + unsigned int i, maxid; > > + struct xen_platform_op op = { > > + .cmd = XENPF_get_cpuinfo, > > + .interface_version = XENPF_INTERFACE_VERSION, > > + }; > > + int ret = HYPERVISOR_platform_op(&op); > > + > > + if (ret) > > + return false; > > + > > + maxid = op.u.pcpu_info.max_present; > > + for (i = 0; i <= maxid; i++) { > > + op.u.pcpu_info.xen_cpuid = i; > > + ret = HYPERVISOR_platform_op(&op); > > + if (ret) > > + continue; > > + if (op.u.pcpu_info.acpi_id == acpi_id) > > + return op.u.pcpu_info.flags & XEN_PCPU_FLAGS_ONLINE; > > + } > > + > > + return false; > > +} > > +#endif > > Did you consider not to do the hypercall again and again, but to reuse the > pcpu list from drivers/xen/pcpu.c instead? You'd need to store the acpi_id > in this list, of course. Oh, didn't know this was available. Seems to be initialized before the _PDC evaluation, so I can give it a try. Thanks, Roger.
diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h index 5fc35f889cd1..f14e39bce2cb 100644 --- a/arch/x86/include/asm/xen/hypervisor.h +++ b/arch/x86/include/asm/xen/hypervisor.h @@ -63,4 +63,14 @@ void __init xen_pvh_init(struct boot_params *boot_params); void __init mem_map_via_hcall(struct boot_params *boot_params_p); #endif +#ifdef CONFIG_XEN_DOM0 +bool __init xen_processor_present(uint32_t acpi_id); +#else +static inline bool xen_processor_present(uint32_t acpi_id) +{ + BUG(); + return false; +} +#endif + #endif /* _ASM_X86_XEN_HYPERVISOR_H */ diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index b8db2148c07d..d4c44361a26c 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -346,3 +346,30 @@ void xen_arch_unregister_cpu(int num) } EXPORT_SYMBOL(xen_arch_unregister_cpu); #endif + +#ifdef CONFIG_XEN_DOM0 +bool __init xen_processor_present(uint32_t acpi_id) +{ + unsigned int i, maxid; + struct xen_platform_op op = { + .cmd = XENPF_get_cpuinfo, + .interface_version = XENPF_INTERFACE_VERSION, + }; + int ret = HYPERVISOR_platform_op(&op); + + if (ret) + return false; + + maxid = op.u.pcpu_info.max_present; + for (i = 0; i <= maxid; i++) { + op.u.pcpu_info.xen_cpuid = i; + ret = HYPERVISOR_platform_op(&op); + if (ret) + continue; + if (op.u.pcpu_info.acpi_id == acpi_id) + return op.u.pcpu_info.flags & XEN_PCPU_FLAGS_ONLINE; + } + + return false; +} +#endif diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c index 8c3f82c9fff3..18fb04523f93 100644 --- a/drivers/acpi/processor_pdc.c +++ b/drivers/acpi/processor_pdc.c @@ -14,6 +14,8 @@ #include <linux/acpi.h> #include <acpi/processor.h> +#include <xen/xen.h> + #include "internal.h" static bool __init processor_physically_present(acpi_handle handle) @@ -47,6 +49,15 @@ static bool __init processor_physically_present(acpi_handle handle) return false; } + if (xen_initial_domain()) + /* + * When running as a Xen dom0 the number of processors Linux + * sees can be different from the real number of processors on + * the system, and we still need to execute _PDC for all of + * them. + */ + return xen_processor_present(acpi_id); + type = (acpi_type == ACPI_TYPE_DEVICE) ? 1 : 0; cpuid = acpi_get_cpuid(handle, type, acpi_id);
In ACPI systems, the OS can direct power management, as opposed to the firmware. This OS-directed Power Management is called OSPM. Part of telling the firmware that the OS going to direct power management is making ACPI "_PDC" (Processor Driver Capabilities) calls. These _PDC methods must be evaluated for every processor object. If these _PDC calls are not completed for every processor it can lead to inconsistency and later failures in things like the CPU frequency driver. In a Xen system, the dom0 kernel is responsible for system-wide power management. The dom0 kernel is in charge of OSPM. However, the number of CPUs available to dom0 can be different than the number of CPUs physically present on the system. This leads to a problem: the dom0 kernel needs to evaluate _PDC for all the processors, but it can't always see them. In dom0 kernels, ignore the existing ACPI method for determining if a processor is physically present because it might not be accurate. Instead, ask the hypervisor for this information. Fix this by introducing a custom function to use when running as Xen dom0 in order to check whether a processor object matches a CPU that's online. This ensures that _PDC method gets evaluated for all physically online CPUs, regardless of the number of CPUs made available to dom0. Fixes: 5d554a7bb064 ('ACPI: processor: add internal processor_physically_present()') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Reword commit message. --- arch/x86/include/asm/xen/hypervisor.h | 10 ++++++++++ arch/x86/xen/enlighten.c | 27 +++++++++++++++++++++++++++ drivers/acpi/processor_pdc.c | 11 +++++++++++ 3 files changed, 48 insertions(+)