>>> On 23.02.16 at 12:43, <quan...@intel.com> wrote:
>> On February 11, 2016 1:01am, Jan Beulich <jbeul...@suse.com> wrote:
>> >>> On 05.02.16 at 11:18, <quan...@intel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
