From mboxrd@z Thu Jan  1 00:00:00 1970
From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH V3 1/1] amd/iommu: Fix infinite loop due to
 ivrs_bdf_entries larger than 16-bit value
Date: Mon, 30 Dec 2013 10:51:06 -0600
Message-ID: <52C1A47A.1050306@amd.com>
References: <1388360056-3314-1-git-send-email-suravee.suthikulpanit@amd.com>
	<52C16F51.2010608@oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <52C16F51.2010608@oracle.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: andrew.cooper3@citrix.com, JBeulich@suse.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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