Re: [PATCH v2 01/14] x86/cpu: Create Hygon Dhyana architecture support file

From: "Jan Beulich" <JBeulich@suse.com>
To: Pu Wen <puwen@hygon.cn>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v2 01/14] x86/cpu: Create Hygon Dhyana architecture support file
Date: Mon, 18 Mar 2019 02:54:44 -0600	[thread overview]
Message-ID: <5C8F5CD4020000780021FB77@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <f723e493-e986-432e-0b4a-e175f0df41c7@hygon.cn>

>>> On 16.03.19 at 10:57, <puwen@hygon.cn> wrote:
> On 2019/3/15 19:18, Jan Beulich wrote:
>>>>> On 15.03.19 at 11:17, <puwen@hygon.cn> wrote:
>>> On 2019/3/15 1:11, Jan Beulich wrote:
>>>>>>> On 21.02.19 at 10:48, <puwen@hygon.cn> wrote:
>>>>> +static void __init noinline probe_masking_msrs(void)
>>>>> +{
>>>>> +	const struct cpuinfo_x86 *c = &boot_cpu_data;
>>>>> +
>>>>> +	/* Work out which masking MSRs we should have. */
>>>>> +	cpuidmask_defaults._1cd =
>>>>> +		_probe_mask_msr(MSR_K8_FEATURE_MASK, LCAP_1cd);
>>>>> +	cpuidmask_defaults.e1cd =
>>>>> +		_probe_mask_msr(MSR_K8_EXT_FEATURE_MASK, LCAP_e1cd);
>>>>> +	if (c->cpuid_level >= 7)
>>>>> +		cpuidmask_defaults._7ab0 =
>>>>> +			_probe_mask_msr(MSR_AMD_L7S0_FEATURE_MASK, LCAP_7ab0);
>>>>
>>>> There's more relevant code here in the original function.
>>>
>>> The code is used for family 15h. Hygon CPU do not need it.
>> 
>> There's a single Fam15 conditional in the middle of the function.
>> Everything beyond that is again family-independent.
>> 
>>>>> +	if (opt_cpu_info) {
>>>>> +		printk(XENLOG_INFO "Levelling caps: %#x\n", levelling_caps);
>>>>> +		printk(XENLOG_INFO
>>>>> +		       "MSR defaults: 1d 0x%08x, 1c 0x%08x, e1d 0x%08x, "
>>>>> +		       "e1c 0x%08x, 7a0 0x%08x, 7b0 0x%08x\n",
>>>>> +		       (uint32_t)cpuidmask_defaults._1cd,
>>>>> +		       (uint32_t)(cpuidmask_defaults._1cd >> 32),
>>>>> +		       (uint32_t)cpuidmask_defaults.e1cd,
>>>>> +		       (uint32_t)(cpuidmask_defaults.e1cd >> 32),
>>>>> +		       (uint32_t)(cpuidmask_defaults._7ab0 >> 32),
>>>>> +		       (uint32_t)cpuidmask_defaults._7ab0);
>>>>> +	}
>>>>> +
>>>>> +	if (levelling_caps)
>>>>> +		ctxt_switch_masking = hygon_ctxt_switch_masking;
>>>>> +}
>>>>
>>>> This is a lot of duplicated code with only minor differences. I think
>>>> you would be better off calling into the AMD original functions.
>>>
>>> These functions and AMD original ones are static. So Hygon cannot directly
>>> call into them. Or we can put them into the common cpu code, but I think
>>> it's not good for future maintenance.
>> 
>> Just make non-static what needs to be, add an amd_ prefix, and
>> call it from your code.
> 
> That's OK. With this method only init_levelling in amd.c should be added
> an amd_ prefix and called by hygon.c.
> 
> But I'm afraid Hygon should have its own init functions and not call the
> AMD ones. The current Hygon init functions have been heavily stripped
> from the original AMD's.

Let me give you this rule of thumb (subject to discussion): If you can
safely re-use any non-trivial current AMD function with at most minor
adjustments (and irrespective of certain code there being unreachable
on Hygon), then I think it would be better to re-use it than to duplicate
it.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel