> -----Original Message-----
> From: Roger Pau Monne <[email protected]>
> Sent: 23 August 2019 11:56
> To: Paul Durrant <[email protected]>
> Cc: [email protected]; Stefano Stabellini
> <[email protected]>; Julien Grall
> <[email protected]>; Volodymyr Babchuk <[email protected]>; Jan
> Beulich
> <[email protected]>; Andrew Cooper <[email protected]>; Wei Liu
> <[email protected]>; Jun Nakajima
> <[email protected]>; Kevin Tian <[email protected]>; George Dunlap
> <[email protected]>;
> Suravee Suthikulpanit <[email protected]>; Brian Woods
> <[email protected]>; Daniel De
> Graaf <[email protected]>
> 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 <[email protected]>
>
> Reviewed-by: Roger Pau Monné <[email protected]>
>
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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
