On 19.06.2024 17:46, Petr Beneš wrote:
> 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);
> }
Not exactly. You may not assert on idx. The assertion, if any, wants to
check d->nr_altp2m against MAX_EPTP.
> 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?
You're again comparing cases where we control the index (in the loop) with
cases where we don't (hypercall inputs).
Jan
