Re: [Xen-devel] [PATCH] IOMMU: don't BUG() on exotic hardware

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.16 at 09:55, <quan.xu@xxxxxxxxx> wrote:
> On May 06, 2016 10:24 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
>> hardware. While, with our current code, that's a correct prerequisite
>> assumption for IOMMU presence, this is wrong on systems without IOMMU.
>> Hence iommu_enabled (and alike) checks should be done prior to calling that
>> function, not after.
>> 
>> Also move iommu_suspend() next to iommu_resume() - it escapes me why
>> iommu_do_domctl() had got put between the two.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -337,11 +337,16 @@ int __init iommu_setup(void)
>>      return rc;
>>  }
>> 
>> +void iommu_suspend()
>> +{
>> +    if ( iommu_enabled )
>> +        iommu_get_ops()->suspend();
>> +}
>> +
> 
> 
> What about this style:
> 
> +void iommu_suspend()
> +{
> +    if ( iommu_enabled &&
> +         iommu_get_ops()->suspend )
> +        iommu_get_ops()->suspend();
> +}
> +

Where needed - sure. But I don't see the point in adding NULL
checks when the hook is required to be there.

>>  void iommu_crash_shutdown(void)
>>  {
>> -    const struct iommu_ops *ops = iommu_get_ops();
>>      if ( iommu_enabled )
>> -        ops->crash_shutdown();
>> +        iommu_get_ops()->crash_shutdown();
>>      iommu_enabled = iommu_intremap = iommu_intpost = 0;
> 
> btw, is this line still a code style issue?

Which one - the changed one or the context one? In the latter
case, even if there were a coding style issue (which I don't see)
correcting it wouldn't belong here.

Jan


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