[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 05/15] xen/x86: introduce "cpufreq=amd-cppc" xen cmdline
On 14.04.2025 09:40, Penny Zheng wrote: > @@ -514,5 +515,16 @@ acpi_cpufreq_driver = { > > int __init acpi_cpufreq_register(void) > { > - return cpufreq_register_driver(&acpi_cpufreq_driver); > + int ret; > + > + ret = cpufreq_register_driver(&acpi_cpufreq_driver); > + if ( ret ) > + return ret; > + /* > + * After cpufreq driver registeration, XEN_PROCESSOR_PM_CPPC > + * and XEN_PROCESSOR_PM_PX shall become exclusive flags > + */ > + xen_processor_pmbits &= ~XEN_PROCESSOR_PM_CPPC; > + > + return ret; > } Why is no similar adjustment needed in powernow_register_driver()? In principle I would have expected that it's not each individual driver which needs to care about this aspect, but that the framework is taking care of this. > --- /dev/null > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c > @@ -0,0 +1,81 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * amd-cppc.c - AMD Processor CPPC Frequency Driver > + * > + * Copyright (C) 2025 Advanced Micro Devices, Inc. All Rights Reserved. > + * > + * Author: Penny Zheng <penny.zheng@xxxxxxx> > + * > + * AMD CPPC cpufreq driver introduces a new CPU performance scaling design > + * for AMD processors using the ACPI Collaborative Performance and Power > + * Control (CPPC) feature which provides finer grained frequency control > range. > + */ > + > +#include <xen/domain.h> > +#include <xen/init.h> > +#include <xen/param.h> > +#include <acpi/cpufreq/cpufreq.h> > + > +static bool __init amd_cppc_handle_option(const char *s, const char *end) > +{ > + int ret; > + > + ret = parse_boolean("verbose", s, end); > + if ( ret >= 0 ) > + { > + cpufreq_verbose = ret; > + return true; > + } > + > + return false; > +} > + > +int __init amd_cppc_cmdline_parse(const char *s, const char *e) > +{ > + do > + { Nit (style): Brace placement is special here, just like it is ... > + const char *end = strpbrk(s, ",;"); > + > + if ( !amd_cppc_handle_option(s, end) ) > + { > + printk(XENLOG_WARNING > + "cpufreq/amd-cppc: option '%.*s' not recognized\n", > + (int)((end ?: e) - s), s); > + > + return -EINVAL; > + } > + > + s = end ? end + 1 : NULL; > + } while ( s && s < e ); ... here. > --- a/xen/arch/x86/platform_hypercall.c > +++ b/xen/arch/x86/platform_hypercall.c > @@ -542,6 +542,9 @@ ret_t do_platform_op( > ret = -ENOSYS; > break; > } > + /* Xen doesn't support mixed mode */ > + ASSERT((xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) == 0); Please prefer ! over "== 0" in such purely boolean contexts. > @@ -573,6 +576,14 @@ ret_t do_platform_op( > } > > case XEN_PM_CPPC: > + if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC) ) > + { > + ret = -EOPNOTSUPP; > + break; > + } While at least you no longer use -ENOSYS here, I question this behavior, including that for the pre-existing cases: How is the caller supposed to know whether to invoke this sub-op? Ignoring errors is generally not a good idea, so it would be better if the caller could blindly issue this request, getting back success unless there really was an issue with the data provided. > @@ -102,6 +103,9 @@ static int __init handle_cpufreq_cmdline(enum > cpufreq_xen_opt option) > cpufreq_xen_opts[cpufreq_xen_cnt++] = option; > switch ( option ) > { > + case CPUFREQ_amd_cppc: > + xen_processor_pmbits |= XEN_PROCESSOR_PM_CPPC; > + break; > case CPUFREQ_hwp: > case CPUFREQ_xen: > xen_processor_pmbits |= XEN_PROCESSOR_PM_PX; Here and (about) everywhere else: Blank line please between non-fall-through case blocks. I guess I'm not going to repeat this any further. There are very tiny switch() statements where it is okay to violate this principle, but as a rule of thumb - if in doubt, put a blank line there. > --- a/xen/include/acpi/cpufreq/processor_perf.h > +++ b/xen/include/acpi/cpufreq/processor_perf.h > @@ -5,6 +5,9 @@ > #include <public/sysctl.h> > #include <xen/acpi.h> > > +/* ability bits */ > +#define XEN_PROCESSOR_PM_CPPC 8 This needs correlating (at least via commentary, better by build-time checking) with the other XEN_PROCESSOR_PM_* values. Otherwise someone adding a new #define in the public header may not (easily) notice a possible conflict. With that in mind I also question whether 8 is actually a good choice: That's the obvious next value to use in the public interface. SIF_PM_MASK is 8 bits wide, so a sensible value to use here would by e.g. 0x100. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |