Re: Questions about Cx and Px state uploading from dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.