Re: [XEN PATCH v4 02/14] x86/altp2m: add static inline stub for altp2m_vcpu_idx()

[Sergiy Kibrik <sergiy_kibrik@xxxxxxxx>]

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@xxxxxxxx>
> 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@xxxxxxxx/

would that be acceptable ?

  -Sergiy