|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |