| Message ID | 14dcae729e8d2ed9ba54565a61e027c83b9df529.1553520193.git.puwen@hygon.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Hygon Dhyana Family 18h processor | expand |
>>> On 25.03.19 at 14:29, <puwen@hygon.cn> wrote: > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -32,11 +32,6 @@ > static char __initdata opt_famrev[14]; > string_param("cpuid_mask_cpu", opt_famrev); > > -static unsigned int __initdata opt_cpuid_mask_l7s0_eax = ~0u; > -integer_param("cpuid_mask_l7s0_eax", opt_cpuid_mask_l7s0_eax); > -static unsigned int __initdata opt_cpuid_mask_l7s0_ebx = ~0u; > -integer_param("cpuid_mask_l7s0_ebx", opt_cpuid_mask_l7s0_ebx); This is no longer needed - all references are now local to amd.c. > @@ -116,6 +121,9 @@ bool __init probe_cpuid_faulting(void) > uint64_t val; > int rc; > > + if(boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) > + return false; > + > if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0) Isn't this similarly true for AMD, in which case adding both at the same time in a separate patch would seem better? Yet then again - did you look at the description of the commit moving the function here (6e2fdc0f89 ["x86: Common cpuid faulting support"])? Hence if anything this would need qualifying by !cpu_has_hypervisor. Also the contextual if() tells you that there's a blank missing in the one you add. However, there's a wider style question to raise: This file is not a Linux clone, so generally I'd expect it to be written in plain Xen style. > +static void early_init_hygon(struct cpuinfo_x86 *c) > +{ > + early_init_amd(c); > +} Why the wrapper function? It can be introduced once you actually need to do more than just the call. > +static void init_hygon(struct cpuinfo_x86 *c) > +{ > + u32 l, h; As mentioned before, please prefer uint32_t et al over u32 and friends. While that's applicable to the entire series (and then also to use basic types in preference to the fixed width ones, where possible), in this particular case it would be even better if you dropped the variables altogether, using ... > + unsigned long long value; > + > + /* Attempt to set LFENCE to be Dispatch Serialising. */ > + if (rdmsr_safe(MSR_AMD64_DE_CFG, value)) > + /* Unable to read. Assume the safer default. */ > + __clear_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability); > + if (value & AMD64_DE_CFG_LFENCE_SERIALISE) > + /* Dispatch Serialising. */ > + __set_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability); > + > + /* > + * If the user has explicitly chosen to disable Memory Disambiguation > + * to mitigiate Speculative Store Bypass, poke the appropriate MSR. > + */ > + if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) { > + value |= 1ull << 10; > + wrmsr_safe(MSR_AMD64_LS_CFG, value); > + } > + > + display_cacheinfo(c); > + > + if (cpu_has(c, X86_FEATURE_ITSC)) { > + __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability); > + __set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability); > + __set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability); > + } > + > + c->x86_max_cores = (cpuid_ecx(0x80000008) & 0xff) + 1; > + > + hygon_get_topology(c); > + > + /* Hygon CPUs do not support SYSENTER outside of legacy mode. */ > + __clear_bit(X86_FEATURE_SEP, c->x86_capability); > + > + /* Hygon processors have APIC timer running in deep C states. */ > + if (opt_arat) > + __set_bit(X86_FEATURE_ARAT, c->x86_capability); > + > + if (cpu_has(c, X86_FEATURE_EFRO)) { > + rdmsr(MSR_K7_HWCR, l, h); > + l |= (1 << 27); /* Enable read-only APERF/MPERF bit */ > + wrmsr(MSR_K7_HWCR, l, h); > + } ... "value" and rdmsrl() / wrmsrl() here instead. Jan
On 2019/3/26 23:49, Jan Beulich wrote: > On 25.03.19 at 14:29, <puwen@hygon.cn> wrote: >> -static unsigned int __initdata opt_cpuid_mask_l7s0_eax = ~0u; >> -integer_param("cpuid_mask_l7s0_eax", opt_cpuid_mask_l7s0_eax); >> -static unsigned int __initdata opt_cpuid_mask_l7s0_ebx = ~0u; >> -integer_param("cpuid_mask_l7s0_ebx", opt_cpuid_mask_l7s0_ebx); > > This is no longer needed - all references are now local to amd.c. Okay, will put them back to amd.c. >> @@ -116,6 +121,9 @@ bool __init probe_cpuid_faulting(void) >> uint64_t val; >> int rc; >> >> + if(boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) >> + return false; >> + >> if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0) > > Isn't this similarly true for AMD, in which case adding both at the There is no MSR_INTEL_PLATFORM_INFO for AMD family 17h and Hygon family 18h. Reading this MSR will stall on Hygon system. I don't know if it would successfully returned when reading it on AMD system. > same time in a separate patch would seem better? Yet then again - In a separate patch is fine. > did you look at the description of the commit moving the function > here (6e2fdc0f89 ["x86: Common cpuid faulting support"])? Hence > if anything this would need qualifying by !cpu_has_hypervisor. Then it would be like this: if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON && !cpu_has_hypervisor) return false; > Also the contextual if() tells you that there's a blank missing in the > one you add. However, there's a wider style question to raise: > This file is not a Linux clone, so generally I'd expect it to be > written in plain Xen style. I'm sorry for the missing blank. Okay, will use the right style. >> +static void early_init_hygon(struct cpuinfo_x86 *c) >> +{ >> + early_init_amd(c); >> +} > > Why the wrapper function? It can be introduced once you actually To keep the functions uniform Hygon ones in hygon_cpu_dev. > need to do more than just the call. Okay, will remove the wrapper and directly call early_init_amd(). >> +static void init_hygon(struct cpuinfo_x86 *c) >> +{ >> + u32 l, h; > > As mentioned before, please prefer uint32_t et al over u32 and > friends. While that's applicable to the entire series (and then > also to use basic types in preference to the fixed width ones, Okay. > where possible), in this particular case it would be even better if > you dropped the variables altogether, using ... >> + unsigned long long value; ... >> + if (cpu_has(c, X86_FEATURE_EFRO)) { >> + rdmsr(MSR_K7_HWCR, l, h); >> + l |= (1 << 27); /* Enable read-only APERF/MPERF bit */ >> + wrmsr(MSR_K7_HWCR, l, h); >> + } > > ... "value" and rdmsrl() / wrmsrl() here instead. Will use rdmsrl()/wrmsrl() instead.
>>> On 27.03.19 at 09:14, <puwen@hygon.cn> wrote: > On 2019/3/26 23:49, Jan Beulich wrote: >> On 25.03.19 at 14:29, <puwen@hygon.cn> wrote: >>> @@ -116,6 +121,9 @@ bool __init probe_cpuid_faulting(void) >>> uint64_t val; >>> int rc; >>> >>> + if(boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) >>> + return false; >>> + >>> if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0) >> >> Isn't this similarly true for AMD, in which case adding both at the > > There is no MSR_INTEL_PLATFORM_INFO for AMD family 17h and Hygon family > 18h. Reading this MSR will stall on Hygon system. I don't know if it > would successfully returned when reading it on AMD system. What do you mean by "stall"? Reading an unimplemented MSR should produce #GP(0). >> same time in a separate patch would seem better? Yet then again - > > In a separate patch is fine. > >> did you look at the description of the commit moving the function >> here (6e2fdc0f89 ["x86: Common cpuid faulting support"])? Hence >> if anything this would need qualifying by !cpu_has_hypervisor. > > Then it would be like this: > if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON && > !cpu_has_hypervisor) > return false; Right, plus perhaps said AMD addition, unless Andrew objects to it for some reason. Jan
On 2019/3/27 16:31, Jan Beulich wrote: > On 27.03.19 at 09:14, <puwen@hygon.cn> wrote: >> On 2019/3/26 23:49, Jan Beulich wrote: >>> On 25.03.19 at 14:29, <puwen@hygon.cn> wrote: >>>> @@ -116,6 +121,9 @@ bool __init probe_cpuid_faulting(void) >>>> uint64_t val; >>>> int rc; >>>> >>>> + if(boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) >>>> + return false; >>>> + >>>> if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0) >>> >>> Isn't this similarly true for AMD, in which case adding both at the >> >> There is no MSR_INTEL_PLATFORM_INFO for AMD family 17h and Hygon family >> 18h. Reading this MSR will stall on Hygon system. I don't know if it >> would successfully returned when reading it on AMD system. > > What do you mean by "stall"? Reading an unimplemented MSR > should produce #GP(0). On certain old Hygon system there is no #GP produced. And the Xen initialization process is stopped. Beyond that it will indeed produce: "traps.c:1574: GPF (0000)" and return false from the last if() conditional. >>> same time in a separate patch would seem better? Yet then again - >> >> In a separate patch is fine. >> >>> did you look at the description of the commit moving the function >>> here (6e2fdc0f89 ["x86: Common cpuid faulting support"])? Hence >>> if anything this would need qualifying by !cpu_has_hypervisor. >> >> Then it would be like this: >> if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON && >> !cpu_has_hypervisor) >> return false; > > Right, plus perhaps said AMD addition, unless Andrew objects to it > for some reason. Then it would be like this: if ((boot_cpu_data.x86_vendor == X86_VENDOR_HYGON || boot_cpu_data.x86_vendor == X86_VENDOR_AMD) && !cpu_has_hypervisor) return false; Andrew, any objections?
diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile index 34a01ca..466acc8 100644 --- a/xen/arch/x86/cpu/Makefile +++ b/xen/arch/x86/cpu/Makefile @@ -4,6 +4,7 @@ subdir-y += mtrr obj-y += amd.o obj-y += centaur.o obj-y += common.o +obj-y += hygon.o obj-y += intel.o obj-y += intel_cacheinfo.o obj-y += mwait-idle.o diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index c790416..812d54d 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -32,11 +32,6 @@ static char __initdata opt_famrev[14]; string_param("cpuid_mask_cpu", opt_famrev); -static unsigned int __initdata opt_cpuid_mask_l7s0_eax = ~0u; -integer_param("cpuid_mask_l7s0_eax", opt_cpuid_mask_l7s0_eax); -static unsigned int __initdata opt_cpuid_mask_l7s0_ebx = ~0u; -integer_param("cpuid_mask_l7s0_ebx", opt_cpuid_mask_l7s0_ebx); - static unsigned int __initdata opt_cpuid_mask_thermal_ecx = ~0u; integer_param("cpuid_mask_thermal_ecx", opt_cpuid_mask_thermal_ecx); @@ -526,7 +521,7 @@ static void amd_get_topology(struct cpuinfo_x86 *c) : c->cpu_core_id); } -static void early_init_amd(struct cpuinfo_x86 *c) +void early_init_amd(struct cpuinfo_x86 *c) { if (c == &boot_cpu_data) amd_init_levelling(); diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 53bb0a9..9fb75dd 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -36,6 +36,11 @@ integer_param("cpuid_mask_ext_ecx", opt_cpuid_mask_ext_ecx); unsigned int opt_cpuid_mask_ext_edx = ~0u; integer_param("cpuid_mask_ext_edx", opt_cpuid_mask_ext_edx); +unsigned int opt_cpuid_mask_l7s0_eax = ~0u; +integer_param("cpuid_mask_l7s0_eax", opt_cpuid_mask_l7s0_eax); +unsigned int opt_cpuid_mask_l7s0_ebx = ~0u; +integer_param("cpuid_mask_l7s0_ebx", opt_cpuid_mask_l7s0_ebx); + unsigned int __initdata expected_levelling_cap; unsigned int __read_mostly levelling_caps; @@ -116,6 +121,9 @@ bool __init probe_cpuid_faulting(void) uint64_t val; int rc; + if(boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) + return false; + if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0) raw_msr_policy.plaform_info.cpuid_faulting = val & MSR_PLATFORM_INFO_CPUID_FAULTING; @@ -710,6 +718,7 @@ void __init early_cpu_init(void) amd_init_cpu(); centaur_init_cpu(); shanghai_init_cpu(); + hygon_init_cpu(); early_cpu_detect(); } diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h index 2fcb931..971077a 100644 --- a/xen/arch/x86/cpu/cpu.h +++ b/xen/arch/x86/cpu/cpu.h @@ -13,11 +13,15 @@ extern bool_t opt_arat; extern unsigned int opt_cpuid_mask_ecx, opt_cpuid_mask_edx; extern unsigned int opt_cpuid_mask_xsave_eax; extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx; +extern unsigned int opt_cpuid_mask_l7s0_eax, opt_cpuid_mask_l7s0_ebx; extern int get_model_name(struct cpuinfo_x86 *c); extern void display_cacheinfo(struct cpuinfo_x86 *c); +void early_init_amd(struct cpuinfo_x86 *c); + int intel_cpu_init(void); int amd_init_cpu(void); int centaur_init_cpu(void); int shanghai_init_cpu(void); +int hygon_init_cpu(void); diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c new file mode 100644 index 0000000..bbe13c5 --- /dev/null +++ b/xen/arch/x86/cpu/hygon.c @@ -0,0 +1,95 @@ +#include <xen/init.h> +#include <asm/processor.h> +#include <asm/hvm/support.h> +#include <asm/spec_ctrl.h> + +#include "cpu.h" + +#define APICID_SOCKET_ID_BIT 6 + +static void hygon_get_topology(struct cpuinfo_x86 *c) +{ + u32 ebx; + + if (c->x86_max_cores <= 1) + return; + + /* Socket ID is ApicId[6] for Hygon processors. */ + c->phys_proc_id >>= APICID_SOCKET_ID_BIT; + + ebx = cpuid_ebx(0x8000001e); + c->x86_num_siblings = ((ebx >> 8) & 0x3) + 1; + c->x86_max_cores /= c->x86_num_siblings; + c->cpu_core_id = ebx & 0xff; + + if (opt_cpu_info) + printk("CPU %d(%d) -> Processor %d, Core %d\n", + smp_processor_id(), c->x86_max_cores, + c->phys_proc_id, c->cpu_core_id); +} + +static void early_init_hygon(struct cpuinfo_x86 *c) +{ + early_init_amd(c); +} + +static void init_hygon(struct cpuinfo_x86 *c) +{ + u32 l, h; + unsigned long long value; + + /* Attempt to set LFENCE to be Dispatch Serialising. */ + if (rdmsr_safe(MSR_AMD64_DE_CFG, value)) + /* Unable to read. Assume the safer default. */ + __clear_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability); + if (value & AMD64_DE_CFG_LFENCE_SERIALISE) + /* Dispatch Serialising. */ + __set_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability); + + /* + * If the user has explicitly chosen to disable Memory Disambiguation + * to mitigiate Speculative Store Bypass, poke the appropriate MSR. + */ + if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) { + value |= 1ull << 10; + wrmsr_safe(MSR_AMD64_LS_CFG, value); + } + + display_cacheinfo(c); + + if (cpu_has(c, X86_FEATURE_ITSC)) { + __set_bit(X86_FEATURE_CONSTANT_TSC, c->x86_capability); + __set_bit(X86_FEATURE_NONSTOP_TSC, c->x86_capability); + __set_bit(X86_FEATURE_TSC_RELIABLE, c->x86_capability); + } + + c->x86_max_cores = (cpuid_ecx(0x80000008) & 0xff) + 1; + + hygon_get_topology(c); + + /* Hygon CPUs do not support SYSENTER outside of legacy mode. */ + __clear_bit(X86_FEATURE_SEP, c->x86_capability); + + /* Hygon processors have APIC timer running in deep C states. */ + if (opt_arat) + __set_bit(X86_FEATURE_ARAT, c->x86_capability); + + if (cpu_has(c, X86_FEATURE_EFRO)) { + rdmsr(MSR_K7_HWCR, l, h); + l |= (1 << 27); /* Enable read-only APERF/MPERF bit */ + wrmsr(MSR_K7_HWCR, l, h); + } +} + +static const struct cpu_dev hygon_cpu_dev = { + .c_vendor = "Hygon", + .c_ident = { "HygonGenuine" }, + .c_early_init = early_init_hygon, + .c_init = init_hygon, +}; + +int __init hygon_init_cpu(void) +{ + cpu_devs[X86_VENDOR_HYGON] = &hygon_cpu_dev; + return 0; +} diff --git a/xen/include/asm-x86/x86-vendors.h b/xen/include/asm-x86/x86-vendors.h index 38a81c3..fa1cbb4 100644 --- a/xen/include/asm-x86/x86-vendors.h +++ b/xen/include/asm-x86/x86-vendors.h @@ -9,6 +9,7 @@ #define X86_VENDOR_AMD 2 #define X86_VENDOR_CENTAUR 3 #define X86_VENDOR_SHANGHAI 4 -#define X86_VENDOR_NUM 5 +#define X86_VENDOR_HYGON 5 +#define X86_VENDOR_NUM 6 #endif /* __XEN_X86_VENDORS_H__ */
Add x86 architecture support for a new processor: Hygon Dhyana Family 18h. To make Hygon initialization flow more clear, carve out code from amd.c into a separate file hygon.c, and remove unnecessary code for Hygon Dhyana. To identify Hygon Dhyana CPU, add a new vendor type X86_VENDOR_HYGON for system recognition. Hygon can fully use the function early_init_amd(), so make this common function non-static and direct call it from Hygon code. As opt_cpuid_mask_l7s0_eax and opt_cpuid_mask_l7s0_ebx are used by both AMD and Hygon, so move them to common.c. Hygon Dhyana has no CPUID faulting, so directly return false in the function probe_cpuid_faulting(). Add a separate hygon_get_topology(), which calculate phys_proc_id from AcpiId[6](see reference [1]). Reference: [1] https://git.kernel.org/tip/e0ceeae708cebf22c990c3d703a4ca187dc837f5 Signed-off-by: Pu Wen <puwen@hygon.cn> --- xen/arch/x86/cpu/Makefile | 1 + xen/arch/x86/cpu/amd.c | 7 +-- xen/arch/x86/cpu/common.c | 9 ++++ xen/arch/x86/cpu/cpu.h | 4 ++ xen/arch/x86/cpu/hygon.c | 95 +++++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/x86-vendors.h | 3 +- 6 files changed, 112 insertions(+), 7 deletions(-) create mode 100644 xen/arch/x86/cpu/hygon.c