Re: [PATCH v2 01/17] x86/cpu: create Dhyana init file and register new cpu_dev to system

[Pu Wen]

On 2018-07-29 07:42, Paolo Bonzini wrote:
> If the maintainers are okay with X86_FEATURE_HYGON that's certainly
> fine, however I think you can improve the consistency of the patches in
> a few ways.

Thanks for your suggestion.
To improve code consistency , will rework the patches.

> Lack of SME/SEV is not an issue, since there are AMD Zen chips without
> SME/SEV too, but potential incompatibility with future AMD fam18h chips
> is important.  Therefore, code like this one in amd_uncore_init
>
>
> -	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD &&
> +	    boot_cpu_data.x86_vendor != X86_VENDOR_HYGON)
> return -ENODEV;
>
> if (!boot_cpu_has(X86_FEATURE_TOPOEXT))
> return -ENODEV;
>
> -	if (boot_cpu_data.x86 == 0x17) {
> +	if (boot_cpu_data.x86 == 0x17 || boot_cpu_data.x86 == 0x18) {
>
> should check explicitly for Hygon before checking for family 18h.  The
> same applies to the edac patch that I've just sent an answer to.

For the family number, As a JV company, to keep the consistency usage of
CPU family convention, Hygon will negotiate with AMD to make sure the CPU
family numbers both company used will not overlap. So as Hygon will use
the family 18h for Dhyana, AMD will skip the family 18h and directly use
family 19h for its new product.

Based on this assumption, this patch set direct check the family number
for 18h to see if it is Hygon processor to create a minimal patch set.

For the consistency, will modify the codes as follows:
-       if (boot_cpu_data.x86 == 0x17) {
+       if (boot_cpu_data.x86 == 0x17 ||
+          (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON &&
+           boot_cpu_data.x86 == 0x18)) {

> On the other hand, in many cases the AMD code is testing something like
> "AMD && family >= 0x0f".  In this case you have a mix of:
>
> - duplicate code for HYGON, such as modern_apic or mce_is_correctable
>
> - change the condition to (AMD || HYGON) && family >= 0x0f, such as
> k8_check_syscfg_dram_mod_en and amd_special_default_mtrr
>
> - change the condition to (AMD && family >= 0x0f) || (HYGON && family >=
> 0x18), such as smp_quirk_init_udelay
>
> I couldn't find any case where you used (AMD && family >= 0x0f) ||
> HYGON, but I think it would be clearer in most cases than all the above
> three alternatives.

Your suggestion is correct, will try to make the code more consistent and
update the next patch set to use (AMD && family >= 0x0f) || HYGON.

> In general, I would duplicate code if and only if the AMD code is a maze
> of if/elseif/elseif.  In particular, code like this
>
> 	case X86_VENDOR_AMD:
> if (msr >= MSR_F15H_PERF_CTL)
> return (msr - MSR_F15H_PERF_CTL) >> 1;
> return msr - MSR_K7_EVNTSEL0;
> +	case X86_VENDOR_HYGON:
> +	if (msr >= MSR_F15H_PERF_CTL)
> +	return (msr - MSR_F15H_PERF_CTL) >> 1;
> +	return msr - MSR_K7_EVNTSEL0;

> or this
>
> 	case X86_VENDOR_AMD:
> rdmsr_on_cpu(cpu, MSR_K7_HWCR, &lo, &hi);
> msr = lo | ((u64)hi << 32);
> return !(msr & MSR_K7_HWCR_CPB_DIS);
> +	case X86_VENDOR_HYGON:
> +	rdmsr_on_cpu(cpu, MSR_K7_HWCR, &lo, &hi);
> +	msr = lo | ((u64)hi << 32);
> +	return !(msr & MSR_K7_HWCR_CPB_DIS);
>
> looks a bit silly, and you already have several cases when you are not
> introducing duplication (e.g. __mcheck_cpu_init_early).  On the other
> hand, code like xen_pmu_arch_init can be very simple for Hygon and so it
> may be useful to have a separate branch.

Thanks for the suggestion, will change this by directly reusing condition
check if reused codes are direct:
+	case X86_VENDOR_HYGON:
	case X86_VENDOR_AMD:
		if (msr >= MSR_F15H_PERF_CTR)
			return (msr - MSR_F15H_PERF_CTR) >> 1;
		return msr - MSR_K7_PERFCTR0;

Also will branch codes for Hygon in case of complicated checking condition
such as in xen_pmu_arch_init().

Thanks,
Pu Wen