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
