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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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".

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

Jan


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