Re: [Xen-devel] [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 07 August 2019 10:22
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>;
> Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 1/6] domain: introduce XEN_DOMCTL_CDF_iommu
> 
> On 30.07.2019 15:44, Paul Durrant wrote:
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -673,8 +673,7 @@ int arch_domain_create(struct domain *d,
> >
> >       ASSERT(config != NULL);
> >
> > -    /* p2m_init relies on some value initialized by the IOMMU subsystem */
> > -    if ( (rc = iommu_domain_init(d)) != 0 )
> > +    if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 )
> >           goto fail;
> 
> Instead of this and ...
> 
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -604,7 +604,7 @@ int arch_domain_create(struct domain *d,
> >       if ( (rc = init_domain_irq_mapping(d)) != 0 )
> >           goto fail;
> >
> > -    if ( (rc = iommu_domain_init(d)) != 0 )
> > +    if ( is_iommu_enabled(d) && (rc = iommu_domain_init(d)) != 0 )
> >           goto fail;
> 
> ... this (and any further copies in future ports), wouldn't it
> be better to centrally do this in iommu_domain_init() itself?

Ok... it kind of seemed more logical to avoid the call if the flag is not present... but there's no real consistency on this kind of thing in the Xen codebase so I'll change it to shorten the patch.

> 
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -301,7 +301,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
> >                              XEN_DOMCTL_CDF_hap |
> >                              XEN_DOMCTL_CDF_s3_integrity |
> >                              XEN_DOMCTL_CDF_oos_off |
> > -                           XEN_DOMCTL_CDF_xs_domain) )
> > +                           XEN_DOMCTL_CDF_xs_domain |
> > +                           XEN_DOMCTL_CDF_iommu) )
> >       {
> >           dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
> >           return -EINVAL;
> 
> Also refuse XEN_DOMCTL_CDF_iommu when !iommu_enabled?

Yes, that would be reasonable.

> 
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -981,6 +981,11 @@ static inline bool is_xenstore_domain(const struct domain *d)
> >       return d->options & XEN_DOMCTL_CDF_xs_domain;
> >   }
> >
> > +static inline bool is_iommu_enabled(const struct domain *d)
> > +{
> > +    return d->options & XEN_DOMCTL_CDF_iommu;
> > +}
> 
> Perhaps wrap in evaluate_nospec()?
> 

Given the codepaths that it will eventually get, yes it probably should have the guard.

  Paul

> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel