Page MenuHomeFreeBSD

Add support for Hygon Dhyana Family 18h processor
ClosedPublic

Authored by puwen_hygon.cn on Jan 14 2020, 2:57 AM.

Details

Summary

As a new x86 CPU vendor, Chengdu Haiguang IC Design Co., Ltd (Hygon)
is a joint venture between AMD and Haiguang Information Technology Co.,
Ltd., aims at providing x86 processors for China server market.

The first generation Hygon processor(Dhyana) shares most architecture
with AMD's family 17h, but with different CPU vendor ID("HygonGenuine")
and PCI vendor ID(0x1d94) and family series number 18h(Hygon negotiated
with AMD to confirm that only Hygon use family 18h).

To enable Hygon Dhyana support in FreeBSD, add new definitions
HYGON_VENDOR_ID("HygonGenuine") and X86_VENDOR_HYGON(0x1d94) to identify
Hygon Dhyana CPU.

Initialize the CPU features(topology, local APIC ext, MSI, TSC, hwpstate,
MCA, DEBUG_CTL, etc) for amd64 and i386 mode by sharing the code path of
AMD family 17h.

The changes have been applied on FreeBSD 13.0-CURRENT and tested
successfully on Hygon Dhyana processor.

References:
[1] Linux kernel patches for Hygon Dhyana, merged in 4.20:

https://git.kernel.org/tip/c9661c1e80b609cd038db7c908e061f0535804ef

[2] MSR and CPUID definition:

https://www.amd.com/system/files/TechDocs/54945_PPR_Family_17h_Models_00h-0Fh.pdf

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

puwen_hygon.cn created this revision.Jan 14 2020, 2:57 AM
imp accepted this revision.Jan 14 2020, 5:53 AM

src/stand looks good for sure. I eyeballed the rest and didn't see problems

This revision is now accepted and ready to land.Jan 14 2020, 5:53 AM
cem added a comment.Jan 14 2020, 6:00 AM

Is this chip something we will be able to obtain outside of China? Will hardware documentation be published in English, other than that provided by AMD for its not-quite-the-same-hardware? What plans does Hygon have going forward? Will they continue to just license future AMD designs or will there be any differentiation?

It seems that Hygon is just a licensed AMD Zen core with zero differentiation. Why not just set cpu_vendor_id to CPU_VENDOR_AMD if Hygon cpuid string is detected? That way we can avoid rototilling almost all of this code by treating hygon / family 0x18 as a weird AMD (which it is).

My concern is that this adds maintenance burden to the AMD MD codebase for an obscure chip no one actually has, uses, or has documentation for.

In D23163#507790, @imp wrote:

src/stand looks good for sure. I eyeballed the rest and didn't see problems

Thanks a lot for the review.

In D23163#507793, @cem wrote:

Is this chip something we will be able to obtain outside of China? Will hardware documentation be published in English, other than that provided by AMD for its not-quite-the-same-hardware? What plans does Hygon have going forward? Will they continue to just license future AMD designs or will there be any differentiation?
It seems that Hygon is just a licensed AMD Zen core with zero differentiation. Why not just set cpu_vendor_id to CPU_VENDOR_AMD if Hygon cpuid string is detected? That way we can avoid rototilling almost all of this code by treating hygon / family 0x18 as a weird AMD (which it is).
My concern is that this adds maintenance burden to the AMD MD codebase for an obscure chip no one actually has, uses, or has documentation for.

Hi cem:
Thanks for the quick response.
Of course, Hygon's CPU documentation is written in English and will be published when it's publicly available.
Although Hygon Dhyana's technology is licensed from AMD, but we are not the same with AMD Zen core. As shown in [1], we already have multi-die products which is different from AMD existing products. Based on existing licensed technology, we will continue to develop our products for our customer which has different market requirements. Surely, we will have our different design. If we direct use CPU_VENDOR_AMD in cpu_vendor_id, which might cause mis-understanding and make CPU support codes more obscure.
We understand the worry of code maintenance burden, and we hope to find a clean solution to support FreeBSD. As a different CPU vendor, based on other community experience, we assume the technology of using different CPU vendor ID and family to separate code path, balancing between code reuse and code separation might be a better solution.
For the product adoption and usage amount, we are not worried. Actually, we already got enormous FreeBSD-based solution (FreeBSD/FreeNAS…) support requirements from our customers, that's why we are eager to get involved and get supported in FreeBSD community.

Reference:
[1] x86/CPU/hygon: Fix phys_proc_id calculation logic for multi-die processors:

https://git.kernel.org/tip/e0ceeae708cebf22c990c3d703a4ca187dc837f5
markj added a reviewer: kib.Jan 14 2020, 3:17 PM
kib added inline comments.Jan 14 2020, 3:48 PM
sys/amd64/amd64/initcpu.c
174 ↗(On Diff #66716)

Do you have the same bug as amd ?

sys/i386/i386/machdep.c
1612 ↗(On Diff #66716)

I do not think there is a Hygon' cpu with family != 6. In other words, I believe you need to or Hygon with old condition.

sys/x86/cpufreq/hwpstate.c
450 ↗(On Diff #66716)

Did AMD promised to not release family 0x18, to not collide with Hygon ? If not, then this case should be more specific.

sys/x86/x86/mca.c
1210 ↗(On Diff #66716)

I would suggest to not bother patching this place.

sys/x86/x86/mp_x86.c
519 ↗(On Diff #66716)

The indent is wrong, it should be +4 spaces for the continuation line.

sys/i386/i386/machdep.c
1612 ↗(On Diff #66716)

Yes, you are correct, will modify related codes.

sys/x86/cpufreq/hwpstate.c
450 ↗(On Diff #66716)

Yes. Hygon already negotiated with AMD to confirm that only Hygon will use family 0x18, new AMD CPU already use family 0x19 [1].
[1] http://lkml.iu.edu/hypermail/linux/kernel/2001.1/02392.html

sys/x86/x86/mca.c
1210 ↗(On Diff #66716)

If not patch here, there will be "Starting AMD thresholding on bank x" in the dmesg log, which we expect is "Starting Hygon thresholding on bank x".

sys/x86/x86/mp_x86.c
519 ↗(On Diff #66716)

Thanks, will correct it.

cem added a comment.Jan 15 2020, 5:48 PM
In D23163#507793, @cem wrote:

Is this chip something we will be able to obtain outside of China? Will hardware documentation be published in English, other than that provided by AMD for its not-quite-the-same-hardware? What plans does Hygon have going forward? Will they continue to just license future AMD designs or will there be any differentiation?
...
My concern is that this adds maintenance burden to the AMD MD codebase for an obscure chip no one actually has, uses, or has documentation for.

Of course, Hygon's CPU documentation is written in English and will be published when it's publicly available.

When will it be publicly available? Can people buy this chip outside of China? Will there be a next-generation Hygon core and will it need entirely different architectural support than AMD?

sys/x86/x86/mca.c
1210 ↗(On Diff #66716)

The function is amd_thresholding_monitor and here "AMD" is in contrast to the "Intel." AMD thresholding is an accurate characterization out of the (debug) output string. There's no need to key a string off of CPU_VENDOR for this function, which has the same behavior on AMD and AMD clones like Hygon.

puwen_hygon.cn added inline comments.Jan 17 2020, 9:44 AM
sys/x86/x86/mca.c
1210 ↗(On Diff #66716)

OK, will revert the change here.

sys/amd64/amd64/initcpu.c
174 ↗(On Diff #66716)

We confirmed that Hygon Dhyana CPU has the same bug as AMD, so the 0x18 condition should be added here.

Or Hygon with the old condition in db_show_sysregs.

Revert the change in the function amd_thresholding_monitor().

Correct the indent to +4 spaces for Hygon condition in the function topo_probe().

This revision now requires review to proceed.Jan 20 2020, 12:40 PM
kib accepted this revision.Jan 20 2020, 3:52 PM

Do you want me to commit this now ?

This revision is now accepted and ready to land.Jan 20 2020, 3:52 PM
In D23163#510442, @kib wrote:

Do you want me to commit this now ?

Yes, thanks a lot.

This revision was automatically updated to reflect the committed changes.