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

[Petr Beneš <w1benny@xxxxxxxxx>]

On Thu, Jun 13, 2024 at 2:03 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > @@ -510,13 +526,13 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
> > int idx,
> >      mfn_t mfn;
> >      int rc = -EINVAL;
> >
> > -    if ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> > +    if ( idx >= d->nr_altp2m ||
> >           d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
>
> This ends up being suspicious: The range check is against a value different
> from what is passed to array_index_nospec(). The two weren't the same
> before either, but there the range check was more strict (which now isn't
> visible anymore, even though I think it would still be true). Imo this
> wants a comment, or an assertion effectively taking the place of a comment.

> Since they're all "is this slot populated" checks, maybe we want
> an is_altp2m_eptp_valid() helper?

Let me see if I understand correctly. You're suggesting the condition
should be replaced with something like this? (Also, I would suggest
altp2m_is_eptp_valid() name, since it's consistent e.g. with
p2m_is_altp2m().)

static inline bool altp2m_is_eptp_valid(const struct domain *d,
                                        unsigned int idx)
{
    /*
     * EPTP index is correlated with altp2m index and should not exceed
     * d->nr_altp2m.
     */
    assert(idx < d->nr_altp2m);

    return idx < MAX_EPTP &&
        d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] !=
        mfn_x(INVALID_MFN);
}

Note that in the codebase there are also very similar checks, but
again without array_index_nospec. For instance, in the
p2m_altp2m_propagate_change() function (which is called fairly
frequently):

int p2m_altp2m_propagate_change(struct domain *d, gfn_t gfn,
                                mfn_t mfn, unsigned int page_order,
                                p2m_type_t p2mt, p2m_access_t p2ma)
{
    struct p2m_domain *p2m;
    unsigned int i;
    unsigned int reset_count = 0;
    unsigned int last_reset_idx = ~0;
    int ret = 0;

    if ( !altp2m_active(d) )
        return 0;

    altp2m_list_lock(d);

    for ( i = 0; i < d->nr_altp2m; i++ )
    {
        p2m_type_t t;
        p2m_access_t a;

        // XXX this could be replaced with altp2m_is_eptp_valid(), but
based on previous review remarks,
        // it would introduce unnecessary perf. hit. So, should these
occurrences left unchanged?
        if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
            continue;

       ...

There are more instances of this. Which re-opens again the issue from
previous conversation: should I introduce a function which will be
used in some cases (where _nospec is used) and not used elsewhere?

P.