On 10/07/18 15:34, Jan Beulich wrote: >>>> On 10.07.18 at 16:29, <[email protected]> wrote: >> On Thu, Mar 15, 2018 at 3:44 PM, Jan Beulich <[email protected]> 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. > > As said in the other reply, something like > iommu_map_page(..., _bfn(frame), frame, ...) > makes pretty clear that a 1:1 mapping is wanted.
TBH, I think _bfn(gfn) is better, but any such mixing of address spaces needs a comment explaining the correctness, even if it is a short /* 1:1 mapping here */ Indirecting through an unsigned long (particularly one with a generic name) is a misuse of the typesafe interface, because all it does is serve to confuse the reader. ~Andrew _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
