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