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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.03.19 at 13:33, <puwen@xxxxxxxx> wrote:
> On 2019/3/18 16:55, Jan Beulich wrote:
>> On 16.03.19 at 10:57, <puwen@xxxxxxxx> wrote:
>>> On 2019/3/15 19:18, Jan Beulich wrote:
>>>> On 15.03.19 at 11:17, <puwen@xxxxxxxx> wrote:
>>>>> On 2019/3/15 1:11, Jan Beulich wrote:
>>>>>> 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.
>>>> Just make non-static what needs to be, add an amd_ prefix, and
>>>> call it from your code.
>>> That's OK. With this method only init_levelling in amd.c should be added
>>> an amd_ prefix and called by hygon.c.
>>> But I'm afraid Hygon should have its own init functions and not call the
>>> AMD ones. The current Hygon init functions have been heavily stripped
>>> from the original AMD's.
>> Let me give you this rule of thumb (subject to discussion): If you can
>> safely re-use any non-trivial current AMD function with at most minor
>> adjustments (and irrespective of certain code there being unreachable
>> on Hygon), then I think it would be better to re-use it than to duplicate
>> it.
> 
> I tried, but it will add 0x18 to init_amd(). It's strange because AMD
> does not have family 18h now. So it becomes unwieldy to maintain
> init_amd() just at this time. The same situation for amd_get_topology().
> 
> So I think it's better to carve out init_hygon() and hygon_get_topology()
> (which also need Hygon's own adjustment for computing phys_proc_id).

Just to be clear: I've never objected to a separate init_hygon().
For amd_get_topology() it's less clear: It looks to me as if you
wouldn't need to add any Hygon conditionals to the function, as
long as cpu_has(c, X86_FEATURE_TOPOEXT) is true in your case.

Jan



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