[Qemu-devel] [PATCH] A bit optimization for tlb_set_page()

[Qemu-devel] [PATCH] A bit optimization for tlb_set_page()

[Jun Koi]

It is not necessary to continue searching for watchpoint when we
already found one and setup for handling watchpoint in a search loop
in tlb_set_page().
This patch breaks that search loop on then.

Signed-off-by: Jun Koi <junkoi2004@gmail.com>

diff --git a/exec.c b/exec.c
index 14d1fd7..6329775 100644
--- a/exec.c
+++ b/exec.c
@@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
             /* TODO: The memory case can be optimized by not trapping
                reads of pages with a write breakpoint.  */
             address |= TLB_MMIO;
+            break;
         }
     }

[Qemu-devel] Re: [PATCH] A bit optimization for tlb_set_page()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

Jun Koi wrote:
> It is not necessary to continue searching for watchpoint when we
> already found one and setup for handling watchpoint in a search loop
> in tlb_set_page().
> This patch breaks that search loop on then.

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

> 
> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
> 
> diff --git a/exec.c b/exec.c
> index 14d1fd7..6329775 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>              /* TODO: The memory case can be optimized by not trapping
>                 reads of pages with a write breakpoint.  */

PS: Don't you also want to address this todo while you are at it? :)

>              address |= TLB_MMIO;
> +            break;
>          }
>      }
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] A bit optimization for tlb_set_page()

[Jun Koi]

On Wed, Apr 28, 2010 at 3:36 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Jun Koi wrote:
>> It is not necessary to continue searching for watchpoint when we
>> already found one and setup for handling watchpoint in a search loop
>> in tlb_set_page().
>> This patch breaks that search loop on then.
>
> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
>
>>
>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>
>> diff --git a/exec.c b/exec.c
>> index 14d1fd7..6329775 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>              /* TODO: The memory case can be optimized by not trapping
>>                 reads of pages with a write breakpoint.  */
>
> PS: Don't you also want to address this todo while you are at it? :)
>

OK, I will look at that in my little spare time.

Thanks,
J

[Qemu-devel] Re: [PATCH] A bit optimization for tlb_set_page()

[Jun Koi]

On Wed, Apr 28, 2010 at 8:48 AM, Jun Koi <junkoi2004@gmail.com> wrote:
> On Wed, Apr 28, 2010 at 3:36 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Jun Koi wrote:
>>> It is not necessary to continue searching for watchpoint when we
>>> already found one and setup for handling watchpoint in a search loop
>>> in tlb_set_page().
>>> This patch breaks that search loop on then.
>>
>> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
>>
>>>
>>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 14d1fd7..6329775 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>>              /* TODO: The memory case can be optimized by not trapping
>>>                 reads of pages with a write breakpoint.  */
>>
>> PS: Don't you also want to address this todo while you are at it? :)
>>

Do you have any comment on below patch?

Thanks,
J


Dont handle write watchpoints on read-only memory access.
This patch also breaks the searching loop for watchpoint once the
setup facilities for handling watchpoint later is done.

Signed-off-by: Jun Koi <junkoi2004@gmail.com>

diff --git a/exec.c b/exec.c
index 14d1fd7..3f2d9ed 100644
--- a/exec.c
+++ b/exec.c
@@ -2236,10 +2236,12 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
        watchpoint trap routines.  */
     QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
         if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
-            iotlb = io_mem_watch + paddr;
-            /* TODO: The memory case can be optimized by not trapping
-               reads of pages with a write breakpoint.  */
-            address |= TLB_MMIO;
+            /* Avoid trapping reads of pages with a write breakpoint. */
+            if (!((prot & PAGE_WRITE == 0) && (wp->flags &
BP_MEM_WRITE != 0))) {
+                iotlb = io_mem_watch + paddr;
+                address |= TLB_MMIO;
+                break;
+            }
         }
     }

[Qemu-devel] Re: [PATCH] A bit optimization for tlb_set_page()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]

Jun Koi wrote:
> On Wed, Apr 28, 2010 at 8:48 AM, Jun Koi <junkoi2004@gmail.com> wrote:
>> On Wed, Apr 28, 2010 at 3:36 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Jun Koi wrote:
>>>> It is not necessary to continue searching for watchpoint when we
>>>> already found one and setup for handling watchpoint in a search loop
>>>> in tlb_set_page().
>>>> This patch breaks that search loop on then.
>>> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>>>
>>>> diff --git a/exec.c b/exec.c
>>>> index 14d1fd7..6329775 100644
>>>> --- a/exec.c
>>>> +++ b/exec.c
>>>> @@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>>>              /* TODO: The memory case can be optimized by not trapping
>>>>                 reads of pages with a write breakpoint.  */
>>> PS: Don't you also want to address this todo while you are at it? :)
>>>
> 
> Do you have any comment on below patch?
> 
> Thanks,
> J
> 
> 
> Dont handle write watchpoints on read-only memory access.
> This patch also breaks the searching loop for watchpoint once the
> setup facilities for handling watchpoint later is done.
> 
> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
> 
> diff --git a/exec.c b/exec.c
> index 14d1fd7..3f2d9ed 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2236,10 +2236,12 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>         watchpoint trap routines.  */
>      QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
>          if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
> -            iotlb = io_mem_watch + paddr;
> -            /* TODO: The memory case can be optimized by not trapping
> -               reads of pages with a write breakpoint.  */
> -            address |= TLB_MMIO;
> +            /* Avoid trapping reads of pages with a write breakpoint. */
> +            if (!((prot & PAGE_WRITE == 0) && (wp->flags &
> BP_MEM_WRITE != 0))) {

Patch is line-wrapped. Besides that

if ((wp->flags & BP_MEM_READ) || (prot & PAGE_WRITE))

is more readable IMHO.

> +                iotlb = io_mem_watch + paddr;
> +                address |= TLB_MMIO;
> +                break;
> +            }
>          }
>      }

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH] A bit optimization for tlb_set_page()

[Jun Koi]

On Wed, Apr 28, 2010 at 3:53 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Jun Koi wrote:
>> On Wed, Apr 28, 2010 at 8:48 AM, Jun Koi <junkoi2004@gmail.com> wrote:
>>> On Wed, Apr 28, 2010 at 3:36 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> Jun Koi wrote:
>>>>> It is not necessary to continue searching for watchpoint when we
>>>>> already found one and setup for handling watchpoint in a search loop
>>>>> in tlb_set_page().
>>>>> This patch breaks that search loop on then.
>>>> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>>>>
>>>>> diff --git a/exec.c b/exec.c
>>>>> index 14d1fd7..6329775 100644
>>>>> --- a/exec.c
>>>>> +++ b/exec.c
>>>>> @@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>>>>              /* TODO: The memory case can be optimized by not trapping
>>>>>                 reads of pages with a write breakpoint.  */
>>>> PS: Don't you also want to address this todo while you are at it? :)
>>>>
>>
>> Do you have any comment on below patch?
>>
>> Thanks,
>> J
>>
>>
>> Dont handle write watchpoints on read-only memory access.
>> This patch also breaks the searching loop for watchpoint once the
>> setup facilities for handling watchpoint later is done.
>>
>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>
>> diff --git a/exec.c b/exec.c
>> index 14d1fd7..3f2d9ed 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2236,10 +2236,12 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>         watchpoint trap routines.  */
>>      QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
>>          if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
>> -            iotlb = io_mem_watch + paddr;
>> -            /* TODO: The memory case can be optimized by not trapping
>> -               reads of pages with a write breakpoint.  */
>> -            address |= TLB_MMIO;
>> +            /* Avoid trapping reads of pages with a write breakpoint. */
>> +            if (!((prot & PAGE_WRITE == 0) && (wp->flags &
>> BP_MEM_WRITE != 0))) {
>
> Patch is line-wrapped.

That is the fault of Gmail.

> Besides that
>
> if ((wp->flags & BP_MEM_READ) || (prot & PAGE_WRITE))
>
> is more readable IMHO.

Agreed. Please see my updated patch.

Thanks,
Jun



Dont handle write watchpoints on read-only memory access.
This patch also breaks the searching loop for watchpoint once the
setup for handling watchpoint later is done.

Signed-off-by: Jun Koi <junkoi2004@gmail.com>


diff --git a/exec.c b/exec.c
index 14d1fd7..6fd859f 100644
--- a/exec.c
+++ b/exec.c
@@ -2236,10 +2236,12 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
        watchpoint trap routines.  */
     QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
         if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
-            iotlb = io_mem_watch + paddr;
-            /* TODO: The memory case can be optimized by not trapping
-               reads of pages with a write breakpoint.  */
-            address |= TLB_MMIO;
+            /* Avoid trapping reads of pages with a write breakpoint. */
+            if ((prot & PAGE_WRITE) || (wp->flags & BP_MEM_READ)) {
+                iotlb = io_mem_watch + paddr;
+                address |= TLB_MMIO;
+                break;
+            }
         }
     }

[Qemu-devel] Re: [PATCH] A bit optimization for tlb_set_page()

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3682 bytes --]

Jun Koi wrote:
> On Wed, Apr 28, 2010 at 3:53 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Jun Koi wrote:
>>> On Wed, Apr 28, 2010 at 8:48 AM, Jun Koi <junkoi2004@gmail.com> wrote:
>>>> On Wed, Apr 28, 2010 at 3:36 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> Jun Koi wrote:
>>>>>> It is not necessary to continue searching for watchpoint when we
>>>>>> already found one and setup for handling watchpoint in a search loop
>>>>>> in tlb_set_page().
>>>>>> This patch breaks that search loop on then.
>>>>> Acked-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>
>>>>>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>>>>>
>>>>>> diff --git a/exec.c b/exec.c
>>>>>> index 14d1fd7..6329775 100644
>>>>>> --- a/exec.c
>>>>>> +++ b/exec.c
>>>>>> @@ -2240,6 +2240,7 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>>>>>              /* TODO: The memory case can be optimized by not trapping
>>>>>>                 reads of pages with a write breakpoint.  */
>>>>> PS: Don't you also want to address this todo while you are at it? :)
>>>>>
>>> Do you have any comment on below patch?
>>>
>>> Thanks,
>>> J
>>>
>>>
>>> Dont handle write watchpoints on read-only memory access.
>>> This patch also breaks the searching loop for watchpoint once the
>>> setup facilities for handling watchpoint later is done.
>>>
>>> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
>>>
>>> diff --git a/exec.c b/exec.c
>>> index 14d1fd7..3f2d9ed 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>>> @@ -2236,10 +2236,12 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>>>         watchpoint trap routines.  */
>>>      QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
>>>          if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
>>> -            iotlb = io_mem_watch + paddr;
>>> -            /* TODO: The memory case can be optimized by not trapping
>>> -               reads of pages with a write breakpoint.  */
>>> -            address |= TLB_MMIO;
>>> +            /* Avoid trapping reads of pages with a write breakpoint. */
>>> +            if (!((prot & PAGE_WRITE == 0) && (wp->flags &
>>> BP_MEM_WRITE != 0))) {
>> Patch is line-wrapped.
> 
> That is the fault of Gmail.
> 
>> Besides that
>>
>> if ((wp->flags & BP_MEM_READ) || (prot & PAGE_WRITE))
>>
>> is more readable IMHO.
> 
> Agreed. Please see my updated patch.
> 

Looks ok, but you will likely have to resend it with proper subject and
without any traces of this thread. Otherwise the patch won't be found by
any maintainer.

Jan

> Thanks,
> Jun
> 
> 
> 
> Dont handle write watchpoints on read-only memory access.
> This patch also breaks the searching loop for watchpoint once the
> setup for handling watchpoint later is done.
> 
> Signed-off-by: Jun Koi <junkoi2004@gmail.com>
> 
> 
> diff --git a/exec.c b/exec.c
> index 14d1fd7..6fd859f 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2236,10 +2236,12 @@ void tlb_set_page(CPUState *env, target_ulong vaddr,
>         watchpoint trap routines.  */
>      QTAILQ_FOREACH(wp, &env->watchpoints, entry) {
>          if (vaddr == (wp->vaddr & TARGET_PAGE_MASK)) {
> -            iotlb = io_mem_watch + paddr;
> -            /* TODO: The memory case can be optimized by not trapping
> -               reads of pages with a write breakpoint.  */
> -            address |= TLB_MMIO;
> +            /* Avoid trapping reads of pages with a write breakpoint. */
> +            if ((prot & PAGE_WRITE) || (wp->flags & BP_MEM_READ)) {
> +                iotlb = io_mem_watch + paddr;
> +                address |= TLB_MMIO;
> +                break;
> +            }
>          }
>      }



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]