Re: [PATCH v1] target/loongarch: Fix qemu-system-loongarch64 assert failed with the option '-d int'

[gaosong]

在 2024/3/21 上午10:50, Richard Henderson 写道:
> On 3/20/24 16:11, Song Gao wrote:
> > qemu-system-loongarch64 assert failed with the option '-d int',
> > the helper_idle() raise an exception EXCP_HLT, but the exception name 
> > is undefined.
> >
> > Signed-off-by: Song Gao <gaosong@loongson.cn>
> > ---
> >   target/loongarch/cpu.c | 75 ++++++++++++++++++++++++++----------------
> >   1 file changed, 46 insertions(+), 29 deletions(-)
> >
> > diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> > index f6ffb3aadb..17a923de02 100644
> > --- a/target/loongarch/cpu.c
> > +++ b/target/loongarch/cpu.c
> > @@ -45,33 +45,46 @@ const char * const fregnames[32] = {
> >       "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31",
> >   };
> >   -static const char * const excp_names[] = {
> > -    [EXCCODE_INT] = "Interrupt",
> > -    [EXCCODE_PIL] = "Page invalid exception for load",
> > -    [EXCCODE_PIS] = "Page invalid exception for store",
> > -    [EXCCODE_PIF] = "Page invalid exception for fetch",
> > -    [EXCCODE_PME] = "Page modified exception",
> > -    [EXCCODE_PNR] = "Page Not Readable exception",
> > -    [EXCCODE_PNX] = "Page Not Executable exception",
> > -    [EXCCODE_PPI] = "Page Privilege error",
> > -    [EXCCODE_ADEF] = "Address error for instruction fetch",
> > -    [EXCCODE_ADEM] = "Address error for Memory access",
> > -    [EXCCODE_SYS] = "Syscall",
> > -    [EXCCODE_BRK] = "Break",
> > -    [EXCCODE_INE] = "Instruction Non-Existent",
> > -    [EXCCODE_IPE] = "Instruction privilege error",
> > -    [EXCCODE_FPD] = "Floating Point Disabled",
> > -    [EXCCODE_FPE] = "Floating Point Exception",
> > -    [EXCCODE_DBP] = "Debug breakpoint",
> > -    [EXCCODE_BCE] = "Bound Check Exception",
> > -    [EXCCODE_SXD] = "128 bit vector instructions Disable exception",
> > -    [EXCCODE_ASXD] = "256 bit vector instructions Disable exception",
> > +struct TypeExcp {
> > +    int32_t exccode;
> > +    const char *name;
> > +};
> > +
> > +static const struct TypeExcp excp_names[] = {
> > +    {EXCCODE_INT, "Interrupt"},
> > +    {EXCCODE_PIL, "Page invalid exception for load"},
> > +    {EXCCODE_PIS, "Page invalid exception for store"},
> > +    {EXCCODE_PIF, "Page invalid exception for fetch"},
> > +    {EXCCODE_PME, "Page modified exception"},
> > +    {EXCCODE_PNR, "Page Not Readable exception"},
> > +    {EXCCODE_PNX, "Page Not Executable exception"},
> > +    {EXCCODE_PPI, "Page Privilege error"},
> > +    {EXCCODE_ADEF, "Address error for instruction fetch"},
> > +    {EXCCODE_ADEM, "Address error for Memory access"},
> > +    {EXCCODE_SYS, "Syscall"},
> > +    {EXCCODE_BRK, "Break"},
> > +    {EXCCODE_INE, "Instruction Non-Existent"},
> > +    {EXCCODE_IPE, "Instruction privilege error"},
> > +    {EXCCODE_FPD, "Floating Point Disabled"},
> > +    {EXCCODE_FPE, "Floating Point Exception"},
> > +    {EXCCODE_DBP, "Debug breakpoint"},
> > +    {EXCCODE_BCE, "Bound Check Exception"},
> > +    {EXCCODE_SXD, "128 bit vector instructions Disable exception"},
> > +    {EXCCODE_ASXD, "256 bit vector instructions Disable exception"},
> >   };
> >     const char *loongarch_exception_name(int32_t exception)
> >   {
> > -    assert(excp_names[exception]);
> > -    return excp_names[exception];
> > +    int i;
> > +    const char *name = "unknown";
> > +
> > +    for (i = 0; i < ARRAY_SIZE(excp_names); i++) {
> > +        if (excp_names[i].exccode == exception) {
> > +            name = excp_names[i].name;
> > +            break;
> > +        }
> > +    }
> > +    return name;
> >   }
>
> I think you should return null for unknown, and then...
>
> >     void G_NORETURN do_raise_exception(CPULoongArchState *env,
> > @@ -79,11 +92,17 @@ void G_NORETURN 
> > do_raise_exception(CPULoongArchState *env,
> >                                      uintptr_t pc)
> >   {
> >       CPUState *cs = env_cpu(env);
> > +    const char *name;
> >   +    if (exception == EXCP_HLT) {
> > +        name = "EXCP_HLT";
> > +    } else {
> > +        name = loongarch_exception_name(exception);
> > +    }
> >       qemu_log_mask(CPU_LOG_INT, "%s: %d (%s)\n",
> >                     __func__,
> >                     exception,
> > -                  loongarch_exception_name(exception));
> > +                  name);
>
> ... use two different printfs, one of which prints the exception number.
> Why would you special case HLT here instead of putting it in the table?
Hmm,  put HLT in the table no problem.  I will correct it.

I considered HLT not a real exception to the LoongAarh architecture, so 
I didn't put it in the table.

Thanks.
Song Gao
> r~