[PATCH] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[PATCH] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Jan Kiszka]

This is at best a PPC topi but according to [1] even there unneeded. In
any case, remove this diff to upstream, it should be handled there if
actually needed.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/exec.c b/exec.c
index db837e2..0ca17fc 100644
--- a/exec.c
+++ b/exec.c
@@ -26,8 +26,6 @@
 
 #include "qemu-common.h"
 #include "cpu.h"
-#include "cache-utils.h"
-
 #include "tcg.h"
 #include "hw/hw.h"
 #include "hw/qdev.h"
@@ -3599,11 +3597,6 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                     cpu_physical_memory_set_dirty_flags(
                         addr1, (0xff & ~CODE_DIRTY_FLAG));
                 }
-		/* qemu doesn't execute guest code directly, but kvm does
-		   therefore flush instruction caches */
-		if (kvm_enabled())
-		    flush_icache_range((unsigned long)ptr,
-				       ((unsigned long)ptr)+l);
                 qemu_put_ram_ptr(ptr);
             }
         } else {
-- 
1.7.3.4

Re: [PATCH] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Marcelo Tosatti]

On Thu, Jan 19, 2012 at 01:39:24PM +0100, Jan Kiszka wrote:
> This is at best a PPC topi but according to [1] even there unneeded. In
> any case, remove this diff to upstream, it should be handled there if
> actually needed.

[1] ? 

[PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Jan Kiszka]

On 2012-01-19 18:54, Marcelo Tosatti wrote:
> On Thu, Jan 19, 2012 at 01:39:24PM +0100, Jan Kiszka wrote:
>> This is at best a PPC topi but according to [1] even there unneeded. In
>> any case, remove this diff to upstream, it should be handled there if
>> actually needed.
> 
> [1] ? 
> 

Oops.

--------8<---------

This is at best a PPC topi but according to [1] even there unneeded. In
any case, remove this diff to upstream, it should be handled there if
actually needed.

[1] http://thread.gmane.org/gmane.comp.emulators.qemu/119022/focus=119086

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 exec.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/exec.c b/exec.c
index db837e2..0ca17fc 100644
--- a/exec.c
+++ b/exec.c
@@ -26,8 +26,6 @@
 
 #include "qemu-common.h"
 #include "cpu.h"
-#include "cache-utils.h"
-
 #include "tcg.h"
 #include "hw/hw.h"
 #include "hw/qdev.h"
@@ -3599,11 +3597,6 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
                     cpu_physical_memory_set_dirty_flags(
                         addr1, (0xff & ~CODE_DIRTY_FLAG));
                 }
-		/* qemu doesn't execute guest code directly, but kvm does
-		   therefore flush instruction caches */
-		if (kvm_enabled())
-		    flush_icache_range((unsigned long)ptr,
-				       ((unsigned long)ptr)+l);
                 qemu_put_ram_ptr(ptr);
             }
         } else {
-- 
1.7.3.4

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Marcelo Tosatti]

On Thu, Jan 19, 2012 at 07:04:38PM +0100, Jan Kiszka wrote:
> On 2012-01-19 18:54, Marcelo Tosatti wrote:
> > On Thu, Jan 19, 2012 at 01:39:24PM +0100, Jan Kiszka wrote:
> >> This is at best a PPC topi but according to [1] even there unneeded. In
> >> any case, remove this diff to upstream, it should be handled there if
> >> actually needed.
> > 
> > [1] ? 
> > 
> 
> Oops.
> 
> --------8<---------
> 
> This is at best a PPC topi but according to [1] even there unneeded. In
> any case, remove this diff to upstream, it should be handled there if
> actually needed.
> 
> [1] http://thread.gmane.org/gmane.comp.emulators.qemu/119022/focus=119086
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Applied, thanks.

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Scott Wood]

On 01/19/2012 12:04 PM, Jan Kiszka wrote:
> On 2012-01-19 18:54, Marcelo Tosatti wrote:
>> On Thu, Jan 19, 2012 at 01:39:24PM +0100, Jan Kiszka wrote:
>>> This is at best a PPC topi but according to [1] even there unneeded. In
>>> any case, remove this diff to upstream, it should be handled there if
>>> actually needed.
>>
>> [1] ? 
>>
> 
> Oops.
> 
> --------8<---------
> 
> This is at best a PPC topi but according to [1] even there unneeded. In
> any case, remove this diff to upstream, it should be handled there if
> actually needed.
> 
> [1] http://thread.gmane.org/gmane.comp.emulators.qemu/119022/focus=119086

That says that it's unneeded on (some?) IBM Power systems.  We need it
on Freescale chips.  I submitted an upstream-QEMU patch to do this flush
(referenced in that thread, still not applied) because I was seeing
cache problems when loading images.

-Scott

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Jan Kiszka]

On 2012-01-24 01:26, Scott Wood wrote:
> On 01/19/2012 12:04 PM, Jan Kiszka wrote:
>> On 2012-01-19 18:54, Marcelo Tosatti wrote:
>>> On Thu, Jan 19, 2012 at 01:39:24PM +0100, Jan Kiszka wrote:
>>>> This is at best a PPC topi but according to [1] even there unneeded. In
>>>> any case, remove this diff to upstream, it should be handled there if
>>>> actually needed.
>>>
>>> [1] ? 
>>>
>>
>> Oops.
>>
>> --------8<---------
>>
>> This is at best a PPC topi but according to [1] even there unneeded. In
>> any case, remove this diff to upstream, it should be handled there if
>> actually needed.
>>
>> [1] http://thread.gmane.org/gmane.comp.emulators.qemu/119022/focus=119086
> 
> That says that it's unneeded on (some?) IBM Power systems.  We need it
> on Freescale chips.  I submitted an upstream-QEMU patch to do this flush
> (referenced in that thread, still not applied) because I was seeing
> cache problems when loading images.

Then you probably want to limit your patch's effect to that particular
set of chips and repost it to qemu-devel. In any case, it's not a
qemu-kvm topic as we are x86-only for quite a while.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Avi Kivity]

On 01/24/2012 10:57 AM, Jan Kiszka wrote:
> > That says that it's unneeded on (some?) IBM Power systems.  We need it
> > on Freescale chips.  I submitted an upstream-QEMU patch to do this flush
> > (referenced in that thread, still not applied) because I was seeing
> > cache problems when loading images.
>
> Then you probably want to limit your patch's effect to that particular
> set of chips and repost it to qemu-devel. In any case, it's not a
> qemu-kvm topic as we are x86-only for quite a while.
>

IIRC introduced for ia64.  But isn't the correct action adding it to
qemu.git instead of removing it completely?

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

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Jan Kiszka]

On 2012-01-24 12:34, Avi Kivity wrote:
> On 01/24/2012 10:57 AM, Jan Kiszka wrote:
>>> That says that it's unneeded on (some?) IBM Power systems.  We need it
>>> on Freescale chips.  I submitted an upstream-QEMU patch to do this flush
>>> (referenced in that thread, still not applied) because I was seeing
>>> cache problems when loading images.
>>
>> Then you probably want to limit your patch's effect to that particular
>> set of chips and repost it to qemu-devel. In any case, it's not a
>> qemu-kvm topic as we are x86-only for quite a while.
>>
> 
> IIRC introduced for ia64.  But isn't the correct action adding it to
> qemu.git instead of removing it completely?

Adding something to qemu.git, yes, keeping it here without knowing the
final code, no. This is just another unused variation from upstream.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Avi Kivity]

On 01/24/2012 01:38 PM, Jan Kiszka wrote:
> > 
> > IIRC introduced for ia64.  But isn't the correct action adding it to
> > qemu.git instead of removing it completely?
>
> Adding something to qemu.git, yes, keeping it here without knowing the
> final code, no. 

This is called introducing a regression.

> This is just another unused variation from upstream.

It's wasn't unused when ia64 was alive.  Just removing it means more
work later to rediscover it (and anyway ppc needs it).

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

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Jan Kiszka]

On 2012-01-24 14:09, Avi Kivity wrote:
> On 01/24/2012 01:38 PM, Jan Kiszka wrote:
>>>
>>> IIRC introduced for ia64.  But isn't the correct action adding it to
>>> qemu.git instead of removing it completely?
>>
>> Adding something to qemu.git, yes, keeping it here without knowing the
>> final code, no. 
> 
> This is called introducing a regression.

There is nothing to regress here.

> 
>> This is just another unused variation from upstream.
> 
> It's wasn't unused when ia64 was alive.  Just removing it means more
> work later to rediscover it (and anyway ppc needs it).

ppc is not used with qemu-kvm, it's used with qemu upstream. qemu-kvm is
a x86-only show today.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Avi Kivity]

On 01/24/2012 03:12 PM, Jan Kiszka wrote:
> On 2012-01-24 14:09, Avi Kivity wrote:
> > On 01/24/2012 01:38 PM, Jan Kiszka wrote:
> >>>
> >>> IIRC introduced for ia64.  But isn't the correct action adding it to
> >>> qemu.git instead of removing it completely?
> >>
> >> Adding something to qemu.git, yes, keeping it here without knowing the
> >> final code, no. 
> > 
> > This is called introducing a regression.
>
> There is nothing to regress here.
>
> > 
> >> This is just another unused variation from upstream.
> > 
> > It's wasn't unused when ia64 was alive.  Just removing it means more
> > work later to rediscover it (and anyway ppc needs it).
>
> ppc is not used with qemu-kvm, it's used with qemu upstream. qemu-kvm is
> a x86-only show today.
>

Does qemu.git have this hook for ppc?

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

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Jan Kiszka]

On 2012-01-24 14:26, Avi Kivity wrote:
> On 01/24/2012 03:12 PM, Jan Kiszka wrote:
>> On 2012-01-24 14:09, Avi Kivity wrote:
>>> On 01/24/2012 01:38 PM, Jan Kiszka wrote:
>>>>>
>>>>> IIRC introduced for ia64.  But isn't the correct action adding it to
>>>>> qemu.git instead of removing it completely?
>>>>
>>>> Adding something to qemu.git, yes, keeping it here without knowing the
>>>> final code, no. 
>>>
>>> This is called introducing a regression.
>>
>> There is nothing to regress here.
>>
>>>
>>>> This is just another unused variation from upstream.
>>>
>>> It's wasn't unused when ia64 was alive.  Just removing it means more
>>> work later to rediscover it (and anyway ppc needs it).
>>
>> ppc is not used with qemu-kvm, it's used with qemu upstream. qemu-kvm is
>> a x86-only show today.
>>
> 
> Does qemu.git have this hook for ppc?

It was introduced for ia64. That's buried now. Then ppc came along and
may have needed it as well. But ppc became broken in qemu-kvm a long
time ago, may work today again (due to thread model unifications), but
is definitely out of focus for this tree. So these bits are misplaced
here IMHO.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] qemu-kvm: Remove icache flush from cpu_physical_memory_rw

[Alexander Graf]

On 01/24/2012 02:26 PM, Avi Kivity wrote:
> On 01/24/2012 03:12 PM, Jan Kiszka wrote:
>> On 2012-01-24 14:09, Avi Kivity wrote:
>>> On 01/24/2012 01:38 PM, Jan Kiszka wrote:
>>>>> IIRC introduced for ia64.  But isn't the correct action adding it to
>>>>> qemu.git instead of removing it completely?
>>>> Adding something to qemu.git, yes, keeping it here without knowing the
>>>> final code, no.
>>> This is called introducing a regression.
>> There is nothing to regress here.
>>
>>>> This is just another unused variation from upstream.
>>> It's wasn't unused when ia64 was alive.  Just removing it means more
>>> work later to rediscover it (and anyway ppc needs it).
>> ppc is not used with qemu-kvm, it's used with qemu upstream. qemu-kvm is
>> a x86-only show today.
>>
> Does qemu.git have this hook for ppc?

See Scott's patch:

http://patchwork.ozlabs.org/patch/90403/


Alex