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

[Pu Wen <puwen@xxxxxxxx>]

On 2019/3/19 21:02, Jan Beulich wrote:
> 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.
 
Yes, it's no need to add Hygon conditional to amd_get_topology() as >=0x17
conditionals are default supported.

-- 
Regards,
Pu Wen

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