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

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

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.

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?

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

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.

 -George

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