On Thu, Jun 13, 2024 at 2:03 PM Jan Beulich <jbeul...@suse.com> 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.
