On Wed, Jun 14, 2023 at 03:57:11PM -0400, Jason Andryuk wrote:
> Hi, Roger,
>
> On Mon, Nov 21, 2022 at 10:04 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Mon, Nov 21, 2022 at 03:10:36PM +0100, Jan Beulich wrote:
> > > On 21.11.2022 11:21, Roger Pau Monne wrote:
> > > > --- a/drivers/acpi/processor_pdc.c
> > > > +++ b/drivers/acpi/processor_pdc.c
> > > > @@ -137,6 +137,14 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
> > > > buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> > > >
> > > > }
> > > > + if (xen_initial_domain())
> > > > + /*
> > > > + * When Linux is running as Xen dom0 it's the hypervisor the
> > > > + * entity in charge of the processor power management, and so
> > > > + * Xen needs to check the OS capabilities reported in the _PDC
> > > > + * buffer matches what the hypervisor driver supports.
> > > > + */
> > > > + xen_sanitize_pdc((uint32_t *)pdc_in->pointer->buffer.pointer);
> > > > status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
> > >
> > > Again looking at our old XenoLinux forward port we had this inside the
> > > earlier if(), as an _alternative_ to the &= (I don't think it's valid
> > > to apply both the kernel's and Xen's adjustments). That would also let
> > > you use "buffer" rather than re-calculating it via yet another (risky
> > > from an abstract pov) cast.
> >
> > Hm, I've wondered this and decided it wasn't worth to short-circuit
> > the boot_option_idle_override conditional because ACPI_PDC_C_C2C3_FFH
> > and ACPI_PDC_C_C1_FFH will be set anyway by Xen in
> > arch_acpi_set_pdc_bits() as part of ACPI_PDC_C_CAPABILITY_SMP.
> >
> > I could re-use some of the code in there, but didn't want to make it
> > more difficult to read just for the benefit of reusing buffer.
> >
> > > It was the very nature of requiring Xen-specific conditionals which I
> > > understand was the reason why so far no attempt was made to get this
> > > (incl the corresponding logic for patch 1) into any upstream kernel.
> >
> > Yes, well, it's all kind of ugly. Hence my suggestion to simply avoid
> > doing any ACPI Processor object handling in Linux with the native code
> > and handle it all in a Xen specific driver. That requires the Xen
> > driver being able to fetch more data itself form the ACPI Processor
> > methods, but also unties it from the dependency on the data being
> > filled by the generic code, and the 'tricks' is plays into fooling
> > generic code to think certain processors are online.
>
> Are you working on this patch anymore?
Not really, I didn't get any feedback from maintainers (apart from
Jans comments, which I do value), and wasn't aware of this causing
issues, or being required by any other work, hence I kind of dropped
it (I have plenty of other stuff to work on).
> My Xen HWP patches need a
> Linux patch like this one to set bit 12 in the PDC. I had an affected
> user test with this patch and it worked, serving as an equivalent of
> Linux commit a21211672c9a ("ACPI / processor: Request native thermal
> interrupt handling via _OSC").
>
> Another idea is to use Linux's arch_acpi_set_pdc_bits() to make the
> hypercall to Xen. It occurs earlier:
> acpi_processor_set_pdc()
> acpi_processor_alloc_pdc()
> acpi_set_pdc_bits()
> arch_acpi_set_pdc_bits()
> acpi_processor_eval_pdc()
>
> So the IDLE_NOMWAIT masking in acpi_processor_eval_pdc() would still
> apply. arch_acpi_set_pdc_bits() is provided the buffer, so it's a
> little cleaner in that respect.
I see. My reasoning for placing the Xen filtering in
acpi_processor_eval_pdc() is so that there are no further
modifications to the buffer by Linux after the call to sanitize the
buffer (XENPF_set_processor_pminfo).
I think if the filtering done by Xen is moved to
arch_acpi_set_pdc_bits() we would then need to disable the evaluation
of boot_option_idle_override in acpi_processor_eval_pdc() as we don't
want dom0 choices affecting the selection of _PDC features done by
Xen?
In any case, feel free to pick this patch and re-submit upstream if
you want.
Thanks, Roger.
