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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 10/07/18 15:34, Jan Beulich wrote:
>>>> On 10.07.18 at 16:29, <George.Dunlap@eu.citrix.com> wrote:
>> On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich <JBeulich@suse.com> 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.
> Okay - differing opinions, what do you do. To me an expression like
> _bfn(gfn) looks buggy. And iirc we've had bugs of this kind in the
> past, which would then contradict your "carefully changing address
> spaces" assumption.
>
> As said in the other reply, something like
> 	iommu_map_page(..., _bfn(frame), frame, ...)
> makes pretty clear that a 1:1 mapping is wanted.

TBH, I think _bfn(gfn) is better, but any such mixing of address spaces
needs a comment explaining the correctness, even if it is a short /* 1:1
mapping here */

Indirecting through an unsigned long (particularly one with a generic
name) is a misuse of the typesafe interface, because all it does is
serve to confuse the reader.

~Andrew

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