[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 2019/3/15 1:11, Jan Beulich wrote:
>>>> On 21.02.19 at 10:48, <puwen@xxxxxxxx> wrote:
>> As opt_cpuid_mask_l7s0_eax and opt_cpuid_mask_l7s0_ebx are used by both
>> AMD and Hygon, so move them to common.c.
> 
> I'm not convinced of this - it'll get in the way of introducing something
> like Linux'es CONFIG_CPU_SUP_*. Our command line parsing logic
> handles multiple instances of an option quite fine I think, so I'd prefer
> if these were kept static to both files. Or did Andrew ask you to do
> this?

Yes, Andrew asked that these should be moved from the AMD specific code into
the common cpu code (alongside the other masks) rather than duplicated here.

>> --- a/xen/arch/x86/cpu/Makefile
>> +++ b/xen/arch/x86/cpu/Makefile
>> @@ -8,4 +8,5 @@ obj-y += intel.o
>>   obj-y += intel_cacheinfo.o
>>   obj-y += mwait-idle.o
>>   obj-y += shanghai.o
>> +obj-y += hygon.o
>>   obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
> 
> Please insert at the alphabetically correct place.

OK.

...
>>   int intel_cpu_init(void);
>>   int amd_init_cpu(void);
>> +int hygon_init_cpu(void);
>>   int centaur_init_cpu(void);
>>   int shanghai_init_cpu(void);
> 
> Whereas here you should really put the new declaration at the end.

OK.

...
>> +/* Probe for the existance of the expected masking MSRs. */
> 
> Please don't drop the other part of the original comment.

OK. Will revert to the original one.

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

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

In fact in Linux we keep hygon.c separated from AMD even though there are
some duplicated codes at the moment. Andrew also agreed the logic that
following suit with Linux.

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

>> +    /* Hygon processors have APIC timer running in deep C states. */
>> +    if ( opt_arat )
> 
> Please don't copy the bad spaces inside the parentheses here.

OK. Thanks for the reminding.

-- 
Regards,
Pu Wen

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.