Re: [PATCH v1 2/3] target/loongarch: Allow user enable/disable LSX/LASX features

[gaosong]

在 2023/10/19 上午7:47, Richard Henderson 写道:

On 10/18/23 01:59, Song Gao wrote:

Some users may not need LSX/LASX, this patch allows the user
enable/disable LSX/LASX features.

  e.g
  '-cpu max,lsx=on,lasx=on'   (default);
  '-cpu max,lsx=on,lasx=off'  (enabled LSX);
  '-cpu max,lsx=off,lasx=on'  (error, need lsx=on);
  '-cpu max,lsx=off'  (disable LSX and LASX).


...


+    /* CPU has LSX */
+    bool has_lsx;
+    /* CPU has  LASX */
+    bool has_lasx;


Why do you need these variables?

I suspect that you've copied them from one of the more complex Arm 
cases where we need to resolve multiple properties simultaneously 
during realize.


You'll get identical behaviour in your current code if you drop these 
and rely only on the CPUCFG2 bits.


If you wanted to do something more complex, you could use OnOffAuto, 
so that you can detect conflicting settings (such as #3 above), but 
not generate an error for


  -cpu foo,lasx=on


Got it, thanks for you suggestion.
where 'foo' is some cpu model which does *no* default lsx=on.  You 
would see that has_lsx==AUTO && has_lasx==ON and then set lsx=ON.


Some cpu model not support lasx or lsx feature,  we should't allow user 
set lsx=on or lasx=on.


I think we need env->features. set the feature when the cpu model 
support this feature.
If the cpu model support the feature,  we allow user set CPUCFG to 
enable/disable this feature.


Do you have more suggestion?

Thanks.
Song Gao

Re: [PATCH v1 2/3] target/loongarch: Allow user enable/disable LSX/LASX features

[Richard Henderson]

On 10/18/23 01:59, Song Gao wrote:

Some users may not need LSX/LASX, this patch allows the user
enable/disable LSX/LASX features.

  e.g
  '-cpu max,lsx=on,lasx=on'   (default);
  '-cpu max,lsx=on,lasx=off'  (enabled LSX);
  '-cpu max,lsx=off,lasx=on'  (error, need lsx=on);
  '-cpu max,lsx=off'  (disable LSX and LASX).


...


+/* CPU has LSX */
+bool has_lsx;
+/* CPU has  LASX */
+bool has_lasx;


Why do you need these variables?

I suspect that you've copied them from one of the more complex Arm cases where we need to 
resolve multiple properties simultaneously during realize.


You'll get identical behaviour in your current code if you drop these and rely only on the 
CPUCFG2 bits.


If you wanted to do something more complex, you could use OnOffAuto, so that you can 
detect conflicting settings (such as #3 above), but not generate an error for


  -cpu foo,lasx=on

where 'foo' is some cpu model which does *no* default lsx=on.  You would see that 
has_lsx==AUTO && has_lasx==ON and then set lsx=ON.



r~

[PATCH v1 2/3] target/loongarch: Allow user enable/disable LSX/LASX features

[Song Gao]

Some users may not need LSX/LASX, this patch allows the user
enable/disable LSX/LASX features.

 e.g
 '-cpu max,lsx=on,lasx=on'   (default);
 '-cpu max,lsx=on,lasx=off'  (enabled LSX);
 '-cpu max,lsx=off,lasx=on'  (error, need lsx=on);
 '-cpu max,lsx=off'  (disable LSX and LASX).

Signed-off-by: Song Gao 
---
 target/loongarch/cpu.c | 64 ++
 target/loongarch/cpu.h |  7 +
 2 files changed, 71 insertions(+)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index ef6922e812..8a47d85d8a 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -478,6 +478,7 @@ static void loongarch_max_initfn(Object *obj)
 {
 /* '-cpu max' for TCG: we use cpu la464. */
 loongarch_la464_initfn(obj);
+loongarch_cpu_post_init(obj);
 }
 
 static void loongarch_cpu_list_entry(gpointer data, gpointer user_data)
@@ -622,6 +623,69 @@ static const MemoryRegionOps loongarch_qemu_ops = {
 };
 #endif
 
+static bool loongarch_get_lsx(Object *obj, Error **errp)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(obj);
+
+if (FIELD_EX32(cpu->env.cpucfg[2], CPUCFG2, LSX)) {
+cpu->has_lsx = true;
+} else {
+cpu->has_lsx = false;
+}
+return cpu->has_lsx;
+}
+
+static void loongarch_set_lsx(Object *obj, bool value, Error **errp)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(obj);
+
+if (value) {
+cpu->env.cpucfg[2] = FIELD_DP32(cpu->env.cpucfg[2], CPUCFG2, LSX, 1);
+} else {
+cpu->env.cpucfg[2] = FIELD_DP32(cpu->env.cpucfg[2], CPUCFG2, LSX, 0);
+cpu->env.cpucfg[2] = FIELD_DP32(cpu->env.cpucfg[2], CPUCFG2, LASX, 0);
+}
+
+cpu->has_lsx = value;
+}
+
+static bool loongarch_get_lasx(Object *obj, Error **errp)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(obj);
+
+if (FIELD_EX32(cpu->env.cpucfg[2], CPUCFG2, LASX)) {
+cpu->has_lasx = true;
+} else {
+cpu->has_lasx = false;
+}
+return cpu->has_lasx;
+}
+
+static void loongarch_set_lasx(Object *obj, bool value, Error **errp)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(obj);
+
+if (value) {
+if (!FIELD_EX32(cpu->env.cpucfg[2], CPUCFG2, LSX)) {
+error_setg(errp, "Enabled LASX, need enabled LSX first!");
+return;
+   }
+cpu->env.cpucfg[2] = FIELD_DP32(cpu->env.cpucfg[2], CPUCFG2, LASX, 1);
+} else {
+cpu->env.cpucfg[2] = FIELD_DP32(cpu->env.cpucfg[2], CPUCFG2, LASX, 0);
+}
+
+cpu->has_lasx = value;
+}
+
+void loongarch_cpu_post_init(Object *obj)
+{
+object_property_add_bool(obj, "lsx", loongarch_get_lsx,
+ loongarch_set_lsx);
+object_property_add_bool(obj, "lasx", loongarch_get_lasx,
+ loongarch_set_lasx);
+}
+
 static void loongarch_cpu_init(Object *obj)
 {
 #ifndef CONFIG_USER_ONLY
diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index 8b54cf109c..d927377c49 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -381,6 +381,11 @@ struct ArchCPU {
 
 /* 'compatible' string for this CPU for Linux device trees */
 const char *dtb_compatible;
+
+/* CPU has LSX */
+bool has_lsx;
+/* CPU has  LASX */
+bool has_lasx;
 };
 
 #define TYPE_LOONGARCH_CPU "loongarch-cpu"
@@ -486,4 +491,6 @@ void loongarch_cpu_list(void);
 #define LOONGARCH_CPU_TYPE_NAME(model) model LOONGARCH_CPU_TYPE_SUFFIX
 #define CPU_RESOLVING_TYPE TYPE_LOONGARCH_CPU
 
+void loongarch_cpu_post_init(Object *obj);
+
 #endif /* LOONGARCH_CPU_H */
-- 
2.25.1