abstract model of IOMMU unmaping/mapping failures

["Xu, Quan" <quan.xu@intel.com>]

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