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

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

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

Would you be fine adjusting at commit, or would you prefer me to send
an updated version?

Thanks, Roger.