* [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init()
@ 2016-06-14 9:03 Jan Beulich
2016-06-14 9:09 ` Andrew Cooper
2016-06-16 2:03 ` Xu, Quan
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2016-06-14 9:03 UTC (permalink / raw)
To: xen-devel; +Cc: Suravee Suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 2541 bytes --]
... instead of using -ENODEV for any kind of error. It in particular
addresses Coverity ID 1362694 (introduced by commit eb48587210 ["AMD
IOMMU: introduce support for IVHD block type 11h"]).
Coverity ID: 1362694
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1226,6 +1226,7 @@ static bool_t __init amd_sp5100_erratum2
int __init amd_iommu_init(void)
{
struct amd_iommu *iommu;
+ int rc = -ENODEV;
BUG_ON( !iommu_found() );
@@ -1237,28 +1238,39 @@ int __init amd_iommu_init(void)
if ( unlikely(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_MSI) )
goto error_out;
- ivhd_type = amd_iommu_get_supported_ivhd_type();
- if ( ivhd_type < 0 )
+ rc = amd_iommu_get_supported_ivhd_type();
+ if ( rc < 0 )
goto error_out;
+ ivhd_type = rc;
- ivrs_bdf_entries = amd_iommu_get_ivrs_dev_entries();
- if ( !ivrs_bdf_entries )
+ rc = amd_iommu_get_ivrs_dev_entries();
+ if ( !rc )
+ rc = -ENODEV;
+ if ( rc < 0 )
goto error_out;
+ ivrs_bdf_entries = rc;
radix_tree_init(&ivrs_maps);
for_each_amd_iommu ( iommu )
- if ( alloc_ivrs_mappings(iommu->seg) != 0 )
+ {
+ rc = alloc_ivrs_mappings(iommu->seg);
+ if ( rc )
goto error_out;
+ }
- if ( amd_iommu_update_ivrs_mapping_acpi() != 0 )
+ rc = amd_iommu_update_ivrs_mapping_acpi();
+ if ( rc )
goto error_out;
/* initialize io-apic interrupt remapping entries */
- if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )
+ if ( iommu_intremap )
+ rc = amd_iommu_setup_ioapic_remapping();
+ if ( rc )
goto error_out;
/* allocate and initialize a global device table shared by all iommus */
- if ( iterate_ivrs_mappings(amd_iommu_setup_device_table) != 0 )
+ rc = iterate_ivrs_mappings(amd_iommu_setup_device_table);
+ if ( rc )
goto error_out;
/*
@@ -1271,14 +1283,17 @@ int __init amd_iommu_init(void)
/* per iommu initialization */
for_each_amd_iommu ( iommu )
- if ( amd_iommu_init_one(iommu) != 0 )
+ {
+ rc = amd_iommu_init_one(iommu);
+ if ( rc )
goto error_out;
+ }
return 0;
error_out:
amd_iommu_init_cleanup();
- return -ENODEV;
+ return rc;
}
static void disable_iommu(struct amd_iommu *iommu)
[-- Attachment #2: CID1362694.patch --]
[-- Type: text/plain, Size: 2598 bytes --]
AMD IOMMU: correctly propagate errors from amd_iommu_init()
... instead of using -ENODEV for any kind of error. It in particular
addresses Coverity ID 1362694 (introduced by commit eb48587210 ["AMD
IOMMU: introduce support for IVHD block type 11h"]).
Coverity ID: 1362694
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1226,6 +1226,7 @@ static bool_t __init amd_sp5100_erratum2
int __init amd_iommu_init(void)
{
struct amd_iommu *iommu;
+ int rc = -ENODEV;
BUG_ON( !iommu_found() );
@@ -1237,28 +1238,39 @@ int __init amd_iommu_init(void)
if ( unlikely(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_MSI) )
goto error_out;
- ivhd_type = amd_iommu_get_supported_ivhd_type();
- if ( ivhd_type < 0 )
+ rc = amd_iommu_get_supported_ivhd_type();
+ if ( rc < 0 )
goto error_out;
+ ivhd_type = rc;
- ivrs_bdf_entries = amd_iommu_get_ivrs_dev_entries();
- if ( !ivrs_bdf_entries )
+ rc = amd_iommu_get_ivrs_dev_entries();
+ if ( !rc )
+ rc = -ENODEV;
+ if ( rc < 0 )
goto error_out;
+ ivrs_bdf_entries = rc;
radix_tree_init(&ivrs_maps);
for_each_amd_iommu ( iommu )
- if ( alloc_ivrs_mappings(iommu->seg) != 0 )
+ {
+ rc = alloc_ivrs_mappings(iommu->seg);
+ if ( rc )
goto error_out;
+ }
- if ( amd_iommu_update_ivrs_mapping_acpi() != 0 )
+ rc = amd_iommu_update_ivrs_mapping_acpi();
+ if ( rc )
goto error_out;
/* initialize io-apic interrupt remapping entries */
- if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )
+ if ( iommu_intremap )
+ rc = amd_iommu_setup_ioapic_remapping();
+ if ( rc )
goto error_out;
/* allocate and initialize a global device table shared by all iommus */
- if ( iterate_ivrs_mappings(amd_iommu_setup_device_table) != 0 )
+ rc = iterate_ivrs_mappings(amd_iommu_setup_device_table);
+ if ( rc )
goto error_out;
/*
@@ -1271,14 +1283,17 @@ int __init amd_iommu_init(void)
/* per iommu initialization */
for_each_amd_iommu ( iommu )
- if ( amd_iommu_init_one(iommu) != 0 )
+ {
+ rc = amd_iommu_init_one(iommu);
+ if ( rc )
goto error_out;
+ }
return 0;
error_out:
amd_iommu_init_cleanup();
- return -ENODEV;
+ return rc;
}
static void disable_iommu(struct amd_iommu *iommu)
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init()
2016-06-14 9:03 [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init() Jan Beulich
@ 2016-06-14 9:09 ` Andrew Cooper
2016-06-15 15:25 ` Suravee Suthikulanit
2016-06-16 2:03 ` Xu, Quan
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2016-06-14 9:09 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Suravee Suthikulpanit
On 14/06/16 10:03, Jan Beulich wrote:
> ... instead of using -ENODEV for any kind of error. It in particular
> addresses Coverity ID 1362694 (introduced by commit eb48587210 ["AMD
> IOMMU: introduce support for IVHD block type 11h"]).
>
> Coverity ID: 1362694
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init()
2016-06-14 9:09 ` Andrew Cooper
@ 2016-06-15 15:25 ` Suravee Suthikulanit
0 siblings, 0 replies; 6+ messages in thread
From: Suravee Suthikulanit @ 2016-06-15 15:25 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich, xen-devel
On 6/14/2016 4:09 AM, Andrew Cooper wrote:
> On 14/06/16 10:03, Jan Beulich wrote:
>> ... instead of using -ENODEV for any kind of error. It in particular
>> addresses Coverity ID 1362694 (introduced by commit eb48587210 ["AMD
>> IOMMU: introduce support for IVHD block type 11h"]).
>>
>> Coverity ID: 1362694
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Thank you for the fix up.
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init()
2016-06-14 9:03 [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init() Jan Beulich
2016-06-14 9:09 ` Andrew Cooper
@ 2016-06-16 2:03 ` Xu, Quan
2016-06-16 8:28 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Xu, Quan @ 2016-06-16 2:03 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Suravee Suthikulpanit
On June 14, 2016 5:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> - if ( amd_iommu_update_ivrs_mapping_acpi() != 0 )
> + rc = amd_iommu_update_ivrs_mapping_acpi();
> + if ( rc )
> goto error_out;
>
> /* initialize io-apic interrupt remapping entries */
> - if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )
> + if ( iommu_intremap )
> + rc = amd_iommu_setup_ioapic_remapping();
> + if ( rc )
> goto error_out;
Is it better to indent this if() here? Then,
+ if ( iommu_intremap )
+ {
+ rc = amd_iommu_setup_ioapic_remapping();
+ if ( rc )
+ goto error_out;
+ }
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init()
2016-06-16 2:03 ` Xu, Quan
@ 2016-06-16 8:28 ` Jan Beulich
2016-06-16 8:57 ` Xu, Quan
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-06-16 8:28 UTC (permalink / raw)
To: Quan Xu; +Cc: Andrew Cooper, Suravee Suthikulpanit, xen-devel
>>> On 16.06.16 at 04:03, <quan.xu@intel.com> wrote:
> On June 14, 2016 5:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> - if ( amd_iommu_update_ivrs_mapping_acpi() != 0 )
>> + rc = amd_iommu_update_ivrs_mapping_acpi();
>> + if ( rc )
>> goto error_out;
>>
>> /* initialize io-apic interrupt remapping entries */
>> - if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )
>> + if ( iommu_intremap )
>> + rc = amd_iommu_setup_ioapic_remapping();
>> + if ( rc )
>> goto error_out;
>
>
> Is it better to indent this if() here? Then,
>
> + if ( iommu_intremap )
> + {
> + rc = amd_iommu_setup_ioapic_remapping();
> + if ( rc )
> + goto error_out;
> + }
What would this help (apart from increasing LOC and patch size)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init()
2016-06-16 8:28 ` Jan Beulich
@ 2016-06-16 8:57 ` Xu, Quan
0 siblings, 0 replies; 6+ messages in thread
From: Xu, Quan @ 2016-06-16 8:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Suravee Suthikulpanit, xen-devel
On June 16, 2016 4:29 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 16.06.16 at 04:03, <quan.xu@intel.com> wrote:
> > On June 14, 2016 5:03 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> - if ( amd_iommu_update_ivrs_mapping_acpi() != 0 )
> >> + rc = amd_iommu_update_ivrs_mapping_acpi();
> >> + if ( rc )
> >> goto error_out;
> >>
> >> /* initialize io-apic interrupt remapping entries */
> >> - if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )
> >> + if ( iommu_intremap )
> >> + rc = amd_iommu_setup_ioapic_remapping();
> >> + if ( rc )
> >> goto error_out;
> >
> >
> > Is it better to indent this if() here? Then,
> >
> > + if ( iommu_intremap )
> > + {
> > + rc = amd_iommu_setup_ioapic_remapping();
> > + if ( rc )
> > + goto error_out;
> > + }
>
> What would this help (apart from increasing LOC and patch size)?
>
Ah, first of all, it is not a logic issue, but just make the code clear:
- this if( rc ) is called only if ( iommu_intremap ) is true.
- this error is from amd_iommu_setup_ioapic_remapping().
Also from '- if ( iommu_intremap && amd_iommu_setup_ioapic_remapping() != 0 )', I'd like to fix it as my suggestion.
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-16 8:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-14 9:03 [PATCH] AMD IOMMU: correctly propagate errors from amd_iommu_init() Jan Beulich
2016-06-14 9:09 ` Andrew Cooper
2016-06-15 15:25 ` Suravee Suthikulanit
2016-06-16 2:03 ` Xu, Quan
2016-06-16 8:28 ` Jan Beulich
2016-06-16 8:57 ` Xu, Quan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).