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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/12/2018 11:42, Pu Wen wrote:
> On 2018/12/21 18:20, Andrew Cooper wrote:
> ....
>>> +   /* 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.

The most important question here is how likely is it to diverge in the
future?

Where possible, duplicate code should be kept to a minimum, because of
the risk of it being modified in only one of the places.

If Hygon is expected to diverge substantially in the future, then
perhaps the duplication is fine.  If Hygon is unlikely to diverge far
from Zen (particularly if you intend to use newer Zen cores as new Hygon
bases), then perhaps it would be worth making a common amd_base.c file,
and restrict amd.c and hygon.c to unique features.

~Andrew

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