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
