Re: [PATCH for-4.19 v3 2/3] xen: enable altp2m at create domain domctl

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2024 15:33, Roger Pau Monne wrote:
> Enabling it using an HVM param is fragile, and complicates the logic when
> deciding whether options that interact with altp2m can also be enabled.
> 
> Leave the HVM param value for consumption by the guest, but prevent it from
> being set.  Enabling is now done using and additional altp2m specific field in
> xen_domctl_createdomain.
> 
> Note that albeit only currently implemented in x86, altp2m could be 
> implemented
> in other architectures, hence why the field is added to 
> xen_domctl_createdomain
> instead of xen_arch_domainconfig.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx> # hypervisor
albeit with one question:

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -637,6 +637,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>      bool hap = config->flags & XEN_DOMCTL_CDF_hap;
>      bool nested_virt = config->flags & XEN_DOMCTL_CDF_nested_virt;
>      unsigned int max_vcpus;
> +    unsigned int altp2m_mode = MASK_EXTR(config->altp2m_opts,
> +                                         XEN_DOMCTL_ALTP2M_mode_mask);
>  
>      if ( hvm ? !hvm_enabled : !IS_ENABLED(CONFIG_PV) )
>      {
> @@ -715,6 +717,26 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( config->altp2m_opts & ~XEN_DOMCTL_ALTP2M_mode_mask )
> +    {
> +        dprintk(XENLOG_INFO, "Invalid altp2m options selected: %#x\n",
> +                config->flags);
> +        return -EINVAL;
> +    }
> +
> +    if ( altp2m_mode && nested_virt )
> +    {
> +        dprintk(XENLOG_INFO,
> +                "Nested virt and altp2m are not supported together\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( altp2m_mode && !hap )
> +    {
> +        dprintk(XENLOG_INFO, "altp2m is only supported with HAP\n");
> +        return -EINVAL;
> +    }

Should this last one perhaps be further extended to permit altp2m with EPT
only?

Jan