Re: [PATCH 5/6] target/ppc: move msgclrp/msgsndp to decodetree

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 10/20/22 09:18, Matheus K. Ferst wrote:
> On 19/10/2022 17:59, Daniel Henrique Barboza wrote:
>> Matheus,
>>
>> This patch fails ppc-softmmu emulation:
>>
>>
>> FAILED: libqemu-ppc-softmmu.fa.p/target_ppc_translate.c.o
>> cc -m64 -mcx16 -Ilibqemu-ppc-softmmu.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../dtc/libfdt -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/pixman-1 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/danielhb/qemu/linux-headers -isystem linux-headers -iquote . -iquote /home/danielhb/qemu -iquote /home/danielhb/qemu/include -iquote /home/danielhb/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi 
>> -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc-softmmu-config-target.h"' '-DCONFIG_DEVICES="ppc-softmmu-config-devices.h"' -MD -MQ libqemu-ppc-softmmu.fa.p/target_ppc_translate.c.o -MF libqemu-ppc-softmmu.fa.p/target_ppc_translate.c.o.d -o libqemu-ppc-softmmu.fa.p/target_ppc_translate.c.o -c ../target/ppc/translate.c
>> In file included from ../target/ppc/translate.c:21:
>> In function ‘trans_MSGCLRP’,
>>      inlined from ‘decode_insn32’ at libqemu-ppc-softmmu.fa.p/decode-insn32.c.inc:3250:21,
>>      inlined from ‘ppc_tr_translate_insn’ at ../target/ppc/translate.c:7552:15:
>> /home/danielhb/qemu/include/qemu/osdep.h:184:35: error: call to ‘qemu_build_not_reached_always’ declared with attribute error: code path is reachable
>>    184 | #define qemu_build_not_reached()  qemu_build_not_reached_always()
>>        |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../target/ppc/translate/processor-ctrl-impl.c.inc:79:5: note: in expansion of macro ‘qemu_build_not_reached’
>>     79 |     qemu_build_not_reached();
>>        |     ^~~~~~~~~~~~~~~~~~~~~~
>>
>> The error is down there:
>>
> 
> Hmm, that's strange. I always build ppc-softmmu on my tests and I'm not seeing this error. I'm using gcc 9.4 though, maybe it's time to update my compiler...
> 
>>
>>
>>
>> On 10/6/22 17:06, Matheus Ferst wrote:
>>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>> ---
>>>   target/ppc/insn32.decode                      |  2 ++
>>>   target/ppc/translate.c                        | 26 -------------------
>>>   .../ppc/translate/processor-ctrl-impl.c.inc   | 24 +++++++++++++++++
>>>   3 files changed, 26 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
>>> index bba49ded1b..5ba4a6807d 100644
>>> --- a/target/ppc/insn32.decode
>>> +++ b/target/ppc/insn32.decode
>>> @@ -913,3 +913,5 @@ TLBIEL          011111 ..... - .. . . ..... 0100010010 -            @X_tlbie
>>>
>>>   MSGCLR          011111 ----- ----- ..... 0011101110 -   @X_rb
>>>   MSGSND          011111 ----- ----- ..... 0011001110 -   @X_rb
>>> +MSGCLRP         011111 ----- ----- ..... 0010101110 -   @X_rb
>>> +MSGSNDP         011111 ----- ----- ..... 0010001110 -   @X_rb
>>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>>> index 889cca6325..087ab8e69d 100644
>>> --- a/target/ppc/translate.c
>>> +++ b/target/ppc/translate.c
>>> @@ -6241,28 +6241,6 @@ static void gen_icbt_440(DisasContext *ctx)
>>>
>>>   /* Embedded.Processor Control */
>>>
>>> -#if defined(TARGET_PPC64)
>>> -static void gen_msgclrp(DisasContext *ctx)
>>> -{
>>> -#if defined(CONFIG_USER_ONLY)
>>> -    GEN_PRIV(ctx);
>>> -#else
>>> -    CHK_SV(ctx);
>>> -    gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>>> -#endif /* defined(CONFIG_USER_ONLY) */
>>> -}
>>> -
>>> -static void gen_msgsndp(DisasContext *ctx)
>>> -{
>>> -#if defined(CONFIG_USER_ONLY)
>>> -    GEN_PRIV(ctx);
>>> -#else
>>> -    CHK_SV(ctx);
>>> -    gen_helper_book3s_msgsndp(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>>> -#endif /* defined(CONFIG_USER_ONLY) */
>>> -}
>>> -#endif
>>> -
>>>   static void gen_msgsync(DisasContext *ctx)
>>>   {
>>>   #if defined(CONFIG_USER_ONLY)
>>> @@ -6896,10 +6874,6 @@ GEN_HANDLER(vmladduhm, 0x04, 0x11, 0xFF, 0x00000000, PPC_ALTIVEC),
>>>   GEN_HANDLER_E(maddhd_maddhdu, 0x04, 0x18, 0xFF, 0x00000000, PPC_NONE,
>>>                 PPC2_ISA300),
>>>   GEN_HANDLER_E(maddld, 0x04, 0x19, 0xFF, 0x00000000, PPC_NONE, PPC2_ISA300),
>>> -GEN_HANDLER2_E(msgsndp, "msgsndp", 0x1F, 0x0E, 0x04, 0x03ff0001,
>>> -               PPC_NONE, PPC2_ISA207S),
>>> -GEN_HANDLER2_E(msgclrp, "msgclrp", 0x1F, 0x0E, 0x05, 0x03ff0001,
>>> -               PPC_NONE, PPC2_ISA207S),
>>>   #endif
>>>
>>>   #undef GEN_INT_ARITH_ADD
>>> diff --git a/target/ppc/translate/processor-ctrl-impl.c.inc b/target/ppc/translate/processor-ctrl-impl.c.inc
>>> index 0192b45c8f..3703001f31 100644
>>> --- a/target/ppc/translate/processor-ctrl-impl.c.inc
>>> +++ b/target/ppc/translate/processor-ctrl-impl.c.inc
>>> @@ -68,3 +68,27 @@ static bool trans_MSGSND(DisasContext *ctx, arg_X_rb *a)
>>>   #endif
>>>       return true;
>>>   }
>>> +
>>> +static bool trans_MSGCLRP(DisasContext *ctx, arg_X_rb *a)
>>> +{
>>> +    REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>>> +    REQUIRE_SV(ctx);
>>> +#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>> +    gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[a->rb]);
>>> +#else
>>> +    qemu_build_not_reached();
>>
>>
>> ^ here. And also
>>
>>
>>
>>> +#endif
>>> +    return true;
>>> +}
>>> +
>>> +static bool trans_MSGSNDP(DisasContext *ctx, arg_X_rb *a)
>>> +{
>>> +    REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>>> +    REQUIRE_SV(ctx);
>>> +#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>> +    gen_helper_book3s_msgsndp(cpu_env, cpu_gpr[a->rb]);
>>> +#else
>>> +    qemu_build_not_reached();
>>
>>
>> ^ here. It seems like you're filtering for TARGET_PPC64 but you're not
>> guarding for it, and the qemu_build_not_reached() is being hit.
>>
>>
>> I fixed by squashing this in:
>>
>> diff --git a/target/ppc/translate/processor-ctrl-impl.c.inc b/target/ppc/translate/processor-ctrl-impl.c.inc
>> index f253226a75..ff231db1af 100644
>> --- a/target/ppc/translate/processor-ctrl-impl.c.inc
>> +++ b/target/ppc/translate/processor-ctrl-impl.c.inc
>> @@ -73,6 +73,7 @@ static bool trans_MSGCLRP(DisasContext *ctx, arg_X_rb *a)
>>   {
>>       REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>>       REQUIRE_SV(ctx);
>> +    REQUIRE_64BIT(ctx);
>>   #if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>       gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[a->rb]);
>>   #else
>> @@ -85,6 +86,7 @@ static bool trans_MSGSNDP(DisasContext *ctx, arg_X_rb *a)
>>   {
>>       REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>>       REQUIRE_SV(ctx);
>> +    REQUIRE_64BIT(ctx);
>>   #if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>>       gen_helper_book3s_msgsndp(cpu_env, cpu_gpr[a->rb]);
>>   #else
>>
>>
>> If you think this fix is acceptable you can consider this patch acked as well.
>>
> 
> It shouldn't matter since we only want to make the compiler happy, but we should check instruction flags before privilege, so we throw POWERPC_EXCP_INVAL_INVAL instead of POWERPC_EXCP_PRIV_OPC if the CPU doesn't have the instruction:
> 
>  > @@ -73,6 +73,7 @@ static bool trans_MSGCLRP(DisasContext *ctx, arg_X_rb *a)
>  >   {
>  > +     REQUIRE_64BIT(ctx);
>  >       REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>  >       REQUIRE_SV(ctx);
>  >   #if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>  >       gen_helper_book3s_msgclrp(cpu_env, cpu_gpr[a->rb]);
>  >   #else
>  > @@ -85,6 +86,7 @@ static bool trans_MSGSNDP(DisasContext *ctx, arg_X_rb *a)
>  >   {
>  > +     REQUIRE_64BIT(ctx);
>  >       REQUIRE_INSNS_FLAGS2(ctx, ISA207S);
>  >       REQUIRE_SV(ctx);
>  >   #if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
>  >       gen_helper_book3s_msgsndp(cpu_env, cpu_gpr[a->rb]);
>  >   #else
> 
> Since all CPUs with ISA207S are 64-bit, it shouldn't make any difference in this context, but someone might use this code as an example, so it's better to have these checks in the correct order. Do you want me to resend with this change?


Nah it's ok. I'll move the REQUIRE_64BIT() call to the start of the
function in-tree.


Thanks,

Daniel

> 
> Thanks,
> Matheus K. Ferst
> Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
> Analista de Software
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
>