[Qemu-devel] [PATCH] translate: remove file translate-all.h

[Qemu-devel] [PATCH] translate: remove file translate-all.h

[Xuebing Wang]

This patch does below:
-   Move the declaration of 2 translate functions from translate-all.h into
    include/exec/exec-all.h
-   remove file translate-all.h

"translate-all.h" => "exec/exec-all.h" can be done by:
git grep -w "translate-all.h" | cut -d: -f1 |
    xargs sed -i 's/\<translate-all.h\>/exec\/exec-all.h/g'

Note:
1)  "exact whole word match" is considered.
2)  We may move translate related from include/exec/exec-all.h into
    include/exec/translate.h later.

Signed-off-by: Xuebing Wang <xbing6@gmail.com>
---
 exec.c                  |    2 +-
 include/exec/exec-all.h |    4 ++++
 translate-all.c         |    2 +-
 translate-all.h         |   27 ---------------------------
 4 files changed, 6 insertions(+), 29 deletions(-)
 delete mode 100644 translate-all.h

diff --git a/exec.c b/exec.c
index b69fd29..ec3890e 100644
--- a/exec.c
+++ b/exec.c
@@ -47,7 +47,7 @@
 #include "exec/cpu-all.h"
 
 #include "exec/cputlb.h"
-#include "translate-all.h"
+#include "exec/exec-all.h"
 
 #include "exec/memory-internal.h"
 #include "exec/ram_addr.h"
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index a387922..f9afb7a 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -213,6 +213,10 @@ void tb_free(TranslationBlock *tb);
 void tb_flush(CPUArchState *env);
 void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr);
 
+/* translate-all.c */
+void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len);
+void tb_check_watchpoint(CPUArchState *env);
+
 #if defined(USE_DIRECT_JUMP)
 
 #if defined(CONFIG_TCG_INTERPRETER)
diff --git a/translate-all.c b/translate-all.c
index 1ac0246..6379b1a 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -57,7 +57,7 @@
 #endif
 
 #include "exec/cputlb.h"
-#include "translate-all.h"
+#include "exec/exec-all.h"
 #include "qemu/timer.h"
 
 //#define DEBUG_TB_INVALIDATE
diff --git a/translate-all.h b/translate-all.h
deleted file mode 100644
index f7e5932..0000000
--- a/translate-all.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/*
- *  Translated block handling
- *
- *  Copyright (c) 2003 Fabrice Bellard
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- */
-#ifndef TRANSLATE_ALL_H
-#define TRANSLATE_ALL_H
-
-/* translate-all.c */
-void tb_invalidate_phys_page_fast(tb_page_addr_t start, int len);
-void cpu_unlink_tb(CPUState *cpu);
-void tb_check_watchpoint(CPUArchState *env);
-
-#endif /* TRANSLATE_ALL_H */
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] translate: remove file translate-all.h

[Peter Maydell]

On 26 February 2014 09:25, Xuebing Wang <xbing6@gmail.com> wrote:
> This patch does below:
> -   Move the declaration of 2 translate functions from translate-all.h into
>     include/exec/exec-all.h
> -   remove file translate-all.h
>
> "translate-all.h" => "exec/exec-all.h" can be done by:
> git grep -w "translate-all.h" | cut -d: -f1 |
>     xargs sed -i 's/\<translate-all.h\>/exec\/exec-all.h/g'
>
> Note:
> 1)  "exact whole word match" is considered.
> 2)  We may move translate related from include/exec/exec-all.h into
>     include/exec/translate.h later.

Is there any particular benefit to this change? The function
prototypes go from being in a header only included by the file
that uses them to being in a header that's included by a lot
of other code...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] translate: remove file translate-all.h

[Paolo Bonzini]

Il 26/02/2014 10:55, Peter Maydell ha scritto:
> On 26 February 2014 09:25, Xuebing Wang <xbing6@gmail.com> wrote:
>> This patch does below:
>> -   Move the declaration of 2 translate functions from translate-all.h into
>>     include/exec/exec-all.h
>> -   remove file translate-all.h
>>
>> "translate-all.h" => "exec/exec-all.h" can be done by:
>> git grep -w "translate-all.h" | cut -d: -f1 |
>>     xargs sed -i 's/\<translate-all.h\>/exec\/exec-all.h/g'
>>
>> Note:
>> 1)  "exact whole word match" is considered.
>> 2)  We may move translate related from include/exec/exec-all.h into
>>     include/exec/translate.h later.
>
> Is there any particular benefit to this change? The function
> prototypes go from being in a header only included by the file
> that uses them to being in a header that's included by a lot
> of other code...

Indeed, that's the point of translate-all.h.

Paolo

Re: [Qemu-devel] [PATCH] translate: remove file translate-all.h

[Xuebing wang]

Hi Peter/Paolo,

Thanks for replying.

1) I am wondering if some slight refactoring (in the sense of 
superficial change of moving things around) can make 
tcg/translationblock/cpu/exec/memory/softmmu more understandable, 
especially for new engineers.

My goal is to explore the possibility of implementing tcg 
multi-threading, in a similar fashion of kvm multi-threading. It's still 
at early stage of a work-in-progress.

As I am new to qemu code base, I spent some time trying to figure out 
the design. Examples are:
-- What does exec (as in the filename of exec.c) really mean?
-- What does "all" mean as in kvm-all,c (xen-all.c, translate-all.c)? I 
assume "all" means "target independent". But, aren't other files (e.g. 
block.c, gdbstub.c) target independent?
-- What is the difference between "all" and "common" as in 
include/exec/cpu-all.h and cpu-common.h?

In my local repository, I did some refactoring. Examples are:
-- I renamed cpu-exec.c => tcg.c, to reflect the design that it's about 
one of the 3 accelerators (tcg, kvm, xen).
-- I renamed cpus.c => vcpu.c (this can be tricky). My idea of the 
object hierarchy is:
[   qom/Object                   ]
[   qdev/DeviceState         ]
[   qom/CPUState               ]
[   vcpu API (or qom/vcpu)  ]   [  CPUArchState  ]
(in the other words, I am trying to abstract/model vcpu)

-- I renamed include/sysemu/cpus.h => include/exec/vcpu.h
-- And other changes. Examples:
1) cpu_exec_init_all() in exec.c is about memory, and its name is kind 
of confusing
2) include/exec/address-spaces.h says below, but "git grep -nw 
address-spaces.h" reveals a lot.
/*
  * Internal interfaces between memory.c/exec.c/vl.c.  Do not #include 
unless
  * you're one of them.
  */

I am wondering if we should slightly re-factor the organization of 
files, thus new engineers will be easier to understand the design about 
tcg/exec.


2) Theoretically, include/exec/exec-all.h should work when included from 
*ANY files*, even mistakenly, right?

I hope this patch does NOT do any harm.

In my local repository, I split TranslationBlock related from it to be 
another file include/translate.h.

I will see what is the best way to "git send-email" these refactoring 
for the community to review, without causing too much confusion. The 
purpose is to make the design easier to understand for new engineers.


Thanks again.


On 02/26/2014 06:34 PM, Paolo Bonzini wrote:
> Il 26/02/2014 10:55, Peter Maydell ha scritto:
>> On 26 February 2014 09:25, Xuebing Wang <xbing6@gmail.com> wrote:
>>> This patch does below:
>>> -   Move the declaration of 2 translate functions from 
>>> translate-all.h into
>>>     include/exec/exec-all.h
>>> -   remove file translate-all.h
>>>
>>> "translate-all.h" => "exec/exec-all.h" can be done by:
>>> git grep -w "translate-all.h" | cut -d: -f1 |
>>>     xargs sed -i 's/\<translate-all.h\>/exec\/exec-all.h/g'
>>>
>>> Note:
>>> 1)  "exact whole word match" is considered.
>>> 2)  We may move translate related from include/exec/exec-all.h into
>>>     include/exec/translate.h later.
>>
>> Is there any particular benefit to this change? The function
>> prototypes go from being in a header only included by the file
>> that uses them to being in a header that's included by a lot
>> of other code...
>
> Indeed, that's the point of translate-all.h.
>
> Paolo
>
>

-- 
Thanks,
Xuebing Wang