[PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monne]

When running as a Xen dom0 the number of CPUs available to Linux can
be different from the number of CPUs present on the system, but in
order to properly fetch processor performance related data _PDC must
be executed on all the physical CPUs online on the system.

The current checks in processor_physically_present() result in some
processor objects not getting their _PDC methods evaluated when Linux
is running as Xen dom0.  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.

Fixes: 5d554a7bb064 ('ACPI: processor: add internal processor_physically_present()')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 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 16f548a661cf..b9f512138043 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -61,4 +61,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);
 
-- 
2.37.3

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Jan Beulich]

On 21.11.2022 11:21, Roger Pau Monne wrote:
> @@ -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);

We had to deal with this in our XenoLinux forward ports as well, but at
the time it appeared upstream I decided to make use of acpi_get_apicid()
(which meanwhile was renamed to acpi_get_phys_id()). Wouldn't than be an
option, eliminating the need for a Xen-specific new function?

Jan

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

On Mon, Nov 21, 2022 at 03:02:30PM +0100, Jan Beulich wrote:
> On 21.11.2022 11:21, Roger Pau Monne wrote:
> > @@ -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);
> 
> We had to deal with this in our XenoLinux forward ports as well, but at
> the time it appeared upstream I decided to make use of acpi_get_apicid()
> (which meanwhile was renamed to acpi_get_phys_id()). Wouldn't than be an
> option, eliminating the need for a Xen-specific new function?

While this would work for PV, it won't work on a PVH dom0, since the
ACPI MADT table is not the native one in that case, and thus the
Processor UIDs in the MADT don't match the ones in the Processor
objects/devices.

Thanks, Roger.

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Jan Beulich]

On 21.11.2022 15:29, Roger Pau Monné wrote:
> On Mon, Nov 21, 2022 at 03:02:30PM +0100, Jan Beulich wrote:
>> On 21.11.2022 11:21, Roger Pau Monne wrote:
>>> @@ -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);
>>
>> We had to deal with this in our XenoLinux forward ports as well, but at
>> the time it appeared upstream I decided to make use of acpi_get_apicid()
>> (which meanwhile was renamed to acpi_get_phys_id()). Wouldn't than be an
>> option, eliminating the need for a Xen-specific new function?
> 
> While this would work for PV, it won't work on a PVH dom0, since the
> ACPI MADT table is not the native one in that case, and thus the
> Processor UIDs in the MADT don't match the ones in the Processor
> objects/devices.

I wonder whether we can actually get away with this difference long term.
I've gone back and looked at the commit introducing the code to build the
replacement MADT, but there's no mention of either the reason for the
changed numbering or the reason for limiting MADT entries to just the
number of CPUs Dom0 will have. (Clearly we need distinct APIC IDs, at
least until Xen becomes more flexible / correct in this regard. And
clearly we'd need to "invent" ACPI IDs in case Dom0 had more vCPU-s than
there are pCPU-s.)

Jan

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

On Mon, Nov 21, 2022 at 03:51:58PM +0100, Jan Beulich wrote:
> On 21.11.2022 15:29, Roger Pau Monné wrote:
> > On Mon, Nov 21, 2022 at 03:02:30PM +0100, Jan Beulich wrote:
> >> On 21.11.2022 11:21, Roger Pau Monne wrote:
> >>> @@ -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);
> >>
> >> We had to deal with this in our XenoLinux forward ports as well, but at
> >> the time it appeared upstream I decided to make use of acpi_get_apicid()
> >> (which meanwhile was renamed to acpi_get_phys_id()). Wouldn't than be an
> >> option, eliminating the need for a Xen-specific new function?
> > 
> > While this would work for PV, it won't work on a PVH dom0, since the
> > ACPI MADT table is not the native one in that case, and thus the
> > Processor UIDs in the MADT don't match the ones in the Processor
> > objects/devices.
> 
> I wonder whether we can actually get away with this difference long term.
> I've gone back and looked at the commit introducing the code to build the
> replacement MADT, but there's no mention of either the reason for the
> changed numbering or the reason for limiting MADT entries to just the
> number of CPUs Dom0 will have. (Clearly we need distinct APIC IDs, at
> least until Xen becomes more flexible / correct in this regard. And
> clearly we'd need to "invent" ACPI IDs in case Dom0 had more vCPU-s than
> there are pCPU-s.)

Linux when running in PVH/HVM mode uses the ACPI Processor UID in the
MADT as the vCPU ID, so attempting to re-use the native UIDs doesn't
work.

We could expand the dom0 crafted MADT to make sure all the native ACPI
Processor UIDs are present in the crafted MADT, by adding them as not
present entries, but that seems more like a bodge than a proper
solution.  Even then those X2APIC entries would appear as offline by
the current checks, and thus won't get _PDC evaluated either.

Thanks, Roger.

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

Ping?

So far I've got some feedback from Jan which I've replied to, but I
haven't got any more feedback.

Both patches 1 and 2 are required in order for Xen dom0s to properly
handle ACPI Processor related data to the hypervisor. Patch 3 can be
deal with later.

Thanks, Roger.

On Mon, Nov 21, 2022 at 11:21:10AM +0100, Roger Pau Monne wrote:
> When running as a Xen dom0 the number of CPUs available to Linux can
> be different from the number of CPUs present on the system, but in
> order to properly fetch processor performance related data _PDC must
> be executed on all the physical CPUs online on the system.
> 
> The current checks in processor_physically_present() result in some
> processor objects not getting their _PDC methods evaluated when Linux
> is running as Xen dom0.  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.
> 
> Fixes: 5d554a7bb064 ('ACPI: processor: add internal processor_physically_present()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  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 16f548a661cf..b9f512138043 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -61,4 +61,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);
>  
> -- 
> 2.37.3
> 

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Dave Hansen]

On 11/21/22 02:21, Roger Pau Monne wrote:
> When running as a Xen dom0 the number of CPUs available to Linux can
> be different from the number of CPUs present on the system, but in
> order to properly fetch processor performance related data _PDC must
> be executed on all the physical CPUs online on the system.

How is the number of CPUs available to Linux different?

Is this a result of the ACPI tables that dom0 sees being "wrong"?

> The current checks in processor_physically_present() result in some
> processor objects not getting their _PDC methods evaluated when Linux
> is running as Xen dom0.  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.

What is the end user visible effect of this problem and of the solution?

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

On Tue, Nov 29, 2022 at 09:43:53AM -0800, Dave Hansen wrote:
> On 11/21/22 02:21, Roger Pau Monne wrote:
> > When running as a Xen dom0 the number of CPUs available to Linux can
> > be different from the number of CPUs present on the system, but in
> > order to properly fetch processor performance related data _PDC must
> > be executed on all the physical CPUs online on the system.
> 
> How is the number of CPUs available to Linux different?
> 
> Is this a result of the ACPI tables that dom0 sees being "wrong"?

Depends on the mode.  This is all specific to Linux running as a Xen
dom0.

For PV dom0 the ACPI tables that dom0 sees are the native ones,
however available CPUs are not detected based on the MADT, but using
hypercalls, see xen_smp_ops struct and the
x86_init.mpparse.get_smp_config hook used in smp_pv.c
(_get_smp_config()).

For a PVH dom0 Xen provides dom0 with a crafted MADT table that does
only contain the CPUs available to dom0, and hence is likely different
from the native one present on the hardware.

In any case, the dynamic tables dom0 sees where the Processor
objects/devices reside are not modified by Xen in any way, so the ACPI
Processors are always exposed to dom0 as present on the native
tables.

Xen cannot parse the dynamic ACPI tables (neither should it, since
then it would act as OSPM), so it relies on dom0 to provide same data
present on those tables for Xen to properly manage the frequency and
idle states of the CPUs on the system.

> > The current checks in processor_physically_present() result in some
> > processor objects not getting their _PDC methods evaluated when Linux
> > is running as Xen dom0.  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.
> 
> What is the end user visible effect of this problem and of the solution?

Without this fix _PDC is only evaluated for the CPUs online from dom0
point of view, which means that if dom0 is limited to 8 CPUs but the
system has 24 CPUs, _PDC will only get evaluated for 8 CPUs, and that
can have the side effect of the data then returned by _PSD method or
other methods being different between CPUs where _PDC was evaluated vs
CPUs where the method wasn't evaluated.  Such mismatches can
ultimately lead to for example the CPU frequency driver in Xen not
initializing properly because the coordination methods between CPUs on
the same domain don't match.

Also not evaluating _PDC prevents the OS (or Xen in this case)
from notifying ACPI of the features it supports.

IOW this fix attempts to make sure all physically online CPUs get _PDC
evaluated, and in order to to that we need to ask the hypervisor if a
Processor ACPI ID matches an online CPU or not, because Linux doesn't
have that information when running as dom0.

Hope the above makes sense and allows to make some progress on the
issue, sometimes it's hard to summarize without getting too
specific,

Thanks, Roger.

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Dave Hansen]

On 11/30/22 07:53, Roger Pau Monné wrote:
> On Tue, Nov 29, 2022 at 09:43:53AM -0800, Dave Hansen wrote:
>> On 11/21/22 02:21, Roger Pau Monne wrote:
>>> When running as a Xen dom0 the number of CPUs available to Linux can
>>> be different from the number of CPUs present on the system, but in
>>> order to properly fetch processor performance related data _PDC must
>>> be executed on all the physical CPUs online on the system.
>>
>> How is the number of CPUs available to Linux different?
>>
>> Is this a result of the ACPI tables that dom0 sees being "wrong"?
> 
> Depends on the mode.  This is all specific to Linux running as a Xen
> dom0.
> 
> For PV dom0 the ACPI tables that dom0 sees are the native ones,
> however available CPUs are not detected based on the MADT, but using
> hypercalls, see xen_smp_ops struct and the
> x86_init.mpparse.get_smp_config hook used in smp_pv.c
> (_get_smp_config()).
> 
> For a PVH dom0 Xen provides dom0 with a crafted MADT table that does
> only contain the CPUs available to dom0, and hence is likely different
> from the native one present on the hardware.
> 
> In any case, the dynamic tables dom0 sees where the Processor
> objects/devices reside are not modified by Xen in any way, so the ACPI
> Processors are always exposed to dom0 as present on the native
> tables.
> 
> Xen cannot parse the dynamic ACPI tables (neither should it, since
> then it would act as OSPM), so it relies on dom0 to provide same data
> present on those tables for Xen to properly manage the frequency and
> idle states of the CPUs on the system.
> 
>>> The current checks in processor_physically_present() result in some
>>> processor objects not getting their _PDC methods evaluated when Linux
>>> is running as Xen dom0.  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.
>>
>> What is the end user visible effect of this problem and of the solution?
> 
> Without this fix _PDC is only evaluated for the CPUs online from dom0
> point of view, which means that if dom0 is limited to 8 CPUs but the
> system has 24 CPUs, _PDC will only get evaluated for 8 CPUs, and that
> can have the side effect of the data then returned by _PSD method or
> other methods being different between CPUs where _PDC was evaluated vs
> CPUs where the method wasn't evaluated.  Such mismatches can
> ultimately lead to for example the CPU frequency driver in Xen not
> initializing properly because the coordination methods between CPUs on
> the same domain don't match.
> 
> Also not evaluating _PDC prevents the OS (or Xen in this case)
> from notifying ACPI of the features it supports.
> 
> IOW this fix attempts to make sure all physically online CPUs get _PDC
> evaluated, and in order to to that we need to ask the hypervisor if a
> Processor ACPI ID matches an online CPU or not, because Linux doesn't
> have that information when running as dom0.
> 
> Hope the above makes sense and allows to make some progress on the
> issue, sometimes it's hard to summarize without getting too
> specific,

Yes, writing changelogs is hard. :)

Let's try though.  I was missing some key pieces of background here.
Believe it or not, I had no idea off the top of my head what _PDC was or
why it's important.

the information about _PDC being required on all processors was missing,
as was the information about the dom0's incomplete concept of the
available physical processors.

== Background ==

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
calls must be made on every processor.  If these _PDC calls are not
completed on 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 Xen
hypervisor hides some processors information from the dom0 kernel.  This
is presumably done to ensure that the dom0 system has less interference
with guests that want to use the other processors.

== Problem ==

But, this leads to a problem: the dom0 kernel needs to run _PDC on all
the processors, but it can't always see them.

== Solution ==

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.

This ensures that ...

----

Is that about right?

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

On Wed, Nov 30, 2022 at 08:48:14AM -0800, Dave Hansen wrote:
> On 11/30/22 07:53, Roger Pau Monné wrote:
> > On Tue, Nov 29, 2022 at 09:43:53AM -0800, Dave Hansen wrote:
> >> On 11/21/22 02:21, Roger Pau Monne wrote:
> >>> When running as a Xen dom0 the number of CPUs available to Linux can
> >>> be different from the number of CPUs present on the system, but in
> >>> order to properly fetch processor performance related data _PDC must
> >>> be executed on all the physical CPUs online on the system.
> >>
> >> How is the number of CPUs available to Linux different?
> >>
> >> Is this a result of the ACPI tables that dom0 sees being "wrong"?
> > 
> > Depends on the mode.  This is all specific to Linux running as a Xen
> > dom0.
> > 
> > For PV dom0 the ACPI tables that dom0 sees are the native ones,
> > however available CPUs are not detected based on the MADT, but using
> > hypercalls, see xen_smp_ops struct and the
> > x86_init.mpparse.get_smp_config hook used in smp_pv.c
> > (_get_smp_config()).
> > 
> > For a PVH dom0 Xen provides dom0 with a crafted MADT table that does
> > only contain the CPUs available to dom0, and hence is likely different
> > from the native one present on the hardware.
> > 
> > In any case, the dynamic tables dom0 sees where the Processor
> > objects/devices reside are not modified by Xen in any way, so the ACPI
> > Processors are always exposed to dom0 as present on the native
> > tables.
> > 
> > Xen cannot parse the dynamic ACPI tables (neither should it, since
> > then it would act as OSPM), so it relies on dom0 to provide same data
> > present on those tables for Xen to properly manage the frequency and
> > idle states of the CPUs on the system.
> > 
> >>> The current checks in processor_physically_present() result in some
> >>> processor objects not getting their _PDC methods evaluated when Linux
> >>> is running as Xen dom0.  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.
> >>
> >> What is the end user visible effect of this problem and of the solution?
> > 
> > Without this fix _PDC is only evaluated for the CPUs online from dom0
> > point of view, which means that if dom0 is limited to 8 CPUs but the
> > system has 24 CPUs, _PDC will only get evaluated for 8 CPUs, and that
> > can have the side effect of the data then returned by _PSD method or
> > other methods being different between CPUs where _PDC was evaluated vs
> > CPUs where the method wasn't evaluated.  Such mismatches can
> > ultimately lead to for example the CPU frequency driver in Xen not
> > initializing properly because the coordination methods between CPUs on
> > the same domain don't match.
> > 
> > Also not evaluating _PDC prevents the OS (or Xen in this case)
> > from notifying ACPI of the features it supports.
> > 
> > IOW this fix attempts to make sure all physically online CPUs get _PDC
> > evaluated, and in order to to that we need to ask the hypervisor if a
> > Processor ACPI ID matches an online CPU or not, because Linux doesn't
> > have that information when running as dom0.
> > 
> > Hope the above makes sense and allows to make some progress on the
> > issue, sometimes it's hard to summarize without getting too
> > specific,
> 
> Yes, writing changelogs is hard. :)
> 
> Let's try though.  I was missing some key pieces of background here.
> Believe it or not, I had no idea off the top of my head what _PDC was or
> why it's important.
> 
> the information about _PDC being required on all processors was missing,
> as was the information about the dom0's incomplete concept of the
> available physical processors.
> 
> == Background ==
> 
> 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
> calls must be made on every processor.  If these _PDC calls are not
> completed on every processor it can lead to inconsistency and later
> failures in things like the CPU frequency driver.

I think the "on every processor" is not fully accurate.  _PDC methods
need to be evaluated for every Processor object.  Whether that
evaluation is executed on the physical processor that matches the ACPI
UID of the object/device is not mandatory (iow: you can evaluate
the _PDC methods of all Processor objects from the BSP).  The usage of
'on' seems to me to note that the methods are executed on the matching
physical processors.

I would instead use: "... must be made for every processor.  If these
_PDC calls are not completed for every processor..."

But I'm not a native English speaker, so this might all be irrelevant.

> 
> In a Xen system, the dom0 kernel is responsible for system-wide power
> management.  The dom0 kernel is in charge of OSPM.  However, the Xen
> hypervisor hides some processors information from the dom0 kernel.  This
> is presumably done to ensure that the dom0 system has less interference
> with guests that want to use the other processors.

dom0 on a Xen system is just another guest, so the admin can limit the
number of CPUs available to dom0, that's why we get into this weird
situation.

> == Problem ==
> 
> But, this leads to a problem: the dom0 kernel needs to run _PDC on all
> the processors, but it can't always see them.
> 
> == Solution ==
> 
> 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.
> 
> This ensures that ...
> 
> ----
> 
> Is that about right?

Yes, I think it's accurate.  I will add to my commit log, thanks!

On the implementation side, is the proposed approach acceptable?
Mostly asking because it adds Xen conditionals to otherwise generic
ACPI code.

Thanks, Roger.

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Dave Hansen]

On 12/2/22 04:24, Roger Pau Monné wrote:
> On the implementation side, is the proposed approach acceptable?
> Mostly asking because it adds Xen conditionals to otherwise generic
> ACPI code.

That's a good Rafael question.

But, how do other places in the ACPI code handle things like this?

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

On Fri, Dec 02, 2022 at 08:17:56AM -0800, Dave Hansen wrote:
> On 12/2/22 04:24, Roger Pau Monné wrote:
> > On the implementation side, is the proposed approach acceptable?
> > Mostly asking because it adds Xen conditionals to otherwise generic
> > ACPI code.
> 
> That's a good Rafael question.
> 
> But, how do other places in the ACPI code handle things like this?

Hm, I don't know of other places in the Xen case, the only resource
in ACPI AML tables managed by Xen are Processor objects/devices AFAIK.
The rest of devices are fully managed by the dom0 guest.

I think such special handling is very specific to Xen, but maybe I'm
wrong and there are similar existing cases in ACPI code already.

We could add some kind of hook (iow: a function pointer in some struct
that could be filled on a implementation basis?) but I didn't want
overengineering this if adding a conditional was deemed OK.

Thanks, Roger.

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Rafael J. Wysocki]

On Fri, Dec 2, 2022 at 5:37 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Fri, Dec 02, 2022 at 08:17:56AM -0800, Dave Hansen wrote:
> > On 12/2/22 04:24, Roger Pau Monné wrote:
> > > On the implementation side, is the proposed approach acceptable?
> > > Mostly asking because it adds Xen conditionals to otherwise generic
> > > ACPI code.
> >
> > That's a good Rafael question.

Sorry for joining late, but first off _PDC has been deprecated since
ACPI 3.0 (2004) and it is not even present in ACPI 6.5 any more.

It follows from your description that _PDC is still used in the field,
though, after 18 years of deprecation.  Who uses it, if I may know?

> > But, how do other places in the ACPI code handle things like this?
>
> Hm, I don't know of other places in the Xen case, the only resource
> in ACPI AML tables managed by Xen are Processor objects/devices AFAIK.
> The rest of devices are fully managed by the dom0 guest.
>
> I think such special handling is very specific to Xen, but maybe I'm
> wrong and there are similar existing cases in ACPI code already.
>
> We could add some kind of hook (iow: a function pointer in some struct
> that could be filled on a implementation basis?) but I didn't want
> overengineering this if adding a conditional was deemed OK.

What _PDC capabilities specifically do you need to pass to the
firmware for things to work correctly?

What platforms are affected?

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Roger Pau Monné]

On Fri, Dec 02, 2022 at 06:06:26PM +0100, Rafael J. Wysocki wrote:
> On Fri, Dec 2, 2022 at 5:37 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Fri, Dec 02, 2022 at 08:17:56AM -0800, Dave Hansen wrote:
> > > On 12/2/22 04:24, Roger Pau Monné wrote:
> > > > On the implementation side, is the proposed approach acceptable?
> > > > Mostly asking because it adds Xen conditionals to otherwise generic
> > > > ACPI code.
> > >
> > > That's a good Rafael question.
> 
> Sorry for joining late, but first off _PDC has been deprecated since
> ACPI 3.0 (2004) and it is not even present in ACPI 6.5 any more.
> 
> It follows from your description that _PDC is still used in the field,
> though, after 18 years of deprecation.  Who uses it, if I may know?

I saw this issue on a Sapphire Rapids SDP from Intel, but I would
think there are other platforms affected.

> > > But, how do other places in the ACPI code handle things like this?
> >
> > Hm, I don't know of other places in the Xen case, the only resource
> > in ACPI AML tables managed by Xen are Processor objects/devices AFAIK.
> > The rest of devices are fully managed by the dom0 guest.
> >
> > I think such special handling is very specific to Xen, but maybe I'm
> > wrong and there are similar existing cases in ACPI code already.
> >
> > We could add some kind of hook (iow: a function pointer in some struct
> > that could be filled on a implementation basis?) but I didn't want
> > overengineering this if adding a conditional was deemed OK.
> 
> What _PDC capabilities specifically do you need to pass to the
> firmware for things to work correctly?

I'm not sure what capabilities do I need to pass explicitly to _PSD,
but the call to _PDC as done by Linux currently changes the reported
_PSD Coordination Field (vs not doing the call).

Thanks, Roger.

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Josef Johansson]

On 11/21/22 11:21, Roger Pau Monne wrote:
> When running as a Xen dom0 the number of CPUs available to Linux can
> be different from the number of CPUs present on the system, but in
> order to properly fetch processor performance related data _PDC must
> be executed on all the physical CPUs online on the system.
>
> The current checks in processor_physically_present() result in some
> processor objects not getting their _PDC methods evaluated when Linux
> is running as Xen dom0.  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.
>
> Fixes: 5d554a7bb064 ('ACPI: processor: add internal processor_physically_present()')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   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 16f548a661cf..b9f512138043 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -61,4 +61,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;
> +}
My compiler (Default GCC on Fedora 32, compiling for Qubes) complain 
loudly that the below was missing.

+}
+EXPORT_SYMBOL(xen_processor_present);

`ERROR: MODPOST xen_processor_present 
[drivers/xen/xen-acpi-processor.ko] undefined!`

Same thing with xen_sanitize_pdc in the next patch.

+}
+EXPORT_SYMBOL(xen_sanitize_pdc);

Everything compiled fine after those changes.
> +#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);
>   

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Jan Beulich]

On 30.01.2023 10:21, Josef Johansson wrote:
> On 11/21/22 11:21, Roger Pau Monne wrote:
>> --- 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;
>> +}
> My compiler (Default GCC on Fedora 32, compiling for Qubes) complain 
> loudly that the below was missing.
> 
> +}
> +EXPORT_SYMBOL(xen_processor_present);
> 
> `ERROR: MODPOST xen_processor_present 
> [drivers/xen/xen-acpi-processor.ko] undefined!`
> 
> Same thing with xen_sanitize_pdc in the next patch.
> 
> +}
> +EXPORT_SYMBOL(xen_sanitize_pdc);
> 
> Everything compiled fine after those changes.

Except that you may not export __init symbols. The section mismatch checker
should actually complain about that.

Jan

Re: [PATCH 1/3] acpi/processor: fix evaluating _PDC method when running as Xen dom0

[Josef Johansson]

On 2/3/23 08:05, Jan Beulich wrote:
> On 30.01.2023 10:21, Josef Johansson wrote:
>> On 11/21/22 11:21, Roger Pau Monne wrote:
>>> --- 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;
>>> +}
>> My compiler (Default GCC on Fedora 32, compiling for Qubes) complain
>> loudly that the below was missing.
>>
>> +}
>> +EXPORT_SYMBOL(xen_processor_present);
>>
>> `ERROR: MODPOST xen_processor_present
>> [drivers/xen/xen-acpi-processor.ko] undefined!`
>>
>> Same thing with xen_sanitize_pdc in the next patch.
>>
>> +}
>> +EXPORT_SYMBOL(xen_sanitize_pdc);
>>
>> Everything compiled fine after those changes.
> Except that you may not export __init symbols. The section mismatch checker
> should actually complain about that.
>
> Jan

That makes sense. Patch 3 does change it from an __init though.

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 394dd6675113..a7b41103d3e5 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -348,7 +348,7 @@ EXPORT_SYMBOL(xen_arch_unregister_cpu);
  #endif

  #ifdef CONFIG_XEN_DOM0
-bool __init xen_processor_present(uint32_t acpi_id)
+bool xen_processor_present(uint32_t acpi_id)
  {


So the change should be in Patch 3 I guess.

Regards
- Josef

[PATCH 2/3] acpi/processor: sanitize _PDC buffer bits when running as Xen dom0

[Roger Pau Monne]

The Processor _PDC buffer 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, it's the hypervisor the entity
in charge of processor power management, and hence Xen needs to make
sure the capabilities reported in the _PDC buffer match the
capabilities of the driver in Xen.

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

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: stable@vger.kernel.org
---
 arch/x86/include/asm/xen/hypervisor.h |  2 ++
 arch/x86/xen/enlighten.c              | 17 +++++++++++++++++
 drivers/acpi/processor_pdc.c          |  8 ++++++++
 3 files changed, 27 insertions(+)

diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index b9f512138043..b4ed90ef5e68 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -63,12 +63,14 @@ void __init mem_map_via_hcall(struct boot_params *boot_params_p);
 
 #ifdef CONFIG_XEN_DOM0
 bool __init xen_processor_present(uint32_t acpi_id);
+void xen_sanitize_pdc(uint32_t *buf);
 #else
 static inline bool xen_processor_present(uint32_t acpi_id)
 {
 	BUG();
 	return false;
 }
+static inline void xen_sanitize_pdc(uint32_t *buf) { BUG(); }
 #endif
 
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index d4c44361a26c..394dd6675113 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -372,4 +372,21 @@ bool __init xen_processor_present(uint32_t acpi_id)
 
 	return false;
 }
+
+void xen_sanitize_pdc(uint32_t *buf)
+{
+	struct xen_platform_op op = {
+		.cmd			= XENPF_set_processor_pminfo,
+		.interface_version	= XENPF_INTERFACE_VERSION,
+		.u.set_pminfo.id	= -1,
+		.u.set_pminfo.type	= XEN_PM_PDC,
+	};
+	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);
+}
 #endif
diff --git a/drivers/acpi/processor_pdc.c b/drivers/acpi/processor_pdc.c
index 18fb04523f93..58f4c208517a 100644
--- a/drivers/acpi/processor_pdc.c
+++ b/drivers/acpi/processor_pdc.c
@@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
 
 	}
+	if (xen_initial_domain())
+		/*
+		 * When Linux is running as Xen dom0 it's the hypervisor 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((uint32_t *)pdc_in->pointer->buffer.pointer);
 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
 
 	if (ACPI_FAILURE(status))
-- 
2.37.3

Re: [PATCH 2/3] acpi/processor: sanitize _PDC buffer bits when running as Xen dom0

[Jan Beulich]

On 21.11.2022 11:21, Roger Pau Monne wrote:
> --- a/drivers/acpi/processor_pdc.c
> +++ b/drivers/acpi/processor_pdc.c
> @@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
>  		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>  
>  	}
> +	if (xen_initial_domain())
> +		/*
> +		 * When Linux is running as Xen dom0 it's the hypervisor 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((uint32_t *)pdc_in->pointer->buffer.pointer);
>  	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);

Again looking at our old XenoLinux forward port we had this inside the
earlier if(), as an _alternative_ to the &= (I don't think it's valid
to apply both the kernel's and Xen's adjustments). That would also let
you use "buffer" rather than re-calculating it via yet another (risky
from an abstract pov) cast.

It was the very nature of requiring Xen-specific conditionals which I
understand was the reason why so far no attempt was made to get this
(incl the corresponding logic for patch 1) into any upstream kernel.

Jan

Re: [PATCH 2/3] acpi/processor: sanitize _PDC buffer bits when running as Xen dom0

[Jan Beulich]

On 21.11.2022 15:10, Jan Beulich wrote:
> On 21.11.2022 11:21, Roger Pau Monne wrote:
>> --- a/drivers/acpi/processor_pdc.c
>> +++ b/drivers/acpi/processor_pdc.c
>> @@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
>>  		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>>  
>>  	}
>> +	if (xen_initial_domain())
>> +		/*
>> +		 * When Linux is running as Xen dom0 it's the hypervisor 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((uint32_t *)pdc_in->pointer->buffer.pointer);
>>  	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> 
> Again looking at our old XenoLinux forward port we had this inside the
> earlier if(), as an _alternative_ to the &= (I don't think it's valid
> to apply both the kernel's and Xen's adjustments). That would also let
> you use "buffer" rather than re-calculating it via yet another (risky
> from an abstract pov) cast.

Oh, I notice this can end up being misleading: Besides having it in the
earlier if() we had also #ifdef-ed out that if() itself (keeping just
the body). The equivalent of this here might then be

	if (boot_option_idle_override == IDLE_NOMWAIT || xen_initial_domain()) {

Jan

Re: [PATCH 2/3] acpi/processor: sanitize _PDC buffer bits when running as Xen dom0

[Roger Pau Monné]

On Mon, Nov 21, 2022 at 03:10:36PM +0100, Jan Beulich wrote:
> On 21.11.2022 11:21, Roger Pau Monne wrote:
> > --- a/drivers/acpi/processor_pdc.c
> > +++ b/drivers/acpi/processor_pdc.c
> > @@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> >  		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> >  
> >  	}
> > +	if (xen_initial_domain())
> > +		/*
> > +		 * When Linux is running as Xen dom0 it's the hypervisor 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((uint32_t *)pdc_in->pointer->buffer.pointer);
> >  	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> 
> Again looking at our old XenoLinux forward port we had this inside the
> earlier if(), as an _alternative_ to the &= (I don't think it's valid
> to apply both the kernel's and Xen's adjustments). That would also let
> you use "buffer" rather than re-calculating it via yet another (risky
> from an abstract pov) cast.

Hm, I've wondered this and decided it wasn't worth to short-circuit
the boot_option_idle_override conditional because ACPI_PDC_C_C2C3_FFH
and ACPI_PDC_C_C1_FFH will be set anyway by Xen in
arch_acpi_set_pdc_bits() as part of ACPI_PDC_C_CAPABILITY_SMP.

I could re-use some of the code in there, but didn't want to make it
more difficult to read just for the benefit of reusing buffer.

> It was the very nature of requiring Xen-specific conditionals which I
> understand was the reason why so far no attempt was made to get this
> (incl the corresponding logic for patch 1) into any upstream kernel.

Yes, well, it's all kind of ugly.  Hence my suggestion to simply avoid
doing any ACPI Processor object handling in Linux with the native code
and handle it all in a Xen specific driver.  That requires the Xen
driver being able to fetch more data itself form the ACPI Processor
methods, but also unties it from the dependency on the data being
filled by the generic code, and the 'tricks' is plays into fooling
generic code to think certain processors are online.

Thanks, Roger.

Re: [PATCH 2/3] acpi/processor: sanitize _PDC buffer bits when running as Xen dom0

[Jason Andryuk]

Hi, Roger,

On Mon, Nov 21, 2022 at 10:04 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Mon, Nov 21, 2022 at 03:10:36PM +0100, Jan Beulich wrote:
> > On 21.11.2022 11:21, Roger Pau Monne wrote:
> > > --- a/drivers/acpi/processor_pdc.c
> > > +++ b/drivers/acpi/processor_pdc.c
> > > @@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> > >             buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> > >
> > >     }
> > > +   if (xen_initial_domain())
> > > +           /*
> > > +            * When Linux is running as Xen dom0 it's the hypervisor 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((uint32_t *)pdc_in->pointer->buffer.pointer);
> > >     status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> >
> > Again looking at our old XenoLinux forward port we had this inside the
> > earlier if(), as an _alternative_ to the &= (I don't think it's valid
> > to apply both the kernel's and Xen's adjustments). That would also let
> > you use "buffer" rather than re-calculating it via yet another (risky
> > from an abstract pov) cast.
>
> Hm, I've wondered this and decided it wasn't worth to short-circuit
> the boot_option_idle_override conditional because ACPI_PDC_C_C2C3_FFH
> and ACPI_PDC_C_C1_FFH will be set anyway by Xen in
> arch_acpi_set_pdc_bits() as part of ACPI_PDC_C_CAPABILITY_SMP.
>
> I could re-use some of the code in there, but didn't want to make it
> more difficult to read just for the benefit of reusing buffer.
>
> > It was the very nature of requiring Xen-specific conditionals which I
> > understand was the reason why so far no attempt was made to get this
> > (incl the corresponding logic for patch 1) into any upstream kernel.
>
> Yes, well, it's all kind of ugly.  Hence my suggestion to simply avoid
> doing any ACPI Processor object handling in Linux with the native code
> and handle it all in a Xen specific driver.  That requires the Xen
> driver being able to fetch more data itself form the ACPI Processor
> methods, but also unties it from the dependency on the data being
> filled by the generic code, and the 'tricks' is plays into fooling
> generic code to think certain processors are online.

Are you working on this patch anymore?  My Xen HWP patches need a
Linux patch like this one to set bit 12 in the PDC.  I had an affected
user test with this patch and it worked, serving as an equivalent of
Linux commit a21211672c9a ("ACPI / processor: Request native thermal
interrupt handling via _OSC").

Another idea is to use Linux's arch_acpi_set_pdc_bits() to make the
hypercall to Xen.  It occurs earlier:
acpi_processor_set_pdc()
    acpi_processor_alloc_pdc()
        acpi_set_pdc_bits()
            arch_acpi_set_pdc_bits()
    acpi_processor_eval_pdc()

So the IDLE_NOMWAIT masking in acpi_processor_eval_pdc() would still
apply.  arch_acpi_set_pdc_bits() is provided the buffer, so it's a
little cleaner in that respect.

Thanks,
Jason

Re: [PATCH 2/3] acpi/processor: sanitize _PDC buffer bits when running as Xen dom0

[Roger Pau Monné]

On Wed, Jun 14, 2023 at 03:57:11PM -0400, Jason Andryuk wrote:
> Hi, Roger,
> 
> On Mon, Nov 21, 2022 at 10:04 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Mon, Nov 21, 2022 at 03:10:36PM +0100, Jan Beulich wrote:
> > > On 21.11.2022 11:21, Roger Pau Monne wrote:
> > > > --- a/drivers/acpi/processor_pdc.c
> > > > +++ b/drivers/acpi/processor_pdc.c
> > > > @@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> > > >             buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> > > >
> > > >     }
> > > > +   if (xen_initial_domain())
> > > > +           /*
> > > > +            * When Linux is running as Xen dom0 it's the hypervisor 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((uint32_t *)pdc_in->pointer->buffer.pointer);
> > > >     status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> > >
> > > Again looking at our old XenoLinux forward port we had this inside the
> > > earlier if(), as an _alternative_ to the &= (I don't think it's valid
> > > to apply both the kernel's and Xen's adjustments). That would also let
> > > you use "buffer" rather than re-calculating it via yet another (risky
> > > from an abstract pov) cast.
> >
> > Hm, I've wondered this and decided it wasn't worth to short-circuit
> > the boot_option_idle_override conditional because ACPI_PDC_C_C2C3_FFH
> > and ACPI_PDC_C_C1_FFH will be set anyway by Xen in
> > arch_acpi_set_pdc_bits() as part of ACPI_PDC_C_CAPABILITY_SMP.
> >
> > I could re-use some of the code in there, but didn't want to make it
> > more difficult to read just for the benefit of reusing buffer.
> >
> > > It was the very nature of requiring Xen-specific conditionals which I
> > > understand was the reason why so far no attempt was made to get this
> > > (incl the corresponding logic for patch 1) into any upstream kernel.
> >
> > Yes, well, it's all kind of ugly.  Hence my suggestion to simply avoid
> > doing any ACPI Processor object handling in Linux with the native code
> > and handle it all in a Xen specific driver.  That requires the Xen
> > driver being able to fetch more data itself form the ACPI Processor
> > methods, but also unties it from the dependency on the data being
> > filled by the generic code, and the 'tricks' is plays into fooling
> > generic code to think certain processors are online.
> 
> Are you working on this patch anymore?

Not really, I didn't get any feedback from maintainers (apart from
Jans comments, which I do value), and wasn't aware of this causing
issues, or being required by any other work, hence I kind of dropped
it (I have plenty of other stuff to work on).

> My Xen HWP patches need a
> Linux patch like this one to set bit 12 in the PDC.  I had an affected
> user test with this patch and it worked, serving as an equivalent of
> Linux commit a21211672c9a ("ACPI / processor: Request native thermal
> interrupt handling via _OSC").
> 
> Another idea is to use Linux's arch_acpi_set_pdc_bits() to make the
> hypercall to Xen.  It occurs earlier:
> acpi_processor_set_pdc()
>     acpi_processor_alloc_pdc()
>         acpi_set_pdc_bits()
>             arch_acpi_set_pdc_bits()
>     acpi_processor_eval_pdc()
> 
> So the IDLE_NOMWAIT masking in acpi_processor_eval_pdc() would still
> apply.  arch_acpi_set_pdc_bits() is provided the buffer, so it's a
> little cleaner in that respect.

I see.  My reasoning for placing the Xen filtering in
acpi_processor_eval_pdc() is so that there are no further
modifications to the buffer by Linux after the call to sanitize the
buffer (XENPF_set_processor_pminfo).

I think if the filtering done by Xen is moved to
arch_acpi_set_pdc_bits() we would then need to disable the evaluation
of boot_option_idle_override in acpi_processor_eval_pdc() as we don't
want dom0 choices affecting the selection of _PDC features done by
Xen?

In any case, feel free to pick this patch and re-submit upstream if
you want.

Thanks, Roger.