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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.05.17 at 17:34, <JBeulich@xxxxxxxx> wrote:
>>>> On 12.05.17 at 17:25, <olekstysh@xxxxxxxxx> wrote:
>> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 10.05.17 at 16:03, <olekstysh@xxxxxxxxx> 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@xxxxxxxx>
>>>> CC: Jan Beulich <jbeulich@xxxxxxxx>
>>>> CC: Julien Grall <julien.grall@xxxxxxx>
>>>>
>>>> ---
>>>>    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.

Having thought about this some more, what's still missing is a
clear explanation why this new need of a non-stub mfn_to_gmfn()
isn't finally enough of a reason to introduce an M2P on ARM. We
currently have several uses already which ARM fakes one way or
another:
- gnttab_shared_gmfn()
- gnttab_status_gmfn()
- memory_exchange()
- getdomaininfo()
With this I think there's quite a bit of justification needed to keep
going without M2P on ARM.

Jan


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