Re: [PATCH v3] acpi/processor: sanitize _OSC/_PDC capabilities for Xen dom0

[Jason Andryuk <jandryuk@xxxxxxxxx>]

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