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

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

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?

>
> Jan
>



-- 
Regards,

Oleksandr Tyshchenko

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