> -----Original Message----- > From: George Dunlap [mailto:[email protected]] > Sent: 10 July 2018 17:13 > To: Paul Durrant <[email protected]>; [email protected] > Cc: Jan Beulich <[email protected]>; Andrew Cooper > <[email protected]>; George Dunlap > <[email protected]>; Ian Jackson <[email protected]>; Konrad > Rzeszutek Wilk <[email protected]>; Stefano Stabellini > <[email protected]>; Julien Grall <[email protected]>; Tim (Xen.org) > <[email protected]>; Wei Liu <[email protected]>; Jun Nakajima > <[email protected]>; Kevin Tian <[email protected]> > Subject: Re: [PATCH v2 03/13] iommu: make use of type-safe BFN and MFN > in exported functions > > On 07/07/2018 12:05 PM, Paul Durrant wrote: > > This patch modifies the declaration of the entry points to the IOMMU > > sub-system to use bfn_t and mfn_t in place of unsigned long. A > subsequent > > patch will similarly modify the methods in the iommu_ops structure. > > > > Signed-off-by: Paul Durrant <[email protected]> > > --- > > Cc: Jan Beulich <[email protected]> > > Cc: Andrew Cooper <[email protected]> > > Cc: George Dunlap <[email protected]> > > Cc: Ian Jackson <[email protected]> > > Cc: Konrad Rzeszutek Wilk <[email protected]> > > Cc: Stefano Stabellini <[email protected]> > > Cc: Julien Grall <[email protected]> > > Cc: Tim Deegan <[email protected]> > > Cc: Wei Liu <[email protected]> > > Cc: Jun Nakajima <[email protected]> > > Cc: Kevin Tian <[email protected]> > > Cc: George Dunlap <[email protected]> > > > > v2: > > - Addressed comments from Jan. > > - Use intermediate 'frame' variable to avoid directly encapsulating > > mfn or gfn values as bfns. > > A couple of comments on particular instances... > > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > > index c53cab44d9..ce12bcff42 100644 > > --- a/xen/arch/x86/mm/p2m.c > > +++ b/xen/arch/x86/mm/p2m.c > > @@ -714,9 +714,12 @@ p2m_remove_page(struct p2m_domain *p2m, > unsigned long gfn_l, unsigned long mfn, > > > > if ( need_iommu(p2m->domain) ) > > { > > + unsigned long frame = mfn; > > + bfn_t bfn = _bfn(frame); > > + > > for ( i = 0; i < (1 << page_order); i++ ) > > { > > - int ret = iommu_unmap_page(p2m->domain, mfn + i); > > + int ret = iommu_unmap_page(p2m->domain, bfn_add(bfn, i)); > > Having a 'bfn' variable here makes some sense, because otherwise, if mfn > ever gets the mfn_t type, you'll have > > iommu_unmap_page(... _bfn(mfn_x(mfn)+i)); > > being able to use bfn_add() is much cleaner. I don't think the > intermediate 'frame' variable in the case, really adds anything. > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > > index d2610e320c..d0926d13e0 100644 > > --- a/xen/common/grant_table.c > > +++ b/xen/common/grant_table.c > > @@ -1132,6 +1132,8 @@ map_grant_ref( > > need_iommu = gnttab_need_iommu_mapping(ld); > > if ( need_iommu ) > > { > > + unsigned long frame = mfn_x(mfn); > > + bfn_t bfn = _bfn(frame); > > unsigned int kind; > > int err = 0; > > > > @@ -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. Paul > > -George _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
