| Message ID | 7a6980b1c67dedb306985f73afb23db359771e8f.1715761386.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make cpu virtualization support configurable | expand |
On Wed, 15 May 2024, Sergiy Kibrik wrote: > Instead of using generic CONFIG_HVM option switch to a bit more specific > CONFIG_ALTP2M option for altp2m support. Also guard altp2m routines, so that > they can be disabled completely in the build -- when target platform does not > actually support altp2m (AMD-V & ARM as of now). > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > CC: Tamas K Lengyel <tamas@tklengyel.com> > --- > changes in v2: > - use separate CONFIG_ALTP2M option instead of CONFIG_VMX > --- > xen/arch/x86/include/asm/altp2m.h | 5 ++++- > xen/arch/x86/include/asm/hvm/hvm.h | 2 +- > xen/arch/x86/include/asm/p2m.h | 17 ++++++++++++++++- > xen/arch/x86/mm/Makefile | 2 +- > 4 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h > index e5e59cbd68..092b13e231 100644 > --- a/xen/arch/x86/include/asm/altp2m.h > +++ b/xen/arch/x86/include/asm/altp2m.h > @@ -7,7 +7,7 @@ > #ifndef __ASM_X86_ALTP2M_H > #define __ASM_X86_ALTP2M_H > > -#ifdef CONFIG_HVM > +#ifdef CONFIG_ALTP2M > > #include <xen/types.h> > #include <xen/sched.h> /* for struct vcpu, struct domain */ > @@ -38,7 +38,10 @@ static inline bool altp2m_active(const struct domain *d) > } > > /* Only declaration is needed. DCE will optimise it out when linking. */ > +void altp2m_vcpu_initialise(struct vcpu *v); > +void altp2m_vcpu_destroy(struct vcpu *v); > uint16_t altp2m_vcpu_idx(const struct vcpu *v); > +int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn); > void altp2m_vcpu_disable_ve(struct vcpu *v); > > #endif > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h > index 0c9e6f1564..4f03dd7af8 100644 > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -670,7 +670,7 @@ static inline bool hvm_hap_supported(void) > /* returns true if hardware supports alternate p2m's */ > static inline bool hvm_altp2m_supported(void) > { > - return hvm_funcs.caps.altp2m; > + return IS_ENABLED(CONFIG_ALTP2M) && hvm_funcs.caps.altp2m; > } > > /* Returns true if we have the minimum hardware requirements for nested virt */ > diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h > index 111badf89a..855e69d24a 100644 > --- a/xen/arch/x86/include/asm/p2m.h > +++ b/xen/arch/x86/include/asm/p2m.h > @@ -581,9 +581,9 @@ static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) > return _gfn(mfn_x(mfn)); > } > > -#ifdef CONFIG_HVM > #define AP2MGET_prepopulate true > #define AP2MGET_query false > +#ifdef CONFIG_ALTP2M Is it necessary? Can't we just replace CONFIG_HVM with CONFIG_ALTP2M on the same line
On 16.05.2024 02:38, Stefano Stabellini wrote: > On Wed, 15 May 2024, Sergiy Kibrik wrote: >> --- a/xen/arch/x86/include/asm/p2m.h >> +++ b/xen/arch/x86/include/asm/p2m.h >> @@ -581,9 +581,9 @@ static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) >> return _gfn(mfn_x(mfn)); >> } >> >> -#ifdef CONFIG_HVM >> #define AP2MGET_prepopulate true >> #define AP2MGET_query false >> +#ifdef CONFIG_ALTP2M > > Is it necessary? Can't we just replace CONFIG_HVM with CONFIG_ALTP2M on > the same line No, because of where those constants are used. Sergiy, you want to move the new #ifdef down by a line though, such that the #define-s are separated from it by the blank line that presently (with your change applied) follows the #ifdef line. Jan
On 15.05.2024 11:10, Sergiy Kibrik wrote: > @@ -38,7 +38,10 @@ static inline bool altp2m_active(const struct domain *d) > } > > /* Only declaration is needed. DCE will optimise it out when linking. */ > +void altp2m_vcpu_initialise(struct vcpu *v); > +void altp2m_vcpu_destroy(struct vcpu *v); > uint16_t altp2m_vcpu_idx(const struct vcpu *v); > +int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn); > void altp2m_vcpu_disable_ve(struct vcpu *v); These additions look unrelated, as long as the description says nothing in this regard. > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -670,7 +670,7 @@ static inline bool hvm_hap_supported(void) > /* returns true if hardware supports alternate p2m's */ > static inline bool hvm_altp2m_supported(void) > { > - return hvm_funcs.caps.altp2m; > + return IS_ENABLED(CONFIG_ALTP2M) && hvm_funcs.caps.altp2m; Which in turn raises the question whether the altp2m struct field shouldn't become conditional upon CONFIG_ALTP2M too (or rather: instead, as the change here then would need to be done differently). Yet maybe that would entail further changes elsewhere, so may well better be left for later. > --- a/xen/arch/x86/mm/Makefile > +++ b/xen/arch/x86/mm/Makefile > @@ -1,7 +1,7 @@ > obj-y += shadow/ > obj-$(CONFIG_HVM) += hap/ > > -obj-$(CONFIG_HVM) += altp2m.o > +obj-$(CONFIG_ALTP2M) += altp2m.o This change I think wants to move to patch 5. Jan
16.05.24 14:01, Jan Beulich: > On 15.05.2024 11:10, Sergiy Kibrik wrote: >> @@ -38,7 +38,10 @@ static inline bool altp2m_active(const struct domain *d) >> } >> >> /* Only declaration is needed. DCE will optimise it out when linking. */ >> +void altp2m_vcpu_initialise(struct vcpu *v); >> +void altp2m_vcpu_destroy(struct vcpu *v); >> uint16_t altp2m_vcpu_idx(const struct vcpu *v); >> +int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn); >> void altp2m_vcpu_disable_ve(struct vcpu *v); > > These additions look unrelated, as long as the description says nothing in > this regard. agree, I'll update description on why these declarations are added > >> --- a/xen/arch/x86/include/asm/hvm/hvm.h >> +++ b/xen/arch/x86/include/asm/hvm/hvm.h >> @@ -670,7 +670,7 @@ static inline bool hvm_hap_supported(void) >> /* returns true if hardware supports alternate p2m's */ >> static inline bool hvm_altp2m_supported(void) >> { >> - return hvm_funcs.caps.altp2m; >> + return IS_ENABLED(CONFIG_ALTP2M) && hvm_funcs.caps.altp2m; > > Which in turn raises the question whether the altp2m struct field shouldn't > become conditional upon CONFIG_ALTP2M too (or rather: instead, as the change > here then would need to be done differently). Yet maybe that would entail > further changes elsewhere, so may well better be left for later. > but hvm_funcs.caps.altp2m is only a capability bit -- is it worth to become conditional? >> --- a/xen/arch/x86/mm/Makefile >> +++ b/xen/arch/x86/mm/Makefile >> @@ -1,7 +1,7 @@ >> obj-y += shadow/ >> obj-$(CONFIG_HVM) += hap/ >> >> -obj-$(CONFIG_HVM) += altp2m.o >> +obj-$(CONFIG_ALTP2M) += altp2m.o > > This change I think wants to move to patch 5. > If this moves to patch 5 then HVM=y && ALTP2M=n configuration combination will break the build in between patch 5 and 6, so I've decided to put it together with fixes of these build failures in patch 6. Maybe I can merge patch 5 & 6 together then ? -Sergiy
On 23.05.2024 12:44, Sergiy Kibrik wrote: > 16.05.24 14:01, Jan Beulich: >> On 15.05.2024 11:10, Sergiy Kibrik wrote: >>> --- a/xen/arch/x86/include/asm/hvm/hvm.h >>> +++ b/xen/arch/x86/include/asm/hvm/hvm.h >>> @@ -670,7 +670,7 @@ static inline bool hvm_hap_supported(void) >>> /* returns true if hardware supports alternate p2m's */ >>> static inline bool hvm_altp2m_supported(void) >>> { >>> - return hvm_funcs.caps.altp2m; >>> + return IS_ENABLED(CONFIG_ALTP2M) && hvm_funcs.caps.altp2m; >> >> Which in turn raises the question whether the altp2m struct field shouldn't >> become conditional upon CONFIG_ALTP2M too (or rather: instead, as the change >> here then would need to be done differently). Yet maybe that would entail >> further changes elsewhere, so may well better be left for later. > > but hvm_funcs.caps.altp2m is only a capability bit -- is it worth to > become conditional? Well, the comment was more based on the overall principle than the actual space savings that might result. Plus as said - likely that would not work anyway without further changes elsewhere. So perhaps okay to leave as you have it. >>> --- a/xen/arch/x86/mm/Makefile >>> +++ b/xen/arch/x86/mm/Makefile >>> @@ -1,7 +1,7 @@ >>> obj-y += shadow/ >>> obj-$(CONFIG_HVM) += hap/ >>> >>> -obj-$(CONFIG_HVM) += altp2m.o >>> +obj-$(CONFIG_ALTP2M) += altp2m.o >> >> This change I think wants to move to patch 5. >> > > If this moves to patch 5 then HVM=y && ALTP2M=n configuration > combination will break the build in between patch 5 and 6, so I've > decided to put it together with fixes of these build failures in patch 6. Hmm, yes, I think I see what you mean. > Maybe I can merge patch 5 & 6 together then ? Perhaps more consistent that way, yes. Jan
diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h index e5e59cbd68..092b13e231 100644 --- a/xen/arch/x86/include/asm/altp2m.h +++ b/xen/arch/x86/include/asm/altp2m.h @@ -7,7 +7,7 @@ #ifndef __ASM_X86_ALTP2M_H #define __ASM_X86_ALTP2M_H -#ifdef CONFIG_HVM +#ifdef CONFIG_ALTP2M #include <xen/types.h> #include <xen/sched.h> /* for struct vcpu, struct domain */ @@ -38,7 +38,10 @@ static inline bool altp2m_active(const struct domain *d) } /* Only declaration is needed. DCE will optimise it out when linking. */ +void altp2m_vcpu_initialise(struct vcpu *v); +void altp2m_vcpu_destroy(struct vcpu *v); uint16_t altp2m_vcpu_idx(const struct vcpu *v); +int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn); void altp2m_vcpu_disable_ve(struct vcpu *v); #endif diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 0c9e6f1564..4f03dd7af8 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -670,7 +670,7 @@ static inline bool hvm_hap_supported(void) /* returns true if hardware supports alternate p2m's */ static inline bool hvm_altp2m_supported(void) { - return hvm_funcs.caps.altp2m; + return IS_ENABLED(CONFIG_ALTP2M) && hvm_funcs.caps.altp2m; } /* Returns true if we have the minimum hardware requirements for nested virt */ diff --git a/xen/arch/x86/include/asm/p2m.h b/xen/arch/x86/include/asm/p2m.h index 111badf89a..855e69d24a 100644 --- a/xen/arch/x86/include/asm/p2m.h +++ b/xen/arch/x86/include/asm/p2m.h @@ -581,9 +581,9 @@ static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) return _gfn(mfn_x(mfn)); } -#ifdef CONFIG_HVM #define AP2MGET_prepopulate true #define AP2MGET_query false +#ifdef CONFIG_ALTP2M /* * Looks up altp2m entry. If the entry is not found it looks up the entry in @@ -593,6 +593,15 @@ static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) int altp2m_get_effective_entry(struct p2m_domain *ap2m, gfn_t gfn, mfn_t *mfn, p2m_type_t *t, p2m_access_t *a, bool prepopulate); +#else +static inline int altp2m_get_effective_entry(struct p2m_domain *ap2m, + gfn_t gfn, mfn_t *mfn, + p2m_type_t *t, p2m_access_t *a, + bool prepopulate) +{ + ASSERT_UNREACHABLE(); + return -EOPNOTSUPP; +} #endif /* Init the datastructures for later use by the p2m code */ @@ -909,8 +918,14 @@ static inline bool p2m_set_altp2m(struct vcpu *v, unsigned int idx) /* Switch alternate p2m for a single vcpu */ bool p2m_switch_vcpu_altp2m_by_id(struct vcpu *v, unsigned int idx); +#ifdef CONFIG_ALTP2M /* Check to see if vcpu should be switched to a different p2m. */ void p2m_altp2m_check(struct vcpu *v, uint16_t idx); +#else +static inline void p2m_altp2m_check(struct vcpu *v, uint16_t idx) +{ +} +#endif /* Flush all the alternate p2m's for a domain */ void p2m_flush_altp2m(struct domain *d); diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile index 0128ca7ab6..d7d57b8190 100644 --- a/xen/arch/x86/mm/Makefile +++ b/xen/arch/x86/mm/Makefile @@ -1,7 +1,7 @@ obj-y += shadow/ obj-$(CONFIG_HVM) += hap/ -obj-$(CONFIG_HVM) += altp2m.o +obj-$(CONFIG_ALTP2M) += altp2m.o obj-$(CONFIG_HVM) += guest_walk_2.o guest_walk_3.o guest_walk_4.o obj-$(CONFIG_SHADOW_PAGING) += guest_walk_4.o obj-$(CONFIG_MEM_ACCESS) += mem_access.o
Instead of using generic CONFIG_HVM option switch to a bit more specific CONFIG_ALTP2M option for altp2m support. Also guard altp2m routines, so that they can be disabled completely in the build -- when target platform does not actually support altp2m (AMD-V & ARM as of now). Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: Tamas K Lengyel <tamas@tklengyel.com> --- changes in v2: - use separate CONFIG_ALTP2M option instead of CONFIG_VMX --- xen/arch/x86/include/asm/altp2m.h | 5 ++++- xen/arch/x86/include/asm/hvm/hvm.h | 2 +- xen/arch/x86/include/asm/p2m.h | 17 ++++++++++++++++- xen/arch/x86/mm/Makefile | 2 +- 4 files changed, 22 insertions(+), 4 deletions(-)