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:58, <George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Tue, Jul 10, 2018 at 3:34 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> 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.
> 
> To me it looks the same as
>   unsigned long x;
>   char * s;
>   [do something to calculate x]
>   s = (char *)x
> 
> Obviously that sort of casting in C has sharp edges, so you need to be 
> careful.

Right, and I object to casts wherever I can.

> Bugs can happen in any sort of code -- would the bug you have in mind
> actually have been prevented with the use of an extra variable?

How can I tell? Maybe, maybe not.

>> As said in the other reply, something like
>>         iommu_map_page(..., _bfn(frame), frame, ...)
>> makes pretty clear that a 1:1 mapping is wanted.
> 
> I just don't see how this is supposed to catch more bugs than
>    /* gfns are mapped 1:1 with mfns */
>     iommu_map_page(..., _bfn(gfn), gfn, ...)

Well, with the comment it probably doesn't matter how the
variable(s) is/are named.

> There may be some places where having an intermediate variable might
> make things clearer, but there are an awful lot of places in Paul's
> patches where the code just looks like this:
>   unsigned long frame = bfn;
>   gfn_t gfn = _gfn(frame);
>   mfn_t mfn = _mfn(frame);
> 
> Which just seems really pointless.

This indeed looks to be going too far.

Jan


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