Re: [Xen-devel] [PATCH v1 06/10] iommu: Add extra use_iommu argument to iommu_domain_init()

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, Jan

On Fri, May 19, 2017 at 9:30 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 18.05.17 at 19:41, <olekstysh@xxxxxxxxx> wrote:
>> patch #6: As for the current patch, likely the "init" platform
>> callback should be extended with
>> extra "use_iommu" argument as well as the "iommu_domain_init" API. In
>> that case we
>> would just pass thought incoming flag to the IOMMU drivers followed by
>> updating "need_iommu" domain flag.
>>
>> Something like this:
>> ...
>> int iommu_domain_init(struct domain *d, bool use_iommu)
>> {
>>     struct domain_iommu *hd = dom_iommu(d);
>>     int ret = 0;
>>
>>     ret = arch_iommu_domain_init(d);
>>     if ( ret )
>>         return ret;
>>
>>     if ( !iommu_enabled )
>>         return 0;
>>
>>     hd->platform_ops = iommu_get_ops();
>>     ret = hd->platform_ops->init(d, use_iommu);
>>     if ( ret )
>>         return ret;
>>
>>     d->need_iommu = !!use_iommu;
>>
>>     return 0;
>> }
>> ...
>
> The final shape of this primarily depends on ARM side needs.
> However, you need to be careful to make sure the final setting
> of d->need_iommu then is no different than it is today on at
> least x86. I'm mentioning this in particular because of e.g.
>
>     d->need_iommu = !!iommu_dom0_strict;
>
> in iommu_hwdom_init().
Yes, sure, I will take it into the account.

>
> Also as a minor remark note that in your new code the !! would
> not be needed.
>
>> patch #7: This patch should be just dropped.
>>
>> patch #8: As we always allocate the page table for hardware domain,
>> this patch should be reworked.
>> The use_iommu flag should be set for both archs in case of hardware
>> domain. Having d->need_iommu set at the early stage we won't skip
>> IOMMU
>> mapping updates anymore. And as the result the existing in
>> iommu_hwdom_init() code that goes through the list of page and tries
>> to retrieve mapping could be just dropped
>> instead of moving it to the arch-specific part.
>
> And again, careful here: There are three command line options
> influencing which pages do actually get mapped, and in which way
> (iommu=dom0-passthrough, iommu=dom0-strict, and VT-d's
> iommu_inclusive_mapping). The behavior after your change must
> not differ from current behavior regardless of which of these
> options may be used.
Yes, sure. This is my target not to brake things.

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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