Re: [Xen-devel] [RFC PATCH 2/9] iommu: Add ability to map/unmap the number of pages

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On Fri, Apr 28, 2017 at 1:29 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 28.04.17 at 12:16, <olekstysh@xxxxxxxxx> wrote:
>> On Fri, Apr 28, 2017 at 9:23 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>> >>> On 27.04.17 at 18:56, <olekstysh@xxxxxxxxx> wrote:
>>> > 2. xen/drivers/passthrough/vtd/x86/vtd.c:143:
>>> > ...
>>> > tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
>>> > for ( j = 0; j < tmp; j++ )
>>> > {
>>> >     int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
>>> >                              IOMMUF_readable|IOMMUF_writable);
>>> >
>>> >     if ( !rc )
>>> >        rc = ret;
>>> > }
>>> > ...
>>> >
>>> > Here we don't try to rollback mapping at all. Was the idea to do so? Or 
>>> > was
>>> > this place simply missed?
>>>
>>> Did you consider both the context this is in (establishing hwdom
>>> mappings) and the code's history? Both would tell you that yes,
>>> this is a best effort mapping attempt deliberately continuing
>>> despite errors (but reporting the first one). This behavior will
>>> need to be retained. Plus I'm sure you've noticed that this
>>> effectively is a single page mapping only anyway (due to
>>> PAGE_SHIFT == PAGE_SHIFT_4K); I have no idea why this
>>> "clever" code was used.
>> So, if I understand what your meant I don't even need to try to
>> optimize this code.
>> Despite the fact that this code would become much more simple:
>> ...
>> rc = iommu_map_pages(d, pfn, pfn, 1,
>>                   IOMMUF_readable|IOMMUF_writable);
>> ...
>> Right?
>
> Well, the actual simplification is entirely independent of you patch;
> what you'd add is just the extra order argument (which ought to
> be zero, btw). I am, however, of the opinion that the simplification
> would be good to do, but independent of your patch, and only
> unless the VT-d maintainers actually think there is a reason for this
> "cleverness".
Clear enough.

>
>>> > I can introduce something like this:
>>> >
>>> > #define __iommu_map_pages(d,gfn,mfn,order,flags)
>>> > (iommu_map_pages(d,gfn,mfn,1U << (order),flags))
>>>
>>> I'd prefer if you didn't. In no case should this be an identifier
>>> starting with an underscore.
>> I got it. I proposed because I had seen analogy with
>> __map_domain_page/map_domain_page.
>
> Well, there are quite a few things in long standing code which
> we wouldn't allow in new instances of anymore, one of which
> being the non-standard-conforming use of leading underscores.
> Eventually old code would be nice to be cleaned up too, but
> that's tedious and time consuming, and most if not all of us
> have better uses for their time. So commonly we do such
> cleanup only when respective code needs touching anyway.
Clear enough.

>
> Jan
>

-- 
Regards,

Oleksandr Tyshchenko

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