Questions about Cx and Px state uploading from dom0

Questions about Cx and Px state uploading from dom0

[Roger Pau Monné]

Hello,

I was looking at implementing ACPI Cx and Px state uploading from
FreeBSD dom0, as my main test box is considerably slower without Xen
knowing about the Px states.  That has raised a couple of questions.

1. How to figure out what features to report available by OSPM when
calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
this from Linux: it seems to be used to detect mwait support in
xen_check_mwait but not when calling _PDC (ie: in
acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
the caller to provide.  Should buf[2] be set to all the possible
features supported by the OS and Xen will trim those as required?

2. When uploading Px states, what's the meaning of the shared_type
field in xen_processor_performance?  I've looked at the usage of the
field by Xen, and first of all it seems to be a layering violation
because the values set in the field (CPUFREQ_SHARED_TYPE_*) are not
exposed as part of the public interface.  This all works for Linux
because the same values are used by Xen and the Linux kernel.
Secondly, this is not part of the data fetched from ACPI AFAICT, so
I'm unsure how the value should be calculated.  I also wonder whether
this couldn't be done by Xen itself from the uploaded Px data (but
without knowing exactly how the value should be calculated it's hard
to tell).

Thanks, Roger.

Re: Questions about Cx and Px state uploading from dom0

[Elliott Mitchell]

On Wed, Mar 23, 2022 at 09:54:24AM +0100, Roger Pau Monn?? wrote:
> 
> 2. When uploading Px states, what's the meaning of the shared_type
> field in xen_processor_performance?  I've looked at the usage of the
> field by Xen, and first of all it seems to be a layering violation
> because the values set in the field (CPUFREQ_SHARED_TYPE_*) are not
> exposed as part of the public interface.  This all works for Linux
> because the same values are used by Xen and the Linux kernel.
> Secondly, this is not part of the data fetched from ACPI AFAICT, so
> I'm unsure how the value should be calculated.  I also wonder whether
> this couldn't be done by Xen itself from the uploaded Px data (but
> without knowing exactly how the value should be calculated it's hard
> to tell).

This would account for some of the behavior with Xen and Linux.  The
Xen C-state support doesn't seem to be in a stable state.  I've seen the
level of functionality vary by version of Xen and Linux.

In particular C-states appear to be a problem.  Enabling C-states besides
C0 appears to require a corresponding Domain 0 vCPU.  If Domain 0 has
fewer vCPUs than physical cores, C2 will be unavailable on some cores.
Also, C2 is only available with some combinations of Xen and Linux.

This isn't an issue for datacenters where idle processors are wasted
money, but for smaller systems reduced power consumption is a good thing.
Hypervisors are moving onto smaller and smaller systems, so power
consumption is a bigger issue now.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: Questions about Cx and Px state uploading from dom0

[Jan Beulich]

On 23.03.2022 09:54, Roger Pau Monné wrote:
> Hello,
> 
> I was looking at implementing ACPI Cx and Px state uploading from
> FreeBSD dom0, as my main test box is considerably slower without Xen
> knowing about the Px states.  That has raised a couple of questions.
> 
> 1. How to figure out what features to report available by OSPM when
> calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
> this from Linux: it seems to be used to detect mwait support in
> xen_check_mwait but not when calling _PDC (ie: in
> acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
> the caller to provide.  Should buf[2] be set to all the possible
> features supported by the OS and Xen will trim those as required?

I'm afraid upstream Linux doesn't quite use this as originally
intended. Consulting my most recent (but meanwhile quite old) forward
port tree of XenoLinux that I still have readily available, I find in
drivers/acpi/processor_pdc.c:

static acpi_status
acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
{
	acpi_status status = AE_OK;

#ifndef CONFIG_XEN
	if (boot_option_idle_override == IDLE_NOMWAIT) {
		/*
		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
		 * mode will be disabled in the parameter of _PDC object.
		 * Of course C1_FFH access mode will also be disabled.
		 */
#else
	{
		struct xen_platform_op op;
#endif
		union acpi_object *obj;
		u32 *buffer = NULL;

		obj = pdc_in->pointer;
		buffer = (u32 *)(obj->buffer.pointer);
#ifndef CONFIG_XEN
		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
#else
		op.cmd = XENPF_set_processor_pminfo;
		op.u.set_pminfo.id = -1;
		op.u.set_pminfo.type = XEN_PM_PDC;
		set_xen_guest_handle(op.u.set_pminfo.u.pdc, buffer);
		VOID(HYPERVISOR_platform_op(&op));
#endif
	}
	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);

	if (ACPI_FAILURE(status))
		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
		    "Could not evaluate _PDC, using legacy perf. control.\n"));

	return status;
}

(This is a 4.4-based tree, for reference.)

IOW the buffer is passed to Xen for massaging before invoking _PDC.

> 2. When uploading Px states, what's the meaning of the shared_type
> field in xen_processor_performance?  I've looked at the usage of the
> field by Xen, and first of all it seems to be a layering violation
> because the values set in the field (CPUFREQ_SHARED_TYPE_*) are not
> exposed as part of the public interface.  This all works for Linux
> because the same values are used by Xen and the Linux kernel.

Well, yes - that's the way code was written back at the time when
cpufreq support was introduced. It should rather have been
DOMAIN_COORD_TYPE_* to be used in the interface, which Linux
translates to CPUFREQ_SHARED_TYPE_*.

> Secondly, this is not part of the data fetched from ACPI AFAICT, so
> I'm unsure how the value should be calculated.  I also wonder whether
> this couldn't be done by Xen itself from the uploaded Px data (but
> without knowing exactly how the value should be calculated it's hard
> to tell).

As per above - while it's not fetched from ACPI directly, there
looks to be a direct translation from what ACPI provides (see
acpi_processor_preregister_performance()).

Jan

Re: Questions about Cx and Px state uploading from dom0

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 10:13:41AM +0200, Jan Beulich wrote:
> On 23.03.2022 09:54, Roger Pau Monné wrote:
> > Hello,
> > 
> > I was looking at implementing ACPI Cx and Px state uploading from
> > FreeBSD dom0, as my main test box is considerably slower without Xen
> > knowing about the Px states.  That has raised a couple of questions.
> > 
> > 1. How to figure out what features to report available by OSPM when
> > calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
> > this from Linux: it seems to be used to detect mwait support in
> > xen_check_mwait but not when calling _PDC (ie: in
> > acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
> > the caller to provide.  Should buf[2] be set to all the possible
> > features supported by the OS and Xen will trim those as required?
> 
> I'm afraid upstream Linux doesn't quite use this as originally
> intended. Consulting my most recent (but meanwhile quite old) forward
> port tree of XenoLinux that I still have readily available, I find in
> drivers/acpi/processor_pdc.c:
> 
> static acpi_status
> acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> {
> 	acpi_status status = AE_OK;
> 
> #ifndef CONFIG_XEN
> 	if (boot_option_idle_override == IDLE_NOMWAIT) {
> 		/*
> 		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> 		 * mode will be disabled in the parameter of _PDC object.
> 		 * Of course C1_FFH access mode will also be disabled.
> 		 */
> #else
> 	{
> 		struct xen_platform_op op;
> #endif
> 		union acpi_object *obj;
> 		u32 *buffer = NULL;
> 
> 		obj = pdc_in->pointer;
> 		buffer = (u32 *)(obj->buffer.pointer);
> #ifndef CONFIG_XEN
> 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> #else
> 		op.cmd = XENPF_set_processor_pminfo;
> 		op.u.set_pminfo.id = -1;
> 		op.u.set_pminfo.type = XEN_PM_PDC;
> 		set_xen_guest_handle(op.u.set_pminfo.u.pdc, buffer);
> 		VOID(HYPERVISOR_platform_op(&op));
> #endif
> 	}
> 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> 
> 	if (ACPI_FAILURE(status))
> 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> 		    "Could not evaluate _PDC, using legacy perf. control.\n"));
> 
> 	return status;
> }
> 
> (This is a 4.4-based tree, for reference.)
> 
> IOW the buffer is passed to Xen for massaging before invoking _PDC.

Indeed.  I'm however confused by what should be pre-filled into the
buffer by the OS.  _PDC is about the processor driver power management
support, and none of this power management is done by the OS (I don't
plan to let FreeBSD do CPU power management when running as hardware
domain), so IMO passing an empty buffer and letting Xen fill it is the
correct thing to do, at least for the use-case in FreeBSD.

> > 2. When uploading Px states, what's the meaning of the shared_type
> > field in xen_processor_performance?  I've looked at the usage of the
> > field by Xen, and first of all it seems to be a layering violation
> > because the values set in the field (CPUFREQ_SHARED_TYPE_*) are not
> > exposed as part of the public interface.  This all works for Linux
> > because the same values are used by Xen and the Linux kernel.
> 
> Well, yes - that's the way code was written back at the time when
> cpufreq support was introduced. It should rather have been
> DOMAIN_COORD_TYPE_* to be used in the interface, which Linux
> translates to CPUFREQ_SHARED_TYPE_*.

I will send a patch to add those to the public headers.

> > Secondly, this is not part of the data fetched from ACPI AFAICT, so
> > I'm unsure how the value should be calculated.  I also wonder whether
> > this couldn't be done by Xen itself from the uploaded Px data (but
> > without knowing exactly how the value should be calculated it's hard
> > to tell).
> 
> As per above - while it's not fetched from ACPI directly, there
> looks to be a direct translation from what ACPI provides (see
> acpi_processor_preregister_performance()).

Yes, the translation from DOMAIN_COORD_TYPE_ to CPUFREQ_SHARED_TYPE_
is not a problem.

My concern is that there's some logic in Linux to assert the
correctness of the provided data in ACPI, checking the match of the
domain and the coordination type between all the processor objects as
part of setting the field.

I see that Xen also does some checks on the uploaded data in
cpufreq_add_cpu, so I wonder if I can get away with just setting the
shared_type field based on the coord_type of the current processor
object, without having to cross check it's coherent with the values on
other processors.

Thanks, Roger.

Re: Questions about Cx and Px state uploading from dom0

[Jan Beulich]

On 06.04.2022 12:38, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 10:13:41AM +0200, Jan Beulich wrote:
>> On 23.03.2022 09:54, Roger Pau Monné wrote:
>>> Hello,
>>>
>>> I was looking at implementing ACPI Cx and Px state uploading from
>>> FreeBSD dom0, as my main test box is considerably slower without Xen
>>> knowing about the Px states.  That has raised a couple of questions.
>>>
>>> 1. How to figure out what features to report available by OSPM when
>>> calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
>>> this from Linux: it seems to be used to detect mwait support in
>>> xen_check_mwait but not when calling _PDC (ie: in
>>> acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
>>> the caller to provide.  Should buf[2] be set to all the possible
>>> features supported by the OS and Xen will trim those as required?
>>
>> I'm afraid upstream Linux doesn't quite use this as originally
>> intended. Consulting my most recent (but meanwhile quite old) forward
>> port tree of XenoLinux that I still have readily available, I find in
>> drivers/acpi/processor_pdc.c:
>>
>> static acpi_status
>> acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
>> {
>> 	acpi_status status = AE_OK;
>>
>> #ifndef CONFIG_XEN
>> 	if (boot_option_idle_override == IDLE_NOMWAIT) {
>> 		/*
>> 		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
>> 		 * mode will be disabled in the parameter of _PDC object.
>> 		 * Of course C1_FFH access mode will also be disabled.
>> 		 */
>> #else
>> 	{
>> 		struct xen_platform_op op;
>> #endif
>> 		union acpi_object *obj;
>> 		u32 *buffer = NULL;
>>
>> 		obj = pdc_in->pointer;
>> 		buffer = (u32 *)(obj->buffer.pointer);
>> #ifndef CONFIG_XEN
>> 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>> #else
>> 		op.cmd = XENPF_set_processor_pminfo;
>> 		op.u.set_pminfo.id = -1;
>> 		op.u.set_pminfo.type = XEN_PM_PDC;
>> 		set_xen_guest_handle(op.u.set_pminfo.u.pdc, buffer);
>> 		VOID(HYPERVISOR_platform_op(&op));
>> #endif
>> 	}
>> 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
>>
>> 	if (ACPI_FAILURE(status))
>> 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> 		    "Could not evaluate _PDC, using legacy perf. control.\n"));
>>
>> 	return status;
>> }
>>
>> (This is a 4.4-based tree, for reference.)
>>
>> IOW the buffer is passed to Xen for massaging before invoking _PDC.
> 
> Indeed.  I'm however confused by what should be pre-filled into the
> buffer by the OS.  _PDC is about the processor driver power management
> support, and none of this power management is done by the OS (I don't
> plan to let FreeBSD do CPU power management when running as hardware
> domain), so IMO passing an empty buffer and letting Xen fill it is the
> correct thing to do, at least for the use-case in FreeBSD.

I don't think that would work: Xen doesn't "fill in" the buffer, but
merely alters individual bits. The buffer really is IN/OUT here for
Xen.

>>> 2. When uploading Px states, what's the meaning of the shared_type
>>> field in xen_processor_performance?  I've looked at the usage of the
>>> field by Xen, and first of all it seems to be a layering violation
>>> because the values set in the field (CPUFREQ_SHARED_TYPE_*) are not
>>> exposed as part of the public interface.  This all works for Linux
>>> because the same values are used by Xen and the Linux kernel.
>>
>> Well, yes - that's the way code was written back at the time when
>> cpufreq support was introduced. It should rather have been
>> DOMAIN_COORD_TYPE_* to be used in the interface, which Linux
>> translates to CPUFREQ_SHARED_TYPE_*.
> 
> I will send a patch to add those to the public headers.
> 
>>> Secondly, this is not part of the data fetched from ACPI AFAICT, so
>>> I'm unsure how the value should be calculated.  I also wonder whether
>>> this couldn't be done by Xen itself from the uploaded Px data (but
>>> without knowing exactly how the value should be calculated it's hard
>>> to tell).
>>
>> As per above - while it's not fetched from ACPI directly, there
>> looks to be a direct translation from what ACPI provides (see
>> acpi_processor_preregister_performance()).
> 
> Yes, the translation from DOMAIN_COORD_TYPE_ to CPUFREQ_SHARED_TYPE_
> is not a problem.
> 
> My concern is that there's some logic in Linux to assert the
> correctness of the provided data in ACPI, checking the match of the
> domain and the coordination type between all the processor objects as
> part of setting the field.
> 
> I see that Xen also does some checks on the uploaded data in
> cpufreq_add_cpu, so I wonder if I can get away with just setting the
> shared_type field based on the coord_type of the current processor
> object, without having to cross check it's coherent with the values on
> other processors.

I guess you'll get away as long as you don't hit systems with flawed
firmware. Whether the amount of checking Xen does is sufficient
depends on particular flaws found in the wild (which I lack knowledge
of).

Jan

Re: Questions about Cx and Px state uploading from dom0

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 02:47:38PM +0200, Jan Beulich wrote:
> On 06.04.2022 12:38, Roger Pau Monné wrote:
> > On Wed, Apr 06, 2022 at 10:13:41AM +0200, Jan Beulich wrote:
> >> On 23.03.2022 09:54, Roger Pau Monné wrote:
> >>> Hello,
> >>>
> >>> I was looking at implementing ACPI Cx and Px state uploading from
> >>> FreeBSD dom0, as my main test box is considerably slower without Xen
> >>> knowing about the Px states.  That has raised a couple of questions.
> >>>
> >>> 1. How to figure out what features to report available by OSPM when
> >>> calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
> >>> this from Linux: it seems to be used to detect mwait support in
> >>> xen_check_mwait but not when calling _PDC (ie: in
> >>> acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
> >>> the caller to provide.  Should buf[2] be set to all the possible
> >>> features supported by the OS and Xen will trim those as required?
> >>
> >> I'm afraid upstream Linux doesn't quite use this as originally
> >> intended. Consulting my most recent (but meanwhile quite old) forward
> >> port tree of XenoLinux that I still have readily available, I find in
> >> drivers/acpi/processor_pdc.c:
> >>
> >> static acpi_status
> >> acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> >> {
> >> 	acpi_status status = AE_OK;
> >>
> >> #ifndef CONFIG_XEN
> >> 	if (boot_option_idle_override == IDLE_NOMWAIT) {
> >> 		/*
> >> 		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> >> 		 * mode will be disabled in the parameter of _PDC object.
> >> 		 * Of course C1_FFH access mode will also be disabled.
> >> 		 */
> >> #else
> >> 	{
> >> 		struct xen_platform_op op;
> >> #endif
> >> 		union acpi_object *obj;
> >> 		u32 *buffer = NULL;
> >>
> >> 		obj = pdc_in->pointer;
> >> 		buffer = (u32 *)(obj->buffer.pointer);
> >> #ifndef CONFIG_XEN
> >> 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> >> #else
> >> 		op.cmd = XENPF_set_processor_pminfo;
> >> 		op.u.set_pminfo.id = -1;
> >> 		op.u.set_pminfo.type = XEN_PM_PDC;
> >> 		set_xen_guest_handle(op.u.set_pminfo.u.pdc, buffer);
> >> 		VOID(HYPERVISOR_platform_op(&op));
> >> #endif
> >> 	}
> >> 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> >>
> >> 	if (ACPI_FAILURE(status))
> >> 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >> 		    "Could not evaluate _PDC, using legacy perf. control.\n"));
> >>
> >> 	return status;
> >> }
> >>
> >> (This is a 4.4-based tree, for reference.)
> >>
> >> IOW the buffer is passed to Xen for massaging before invoking _PDC.
> > 
> > Indeed.  I'm however confused by what should be pre-filled into the
> > buffer by the OS.  _PDC is about the processor driver power management
> > support, and none of this power management is done by the OS (I don't
> > plan to let FreeBSD do CPU power management when running as hardware
> > domain), so IMO passing an empty buffer and letting Xen fill it is the
> > correct thing to do, at least for the use-case in FreeBSD.
> 
> I don't think that would work: Xen doesn't "fill in" the buffer, but
> merely alters individual bits. The buffer really is IN/OUT here for
> Xen.

Hm, but I have no idea what to put here from FreeBSD PoV, as said
FreeBSD will only use the processor object to upload the required data
to Xen, but won't attach any driver itself.

I've so far been providing an empty buffer to Xen and it does seem to
set the right flags so that the Cx and Px states can be fetched
afterwards.

arch_acpi_set_pdc_bits() does explicitly set some feature bits, so
there's not only cleanup done there.

> >>> 2. When uploading Px states, what's the meaning of the shared_type
> >>> field in xen_processor_performance?  I've looked at the usage of the
> >>> field by Xen, and first of all it seems to be a layering violation
> >>> because the values set in the field (CPUFREQ_SHARED_TYPE_*) are not
> >>> exposed as part of the public interface.  This all works for Linux
> >>> because the same values are used by Xen and the Linux kernel.
> >>
> >> Well, yes - that's the way code was written back at the time when
> >> cpufreq support was introduced. It should rather have been
> >> DOMAIN_COORD_TYPE_* to be used in the interface, which Linux
> >> translates to CPUFREQ_SHARED_TYPE_*.
> > 
> > I will send a patch to add those to the public headers.
> > 
> >>> Secondly, this is not part of the data fetched from ACPI AFAICT, so
> >>> I'm unsure how the value should be calculated.  I also wonder whether
> >>> this couldn't be done by Xen itself from the uploaded Px data (but
> >>> without knowing exactly how the value should be calculated it's hard
> >>> to tell).
> >>
> >> As per above - while it's not fetched from ACPI directly, there
> >> looks to be a direct translation from what ACPI provides (see
> >> acpi_processor_preregister_performance()).
> > 
> > Yes, the translation from DOMAIN_COORD_TYPE_ to CPUFREQ_SHARED_TYPE_
> > is not a problem.
> > 
> > My concern is that there's some logic in Linux to assert the
> > correctness of the provided data in ACPI, checking the match of the
> > domain and the coordination type between all the processor objects as
> > part of setting the field.
> > 
> > I see that Xen also does some checks on the uploaded data in
> > cpufreq_add_cpu, so I wonder if I can get away with just setting the
> > shared_type field based on the coord_type of the current processor
> > object, without having to cross check it's coherent with the values on
> > other processors.
> 
> I guess you'll get away as long as you don't hit systems with flawed
> firmware. Whether the amount of checking Xen does is sufficient
> depends on particular flaws found in the wild (which I lack knowledge
> of).

I guess I will do with that. It's all experimental anyway, and if I
find one of such systems I would try to fix on Xen then.

Thanks, Roger.

Re: Questions about Cx and Px state uploading from dom0

[Jan Beulich]

On 06.04.2022 17:09, Roger Pau Monné wrote:
> On Wed, Apr 06, 2022 at 02:47:38PM +0200, Jan Beulich wrote:
>> On 06.04.2022 12:38, Roger Pau Monné wrote:
>>> On Wed, Apr 06, 2022 at 10:13:41AM +0200, Jan Beulich wrote:
>>>> On 23.03.2022 09:54, Roger Pau Monné wrote:
>>>>> Hello,
>>>>>
>>>>> I was looking at implementing ACPI Cx and Px state uploading from
>>>>> FreeBSD dom0, as my main test box is considerably slower without Xen
>>>>> knowing about the Px states.  That has raised a couple of questions.
>>>>>
>>>>> 1. How to figure out what features to report available by OSPM when
>>>>> calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
>>>>> this from Linux: it seems to be used to detect mwait support in
>>>>> xen_check_mwait but not when calling _PDC (ie: in
>>>>> acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
>>>>> the caller to provide.  Should buf[2] be set to all the possible
>>>>> features supported by the OS and Xen will trim those as required?
>>>>
>>>> I'm afraid upstream Linux doesn't quite use this as originally
>>>> intended. Consulting my most recent (but meanwhile quite old) forward
>>>> port tree of XenoLinux that I still have readily available, I find in
>>>> drivers/acpi/processor_pdc.c:
>>>>
>>>> static acpi_status
>>>> acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
>>>> {
>>>> 	acpi_status status = AE_OK;
>>>>
>>>> #ifndef CONFIG_XEN
>>>> 	if (boot_option_idle_override == IDLE_NOMWAIT) {
>>>> 		/*
>>>> 		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
>>>> 		 * mode will be disabled in the parameter of _PDC object.
>>>> 		 * Of course C1_FFH access mode will also be disabled.
>>>> 		 */
>>>> #else
>>>> 	{
>>>> 		struct xen_platform_op op;
>>>> #endif
>>>> 		union acpi_object *obj;
>>>> 		u32 *buffer = NULL;
>>>>
>>>> 		obj = pdc_in->pointer;
>>>> 		buffer = (u32 *)(obj->buffer.pointer);
>>>> #ifndef CONFIG_XEN
>>>> 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
>>>> #else
>>>> 		op.cmd = XENPF_set_processor_pminfo;
>>>> 		op.u.set_pminfo.id = -1;
>>>> 		op.u.set_pminfo.type = XEN_PM_PDC;
>>>> 		set_xen_guest_handle(op.u.set_pminfo.u.pdc, buffer);
>>>> 		VOID(HYPERVISOR_platform_op(&op));
>>>> #endif
>>>> 	}
>>>> 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
>>>>
>>>> 	if (ACPI_FAILURE(status))
>>>> 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>>>> 		    "Could not evaluate _PDC, using legacy perf. control.\n"));
>>>>
>>>> 	return status;
>>>> }
>>>>
>>>> (This is a 4.4-based tree, for reference.)
>>>>
>>>> IOW the buffer is passed to Xen for massaging before invoking _PDC.
>>>
>>> Indeed.  I'm however confused by what should be pre-filled into the
>>> buffer by the OS.  _PDC is about the processor driver power management
>>> support, and none of this power management is done by the OS (I don't
>>> plan to let FreeBSD do CPU power management when running as hardware
>>> domain), so IMO passing an empty buffer and letting Xen fill it is the
>>> correct thing to do, at least for the use-case in FreeBSD.
>>
>> I don't think that would work: Xen doesn't "fill in" the buffer, but
>> merely alters individual bits. The buffer really is IN/OUT here for
>> Xen.
> 
> Hm, but I have no idea what to put here from FreeBSD PoV, as said
> FreeBSD will only use the processor object to upload the required data
> to Xen, but won't attach any driver itself.
> 
> I've so far been providing an empty buffer to Xen and it does seem to
> set the right flags so that the Cx and Px states can be fetched
> afterwards.

Hmm, an empty buffer should result in -EINVAL from the hypercall.
The first of the three uint32_t-s should be 1 (ACPI_PDC_REVISION_ID)
and the 2nd (size) is supposed to be non-zero.

> arch_acpi_set_pdc_bits() does explicitly set some feature bits, so
> there's not only cleanup done there.

Well, yes, I did say "alter", not "clear", for that reason.

Jan

Re: Questions about Cx and Px state uploading from dom0

[Roger Pau Monné]

On Wed, Apr 06, 2022 at 05:51:06PM +0200, Jan Beulich wrote:
> On 06.04.2022 17:09, Roger Pau Monné wrote:
> > On Wed, Apr 06, 2022 at 02:47:38PM +0200, Jan Beulich wrote:
> >> On 06.04.2022 12:38, Roger Pau Monné wrote:
> >>> On Wed, Apr 06, 2022 at 10:13:41AM +0200, Jan Beulich wrote:
> >>>> On 23.03.2022 09:54, Roger Pau Monné wrote:
> >>>>> Hello,
> >>>>>
> >>>>> I was looking at implementing ACPI Cx and Px state uploading from
> >>>>> FreeBSD dom0, as my main test box is considerably slower without Xen
> >>>>> knowing about the Px states.  That has raised a couple of questions.
> >>>>>
> >>>>> 1. How to figure out what features to report available by OSPM when
> >>>>> calling the _PDC (or _OSC) ACPI method.  I'm confused by the usage of
> >>>>> this from Linux: it seems to be used to detect mwait support in
> >>>>> xen_check_mwait but not when calling _PDC (ie: in
> >>>>> acpi_processor_set_pdc).  I'm also not sure what the hypercall expects
> >>>>> the caller to provide.  Should buf[2] be set to all the possible
> >>>>> features supported by the OS and Xen will trim those as required?
> >>>>
> >>>> I'm afraid upstream Linux doesn't quite use this as originally
> >>>> intended. Consulting my most recent (but meanwhile quite old) forward
> >>>> port tree of XenoLinux that I still have readily available, I find in
> >>>> drivers/acpi/processor_pdc.c:
> >>>>
> >>>> static acpi_status
> >>>> acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> >>>> {
> >>>> 	acpi_status status = AE_OK;
> >>>>
> >>>> #ifndef CONFIG_XEN
> >>>> 	if (boot_option_idle_override == IDLE_NOMWAIT) {
> >>>> 		/*
> >>>> 		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> >>>> 		 * mode will be disabled in the parameter of _PDC object.
> >>>> 		 * Of course C1_FFH access mode will also be disabled.
> >>>> 		 */
> >>>> #else
> >>>> 	{
> >>>> 		struct xen_platform_op op;
> >>>> #endif
> >>>> 		union acpi_object *obj;
> >>>> 		u32 *buffer = NULL;
> >>>>
> >>>> 		obj = pdc_in->pointer;
> >>>> 		buffer = (u32 *)(obj->buffer.pointer);
> >>>> #ifndef CONFIG_XEN
> >>>> 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> >>>> #else
> >>>> 		op.cmd = XENPF_set_processor_pminfo;
> >>>> 		op.u.set_pminfo.id = -1;
> >>>> 		op.u.set_pminfo.type = XEN_PM_PDC;
> >>>> 		set_xen_guest_handle(op.u.set_pminfo.u.pdc, buffer);
> >>>> 		VOID(HYPERVISOR_platform_op(&op));
> >>>> #endif
> >>>> 	}
> >>>> 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> >>>>
> >>>> 	if (ACPI_FAILURE(status))
> >>>> 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >>>> 		    "Could not evaluate _PDC, using legacy perf. control.\n"));
> >>>>
> >>>> 	return status;
> >>>> }
> >>>>
> >>>> (This is a 4.4-based tree, for reference.)
> >>>>
> >>>> IOW the buffer is passed to Xen for massaging before invoking _PDC.
> >>>
> >>> Indeed.  I'm however confused by what should be pre-filled into the
> >>> buffer by the OS.  _PDC is about the processor driver power management
> >>> support, and none of this power management is done by the OS (I don't
> >>> plan to let FreeBSD do CPU power management when running as hardware
> >>> domain), so IMO passing an empty buffer and letting Xen fill it is the
> >>> correct thing to do, at least for the use-case in FreeBSD.
> >>
> >> I don't think that would work: Xen doesn't "fill in" the buffer, but
> >> merely alters individual bits. The buffer really is IN/OUT here for
> >> Xen.
> > 
> > Hm, but I have no idea what to put here from FreeBSD PoV, as said
> > FreeBSD will only use the processor object to upload the required data
> > to Xen, but won't attach any driver itself.
> > 
> > I've so far been providing an empty buffer to Xen and it does seem to
> > set the right flags so that the Cx and Px states can be fetched
> > afterwards.
> 
> Hmm, an empty buffer should result in -EINVAL from the hypercall.
> The first of the three uint32_t-s should be 1 (ACPI_PDC_REVISION_ID)
> and the 2nd (size) is supposed to be non-zero.

Right, I guess I was too simplistic here. I'm passing a buffer with
{1, 1, 0}, so no features added by the OS (because it won't attach any
driver to the processor object).

Thanks, Roger.