[PATCH 0/3] xen: ACPI processor related fixes

[PATCH 0/3] xen: ACPI processor related fixes

[Roger Pau Monne]

Hello,

This series aims to fix some shortcomings with the handling of ACPI
Processors objects when running as a Xen dom0.

First two patches fix the execution of the _PDC methods for all CPUs on
the system and not just the ones available to dom0, while also making
sure that the _PDC capabilities reported to ACPI match what the
perfrmance and power drivers in Xen can handle.

Final patch fixes the Xen ACPI Processor driver to also work when used
in a PVH dom0, that has a custom build ACPI MADT table and mismatched
Processor UIDs between the MADT and the Processor objects in the dynamic
AML.

I don't really like the current implementation of the Xen ACPI Processor
driver, it IMO relies too much on data being fetched by generic kernel
code.  For one the generic fetcher functions can take CPUID data into
account in order to sanitize what's found in ACPI, but capabilities
reported to dom0 can be different from the native ones.  Also the Xen
ACPI Processor code relies on cloning the data from CPUs in order to fill
for the pCPUs > vCPUs, but this is wrong when running on heterogeneous
systems.

Last patch introduces some helpers to Xen ACPI Processor that should
allow fetching all the required data, for each ACPI Processor object on
the dynamic tables.  It might be helpful to explore disabling any
Processor object handling done by generic drivers and just fetch all the
data from the Xen Processor driver itself for every Processor object on
the namespace.  Likewise it might be better to just execute _PDC from
that same Xen ACPI Processor driver instead of polluting the generic
ACPI Processor driver.

The series should be taken as a RFC partially, due to my own doubts
about whether the current implementation is indeed the right one moving
forward.

Thanks, Roger.

Roger Pau Monne (3):
  acpi/processor: fix evaluating _PDC method when running as Xen dom0
  acpi/processor: sanitize _PDC buffer bits when running as Xen dom0
  xen/acpi: upload power and performance related data from a PVH dom0

 arch/x86/include/asm/xen/hypervisor.h |  12 ++
 arch/x86/xen/enlighten.c              |  44 +++++
 drivers/acpi/processor_pdc.c          |  19 +++
 drivers/xen/xen-acpi-processor.c      | 225 ++++++++++++++++++++++++--
 4 files changed, 284 insertions(+), 16 deletions(-)

-- 
2.37.3

[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.

[PATCH 3/3] xen/acpi: upload power and performance related data from a PVH dom0

[Roger Pau Monne]

When running as a PVH dom0 the ACPI MADT is crafted by Xen in order to
report the correct numbers of vCPUs that dom0 has, so the host MADT is
not provided to dom0.  This creates issues when parsing the power and
performance related data from ACPI dynamic tables, as the ACPI
Processor UIDs found on the dynamic code are likely to not match the
ones crafted by Xen in the dom0 MADT.

Xen would rely on Linux having filled at least the power and
performance related data of the vCPUs on the system, and would clone
that information in order to setup the remaining pCPUs on the system
if dom0 vCPUs < pCPUs.  However when running as PVH dom0 it's likely
that none of dom0 CPUs will have the power and performance data
filled, and hence the Xen ACPI Processor driver needs to fetch that
information by itself.

In order to do so correctly, introduce a new helper to fetch the _CST
data without taking into account the system capabilities from the
CPUID output, as the capabilities reported to dom0 in CPUID might be
different from the ones on the host.

Note that the newly introduced code will only fetch the _CST, _PSS,
_PPC and _PCT from a single CPU, and clone that information for all the
other Processors.  This won't work on an heterogeneous system with
Processors having different power and performance related data between
them.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 arch/x86/include/asm/xen/hypervisor.h |   2 +-
 arch/x86/xen/enlighten.c              |   2 +-
 drivers/xen/xen-acpi-processor.c      | 225 ++++++++++++++++++++++++--
 3 files changed, 211 insertions(+), 18 deletions(-)

diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index b4ed90ef5e68..1ead5253bc6c 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -62,7 +62,7 @@ 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);
+bool 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)
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)
 {
 	unsigned int i, maxid;
 	struct xen_platform_op op = {
diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 9cb61db67efd..b189ea69d557 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -48,6 +48,8 @@ static unsigned long *acpi_id_cst_present;
 /* Which ACPI P-State dependencies for a enumerated processor */
 static struct acpi_psd_package *acpi_psd;
 
+static bool pr_initialized;
+
 static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
 {
 	struct xen_platform_op op = {
@@ -172,8 +174,13 @@ static int xen_copy_psd_data(struct acpi_processor *_pr,
 
 	/* 'acpi_processor_preregister_performance' does not parse if the
 	 * num_processors <= 1, but Xen still requires it. Do it manually here.
+	 *
+	 * Also init the field if not set, as that's possible if the physical
+	 * CPUs on the system doesn't match the data provided in the MADT when
+	 * running as a PVH dom0.
 	 */
-	if (pdomain->num_processors <= 1) {
+	if (pdomain->num_processors <= 1 ||
+	    dst->shared_type == CPUFREQ_SHARED_TYPE_NONE) {
 		if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL)
 			dst->shared_type = CPUFREQ_SHARED_TYPE_ALL;
 		else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL)
@@ -313,6 +320,155 @@ static unsigned int __init get_max_acpi_id(void)
 	pr_debug("Max ACPI ID: %u\n", max_acpi_id);
 	return max_acpi_id;
 }
+
+/*
+ * Custom version of the native acpi_processor_evaluate_cst() function, to
+ * avoid some sanity checks done based on the CPUID data.  When running as a
+ * Xen domain the CPUID data provided to dom0 is not the native one, so C
+ * states cannot be sanity checked.  Leave it to the hypervisor which is also
+ * the entity running the driver.
+ */
+static int xen_acpi_processor_evaluate_cst(acpi_handle handle,
+					   struct acpi_processor_power *info)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *cst;
+	acpi_status status;
+	u64 count;
+	int last_index = 0;
+	int i, ret = 0;
+
+	status = acpi_evaluate_object(handle, "_CST", NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_debug(handle, "No _CST\n");
+		return -ENODEV;
+	}
+
+	cst = buffer.pointer;
+
+	/* There must be at least 2 elements. */
+	if (!cst || cst->type != ACPI_TYPE_PACKAGE || cst->package.count < 2) {
+		acpi_handle_warn(handle, "Invalid _CST output\n");
+		ret = -EFAULT;
+		goto end;
+	}
+
+	count = cst->package.elements[0].integer.value;
+
+	/* Validate the number of C-states. */
+	if (count < 1 || count != cst->package.count - 1) {
+		acpi_handle_warn(handle, "Inconsistent _CST data\n");
+		ret = -EFAULT;
+		goto end;
+	}
+
+	for (i = 1; i <= count; i++) {
+		union acpi_object *element;
+		union acpi_object *obj;
+		struct acpi_power_register *reg;
+		struct acpi_processor_cx cx;
+
+		/*
+		 * If there is not enough space for all C-states, skip the
+		 * excess ones and log a warning.
+		 */
+		if (last_index >= ACPI_PROCESSOR_MAX_POWER - 1) {
+			acpi_handle_warn(handle, "No room for more idle states (limit: %d)\n",
+					 ACPI_PROCESSOR_MAX_POWER - 1);
+			break;
+		}
+
+		memset(&cx, 0, sizeof(cx));
+
+		element = &cst->package.elements[i];
+		if (element->type != ACPI_TYPE_PACKAGE) {
+			acpi_handle_info(handle, "_CST C%d type(%x) is not package, skip...\n",
+					 i, element->type);
+			continue;
+		}
+
+		if (element->package.count != 4) {
+			acpi_handle_info(handle, "_CST C%d package count(%d) is not 4, skip...\n",
+				i, element->package.count);
+			continue;
+		}
+
+		obj = &element->package.elements[0];
+
+		if (obj->type != ACPI_TYPE_BUFFER) {
+			acpi_handle_info(handle, "_CST C%d package element[0] type(%x) is not buffer, skip...\n",
+					 i, obj->type);
+			continue;
+		}
+
+		reg = (struct acpi_power_register *)obj->buffer.pointer;
+
+		obj = &element->package.elements[1];
+		if (obj->type != ACPI_TYPE_INTEGER) {
+			acpi_handle_info(handle, "_CST C[%d] package element[1] type(%x) is not integer, skip...\n",
+					 i, obj->type);
+			continue;
+		}
+
+		cx.type = obj->integer.value;
+		/*
+		 * There are known cases in which the _CST output does not
+		 * contain C1, so if the type of the first state found is not
+		 * C1, leave an empty slot for C1 to be filled in later.
+		 */
+		if (i == 1 && cx.type != ACPI_STATE_C1)
+			last_index = 1;
+
+		cx.address = reg->address;
+		cx.index = last_index + 1;
+
+		switch (reg->space_id) {
+		case ACPI_ADR_SPACE_FIXED_HARDWARE:
+			cx.entry_method = ACPI_CSTATE_FFH;
+			break;
+
+		case ACPI_ADR_SPACE_SYSTEM_IO:
+			cx.entry_method = ACPI_CSTATE_SYSTEMIO;
+			break;
+
+		default:
+			acpi_handle_info(handle, "_CST C%d space_id(%x) neither FIXED_HARDWARE nor SYSTEM_IO, skip...\n",
+					 i, reg->space_id);
+			continue;
+		}
+
+		if (cx.type == ACPI_STATE_C1)
+			cx.valid = 1;
+
+		obj = &element->package.elements[2];
+		if (obj->type != ACPI_TYPE_INTEGER) {
+			acpi_handle_info(handle, "_CST C%d package element[2] type(%x) not integer, skip...\n",
+					 i, obj->type);
+			continue;
+		}
+
+		cx.latency = obj->integer.value;
+
+		obj = &element->package.elements[3];
+		if (obj->type != ACPI_TYPE_INTEGER) {
+			acpi_handle_info(handle, "_CST C%d package element[3] type(%x) not integer, skip...\n",
+					 i, obj->type);
+			continue;
+		}
+
+		memcpy(&info->states[++last_index], &cx, sizeof(cx));
+	}
+
+	acpi_handle_info(handle, "Found %d idle states\n", last_index);
+
+	info->count = last_index;
+
+end:
+	kfree(buffer.pointer);
+
+	return ret;
+}
+
 /*
  * The read_acpi_id and check_acpi_ids are there to support the Xen
  * oddity of virtual CPUs != physical CPUs in the initial domain.
@@ -354,24 +510,44 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 	default:
 		return AE_OK;
 	}
-	if (invalid_phys_cpuid(acpi_get_phys_id(handle,
-						acpi_type == ACPI_TYPE_DEVICE,
-						acpi_id))) {
+
+	if (!xen_processor_present(acpi_id)) {
 		pr_debug("CPU with ACPI ID %u is unavailable\n", acpi_id);
 		return AE_OK;
 	}
-	/* There are more ACPI Processor objects than in x2APIC or MADT.
-	 * This can happen with incorrect ACPI SSDT declerations. */
-	if (acpi_id >= nr_acpi_bits) {
-		pr_debug("max acpi id %u, trying to set %u\n",
-			 nr_acpi_bits - 1, acpi_id);
-		return AE_OK;
-	}
+
 	/* OK, There is a ACPI Processor object */
 	__set_bit(acpi_id, acpi_id_present);
 
 	pr_debug("ACPI CPU%u w/ PBLK:0x%lx\n", acpi_id, (unsigned long)pblk);
 
+	if (!pr_initialized) {
+		struct acpi_processor *pr = context;
+		int rc;
+
+		/*
+		 * There's no CPU on the system that has any performance or
+		 * power related data, initialize all the required fields by
+		 * fetching that info here.
+		 *
+		 * Note such information is only fetched once, and then reused
+		 * for all pCPUs.  This won't work on heterogeneous systems
+		 * with different Cx anb/or Px states between CPUs.
+		 */
+
+		pr->handle = handle;
+
+		rc = acpi_processor_get_performance_info(pr);
+		if (rc)
+			pr_debug("ACPI CPU%u failed to get performance data\n",
+				 acpi_id);
+		rc = xen_acpi_processor_evaluate_cst(handle, &pr->power);
+		if (rc)
+			pr_debug("ACPI CPU%u failed to get _CST data\n", acpi_id);
+
+		pr_initialized = true;
+	}
+
 	/* It has P-state dependencies */
 	if (!acpi_processor_get_psd(handle, &acpi_psd[acpi_id])) {
 		pr_debug("ACPI CPU%u w/ PST:coord_type = %llu domain = %llu\n",
@@ -392,8 +568,7 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 static int check_acpi_ids(struct acpi_processor *pr_backup)
 {
 
-	if (!pr_backup)
-		return -ENODEV;
+	BUG_ON(!pr_backup);
 
 	if (acpi_id_present && acpi_id_cst_present)
 		/* OK, done this once .. skip to uploading */
@@ -422,8 +597,8 @@ static int check_acpi_ids(struct acpi_processor *pr_backup)
 
 	acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
 			    ACPI_UINT32_MAX,
-			    read_acpi_id, NULL, NULL, NULL);
-	acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, read_acpi_id, NULL, NULL);
+			    read_acpi_id, NULL, pr_backup, NULL);
+	acpi_get_devices(ACPI_PROCESSOR_DEVICE_HID, read_acpi_id, pr_backup, NULL);
 
 upload:
 	if (!bitmap_equal(acpi_id_present, acpi_ids_done, nr_acpi_bits)) {
@@ -464,6 +639,7 @@ static int xen_upload_processor_pm_data(void)
 	struct acpi_processor *pr_backup = NULL;
 	int i;
 	int rc = 0;
+	bool free_perf = false;
 
 	pr_info("Uploading Xen processor PM info\n");
 
@@ -475,13 +651,30 @@ static int xen_upload_processor_pm_data(void)
 
 		if (!pr_backup) {
 			pr_backup = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
-			if (pr_backup)
+			if (pr_backup) {
 				memcpy(pr_backup, _pr, sizeof(struct acpi_processor));
+				pr_initialized = true;
+			}
 		}
 		(void)upload_pm_data(_pr);
 	}
 
+	if (!pr_backup) {
+		pr_backup = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
+		if (!pr_backup)
+			return -ENOMEM;
+		pr_backup->performance = kzalloc(sizeof(struct acpi_processor_performance),
+						 GFP_KERNEL);
+		if (!pr_backup->performance) {
+			kfree(pr_backup);
+			return -ENOMEM;
+		}
+		free_perf = true;
+	}
+
 	rc = check_acpi_ids(pr_backup);
+	if (free_perf)
+		kfree(pr_backup->performance);
 	kfree(pr_backup);
 
 	return rc;
-- 
2.37.3

Re: [PATCH 3/3] xen/acpi: upload power and performance related data from a PVH dom0

[Josef Johansson]

On 11/21/22 11:21, Roger Pau Monne wrote:
> When running as a PVH dom0 the ACPI MADT is crafted by Xen in order to
> report the correct numbers of vCPUs that dom0 has, so the host MADT is
> not provided to dom0.  This creates issues when parsing the power and
> performance related data from ACPI dynamic tables, as the ACPI
> Processor UIDs found on the dynamic code are likely to not match the
> ones crafted by Xen in the dom0 MADT.
>
> Xen would rely on Linux having filled at least the power and
> performance related data of the vCPUs on the system, and would clone
> that information in order to setup the remaining pCPUs on the system
> if dom0 vCPUs < pCPUs.  However when running as PVH dom0 it's likely
> that none of dom0 CPUs will have the power and performance data
> filled, and hence the Xen ACPI Processor driver needs to fetch that
> information by itself.
>
> In order to do so correctly, introduce a new helper to fetch the _CST
> data without taking into account the system capabilities from the
> CPUID output, as the capabilities reported to dom0 in CPUID might be
> different from the ones on the host.
>
>

Hi Roger,

First of all, thanks for doing the grunt work here to clear up the ACPI 
situation.

A bit of background, I'm trying to get an AMD APU (CPUID 0x17 (23)) to 
work properly
under Xen. It works fine otherwise but something is amiss under Xen.

Just to make sure that the patch is working as intended, what I found 
when trying it out
is that the first 8 CPUs have C-States, but 0, 2, 4, 6, 8, 10, 12, 14 
have P-States, out of
16 CPUs. Xen tells Linux that there are 8 CPUs since it's running with 
nosmt.

Regards
- Josef

xen_acpi_processor: Max ACPI ID: 30
xen_acpi_processor: Uploading Xen processor PM info
xen_acpi_processor: ACPI CPU0 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU0 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU1 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU2 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU2 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU3 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU4 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU4 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU5 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU6 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU6 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU7 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU0 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU0 w/ PST:coord_type = 254 domain = 0
xen_acpi_processor: CPU with ACPI ID 1 is unavailable
xen_acpi_processor: ACPI CPU2 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU2 w/ PST:coord_type = 254 domain = 1
xen_acpi_processor: CPU with ACPI ID 3 is unavailable
xen_acpi_processor: ACPI CPU4 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU4 w/ PST:coord_type = 254 domain = 2
xen_acpi_processor: CPU with ACPI ID 5 is unavailable
xen_acpi_processor: ACPI CPU6 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU6 w/ PST:coord_type = 254 domain = 3
xen_acpi_processor: CPU with ACPI ID 7 is unavailable
xen_acpi_processor: ACPI CPU8 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU8 w/ PST:coord_type = 254 domain = 4
xen_acpi_processor: CPU with ACPI ID 9 is unavailable
xen_acpi_processor: ACPI CPU10 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU10 w/ PST:coord_type = 254 domain = 5
xen_acpi_processor: CPU with ACPI ID 11 is unavailable
xen_acpi_processor: ACPI CPU12 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU12 w/ PST:coord_type = 254 domain = 6
xen_acpi_processor: CPU with ACPI ID 13 is unavailable
xen_acpi_processor: ACPI CPU14 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU14 w/ PST:coord_type = 254 domain = 7
xen_acpi_processor: CPU with ACPI ID 15 is unavailable
xen_acpi_processor: ACPI CPU8 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU10 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU12 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU14 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS

As a bonus, here are the previous debug output on the same machine.

xen_acpi_processor: Max ACPI ID: 30
xen_acpi_processor: Uploading Xen processor PM info
xen_acpi_processor: ACPI CPU0 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU0 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU1 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU2 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU2 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU3 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU4 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU4 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU5 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU6 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU6 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU7 - C-states uploaded.
xen_acpi_processor:      C1: ACPI HLT 1 uS
xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
xen_acpi_processor: ACPI CPU0 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU0 w/ PST:coord_type = 254 domain = 0
xen_acpi_processor: ACPI CPU1 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU1 w/ PST:coord_type = 254 domain = 0
xen_acpi_processor: ACPI CPU2 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU2 w/ PST:coord_type = 254 domain = 1
xen_acpi_processor: ACPI CPU3 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU3 w/ PST:coord_type = 254 domain = 1
xen_acpi_processor: ACPI CPU4 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU4 w/ PST:coord_type = 254 domain = 2
xen_acpi_processor: ACPI CPU5 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU5 w/ PST:coord_type = 254 domain = 2
xen_acpi_processor: ACPI CPU6 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU6 w/ PST:coord_type = 254 domain = 3
xen_acpi_processor: ACPI CPU7 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU7 w/ PST:coord_type = 254 domain = 3
xen_acpi_processor: ACPI CPU8 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU8 w/ PST:coord_type = 254 domain = 4
xen_acpi_processor: ACPI CPU9 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU9 w/ PST:coord_type = 254 domain = 4
xen_acpi_processor: ACPI CPU10 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU10 w/ PST:coord_type = 254 domain = 5
xen_acpi_processor: ACPI CPU11 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU11 w/ PST:coord_type = 254 domain = 5
xen_acpi_processor: ACPI CPU12 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU12 w/ PST:coord_type = 254 domain = 6
xen_acpi_processor: ACPI CPU13 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU13 w/ PST:coord_type = 254 domain = 6
xen_acpi_processor: ACPI CPU14 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU14 w/ PST:coord_type = 254 domain = 7
xen_acpi_processor: ACPI CPU15 w/ PBLK:0x0
xen_acpi_processor: ACPI CPU15 w/ PST:coord_type = 254 domain = 7
xen_acpi_processor: ACPI CPU8 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU10 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU12 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
xen_acpi_processor: ACPI CPU14 - P-states uploaded.
xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS

Re: [PATCH 3/3] xen/acpi: upload power and performance related data from a PVH dom0

[Roger Pau Monné]

On Mon, Jan 30, 2023 at 10:10:05AM +0100, Josef Johansson wrote:
> 
> On 11/21/22 11:21, Roger Pau Monne wrote:
> > When running as a PVH dom0 the ACPI MADT is crafted by Xen in order to
> > report the correct numbers of vCPUs that dom0 has, so the host MADT is
> > not provided to dom0.  This creates issues when parsing the power and
> > performance related data from ACPI dynamic tables, as the ACPI
> > Processor UIDs found on the dynamic code are likely to not match the
> > ones crafted by Xen in the dom0 MADT.
> > 
> > Xen would rely on Linux having filled at least the power and
> > performance related data of the vCPUs on the system, and would clone
> > that information in order to setup the remaining pCPUs on the system
> > if dom0 vCPUs < pCPUs.  However when running as PVH dom0 it's likely
> > that none of dom0 CPUs will have the power and performance data
> > filled, and hence the Xen ACPI Processor driver needs to fetch that
> > information by itself.
> > 
> > In order to do so correctly, introduce a new helper to fetch the _CST
> > data without taking into account the system capabilities from the
> > CPUID output, as the capabilities reported to dom0 in CPUID might be
> > different from the ones on the host.
> > 
> > 
> 
> Hi Roger,
> 
> First of all, thanks for doing the grunt work here to clear up the ACPI
> situation.
> 
> A bit of background, I'm trying to get an AMD APU (CPUID 0x17 (23)) to work
> properly
> under Xen. It works fine otherwise but something is amiss under Xen.

Hello,

Sorry for the delay, I've been on paternity leave and just caching up
on emails.

> Just to make sure that the patch is working as intended, what I found when
> trying it out
> is that the first 8 CPUs have C-States, but 0, 2, 4, 6, 8, 10, 12, 14 have
> P-States, out of
> 16 CPUs. Xen tells Linux that there are 8 CPUs since it's running with
> nosmt.
> 
> Regards
> - Josef
> 
> xen_acpi_processor: Max ACPI ID: 30
> xen_acpi_processor: Uploading Xen processor PM info
> xen_acpi_processor: ACPI CPU0 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU0 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU1 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU2 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU2 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU3 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU4 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU4 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU5 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU6 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU6 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU7 - C-states uploaded.
> xen_acpi_processor:      C1: ACPI HLT 1 uS
> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> xen_acpi_processor: ACPI CPU0 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU0 w/ PST:coord_type = 254 domain = 0
> xen_acpi_processor: CPU with ACPI ID 1 is unavailable

Hm, that's weird, do you think you could check why it reports the CPU
is unavailable?

Overall I don't like the situation of the ACPI handling when running
as dom0. It's fragile to rely on the data for dom0 CPUs to be
filled by the system (by adding some band aids here and there so that
the PV vCPUs are matched against the MADT objects) and then cloning
the data for any physical CPU exceeding the number of dom0 virtual
CPUs.

IMO it would be much better to just do the handling of ACPI processor
objects in a Xen specific driver (preventing the native driver from
attaching) in order to fetch the data and upload it to Xen.  This is
what I've attempted to do on FreeBSD, and resulted in a cleaner
implementation:

https://cgit.freebsd.org/src/tree/sys/dev/xen/cpu/xen_acpi_cpu.c

I however don't have time to do this right now for Linux.

> xen_acpi_processor: ACPI CPU2 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU2 w/ PST:coord_type = 254 domain = 1
> xen_acpi_processor: CPU with ACPI ID 3 is unavailable
> xen_acpi_processor: ACPI CPU4 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU4 w/ PST:coord_type = 254 domain = 2
> xen_acpi_processor: CPU with ACPI ID 5 is unavailable
> xen_acpi_processor: ACPI CPU6 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU6 w/ PST:coord_type = 254 domain = 3
> xen_acpi_processor: CPU with ACPI ID 7 is unavailable
> xen_acpi_processor: ACPI CPU8 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU8 w/ PST:coord_type = 254 domain = 4
> xen_acpi_processor: CPU with ACPI ID 9 is unavailable
> xen_acpi_processor: ACPI CPU10 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU10 w/ PST:coord_type = 254 domain = 5
> xen_acpi_processor: CPU with ACPI ID 11 is unavailable
> xen_acpi_processor: ACPI CPU12 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU12 w/ PST:coord_type = 254 domain = 6
> xen_acpi_processor: CPU with ACPI ID 13 is unavailable
> xen_acpi_processor: ACPI CPU14 w/ PBLK:0x0
> xen_acpi_processor: ACPI CPU14 w/ PST:coord_type = 254 domain = 7
> xen_acpi_processor: CPU with ACPI ID 15 is unavailable
> xen_acpi_processor: ACPI CPU8 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU10 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU12 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> xen_acpi_processor: ACPI CPU14 - P-states uploaded.
> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> 
> As a bonus, here are the previous debug output on the same machine.

I think the output below is with dom0 running as plain PV rather than
PVH?

Thanks, Roger.

Re: [PATCH 3/3] xen/acpi: upload power and performance related data from a PVH dom0

[Josef Johansson]

On 3/15/23 12:39, Roger Pau Monné wrote:
> On Mon, Jan 30, 2023 at 10:10:05AM +0100, Josef Johansson wrote:
>> On 11/21/22 11:21, Roger Pau Monne wrote:
>>> When running as a PVH dom0 the ACPI MADT is crafted by Xen in order to
>>> report the correct numbers of vCPUs that dom0 has, so the host MADT is
>>> not provided to dom0.  This creates issues when parsing the power and
>>> performance related data from ACPI dynamic tables, as the ACPI
>>> Processor UIDs found on the dynamic code are likely to not match the
>>> ones crafted by Xen in the dom0 MADT.
>>>
>>> Xen would rely on Linux having filled at least the power and
>>> performance related data of the vCPUs on the system, and would clone
>>> that information in order to setup the remaining pCPUs on the system
>>> if dom0 vCPUs < pCPUs.  However when running as PVH dom0 it's likely
>>> that none of dom0 CPUs will have the power and performance data
>>> filled, and hence the Xen ACPI Processor driver needs to fetch that
>>> information by itself.
>>>
>>> In order to do so correctly, introduce a new helper to fetch the _CST
>>> data without taking into account the system capabilities from the
>>> CPUID output, as the capabilities reported to dom0 in CPUID might be
>>> different from the ones on the host.
>>>
>>>
>> Hi Roger,
>>
>> First of all, thanks for doing the grunt work here to clear up the ACPI
>> situation.
>>
>> A bit of background, I'm trying to get an AMD APU (CPUID 0x17 (23)) to work
>> properly
>> under Xen. It works fine otherwise but something is amiss under Xen.
> Hello,
>
> Sorry for the delay, I've been on paternity leave and just caching up
> on emails.
Hi Roger,

Congratulations! I hope you had time to really connect. It's the most 
important thing we can do here in life.

I came into this to understand each and every error in my boot-log, it 
turns out that the latest
kernel+xen+firmware fixes suspend/resume for me, thus is this not 
related. But as I pointed out,
the output does not make any sense (nor yours nor the upstream). I 
should check the debug
output with suspend working fine now to see if there are any changes, 
that would be quite
interesting.

Also, I should mention that your patch broke some things on my system 
and made it
unstable. I don't remember exactly and I know you said that this is more 
of a PoC. Just a
heads up.
>> Just to make sure that the patch is working as intended, what I found when
>> trying it out
>> is that the first 8 CPUs have C-States, but 0, 2, 4, 6, 8, 10, 12, 14 have
>> P-States, out of
>> 16 CPUs. Xen tells Linux that there are 8 CPUs since it's running with
>> nosmt.
>>
>> Regards
>> - Josef
>>
>> xen_acpi_processor: Max ACPI ID: 30
>> xen_acpi_processor: Uploading Xen processor PM info
>> xen_acpi_processor: ACPI CPU0 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU0 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU1 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU2 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU2 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU3 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU4 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU4 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU5 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU6 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU6 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU7 - C-states uploaded.
>> xen_acpi_processor:      C1: ACPI HLT 1 uS
>> xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
>> xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
>> xen_acpi_processor: ACPI CPU0 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU0 w/ PST:coord_type = 254 domain = 0
>> xen_acpi_processor: CPU with ACPI ID 1 is unavailable
> Hm, that's weird, do you think you could check why it reports the CPU
> is unavailable?
If you would give me a hint at how I could check?
Right now my guess is that C state and P state is not in sync, thus P 
state are for every other
CPU and C state is for the first 8. AFAIK AMD only have 
performance-cores (unlike Intel).
>
> Overall I don't like the situation of the ACPI handling when running
> as dom0. It's fragile to rely on the data for dom0 CPUs to be
> filled by the system (by adding some band aids here and there so that
> the PV vCPUs are matched against the MADT objects) and then cloning
> the data for any physical CPU exceeding the number of dom0 virtual
> CPUs.
That's my understanding from earlier implementation as well, nobody 
actually like it,
But the current solution is something working in a bad environment.
>
> IMO it would be much better to just do the handling of ACPI processor
> objects in a Xen specific driver (preventing the native driver from
> attaching) in order to fetch the data and upload it to Xen.  This is
> what I've attempted to do on FreeBSD, and resulted in a cleaner
> implementation:
>
> <link>
>
> I however don't have time to do this right now for Linux.

Maybe I can take a stab, I very much like the climate of the kernel but 
everything
seem so scary :) I've been trying to understand things better, how 
they're all
connected.
>
>> xen_acpi_processor: ACPI CPU2 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU2 w/ PST:coord_type = 254 domain = 1
>> xen_acpi_processor: CPU with ACPI ID 3 is unavailable
>> xen_acpi_processor: ACPI CPU4 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU4 w/ PST:coord_type = 254 domain = 2
>> xen_acpi_processor: CPU with ACPI ID 5 is unavailable
>> xen_acpi_processor: ACPI CPU6 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU6 w/ PST:coord_type = 254 domain = 3
>> xen_acpi_processor: CPU with ACPI ID 7 is unavailable
>> xen_acpi_processor: ACPI CPU8 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU8 w/ PST:coord_type = 254 domain = 4
>> xen_acpi_processor: CPU with ACPI ID 9 is unavailable
>> xen_acpi_processor: ACPI CPU10 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU10 w/ PST:coord_type = 254 domain = 5
>> xen_acpi_processor: CPU with ACPI ID 11 is unavailable
>> xen_acpi_processor: ACPI CPU12 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU12 w/ PST:coord_type = 254 domain = 6
>> xen_acpi_processor: CPU with ACPI ID 13 is unavailable
>> xen_acpi_processor: ACPI CPU14 w/ PBLK:0x0
>> xen_acpi_processor: ACPI CPU14 w/ PST:coord_type = 254 domain = 7
>> xen_acpi_processor: CPU with ACPI ID 15 is unavailable
>> xen_acpi_processor: ACPI CPU8 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU10 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU12 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>> xen_acpi_processor: ACPI CPU14 - P-states uploaded.
>> xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
>> xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
>> xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
>>
>> As a bonus, here are the previous debug output on the same machine.
> I think the output below is with dom0 running as plain PV rather than
> PVH?
This is the upstream ACPI implementation vs yours. What would plain PV 
vs PVH be in dom0?

Regards
- Josef
> Thanks, Roger.

Re: [PATCH 3/3] xen/acpi: upload power and performance related data from a PVH dom0

[Roger Pau Monné]

On Thu, Mar 16, 2023 at 08:54:57AM +0100, Josef Johansson wrote:
> On 3/15/23 12:39, Roger Pau Monné wrote:
> > On Mon, Jan 30, 2023 at 10:10:05AM +0100, Josef Johansson wrote:
> > > On 11/21/22 11:21, Roger Pau Monne wrote:
> > > > When running as a PVH dom0 the ACPI MADT is crafted by Xen in order to
> > > > report the correct numbers of vCPUs that dom0 has, so the host MADT is
> > > > not provided to dom0.  This creates issues when parsing the power and
> > > > performance related data from ACPI dynamic tables, as the ACPI
> > > > Processor UIDs found on the dynamic code are likely to not match the
> > > > ones crafted by Xen in the dom0 MADT.
> > > > 
> > > > Xen would rely on Linux having filled at least the power and
> > > > performance related data of the vCPUs on the system, and would clone
> > > > that information in order to setup the remaining pCPUs on the system
> > > > if dom0 vCPUs < pCPUs.  However when running as PVH dom0 it's likely
> > > > that none of dom0 CPUs will have the power and performance data
> > > > filled, and hence the Xen ACPI Processor driver needs to fetch that
> > > > information by itself.
> > > > 
> > > > In order to do so correctly, introduce a new helper to fetch the _CST
> > > > data without taking into account the system capabilities from the
> > > > CPUID output, as the capabilities reported to dom0 in CPUID might be
> > > > different from the ones on the host.
> > > > 
> > > > 
> > > Hi Roger,
> > > 
> > > First of all, thanks for doing the grunt work here to clear up the ACPI
> > > situation.
> > > 
> > > A bit of background, I'm trying to get an AMD APU (CPUID 0x17 (23)) to work
> > > properly
> > > under Xen. It works fine otherwise but something is amiss under Xen.
> > Hello,
> > 
> > Sorry for the delay, I've been on paternity leave and just caching up
> > on emails.
> Hi Roger,
> 
> Congratulations! I hope you had time to really connect. It's the most
> important thing we can do here in life.
> 
> I came into this to understand each and every error in my boot-log, it turns
> out that the latest
> kernel+xen+firmware fixes suspend/resume for me, thus is this not related.
> But as I pointed out,
> the output does not make any sense (nor yours nor the upstream). I should
> check the debug
> output with suspend working fine now to see if there are any changes, that
> would be quite
> interesting.
> 
> Also, I should mention that your patch broke some things on my system and
> made it
> unstable. I don't remember exactly and I know you said that this is more of
> a PoC. Just a
> heads up.

Right, I don't plan to send the PVH part just now, and instead I'm
focusing in the first patch that should fix _PDC evaluation for PV
dom0.  I will Cc you on the last version so you can give it a try and
assert is not regressing stuff for you.

> > > Just to make sure that the patch is working as intended, what I found when
> > > trying it out
> > > is that the first 8 CPUs have C-States, but 0, 2, 4, 6, 8, 10, 12, 14 have
> > > P-States, out of
> > > 16 CPUs. Xen tells Linux that there are 8 CPUs since it's running with
> > > nosmt.
> > > 
> > > Regards
> > > - Josef
> > > 
> > > xen_acpi_processor: Max ACPI ID: 30
> > > xen_acpi_processor: Uploading Xen processor PM info
> > > xen_acpi_processor: ACPI CPU0 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU0 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU1 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU2 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU2 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU3 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU4 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU4 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU5 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU6 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU6 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU7 - C-states uploaded.
> > > xen_acpi_processor:      C1: ACPI HLT 1 uS
> > > xen_acpi_processor:      C2: ACPI IOPORT 0x414 18 uS
> > > xen_acpi_processor:      C3: ACPI IOPORT 0x415 350 uS
> > > xen_acpi_processor: ACPI CPU0 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU0 w/ PST:coord_type = 254 domain = 0
> > > xen_acpi_processor: CPU with ACPI ID 1 is unavailable
> > Hm, that's weird, do you think you could check why it reports the CPU
> > is unavailable?
> If you would give me a hint at how I could check?

It likely requires you to add printk statements to the kernel in order
to figure out which conditional fails when running as a PVH dom0.

> Right now my guess is that C state and P state is not in sync, thus P state
> are for every other
> CPU and C state is for the first 8. AFAIK AMD only have performance-cores
> (unlike Intel).

Linux thinking the CPU is not online is more likely to be due to the
ACPI ID differences when running as a PVH dom0.  Anyway, I will try to
revisit this and figure out what's wrong.

> > 
> > Overall I don't like the situation of the ACPI handling when running
> > as dom0. It's fragile to rely on the data for dom0 CPUs to be
> > filled by the system (by adding some band aids here and there so that
> > the PV vCPUs are matched against the MADT objects) and then cloning
> > the data for any physical CPU exceeding the number of dom0 virtual
> > CPUs.
> That's my understanding from earlier implementation as well, nobody actually
> like it,
> But the current solution is something working in a bad environment.
> > 
> > IMO it would be much better to just do the handling of ACPI processor
> > objects in a Xen specific driver (preventing the native driver from
> > attaching) in order to fetch the data and upload it to Xen.  This is
> > what I've attempted to do on FreeBSD, and resulted in a cleaner
> > implementation:
> > 
> > <link>
> > 
> > I however don't have time to do this right now for Linux.
> 
> Maybe I can take a stab, I very much like the climate of the kernel but
> everything
> seem so scary :) I've been trying to understand things better, how they're
> all
> connected.
> > 
> > > xen_acpi_processor: ACPI CPU2 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU2 w/ PST:coord_type = 254 domain = 1
> > > xen_acpi_processor: CPU with ACPI ID 3 is unavailable
> > > xen_acpi_processor: ACPI CPU4 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU4 w/ PST:coord_type = 254 domain = 2
> > > xen_acpi_processor: CPU with ACPI ID 5 is unavailable
> > > xen_acpi_processor: ACPI CPU6 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU6 w/ PST:coord_type = 254 domain = 3
> > > xen_acpi_processor: CPU with ACPI ID 7 is unavailable
> > > xen_acpi_processor: ACPI CPU8 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU8 w/ PST:coord_type = 254 domain = 4
> > > xen_acpi_processor: CPU with ACPI ID 9 is unavailable
> > > xen_acpi_processor: ACPI CPU10 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU10 w/ PST:coord_type = 254 domain = 5
> > > xen_acpi_processor: CPU with ACPI ID 11 is unavailable
> > > xen_acpi_processor: ACPI CPU12 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU12 w/ PST:coord_type = 254 domain = 6
> > > xen_acpi_processor: CPU with ACPI ID 13 is unavailable
> > > xen_acpi_processor: ACPI CPU14 w/ PBLK:0x0
> > > xen_acpi_processor: ACPI CPU14 w/ PST:coord_type = 254 domain = 7
> > > xen_acpi_processor: CPU with ACPI ID 15 is unavailable
> > > xen_acpi_processor: ACPI CPU8 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU10 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU12 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > xen_acpi_processor: ACPI CPU14 - P-states uploaded.
> > > xen_acpi_processor:      *P0: 1700 MHz, 2071 mW, 0 uS
> > > xen_acpi_processor:       P1: 1600 MHz, 1520 mW, 0 uS
> > > xen_acpi_processor:       P2: 1400 MHz, 1277 mW, 0 uS
> > > 
> > > As a bonus, here are the previous debug output on the same machine.
> > I think the output below is with dom0 running as plain PV rather than
> > PVH?
> This is the upstream ACPI implementation vs yours. What would plain PV vs
> PVH be in dom0?

But that's always with Linux running as a dom0, or just running bare
metal?

Thanks, Roger.

Re: [PATCH 0/3] xen: ACPI processor related fixes

[Jan Beulich]

On 21.11.2022 11:21, Roger Pau Monne wrote:
> Hello,
> 
> This series aims to fix some shortcomings with the handling of ACPI
> Processors objects when running as a Xen dom0.
> 
> First two patches fix the execution of the _PDC methods for all CPUs on
> the system and not just the ones available to dom0, while also making
> sure that the _PDC capabilities reported to ACPI match what the
> perfrmance and power drivers in Xen can handle.
> 
> Final patch fixes the Xen ACPI Processor driver to also work when used
> in a PVH dom0, that has a custom build ACPI MADT table and mismatched
> Processor UIDs between the MADT and the Processor objects in the dynamic
> AML.
> 
> I don't really like the current implementation of the Xen ACPI Processor
> driver, it IMO relies too much on data being fetched by generic kernel
> code.  For one the generic fetcher functions can take CPUID data into
> account in order to sanitize what's found in ACPI, but capabilities
> reported to dom0 can be different from the native ones.  Also the Xen
> ACPI Processor code relies on cloning the data from CPUs in order to fill
> for the pCPUs > vCPUs, but this is wrong when running on heterogeneous
> systems.

Yes, these are problems (and as per reading the description of the
last patch you even extend this "cloning" of data), but ...

> Last patch introduces some helpers to Xen ACPI Processor that should
> allow fetching all the required data, for each ACPI Processor object on
> the dynamic tables.  It might be helpful to explore disabling any
> Processor object handling done by generic drivers and just fetch all the
> data from the Xen Processor driver itself for every Processor object on
> the namespace.  Likewise it might be better to just execute _PDC from
> that same Xen ACPI Processor driver instead of polluting the generic
> ACPI Processor driver.

... cloning functions living elsewhere also has the genuine problem of
them then needing to be kept in sync without there being any trigger to
know when an original function was changed in some way.

Jan

Re: [PATCH 0/3] xen: ACPI processor related fixes

[Roger Pau Monné]

On Mon, Nov 21, 2022 at 03:20:53PM +0100, Jan Beulich wrote:
> On 21.11.2022 11:21, Roger Pau Monne wrote:
> > Hello,
> > 
> > This series aims to fix some shortcomings with the handling of ACPI
> > Processors objects when running as a Xen dom0.
> > 
> > First two patches fix the execution of the _PDC methods for all CPUs on
> > the system and not just the ones available to dom0, while also making
> > sure that the _PDC capabilities reported to ACPI match what the
> > perfrmance and power drivers in Xen can handle.
> > 
> > Final patch fixes the Xen ACPI Processor driver to also work when used
> > in a PVH dom0, that has a custom build ACPI MADT table and mismatched
> > Processor UIDs between the MADT and the Processor objects in the dynamic
> > AML.
> > 
> > I don't really like the current implementation of the Xen ACPI Processor
> > driver, it IMO relies too much on data being fetched by generic kernel
> > code.  For one the generic fetcher functions can take CPUID data into
> > account in order to sanitize what's found in ACPI, but capabilities
> > reported to dom0 can be different from the native ones.  Also the Xen
> > ACPI Processor code relies on cloning the data from CPUs in order to fill
> > for the pCPUs > vCPUs, but this is wrong when running on heterogeneous
> > systems.
> 
> Yes, these are problems (and as per reading the description of the
> last patch you even extend this "cloning" of data), but ...
> 
> > Last patch introduces some helpers to Xen ACPI Processor that should
> > allow fetching all the required data, for each ACPI Processor object on
> > the dynamic tables.  It might be helpful to explore disabling any
> > Processor object handling done by generic drivers and just fetch all the
> > data from the Xen Processor driver itself for every Processor object on
> > the namespace.  Likewise it might be better to just execute _PDC from
> > that same Xen ACPI Processor driver instead of polluting the generic
> > ACPI Processor driver.
> 
> ... cloning functions living elsewhere also has the genuine problem of
> them then needing to be kept in sync without there being any trigger to
> know when an original function was changed in some way.

Well, yes, but using generic functions also has the risk of them being
modified to take into account CPUID data for example and then the
result would no longer be suitable for Xen's usage without us
noticing.

Also has the downside of parsing the data into Linux structures which
then need to be translated into Xen format.  It might be more straight
forward to just evaluate the required ACPI methods and parse the ACPI
data from the Xen ACPI Processor driver into the format used by Xen
and upload that to the hypervisor.

I realize however this is a big change, and would mean almost a
rewrite from scratch of the Xen ACPI Processor driver.  I wouldn't
want to start that task without having agreement that this is the
correct way forward.

Roger.