| Message ID | 1461921917-48394-3-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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@intel.com> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > CC: Jan Beulich <jbeulich@suse.com> > --- > 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); Thanks Kevin
On May 04, 2016 9:29 AM, Tian, Kevin <kevin.tian@intel.com> 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@intel.com> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > > > CC: Jan Beulich <jbeulich@suse.com> > > --- > > 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
>>> On 04.05.16 at 13:49, <quan.xu@intel.com> wrote: > On May 04, 2016 9:29 AM, Tian, Kevin <kevin.tian@intel.com> 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@intel.com> >> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> >> > >> > CC: Jan Beulich <jbeulich@suse.com> >> > --- >> > 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
On May 04, 2016 9:44 PM, Jan Beulich <JBeulich@suse.com> wrote: > >>> On 04.05.16 at 13:49, <quan.xu@intel.com> wrote: > > On May 04, 2016 9:29 AM, Tian, Kevin <kevin.tian@intel.com> 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@intel.com> > >> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > >> > > >> > CC: Jan Beulich <jbeulich@suse.com> > >> > --- > >> > 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.
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); + } + + return rc; } int iommu_unmap_page(struct domain *d, unsigned long gfn) { struct hvm_iommu *hd = domain_hvm_iommu(d); + int rc; if ( !iommu_enabled || !hd->platform_ops ) return 0; - return hd->platform_ops->unmap_page(d, gfn); + rc = hd->platform_ops->unmap_page(d, gfn); + + if ( rc ) + { + if ( is_hardware_domain(d) ) + printk(XENLOG_ERR + "iommu_unmap_page: IOMMU unmapping gfn %#lx failed for dom%d.", + gfn, d->domain_id); + else + domain_crash(d); + } + + return rc; } static void iommu_free_pagetables(unsigned long unused)