Re: [PATCH for-4.19? v5 06/10] x86/altp2m: Introduce accessor functions for safer array indexing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.06.2024 22:04, Petr Beneš wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4887,10 +4887,10 @@ bool asmlinkage vmx_vmenter_helper(const struct 
> cpu_user_regs *regs)
> 
>              for ( i = 0; i < MAX_ALTP2M; ++i )
>              {
> -                if ( currd->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
> +                if ( altp2m_get_eptp(currd, i) == mfn_x(INVALID_MFN) )
>                      continue;
> 
> -                ept = &currd->arch.altp2m_p2m[i]->ept;
> +                ept = &altp2m_get_p2m(currd, i)->ept;
>                  if ( cpumask_test_cpu(cpu, ept->invalidate) )
>                  {
>                      cpumask_clear_cpu(cpu, ept->invalidate);

I'm not convinced we want to add the extra overhead in cases like
this one, where we shouldn't need it. I'd like to hear other
maintainers' views.

> --- a/xen/arch/x86/include/asm/altp2m.h
> +++ b/xen/arch/x86/include/asm/altp2m.h
> @@ -19,6 +19,38 @@ static inline bool altp2m_active(const struct domain *d)
>      return d->arch.altp2m_active;
>  }
> 
> +static inline struct p2m_domain *altp2m_get_p2m(const struct domain* d,

Nit: Style (misplaced *).

> +                                                unsigned int idx)
> +{
> +    return d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
> +}
> +
> +static inline uint64_t altp2m_get_eptp(const struct domain* d,

Same here. And more further down.

Further: At this point it's not necessary yet to switch away from
array_access_nospec(). You doing so right away is probably okay, but
then needs justifying in the description.

Jan