Re: [Xen-devel] [Patch v6 04/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU mapping (top level ones)

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.16 at 09:25, <quan.xu@xxxxxxxxx> wrote:
> On June 01, 2016 6:24 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 31.05.16 at 15:57, <quan.xu@xxxxxxxxx> wrote:
>  As would a respective change to vtd_set_hwdom_mapping(), which
>> I'm not sure which patch you've put that in.
>> 
> 
> Sorry,  I missed it. I indeed it need to fix it as similar as above.
> I wonder whether I could add a __must_check annotation to iommu_map_page() 
> or not, as which may be inconsistent with iommu_unmap_page().

Urgh - are you saying that by the end of the series they aren't
_both_ __must_check? Then I must have overlooked something
while reviewing: They definitely both ought to be. Or wait - I've
pointed this out in the context of this patch, still seen below.

>> > --- a/xen/include/xen/iommu.h
>> > +++ b/xen/include/xen/iommu.h
>> > @@ -166,8 +166,8 @@ struct iommu_ops {  #endif /* HAS_PCI */
>> >
>> >      void (*teardown)(struct domain *d);
>> > -    int (*map_page)(struct domain *d, unsigned long gfn, unsigned long 
>> > mfn,
>> > -                    unsigned int flags);
>> > +    int __must_check (*map_page)(struct domain *d, unsigned long gfn,
>> > +                                 unsigned long mfn, unsigned int
>> > + flags);
>> 
>> With this and with the rule set forth in the context of the discussion of 
> v5,
>> iommu_map_page() (as well as any other caller of this hook that do not
>> themselves _consume_ the error [e.g. hwdom init ones]) should become or
>> already be __must_check, which afaict isn't the case.
> 
> But does this rule also apply to these 'void' annotation functions?  .e.g, 
> in the call tree of hwdom init ones / domain crash ones, we are no need to 
> bubble up
> error code, leaving these void annotation as is.

Note that my previous reply already answered that question (as I
expected you would otherwise ask): I specifically excluded those
functions that _consume_ the error, and I did give an example. I
really don't know what else I could have done to make clear what
exceptions are to be expected.

>> The same then, btw.,
>> applies to patch 3, and hence I have to withdraw the R-b that you've got
>> there.
> 
> I find these callers are grant_table/mm, and we limit __must_check 
> annotation to IOMMU functions for this patch set..

Talk isn't of those ones. The subject of patch 3 is unmapping, and
hence the parallel to the one here is that iommu_unmap_page()
needs to become __must_check there, along with the iommu_ops
unmap_page() hook.

> So I think I can remain R-b as is for patch 3. 

No.

Jan


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