Re: [PATCH] xen/iommu: arm: Don't insert an IOMMU mapping when the grantee and granter...

From: Stefano Stabellini <sstabellini@kernel.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>, Julien Grall <jgrall@amazon.com>,
	 Stefano Stabellini <sstabellini@kernel.org>,
	 Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	 Rahul Singh <Rahul.Singh@arm.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH] xen/iommu: arm: Don't insert an IOMMU mapping when the grantee and granter...
Date: Tue, 16 Feb 2021 10:50:14 -0800 (PST)	[thread overview]
Message-ID: <alpine.DEB.2.21.2102161049030.3234@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <3a827099-1d8f-826d-42ef-743d86d9ccce@suse.com>

On Mon, 15 Feb 2021, Jan Beulich wrote:
> On 14.02.2021 15:35, Julien Grall wrote:
> > From: Julien Grall <jgrall@amazon.com>
> > 
> > ... are the same.
> > 
> > When the IOMMU is enabled and the domain is direct mapped (e.g. Dom0),
> > Xen will insert a 1:1 mapping for each grant mapping in the P2M to
> > allow DMA.
> > 
> > This works quite well when the grantee and granter and not the same
> > because the GFN in the P2M should not be mapped. However, if they are
> > the same, we will overwrite the mapping. Worse, it will be completely
> > removed when the grant is unmapped.
> > 
> > As the domain is direct mapped, a 1:1 mapping should always present in
> > the P2M. This is not 100% guaranteed if the domain decides to mess with
> > the P2M. However, such domain would already end up in trouble as the
> > page would be soon be freed (when the last reference dropped).
> > 
> > Add an additional check in arm_iommu_{,un}map_page() to check whether
> > the page belongs to the domain. If it is belongs to it, then ignore the
> > request.
> 
> Doesn't this want / need solving in grant_table.c itself, as it also
> affects PV on x86? Or alternatively in gnttab_need_iommu_mapping(),
> handing the macro the MFN alongside the domain? No matter which one
> was chosen, it could at the same time avoid the expensive mapkind()
> invocation in this case.

Not knowing the x86 side I don't have an opinion on the best location
for the check. But I wanted to say for the records that the patch has
already been tested successfully and looks good to me.