Re: [PATCH for-4.19? v4 4/6] x86: Make the maximum number of altp2m views configurable

[Petr Beneš <w1benny@xxxxxxxxx>]

On Mon, May 27, 2024 at 8:19 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
>
> This is suspicious: You compare against one value but log another. This
> isn't EPT-specific, so shouldn't use MAX_EPTP.

Sorry, I copy-pasted a snippet and didn't edit it correctly. Of
course, it should have been:

if ( config->nr_altp2m > MAX_NR_ALTP2M )
{
    dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_NR_ALTP2M);
    return -EINVAL;
}

> > ... should I be consistent and also replace these accesses with
> > altp2m_get_eptp/altp2m_get_p2m (which will internally use
> > array_index_nospec), or should I leave them as they are?
>
> Perhaps leave them as they are, unless you can technically justify the
> adjustment.

If we can avoid speculative execution, why just not do it? The
performance overhead array_index_nospec is negligible compared to the
whole VMEXIT handling. It will also serve as future-proofing, since
nobody will be confused whether they should directly access the array,
but instead use the accessor function.

Currently, the idea seems to be that array_index_nospec() is used when
the index is user-controlled, and not used when the index is under
xen's control (i.e. in loops). But I found at least 2 instances where
the index _is_ user controlled and the nospec access is not used -
further proving my previous point.

That being said, if there are no protests, I would replace all array
index accesses with the newly introduced accessor functions, which
will unconditionally use array_index_nospec().

P.