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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.05.2024 13:02, Petr Beneš wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -685,6 +685,18 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
> 
> +    if ( config->nr_altp2m && !hvm_altp2m_supported() )
> +    {
> +        dprintk(XENLOG_INFO, "altp2m requested but not available\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( config->nr_altp2m > MAX_EPTP )

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

> @@ -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) )

Jan