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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.07.18 at 16:29, <George.Dunlap@xxxxxxxxxxxxx> wrote:
> 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.

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.

Jan



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