| Message ID | 1460988011-17758-3-git-send-email-quan.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Quan Xu > Sent: Monday, April 18, 2016 10:00 PM > > Now IOMMU mapping and unmapping failures are treated as a fatal to > the domain (with the exception of the hardware domain). 'Now' is more about eixsting state, while it's actually what you want to change towards. Better directly say "Treat IOMMU mapping...". > > 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> > > CC: Jan Beulich <jbeulich@suse.com> Otherwise looks OK to me. Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On April 19, 2016 2:37pm, Tian, Kevin <kevin.tian@intel.com> wrote: > > From: Quan Xu > > Sent: Monday, April 18, 2016 10:00 PM > > > > Now IOMMU mapping and unmapping failures are treated as a fatal to the > > domain (with the exception of the hardware domain). > > 'Now' is more about eixsting state, while it's actually what you want to change > towards. Better directly say "Treat IOMMU mapping...". > Agreed. I'll modify it in next v3. Thanks for your review!! Quan > > > > 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> > > > > CC: Jan Beulich <jbeulich@suse.com> > > Otherwise looks OK to me. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>> On 18.04.16 at 16:00, <quan.xu@intel.com> wrote: > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -243,21 +243,33 @@ 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 && !is_hardware_domain(d) ) > + domain_crash(d); > + > + return rc; > } As said before - letting this go completely silently for the hardware domain is bad. At least the first instance of such an event needs a message to be logged. Advanced variants where a message gets logged once in a while if the issue re-occurs would be nice, but aren't strictly necessary imo. And note that even logging all occurrences would not be a security issue, but just a usability one (but I still recommend against this). Jan
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index b64676f..850101b 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -243,21 +243,33 @@ 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 && !is_hardware_domain(d) ) + 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 && !is_hardware_domain(d) ) + domain_crash(d); + + return rc; } static void iommu_free_pagetables(unsigned long unused)
Now IOMMU mapping and unmapping failures are treated 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> CC: Jan Beulich <jbeulich@suse.com> --- xen/drivers/passthrough/iommu.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)