Re: [Xen-devel] [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.16 at 08:54, <quan.xu@xxxxxxxxx> wrote:
> On March 17, 2016 8:30pm, <dunlapg@xxxxxxxxx> wrote:
>> On Thu, Mar 17, 2016 at 6:54 AM, Quan Xu <quan.xu@xxxxxxxxx> wrote:
>> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index
>> > c997b53..526548e 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 ( !rc )
>> > +        rc = ret;
>> > +
>> 
>> What's this about?  If the iommu_[un]map_page() operation times out,
>> we still go through with calling alloc_page_type(); and if
>> alloc_page_type() fails we return its failure value, but if it
>> succeeds, we return the error from iommu_[un]map_page()?
>> 
> Yes.
> To be honest, to me, it is tricky too. 
> I found it is not right to return directly if the iommu_[un]map_page() 
> operation times out.
> 
>  """if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )"""
> Does IOMMU support pv domain? If not, we'd better remove the " if(...){...}"

What strange a question: Of course it does.

As you can infer form the reply I sent yesterday, you first need to
settle on an abstract model: What do failures mean? If, in the flush
case, a timeout is going to kill the domain anyway, then error
handling can be lighter weight than if you mean to try to keep the
domain running. Of course in this context you also should not forget
that iommu_map_page() could already return errors prior to your
changes (most notably -ENOMEM, but at least the AMD side also
produces others, with -EFAULT generally being accompanied by
domain_crash()). As mentioned elsewhere - it seems extremely
bogus that these errors weren't check for from the beginning.

Jan


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