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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.19 at 10:57, <puwen@xxxxxxxx> wrote:
> On 2019/3/15 19:18, Jan Beulich wrote:
>>>>> On 15.03.19 at 11:17, <puwen@xxxxxxxx> wrote:
>>> On 2019/3/15 1:11, Jan Beulich wrote:
>>>>>>> On 21.02.19 at 10:48, <puwen@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel