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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 22, 2024 at 03:34:29PM +0200, Jan Beulich wrote:
> On 22.05.2024 15:16, Roger Pau Monné wrote:
> > On Tue, May 21, 2024 at 12:30:32PM +0200, Jan Beulich wrote:
> >> 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?
> > 
> > Hm, yes, that would be more accurate as:
> > 
> > if ( altp2m_mode && (!hap || !hvm_altp2m_supported()) )
> 
> Wouldn't
> 
>    if ( altp2m_mode && !hvm_altp2m_supported() )
> 
> suffice? hvm_funcs.caps.altp2m is not supposed to be set when no HAP,
> as long as HAP continues to be a pre-condition?

No, `hap` here signals whether the domain is using HAP, and we need to
take this int account, otherwise we would allow enabling altp2m for
domains using shadow.

Thanks, Roger.