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

[Pu Wen <puwen@xxxxxxxx>]

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.

>> There are many codes to support AMD's
>> different families, the family/model/stepping checking are deeply embedded
>> in codes. If we add Hygon family/model/stepping checking in future version,
>> it will make code interleaved together and hard to maintain.
> 
> We can think about adding the duplication when it becomes
> unwieldy to maintain the shared variant.
> 
>>>> +static void init_hygon(struct cpuinfo_x86 *c)
>>>> +{
>>>> +  u32 l, h;
>>>> +  unsigned long long value;
>>>> +
>>>> +  /* Attempt to set lfence to be Dispatch Serialising. */
>>>> +  if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
>>>> +          /* Unable to read.  Assume the safer default. */
>>>> +          __clear_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability);
>>>> +  else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
>>>> +          /* Already dispatch serialising. */
>>>> +          __set_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability);
>>>
>>> Didn't you cut off too much from the original code (which again
>>> may better be shared, by splitting out into a function)?
>>
>> The cut codes are used for other AMD families except family 17h. Hygon
>> Dhyana is derived from AMD Zen, so we only need the family 17h parts.
>> This also make the init flow minimal.
> 
> How is
> 
>               else if (wrmsr_safe(MSR_AMD64_DE_CFG,
>                                   value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
>                        rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
>                        !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
>                       /* Attempt to set failed.  Assume the safer default. */
>                       __clear_bit(X86_FEATURE_LFENCE_DISPATCH,
>                                   c->x86_capability);
>               else
>                       /* Successfully enabled! */
>                       __set_bit(X86_FEATURE_LFENCE_DISPATCH,
>                                 c->x86_capability);
> 
> family dependent in any way?

Independent here.

-- 
Regards,
Pu Wen

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