| Message ID | 952cb307528f16fc36a3fadbe26d83bc6805f81e.1720501197.git.Sergiy_Kibrik@epam.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: make CPU virtualisation support configurable | expand |
On 09.07.2024 07:48, Sergiy Kibrik wrote: > The stub just returns 0 due to implementation of p2m_get_mem_access() for > x86 & ARM expects it to be 0 when altp2m not active or not implemented. > > The separate stub is favoured over dynamic check for alt2pm availability > inside regular altp2m_vcpu_idx() because this way we retain the possibility > to later put the whole struct altp2mvcpu under CONFIG_ALTP2M. > > The purpose of the change is later to be able to disable altp2m support and > exclude its code from the build completely, when not supported by target > platform (as of now it's implemented for Intel EPT only). > > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> Yet what doesn't become clear is why 0 is a valid value to return. On the surface 0 is a valid index when altp2m is enabled. In which case it couldn't be used (without clarification) when altp2m is disabled. Jan
09.07.24 10:09, Jan Beulich: > On 09.07.2024 07:48, Sergiy Kibrik wrote: >> The stub just returns 0 due to implementation of p2m_get_mem_access() for >> x86 & ARM expects it to be 0 when altp2m not active or not implemented. >> >> The separate stub is favoured over dynamic check for alt2pm availability >> inside regular altp2m_vcpu_idx() because this way we retain the possibility >> to later put the whole struct altp2mvcpu under CONFIG_ALTP2M. >> >> The purpose of the change is later to be able to disable altp2m support and >> exclude its code from the build completely, when not supported by target >> platform (as of now it's implemented for Intel EPT only). >> >> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> > > Yet what doesn't become clear is why 0 is a valid value to return. On the > surface 0 is a valid index when altp2m is enabled. In which case it > couldn't be used (without clarification) when altp2m is disabled. > I looked into it a bit more and found your concerns to be valid -- indeed altp2m_vcpu_idx() should not return valid index when altp2m not supported. In fact it seems that this routine should not even be called when altp2m is not active -- all but one call sites check altp2m_active() first, and there's stub in include/asm-generic/altp2m.h to block accidental calls to it. So I think about falling back to v2 of this patch i.e. to guard that one out of line call in hvm_monitor_check_p2m() but with better patch description: https://lore.kernel.org/xen-devel/01767c3f98a88999d4b8ed3ae742ad66a0921ba3.1715761386.git.Sergiy_Kibrik@epam.com/ would that be acceptable ? -Sergiy
On 19.07.2024 12:25, Sergiy Kibrik wrote: > 09.07.24 10:09, Jan Beulich: >> On 09.07.2024 07:48, Sergiy Kibrik wrote: >>> The stub just returns 0 due to implementation of p2m_get_mem_access() for >>> x86 & ARM expects it to be 0 when altp2m not active or not implemented. >>> >>> The separate stub is favoured over dynamic check for alt2pm availability >>> inside regular altp2m_vcpu_idx() because this way we retain the possibility >>> to later put the whole struct altp2mvcpu under CONFIG_ALTP2M. >>> >>> The purpose of the change is later to be able to disable altp2m support and >>> exclude its code from the build completely, when not supported by target >>> platform (as of now it's implemented for Intel EPT only). >>> >>> Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> >> >> Yet what doesn't become clear is why 0 is a valid value to return. On the >> surface 0 is a valid index when altp2m is enabled. In which case it >> couldn't be used (without clarification) when altp2m is disabled. >> > > I looked into it a bit more and found your concerns to be valid -- > indeed altp2m_vcpu_idx() should not return valid index when altp2m not > supported. In fact it seems that this routine should not even be called > when altp2m is not active -- all but one call sites check > altp2m_active() first, and there's stub in include/asm-generic/altp2m.h > to block accidental calls to it. > > So I think about falling back to v2 of this patch i.e. to guard that one > out of line call in hvm_monitor_check_p2m() but with better patch > description: > > https://lore.kernel.org/xen-devel/01767c3f98a88999d4b8ed3ae742ad66a0921ba3.1715761386.git.Sergiy_Kibrik@epam.com/ > > would that be acceptable ? Yes, if a stub can't properly fulfill the needed role, then that's the (about only) way to go, I think. Jan
diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h index e5e59cbd68..c655b8a34e 100644 --- a/xen/arch/x86/include/asm/altp2m.h +++ b/xen/arch/x86/include/asm/altp2m.h @@ -37,8 +37,12 @@ static inline bool altp2m_active(const struct domain *d) return false; } +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v) +{ + return 0; +} + /* Only declaration is needed. DCE will optimise it out when linking. */ -uint16_t altp2m_vcpu_idx(const struct vcpu *v); void altp2m_vcpu_disable_ve(struct vcpu *v); #endif
The stub just returns 0 due to implementation of p2m_get_mem_access() for x86 & ARM expects it to be 0 when altp2m not active or not implemented. The separate stub is favoured over dynamic check for alt2pm availability inside regular altp2m_vcpu_idx() because this way we retain the possibility to later put the whole struct altp2mvcpu under CONFIG_ALTP2M. The purpose of the change is later to be able to disable altp2m support and exclude its code from the build completely, when not supported by target platform (as of now it's implemented for Intel EPT only). Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com> CC: Tamas K Lengyel <tamas@tklengyel.com> CC: Jan Beulich <jbeulich@suse.com> --- changes in v4: - add static inline stub for altp2m_vcpu_idx() instead of using altp2m_active() check inside altp2m_vcpu_idx() as suggested by Jan - changed patch description changes in v3: - move altp2m_active() check inside altp2m_vcpu_idx() - drop changes to monitor.c - changed patch description changes in v2: - patch description changed, removed VMX mentioning - guard by altp2m_active() instead of hvm_altp2m_supported() --- xen/arch/x86/include/asm/altp2m.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)