On 27.05.2024 01:55, Petr Beneš wrote:
> On Tue, May 21, 2024 at 12:59 PM Jan Beulich <jbeul...@suse.com> 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
