[PATCH V3 1/1] amd/iommu: Fix infinite loop due to ivrs_bdf_entries larger than 16-bit value

[PATCH V3 1/1] amd/iommu: Fix infinite loop due to ivrs_bdf_entries larger than 16-bit value

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

Certain AMD systems could have upto 0x10000 ivrs_bdf_entries.
However, the loop variable (bdf) is declared as u16 which causes
inifinite loop when parsing IOMMU event log with IO_PAGE_FAULT event.
This patch changes the variable to u32 instead.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
V3:
	- More places found in iommu_acpi.c
	- Add signed off message.
V2:
	- Fix in more places as pointed out by Andrew
 xen/drivers/passthrough/amd/iommu_acpi.c |   17 +++++++++++------
 xen/drivers/passthrough/amd/iommu_init.c |   13 +++++++------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index fca2037..b396e0e 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -159,7 +159,7 @@ static int __init register_exclusion_range_for_all_devices(
     int seg = 0; /* XXX */
     unsigned long range_top, iommu_top, length;
     struct amd_iommu *iommu;
-    u16 bdf;
+    u32 bdf;
 
     /* is part of exclusion range inside of IOMMU virtual address space? */
     /* note: 'limit' parameter is assumed to be page-aligned */
@@ -237,7 +237,8 @@ static int __init register_exclusion_range_for_iommu_devices(
     unsigned long base, unsigned long limit, u8 iw, u8 ir)
 {
     unsigned long range_top, iommu_top, length;
-    u16 bdf, req;
+    u32 bdf;
+    u16 req;
 
     /* is part of exclusion range inside of IOMMU virtual address space? */
     /* note: 'limit' parameter is assumed to be page-aligned */
@@ -292,7 +293,8 @@ static int __init parse_ivmd_device_range(
     const struct acpi_ivrs_memory *ivmd_block,
     unsigned long base, unsigned long limit, u8 iw, u8 ir)
 {
-    u16 first_bdf, last_bdf, bdf;
+    u16 first_bdf, last_bdf; 
+    u32 bdf;
     int error;
 
     first_bdf = ivmd_block->header.device_id;
@@ -430,7 +432,8 @@ static u16 __init parse_ivhd_device_range(
     const struct acpi_ivhd_device_range *range,
     u16 header_length, u16 block_length, struct amd_iommu *iommu)
 {
-    u16 dev_length, first_bdf, last_bdf, bdf;
+    u16 dev_length, first_bdf, last_bdf;
+    u32 bdf;
 
     dev_length = sizeof(*range);
     if ( header_length < (block_length + dev_length) )
@@ -511,7 +514,8 @@ static u16 __init parse_ivhd_device_alias_range(
     u16 header_length, u16 block_length, struct amd_iommu *iommu)
 {
 
-    u16 dev_length, first_bdf, last_bdf, alias_id, bdf;
+    u16 dev_length, first_bdf, last_bdf, alias_id;
+    u32 bdf;
 
     dev_length = sizeof(*range);
     if ( header_length < (block_length + dev_length) )
@@ -590,7 +594,8 @@ static u16 __init parse_ivhd_device_extended_range(
     const struct acpi_ivhd_device_extended_range *range,
     u16 header_length, u16 block_length, struct amd_iommu *iommu)
 {
-    u16 dev_length, first_bdf, last_bdf, bdf;
+    u16 dev_length, first_bdf, last_bdf;
+    u32 bdf;
 
     dev_length = sizeof(*range);
     if ( header_length < (block_length + dev_length) )
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index b431d16..c410465 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -524,8 +524,8 @@ static hw_irq_controller iommu_maskable_msi_type = {
 
 static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
 {
-    u16 domain_id, device_id, bdf, flags;
-    u32 code;
+    u16 domain_id, device_id, flags;
+    u32 code, bdf;
     u64 *addr;
     int count = 0;
     static const char *const event_str[] = {
@@ -1103,7 +1103,7 @@ int iterate_ivrs_entries(int (*handler)(u16 seg, struct ivrs_mappings *))
 
     do {
         struct ivrs_mappings *map;
-        int bdf;
+        u32 bdf;
 
         if ( !radix_tree_gang_lookup(&ivrs_maps, (void **)&map, seg, 1) )
             break;
@@ -1118,7 +1118,7 @@ int iterate_ivrs_entries(int (*handler)(u16 seg, struct ivrs_mappings *))
 static int __init alloc_ivrs_mappings(u16 seg)
 {
     struct ivrs_mappings *ivrs_mappings;
-    int bdf;
+    u32 bdf;
 
     BUG_ON( !ivrs_bdf_entries );
 
@@ -1156,7 +1156,7 @@ static int __init alloc_ivrs_mappings(u16 seg)
 static int __init amd_iommu_setup_device_table(
     u16 seg, struct ivrs_mappings *ivrs_mappings)
 {
-    int bdf;
+    u32 bdf;
     void *intr_tb, *dte;
 
     BUG_ON( (ivrs_bdf_entries == 0) );
@@ -1306,7 +1306,8 @@ static void invalidate_all_domain_pages(void)
 static int _invalidate_all_devices(
     u16 seg, struct ivrs_mappings *ivrs_mappings)
 {
-    int bdf, req_id;
+    u32 bdf; 
+    u16 req_id;
     unsigned long flags;
     struct amd_iommu *iommu;
 
-- 
1.7.10.4

Re: [PATCH V3 1/1] amd/iommu: Fix infinite loop due to ivrs_bdf_entries larger than 16-bit value

[Boris Ostrovsky]

On 12/29/2013 06:34 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> Certain AMD systems could have upto 0x10000 ivrs_bdf_entries.
> However, the loop variable (bdf) is declared as u16 which causes
> inifinite loop when parsing IOMMU event log with IO_PAGE_FAULT event.
> This patch changes the variable to u32 instead.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> V3:
> 	- More places found in iommu_acpi.c
> 	- Add signed off message.
> V2:
> 	- Fix in more places as pointed out by Andrew
>   xen/drivers/passthrough/amd/iommu_acpi.c |   17 +++++++++++------
>   xen/drivers/passthrough/amd/iommu_init.c |   13 +++++++------
>   2 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
> index fca2037..b396e0e 100644
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -159,7 +159,7 @@ static int __init register_exclusion_range_for_all_devices(
>       int seg = 0; /* XXX */
>       unsigned long range_top, iommu_top, length;
>       struct amd_iommu *iommu;
> -    u16 bdf;
> +    u32 bdf;
>   
>       /* is part of exclusion range inside of IOMMU virtual address space? */
>       /* note: 'limit' parameter is assumed to be page-aligned */
> @@ -237,7 +237,8 @@ static int __init register_exclusion_range_for_iommu_devices(
>       unsigned long base, unsigned long limit, u8 iw, u8 ir)
>   {
>       unsigned long range_top, iommu_top, length;
> -    u16 bdf, req;
> +    u32 bdf;
> +    u16 req;
>   
>       /* is part of exclusion range inside of IOMMU virtual address space? */
>       /* note: 'limit' parameter is assumed to be page-aligned */
> @@ -292,7 +293,8 @@ static int __init parse_ivmd_device_range(
>       const struct acpi_ivrs_memory *ivmd_block,
>       unsigned long base, unsigned long limit, u8 iw, u8 ir)
>   {
> -    u16 first_bdf, last_bdf, bdf;
> +    u16 first_bdf, last_bdf;
> +    u32 bdf;
>       int error;

Shouldn't first_bdf and last_bdf be u32 as well?

There is, for example, a loop in this routine

     for ( bdf = first_bdf, error = 0; (bdf <= last_bdf) && !error; bdf++ )

And in routines below as well.


-boris

>   
>       first_bdf = ivmd_block->header.device_id;
> @@ -430,7 +432,8 @@ static u16 __init parse_ivhd_device_range(
>       const struct acpi_ivhd_device_range *range,
>       u16 header_length, u16 block_length, struct amd_iommu *iommu)
>   {
> -    u16 dev_length, first_bdf, last_bdf, bdf;
> +    u16 dev_length, first_bdf, last_bdf;
> +    u32 bdf;
>   
>       dev_length = sizeof(*range);
>       if ( header_length < (block_length + dev_length) )
> @@ -511,7 +514,8 @@ static u16 __init parse_ivhd_device_alias_range(
>       u16 header_length, u16 block_length, struct amd_iommu *iommu)
>   {
>   
> -    u16 dev_length, first_bdf, last_bdf, alias_id, bdf;
> +    u16 dev_length, first_bdf, last_bdf, alias_id;
> +    u32 bdf;
>   
>       dev_length = sizeof(*range);
>       if ( header_length < (block_length + dev_length) )
> @@ -590,7 +594,8 @@ static u16 __init parse_ivhd_device_extended_range(
>       const struct acpi_ivhd_device_extended_range *range,
>       u16 header_length, u16 block_length, struct amd_iommu *iommu)
>   {
> -    u16 dev_length, first_bdf, last_bdf, bdf;
> +    u16 dev_length, first_bdf, last_bdf;
> +    u32 bdf;
>   
>       dev_length = sizeof(*range);
>       if ( header_length < (block_length + dev_length) )
> diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
> index b431d16..c410465 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -524,8 +524,8 @@ static hw_irq_controller iommu_maskable_msi_type = {
>   
>   static void parse_event_log_entry(struct amd_iommu *iommu, u32 entry[])
>   {
> -    u16 domain_id, device_id, bdf, flags;
> -    u32 code;
> +    u16 domain_id, device_id, flags;
> +    u32 code, bdf;
>       u64 *addr;
>       int count = 0;
>       static const char *const event_str[] = {
> @@ -1103,7 +1103,7 @@ int iterate_ivrs_entries(int (*handler)(u16 seg, struct ivrs_mappings *))
>   
>       do {
>           struct ivrs_mappings *map;
> -        int bdf;
> +        u32 bdf;
>   
>           if ( !radix_tree_gang_lookup(&ivrs_maps, (void **)&map, seg, 1) )
>               break;
> @@ -1118,7 +1118,7 @@ int iterate_ivrs_entries(int (*handler)(u16 seg, struct ivrs_mappings *))
>   static int __init alloc_ivrs_mappings(u16 seg)
>   {
>       struct ivrs_mappings *ivrs_mappings;
> -    int bdf;
> +    u32 bdf;
>   
>       BUG_ON( !ivrs_bdf_entries );
>   
> @@ -1156,7 +1156,7 @@ static int __init alloc_ivrs_mappings(u16 seg)
>   static int __init amd_iommu_setup_device_table(
>       u16 seg, struct ivrs_mappings *ivrs_mappings)
>   {
> -    int bdf;
> +    u32 bdf;
>       void *intr_tb, *dte;
>   
>       BUG_ON( (ivrs_bdf_entries == 0) );
> @@ -1306,7 +1306,8 @@ static void invalidate_all_domain_pages(void)
>   static int _invalidate_all_devices(
>       u16 seg, struct ivrs_mappings *ivrs_mappings)
>   {
> -    int bdf, req_id;
> +    u32 bdf;
> +    u16 req_id;
>       unsigned long flags;
>       struct amd_iommu *iommu;
>   

Re: [PATCH V3 1/1] amd/iommu: Fix infinite loop due to ivrs_bdf_entries larger than 16-bit value

[Suravee Suthikulpanit]

On 12/30/2013 07:04 AM, Boris Ostrovsky wrote:
> On 12/29/2013 06:34 PM, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> Certain AMD systems could have upto 0x10000 ivrs_bdf_entries.
>> However, the loop variable (bdf) is declared as u16 which causes
>> inifinite loop when parsing IOMMU event log with IO_PAGE_FAULT event.
>> This patch changes the variable to u32 instead.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> V3:
>>     - More places found in iommu_acpi.c
>>     - Add signed off message.
>> V2:
>>     - Fix in more places as pointed out by Andrew
>>   xen/drivers/passthrough/amd/iommu_acpi.c |   17 +++++++++++------
>>   xen/drivers/passthrough/amd/iommu_init.c |   13 +++++++------
>>   2 files changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c
>> b/xen/drivers/passthrough/amd/iommu_acpi.c
>> index fca2037..b396e0e 100644
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -159,7 +159,7 @@ static int __init
>> register_exclusion_range_for_all_devices(
>>       int seg = 0; /* XXX */
>>       unsigned long range_top, iommu_top, length;
>>       struct amd_iommu *iommu;
>> -    u16 bdf;
>> +    u32 bdf;
>>       /* is part of exclusion range inside of IOMMU virtual address
>> space? */
>>       /* note: 'limit' parameter is assumed to be page-aligned */
>> @@ -237,7 +237,8 @@ static int __init
>> register_exclusion_range_for_iommu_devices(
>>       unsigned long base, unsigned long limit, u8 iw, u8 ir)
>>   {
>>       unsigned long range_top, iommu_top, length;
>> -    u16 bdf, req;
>> +    u32 bdf;
>> +    u16 req;
>>       /* is part of exclusion range inside of IOMMU virtual address
>> space? */
>>       /* note: 'limit' parameter is assumed to be page-aligned */
>> @@ -292,7 +293,8 @@ static int __init parse_ivmd_device_range(
>>       const struct acpi_ivrs_memory *ivmd_block,
>>       unsigned long base, unsigned long limit, u8 iw, u8 ir)
>>   {
>> -    u16 first_bdf, last_bdf, bdf;
>> +    u16 first_bdf, last_bdf;
>> +    u32 bdf;
>>       int error;
>
> Shouldn't first_bdf and last_bdf be u32 as well?
>
> There is, for example, a loop in this routine
>
>      for ( bdf = first_bdf, error = 0; (bdf <= last_bdf) && !error; bdf++ )
>
> And in routines below as well.
>
>
> -boris

I am not expecting the first_bdf and last_bdf to be greater than 16-bit. 
However, for the bitwise logic comparisons, I can make them all 32-bit. 
I'll send out V4.  Thanks for the review.

Suravee