+to 2 key maintainers,
On March 31, 2016 5:06pm, Xu, Quan <quan...@intel.com> wrote:
> All,
>
> Here is a summary of my investigation of the abstract model:
>
> Below policies are adopted when deciding whether to rollback a callchain:
>
> 1. Domain will be crashed immediately within iommu_{,un}map_page, treated
> as a fatal error (with the exception of the hardware one). Whether to rollback
> depends on the need of hardware domain;
>
> 2. For hardware domain, roll back 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 and then throw out error message;
>
> Below are a detail analysis of all existing callers on IOMMU interfaces (8-11
> needs more discussions):
>
> ----
> 1. memory_add(): rollback (no change)
>
> (Existing code)
> >>...
> if ( ret )
> goto destroy_m2p;
>
> if ( iommu_enabled && !iommu_passthrough
> && !need_iommu(hardware_domain) )
> {
> for ( i = spfn; i < epfn; i++ )
> if ( iommu_map_page(hardware_domain, i, i,
> IOMMUF_readable|IOMMUF_writable) )
> break;
> if ( i != epfn )
> {
> while (i-- > old_max)
> iommu_unmap_page(hardware_domain, i);
> goto destroy_m2p;
> }
> }
> <<...
> Existing code already rolls back through destroy_m2p cleanly.
>
> ----
> 2. vtd_set_hwdom_mapping(): no rollback (no change).
>
> It is in early initialization phase. A quick thought looks a bit tricky to
> fully
> rollback this path, so propose to leave as it is.
>
> ----
> 3. __gnttab_map_grant_ref():rollback (no change)
>
>
> (Existing code)
> >>...
> if ( (act_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) &&
> !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
> {
> if ( !(kind & MAPKIND_WRITE) )
> err = iommu_map_page(ld, frame, frame,
>
> IOMMUF_readable|IOMMUF_writable);
> }
> else if ( act_pin && !old_pin )
> {
> if ( !kind )
> err = iommu_map_page(ld, frame, frame,
> IOMMUF_readable);
> }
> if ( err )
> {
> double_gt_unlock(lgt, rgt);
> rc = GNTST_general_error;
> goto undo_out;
> }
> <<...
>
> rollback is already cleanly handled.
>
> ----
> 4. __gnttab_unmap_common():rollback (no change)
>
> (Existing code)
> >>...
> if ( !kind )
> err = iommu_unmap_page(ld, op->frame);
> else if ( !(kind & MAPKIND_WRITE) )
> err = iommu_map_page(ld, op->frame, op->frame,
> IOMMUF_readable);
>
> double_gt_unlock(lgt, rgt);
>
> if ( err )
> rc = GNTST_general_error;
> <<...
>
> Similarly no change required, as error has been correctly handled.
>
> ----
> 5. guest_physmap_add_entry():rollback (no change)
>
> (Existing code)
> >>...
> rc = iommu_map_page(
> d, mfn + i, mfn + i, IOMMUF_readable|IOMMUF_writable);
> if ( rc != 0 )
> {
> while ( i-- > 0 )
> iommu_unmap_page(d, mfn + i);
> return rc;
> }
> <<...
>
> Above is in the start of the function, so no additional state to be cleared
> thus it
> is in good shape too.
>
> ----
> 6. clear_identity_p2m_entry():rollback (no change).
>
> (Existing code)
> >>...
> if ( !paging_mode_translate(d) )
> {
> if ( !need_iommu(d) )
> return 0;
> return iommu_unmap_page(d, gfn);
> }
> <<...
>
> in the start, and error already rolled back to caller.
>
> ----
> 7. set_identity_p2m_entry():rollback (no change).
>
> (Existing code)
> >>...
> if ( !paging_mode_translate(p2m->domain) )
> {
> if ( !need_iommu(d) )
> return 0;
> return iommu_map_page(d, gfn, gfn,
> IOMMUF_readable|IOMMUF_writable);
> }
> <<...
> if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
> ret = iommu_map_page(d, gfn, gfn,
> IOMMUF_readable|IOMMUF_writable);
> >> ...
>
> error handling and rollback already in-place.
>
> ----
> 8. p2m_remove_page():rollback one level(change required).
>
> (Existing code)
> >>...
> 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;
> }
> <<...
>
> There is no error checking of iommu_unmap_page. We need to add.
> However, there are several callers of this function which don't handle error
> at
> all. I'm not sure whether hardware domain will use this function.
> Since it is in core p2m logic (which I don't have much knowledge), I hope we
> can
> print a warning and simply return error here (given the fact that non-hardware
> domains are all crashed immediately within unmap call)
>
> ----
> 9. p2m_pt_set_entry(): open (propose no-rollback).
>
> (Existing code)
> >>...
> for ( i = 0; i < (1UL << page_order); i++ )
> iommu_map_page(p2m->domain, gfn + i, mfn_x(mfn) + i,
> iommu_pte_flags);
> else
> for ( i = 0; i < (1UL << page_order); i++ )
> iommu_unmap_page(p2m->domain, gfn + i);
> <<...
>
> Above is in the end of the func.
>
> I'm not sure whether it will be called for hardware domain ( PVH mode is one
> potential concern. as we know, PVH has been introduced on Xen 4.4 as a DomU,
> and on Xen 4.5 as a Dom0.).
>
> If not, we can leave it as today. Otherwise, I want to hear your suggestion
> whether we can use no-rollback (just erring) for hardware domain here since
> this code path is very complex.
>
> 10. ept_set_entry(): open (propose no-rollback).
>
> (Existing code)
> >>...
> if ( iommu_flags )
> for ( i = 0; i < (1 << order); i++ )
> iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> iommu_flags);
> else
> for ( i = 0; i < (1 << order); i++ )
> iommu_unmap_page(d, gfn + i);
> <<...
>
> Above is in the end of the func.
>
> Same open as 9) whether we can use no-rollback here.
>
> ----
> 11. __get_page_type(): open (propose no-rollback).
> The following is in detail:
> >>...
> 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)));
> 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);
> }
> <<...
>
> It's almost in the end of the func.
> As the limited set of cases where __get_page_type() calls
> iommu_{,un}map_page(), and a get_page_type() invocation that previously was
> known to never fail (or at least so we hope, based on the existing code) [I
> got it
> from Jan's reply.], we can classify it as normal PV domain and be treated as a
> fatal error.
> So for I am inclined to leave it as today.
>
>
> I welcome your comments and feedback!!
>
> Quan
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
