| Message ID | 20231102224445.527355-20-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rv64i and rva22u64 CPUs, RVA22U64 profile support | expand |
On Thu, Nov 02, 2023 at 07:44:45PM -0300, Daniel Henrique Barboza wrote: > There's no gain in allowing the 'max' CPU to support profiles, since it > already contains everything that QEMU can support. And we'll open the > door for 'unorthodox' stuff like users disabling profiles of the 'max' > CPU. I don't see a lot of value in this patch, but maybe I'm just too cruel to users that don't know what they're doing. I even see a negative value to this patch because I can conceive of writing a script where I generally want to use rv64i with my explicit list of profiles/extensions, but then I may want to temporarily "boost" my CPU to 'max' for some reason. If I write my script like CPU=rv64i EXTENSIONS=profile=on,extension=on qemu -cpu $CPU,$EXTENSIONS ... then I can't just do CPU=max ./my-script to boost my CPU, since max will error out when it sees profiles being enabled (even though that should be no-op for it). Instead, I need to do CPU=max EXTENSIONS= ./my-script which isn't horrible, but a bit annoying. So, personally, I would drop this patch. Thanks, drew > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/tcg/tcg-cpu.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c > index 553fb337e7..9a964a426e 100644 > --- a/target/riscv/tcg/tcg-cpu.c > +++ b/target/riscv/tcg/tcg-cpu.c > @@ -825,6 +825,11 @@ static bool riscv_cpu_is_vendor(Object *cpu_obj) > return object_dynamic_cast(cpu_obj, TYPE_RISCV_VENDOR_CPU) != NULL; > } > > +static bool riscv_cpu_has_max_extensions(Object *cpu_obj) > +{ > + return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; > +} > + > /* > * We'll get here via the following path: > * > @@ -1003,6 +1008,12 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, > return; > } > > + if (riscv_cpu_has_max_extensions(obj)) { > + error_setg(errp, "Profile %s is not available for the 'max' CPU", > + profile->name); > + return; > + } > + > if (cpu->env.misa_mxl != MXL_RV64) { > error_setg(errp, "Profile %s only available for 64 bit CPUs", > profile->name); > @@ -1251,11 +1262,6 @@ static void riscv_init_max_cpu_extensions(Object *obj) > } > } > > -static bool riscv_cpu_has_max_extensions(Object *cpu_obj) > -{ > - return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; > -} > - > static void tcg_cpu_instance_init(CPUState *cs) > { > RISCVCPU *cpu = RISCV_CPU(cs); > -- > 2.41.0 >
On 11/3/23 06:01, Andrew Jones wrote: > On Thu, Nov 02, 2023 at 07:44:45PM -0300, Daniel Henrique Barboza wrote: >> There's no gain in allowing the 'max' CPU to support profiles, since it >> already contains everything that QEMU can support. And we'll open the >> door for 'unorthodox' stuff like users disabling profiles of the 'max' >> CPU. > > I don't see a lot of value in this patch, but maybe I'm just too cruel to > users that don't know what they're doing. I even see a negative value to > this patch because I can conceive of writing a script where I generally > want to use rv64i with my explicit list of profiles/extensions, but then > I may want to temporarily "boost" my CPU to 'max' for some reason. If > I write my script like > > CPU=rv64i > EXTENSIONS=profile=on,extension=on > qemu -cpu $CPU,$EXTENSIONS ... > > then I can't just do > > CPU=max ./my-script > > to boost my CPU, since max will error out when it sees profiles being > enabled (even though that should be no-op for it). Instead, I need to > do > > CPU=max EXTENSIONS= ./my-script > > which isn't horrible, but a bit annoying. > > So, personally, I would drop this patch. Fair enough. I wasn't creative enough with scripting to see the value of 'max' and profiles. Let's drop it. Thanks, Daniel > > Thanks, > drew > >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> --- >> target/riscv/tcg/tcg-cpu.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c >> index 553fb337e7..9a964a426e 100644 >> --- a/target/riscv/tcg/tcg-cpu.c >> +++ b/target/riscv/tcg/tcg-cpu.c >> @@ -825,6 +825,11 @@ static bool riscv_cpu_is_vendor(Object *cpu_obj) >> return object_dynamic_cast(cpu_obj, TYPE_RISCV_VENDOR_CPU) != NULL; >> } >> >> +static bool riscv_cpu_has_max_extensions(Object *cpu_obj) >> +{ >> + return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; >> +} >> + >> /* >> * We'll get here via the following path: >> * >> @@ -1003,6 +1008,12 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, >> return; >> } >> >> + if (riscv_cpu_has_max_extensions(obj)) { >> + error_setg(errp, "Profile %s is not available for the 'max' CPU", >> + profile->name); >> + return; >> + } >> + >> if (cpu->env.misa_mxl != MXL_RV64) { >> error_setg(errp, "Profile %s only available for 64 bit CPUs", >> profile->name); >> @@ -1251,11 +1262,6 @@ static void riscv_init_max_cpu_extensions(Object *obj) >> } >> } >> >> -static bool riscv_cpu_has_max_extensions(Object *cpu_obj) >> -{ >> - return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; >> -} >> - >> static void tcg_cpu_instance_init(CPUState *cs) >> { >> RISCVCPU *cpu = RISCV_CPU(cs); >> -- >> 2.41.0 >>
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c index 553fb337e7..9a964a426e 100644 --- a/target/riscv/tcg/tcg-cpu.c +++ b/target/riscv/tcg/tcg-cpu.c @@ -825,6 +825,11 @@ static bool riscv_cpu_is_vendor(Object *cpu_obj) return object_dynamic_cast(cpu_obj, TYPE_RISCV_VENDOR_CPU) != NULL; } +static bool riscv_cpu_has_max_extensions(Object *cpu_obj) +{ + return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; +} + /* * We'll get here via the following path: * @@ -1003,6 +1008,12 @@ static void cpu_set_profile(Object *obj, Visitor *v, const char *name, return; } + if (riscv_cpu_has_max_extensions(obj)) { + error_setg(errp, "Profile %s is not available for the 'max' CPU", + profile->name); + return; + } + if (cpu->env.misa_mxl != MXL_RV64) { error_setg(errp, "Profile %s only available for 64 bit CPUs", profile->name); @@ -1251,11 +1262,6 @@ static void riscv_init_max_cpu_extensions(Object *obj) } } -static bool riscv_cpu_has_max_extensions(Object *cpu_obj) -{ - return object_dynamic_cast(cpu_obj, TYPE_RISCV_CPU_MAX) != NULL; -} - static void tcg_cpu_instance_init(CPUState *cs) { RISCVCPU *cpu = RISCV_CPU(cs);
There's no gain in allowing the 'max' CPU to support profiles, since it already contains everything that QEMU can support. And we'll open the door for 'unorthodox' stuff like users disabling profiles of the 'max' CPU. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/tcg/tcg-cpu.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)