Re: [Xen-devel] [PATCH 2/7] iommu: make use of type-safe BFN and MFN in exported functions

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info *page, 
>> unsigned long type,
>>          struct domain *d = page_get_owner(page);
>>          if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>>          {
>> -            gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page))));
>> +            bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page))));
>>
>>              if ( (x & PGT_type_mask) == PGT_writable_page )
>> -                iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
>> +                iommu_ret = iommu_unmap_page(d, bfn);
>>              else if ( type == PGT_writable_page )
>> -                iommu_ret = iommu_map_page(d, gfn_x(gfn),
>> -                                           mfn_x(page_to_mfn(page)),
>> +                iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
>
> Along the lines of what I've said earlier about mixing address spaces,
> this would perhaps not so much need a comment (it's a 1:1 mapping
> after all), but rather making more obvious that it's a 1:1 mapping.
> This in particular would mean to me to latch page_to_mfn(page) into
> a (neutrally named, e.g. "frame") local variable, and use the result in
> a way that makes obviously especially on the "map" path that this
> really requests a 1:1 mapping. By implication from the 1:1 mapping
> it'll then (hopefully) be clear to the reader that which exact name
> space is used doesn't really matter.

I'm sorry, I don't think this is a good idea.

First of all, it doesn't communicate what you think it does.  What
having an extra variable communicates is, "I am calculating an extra
value that will be used somewhere".  When I saw the "intermediate"
variables all over the place, I didn't immediately think "abstract
space because there's a 1-1 mapping", I was simply confused.

On the other hand, it is obvious to me that if you 1) have different
kinds of variables (gfn_t, bfn_t, &c) and 2) you cast one from the
other doing some math, that you're carefully changing address spaces;
and that if you do _bfn(gfn), that you know you have a 1-1 mapping --
or at least, you very much better well have one, or you're doing
something wrong.

"Documenting" something by introducing random extra unused variables
isn't a good idea.  Either people will waste time trying to verify
that they're not used a different way, or people will become
conditioned to the idea that they're not changing, and will overlook
bugs introduced when the variables actually do change.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel