read_acpi_id() attempts to evaluate _CST using a stack buffer of
sizeof(union acpi_object) (48 bytes), but _CST returns a nested Package
of sub-Packages (one per C-state, each containing a register descriptor,
type, latency, and power) requiring hundreds of bytes. The evaluation
always fails with AE_BUFFER_OVERFLOW.

On modern systems using FFH/MWAIT entry (where pblk is zero), this
causes the function to return before setting the acpi_id_cst_present
bit. In check_acpi_ids(), flags.power is then zero for all Phase 2 CPUs
(physical CPUs beyond dom0's vCPU count), so push_cxx_to_hypervisor() is
never called for them.

On a system with dom0_max_vcpus=2 and 8 physical CPUs, only PCPUs 0-1
receive C-state data. PCPUs 2-7 are stuck in C0/C1 idle, unable to
enter C2/C3. This costs measurable wall power (4W observed on an Intel
Core Ultra 7 265K with Xen 4.20).

The function never uses the _CST return value -- it only needs to know
whether _CST exists. Replace the broken acpi_evaluate_object() call with
acpi_has_method(), which correctly detects _CST presence using
acpi_get_handle() without any buffer allocation. This brings C-state
detection to parity with the P-state path, which already works correctly
for Phase 2 CPUs.

Fixes: 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by: David Thomson <dt@linux-mail.net>
---
 drivers/xen/xen-acpi-processor.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 2967039..67a4afc 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -379,11 +379,8 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 			 acpi_psd[acpi_id].domain);
 	}
 
-	status = acpi_evaluate_object(handle, "_CST", NULL, &buffer);
-	if (ACPI_FAILURE(status)) {
-		if (!pblk)
-			return AE_OK;
-	}
+	if (!acpi_has_method(handle, "_CST") && !pblk)
+		return AE_OK;
 	/* .. and it has a C-state */
 	__set_bit(acpi_id, acpi_id_cst_present);
 
-- 
2.34.1

On 23.02.2026 20:56, David Thomson wrote:
> read_acpi_id() attempts to evaluate _CST using a stack buffer of
> sizeof(union acpi_object) (48 bytes), but _CST returns a nested Package
> of sub-Packages (one per C-state, each containing a register descriptor,
> type, latency, and power) requiring hundreds of bytes. The evaluation
> always fails with AE_BUFFER_OVERFLOW.
> 
> On modern systems using FFH/MWAIT entry (where pblk is zero), this
> causes the function to return before setting the acpi_id_cst_present
> bit. In check_acpi_ids(), flags.power is then zero for all Phase 2 CPUs
> (physical CPUs beyond dom0's vCPU count), so push_cxx_to_hypervisor() is
> never called for them.
> 
> On a system with dom0_max_vcpus=2 and 8 physical CPUs, only PCPUs 0-1
> receive C-state data. PCPUs 2-7 are stuck in C0/C1 idle, unable to
> enter C2/C3. This costs measurable wall power (4W observed on an Intel
> Core Ultra 7 265K with Xen 4.20).
> 
> The function never uses the _CST return value -- it only needs to know
> whether _CST exists. Replace the broken acpi_evaluate_object() call with
> acpi_has_method(), which correctly detects _CST presence using
> acpi_get_handle() without any buffer allocation. This brings C-state
> detection to parity with the P-state path, which already works correctly
> for Phase 2 CPUs.
> 
> Fixes: 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
> Signed-off-by: David Thomson <dt@linux-mail.net>
> ---
>  drivers/xen/xen-acpi-processor.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 2967039..67a4afc 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -379,11 +379,8 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
>  			 acpi_psd[acpi_id].domain);
>  	}
>  
> -	status = acpi_evaluate_object(handle, "_CST", NULL, &buffer);
> -	if (ACPI_FAILURE(status)) {
> -		if (!pblk)
> -			return AE_OK;
> -	}
> +	if (!acpi_has_method(handle, "_CST") && !pblk)
> +		return AE_OK;

I understand you reflect original behavior in this regard, but why involve any
ACPI function here at all when pblk is non-zero? I.e. why not swap the operands
of && ? Object evaluation could have wanted side effects (in which case,
however, some different change would be needed here), but checking for method
presence surely hasn't.

Jan

> I understand you reflect original behavior in this regard, but why involve any
> ACPI function here at all when pblk is non-zero? I.e. why not swap the operands
> of && ? Object evaluation could have wanted side effects (in which case,
> however, some different change would be needed here), but checking for method
> presence surely hasn't.
> 
> Jan
> 
You're right on both counts. The original evaluate_object() call at least had the appearance of wanting the result (even though buf was undersized and the result was never used). A pure presence check has no such pretense. Swapping the operands is the obvious improvement. I think dropping the _CST check entirely is also defensible since C-state support is confirmed when pblk is set. But I'm not certain there's no edge case where pblk is non-zero and _CST is also not present, so I'd defer to your judgement.

Would you prefer:
a) if (pblk && acpi_has_method(handle, "_CST"))
b) just if (pblk)

Happy to send a v2 either way.

DT

On 24.02.2026 10:10, David Thomson (dt@linux.com) wrote:
>> I understand you reflect original behavior in this regard, but why involve any
>> ACPI function here at all when pblk is non-zero? I.e. why not swap the operands
>> of && ? Object evaluation could have wanted side effects (in which case,
>> however, some different change would be needed here), but checking for method
>> presence surely hasn't.
>>
> You're right on both counts. The original evaluate_object() call at least had the appearance of wanting the result (even though buf was undersized and the result was never used). A pure presence check has no such pretense. Swapping the operands is the obvious improvement. I think dropping the _CST check entirely is also defensible since C-state support is confirmed when pblk is set. But I'm not certain there's no edge case where pblk is non-zero and _CST is also not present, so I'd defer to your judgement.
> 
> Would you prefer:
> a) if (pblk && acpi_has_method(handle, "_CST"))
> b) just if (pblk)

I don't think b) is correct, so a) please (albeit suitably adjusted to really only
flip the operands from what your original patch had).

Jan