| Message ID | 25e80b9271607da56abf9d4193e4d91b3d00bd9c.1723196909.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/CPU: optional build of Intel/AMD CPUs support | expand |
On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote: > With specific config option INTEL in place and most of the code that depends > on intel.c now can be optionally enabled/disabled it's now possible to put > the whole intel.c under INTEL option as well. This will allow for a Xen build > without Intel CPU support. > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > --- > xen/arch/x86/cpu/Makefile | 6 +++--- > xen/arch/x86/cpu/common.c | 4 +++- > xen/arch/x86/include/asm/processor.h | 7 ++++--- > 3 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile > index eafce5f204..020c86bda3 100644 > --- a/xen/arch/x86/cpu/Makefile > +++ b/xen/arch/x86/cpu/Makefile > @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o > +obj-$(CONFIG_INTEL) += intel_cacheinfo.o > obj-y += mwait-idle.o > -obj-y += shanghai.o > +obj-$(CONFIG_INTEL) += shanghai.o Why pick this one too? It's based on VIA IP, aiui. > obj-y += vpmu.o > obj-$(CONFIG_AMD) += vpmu_amd.o > obj-$(CONFIG_INTEL) += vpmu_intel.o > diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c > index ff4cd22897..50ce13f81c 100644 > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -336,11 +336,13 @@ void __init early_cpu_init(bool verbose) > > c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx); > switch (c->x86_vendor) { > +#ifdef CONFIG_INTEL > case X86_VENDOR_INTEL: intel_unlock_cpuid_leaves(c); > actual_cpu = intel_cpu_dev; break; > + case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break; > +#endif > case X86_VENDOR_AMD: actual_cpu = amd_cpu_dev; break; > case X86_VENDOR_CENTAUR: actual_cpu = centaur_cpu_dev; break; > - case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break; > case X86_VENDOR_HYGON: actual_cpu = hygon_cpu_dev; break; > default: > actual_cpu = default_cpu; > diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h > index 66463f6a6d..a88d45252b 100644 > --- a/xen/arch/x86/include/asm/processor.h > +++ b/xen/arch/x86/include/asm/processor.h > @@ -507,15 +507,16 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model, > extern int8_t opt_tsx; > extern bool rtm_disabled; > void tsx_init(void); > +void update_mcu_opt_ctrl(void); > +void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val); > #else > #define opt_tsx 0 /* explicitly indicate TSX is off */ > #define rtm_disabled false /* RTM was not force-disabled */ > static inline void tsx_init(void) {} > +static inline void update_mcu_opt_ctrl(void) {} > +static inline void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val) {} > #endif > > -void update_mcu_opt_ctrl(void); > -void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val); > - > enum ap_boot_method { > AP_BOOT_NORMAL, > AP_BOOT_SKINIT, Cheers, Alejandro
09.08.24 13:36, Alejandro Vallejo: > On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote: >> With specific config option INTEL in place and most of the code that depends >> on intel.c now can be optionally enabled/disabled it's now possible to put >> the whole intel.c under INTEL option as well. This will allow for a Xen build >> without Intel CPU support. >> >> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> >> --- >> xen/arch/x86/cpu/Makefile | 6 +++--- >> xen/arch/x86/cpu/common.c | 4 +++- >> xen/arch/x86/include/asm/processor.h | 7 ++++--- >> 3 files changed, 10 insertions(+), 7 deletions(-) >> >> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile >> index eafce5f204..020c86bda3 100644 >> --- a/xen/arch/x86/cpu/Makefile >> +++ b/xen/arch/x86/cpu/Makefile >> @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o >> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o >> obj-y += mwait-idle.o >> -obj-y += shanghai.o >> +obj-$(CONFIG_INTEL) += shanghai.o > > Why pick this one too? It's based on VIA IP, aiui. shanghai.c and intel.c both use init_intel_cacheinfo() routine, so there's build dependency on Intel code. -Sergiy
On 12.08.2024 11:40, Sergiy Kibrik wrote: > 09.08.24 13:36, Alejandro Vallejo: >> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote: >>> --- a/xen/arch/x86/cpu/Makefile >>> +++ b/xen/arch/x86/cpu/Makefile >>> @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o >>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o >>> obj-y += mwait-idle.o >>> -obj-y += shanghai.o >>> +obj-$(CONFIG_INTEL) += shanghai.o >> >> Why pick this one too? It's based on VIA IP, aiui. > > shanghai.c and intel.c both use init_intel_cacheinfo() routine, so > there's build dependency on Intel code. Yet Shanghai isn't as directly a clone of Intel CPUs as Hygon ones are for AMD. So at the very least you want to justify your choice in the description. After all there's also the alternative of having a separate SHANGHAI Kconfig setting, which would merely have "select INTEL" or "depends on INTEL". Jan
On Mon Aug 12, 2024 at 10:58 AM BST, Jan Beulich wrote: > On 12.08.2024 11:40, Sergiy Kibrik wrote: > > 09.08.24 13:36, Alejandro Vallejo: > >> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote: > >>> --- a/xen/arch/x86/cpu/Makefile > >>> +++ b/xen/arch/x86/cpu/Makefile > >>> @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o > >>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o > >>> obj-y += mwait-idle.o > >>> -obj-y += shanghai.o > >>> +obj-$(CONFIG_INTEL) += shanghai.o > >> > >> Why pick this one too? It's based on VIA IP, aiui. > > > > shanghai.c and intel.c both use init_intel_cacheinfo() routine, so > > there's build dependency on Intel code. My point is that the use of Intel functions on Shanghai and not Centaur is accidental. If shanghai goes under Intel so should Centaur (imo). > > Yet Shanghai isn't as directly a clone of Intel CPUs as Hygon ones are > for AMD. So at the very least you want to justify your choice in the > description. After all there's also the alternative of having a separate > SHANGHAI Kconfig setting, which would merely have "select INTEL" or > "depends on INTEL". > > Jan That's one option, another is for the Kconfig options to explicitly state which vendors they apply to. I'd be fine with either. It's less fine for CONFIG_INTEL to cover a VIA derivative and not the other. Cheers, Alejandro
12.08.24 15:24, Alejandro Vallejo: > On Mon Aug 12, 2024 at 10:58 AM BST, Jan Beulich wrote: >> On 12.08.2024 11:40, Sergiy Kibrik wrote: >>> 09.08.24 13:36, Alejandro Vallejo: >>>> On Fri Aug 9, 2024 at 11:09 AM BST, Sergiy Kibrik wrote: >>>>> --- a/xen/arch/x86/cpu/Makefile >>>>> +++ b/xen/arch/x86/cpu/Makefile >>>>> @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o >>>>> +obj-$(CONFIG_INTEL) += intel_cacheinfo.o >>>>> obj-y += mwait-idle.o >>>>> -obj-y += shanghai.o >>>>> +obj-$(CONFIG_INTEL) += shanghai.o >>>> >>>> Why pick this one too? It's based on VIA IP, aiui. >>> >>> shanghai.c and intel.c both use init_intel_cacheinfo() routine, so >>> there's build dependency on Intel code. > > My point is that the use of Intel functions on Shanghai and not Centaur is > accidental. If shanghai goes under Intel so should Centaur (imo). > >> >> Yet Shanghai isn't as directly a clone of Intel CPUs as Hygon ones are >> for AMD. So at the very least you want to justify your choice in the >> description. After all there's also the alternative of having a separate >> SHANGHAI Kconfig setting, which would merely have "select INTEL" or >> "depends on INTEL". >> >> Jan > > That's one option, another is for the Kconfig options to explicitly state which > vendors they apply to. I'd be fine with either. It's less fine for CONFIG_INTEL > to cover a VIA derivative and not the other. > I think I'll go with separate kconfig options -- for Centaur, Shanghai & Hygon, as we already got a separate submenu for CPUs & everything. Also this way it's more up to the task of config tuning. Thanks for clarification! -Sergiy
On 09.08.2024 12:09, Sergiy Kibrik wrote: > With specific config option INTEL in place and most of the code that depends > on intel.c now can be optionally enabled/disabled it's now possible to put > the whole intel.c under INTEL option as well. This will allow for a Xen build > without Intel CPU support. > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > --- > xen/arch/x86/cpu/Makefile | 6 +++--- > xen/arch/x86/cpu/common.c | 4 +++- > xen/arch/x86/include/asm/processor.h | 7 ++++--- > 3 files changed, 10 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile > index eafce5f204..020c86bda3 100644 > --- a/xen/arch/x86/cpu/Makefile > +++ b/xen/arch/x86/cpu/Makefile > @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o > +obj-$(CONFIG_INTEL) += intel_cacheinfo.o > obj-y += mwait-idle.o > -obj-y += shanghai.o > +obj-$(CONFIG_INTEL) += shanghai.o > obj-y += vpmu.o > obj-$(CONFIG_AMD) += vpmu_amd.o > obj-$(CONFIG_INTEL) += vpmu_intel.o > diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c > index ff4cd22897..50ce13f81c 100644 > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -336,11 +336,13 @@ void __init early_cpu_init(bool verbose) > > c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx); > switch (c->x86_vendor) { > +#ifdef CONFIG_INTEL > case X86_VENDOR_INTEL: intel_unlock_cpuid_leaves(c); > actual_cpu = intel_cpu_dev; break; > + case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break; > +#endif > case X86_VENDOR_AMD: actual_cpu = amd_cpu_dev; break; > case X86_VENDOR_CENTAUR: actual_cpu = centaur_cpu_dev; break; > - case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break; > case X86_VENDOR_HYGON: actual_cpu = hygon_cpu_dev; break; > default: > actual_cpu = default_cpu; > diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h > index 66463f6a6d..a88d45252b 100644 > --- a/xen/arch/x86/include/asm/processor.h > +++ b/xen/arch/x86/include/asm/processor.h > @@ -507,15 +507,16 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model, > extern int8_t opt_tsx; > extern bool rtm_disabled; > void tsx_init(void); > +void update_mcu_opt_ctrl(void); > +void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val); > #else > #define opt_tsx 0 /* explicitly indicate TSX is off */ > #define rtm_disabled false /* RTM was not force-disabled */ > static inline void tsx_init(void) {} > +static inline void update_mcu_opt_ctrl(void) {} > +static inline void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val) {} > #endif > > -void update_mcu_opt_ctrl(void); > -void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val); I'm okay-ish with the simple stubbing out for update_mcu_opt_ctrl(), but set_in_mcu_opt_ctrl() imo requires more work. The call sites in spec_ctrl.c shouldn't give the wrong impression of having some effect. Imo in init_speculation_mitigations() an #endif wants to move down, while all of gds_calculations() wants to be compiled out. And that likely extends to further Intel-only functions there (with an early bail-out keyed to boot_cpu_data.x86_vendor != X86_VENDOR_INTEL). Which overall likely means there wants to be another separate patch dealing with that. (And then maybe a counterpart one for AMD.) Jan
13.08.24 10:40, Jan Beulich: > I'm okay-ish with the simple stubbing out for update_mcu_opt_ctrl(), but > set_in_mcu_opt_ctrl() imo requires more work. The call sites in spec_ctrl.c > shouldn't give the wrong impression of having some effect. Imo in > init_speculation_mitigations() an #endif wants to move down, while all of > gds_calculations() wants to be compiled out. And that likely extends to > further Intel-only functions there (with an early bail-out keyed to > boot_cpu_data.x86_vendor != X86_VENDOR_INTEL). Which overall likely means > there wants to be another separate patch dealing with that. (And then > maybe a counterpart one for AMD.) Thanks for a suggestion, indeed there are many routines there that can be put under CONFIG_{AMD,INTEL}. -Sergiy
diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile index eafce5f204..020c86bda3 100644 --- a/xen/arch/x86/cpu/Makefile +++ b/xen/arch/x86/cpu/Makefile @@ -6,10 +6,10 @@ 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-$(CONFIG_INTEL) += intel.o +obj-$(CONFIG_INTEL) += intel_cacheinfo.o obj-y += mwait-idle.o -obj-y += shanghai.o +obj-$(CONFIG_INTEL) += shanghai.o obj-y += vpmu.o obj-$(CONFIG_AMD) += vpmu_amd.o obj-$(CONFIG_INTEL) += vpmu_intel.o diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index ff4cd22897..50ce13f81c 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -336,11 +336,13 @@ void __init early_cpu_init(bool verbose) c->x86_vendor = x86_cpuid_lookup_vendor(ebx, ecx, edx); switch (c->x86_vendor) { +#ifdef CONFIG_INTEL case X86_VENDOR_INTEL: intel_unlock_cpuid_leaves(c); actual_cpu = intel_cpu_dev; break; + case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break; +#endif case X86_VENDOR_AMD: actual_cpu = amd_cpu_dev; break; case X86_VENDOR_CENTAUR: actual_cpu = centaur_cpu_dev; break; - case X86_VENDOR_SHANGHAI: actual_cpu = shanghai_cpu_dev; break; case X86_VENDOR_HYGON: actual_cpu = hygon_cpu_dev; break; default: actual_cpu = default_cpu; diff --git a/xen/arch/x86/include/asm/processor.h b/xen/arch/x86/include/asm/processor.h index 66463f6a6d..a88d45252b 100644 --- a/xen/arch/x86/include/asm/processor.h +++ b/xen/arch/x86/include/asm/processor.h @@ -507,15 +507,16 @@ static inline uint8_t get_cpu_family(uint32_t raw, uint8_t *model, extern int8_t opt_tsx; extern bool rtm_disabled; void tsx_init(void); +void update_mcu_opt_ctrl(void); +void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val); #else #define opt_tsx 0 /* explicitly indicate TSX is off */ #define rtm_disabled false /* RTM was not force-disabled */ static inline void tsx_init(void) {} +static inline void update_mcu_opt_ctrl(void) {} +static inline void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val) {} #endif -void update_mcu_opt_ctrl(void); -void set_in_mcu_opt_ctrl(uint32_t mask, uint32_t val); - enum ap_boot_method { AP_BOOT_NORMAL, AP_BOOT_SKINIT,
With specific config option INTEL in place and most of the code that depends on intel.c now can be optionally enabled/disabled it's now possible to put the whole intel.c under INTEL option as well. This will allow for a Xen build without Intel CPU support. Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> --- xen/arch/x86/cpu/Makefile | 6 +++--- xen/arch/x86/cpu/common.c | 4 +++- xen/arch/x86/include/asm/processor.h | 7 ++++--- 3 files changed, 10 insertions(+), 7 deletions(-)