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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, all.

On Thu, May 18, 2017 at 11:38 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 17.05.17 at 21:52, <julien.grall@xxxxxxx> wrote:
>> On 05/12/2017 03:31 PM, Jan Beulich wrote:
>>>>>> On 10.05.17 at 16:03, <olekstysh@xxxxxxxxx> wrote:
>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
>>>>
>>>> The presence of this flag lets us know that the guest
>>>> has devices which will most likely be used for passthrough
>>>> and as the result the use of IOMMU is expected for this domain.
>>>> In that case we have to call iommu_construct(), actually
>>>> what the real assign_device call usually does.
>>>>
>>>> As iommu_domain_init() is called with use_iommu flag being forced
>>>> to false for now, no functional change is intended for both ARM and x86.
>>>>
>>>> Basically, this patch is needed for non-shared IOMMUs on ARM only
>>>> since the non-shared IOMMUs on x86 are ok if iommu_construct() is called
>>>> later. But, in order to be more generic and for possible future 
>>>> optimization
>>>> make this change appliable for both platforms.
>>>
>>> I continue to be unconvinced that this is wanted / needed, as I
>>> continue to not see why shared vs unshared really matters here.
>>> After all we have both modes working on x86 without this flag.
>>
>> Well on x86 you allocate the page table on the fly in the unsharing
>> case. This is only useful if you don't know whether a domain will have
>> device assigned (e.g hotplug case).
>>
>> When you know that the domain will have device pass-throughed, you can
>> populate the IOMMU page tables before hand avoiding to have to go
>> through the list of page at the first assigned device.
>>
>> In embedded platform hotplug is likely to be inexistent. For servers, I
>> don't know but likely page tables are going to be shared (or as I
>> mentioned earlier partially shared).
>>
>> So I don't see any benefit of the current code over populating the IOMMU
>> page tables from the beginning.
>
> Interesting. To me, the primary benefit is that we wouldn't need to
> introduce new code to handle yet another case specially. Anyway,
> the changes in this patch are simple enough, so I don't mean to
> block it despite being unconvinced of the basic idea.

Thank you for your comments.
I would like to say that I share Julien's opinion, but understand
Jan's points too.
I think some mutually agreeable solution should be worked out.

It is not completely clear to me what I have to do with patches #6-#8.
So, I will try to summarize thoughts regarding them. Please, correct
me if I am wrong.

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

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.

So, what we would have as the final result:
1. In case of hardware domain "use_iommu" flag is always set for both
ARM and x86.
2. For other domains the "use_iommu" flag is always unset for x86
only, but the real value is passed for ARM
according to the libxl expectation about IOMMU usage.
This would allow us to allocate the page table in advance on ARM and
retain the current behavior for x86 (allocating the page table
on-demand).

What do you think?

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

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