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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Paul Durrant <paul.durrant@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Jun Nakajima <jun.nakajima@intel.com>, Wei Liu <wl@xen.org>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Brian Woods <brian.woods@amd.com>,
	Julien Grall <julien.grall@arm.com>,
	Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xenproject.org,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [Xen-devel] [PATCH v6 07/10] use is_iommu_enabled() where appropriate...
Date: Fri, 23 Aug 2019 12:55:40 +0200	[thread overview]
Message-ID: <20190823105540.t2vbjbyd3zahlaan@Air-de-Roger> (raw)
In-Reply-To: <20190816172001.3905-8-paul.durrant@citrix.com>

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@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

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

> @@ -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