qemu-kvm: Remove icache flush from cpu_physical_memory_rw

PATCH Superseded Jan Kiszka 5 participants 13 messages Jan 19, 2012 – Jan 25, 2012 KVM

Share & tools

Jan Kiszka

author

2012-01-19 12:39 UTC #0 lore raw

PATCH 1/1 qemu-kvm: Remove icache flush from cpu_physical_memory_rw

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 <[email protected]>

---
 exec.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

exec.c +0 -7

index db837e2..0ca17fc 100644
@@ -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 {

Signature · 1 line

1.7.3.4

Marcelo Tosatti

2012-01-19 17:55 UTC #1 lore raw

Quoted 3 lines · 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.

[1] ?

Jan Kiszka

author

2012-01-19 18:04 UTC #2 lore raw

PATCH v2 1/1 qemu-kvm: Remove icache flush from cpu_physical_memory_rw

Quoted 7 lines · Marcelo Tosatti

Quoted 3 lines · 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.
[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 <[email protected]>

---
 exec.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

exec.c +0 -7

index db837e2..0ca17fc 100644
@@ -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 {

Signature · 1 line

1.7.3.4

Marcelo Tosatti

Applied

2012-01-20 17:26 UTC #3 lore raw

Quoted 13 lines · Jan Kiszka

Quoted 7 lines · Marcelo Tosatti
Quoted 3 lines · 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.
[1] ?
Oops.
--------8<---------

Quoted 7 lines

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 <[email protected]>

Applied, thanks.

Scott Wood

2012-01-24 00:26 UTC #4 lore raw

In reply to Jan Kiszka

Quoted 11 lines · Jan Kiszka

Quoted 7 lines · Marcelo Tosatti
Quoted 3 lines · 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.
[1] ?
Oops.

Quoted 7 lines

--------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

Jan Kiszka

author

2012-01-24 08:57 UTC #5 lore raw

Quoted 18 lines · Scott Wood

Quoted 17 lines · Jan Kiszka
Quoted 7 lines · Marcelo Tosatti
Quoted 3 lines · 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.
[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.

Quoted 6 lines

Quoted 1 line
[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

Signature · 2 lines

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

Avi Kivity

2012-01-24 11:34 UTC #6 lore raw

Quoted 9 lines · Jan Kiszka

Quoted 4 lines
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?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2012-01-24 11:38 UTC #7 lore raw

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka
Quoted 4 lines
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.

Quoted 7 lines

Quoted 4 lines
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

Signature · 2 lines

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

Avi Kivity

2012-01-24 13:10 UTC #8 lore raw

Quoted 6 lines · Jan Kiszka

Quoted 3 lines
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.

Quoted 1 line

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).

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2012-01-24 13:12 UTC #9 lore raw

Quoted 9 lines · Avi Kivity

Quoted 6 lines · Jan Kiszka
Quoted 3 lines
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.

Quoted 5 lines

Quoted 2 lines
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

Signature · 2 lines

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

Avi Kivity

2012-01-24 13:26 UTC #10 lore raw

Quoted 14 lines · Jan Kiszka

Quoted 9 lines · Avi Kivity
Quoted 6 lines · Jan Kiszka
Quoted 3 lines
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.
Quoted 1 line

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
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?

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

author

2012-01-24 13:29 UTC #11 lore raw

Quoted 17 lines · Avi Kivity

Quoted 16 lines · Jan Kiszka
Quoted 9 lines · Avi Kivity
Quoted 6 lines · Jan Kiszka
Quoted 3 lines
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.
Quoted 3 lines
Quoted 2 lines
This is just another unused variation from upstream.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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

Signature · 2 lines

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

Alexander Graf

2012-01-25 14:23 UTC #12 lore raw

In reply to Avi Kivity

Quoted 10 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka
Quoted 6 lines · Avi Kivity
Quoted 4 lines · Jan Kiszka
Quoted 2 lines
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.

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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