[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 03/11] S/390 fake TCG implementation
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH 03/11] S/390 fake TCG implementation |
Date: |
Wed, 2 Dec 2009 09:41:50 +0100 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
On Wed, Dec 02, 2009 at 09:29:59AM +0100, Alexander Graf wrote:
>
> On 02.12.2009, at 09:16, Aurelien Jarno wrote:
>
> > 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 <address@hidden>
> >>>> ---
> >>>> 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.
>
> It's called from exec.c.
Then it's clearly not part of this patch, it should probably be part of
patch 1 or 6 instead.
> >
> >>>> 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 <address@hidden>
> >>>> + *
> >>>> + * 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.
>
> Alright.
>
> >
> >>>
> >>>> +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 <address@hidden>
> >>>> + *
> >>>> + * 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.
>
> Ok.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
address@hidden http://www.aurel32.net