Re: Questions about Cx and Px state uploading from dom0

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

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.