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 [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
