Thanks for taking a look, Michal.
On Fri, Oct 13, 2023 at 12:17 PM Wilczynski, Michal
<michal.wilczynski@xxxxxxxxx> wrote:
>
> Hi,
>
> On 10/10/2023 7:39 PM, Jason Andryuk wrote:
> > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> >
> > The Processor capability bits notify ACPI of the OS capabilities, and
> > so ACPI can adjust the return of other Processor methods taking the OS
> > capabilities into account.
> >
> > When Linux is running as a Xen dom0, the hypervisor is the entity
> > in charge of processor power management, and hence Xen needs to make
> > sure the capabilities reported by _OSC/_PDC match the capabilities of
> > the driver in Xen.
> >
> > Introduce a small helper to sanitize the buffer when running as Xen
> > dom0.
> >
> > When Xen supports HWP, this serves as the equivalent of commit
> > a21211672c9a ("ACPI / processor: Request native thermal interrupt
> > handling via _OSC") to avoid SMM crashes. Xen will set bit
> > ACPI_PROC_CAP_COLLAB_PROC_PERF (bit 12) in the capability bits and the
> > _OSC/_PDC call will apply it.
> >
> > [ jandryuk: Mention Xen HWP's need. Support _OSC & _PDC ]
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> > ---
> > v3:
> > Move xen_sanitize_pdc() call to arch_acpi_set_proc_cap_bits() to cover
> > _OSC and _PDC.
> > drivers/xen/pcpu.c is CONFIG_DOM0 && CONFIG_X86
> >
> > v2:
> > Move local variables in acpi_processor_eval_pdc() to reuse in both conditions.
> > ---
> > arch/x86/include/asm/acpi.h | 13 +++++++++++++
> > arch/x86/include/asm/xen/hypervisor.h | 9 +++++++++
> > drivers/xen/pcpu.c | 21 +++++++++++++++++++++
> > 3 files changed, 43 insertions(+)
> >
> > diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> > index c8a7fc23f63c..cc8d1669d6e8 100644
> > --- a/arch/x86/include/asm/acpi.h
> > +++ b/arch/x86/include/asm/acpi.h
> > @@ -16,6 +16,9 @@
> > #include <asm/x86_init.h>
> > #include <asm/cpufeature.h>
> > #include <asm/irq_vectors.h>
> > +#include <asm/xen/hypervisor.h>
> > +
> > +#include <xen/xen.h>
> >
> > #ifdef CONFIG_ACPI_APEI
> > # include <asm/pgtable_types.h>
> > @@ -127,6 +130,16 @@ static inline void arch_acpi_set_proc_cap_bits(u32 *cap)
> > if (!cpu_has(c, X86_FEATURE_MWAIT) ||
> > boot_option_idle_override == IDLE_NOMWAIT)
> > *cap &= ~(ACPI_PROC_CAP_C_C1_FFH | ACPI_PROC_CAP_C_C2C3_FFH);
> > +
> > + if (xen_initial_domain()) {
> > + /*
> > + * When Linux is running as Xen dom0, the hypervisor is the
> > + * entity in charge of the processor power management, and so
> > + * Xen needs to check the OS capabilities reported in the _PDC
>
> I would argue the phrasing here is unfortunate - it's not really _PDC buffer anymore,
> it's more processor capabilities buffer [1]. Your phrasing would indicate that this
> buffer is somehow _PDC specific.
Ok.
> BTW this file is x86 specific code - are you sure it's appropriate to involve Xen
> hypervisor here ? I understand this case if x86 specific, but still.
The Xen hypercall is x86-specific. I see
`arch_acpi_set_proc_cap_bits()` as a hook to set arch-specific bits.
Looking at Xen/x86 as the arch, it makes sense. The other option
would be a Xen conditional back in the acpi code. Keeping it with the
x86 code therefore made more sense to me.
> > + * buffer matches what the hypervisor driver supports.
> > + */
> > + xen_sanitize_pdc(cap);
>
> Same here as in [1], I would call this function xen_sanitize_proc_cap_buffer(),
> or something along those lines for better readability and correctness.
Ok.
> > + }
> > }
> >
> > static inline bool acpi_has_cpu_in_madt(void)
> > diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
> > index 7048dfacc04b..c6c2f174fa30 100644
> > --- a/arch/x86/include/asm/xen/hypervisor.h
> > +++ b/arch/x86/include/asm/xen/hypervisor.h
> > @@ -100,4 +100,13 @@ static inline void leave_lazy(enum xen_lazy_mode mode)
> >
> > enum xen_lazy_mode xen_get_lazy_mode(void);
> >
> > +#if defined(CONFIG_XEN_DOM0) && defined(CONFIG_ACPI)
> > +void xen_sanitize_pdc(uint32_t *buf);
> > +#else
> > +static inline void xen_sanitize_pdc(uint32_t *buf)
> > +{
> > + BUG();
> > +}
> > +#endif
> > +
> > #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> > diff --git a/drivers/xen/pcpu.c b/drivers/xen/pcpu.c
> > index b3e3d1bb37f3..859bb6027105 100644
> > --- a/drivers/xen/pcpu.c
> > +++ b/drivers/xen/pcpu.c
> > @@ -47,6 +47,9 @@
> > #include <asm/xen/hypervisor.h>
> > #include <asm/xen/hypercall.h>
> >
> > +#ifdef CONFIG_ACPI
> > +#include <acpi/processor.h>
> > +#endif
> >
> > /*
> > * @cpu_id: Xen physical cpu logic number
> > @@ -400,4 +403,22 @@ bool __init xen_processor_present(uint32_t acpi_id)
> >
> > return online;
> > }
> > +
> > +void xen_sanitize_pdc(uint32_t *cap)
> > +{
> > + struct xen_platform_op op = {
> > + .cmd = XENPF_set_processor_pminfo,
> > + .u.set_pminfo.id = -1,
> > + .u.set_pminfo.type = XEN_PM_PDC,
>
> It would probably be best to rename this constant as well so it's
> not misleading.
That is a Xen constant, so we can't change it.
> > + };
> > + u32 buf[3] = { ACPI_PDC_REVISION_ID, 1, *cap };
> > + int ret;
> > +
> > + set_xen_guest_handle(op.u.set_pminfo.pdc, buf);
> > + ret = HYPERVISOR_platform_op(&op);
> > + if (ret)
> > + pr_info("sanitize of _PDC buffer bits from Xen failed: %d\n",
> > + ret);
>
> Shouldn't an error be pr_err ?
Sure.
> > + *cap = buf[2];
> > +}
> > #endif
Thanks,
Jason
