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

[Pu Wen <puwen@xxxxxxxx>]

On 2018/12/21 18:20, Andrew Cooper wrote:
....
>> +static unsigned int __initdata opt_cpuid_mask_l7s0_eax = ~0u;
>> +integer_param("cpuid_mask_l7s0_eax", opt_cpuid_mask_l7s0_eax);
>> +static unsigned int __initdata opt_cpuid_mask_l7s0_ebx = ~0u;
>> +integer_param("cpuid_mask_l7s0_ebx", opt_cpuid_mask_l7s0_ebx);
>
> These should be moved from the AMD specific code into the common cpu
> code (alongside the other masks) rather than duplicated here.

Thanks for the suggestion, will move them into the common CPU code in
next version patch.

...
>> +    asm volatile("1: rdmsr\n2:\n"
>> +                 ".section .fixup,\"ax\"\n"
>> +                 "3: movl %6,%2\n"
>> +                 "   jmp 2b\n"
>> +                 ".previous\n"
>> +                 _ASM_EXTABLE(1b, 3b)
>> +                 : "=a" (*lo), "=d" (*hi), "=r" (err)
>> +                 : "c" (msr), "D" (0x9c5a203a), "2" (0), "i" (-EFAULT));
>
> These rdmsr/wrmsr helpers with a password in %edi are only used in the
> K8 processors.  Since Hygon is a Zen derivative, you shouldn't need any
> of these.

Thanks for the correction. Will use the common functions rdmsr_safe and
wrmsr_safe instead, and remove the private helpers rdmsr/wrmsr_hygon_safe.

...
>> +    /* Hygon CPUs do not support SYSENTER outside of legacy mode. */
>> +    __clear_bit(X86_FEATURE_SEP, c->x86_capability);
>> +
>> +    /* Hygon processors have APIC timer running in deep C states. */
>> +    if ( opt_arat )
>> +            __set_bit(X86_FEATURE_ARAT, c->x86_capability);
>> +
>> +    if (cpu_has(c, X86_FEATURE_EFRO)) {
>> +            rdmsr(MSR_K7_HWCR, l, h);
>> +            l |= (1 << 27); /* Enable read-only APERF/MPERF bit */
>> +            wrmsr(MSR_K7_HWCR, l, h);
>> +    }
>
> Is there anything which is actually unique to Hygon here?  I ask,
> because this looks like a lot of duplicate code, considering that the
> processor base is the same.

Right now these codes are necessary for Hygon Dhyana processor even though
they are duplicated. As Hygon Dhyana support many CPU features such as ITSC
and EFRO, so I think the "if cpu_has" determine should be removed to make
the code clear and essential.

Keeping the codes into a separate compilation unit(hygon.c) at least has
two advantages:
1) Make the code flow more clear. Hygon is a new joint venture which has no
   historical old architectures, so I'm afraid that there are sufficient
   motivations to keep a clear new processor init flow.
2) Beneficial for the future maintaining. AMD and Hygon may maintain their
   respective architecture related codes with no interaction with each
   other.

For these reasons, we choose to keep the architecture initialization codes
in hygon.c.

Thx.

-- 
Regards,
Pu Wen


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