[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/7] iommu: make use of type-safe BFN and MFN in exported functions
>>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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. > > NOTE: Since (with this patch applied) bfn_t is now in use, the patch also > introduces the 'cscope/grep fodder' to allow the type declaration to > be easily found. Ah, here we go. But I continue to think this belong in patch 1. > --- 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. > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -873,12 +873,14 @@ out: > if ( iommu_flags ) > for ( i = 0; i < (1 << order); i++ ) > { > - rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, > iommu_flags); > + rc = iommu_map_page(d, _bfn(gfn + i), mfn_add(mfn, i), > + iommu_flags); > if ( unlikely(rc) ) > { > while ( i-- ) > /* If statement to satisfy __must_check. */ > - if ( iommu_unmap_page(p2m->domain, gfn + i) ) > + if ( iommu_unmap_page(p2m->domain, > + _bfn(gfn + i)) ) The fundamental issue of mixed address spaces continues ... > @@ -781,14 +781,14 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, > mfn_t mfn, > { > for ( i = 0; i < (1 << page_order); i++ ) > { > - rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)), > - mfn_x(mfn_add(mfn, i)), > + rc = iommu_map_page(d, _bfn(mfn_x(mfn) + i), > + mfn_add(mfn, i), Please check whether some line wrapping can now be avoided, like apparently here. > @@ -1164,7 +1164,9 @@ int set_identity_p2m_entry(struct domain *d, unsigned > long gfn_l, > { > if ( !need_iommu(d) ) > return 0; > - return iommu_map_page(d, gfn_l, gfn_l, > IOMMUF_readable|IOMMUF_writable); > + > + return iommu_map_page(d, _bfn(gfn_l), _mfn(gfn_l), > + IOMMUF_readable|IOMMUF_writable); Please add spaces around | as you touch this (also elsewhere). > @@ -1254,7 +1256,8 @@ int clear_identity_p2m_entry(struct domain *d, unsigned > long gfn_l) > { > if ( !need_iommu(d) ) > return 0; > - return iommu_unmap_page(d, gfn_l); > + > + return iommu_unmap_page(d, _bfn(gfn_l)); > } No real need for the extra blank line here, as this isn't the main return point. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |