[PATCH] mm,x86: fix span coverage in e820_all_mapped()

[PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Xishi Qiu]

In the following case, e820_all_mapped() will return 1.
A < start < B-1 and B < end < C, it means <start, end> spans two regions.
<start, end>:	        [start - end]
e820 addr:	    ...[A - B-1][B - C]...

Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
---
 arch/x86/kernel/e820.c |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 174da5f..31ecab2 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -85,20 +85,11 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
 
 		if (type && ei->type != type)
 			continue;
-		/* is the region (part) in overlap with the current region ?*/
+		/* is the region (part) in overlap with the current region ? */
 		if (ei->addr >= end || ei->addr + ei->size <= start)
 			continue;
-
-		/* if the region is at the beginning of <start,end> we move
-		 * start to the end of the region since it's ok until there
-		 */
-		if (ei->addr <= start)
-			start = ei->addr + ei->size;
-		/*
-		 * if start is now at or beyond end, we're done, full
-		 * coverage
-		 */
-		if (start >= end)
+		/* is the region full coverage of <start, end> ? */
+		if (ei->addr <= start && ei->addr + ei->size >= end)
 			return 1;
 	}
 	return 0;
-- 
1.7.1


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Yinghai Lu]

On Tue, Dec 10, 2013 at 1:06 AM, Xishi Qiu <qiuxishi@huawei.com> wrote:
> In the following case, e820_all_mapped() will return 1.
> A < start < B-1 and B < end < C, it means <start, end> spans two regions.
> <start, end>:           [start - end]
> e820 addr:          ...[A - B-1][B - C]...

should be [start, end) right?
and
[A, B),[B, C)

>
> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
> ---
>  arch/x86/kernel/e820.c |   15 +++------------
>  1 files changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 174da5f..31ecab2 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -85,20 +85,11 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
>
>                 if (type && ei->type != type)
>                         continue;
> -               /* is the region (part) in overlap with the current region ?*/
> +               /* is the region (part) in overlap with the current region ? */
>                 if (ei->addr >= end || ei->addr + ei->size <= start)
>                         continue;
> -
> -               /* if the region is at the beginning of <start,end> we move
> -                * start to the end of the region since it's ok until there
> -                */
> -               if (ei->addr <= start)
> -                       start = ei->addr + ei->size;

so in your case new start will be B ?

next run will be C

> -               /*
> -                * if start is now at or beyond end, we're done, full
> -                * coverage
> -                */
> -               if (start >= end)


> +               /* is the region full coverage of <start, end> ? */
> +               if (ei->addr <= start && ei->addr + ei->size >= end)
>                         return 1;
>         }
>         return 0;

also e820 should be sanitized already to have [A,C).

or you are talking about [A,B), [B+1, C)
first run start will be B,  and next run with [B+1, ...), that will be
skipped...
will not return 1.

so old code should be ok.

Thanks

Yinghai

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[H. Peter Anvin]

On 12/10/2013 01:06 PM, Yinghai Lu wrote:
> On Tue, Dec 10, 2013 at 1:06 AM, Xishi Qiu <qiuxishi@huawei.com> wrote:
>> In the following case, e820_all_mapped() will return 1.
>> A < start < B-1 and B < end < C, it means <start, end> spans two regions.
>> <start, end>:           [start - end]
>> e820 addr:          ...[A - B-1][B - C]...
> 
> should be [start, end) right?
> and
> [A, B),[B, C)
> 

What happens if it spans more than two regions?

	-hpa


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Yinghai Lu]

On Tue, Dec 10, 2013 at 1:29 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/10/2013 01:06 PM, Yinghai Lu wrote:
>> On Tue, Dec 10, 2013 at 1:06 AM, Xishi Qiu <qiuxishi@huawei.com> wrote:
>>> In the following case, e820_all_mapped() will return 1.
>>> A < start < B-1 and B < end < C, it means <start, end> spans two regions.
>>> <start, end>:           [start - end]
>>> e820 addr:          ...[A - B-1][B - C]...
>>
>> should be [start, end) right?
>> and
>> [A, B),[B, C)
>>
>
> What happens if it spans more than two regions?

[A, B), [B+1, C), [C+1, D) ?
start in [A, B), and end in [C+1, D).

old code:
first with [A, B), start set to B.
then with [B+1, C), start still keep as B.
then with [C+1, D), start still keep as B.
at last still return 0...aka not_all_mapped.

old code is still right.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[H. Peter Anvin]

On 12/10/2013 01:52 PM, Yinghai Lu wrote:
>>
>> What happens if it spans more than two regions?
> 
> [A, B), [B+1, C), [C+1, D) ?
> start in [A, B), and end in [C+1, D).
> 
> old code:
> first with [A, B), start set to B.
> then with [B+1, C), start still keep as B.
> then with [C+1, D), start still keep as B.
> at last still return 0...aka not_all_mapped.
> 
> old code is still right.
> 

Why not_all_mapped?

	-hpa

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Yinghai Lu]

On Tue, Dec 10, 2013 at 2:51 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/10/2013 01:52 PM, Yinghai Lu wrote:
>>>
>>> What happens if it spans more than two regions?
>>
>> [A, B), [B+1, C), [C+1, D) ?
>> start in [A, B), and end in [C+1, D).
>>
>> old code:
>> first with [A, B), start set to B.
>> then with [B+1, C), start still keep as B.
>> then with [C+1, D), start still keep as B.
>> at last still return 0...aka not_all_mapped.
>>
>> old code is still right.
>>
>
> Why not_all_mapped?

[B, B+1), and [C, C+1) are not there.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[H. Peter Anvin]

Ok, the issue I thought we were discussing was actually [A,B) [B,C) [C,D) ...

Yinghai Lu <yinghai@kernel.org> wrote:
>On Tue, Dec 10, 2013 at 2:51 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 12/10/2013 01:52 PM, Yinghai Lu wrote:
>>>>
>>>> What happens if it spans more than two regions?
>>>
>>> [A, B), [B+1, C), [C+1, D) ?
>>> start in [A, B), and end in [C+1, D).
>>>
>>> old code:
>>> first with [A, B), start set to B.
>>> then with [B+1, C), start still keep as B.
>>> then with [C+1, D), start still keep as B.
>>> at last still return 0...aka not_all_mapped.
>>>
>>> old code is still right.
>>>
>>
>> Why not_all_mapped?
>
>[B, B+1), and [C, C+1) are not there.

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Xishi Qiu]

On 2013/12/11 9:06, H. Peter Anvin wrote:

> Ok, the issue I thought we were discussing was actually [A,B) [B,C) [C,D) ...
> 

Hi Peter,

Yes, in this case the function will return 1.

Thanks,
Xishi Qiu

> Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Dec 10, 2013 at 2:51 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>> On 12/10/2013 01:52 PM, Yinghai Lu wrote:
>>>>>
>>>>> What happens if it spans more than two regions?
>>>>
>>>> [A, B), [B+1, C), [C+1, D) ?
>>>> start in [A, B), and end in [C+1, D).
>>>>
>>>> old code:
>>>> first with [A, B), start set to B.
>>>> then with [B+1, C), start still keep as B.
>>>> then with [C+1, D), start still keep as B.
>>>> at last still return 0...aka not_all_mapped.
>>>>
>>>> old code is still right.
>>>>
>>>
>>> Why not_all_mapped?
>>
>> [B, B+1), and [C, C+1) are not there.
> 



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Xishi Qiu]

On 2013/12/11 5:06, Yinghai Lu wrote:

> On Tue, Dec 10, 2013 at 1:06 AM, Xishi Qiu <qiuxishi@huawei.com> wrote:
>> In the following case, e820_all_mapped() will return 1.
>> A < start < B-1 and B < end < C, it means <start, end> spans two regions.
>> <start, end>:           [start - end]
>> e820 addr:          ...[A - B-1][B - C]...
> 
> should be [start, end) right?
> and
> [A, B),[B, C)
> 

Hi Yinghai,

It is right, in this case the function will return 1.

>>
>> Signed-off-by: Xishi Qiu <qiuxishi@huawei.com>
>> ---
>>  arch/x86/kernel/e820.c |   15 +++------------
>>  1 files changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 174da5f..31ecab2 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -85,20 +85,11 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
>>
>>                 if (type && ei->type != type)
>>                         continue;
>> -               /* is the region (part) in overlap with the current region ?*/
>> +               /* is the region (part) in overlap with the current region ? */
>>                 if (ei->addr >= end || ei->addr + ei->size <= start)
>>                         continue;
>> -
>> -               /* if the region is at the beginning of <start,end> we move
>> -                * start to the end of the region since it's ok until there
>> -                */
>> -               if (ei->addr <= start)
>> -                       start = ei->addr + ei->size;
> 
> so in your case new start will be B ?
> 
> next run will be C
> 
>> -               /*
>> -                * if start is now at or beyond end, we're done, full
>> -                * coverage
>> -                */
>> -               if (start >= end)
> 
> 
>> +               /* is the region full coverage of <start, end> ? */
>> +               if (ei->addr <= start && ei->addr + ei->size >= end)
>>                         return 1;
>>         }
>>         return 0;
> 
> also e820 should be sanitized already to have [A,C).
> 

Yes, it should be sanitized already, but maybe someone will change the e820
to support some feature, so this function will be a potential bomb.

> or you are talking about [A,B), [B+1, C)
> first run start will be B,  and next run with [B+1, ...), that will be
> skipped...
> will not return 1.
> 
> so old code should be ok.
> 

In this case, old code is right, but I discuss in another one that
you wrote above.

Thanks,
Xishi Qiu

> Thanks
> 
> Yinghai
> 
> .
> 



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[H. Peter Anvin]

On 12/10/2013 05:35 PM, Xishi Qiu wrote:
> 
> In this case, old code is right, but I discuss in another one that
> you wrote above.
> 

So is there a problem or not?  I have lost track...

	-hpa


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Xishi Qiu]

On 2013/12/11 10:55, H. Peter Anvin wrote:

> On 12/10/2013 05:35 PM, Xishi Qiu wrote:
>>
>> In this case, old code is right, but I discuss in another one that
>> you wrote above.
>>
> 
> So is there a problem or not?  I have lost track...
> 

I think there is a problem.
e.g.
[start, end)=[8, 12), and [A, B)=[0, 10), [B, C)=[10,20),
then e820_all_mapped() will return 1, it spans two regions.

Thanks,
Xishi Qiu

> 	-hpa
> 
> 
> 
> 



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[H. Peter Anvin]

On 12/10/2013 07:55 PM, Xishi Qiu wrote:
> 
> I think there is a problem.
> e.g.
> [start, end)=[8, 12), and [A, B)=[0, 10), [B, C)=[10,20),
> then e820_all_mapped() will return 1, it spans two regions.
> 

Why is that a problem?

	-hpa


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[Xishi Qiu]

On 2013/12/11 12:02, H. Peter Anvin wrote:

> On 12/10/2013 07:55 PM, Xishi Qiu wrote:
>>
>> I think there is a problem.
>> e.g.
>> [start, end)=[8, 12), and [A, B)=[0, 10), [B, C)=[10,20),
>> then e820_all_mapped() will return 1, it spans two regions.
>>
> 
> Why is that a problem?
> 

[start, end) should be included in one region ?

Thanks,
Xishi Qiu

> 	-hpa
> 
> 
> 
> 



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm,x86: fix span coverage in e820_all_mapped()

[H. Peter Anvin]

Is that an actual requirement of the API?

Xishi Qiu <qiuxishi@huawei.com> wrote:
>On 2013/12/11 12:02, H. Peter Anvin wrote:
>
>> On 12/10/2013 07:55 PM, Xishi Qiu wrote:
>>>
>>> I think there is a problem.
>>> e.g.
>>> [start, end)=[8, 12), and [A, B)=[0, 10), [B, C)=[10,20),
>>> then e820_all_mapped() will return 1, it spans two regions.
>>>
>> 
>> Why is that a problem?
>> 
>
>[start, end) should be included in one region ?
>
>Thanks,
>Xishi Qiu
>
>> 	-hpa
>> 
>> 
>> 
>> 

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>