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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.16 at 12:43, <quan.xu@xxxxxxxxx> wrote:
>>  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);

Agreed. Just log some message for failure in the case above, but
make sure that is (a) useful for debugging and (b) not one message
per page.

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

Indeed, same as above.

Jan


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