Re: [PATCH v2 04/11] xen/amd: export processor max frequency value

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2025 08:32, Penny, Zheng wrote:
> I've written the following codes to let amd_log_freq() also adapt for 1a+
> ```
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -579,8 +579,7 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
>         unsigned int idx = 0, h;
>         uint64_t hi, lo, val;
> 
> -       if (c->x86 < 0x10 || c->x86 > 0x19 ||
> -           (c != &boot_cpu_data &&
> +       if (c->x86 < 0x10 || (c != &boot_cpu_data &&
>              (!opt_cpu_info || (c->apicid & (c->x86_num_siblings - 1)))))
>                 return;

On what basis do you drop the upper bound here altogether? Is there some
guarantee given somewhere that the MSR layout isn't going to change again?
You also want to pay attention to style (indentation here in particular)
when making such adjustments.

The conditional here also continues to mean the rest of the function won't
normally be executed for all CPUs. Maybe that was intentional, with the
goal of just adding Fam1A support here. Hard to tell without a patch
description.

> @@ -653,21 +652,23 @@ void amd_log_freq(const struct cpuinfo_x86 *c)
>                 wrmsrl(MSR_AMD64_NB_CFG, nbcfg);
>         }
> 
> +#define VALIDATE_FID(v) (c->x86 < 0x19 ? true : ((v & 0xfff) > 0x0f))

Please be sure to parenthesize macro arguments when used in expressions.
Not doing so violates at least one Misra rule. At the same time, seeing
how many parentheses there are already in e.g. FREQ(), please try to
avoid adding excess ones (here and there).

Also, if you add such validation, Wouldn't that be equally needed for e.g.
Fam19 (didn't check others)? Plus if you validate FID there, wouldn't it
be yet more important to validate the divisor, too? (So far we've gone
from the assumption that firmware will put sane values in when setting
PstateEn.)

>         lo = 0; /* gcc may not recognize the loop having at least 5 
> iterations */
>         for (h = c->x86 == 0x10 ? 5 : 8; h--; )
> -               if (!rdmsr_safe(0xC0010064 + h, lo) && (lo >> 63))
> -                       break;
> +               if (!rdmsr_safe(0xC0010064 + h, lo) && (lo >> 63) && 
> VALIDATE_FID(lo))
> +                               break;
>         if (!(lo >> 63))
>                 return;
> 
> -#define FREQ(v) (c->x86 < 0x17 ? ((((v) & 0x3f) + 0x10) * 100) >> (((v) >> 
> 6) & 7) \
> -                                    : (((v) & 0xff) * 25 * 8) / (((v) >> 8) 
> & 0x3f))
> +#define FREQ(v) (c->x86 > 0x19 ? ((v & 0xfff) * 5) :                         
>                     \
> +                               c->x86 < 0x17 ? ((((v) & 0x3f) + 0x10) * 100) 
> >> (((v) >> 6) & 7) \
> +                               : (((v) & 0xff) * 25 * 8) / (((v) >> 8) & 
> 0x3f))

This is getting unwieldy, I'm afraid. We may need to introduce a helper
function here. Or it would need re-wrapping / re-indentation to become
halfway readable again. Plus can we please arrange things so we handle
families in either consistently ascending order, or consistently
descending one?

Jan