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

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

On 16.08.2019 19:19, 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.
>       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);
>                                      ^~

Why would that be? Afaict there's no short-circuiting of
is_iommu_enabled() (albeit there could be, as we don't mean the shim
to have/use an IOMMU). Oh - I guess I see it: Instead of this change
what I think we want is for x86's iommu_call() to evaluate its 1st
argument. Otherwise we're liable to run into the same issue elsewhere
again.

> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -886,7 +886,7 @@ static int flask_map_domain_msi (struct domain *d, int irq, const void *data,
>  #endif
>  }
>  
> -static u32 flask_iommu_resource_use_perm(void)
> +static u32 flask_iommu_resource_use_perm(struct domain *d)

const?

With these adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel