Re: [Xen-devel] [PATCH v3 02/10] IOMMU: handle IOMMU mapping and unmapping failures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.05.16 at 13:49, <quan.xu@xxxxxxxxx> wrote:
> On May 04, 2016 9:29 AM, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
>> > From: Quan Xu
>> > Sent: Friday, April 29, 2016 5:25 PM
>> >
>> > Treat IOMMU mapping and unmapping failures as a fatal to the domain
>> > (with the exception of the hardware domain).
>> >
>> > If IOMMU mapping and unmapping failed, crash the domain (with the
>> > exception of the hardware domain) and propagate the error up to the
>> > call trees.
>> >
>> > Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
>> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>> >
>> > CC: Jan Beulich <jbeulich@xxxxxxxx>
>> > ---
>> >  xen/drivers/passthrough/iommu.c | 30 ++++++++++++++++++++++++++++--
>> >  1 file changed, 28 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/xen/drivers/passthrough/iommu.c
>> > b/xen/drivers/passthrough/iommu.c index b64676f..a0003ac 100644
>> > --- a/xen/drivers/passthrough/iommu.c
>> > +++ b/xen/drivers/passthrough/iommu.c
>> > @@ -243,21 +243,47 @@ int iommu_map_page(struct domain *d,
>> unsigned
>> > long gfn, unsigned long mfn,
>> >                     unsigned int flags)  {
>> >      struct hvm_iommu *hd = domain_hvm_iommu(d);
>> > +    int rc;
>> >
>> >      if ( !iommu_enabled || !hd->platform_ops )
>> >          return 0;
>> >
>> > -    return hd->platform_ops->map_page(d, gfn, mfn, flags);
>> > +    rc = hd->platform_ops->map_page(d, gfn, mfn, flags);
>> > +
>> > +    if ( rc )
>> > +    {
>> > +        if ( is_hardware_domain(d) )
>> > +            printk(XENLOG_ERR
>> > +                   "iommu_map_page: IOMMU mapping gfn %#lx mfn %#lx
>> > + failed for
>> > dom%d.",
>> > +                   gfn, mfn, d->domain_id);
>> > +        else
>> > +            domain_crash(d);
>> > +    }
>> 
>> It makes sense to print error messages for all domains, and then selectively
>> crash domain:
>> 
>> printk(XENLOG_ERR ...);
>> if ( !is_hardware_domain(d) )
>>      domain_crash(d);
>> 
> 
> But Jan said,
> ..
>     if ( is_hardware_domain() )
>         printk();
>     else
>         domain_crash();
> ..
> is the better approach.

Not exactly. All I complained about was that the Dom0 case went
completely silently.

> I remain neutral for this point. I think this is not a technical problem, 
> but a matter of preference.

Indeed.

> This gap is subject to preventing spamming the log. 
> 
> For iommu_map_page(), I think Kevin's suggestion is better, much more 
> information for the crash domain,
> And also won't spam the log as we stop mapping against any error.
> 
> For iommu_unmap_page(),IOMMU unmapping should perhaps continue despite an 
> error, in an attempt
> to do best effort cleanup. Then, Kevin's suggestion may spam the log.

But I've always been saying that for multiple successive operations
you shouldn't issue one message each.

Jan


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