>>> 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.
>> > --- 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;
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
