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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Jan.

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.

> Of course once you give a proper reason in the description, the
> change itself looks mostly fine - just one remark:
>
>> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>> +{
>> +    const struct domain_iommu *hd = dom_iommu(d);
>
> This isn't used outside of ...
>
>> +    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
>> +    {
>
> ... this if(), so should be moved here.
Agree. Will move.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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