From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NFkNg-0000yD-Fp for qemu-devel@nongnu.org; Wed, 02 Dec 2009 03:16:12 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NFkNe-0000wR-Ik for qemu-devel@nongnu.org; Wed, 02 Dec 2009 03:16:11 -0500 Received: from [199.232.76.173] (port=42782 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NFkNe-0000wO-Eu for qemu-devel@nongnu.org; Wed, 02 Dec 2009 03:16:10 -0500 Received: from mx20.gnu.org ([199.232.41.8]:36385) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NFkNd-0000I1-R1 for qemu-devel@nongnu.org; Wed, 02 Dec 2009 03:16:10 -0500 Received: from hall.aurel32.net ([88.191.82.174]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NFkNc-00051j-S2 for qemu-devel@nongnu.org; Wed, 02 Dec 2009 03:16:09 -0500 Date: Wed, 2 Dec 2009 09:16:07 +0100 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH 03/11] S/390 fake TCG implementation Message-ID: <20091202081607.GS2310@hall.aurel32.net> References: <1259241800-2810-1-git-send-email-agraf@suse.de> <1259241800-2810-4-git-send-email-agraf@suse.de> <20091130181839.GA15790@volta.aurel32.net> <64A99957-57DC-4C8E-A07E-D210CF41AB45@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <64A99957-57DC-4C8E-A07E-D210CF41AB45@suse.de> Sender: Aurelien Jarno List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Carsten Otte , qemu-devel@nongnu.org On Mon, Nov 30, 2009 at 11:27:53PM +0100, Alexander Graf wrote: > > On 30.11.2009, at 19:18, Aurelien Jarno wrote: > > > On Thu, Nov 26, 2009 at 02:23:12PM +0100, Alexander Graf wrote: > >> Qemu won't let us run a KVM target without having host TCG support. Well, for > >> now we don't have any so let's implement a fake target that only stubs out > >> everything. > >> > >> I tried to keep the patch as close to Uli's source as possible, so whenever > >> he feels like it he can easily diff his version against this one. > > > > Please find the comments below. > > > >> Signed-off-by: Alexander Graf > >> --- > >> dyngen-exec.h | 2 +- > >> target-s390x/helper.c | 5 ++ > >> tcg/s390/tcg-target.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> tcg/s390/tcg-target.h | 48 +++++++++++++++++++++++ > >> 4 files changed, 157 insertions(+), 1 deletions(-) > >> create mode 100644 tcg/s390/tcg-target.c > >> create mode 100644 tcg/s390/tcg-target.h > >> > >> diff --git a/dyngen-exec.h b/dyngen-exec.h > >> index 86e61c3..0353f36 100644 > >> --- a/dyngen-exec.h > >> +++ b/dyngen-exec.h > >> @@ -117,7 +117,7 @@ extern int printf(const char *, ...); > >> > >> /* The return address may point to the start of the next instruction. > >> Subtracting one gets us the call instruction itself. */ > >> -#if defined(__s390__) > >> +#if defined(__s390__) && !defined(__s390x__) > >> # define GETPC() ((void*)(((unsigned long)__builtin_return_address(0) & 0x7fffffffUL) - 1)) > >> #elif defined(__arm__) > >> /* Thumb return addresses have the low bit set, so we need to subtract two. > >> diff --git a/target-s390x/helper.c b/target-s390x/helper.c > >> index 4e23b4a..0e222e3 100644 > >> --- a/target-s390x/helper.c > >> +++ b/target-s390x/helper.c > >> @@ -44,6 +44,11 @@ CPUS390XState *cpu_s390x_init(const char *cpu_model) > >> return env; > >> } > >> > >> +target_phys_addr_t cpu_get_phys_page_debug(CPUState *env, target_ulong addr) > >> +{ > >> + return addr; > >> +} > >> + > > > > Why does it appear in this patch? It has nothing to do with TCG support. > > I don't remember. What is this used for anyways? If it is not used, maybe it's better to remove it. > >> void cpu_reset(CPUS390XState *env) > >> { > >> if (qemu_loglevel_mask(CPU_LOG_RESET)) { > >> diff --git a/tcg/s390/tcg-target.c b/tcg/s390/tcg-target.c > >> new file mode 100644 > >> index 0000000..53bbf69 > >> --- /dev/null > >> +++ b/tcg/s390/tcg-target.c > >> @@ -0,0 +1,103 @@ > >> +/* > >> + * Tiny Code Generator for QEMU > >> + * > >> + * Copyright (c) 2009 Ulrich Hecht > >> + * > >> + * Permission is hereby granted, free of charge, to any person obtaining a copy > >> + * of this software and associated documentation files (the "Software"), to deal > >> + * in the Software without restriction, including without limitation the rights > >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > >> + * copies of the Software, and to permit persons to whom the Software is > >> + * furnished to do so, subject to the following conditions: > >> + * > >> + * The above copyright notice and this permission notice shall be included in > >> + * all copies or substantial portions of the Software. > >> + * > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > >> + * THE SOFTWARE. > >> + */ > >> + > >> +static const int tcg_target_reg_alloc_order[] = { > >> +}; > >> + > >> +static const int tcg_target_call_iarg_regs[] = { > >> +}; > >> + > >> +static const int tcg_target_call_oarg_regs[] = { > >> +}; > >> + > >> +static void patch_reloc(uint8_t *code_ptr, int type, > >> + tcg_target_long value, tcg_target_long addend) > >> +{ > >> + tcg_abort(); > >> +} > >> + > >> +static inline int tcg_target_get_call_iarg_regs_count(int flags) > >> +{ > >> + tcg_abort(); > >> + return 0; > >> +} > >> + > >> +/* parse target specific constraints */ > >> +static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str) > >> +{ > >> + tcg_abort(); > >> + return 0; > >> +} > >> + > >> +/* Test if a constant matches the constraint. */ > >> +static inline int tcg_target_const_match(tcg_target_long val, > >> + const TCGArgConstraint *arg_ct) > >> +{ > >> + tcg_abort(); > >> + return 0; > >> +} > >> + > >> +/* load a register with an immediate value */ > >> +static inline void tcg_out_movi(TCGContext *s, TCGType type, > >> + int ret, tcg_target_long arg) > >> +{ > >> + tcg_abort(); > >> +} > >> + > >> +/* load data without address translation or endianness conversion */ > >> +static inline void tcg_out_ld(TCGContext *s, TCGType type, int arg, > >> + int arg1, tcg_target_long arg2) > >> +{ > >> + tcg_abort(); > >> +} > >> + > >> +static inline void tcg_out_st(TCGContext *s, TCGType type, int arg, > >> + int arg1, tcg_target_long arg2) > >> +{ > >> + tcg_abort(); > >> +} > >> + > >> +static inline void tcg_out_op(TCGContext *s, int opc, > >> + const TCGArg *args, const int *const_args) > >> +{ > >> + tcg_abort(); > >> +} > >> + > >> +void tcg_target_init(TCGContext *s) > >> +{ > >> +} > > > > This is probably here that tcg_abort() is actually the most important. > > > >> + > >> +void tcg_target_qemu_prologue(TCGContext *s) > >> +{ > >> +} > >> + > > > > Also adding it here doesn't cost a lot. > > Both places get called with KVM as well. If I call tcg_abort() here KVM doesn't start. Then can you add a comment explaining that code is missing. I am very reluctant in introducing missing/wrong code in qemu. Experience has shown that it stays for a very long time, until someone spend hours or days trying to debug an issue. > > > >> +static inline void tcg_out_mov(TCGContext *s, int ret, int arg) > >> +{ > >> + tcg_abort(); > >> +} > >> + > >> +static inline void tcg_out_addi(TCGContext *s, int reg, tcg_target_long val) > >> +{ > >> + tcg_abort(); > >> +} > >> diff --git a/tcg/s390/tcg-target.h b/tcg/s390/tcg-target.h > >> new file mode 100644 > >> index 0000000..2e66553 > >> --- /dev/null > >> +++ b/tcg/s390/tcg-target.h > >> @@ -0,0 +1,48 @@ > >> +/* > >> + * Tiny Code Generator for QEMU > >> + * > >> + * Copyright (c) 2009 Ulrich Hecht > >> + * > >> + * Permission is hereby granted, free of charge, to any person obtaining a copy > >> + * of this software and associated documentation files (the "Software"), to deal > >> + * in the Software without restriction, including without limitation the rights > >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell > >> + * copies of the Software, and to permit persons to whom the Software is > >> + * furnished to do so, subject to the following conditions: > >> + * > >> + * The above copyright notice and this permission notice shall be included in > >> + * all copies or substantial portions of the Software. > >> + * > >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > >> + * THE SOFTWARE. > >> + */ > >> +#define TCG_TARGET_S390 1 > >> + > >> +#define TCG_TARGET_REG_BITS 64 > >> +#define TCG_TARGET_WORDS_BIGENDIAN > >> + > >> +#define TCG_TARGET_NB_REGS 0 > >> + > >> +enum { > >> + TCG_AREG0 = 0, > >> +}; > > > > As this is defined in Uli's patch, I think it might be a good idea to > > define that correctly. Same for the list of registers. > > I wasn't sure how much of the real architecture I should actually keep. So you think registers belong in? Again what chokes me is that it introduces wrong code. Especially TCG_AREG0 = 0. Introducing the registers will fix that. > >> +/* used for function call generation */ > >> +#define TCG_REG_CALL_STACK 0 > >> +#define TCG_TARGET_STACK_ALIGN 8 > >> +#define TCG_TARGET_CALL_STACK_OFFSET 0 > >> + > >> +static inline void flush_icache_range(unsigned long start, unsigned long stop) > >> +{ > >> +#if QEMU_GNUC_PREREQ(4, 1) > >> + void __clear_cache(char *beg, char *end); > >> + __clear_cache((char *) start, (char *) stop); > > > > You should use __builtin___clear_cache instead to avoid having to > > declare the function. > > Oh, ok :) > > Alex -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurelien@aurel32.net http://www.aurel32.net