Re: [PATCH v2 5/5] target/riscv: Move/refactor ISA extension checks

From: Weiwei Li <liweiwei@iscas.ac.cn>
To: Tsukasa OI <research_trasio@irq.a4lg.com>,
	Alistair Francis <alistair23@gmail.com>,
	Frank Chang <frank.chang@sifive.com>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org
Subject: Re: [PATCH v2 5/5] target/riscv: Move/refactor ISA extension checks
Date: Mon, 16 May 2022 11:12:29 +0800	[thread overview]
Message-ID: <afb58d24-4ba9-7229-8aed-870593222516@iscas.ac.cn> (raw)
In-Reply-To: <c3145fa37a529484cf3047c8cb9841e9effad4b0.1652583332.git.research_trasio@irq.a4lg.com>

[-- Attachment #1: Type: text/plain, Size: 3230 bytes --]

在 2022/5/15 上午10:56, Tsukasa OI 写道:
> We should separate "check" and "configure" steps as possible.
> This commit separates both steps except vector/Zfinx-related checks.
>
> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
> ---
>   target/riscv/cpu.c | 31 ++++++++++++++++---------------
>   1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index f910a41407..5ab246bf63 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -630,14 +630,27 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>               return;
>           }
>   
> +        if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> +            error_setg(errp, "Zve32f/Zve64f extensions require F extension");
> +            return;
> +        }
> +
> +        /* Set the ISA extensions, checks should have happened above */
>           if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
>               cpu->cfg.ext_zhinxmin) {
>               cpu->cfg.ext_zfinx = true;
>           }
>   
> -        if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) {
> -            error_setg(errp, "Zfinx extension requires Zicsr");
> -            return;
> +        if (cpu->cfg.ext_zfinx) {
> +            if (!cpu->cfg.ext_icsr) {
> +                error_setg(errp, "Zfinx extension requires Zicsr");
> +                return;
> +            }
> +            if (cpu->cfg.ext_f) {
> +                error_setg(errp,
> +                    "Zfinx cannot be supported together with F extension");
> +                return;
> +            }
>           }

I think these checks for non-single-letter extensions are  better to 
move  out of the 'if (env->misa_ext == 0)) { ...}', since they are enabled

directly by cfg property, such as we can set cpu option to sifive-u34 
with zfinx=true. This may not be a proper way to set cpu option,

However it's truly a legal command option, but  configure an illegal 
supported ISA which enable both f and zfinx.

Regards,

Weiwei Li

>   
>           if (cpu->cfg.ext_zk) {
> @@ -663,7 +676,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>               cpu->cfg.ext_zksh = true;
>           }
>   
> -        /* Set the ISA extensions, checks should have happened above */
>           if (cpu->cfg.ext_i) {
>               ext |= RVI;
>           }
> @@ -734,20 +746,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>               }
>               set_vext_version(env, vext_version);
>           }
> -        if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> -            error_setg(errp, "Zve32f/Zve64f extension depends upon RVF.");
> -            return;
> -        }
>           if (cpu->cfg.ext_j) {
>               ext |= RVJ;
>           }
> -        if (cpu->cfg.ext_zfinx && ((ext & (RVF | RVD)) || cpu->cfg.ext_zfh ||
> -                                   cpu->cfg.ext_zfhmin)) {
> -            error_setg(errp,
> -                    "'Zfinx' cannot be supported together with 'F', 'D', 'Zfh',"
> -                    " 'Zfhmin'");
> -            return;
> -        }
>   
>           set_misa(env, env->misa_mxl, ext);
>       }

[-- Attachment #2: Type: text/html, Size: 4059 bytes --]