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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.05.17 at 19:00, <olekstysh@xxxxxxxxx> wrote:
> On Fri, May 12, 2017 at 5:31 PM, Jan Beulich <JBeulich@xxxxxxxx> 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.
> I know. But, for ARM we need this hint. We can't reuse the "retrieving
> mapping" code I moved to x86-specific part in patch #8 (due to lack of
> M2P on ARM) .

Well, see the reply to 08/10 I've sent a minute ago.

>>> @@ -142,7 +142,14 @@ int iommu_domain_init(struct domain *d)
>>>          return 0;
>>>
>>>      hd->platform_ops = iommu_get_ops();
>>> -    return hd->platform_ops->init(d);
>>> +    ret = hd->platform_ops->init(d);
>>> +    if ( ret )
>>> +        return ret;
>>> +
>>> +    if ( use_iommu && !is_hardware_domain(d) )
>>> +        ret = iommu_construct(d);
>>
>> You don't handle the -ERESTART you may (and likely will) get here
>> or in the caller.
> Indeed. I forgot about it.
> 
> I most likely rework this patch not to call iommu_construct at all.
> But, there are an open questions here and in patch #7 I would like to
> clarify the first.
> 1. Are you against extra arguments at all?

No, extra arguments aren't the point. The kind of information passed
is the questionable thing.

> 2. If this question still makes sense, Shall I add extra need_iommu
> argument to "init" callback too and just pass thought
>     incoming flag to IOMMU drivers? This change wouldn't affect x86 at
> all since we set this flag for ARM only (see patch #9).
>     For x86 this flag would be always treated as unused.

I won't give a firm answer here considering the wider question on
M2P, but I'd like to say that always-unused flags on one architecture
are often (but not always, namely not when there's a hardware
feature one architecture has but others don't) a sign of a design/
abstraction problem.

Jan


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