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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.07.18 at 18:18, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: George Dunlap [mailto:george.dunlap@xxxxxxxxxx]
>> Sent: 10 July 2018 17:13
>> On 07/07/2018 12:05 PM, Paul Durrant wrote:
>> > @@ -1144,14 +1146,13 @@ map_grant_ref(
>> >               !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
>> >          {
>> >              if ( !(kind & MAPKIND_WRITE) )
>> > -                err = iommu_map_page(ld, mfn_x(mfn), mfn_x(mfn),
>> > -                                     IOMMUF_readable|IOMMUF_writable);
>> > +                err = iommu_map_page(ld, bfn, mfn,
>> > +                                     IOMMUF_readable | IOMMUF_writable);
>> >          }
>> >          else if ( act_pin && !old_pin )
>> >          {
>> >              if ( !kind )
>> > -                err = iommu_map_page(ld, mfn_x(mfn), mfn_x(mfn),
>> > -                                     IOMMUF_readable);
>> > +                err = iommu_map_page(ld, bfn, mfn, IOMMUF_readable);
>> 
>> Here's an example where I think having an extra variable is somewhat
>> dangerous.  Before this change, it's obvious that you have a 1:1
>> mapping; now, looking just at this line, it's not obvious that bfn ==
>> mfn.  Worse, there's a risk that there will be some sort of bug
>> introduced which changes bfn, such that bfn != mfn anymore.
>> 
>> If you have to use an intermediate variable here, this should be
>> 
>>   iommu_map_page(..., _bfn(frame), _mfn(frame), ...);
>> 
>> But I really think
>> 
>>   iommu_map_page(..., _bfn(mfn_x(mfn)), mfn, ...);
>> 
>> makes the most sense here.
> 
> How about:
> 
> #define mfn_to_bfn(mfn) (_bfn(mfn_x(mfn))
> 
> iommu_map_page(..., mfn_to_bfn(mfn), mfn, ...);
> 
> ?
> 
> I can similarly define gfn_to_bfn() for places where it is needed.

Please don't: Such macros (see others like mfn_to_gmfn() or
pfn_to_pdx()) imply translation between address spaces, yet there's
no translation here. If anything, the macro name would have to make
clear there's no translation, e.g. cast_mfn_to_bfn(), but I think the
open coding is then more clear and not significantly longer.

Jan



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