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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.05.2024 01:55, Petr Beneš wrote:
> On Tue, May 21, 2024 at 12:59 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> The compared entities don't really fit together. I think we want a new
>> MAX_NR_ALTP2M, which - for the time being - could simply be

Note that you've stripped too much context - "the compared entities" is
left without any meaning here, yet that's relevant to my earlier reply.

>> #define MAX_NR_ALTP2M MAX_EPTP
>>
>> in the header. That would then be a suitable replacement for the
>> min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) that you're adjusting
>> elsewhere. Which however raises the question whether in EPT-specific
>> code the min() wouldn't better survive, as min(d->nr_altp2m, MAX_EPTP).
>>
> 
> As you mentioned in a previous email, I've removed all the min(...,
> MAX_EPTP) invocations from the code, since nr_altp2m is validated to
> be no greater than that value. The only remaining places where this
> value occurs are:
> 
> - In my newly introduced condition in arch_sanitise_domain_config:
> 
> if ( config->nr_altp2m > MAX_EPTP )
> {
>     dprintk(XENLOG_INFO, "nr_altp2m must be <= %lu\n", MAX_NR_ALTP2M);
>     return -EINVAL;
> }

This is suspicious: You compare against one value but log another. This
isn't EPT-specific, so shouldn't use MAX_EPTP.

> - In hap_enable():
> 
> for ( i = 0; i < MAX_EPTP; i++ )
> {
>     d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
>     d->arch.altp2m_visible_eptp[i] = mfn_x(INVALID_MFN);
> }
> 
> Note that altp2m_eptp/altp2m_visible_eptp is never accessed beyond
> nr_altp2m. From what you're saying, it sounds to me like I should only
> replace the first mentioned occurrence with MAX_NR_ALTP2M. Correct me
> if I'm wrong.

Yes. I suspect though that there may be further places that want adjusting.

>>> @@ -403,12 +403,12 @@ long p2m_set_mem_access_multi(struct domain *d,
>>>      /* altp2m view 0 is treated as the hostp2m */
>>>      if ( altp2m_idx )
>>>      {
>>> -        if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
>>> -             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] 
>>> ==
>>> -             mfn_x(INVALID_MFN) )
>>> +        if ( altp2m_idx >= d->nr_altp2m ||
>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, 
>>> d->nr_altp2m)]
>>> +             == mfn_x(INVALID_MFN) )
>>
>> Please don't break previously correct style: Binary operators (here: == )
>> belong onto the end of the earlier line. That'll render the line too long
>> again, but you want to deal with that e.g. thus:
>>
>>              d->arch.altp2m_eptp[array_index_nospec(altp2m_idx,
>>                                                     d->nr_altp2m)] ==
>>              mfn_x(INVALID_MFN) )
>>
> 
> Roger suggested introducing the altp2m_get_p2m() function, which I
> like. I think introducing altp2m_get_eptp/visible_eptp and
> altp2m_set_eptp/visible_eptp would also elegantly solve the issue of
> overly long lines. My question is: if I go this route, should I
> strictly replace with these functions only accesses that use
> array_index_nospec()? Or should I replace all array accesses? For
> example:
> 
> for ( i = 0; i < d->nr_altp2m; i++ )
> {
>     struct p2m_domain *p2m;
> 
>     if ( d->arch.altp2m_eptp[i] == mfn_x(INVALID_MFN) )
>         continue;
> 
>     p2m = d->arch.altp2m_p2m[i];
> 
>     p2m_lock(p2m);
>     p2m->ept.ad = value;
>     p2m_unlock(p2m);
> }
> 
> ... 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.

Jan