Re: Questions about Cx and Px state uploading from dom0

[Jan Beulich <jbeulich@xxxxxxxx>]

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