Re: [Xen-devel] [RFC PATCH 05/31] pmstat: make pmstat functions more generalizable

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 4 Dec 2017, Oleksandr Tyshchenko wrote:
> Hi Stefano
> 
> On Sat, Dec 2, 2017 at 3:21 AM, Stefano Stabellini
> <sstabellini@xxxxxxxxxx> wrote:
> > On Thu, 9 Nov 2017, Oleksandr Tyshchenko wrote:
> >> From: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> >>
> >> ACPI-specific parts are moved under appropriate ifdefs.
> >> Now pmstat functions can be used in ARM platform.
> >>
> >> This is a rebased version of the original patch:
> >> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00941.html
> >
> > My first maybe naive question is: why do we want to disable the C-states
> > and not the P-states? After all, they are both defined in ACPI?
> 
> Good question. Xen CPUFreq infrastructure based on ACPI P-states. We
> have to either
> completely rework generic code/existing drivers or integrate into
> "current environment" (so, the CPUFreq driver,
> this patch series adds, is pretending that it does understand what the
> P-states are). The second option requires much less
> developing & upstreaming (I hope) efforts. BTW, with the current
> solution you don't have to modify public sysctl & xenpm.
> And looking through all previous discussions [1] I got a feeling that
> the original author of this patch had had similar opinion.
> 
> [1]
> /* RFC v0 */
> https://lists.xen.org/archives/html/xen-devel/2014-08/msg02919.html
> /* RFC v1 */
> https://lists.xenproject.org/archives/html/xen-devel/2014-10/msg00787.html
> /* RFC v2 */
> https://lists.xenproject.org/archives/html/xen-devel/2014-10/msg01879.html
> /* RFC v3 */
> https://marc.info/?l=xen-devel&m=141407701110860&w=2
> /* RFC v4 */
> https://marc.info/?l=xen-devel&m=141510663108037&w=2
> /* RFC v5 */
> https://lists.xen.org/archives/html/xen-devel/2014-11/msg00940.html

thank you, it makes sense


> >
> > The second question is: instead of #ifdef'ing everything C-states,
> > couldn't we just rely on XEN_PROCESSOR_PM_CX not being available?
> 
> I am afraid that relying on XEN_PROCESSOR_PM_CX not being available is
> not enough.
> A few functions, which were #ifdef'd by original author of the patch,
> are located at arch/x86 path.
> So, I think, the question was to get pmstat.c compilable on ARM.
> 
> But completely agree that a scope of #ifdef's can be reduced.
> 
> 1. For next functions we will be able to omit #ifdef CONFIG_ACPI if we
> create corresponding stubs.
> - pmstat_get_cx_nr()
> - pmstat_get_cx_stat()
> - pmstat_reset_cx_stat()
> They won't never be called if XEN_PROCESSOR_PM_CX is not set.

sounds good


> 2. For next functions we, probably, may omit #ifdef CONFIG_ACPI, since
> the corresponding stubs already present (see !CONFIG_ACPI_CSTATE in
> acpi.h)
> - acpi_get_cstate_limit()
> - acpi_set_cstate_limit()

it looks like it, yes


> But acpi_set_pdc_bits() I would leave under #ifdef CONFIG_ACPI
> (CONFIG_X86 ?) or move it to arch/x86.
> It is called from arch/x86/platform_hypercall.c and pulls a bunch of
> #define-s from pdc_intel.h

Yes, I would move it to arch/x86.


> Something like that:
> 
> diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
> index 133e64d..353d0ab 100644
> --- a/xen/drivers/pm/stat.c
> +++ b/xen/drivers/pm/stat.c
> @@ -500,6 +500,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
>      return ret;
>  }
> 
> +#ifdef CONFIG_ACPI /* or CONFIG_X86 ? */
>  int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
>  {
>      u32 bits[3];
> @@ -530,3 +531,4 @@ int acpi_set_pdc_bits(u32 acpi_id,
> XEN_GUEST_HANDLE_PARAM(uint32) pdc)
> 
>      return ret;
>  }
> +#endif /* CONFIG_ACPI */
> diff --git a/xen/include/xen/pmstat.h b/xen/include/xen/pmstat.h
> index 266bc16..05d6b7b 100644
> --- a/xen/include/xen/pmstat.h
> +++ b/xen/include/xen/pmstat.h
> @@ -6,10 +6,17 @@
>  #include <public/sysctl.h>   /* for struct pm_cx_stat */
> 
>  int set_px_pminfo(uint32_t cpu, struct xen_processor_performance *perf);
> +#ifdef CONFIG_ACPI /* or CONFIG_X86 ? */
>  long set_cx_pminfo(uint32_t cpu, struct xen_processor_power *power);
>  uint32_t pmstat_get_cx_nr(uint32_t cpuid);
>  int pmstat_get_cx_stat(uint32_t cpuid, struct pm_cx_stat *stat);
>  int pmstat_reset_cx_stat(uint32_t cpuid);
> +#else
> +static inline long set_cx_pminfo(uint32_t cpu, struct
> xen_processor_power *power) { return 0; }
> +static inline uint32_t pmstat_get_cx_nr(uint32_t cpuid) { return 0; }
> +static inline int pmstat_get_cx_stat(uint32_t cpuid, struct
> pm_cx_stat *stat) { return 0; }
> +static inline int pmstat_reset_cx_stat(uint32_t cpuid) { return 0; }
> +#endif
> 
>  int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
>  int do_pm_op(struct xen_sysctl_pm_op *op);
> 
> What do you think?

much better


> >
> >
> >> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> >> CC: Jan Beulich <jbeulich@xxxxxxxx>
> >> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> CC: Julien Grall <julien.grall@xxxxxxxxxx>
> >> ---
> >>  xen/drivers/pm/stat.c    | 8 +++++++-
> >>  xen/include/xen/pmstat.h | 2 ++
> >>  2 files changed, 9 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/drivers/pm/stat.c b/xen/drivers/pm/stat.c
> >> index 133e64d..986ba41 100644
> >> --- a/xen/drivers/pm/stat.c
> >> +++ b/xen/drivers/pm/stat.c
> >> @@ -35,7 +35,6 @@
> >>  #include <asm/processor.h>
> >>  #include <xen/percpu.h>
> >>  #include <xen/domain.h>
> >> -#include <xen/acpi.h>
> >>
> >>  #include <public/sysctl.h>
> >>  #include <xen/cpufreq.h>
> >> @@ -132,6 +131,8 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
> >>          break;
> >>      }
> >>
> >> +/* For now those operations can be used only when ACPI is enabled */
> >> +#ifdef CONFIG_ACPI
> >>      case PMSTAT_get_max_cx:
> >>      {
> >>          op->u.getcx.nr = pmstat_get_cx_nr(op->cpuid);
> >> @@ -150,6 +151,7 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
> >>          ret = pmstat_reset_cx_stat(op->cpuid);
> >>          break;
> >>      }
> >> +#endif /* CONFIG_ACPI */
> >>
> >>      default:
> >>          printk("not defined sub-hypercall @ do_get_pm_info\n");
> >> @@ -465,6 +467,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
> >>          break;
> >>      }
> >>
> >> +#ifdef CONFIG_ACPI
> >>      case XEN_SYSCTL_pm_op_get_max_cstate:
> >>      {
> >>          op->u.get_max_cstate = acpi_get_cstate_limit();
> >> @@ -476,6 +479,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
> >>          acpi_set_cstate_limit(op->u.set_max_cstate);
> >>          break;
> >>      }
> >> +#endif /* CONFIG_ACPI */
> >>
> >>  #ifdef CONFIG_HAS_CPU_TURBO
> >>      case XEN_SYSCTL_pm_op_enable_turbo:
> >> @@ -500,6 +504,7 @@ int do_pm_op(struct xen_sysctl_pm_op *op)
> >>      return ret;
> >>  }
> >>
> >> +#ifdef CONFIG_ACPI
> >>  int acpi_set_pdc_bits(u32 acpi_id, XEN_GUEST_HANDLE_PARAM(uint32) pdc)
> >>  {
> >>      u32 bits[3];
> >> @@ -530,3 +535,4 @@ int acpi_set_pdc_bits(u32 acpi_id, 
> >> XEN_GUEST_HANDLE_PARAM(uint32) pdc)
> >>
> >>      return ret;
> >>  }
> >> +#endif /* CONFIG_ACPI */
> >> diff --git a/xen/include/xen/pmstat.h b/xen/include/xen/pmstat.h
> >> index 266bc16..a870c8a 100644
> >> --- a/xen/include/xen/pmstat.h
> >> +++ b/xen/include/xen/pmstat.h
> >> @@ -6,10 +6,12 @@
> >>  #include <public/sysctl.h>   /* for struct pm_cx_stat */
> >>
> >>  int set_px_pminfo(uint32_t cpu, struct xen_processor_performance *perf);
> >> +#ifdef CONFIG_ACPI
> >>  long set_cx_pminfo(uint32_t cpu, struct xen_processor_power *power);
> >>  uint32_t pmstat_get_cx_nr(uint32_t cpuid);
> >>  int pmstat_get_cx_stat(uint32_t cpuid, struct pm_cx_stat *stat);
> >>  int pmstat_reset_cx_stat(uint32_t cpuid);
> >> +#endif
> >>
> >>  int do_get_pm_info(struct xen_sysctl_get_pmstat *op);
> >>  int do_pm_op(struct xen_sysctl_pm_op *op);
> >> --
> >> 2.7.4
> >>
> 
> 
> 
> -- 
> Regards,
> 
> Oleksandr Tyshchenko
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel