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_kib...@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_kib...@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
