Re: [PATCH v4 02/15] xen/cpufreq: extract _PSD info from "struct xen_processor_performance"

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2025 09:40, Penny Zheng wrote:
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -191,9 +191,31 @@ int cpufreq_limit_change(unsigned int cpu)
>      return __cpufreq_set_policy(data, &policy);
>  }
>  
> -int cpufreq_add_cpu(unsigned int cpu)
> +static int get_psd_info(uint32_t init, unsigned int cpu,

The 1st parameter seems unnecessary: You access processor_pminfo[cpu]
below anyway, so you could as well use ...

> +                        uint32_t *shared_type,
> +                        struct xen_psd_package *domain_info)
>  {
>      int ret = 0;
> +
> +    switch ( init )

... processor_pminfo[cpu]->init here.

> +    {
> +    case XEN_PX_INIT:
> +        if ( shared_type )
> +            *shared_type = processor_pminfo[cpu]->perf.shared_type;
> +        if ( domain_info )
> +            *domain_info = processor_pminfo[cpu]->perf.domain_info;

Does this need to be a structure copy? Can't you hand back just a pointer,
with the function parameter being const struct xen_psd_package **?

> +        break;
> +    default:

Nit: Blank line please between non-fall-through case blocks.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -440,6 +440,11 @@ struct xen_psd_package {
>      uint64_t num_processors;
>  };
>  
> +/* Coordination type value */
> +#define XEN_CPUPERF_SHARED_TYPE_HW   1 /* HW does needed coordination */
> +#define XEN_CPUPERF_SHARED_TYPE_ALL  2 /* All dependent CPUs should set freq 
> */
> +#define XEN_CPUPERF_SHARED_TYPE_ANY  3 /* Freq can be set from any dependent 
> CPU */
> +
>  struct xen_processor_performance {
>      uint32_t flags;     /* flag for Px sub info type */
>      uint32_t platform_limit;  /* Platform limitation on freq usage */
> @@ -449,10 +454,7 @@ struct xen_processor_performance {
>      XEN_GUEST_HANDLE(xen_processor_px_t) states;
>      struct xen_psd_package domain_info;
>      /* Coordination type of this processor */
> -#define XEN_CPUPERF_SHARED_TYPE_HW   1 /* HW does needed coordination */
> -#define XEN_CPUPERF_SHARED_TYPE_ALL  2 /* All dependent CPUs should set freq 
> */
> -#define XEN_CPUPERF_SHARED_TYPE_ANY  3 /* Freq can be set from any dependent 
> CPU */
> -    uint32_t shared_type;
> +    uint32_t shared_type; /* XEN_CPUPERF_SHARED_TYPE_xxx */
>  };
>  typedef struct xen_processor_performance xen_processor_performance_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_processor_performance_t);

What's this movement about? In the public interface nothing changes?

Jan