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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2024 19:10, Petr Beneš wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -724,16 +724,42 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
> 
> -    if ( altp2m_mode && nested_virt )
> +    if ( altp2m_mode )
>      {
> -        dprintk(XENLOG_INFO,
> -                "Nested virt and altp2m are not supported together\n");
> -        return -EINVAL;
> -    }
> +        if ( nested_virt )
> +        {
> +            dprintk(XENLOG_INFO,
> +                    "Nested virt and altp2m are not supported together\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( !hap )
> +        {
> +            dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( !hvm_altp2m_supported() )
> +        {
> +            dprintk(XENLOG_INFO, "altp2m is not supported\n");
> +            return -EINVAL;
> +        }

Wouldn't this better be first in the group?

> @@ -510,13 +526,13 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
> int idx,
>      mfn_t mfn;
>      int rc = -EINVAL;
> 
> -    if ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> +    if ( idx >= d->nr_altp2m ||
>           d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==

This ends up being suspicious: The range check is against a value different
from what is passed to array_index_nospec(). The two weren't the same
before either, but there the range check was more strict (which now isn't
visible anymore, even though I think it would still be true). Imo this
wants a comment, or an assertion effectively taking the place of a comment.
(I actually wonder whether we really [still] need to allocate a full page
for d->arch.altp2m_eptp.)

> @@ -659,12 +675,13 @@ int p2m_set_suppress_ve_multi(struct domain *d,
> 
>      if ( sve->view > 0 )
>      {
> -        if ( sve->view >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> +        if ( sve->view >= d->nr_altp2m ||
>               d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_EPTP)] ==
>               mfn_x(INVALID_MFN) )
>              return -EINVAL;

Same again here and at least twice more further down, and yet more of those
elsewhere. Since they're all "is this slot populated" checks, maybe we want
an is_altp2m_eptp_valid() helper?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -103,7 +103,10 @@ struct xen_domctl_createdomain {
>  /* Altp2m mode signaling uses bits [0, 1]. */
>  #define XEN_DOMCTL_ALTP2M_mode_mask  (0x3U)
>  #define XEN_DOMCTL_ALTP2M_mode(m)    ((m) & XEN_DOMCTL_ALTP2M_mode_mask)
> -        uint32_t opts;
> +        uint16_t opts;
> +
> +        /* Number of altp2ms to allocate. */
> +        uint16_t nr;
>      } altp2m;

Nit: I wouldn't say "allocate" here, but "permit" or "support" or some such.
Whether any form of per-altp2m allocation is necessary is an implementation
detail.

Jan