[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] out of bounds in set_cc_op()
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] out of bounds in set_cc_op() |
Date: |
Thu, 21 Dec 2017 20:20:40 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 |
Le 21/12/2017 à 15:36, Laurent Vivier a écrit :
> Le 21/12/2017 à 15:14, Paolo Bonzini a écrit :
>> On 21/12/2017 15:13, Laurent Vivier wrote:
>>> Le 21/12/2017 à 15:10, Paolo Bonzini a écrit :
>>>> On 21/12/2017 14:32, Laurent Vivier wrote:
>>>>> Le 21/12/2017 à 14:07, Laurent Vivier a écrit :
>>>>>> Le 21/12/2017 à 13:49, Thomas Huth a écrit :
>>>>>>> On 20.12.2017 22:56, Paolo Bonzini wrote:
>>>>>>>> On 20/12/2017 20:20, Peter Maydell wrote:
>>>>>>>>> On the x86/sanitizer build, new runtime errors:
>>>>>>>>> GTESTER check-qtest-m68k
>>>>>>>>> /home/petmay01/linaro/qemu-for-merges/target/m68k/translate.c:230:12:
>>>>>>>>> runtime error: index -1 out of bounds for type 'const uint8_t [11]'
>>>>>>>>>
>>>>>>>>> ...and similar fails on one or two boards on most of the other
>>>>>>>>> guest architectures.
>>>>>>>>
>>>>>>>> These are preexisting bugs, now exposed by the boot-serial-test.
>>>>>>>> Thomas, can you identify the architectures that have a problem and
>>>>>>>> notify the maintainers? In the meanwhile I'll keep the
>>>>>>>> boot-serial-test
>>>>>>>> enhancements queued locally, and remove them from the pull request.
>>>>>>>
>>>>>>> Laurent, Richard,
>>>>>>>
>>>>>>> looks like old_op is -1 when set_cc_op() is called here for the first
>>>>>>> time. The problem can be reproduced by running the mini-kernel directly.
>>>>>>> Just get http://people.redhat.com/~thuth/m68k-uart.bin and run QEMU like
>>>>>>> this:
>>>>>>>
>>>>>>> qemu-system-m68k -nographic -kernel ~/tmp/m68k-uart.bin -serial none
>>>>>>>
>>>>>>> That kernel only contains these few instructions:
>>>>>>>
>>>>>>> 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */
>>>>>>> 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */
>>>>>>> 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) */
>>>>>>> 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) */
>>>>>>> 0x60, 0xfa /* bra.s loop */
>>>>>>>
>>>>>>> The problem occurs during the second instruction (i.e. the first
>>>>>>> move.b).
>>>>>>>
>>>>>>> Do you have any ideas where this -1 in s->cc_op could come from?
>>>>>>
>>>>>> I think it comes from CCOp: it's the value of CC_OP_DYNAMIC.
>>>>>>
>>>>>> We should not use it to access cc_op_live[].
>>>>>>
>>>>>> I try to find a fix, but I think Richard knows this better than me.
>>>>>
>>>>> This should fix the problem, but I'd like Richard checks it...
>>>>>
>>>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>>>> index b60909222c..721b5801da 100644
>>>>> --- a/target/m68k/translate.c
>>>>> +++ b/target/m68k/translate.c
>>>>> @@ -225,6 +225,11 @@ static void set_cc_op(DisasContext *s, CCOp op)
>>>>> s->cc_op = op;
>>>>> s->cc_op_synced = 0;
>>>>>
>>>>> + if (old_op == CC_OP_DYNAMIC) {
>>>>> + tcg_gen_discard_i32(QREG_CC_OP);
>>>>> + return;
>>>>> + }
>>>>
>>>> This tcg_gen_discard_i32 is correct, but all flags were potentially live
>>>> and can be discarded if the new op uses it(*). So I'd replace "return"
>>>> with "old_op = CC_OP_FLAGS".
>>>
>>> Yes, I agree, we can also have:
>>>
>>> iff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>> index afae5f68ac..5d03764eab 100644
>>> --- a/target/m68k/cpu.h
>>> +++ b/target/m68k/cpu.h
>>> @@ -182,7 +182,7 @@ void cpu_m68k_set_fpcr(CPUM68KState *env, uint32_t val);
>>> */
>>> typedef enum {
>>> /* Translator only -- use env->cc_op. */
>>> - CC_OP_DYNAMIC = -1,
>>> + CC_OP_DYNAMIC,
>>>
>>> /* Each flag bit computed into cc_[xcnvz]. */
>>> CC_OP_FLAGS,
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index b60909222c..61ac1a8e83 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -207,6 +207,7 @@ typedef void (*disas_proc)(CPUM68KState *env,
>>> DisasContext *s, uint16_t insn);
>>> #endif
>>>
>>> static const uint8_t cc_op_live[CC_OP_NB] = {
>>> + [CC_OP_DYNAMIC] = CCF_C | CCF_V | CCF_Z | CCF_N | CCF_X,
>>> [CC_OP_FLAGS] = CCF_C | CCF_V | CCF_Z | CCF_N | CCF_X,
>>> [CC_OP_ADDB ... CC_OP_ADDL] = CCF_X | CCF_N | CCF_V,
>>> [CC_OP_SUBB ... CC_OP_SUBL] = CCF_X | CCF_N | CCF_V,
>>> @@ -237,6 +238,11 @@ static void set_cc_op(DisasContext *s, CCOp op)
>>> if (dead & CCF_V) {
>>> tcg_gen_discard_i32(QREG_CC_V);
>>> }
>>> +
>>> + /* Discard any computed CC_OP value */
>>> + if (old_op == CC_OP_DYNAMIC) {
>>> + tcg_gen_discard_i32(QREG_CC_OP);
>>> + }
>>> }
>>>
>>> /* Update the CPU env CC_OP state. */
>>>
>>>
>>
>> Yes, this is good too. After my pull request is in, feel free to take
>> Thomas's m68k boot-serial-test patch in your tree.
And what about:
tests/boot-serial-test: Add tests for microblaze boards
tests/boot-serial-test: Add a test for the moxiesim machine
tests/boot-serial-test: Add support for the raspi2 machine
?
Thanks,
Laurent
- [Qemu-devel] [PULL 46/46] chardev: convert the socket server to QIONetListener, (continued)
- [Qemu-devel] [PULL 46/46] chardev: convert the socket server to QIONetListener, Paolo Bonzini, 2017/12/20
- Re: [Qemu-devel] [PULL 00/46] First batch of misc patches for QEMU 2.12, Peter Maydell, 2017/12/20
- Re: [Qemu-devel] [PULL 00/46] First batch of misc patches for QEMU 2.12, Paolo Bonzini, 2017/12/20
- Re: [Qemu-devel] out of bounds in set_cc_op() (was: [PULL 00/46] First batch of misc patches for QEMU 2.12), Thomas Huth, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(), Laurent Vivier, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(), Laurent Vivier, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(), Paolo Bonzini, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(), Laurent Vivier, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(), Paolo Bonzini, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(), Laurent Vivier, 2017/12/21
- Re: [Qemu-devel] out of bounds in set_cc_op(),
Laurent Vivier <=
- Re: [Qemu-devel] out of bounds in set_cc_op(), Paolo Bonzini, 2017/12/21
Re: [Qemu-devel] [PULL 00/46] First batch of misc patches for QEMU 2.12, no-reply, 2017/12/20