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

["Xu, Quan" <quan.xu@xxxxxxxxx>]

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.


I remain neutral for this point. I think this is not a technical problem, but a 
matter of preference.
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.


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