Re: [Xen-devel] [PATCH v5 1/7] VT-d: Check VT-d Device-TLB flush error(IOMMU part).

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.02.16 at 11:50, <quan.xu@xxxxxxxxx> wrote:
>>  On February 11, 2016 at 1:01am, <JBeulich@xxxxxxxx> wrote:
>> >>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:
>> > This patch checks all kinds of error and all the way up the call trees
>> > of VT-d Device-TLB flush(IOMMU part).
> 
>> Please be sure to Cc all maintainers of code you modify.
>> 
> OK, also CCed Dario Faggioli.
> Jan, could I re-send out the V5 and cc all maintainers of code I modify?

I'd say rather make sure you Cc the right people on v6, to avoid
spamming everyone's mailboxes.

>> > @@ -2737,8 +2739,9 @@ static int arm_smmu_iommu_domain_init(struct
>> domain *d)
>> >    return 0;
>> >  }
>> >
>> > -static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>> > +static int __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>> >  {
>> > +        return 0;
>> >  }
>> 
>> Bad indentation.
>> 
> 
> I will modify it in next version. I tried to align with the coding style, as 
> 
> There was similar indentation nearby arm_smmu_iommu_hwdom_init() in that 
> file.

Then I'm afraid you didn't check enough of the properties of the
file: It appears to use Linux style, and hence indentation is by
tabs instead of spaces.

>> > @@ -171,7 +172,10 @@ 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);
>> > +            rc = hd->platform_ops->map_page(d, gfn, mfn, mapping);
>> > +            if ( rc )
>> > +                return rc;
>> 
>> So in this patch I can't find error checking getting introduced for the 
> caller of
>> this function. And if you were to add it - what would your intentions be? 
> Panic?
>> IOW I'm not sure bailing here is a good idea. Logging an error message (but 
> only
>> once in this loop) would be a possibility. (This pattern repeats further 
> down.)
>> 
> 
> Could I log an error message and break in this loop?

Logging an error message - yes. Breaking the loop - no; this should
continue to be a best effort attempt at getting things set up for
Dom0.

>> > @@ -369,12 +376,16 @@ void iommu_share_p2m_table(struct domain* d)
>> >          ops->share_p2m(d);
>> >  }
>> >
>> > -void iommu_crash_shutdown(void)
>> > +int iommu_crash_shutdown(void)
>> >  {
>> >      const struct iommu_ops *ops = iommu_get_ops();
>> > +
>> >      if ( iommu_enabled )
>> > -        ops->crash_shutdown();
>> > +        return ops->crash_shutdown();
>> > +
>> >      iommu_enabled = iommu_intremap = 0;
>> > +
>> > +    return 0;
>> >  }
>> 
>> Here again the question is - what is the error value going to be used for? 
>> We're
>> trying to shut down a crashed system when coming here.
>> 
>  I tried to clean up in error handling path chained up. It logs an error 
> message,
>  When it calls iommu_crash_shutdown() and returns a non-zero value [in patch 
> 2/7].

That sounds okay than (I didn't get around to look at patches 2-7
yet), but is somewhat contrary to me request of adding
__must_check as far as possible, which - if done here - would
break the build without also adjusting the caller(s).

>> > @@ -584,37 +587,37 @@ static void __intel_iommu_iotlb_flush(struct
>> domain *d, unsigned long gfn,
>> >              continue;
>> >
>> >          if ( page_count > 1 || gfn == -1 )
>> > -        {
>> > -            if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
>> > -                        0, flush_dev_iotlb) )
>> > -                iommu_flush_write_buffer(iommu);
>> > -        }
>> > +            rc = iommu_flush_iotlb_dsi(iommu, iommu_domid,
>> > +                        0, flush_dev_iotlb);
>> >          else
>> > -        {
>> > -            if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
>> > +            rc = iommu_flush_iotlb_psi(iommu, iommu_domid,
>> >                          (paddr_t)gfn << PAGE_SHIFT_4K, 0,
>> > -                        !dma_old_pte_present, flush_dev_iotlb) )
>> > -                iommu_flush_write_buffer(iommu);
>> > -        }
>> > +                        !dma_old_pte_present, flush_dev_iotlb);
>> > +
>> > +        if ( rc )
>> > +            iommu_flush_write_buffer(iommu);
>> >      }
>> > +
>> > +    return rc;
>> >  }
>> 
>> rc may be positive here (and afaict that's the indication to do the flush, 
> not one
>> of failure). Quite likely this code didn't mean to flush
>> iommu_flush_iotlb_{d,p}si() returning a negative value...
>> 
>> 
> IIUC, you refer to the follow:
>  flush_iotlb_qi(
> {
> ...
>           if ( !cap_caching_mode(iommu->cap) )
>             return 1;
> ...
> }
> 
> 
> As Kevin mentioned, originally 0/1 is used to indicate whether caller needs 
> to flush cache.
> But I try to return ZERO in [PATCH v5 1/7]. Because:
> 1) if It returns _1_, device passthrough doesn't work when I tried to clean 
> up in error handling path chained up. 
> 2) as VT-d spec said, Caching Mode is 0: invalidations are not required for 
> modifications to individual not present
>   or invalid entries.
> 
> That is tricky for this point. Correct me if I am wrong.

I'm not sure I see the trickiness: All you need is
- call iommu_flush_write_buffer() only when rc > 0 (if I remember
  the polarity right, else it might be rc == 0)
- return zero from this function when rc is positive.

Jan

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