On 07/10/2018 05:18 PM, Paul Durrant wrote: >> -----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.
That works for me. -George _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
