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

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

On May 04, 2016 9:44 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>> 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.
>

If no other suggestions, I'll take Kevin's suggestion as a final  conclusion 
for both iommu_map_page() and iommu_unmap_page().
 

Quan

> > 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.


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