Explicitly check whether altp2m is on for domain when getting altp2m index.

The puspose of that 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 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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/altp2m.h b/xen/arch/x86/include/asm/altp2m.h
index e5e59cbd68..2d36c5aa9b 100644
--- a/xen/arch/x86/include/asm/altp2m.h
+++ b/xen/arch/x86/include/asm/altp2m.h
@@ -26,10 +26,6 @@ void altp2m_vcpu_destroy(struct vcpu *v);
 int altp2m_vcpu_enable_ve(struct vcpu *v, gfn_t gfn);
 void altp2m_vcpu_disable_ve(struct vcpu *v);
 
-static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
-{
-    return vcpu_altp2m(v).p2midx;
-}
 #else
 
 static inline bool altp2m_active(const struct domain *d)
@@ -38,9 +34,13 @@ static inline bool altp2m_active(const struct domain *d)
 }
 
 /* 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
 
+static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
+{
+    return altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0;
+}
+
 #endif /* __ASM_X86_ALTP2M_H */
-- 
2.25.1

On 03.06.2024 13:09, Sergiy Kibrik wrote:
> @@ -38,9 +34,13 @@ static inline bool altp2m_active(const struct domain *d)
>  }
>  
>  /* 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
>  
> +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
> +{
> +    return altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0;
> +}

While perhaps okay this way as a first step, my general expectation
would be that with ALTP2M=n there also wouldn't be any p2midx field
in the respective struct. Which in turn will mean that this code
would need re-doing again, and perhaps again splitting between an
inline one and a decl-only one. With that I wonder whether that split
wouldn't better be retained right away.

Jan

On 07.06.2024 09:25, Jan Beulich wrote:
> On 03.06.2024 13:09, Sergiy Kibrik wrote:
>> @@ -38,9 +34,13 @@ static inline bool altp2m_active(const struct domain *d)
>>  }
>>  
>>  /* 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
>>  
>> +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>> +{
>> +    return altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0;
>> +}
> 
> While perhaps okay this way as a first step,

Hmm, or maybe not. 0 is a valid index, and hence could be misleading
at call sites.

Jan

> my general expectation
> would be that with ALTP2M=n there also wouldn't be any p2midx field
> in the respective struct. Which in turn will mean that this code
> would need re-doing again, and perhaps again splitting between an
> inline one and a decl-only one. With that I wonder whether that split
> wouldn't better be retained right away.
> 
> Jan

07.06.24 10:50, Jan Beulich:
> On 07.06.2024 09:25, Jan Beulich wrote:
>> On 03.06.2024 13:09, Sergiy Kibrik wrote:
>>> @@ -38,9 +34,13 @@ static inline bool altp2m_active(const struct domain *d)
>>>   }
>>>   
>>>   /* 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
>>>   
>>> +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>>> +{
>>> +    return altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0;
>>> +}
>>
>> While perhaps okay this way as a first step,
> 
> Hmm, or maybe not. 0 is a valid index, and hence could be misleading
> at call sites.

I'm returning 0 index here because implementation of 
p2m_get_mem_access() for x86 & ARM expects it to be 0 when altp2m not 
active or not implemented.

   -Sergiy

On 07.06.2024 11:40, Sergiy Kibrik wrote:
> 07.06.24 10:50, Jan Beulich:
>> On 07.06.2024 09:25, Jan Beulich wrote:
>>> On 03.06.2024 13:09, Sergiy Kibrik wrote:
>>>> @@ -38,9 +34,13 @@ static inline bool altp2m_active(const struct domain *d)
>>>>   }
>>>>   
>>>>   /* 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
>>>>   
>>>> +static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v)
>>>> +{
>>>> +    return altp2m_active(v->domain) ? vcpu_altp2m(v).p2midx : 0;
>>>> +}
>>>
>>> While perhaps okay this way as a first step,
>>
>> Hmm, or maybe not. 0 is a valid index, and hence could be misleading
>> at call sites.
> 
> I'm returning 0 index here because implementation of 
> p2m_get_mem_access() for x86 & ARM expects it to be 0 when altp2m not 
> active or not implemented.

Tamas, considering the comment in x86'es p2m_get_mem_access(), what purpose
are d->arch.altp2m_p2m[0] and d->arch.altp2m_eptp[0] then? In case it indeed
is unused, why would p2m_init_altp2m() set up slot 0 in the first place?

Jan