Re: [Xen-devel] [PATCH v1 02/10] iommu: Add extra order argument to the IOMMU APIs and platform callbacks

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

Hi, all.

On Wed, May 17, 2017 at 6:39 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 17.05.17 at 17:28, <olekstysh@xxxxxxxxx> wrote:
>> Hi, Jan.
>>
>> On Tue, May 16, 2017 at 4:11 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 16.05.17 at 14:48, <olekstysh@xxxxxxxxx> wrote:
>>>> On Mon, May 15, 2017 at 3:33 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>> On 15.05.17 at 12:43, <olekstysh@xxxxxxxxx> wrote:
>>>>>> Indeed, there was some misunderstanding from my side on this.
>>>>>> Let me elaborate a bit more on this:
>>>>>> 1. Yes, this TODO shouldn't be just dropped, but needs to be
>>>>>> addressed, so at least I will have them back in the patch
>>>>>> 2. I am not a x86 guy and not familiar with the Intel/AMD IOMMUs, so
>>>>>> it makes me lots of work to do this change
>>>>>> properly, so this is not only the question of testing the code, but 
>>>>>> rather
>>>>>> having it written.
>>>>>> 3. Please correct me if I'm wrong, but these are all *optimizations* 
>>>>>> which
>>>>>> I am mentioning in that TODO, not something that breaks x86 or affects it
>>>>>> in any way.
>>>>>>
>>>>>> That being said, can we postpone implementation of the *optimizations*
>>>>>> in question
>>>>>> and have those as a separate activity?
>>>>>> Or if these *optimizations* must be present in the current patch
>>>>>> series, could you, please, provide me with some hints how
>>>>>> these TODO should be properly implemented?
>>>>>
>>>>> I'm puzzled. When I first commented on these TODOs I did say
>>>>> "While I appreciate this not being done in the already large patch,
>>>>> I don't think such a TODO should be left around. If need be (e.g.
>>>>> because you can't test the change), get in touch with the
>>>>> maintainer(s)." Of course the "e.g." extends to the actual
>>>>> implementation. IOW I'm not saying you need to do this work
>>>>> immediately and all by yourself, but there should be a clear plan
>>>>> on getting these items addressed. We shouldn't ship several
>>>>> releases with them still present. I'm sorry this hits you, but we've
>>>>> had too bad experience in the past with people leaving todo or
>>>>> fixme notes in the code, perhaps even promising to address them
>>>>> without ever doing so.
>>>> I see. You are right about leaving TODO)
>>>> Don't mind to get these items addressed *within current patch series*
>>>> as separate patch or patches.
>>>> So, we have to address for three IOMMUs: Intel/AMD and SMMU. I will
>>>> leave SMMU for myself.
>>>>
>>>> Could you, please, provide me with some hints how these TODO should be
>>>> properly implemented?
>>>
>>> I have to admit that I don't really understand the request. Quite
>>> clearly we want to use large pages in the case that hardware
>>> supports them.
>>>
>>>> I was thinking I can even just squash *pages with *page and send you a
>>>> draft as we need to start from somewhere.
>>>
>>> I'm afraid I've lost too much of the context to see what you mean
>>> here.
>> Sorry if I was unclear.
>>
>> At the moment patch contains three TODOs in the following files:
>> 1. a/xen/drivers/passthrough/vtd/iommu.c
>> 2. a/xen/drivers/passthrough/amd/iommu_map.c
>> 3. a/xen/drivers/passthrough/arm/smmu.c
>>
>> And the *optimization* which I mentioned in that TODO is same for all
>> three files.
>> +/* TODO: Optimize by squashing map_pages/unmap_pages with
>> map_page/unmap_page */
>>
>> I think that I could try to address this TODO by myself as I imagine
>> it should be addressed and send you a draft or post RFC patch.
>> As the result of this RFC patch we would have map_pages/unmap_pages
>> callbacks only, but still iterate 4K pages.
>>
>> We need to start from somewhere and this patch would be a base point
>> for continue optimizing.
>> What do you think? Or you have another opinion?
>
> Well, yes, this would reduce the scope of the TODO, but no, it
> wouldn't eliminate it. After all the primary goal (from my
> perspective) of adding the order parameter is to make use of
> large pages whenever possible. And as said before - it's not like
> it has to be you who does the work, but I'd sort of expect that
> whoever introduces TODOs at least tries to arrange for them
> being taken care of (unless e.g. they affect exotic situations
> only), for example by pulling in the respective maintainers.

Jan,
I understand what you are taking about. CCed respective maintainers.

Kevin, Suravee,
First of all my apologies for the fact that I haven't CCed your
earlier. I added changes
to files you are the maintainers of. My bad.

A bit of context.
This patch touches Intel/AMD IOMMUs as well as other IOMMUs since it adds extra
order argument. The purpose of adding extra order argument is to make use of
super pages whenever possible. Being honest I am interested in adding
IPMMU support
on ARM and this kind of IOMMU does support super pages. And as we
don't want to keep
separate single-page and multi-page stuff together it was decided to
rename existing APIs/callbacks
and add order argument.
In order not to brake existing x86-specific drivers (to retain current behavior)
I had to introduce additional helpers inside them and leave some TODO
which describe that
some optimization is needed.

I can try to reduce the scope of these TODO (to have
map_pages/unmap_pages callbacks only,
but still iterate 4K pages even if hardware supports large pages), but
I am sure that I won't be able to eliminate them completely
(to use large pages in the case that hardware supports them) due to
the several reasons.
I am neither familiar with x86 nor even have x86 boards, excuse me,
but I don't even know support these hardware super pages or not.

I want this patch to be accepted, so some common ground should be
found on getting these items addressed. Maybe you already have
some plan regarding adding such support?

>
> Jan

-- 
Regards,

Oleksandr Tyshchenko

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