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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/12/2018 15:46, Pu Wen wrote:
> On 2018/12/28 5:11, Andrew Cooper wrote:
>> On 26/12/2018 11:42, Pu Wen wrote:
>>> On 2018/12/21 18:20, Andrew Cooper wrote:
>>>> 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.
> Yes, we are trying our best to make the duplicated code minimum but
> essential for Hygon Dhyana processor.
>
>> 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.
> Your point is correct, for future version, Hygon CPU will diverge from
> AMD Zen and do its own modification. So we lay the ground for the future
> code maintenance.
>
> Actually we have discussed the same topic with the linux community[1],
> and finally reached some agreement that to keep hygon.c separated from
> AMD even though there are some duplicated codes at the moment[2].
>
> We understand the difficulty to find a balance point between sharing
> codes and maintenance effort.
> Any suggestion is welcome.

If we think this is going to be the extent of the duplication, then lets
follow suit with Linux.

That said, I may want to rethink the "xen/amd: Support for guest
MSR_VIRT_SPEC_CTRL support" series as Hygon needs this logic as well.

~Andrew

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