| Message ID | eb635da990e92443a91b2ae598ed354e35975efa.1553935727.git.puwen@hygon.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Hygon Dhyana Family 18h processor | expand |
On 30/03/2019 10:42, Pu Wen wrote: > +static const struct cpu_dev hygon_cpu_dev = { > + .c_vendor = "Hygon", > + .c_ident = { "HygonGenuine" }, > + .c_early_init = early_init_amd, > + .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 This change will need rebasing over http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=e72309ffbe7c4e507649c74749f130cda691131c , which has dropped the .c_ident field for a more efficient lookup mechanism. Hopefully the adjustments are all obvious. If not, I can draft a patch. ~Andrew
On 2019/4/2 20:16, Andrew Cooper wrote: > On 30/03/2019 10:42, Pu Wen wrote: >> +static const struct cpu_dev hygon_cpu_dev = { >> + .c_vendor = "Hygon", >> + .c_ident = { "HygonGenuine" }, >> + .c_early_init = early_init_amd, >> + .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 > > This change will need rebasing over > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=e72309ffbe7c4e507649c74749f130cda691131c > , which has dropped the .c_ident field for a more efficient lookup Oh, this is already in the staging branch! > mechanism. > > Hopefully the adjustments are all obvious. If not, I can draft a patch. I'll try to rework this patch rebasing over the latest staging branch for your review. Thanks.
>>> On 30.03.19 at 11:42, <puwen@hygon.cn> wrote: > +static void init_hygon(struct cpuinfo_x86 *c) > +{ > + 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); This still isn't in line with the AMD code it was derived from. In particular code and comment do not match up: You don't make any attempt to actually _set_ the intended mode, you only record the setting found in the feature flags. > + /* > + * 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)) > + { Since you've decided to inherit style from amd.c, the opening brace belongs on the previous line (more instances further down). > + value |= 1ull << 10; > + wrmsr_safe(MSR_AMD64_LS_CFG, value); > + } > + > + display_cacheinfo(c); Above from here amd.c sets MFENCE_RDTSC as well. Why would this not be needed for Hygon? > + 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); > + } There is a CPUID extended level check around this and ... > + c->x86_max_cores = (cpuid_ecx(0x80000008) & 0xff) + 1; ... also around this in the AMD original. Why did you drop this? Please don't forget that we may run virtualized ourselves, and that the respective leaves may have got hidden by the lower level hypervisor. Jan
On 2019/4/3 16:43, Jan Beulich wrote: > On 30.03.19 at 11:42, <puwen@hygon.cn> wrote: >> +static void init_hygon(struct cpuinfo_x86 *c) >> +{ >> + 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); > > This still isn't in line with the AMD code it was derived from. In > particular code and comment do not match up: You don't make any > attempt to actually _set_ the intended mode, you only record the > setting found in the feature flags. The code is derived but not fully copied. I tested the conditionals and found that the other branches are not reached on Hygon platforms, so I removed them. Our firmware will make sure that the bit AMD64_DE_CFG_LFENCE_SERIALISE will be set. So I just check here instead of setting. If you think retaining all the original conditionals is better, I'll do that. :) >> + /* >> + * 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)) >> + { > > Since you've decided to inherit style from amd.c, the opening brace > belongs on the previous line (more instances further down). I'm a little confused about which style to follow? In v3 series I followed the style of the derived code. But in other patch you told me to follow the Xen coding style, so in v4 series I changed the style to match the bracing section of CODING_STYLE. Anyway I can still inherit the style from amd.c. >> + value |= 1ull << 10; >> + wrmsr_safe(MSR_AMD64_LS_CFG, value); >> + } >> + >> + display_cacheinfo(c); > > Above from here amd.c sets MFENCE_RDTSC as well. Why would > this not be needed for Hygon? Because Hygon has feature LFENCE_DISPATCH, so the feature MFENCE_RDTSC will not be set here. But if you think the conditional should be retained here for some reason (even though the conditional may not be touched), I'll add it. >> + 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); >> + } > > There is a CPUID extended level check around this and ... > >> + c->x86_max_cores = (cpuid_ecx(0x80000008) & 0xff) + 1; > > ... also around this in the AMD original. Why did you drop this? The reason is somehow the same as the explanations above. Hygon CPU always has CPUID extended level, so I think there is no need to check it here. Different from AMD, which has many old families without the CPUID extended level, Hygon CPU is derived from AMD family 17h and always has the extended features. > Please don't forget that we may run virtualized ourselves, and > that the respective leaves may have got hidden by the lower > level hypervisor. I think this is the most important reason. Previously I only considered to run Hygon Xen on bare hardware, which is the most important usage for a server processor. To match all the using cases I'll add the checking you mentioned above.
>>> On 03.04.19 at 12:05, <puwen@hygon.cn> wrote: > On 2019/4/3 16:43, Jan Beulich wrote: >> On 30.03.19 at 11:42, <puwen@hygon.cn> wrote: >>> +static void init_hygon(struct cpuinfo_x86 *c) >>> +{ >>> + 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); >> >> This still isn't in line with the AMD code it was derived from. In >> particular code and comment do not match up: You don't make any >> attempt to actually _set_ the intended mode, you only record the >> setting found in the feature flags. > > The code is derived but not fully copied. I tested the conditionals and > found that the other branches are not reached on Hygon platforms, so I > removed them. > > Our firmware will make sure that the bit AMD64_DE_CFG_LFENCE_SERIALISE > will be set. So I just check here instead of setting. If you think > retaining all the original conditionals is better, I'll do that. :) I'm not convinced you need to retain everything, but you surely shouldn't limit code to work just on your specific variant of firmware. >>> + /* >>> + * 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)) >>> + { >> >> Since you've decided to inherit style from amd.c, the opening brace >> belongs on the previous line (more instances further down). > > I'm a little confused about which style to follow? In v3 series I > followed the style of the derived code. But in other patch you told me > to follow the Xen coding style, so in v4 series I changed the style to > match the bracing section of CODING_STYLE. Well, taking just the brace placement part doesn't make this the file Xen style. In my earlier response to that style question I did suggest you switch to Xen style for the new file. I'd still view this as the preferred option, but then all aspects should be taken care of. But I won't insist, yet in that case clean Linux style is the only other alternative. >>> + value |= 1ull << 10; >>> + wrmsr_safe(MSR_AMD64_LS_CFG, value); >>> + } >>> + >>> + display_cacheinfo(c); >> >> Above from here amd.c sets MFENCE_RDTSC as well. Why would >> this not be needed for Hygon? > > Because Hygon has feature LFENCE_DISPATCH, so the feature MFENCE_RDTSC > will not be set here. > > But if you think the conditional should be retained here for some reason > (even though the conditional may not be touched), I'll add it. See above - yes, I think it should be retained. Jan
On 2019/4/3 18:22, Jan Beulich wrote: > On 03.04.19 at 12:05, <puwen@hygon.cn> wrote: >> I'm a little confused about which style to follow? In v3 series I >> followed the style of the derived code. But in other patch you told me >> to follow the Xen coding style, so in v4 series I changed the style to >> match the bracing section of CODING_STYLE. > > Well, taking just the brace placement part doesn't make this > the file Xen style. In my earlier response to that style > question I did suggest you switch to Xen style for the new > file. I'd still view this as the preferred option, but then all > aspects should be taken care of. But I won't insist, yet in that > case clean Linux style is the only other alternative. Will inherit the style from amd.c in hygon.c. >> But if you think the conditional should be retained here for some reason >> (even though the conditional may not be touched), I'll add it. > > See above - yes, I think it should be retained. Okay, will retain the conditionals.
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..061ebdc 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -526,7 +526,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..db1ebf1 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -710,6 +710,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..6c52a56 100644 --- a/xen/arch/x86/cpu/cpu.h +++ b/xen/arch/x86/cpu/cpu.h @@ -17,7 +17,10 @@ extern unsigned int opt_cpuid_mask_ext_ecx, opt_cpuid_mask_ext_edx; 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..7ccbd84 --- /dev/null +++ b/xen/arch/x86/cpu/hygon.c @@ -0,0 +1,92 @@ +#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) +{ + unsigned int 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 init_hygon(struct cpuinfo_x86 *c) +{ + 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)) + { + rdmsrl(MSR_K7_HWCR, value); + value |= (1 << 27); /* Enable read-only APERF/MPERF bit */ + wrmsrl(MSR_K7_HWCR, value); + } +} + +static const struct cpu_dev hygon_cpu_dev = { + .c_vendor = "Hygon", + .c_ident = { "HygonGenuine" }, + .c_early_init = early_init_amd, + .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. 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 | 2 +- xen/arch/x86/cpu/common.c | 1 + xen/arch/x86/cpu/cpu.h | 3 ++ xen/arch/x86/cpu/hygon.c | 92 +++++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/x86-vendors.h | 3 +- 6 files changed, 100 insertions(+), 2 deletions(-) create mode 100644 xen/arch/x86/cpu/hygon.c