Re: [Qemu-devel] [PATCH 03/11] S/390 fake TCG implementation

[Alexander Graf]

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.

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