[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Thursday, July 17, 2025 9:36 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Anthony PERARD <anthony.per...@vates.tech>;
> Orzel, Michal <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger Pau
> Monné <roger....@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>;
> xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v6 13/19] xen/x86: implement amd-cppc-epp driver for CPPC
> in active mode
>
> On 11.07.2025 05:51, Penny Zheng wrote:
> > --- a/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> > +++ b/xen/arch/x86/acpi/cpufreq/amd-cppc.c
> > @@ -259,11 +276,18 @@ static void cf_check
> > amd_cppc_write_request_msrs(void *info) }
> >
> > static void amd_cppc_write_request(unsigned int cpu, uint8_t min_perf,
> > - uint8_t des_perf, uint8_t max_perf)
> > + uint8_t des_perf, uint8_t max_perf,
> > + uint8_t epp)
> > {
> > struct amd_cppc_drv_data *data = per_cpu(amd_cppc_drv_data, cpu);
> > uint64_t prev = data->req.raw;
> >
> > + if ( !opt_active_mode )
> > + data->req.des_perf = des_perf;
> > + else
> > + data->req.des_perf = 0;
>
> In amd_cppc_epp_set_policy() you pass 0 anyway. Why is this needed? With this
> change dropped, opt_active_mode can become __initdata. (But of course you may
> want to add an assertion instead, in which case the variable needs to stay
> where it
> is at least in debug builds.)
>
True, the if-else seems redundant. I'll make opt_active_mode __initdata under
NDEBUG
```
#ifndef NDEBUG
static bool __ro_after_init opt_active_mode;
#else
static bool __initdata opt_active_mode;
#endif
```
> > + data->req.epp = epp;
>
> Ahead of this patch, aren't you mis-handling this field then, in that you
> clear it (as
> you never read the MSR)?
>
Yes, It will always be 0 of it in the previous commit. I shall move getting
"pre-defined BIOS value" for epp thingy ahead
>
> Jan
