RE: [PATCH v6 13/19] xen/x86: implement amd-cppc-epp driver for CPPC in active mode

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, July 17, 2025 9:36 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Anthony PERARD <anthony.perard@xxxxxxxxxx>;
> Orzel, Michal <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; 
> xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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