> -----Original Message-----
> From: Roger Pau Monne <roger....@citrix.com>
> Sent: 23 August 2019 11:56
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabell...@kernel.org>; Julien Grall
> <julien.gr...@arm.com>; Volodymyr Babchuk <volodymyr_babc...@epam.com>; Jan
> Beulich
> <jbeul...@suse.com>; Andrew Cooper <andrew.coop...@citrix.com>; Wei Liu
> <w...@xen.org>; Jun Nakajima
> <jun.nakaj...@intel.com>; Kevin Tian <kevin.t...@intel.com>; George Dunlap
> <george.dun...@citrix.com>;
> Suravee Suthikulpanit <suravee.suthikulpa...@amd.com>; Brian Woods
> <brian.wo...@amd.com>; Daniel De
> Graaf <dgde...@tycho.nsa.gov>
> 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.durr...@citrix.com>
>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
