Re: [Xen-devel] [PATCH v8 02/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Mon, Jun 13, 2016 at 4:17 PM, Xu, Quan <quan.xu@xxxxxxxxx> wrote:
> From: Quan Xu <quan.xu@xxxxxxxxx>
>
> When IOMMU mapping is failed, we issue a best effort rollback, stopping
> IOMMU mapping, unmapping the previous IOMMU maps and then reporting the
> error up to the call trees. When rollback is not feasible (in early
> initialization phase or trade-off of complexity) for the hardware domain,
> we do things on a best effort basis, only throwing out an error message.
>
> IOMMU unmapping should perhaps continue despite an error, in an attempt
> to do best effort cleanup.
>
> Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>

Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Phew!

One comment...

> +                        while ( i-- )
> +                            /*
> +                             * IOMMU unmapping should perhaps continue 
> despite an
> +                             * error in an attempt to do best effort 
> cleanup, and
> +                             * consume the error as __must_check annotation.
> +                             */
> +                            if ( iommu_unmap_page(p2m->domain, gfn + i) )
> +                                continue;

I'd take out the "perhaps", (since there's no 'perhaps' about it) but
other than that I think this comment is fine.

It sounds like Jan had something more along the following in mind:

/* If statement to satisfy __must_check */

Either one works.  The shorter one is sufficient, but the longer one
isn't too much I don't think.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel