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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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 ?

Yes, if a stub can't properly fulfill the needed role, then that's the
(about only) way to go, I think.

Jan