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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.
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.

Jan


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