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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> 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?

Jan



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