Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
> -Original Message-
> From: Roger Pau Monne
> Sent: 30 October 2018 14:44
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Kevin Tian ;
> Stefano Stabellini ; Wei Liu
> ; Jun Nakajima ; Konrad
> Rzeszutek Wilk ; Andrew Cooper
> ; Ian Jackson ; George
> Dunlap ; Tim (Xen.org) ; Julien
> Grall ; Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order
> parameter to iommu_map/unmap_page()
>
> On Mon, Oct 29, 2018 at 01:29:28PM +, Paul Durrant wrote:
> > The P2M code currently contains many loops to deal with the fact that,
> > while it may be require to handle page orders greater than 4k, the
> > IOMMU map and unmap functions do not.
> > This patch adds a page_order parameter to those functions and implements
> > the necessary loops within. This allows the P2M code to be substantially
> > simplified.
> >
> > NOTE: This patch does not modify the underlying vendor IOMMU
> > implementations to deal with page orders of more than 4k.
>
> I'm wondering if it would make sense to drop the _page suffix from
> those functions now that they take an order parameter.
Yes, that might well be a good idea at this point since I have to hit all the
call-sites anyway.
>
> > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > index 8b438ae4bc..e02dcb101f 100644
> > --- 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)));
>
> I would consider using IS_ALIGNED for clarity.
>
Ok.
> > +
> > +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--)
>
> Missing spaces in the condition.
>
Yes. Jan mentioned this too and I forgot to fix it.
> > +/* assign to something to avoid compiler warning */
> > +ignored = hd->platform_ops->unmap_page(d, dfn_add(dfn, i));
>
> You could likely declare ignored here to further limit it's scope?
>
I'll re-work as Jan prefers.
> >
> > if ( !is_hardware_domain(d) )
> > domain_crash(d);
> > +
> > +return err;
>
> I might prefer to keep the global rc variable here and just break on
> error, also keeping the 'return rc' below as it was. But that's just a
> question of taste IMO.
>
Ok. I'll see what that looks like. It might be nicer.
Paul
> Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 30 October 2018 17:05
> To: Paul Durrant
> Cc: Julien Grall ; Andrew Cooper
> ; George Dunlap ; Ian
> Jackson ; Wei Liu ; Jun
> Nakajima ; Kevin Tian ;
> Stefano Stabellini ; xen-devel de...@lists.xenproject.org>; Konrad Rzeszutek Wilk
> ; Tim (Xen.org)
> Subject: RE: [PATCH v2] iommu / p2m: add a page_order parameter to
> iommu_map/unmap_page()
>
> >>> On 30.10.18 at 17:56, wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 30 October 2018 16:08
> >>
> >> >>> On 29.10.18 at 14:29, 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)) )
> -
Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
>>> On 30.10.18 at 17:56, wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 30 October 2018 16:08
>>
>> >>> On 29.10.18 at 14:29, 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
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 30 October 2018 16:08
> To: Paul Durrant
> Cc: Julien Grall ; Andrew Cooper
> ; George Dunlap ; Wei
> Liu ; Ian Jackson ; Jun
> Nakajima ; Kevin Tian ;
> Stefano Stabellini ; xen-devel de...@lists.xenproject.org>; Konrad Rzeszutek Wilk
> ; Tim (Xen.org)
> Subject: Re: [PATCH v2] iommu / p2m: add a page_order parameter to
> iommu_map/unmap_page()
>
> >>> On 29.10.18 at 14:29, 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?
>
> > --- 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?
Paul
> Jan
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
>>> On 29.10.18 at 14:29, 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.
> --- 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.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()
On Mon, Oct 29, 2018 at 01:29:28PM +, Paul Durrant wrote:
> The P2M code currently contains many loops to deal with the fact that,
> while it may be require to handle page orders greater than 4k, the
> IOMMU map and unmap functions do not.
> This patch adds a page_order parameter to those functions and implements
> the necessary loops within. This allows the P2M code to be substantially
> simplified.
>
> NOTE: This patch does not modify the underlying vendor IOMMU
> implementations to deal with page orders of more than 4k.
I'm wondering if it would make sense to drop the _page suffix from
those functions now that they take an order parameter.
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 8b438ae4bc..e02dcb101f 100644
> --- 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)));
I would consider using IS_ALIGNED for clarity.
> +
> +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--)
Missing spaces in the condition.
> +/* assign to something to avoid compiler warning */
> +ignored = hd->platform_ops->unmap_page(d, dfn_add(dfn, i));
You could likely declare ignored here to further limit it's scope?
>
> if ( !is_hardware_domain(d) )
> domain_crash(d);
> +
> +return err;
I might prefer to keep the global rc variable here and just break on
error, also keeping the 'return rc' below as it was. But that's just a
question of taste IMO.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
