Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Sent: 23 August 2019 11:56
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>; Julien Grall
> <julien.grall@xxxxxxx>; Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Jan 
> Beulich
> <jbeulich@xxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Jun Nakajima
> <jun.nakajima@xxxxxxxxx>; Kevin Tian <kevin.tian@xxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxx>;
> Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Brian Woods 
> <brian.woods@xxxxxxx>; Daniel De
> Graaf <dgdegra@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
> 
> On Fri, Aug 16, 2019 at 06:19:58PM +0100, Paul Durrant wrote:
> > ...rather than testing the global iommu_enabled flag and ops pointer.
> >
> > Now that there is a per-domain flag indicating whether the domain is
> > permitted to use the IOMMU (which determines whether the ops pointer will
> > be set), many tests of the global iommu_enabled flag and ops pointer can
> > be translated into tests of the per-domain flag. Some of the other tests of
> > purely the global iommu_enabled flag can also be translated into tests of
> > the per-domain flag.
> >
> > NOTE: The comment in iommu_share_p2m_table() is also fixed; need_iommu()
> >       disappeared some time ago. Also, whilst the style of the 'if' in
> >       flask_iommu_resource_use_perm() is fixed, I have not translated any
> >       instances of u32 into uint32_t to keep consistency. IMO such a
> >       translation would be better done globally for the source module in
> >       a separate patch.
> 
> I've usually changed those instances as the line gets modified anyway,
> but I'm not going to ask everyone to do this if they don't feel like
> it.
> 
> The problem with doing wholesale changes of u32 -> uint32_t is that
> you introduce a lot of noise when trying to use git blame afterwards
> for example. Doing it when touching the line for legitimate changes
> avoids the noise.
> 
> >       The change in the initialization of the 'hd' variable in iommu_map()
> >       and iommu_unmap() is done to keep the PV shim build happy. Without
> >       this change it will fail to compile with errors of the form:
> >
> > iommu.c:361:32: error: unused variable ‘hd’ [-Werror=unused-variable]
> >      const struct domain_iommu *hd = dom_iommu(d);
> >                                      ^~
> >
> > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 

Thanks.

> I haven't looked however if there are further cases where
> iommu_enabled should be changed into is_iommu_enabled.
> 

Jan had clearly checked on a previous review iteration because he spotted a 
couple of places I had missed. I'm reasonably confident I've found them all now.

  Paul

> > @@ -556,8 +560,8 @@ int iommu_do_domctl(
> >  {
> >      int ret = -ENODEV;
> >
> > -    if ( !iommu_enabled )
> > -        return -ENOSYS;
> > +    if ( !is_iommu_enabled(d) )
> > +        return -EOPNOTSUPP;
> 
> I would use ENOENT here, but I don't have a strong opinion. The
> hypercall is supported, it's just that there's no iommu for this
> domain.
> 
> Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel