[PATCH] simplify pdpte check

[PATCH] simplify pdpte check

[Muli Ben-Yehuda]

Small cleanup: we check (pdpte & 1) && (pdpte & constant). Instead
just check (pdpte & (constant | 1)).

Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>

Index: kernel/kvm_main.c
===================================================================
--- kernel/kvm_main.c	(revision 4379)
+++ kernel/kvm_main.c	(working copy)
@@ -336,7 +336,7 @@
 	ret = 1;
 	for (i = 0; i < 4; ++i) {
 		pdpte = pdpt[offset + i];
-		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
+		if (pdpte & 0xfffffff0000001e7ull) {
 			ret = 0;
 			goto out;
 		}



-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Avi Kivity]

Muli Ben-Yehuda wrote:
> Small cleanup: we check (pdpte & 1) && (pdpte & constant). Instead
> just check (pdpte & (constant | 1)).
>
> Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>
>
> Index: kernel/kvm_main.c
> ===================================================================
> --- kernel/kvm_main.c	(revision 4379)
> +++ kernel/kvm_main.c	(working copy)
> @@ -336,7 +336,7 @@
>  	ret = 1;
>  	for (i = 0; i < 4; ++i) {
>  		pdpte = pdpt[offset + i];
> -		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
> +		if (pdpte & 0xfffffff0000001e7ull) {
>  			ret = 0;
>  			goto out;
>  		}
>
>   

The code is not equivalent.  For example, pdpte == 2 passes the check 
before the patch and fails it after.



-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Muli Ben-Yehuda]

On Thu, Feb 01, 2007 at 04:07:01PM +0200, Avi Kivity wrote:
> Muli Ben-Yehuda wrote:
> >Small cleanup: we check (pdpte & 1) && (pdpte & constant). Instead
> >just check (pdpte & (constant | 1)).
> >
> >Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>
> >
> >Index: kernel/kvm_main.c
> >===================================================================
> >--- kernel/kvm_main.c	(revision 4379)
> >+++ kernel/kvm_main.c	(working copy)
> >@@ -336,7 +336,7 @@
> > 	ret = 1;
> > 	for (i = 0; i < 4; ++i) {
> > 		pdpte = pdpt[offset + i];
> >-		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
> >+		if (pdpte & 0xfffffff0000001e7ull) {
> > 			ret = 0;
> > 			goto out;
> > 		}
> >
> >  
> 
> The code is not equivalent.  For example, pdpte == 2 passes the check 
> before the patch and fails it after.

err... 2 & 1 == 0, which does not pass the original check. Perhaps the
real error is '&&' instead of '||' in the original conditional?

Cheers,
Muli

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Avi Kivity]

Muli Ben-Yehuda wrote:
> On Thu, Feb 01, 2007 at 04:07:01PM +0200, Avi Kivity wrote:
>   
>> Muli Ben-Yehuda wrote:
>>     
>>> Small cleanup: we check (pdpte & 1) && (pdpte & constant). Instead
>>> just check (pdpte & (constant | 1)).
>>>
>>> Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>
>>>
>>> Index: kernel/kvm_main.c
>>> ===================================================================
>>> --- kernel/kvm_main.c	(revision 4379)
>>> +++ kernel/kvm_main.c	(working copy)
>>> @@ -336,7 +336,7 @@
>>> 	ret = 1;
>>> 	for (i = 0; i < 4; ++i) {
>>> 		pdpte = pdpt[offset + i];
>>> -		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
>>> +		if (pdpte & 0xfffffff0000001e7ull) {
>>> 			ret = 0;
>>> 			goto out;
>>> 		}
>>>
>>>  
>>>       
>> The code is not equivalent.  For example, pdpte == 2 passes the check 
>> before the patch and fails it after.
>>     
>
> err... 2 & 1 == 0, which does not pass the original check. Perhaps the
> real error is '&&' instead of '||' in the original conditional?
>   
pdpte == 2 is a legal value.  in the original code, the (pdpte & 1) == 0 
and we don't 'goto out'.  In the patched code, (pdpte & 0xblah) == 2 and 
we 'goto out'.

if the code had '||' instead of '&&', your change would be correct, but 
'&&' is really meant here.



-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Dor Laor]

>-----Original Message-----
>From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [mailto:kvm-devel-
>bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Muli Ben-Yehuda
>Sent: Thursday, February 01, 2007 4:46 PM
>To: Avi Kivity
>Cc: kvm-devel
>Subject: Re: [kvm-devel] [PATCH] simplify pdpte check
>
>On Thu, Feb 01, 2007 at 04:07:01PM +0200, Avi Kivity wrote:
>> Muli Ben-Yehuda wrote:
>> >Small cleanup: we check (pdpte & 1) && (pdpte & constant). Instead
>> >just check (pdpte & (constant | 1)).
>> >
>> >Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>
>> >
>> >Index: kernel/kvm_main.c
>> >===================================================================
>> >--- kernel/kvm_main.c	(revision 4379)
>> >+++ kernel/kvm_main.c	(working copy)
>> >@@ -336,7 +336,7 @@
>> > 	ret = 1;
>> > 	for (i = 0; i < 4; ++i) {
>> > 		pdpte = pdpt[offset + i];
>> >-		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
>> >+		if (pdpte & 0xfffffff0000001e7ull) {
>> > 			ret = 0;
>> > 			goto out;
>> > 		}
>> >
>> >
>>
>> The code is not equivalent.  For example, pdpte == 2 passes the check
>> before the patch and fails it after.
>
>err... 2 & 1 == 0, which does not pass the original check. Perhaps the
>real error is '&&' instead of '||' in the original conditional?


What about 'if (pdpte & 0xfffffff0000001e7ull > 0x1)' ?
Although the original is more readable.
>
>Cheers,
>Muli
>
>-----------------------------------------------------------------------
--
>Using Tomcat but need to do more? Need to support web services,
security?
>Get stuff done quickly with pre-integrated technology to make your job
>easier.
>Download IBM WebSphere Application Server v.1.0.1 based on Apache
Geronimo
>http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=12164
2
>_______________________________________________
>kvm-devel mailing list
>kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>https://lists.sourceforge.net/lists/listinfo/kvm-devel

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Avi Kivity]

Dor Laor wrote:
>   
>> -----Original Message-----
>> From: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org [mailto:kvm-devel-
>> bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Muli Ben-Yehuda
>> Sent: Thursday, February 01, 2007 4:46 PM
>> To: Avi Kivity
>> Cc: kvm-devel
>> Subject: Re: [kvm-devel] [PATCH] simplify pdpte check
>>
>> On Thu, Feb 01, 2007 at 04:07:01PM +0200, Avi Kivity wrote:
>>     
>>> Muli Ben-Yehuda wrote:
>>>       
>>>> Small cleanup: we check (pdpte & 1) && (pdpte & constant). Instead
>>>> just check (pdpte & (constant | 1)).
>>>>
>>>> Signed-off-by: Muli Ben-Yehuda <muli-7z/5BgaJwgfQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> Index: kernel/kvm_main.c
>>>> ===================================================================
>>>> --- kernel/kvm_main.c	(revision 4379)
>>>> +++ kernel/kvm_main.c	(working copy)
>>>> @@ -336,7 +336,7 @@
>>>> 	ret = 1;
>>>> 	for (i = 0; i < 4; ++i) {
>>>> 		pdpte = pdpt[offset + i];
>>>> -		if ((pdpte & 1) && (pdpte & 0xfffffff0000001e6ull)) {
>>>> +		if (pdpte & 0xfffffff0000001e7ull) {
>>>> 			ret = 0;
>>>> 			goto out;
>>>> 		}
>>>>
>>>>
>>>>         
>>> The code is not equivalent.  For example, pdpte == 2 passes the check
>>> before the patch and fails it after.
>>>       
>> err... 2 & 1 == 0, which does not pass the original check. Perhaps the
>> real error is '&&' instead of '||' in the original conditional?
>>     
>
>
> What about 'if (pdpte & 0xfffffff0000001e7ull > 0x1)' ?
>   

That would work.

> Although the original is more readable.
>   

Yes, and we're not exactly performance critical here.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Avi Kivity]

Avi Kivity wrote:
>>
>> What about 'if (pdpte & 0xfffffff0000001e7ull > 0x1)' ?
>>   
>
> That would work.

Actually it doesn't. pdpte == 2 fails.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Muli Ben-Yehuda]

On Thu, Feb 01, 2007 at 04:52:18PM +0200, Avi Kivity wrote:

> pdpte == 2 is a legal value.  in the original code, the (pdpte & 1)
> == 0 and we don't 'goto out'.  In the patched code, (pdpte & 0xblah)
> == 2 and we 'goto out'.

Yup, I got it now. Thanks.

Cheers,
Muli

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Re: [PATCH] simplify pdpte check

[Muli Ben-Yehuda]

On Thu, Feb 01, 2007 at 06:52:47AM -0800, Dor Laor wrote:

> What about 'if (pdpte & 0xfffffff0000001e7ull > 0x1)' ?
> Although the original is more readable.

The original is better, perhaps with a nice comment :-)

Cheers,
Muli

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier.
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642