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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 07 August 2019 10:22
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Anthony Perard <anthony.perard@xxxxxxxxxx>; 
> Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; 
> George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano 
> Stabellini
> <sstabellini@xxxxxxxxxx>; Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel