Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts

From: "Jan Beulich" <JBeulich@suse.com>
To: Oleksandr Tyshchenko <olekstysh@gmail.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>,
	Julien Grall <julien.grall@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts
Date: Fri, 12 May 2017 09:34:31 -0600	[thread overview]
Message-ID: <5915F22702000078001595D9@prv-mh.provo.novell.com> (raw)
In-Reply-To: <CAPD2p-kHt_oYeW1eX722GZs+V0jQecgyc46vKSHjBU78sjHNNQ@mail.gmail.com>

>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote:
> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote:
>>> The "retrieving mapping" code has never executed since
>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with
>>> introducing the non-shared IOMMU patch series we can no longer keep
>>> this code as is due to the lack of M2P support.
>>>
>>> In order to retain the current behavior for x86 this code was completely
>>> moved to x86 specific part.
>>> For ARM we just need to populate IOMMU page table if need_iommu flag
>>> is already set and the IOMMU is non-shared.
>>>
>>> So, the logic on ARM was changed a bit, but no functional change for x86.
>>>
>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>> CC: Jan Beulich <jbeulich@suse.com>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>    Changes in V1:
>>>       - Clarify patch description.
>>
>> My prior objection stands: You don't make clear why architecture-
>> agnostic code needs to be moved into an architecture-specific file.
> 
> Is the following description more cleaner?
> 
> Although this code looks like architecture-agnostic code, there is
> only one thing that prevents it
> to be *completely* architecture-agnostic -> mfn_to_gmfn helper.
> As we don't have an M2P on ARM, it always returns incoming mfn.
> And if domain is not direct mapped we will get into the trouble using that.
> 
> We didn't care about this code before because it has never been executed
> (iommu_use_hap_pt(d) always returned true on ARM so far).
> But, with introducing the non-shared IOMMU patch series we can no longer keep
> this code as is since it will be executed for non-shared IOMMU.
> So, move this code to x86-specific file in order not to expose a possible bug.

Yes, this helps.

Jan

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