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

[Petr Beneš <w1benny@xxxxxxxxx>]

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
>
> #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;
}

- 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.

> > @@ -5228,7 +5234,7 @@ void hvm_fast_singlestep(struct vcpu *v, uint16_t 
> > p2midx)
> >      if ( !hvm_is_singlestep_supported() )
> >          return;
> >
> > -    if ( p2midx >= MAX_ALTP2M )
> > +    if ( p2midx >= v->domain->nr_altp2m )
> >          return;
>
> You don't introduce a new local variable here. I'd like to ask that you also
> don't ...
>
> > @@ -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?

P.