Re: [Xen-devel] [PATCH] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.10.18 at 17:52, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 26 October 2018 16:46
>> 
>> >>> On 17.10.18 at 10:19, <paul.durrant@xxxxxxxxxx> wrote:
>> >      {
>> > -        if ( !d->is_shutting_down && printk_ratelimit() )
>> > -            printk(XENLOG_ERR
>> > -                   "d%d: IOMMU unmapping dfn %"PRI_dfn" failed: %d\n",
>> > -                   d->domain_id, dfn_x(dfn), rc);
>> > +        int rc = hd->platform_ops->unmap_page(d, dfn_add(dfn, i));
>> >
>> > -        if ( !is_hardware_domain(d) )
>> > -            domain_crash(d);
>> > +        if ( unlikely(rc) )
>> > +        {
>> > +            if ( !d->is_shutting_down && printk_ratelimit() )
>> > +                printk(XENLOG_ERR
>> > +                       "d%d: IOMMU unmapping dfn %"PRI_dfn" failed:
>> %d\n",
>> > +                       d->domain_id, dfn_x(dfn), rc);
>> > +
>> > +            if ( !is_hardware_domain(d) )
>> > +                domain_crash(d);
>> > +
>> > +            return rc;
>> 
>> This is not in line with the code you drop: Unmap should continue
>> the loop, and just record (and then report) the first (if any) error,
>> to be on the safe side if callers assume the unmap won't fail (which
>> can't be excluded despite the __must_check, as the code in
>> iommu_map_page() shows). Perhaps the logging then should also
>> be restricted to just the first case of error, and in any event please
>> with the correct dfn (or with the order also logged).
>> 
> 
> Ok. I guess unmapping as much as is possible for the h/w domain is 
> reasonable. Any other domain will of course be crashed, so it would seem 
> better to bail out of the loop early in that case.

Fine with me.

Jan




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel