> -----Original Message-----
> From: Jan Beulich [mailto:[email protected]]
> Sent: 30 October 2018 17:05
> To: Paul Durrant <[email protected]>
> Cc: Julien Grall <[email protected]>; Andrew Cooper
> <[email protected]>; George Dunlap <[email protected]>; Ian
> Jackson <[email protected]>; Wei Liu <[email protected]>; Jun
> Nakajima <[email protected]>; Kevin Tian <[email protected]>;
> Stefano Stabellini <[email protected]>; xen-devel <xen-
> [email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; Tim (Xen.org) <[email protected]>
> Subject: RE: [PATCH v2] iommu / p2m: add a page_order parameter to
> iommu_map/unmap_page()
>
> >>> On 30.10.18 at 17:56, <[email protected]> wrote:
> >> From: Jan Beulich [mailto:[email protected]]
> >> Sent: 30 October 2018 16:08
> >>
> >> >>> On 29.10.18 at 14:29, <[email protected]> wrote:
> >> > --- a/xen/common/grant_table.c
> >> > +++ b/xen/common/grant_table.c
> >> > @@ -1142,12 +1142,14 @@ map_grant_ref(
> >> > {
> >> > if ( !(kind & MAPKIND_WRITE) )
> >> > err = iommu_map_page(ld, _dfn(mfn_x(mfn)), mfn,
> >> > + PAGE_ORDER_4K,
> >> > IOMMUF_readable |
> >> IOMMUF_writable);
> >> > }
> >> > else if ( act_pin && !old_pin )
> >> > {
> >> > if ( !kind )
> >> > err = iommu_map_page(ld, _dfn(mfn_x(mfn)), mfn,
> >> > + PAGE_ORDER_4K,
> >> > IOMMUF_readable);
> >> > }
> >> > if ( err )
> >> > @@ -1396,10 +1398,11 @@ unmap_common(
> >> >
> >> > kind = mapkind(lgt, rd, op->mfn);
> >> > if ( !kind )
> >> > - err = iommu_unmap_page(ld, _dfn(mfn_x(op->mfn)));
> >> > + err = iommu_unmap_page(ld, _dfn(mfn_x(op->mfn)),
> >> > + PAGE_ORDER_4K);
> >> > else if ( !(kind & MAPKIND_WRITE) )
> >> > err = iommu_map_page(ld, _dfn(mfn_x(op->mfn)), op->mfn,
> >> > - IOMMUF_readable);
> >> > + PAGE_ORDER_4K, IOMMUF_readable);
> >> >
> >> > double_gt_unlock(lgt, rgt);
> >>
> >> I am, btw, uncertain that using PAGE_ORDER_4K is correct here:
> >> Other than in the IOMMU code, grant table code isn't tied to a
> >> particular architecture, and hence ought to work fine on a port
> >> to an architecture with 8k, 16k, or 32k pages.
> >
> > Would you suggest I add an arch specific #define for a grant table page
> > order and then use that?
>
> No, I'd prefer if you used liter 0 zero here.
>
Ok.
> >> > --- a/xen/drivers/passthrough/iommu.c
> >> > +++ b/xen/drivers/passthrough/iommu.c
> >> > @@ -305,47 +305,76 @@ void iommu_domain_destroy(struct domain *d)
> >> > }
> >> >
> >> > int iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
> >> > - unsigned int flags)
> >> > + unsigned int page_order, unsigned int flags)
> >> > {
> >> > const struct domain_iommu *hd = dom_iommu(d);
> >> > - int rc;
> >> > + unsigned long i;
> >> >
> >> > if ( !iommu_enabled || !hd->platform_ops )
> >> > return 0;
> >> >
> >> > - rc = hd->platform_ops->map_page(d, dfn, mfn, flags);
> >> > - if ( unlikely(rc) )
> >> > + ASSERT(!(dfn_x(dfn) & ((1ul << page_order) - 1)));
> >> > + ASSERT(!(mfn_x(mfn) & ((1ul << page_order) - 1)));
> >> > +
> >> > + for ( i = 0; i < (1ul << page_order); i++ )
> >> > {
> >> > + int ignored, err = hd->platform_ops->map_page(d,
> dfn_add(dfn,
> >> i),
> >> > + mfn_add(mfn,
> i),
> >> > + flags);
> >> > +
> >> > + if ( likely(!err) )
> >> > + continue;
> >> > +
> >> > if ( !d->is_shutting_down && printk_ratelimit() )
> >> > printk(XENLOG_ERR
> >> > "d%d: IOMMU mapping dfn %"PRI_dfn" to mfn
> %"PRI_mfn"
> >> failed: %d\n",
> >> > - d->domain_id, dfn_x(dfn), mfn_x(mfn), rc);
> >> > + d->domain_id, dfn_x(dfn_add(dfn, i)),
> >> > + mfn_x(mfn_add(mfn, i)), err);
> >> > +
> >> > + while (i--)
> >> > + /* assign to something to avoid compiler warning */
> >> > + ignored = hd->platform_ops->unmap_page(d, dfn_add(dfn,
> i));
> >>
> >> Hmm, as said on v1 - please use the original mode (while-if-continue)
> >> here. This lets you get away without a local variable that's never
> >> read, and which hence future compiler versions may legitimately warn
> >> about.
> >>
> >
> > Ok, I clearly don't understand what you mean by 'while-if-continue'
> then.
> > Above I have for-if-continue, which is what I thought you wanted. What
> code
> > structure are you actually looking for?
>
> The one your patch removes elsewhere:
>
> - while ( i-- )
> - /* If statement to satisfy __must_check. */
> - if ( iommu_unmap_page(p2m->domain,
> - dfn_add(dfn, i)) )
> - continue;
Oh, ok. Thanks. I'll change it.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
