Re: [Xen-devel] abstract model of IOMMU unmaping/mapping failures

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.03.16 at 11:06, <quan.xu@xxxxxxxxx> wrote:
> 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.

I wouldn't call this "correctly handled", but for the hardware domain
it should be good enough, and by crashing DomU-s simply reporting
the error up the call tree is sufficient. One question though is
whether the loops higher up the call tree in grant_table.c shouldn't
be exited when noticing the domain has crashed, both to avoid
unnecessary work and to reduce the risk of secondary problems.

> 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.

For the first portion of the quoted code you mean. I don't see any
rollback in the paging_mode_translate() case.

> 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)

Yes, at least error propagation needs to be added here. I don't
think more is required. (Be careful, btw, with adding warnings -
you must not spam the log with such.)

> ----
> 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.).

Indeed you can get here for Dom0 only when it's non-PV.

> 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.

Of course (and there's really little point in repeating the same
over and over again) - as long as no-one involved considers the
general model (set forth at the top) problematic, it can be done
the same way everywhere. Which still means (as said before)
that errors need to be propagated.

> 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.],

Please don't mis-quote me: I said this for a specific caller of the
function, not about the function itself.

> 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. 

And again - at least errors need to be propagated.

Jan

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