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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.06.2024 17:46, Petr Beneš wrote:
> 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);
> }

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