Re: Questions about Cx and Px state uploading from dom0

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

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.