> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 15 March 2018 15:45
> To: Paul Durrant <[email protected]>
> Cc: Julien Grall <[email protected]>; Andrew Cooper
> <[email protected]>; Wei Liu <[email protected]>; George
> Dunlap <[email protected]>; Ian Jackson <[email protected]>;
> Jun Nakajima <[email protected]>; Kevin Tian
> <[email protected]>; Stefano Stabellini <[email protected]>; xen-
> [email protected]; Konrad Rzeszutek Wilk
> <[email protected]>; Tim (Xen.org) <[email protected]>
> Subject: Re: [PATCH 2/7] iommu: make use of type-safe BFN and MFN in
> exported functions
>
> >>> On 12.02.18 at 11:47, <[email protected]> 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.
>
Ok. I debated it with myself when I wrote the original patches. I'll move the
relevant hunks.
> > --- 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.
Ok, I'll re-phrase things in v2.
>
> > --- 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 ...
>
I'll add appropriately an appropriately named stack variable.
> > @@ -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.
>
Ok.
> > @@ -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).
>
Ok.
> > @@ -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.
>
Ok.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
