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

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

> On February 11, 2016 1:01am, Jan Beulich <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).
> > @@ -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.)
> 
While I read the code/email again and again,
I think I'd better _not_ return error value. Then the below modifications are 
pointless:
1.
-    void (*hwdom_init)(struct domain *d);
+    int (*hwdom_init)(struct domain *d);

2. 
-void iommu_hwdom_init(struct domain *d);
+int iommu_hwdom_init(struct domain *d);


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

It is similar as the above case.
I think I'd better _not_ return error value. Then the below modifications are 
pointless:
1.
-    void (*crash_shutdown)(void);
+    int (*crash_shutdown)(void);

2.
-void amd_iommu_crash_shutdown(void);
+int amd_iommu_crash_shutdown(void);

3. 
-void iommu_crash_shutdown(void);
+int iommu_crash_shutdown(void);


Right?

Quan

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