[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping



On  April 19, 2016 2:44pm, Tian, Kevin <kevin.tian@xxxxxxxxx> wrote:
> > From: Xu, Quan
> > Sent: Monday, April 18, 2016 10:00 PM
> >
> > If IOMMU mapping and unmapping failed, the domain (with the exception
> > of the hardware domain) is crashed, treated as a fatal error. Rollback
> > can be lighter weight.
> 
> What do you mean by 'lighter weight"? Please clarify it.
> 
> >
> > For the hardware domain, we do things on a best effort basis. When
> > rollback is not feasible (in early initialization phase or trade-off
> > of complexity), at least, unmap upon maps error or then throw out error
> message.
> 
> remove 'or'. Based on next sentence, is above specific for IOMMU mapping?
> 
> >
> > IOMMU unmapping should perhaps continue despite an error, in an
> > attempt to do best effort cleanup.
> >



Could I enhance the commit log as below:
"""
If IOMMU mapping and unmapping failed, the domain (with the exception of the 
hardware domain) is crashed,
treated as a fatal error. Rollback can be lighter weight (at least errors need 
to be propagated).

IOMMU mapping breaks for an error, unmapping upon maps, throwing out error 
message and then reporting
the error up the call trees. When rollback is not feasible (in early 
initialization phase or trade-off of complexity)
for the hardware domain, we do things on a best effort basis, only throwing out 
error message.

IOMMU unmapping should perhaps continue despite an error, in an attempt to do 
best effort cleanup.
"""


I am still not sure whether we really need throw out error message for each 
IOMMU mapping or not.
If yes, I will throw out error message for each IOMMU mapping in next v3.


> > Signed-off-by: Quan Xu <quan.xu@xxxxxxxxx>
> >
> > CC: Keir Fraser <keir@xxxxxxx>
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> > CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> > CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/x86/mm.c               | 13 ++++++++-----
> >  xen/arch/x86/mm/p2m-ept.c       | 25 ++++++++++++++++++++++---
> >  xen/arch/x86/mm/p2m-pt.c        | 22 ++++++++++++++++++----
> >  xen/arch/x86/mm/p2m.c           | 11 +++++++++--
> >  xen/drivers/passthrough/iommu.c | 14 +++++++++++++-
> >  5 files changed, 70 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index
> > c997b53..5c4fb58 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> >                             int preemptible)  {
> >      unsigned long nx, x, y = page->u.inuse.type_info;
> > -    int rc = 0;
> > +    int rc = 0, ret = 0;
> >
> >      ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
> >
> > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> >          if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> >          {
> >              if ( (x & PGT_type_mask) == PGT_writable_page )
> > -                iommu_unmap_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)));
> > +                ret = iommu_unmap_page(d, mfn_to_gmfn(d,
> > + page_to_mfn(page)));
> >              else if ( type == PGT_writable_page )
> > -                iommu_map_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)),
> > -                               page_to_mfn(page),
> > -
> IOMMUF_readable|IOMMUF_writable);
> > +                ret = iommu_map_page(d, mfn_to_gmfn(d,
> page_to_mfn(page)),
> > +                                     page_to_mfn(page),
> > +
> > + IOMMUF_readable|IOMMUF_writable);
> >          }
> >      }
> >
> > @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info
> > *page, unsigned long type,
> >      if ( (x & PGT_partial) && !(nx & PGT_partial) )
> >          put_page(page);
> >
> > +    if ( unlikely(ret) )
> > +        rc = ret;
> > +
> >      return rc;
> >  }
> >
> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > index 3cb6868..22c8d17 100644
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned
> > long gfn, mfn_t mfn,
> >      ept_entry_t *table, *ept_entry = NULL;
> >      unsigned long gfn_remainder = gfn;
> >      unsigned int i, target = order / EPT_TABLE_ORDER;
> > -    int ret, rc = 0;
> > +    int ret, err = 0, rc = 0;
> >      bool_t direct_mmio = (p2mt == p2m_mmio_direct);
> >      uint8_t ipat = 0;
> >      bool_t need_modify_vtd_table = 1; @@ -830,10 +830,26 @@ out:
> >          {
> >              if ( iommu_flags )
> >                  for ( i = 0; i < (1 << order); i++ )
> > -                    iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> iommu_flags);
> > +                {
> > +                    ret = iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> > + iommu_flags);
> > +
> > +                    if ( unlikely(ret) )
> > +                    {
> > +                        while (i)
> > +                            iommu_unmap_page(d, gfn + --i);
> 
> How about below?
> 
> while (i-- >= 0)
>       iommu_unmap_page(d, gfn + i);
> 

this modification is based on discussion rooted at 
http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01779.html
wait for Jan's decision.


> > +
> > +                        err = ret;
> > +                        break;
> > +                    }
> > +                }
> >              else
> >                  for ( i = 0; i < (1 << order); i++ )
> > -                    iommu_unmap_page(d, gfn + i);
> > +                {
> > +                    ret = iommu_unmap_page(d, gfn + i);
> > +
> > +                    if ( unlikely(ret) )
> > +                        err = ret;
> > +                }
> >          }
> >      }
> >
> > @@ -849,6 +865,9 @@ out:
> >      if ( rc == 0 && p2m_is_hostp2m(p2m) )
> >          p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt,
> > p2ma);
> >
> > +    if ( unlikely(err) )
> > +        rc = err;
> > +
> 
> not sure we need three return values here: rc, ret, err...
> 

I am afraid that we need three return values here.
As similar as George mentioned, we need to make sure that the altp2m gets 
updated when the hostp2m is updated, even if the IOMMU mapping or unmapping 
fail.
if altp2m is enabled, rc cannot be non-zero here due to IOMMU mapping or 
unmapping, so I need another return value.

Quan


> >      return rc;
> >  }
> >
> > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index
> > 3d80612..db4257a 100644
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -498,7 +498,7 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> unsigned
> > long gfn, mfn_t mfn,
> >      l1_pgentry_t intermediate_entry = l1e_empty();
> >      l2_pgentry_t l2e_content;
> >      l3_pgentry_t l3e_content;
> > -    int rc;
> > +    int rc, ret;
> >      unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
> >      /*
> >       * old_mfn and iommu_old_flags control possible flush/update
> > needs on the @@ -680,11 +680,25 @@ p2m_pt_set_entry(struct
> p2m_domain
> > *p2m, unsigned long gfn, mfn_t mfn,
> >          }
> >          else if ( iommu_pte_flags )
> >              for ( i = 0; i < (1UL << page_order); i++ )
> > -                iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> > -                               iommu_pte_flags);
> > +            {
> > +                ret = iommu_map_page(p2m->domain, gfn + i,
> mfn_x(mfn) + i,
> > +                                     iommu_pte_flags);
> > +                if ( unlikely(ret) )
> > +                {
> > +                    while (i)
> > +                        iommu_unmap_page(p2m->domain, gfn + --i);
> > +
> > +                    rc = ret;
> > +                }
> > +            }
> >          else
> >              for ( i = 0; i < (1UL << page_order); i++ )
> > -                iommu_unmap_page(p2m->domain, gfn + i);
> > +            {
> > +                ret = iommu_unmap_page(p2m->domain, gfn + i);
> > +
> > +                if ( unlikely(ret) )
> > +                    rc = ret;
> > +            }
> >      }
> >
> >      /*
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index
> > b3fce1b..98450a4 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -610,13 +610,20 @@ p2m_remove_page(struct p2m_domain *p2m,
> unsigned
> > long gfn, unsigned long mfn,
> >      mfn_t mfn_return;
> >      p2m_type_t t;
> >      p2m_access_t a;
> > +    int ret = 0, rc;
> >
> >      if ( !paging_mode_translate(p2m->domain) )
> >      {
> >          if ( need_iommu(p2m->domain) )
> >              for ( i = 0; i < (1 << page_order); i++ )
> > -                iommu_unmap_page(p2m->domain, mfn + i);
> > -        return 0;
> > +            {
> > +               rc = iommu_unmap_page(p2m->domain, mfn + i);
> > +
> > +               if ( unlikely(rc) )
> > +                   ret = rc;
> > +            }
> > +
> > +        return ret;
> >      }
> >
> >      ASSERT(gfn_locked_by_me(p2m, gfn)); diff --git
> > a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index 850101b..c351209 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -172,6 +172,8 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
> >      {
> >          struct page_info *page;
> >          unsigned int i = 0;
> > +        int ret, rc = 0;
> > +
> >          page_list_for_each ( page, &d->page_list )
> >          {
> >              unsigned long mfn = page_to_mfn(page); @@ -182,10
> +184,20
> > @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> >                   ((page->u.inuse.type_info & PGT_type_mask)
> >                    == PGT_writable_page) )
> >                  mapping |= IOMMUF_writable;
> > -            hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > +            ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > +            if ( unlikely(ret) )
> > +                rc = ret;
> > +
> >              if ( !(i++ & 0xfffff) )
> >                  process_pending_softirqs();
> >          }
> > +
> > +        if ( rc )
> > +            printk(XENLOG_G_ERR
> > +                   "dom%d: IOMMU mapping is failed for hardware
> domain.",
> > +                   d->domain_id);
> >      }
> >
> >      return hd->platform_ops->hwdom_init(d);
> > --
> > 1.9.1


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.