All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] target/arm: Support variable sized coprocessor registers
@ 2022-04-11  6:58 Gavin Shan
  2022-04-11  6:58 ` [PATCH 1/5] target/arm/tcg: Indirect addressing for coprocessor register storage Gavin Shan
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  6:58 UTC (permalink / raw)
  To: qemu-arm
  Cc: peter.maydell, drjones, richard.henderson, qemu-devel,
	eric.auger, agraf, shan.gavin, pbonzini

There are two arrays for each CPU, to store the indexes and values of the
coprocessor registers. Currently, 8 bytes fixed storage space is reserved
for each coprocessor register. However, larger coprocessor registers have
been defined and exposed by KVM. Except SVE registers, no coprocessor
register exceeds 8 bytes in size. It doesn't mean large coprocessor registers
won't be exploited in future. For example, I'm looking into SDEI virtualization
support, which isn't merged into Linux upstream yet. I have plan to add
several coprocessor ("firmware pseudo") registers to assist the migration.

This series adds one more array, to track the position or location in the
storage array (@cpreg_values) for the corresponding coprocessor register.
The storage space for one particular coprocessor register is always to
8 bytes so that we needn't worry about the alignment issue. In this way,
the coprocessor register size can be variable.

I had some internal discussion with Eric and Drew. They suggested to
send one mail to qemu-arm@nongnu.org, asking if there is any challenges
to support variable sized coprocessor registers. So another intention
of this series is to invoke the discussion.

PATCH[1-3] adds one more array (@cpreg_value_indexes) to track the location
in the storage array (@cpreg_values) for coprocessor registers. The storage
space for one particular coprocessor register is determined by the additional
array, which is named as indirect addressing mode. Each coprocessor register
is still having 8 bytes fixed storage space, so that thd old mechanism
(direct addressing mode) and indirect address mode can co-exist, event in
migration circumstance. PATCH[4] migrates the additional array. PATCH[5]
initializes @cpreg_value_indexes for KVM.

Gavin Shan (5):
  target/arm/tcg: Indirect addressing for coprocessor register storage
  target/arm/hvf: Indirect addressing for coprocessor register storage
  target/arm/kvm: Indirect addressing for coprocessor register storage
  target/arm: Migrate coprocessor register indirect addressing
    information
  target/arm/kvm: Support coprocessor register with variable size

 target/arm/cpu.h     | 12 +++++--
 target/arm/helper.c  | 27 +++++++++-----
 target/arm/hvf/hvf.c | 20 +++++++++--
 target/arm/kvm.c     | 85 ++++++++++++++++++++++++++++++++++++++------
 target/arm/machine.c | 30 ++++++++++++----
 5 files changed, 145 insertions(+), 29 deletions(-)

-- 
2.23.0



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/5] target/arm/tcg: Indirect addressing for coprocessor register storage
  2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
@ 2022-04-11  6:58 ` Gavin Shan
  2022-04-11  6:58 ` [PATCH 2/5] target/arm/hvf: " Gavin Shan
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  6:58 UTC (permalink / raw)
  To: qemu-arm
  Cc: peter.maydell, drjones, richard.henderson, qemu-devel,
	eric.auger, agraf, shan.gavin, pbonzini

Currently, there is an array used as the storage for the coprocessor
registers. Each element in the array occupies for 8 bytes. It means
we have the assumption that the size of coprocessor can't exceed
8 bytes. The storage mechanism is used by KVM either. Unfortunately,
the assumption is conflicting with KVM's pseudo firmware registers,
whose sizes can be variable and exceeding 8 bytes. So the storage
scheme isn't working for KVM's pseudo firmware registers.

This introduces another array (@cpreg_value_indexes) to track the
storage location in @cpreg_values for the corresponding coprocessor
register. @cpreg_value_array_len is also added to track the total
storage size for all coprocessor registers. After that, the storage
is addressed indirectly by @cpreg_values[cpreg_value_indexes[i]].

For TCG case, each coprocessor register still has fixed 8 bytes
storage space. So the old direct addressing mechanism and new
indirect addressing mechanism can co-exist and interchangeable,
even in migration circumstance.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/cpu.h    | 12 ++++++++++--
 target/arm/helper.c | 27 +++++++++++++++++++--------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 23879de5fa..0129791b3f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -795,17 +795,25 @@ struct ArchCPU {
      * 64 bit indexes, not CPRegInfo 32 bit indexes)
      */
     uint64_t *cpreg_indexes;
-    /* Values of the registers (cpreg_indexes[i]'s value is cpreg_values[i]) */
+    /*
+     * Values of the registers
+     * (cpreg_indexes[i]'s value is cpreg_values[cpreg_value_indexes[i]])
+     */
+    uint32_t *cpreg_value_indexes;
     uint64_t *cpreg_values;
-    /* Length of the indexes, values, reset_values arrays */
+    /* Length of the indexes, value indexes and values arrays */
     int32_t cpreg_array_len;
+    int32_t cpreg_value_array_len;
+
     /* These are used only for migration: incoming data arrives in
      * these fields and is sanity checked in post_load before copying
      * to the working data structures above.
      */
     uint64_t *cpreg_vmstate_indexes;
+    uint32_t *cpreg_vmstate_value_indexes;
     uint64_t *cpreg_vmstate_values;
     int32_t cpreg_vmstate_array_len;
+    int32_t cpreg_vmstate_value_array_len;
 
     DynamicGDBXMLInfo dyn_sysreg_xml;
     DynamicGDBXMLInfo dyn_svereg_xml;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 7d14650615..e8cb4a9edb 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -163,7 +163,7 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync)
              * item in the list, we just recheck "does the raw write we must
              * have made in write_list_to_cpustate() read back OK" here.
              */
-            uint64_t oldval = cpu->cpreg_values[i];
+            uint64_t oldval = cpu->cpreg_values[cpu->cpreg_value_indexes[i]];
 
             if (oldval == newval) {
                 continue;
@@ -176,7 +176,7 @@ bool write_cpustate_to_list(ARMCPU *cpu, bool kvm_sync)
 
             write_raw_cp_reg(&cpu->env, ri, newval);
         }
-        cpu->cpreg_values[i] = newval;
+        cpu->cpreg_values[cpu->cpreg_value_indexes[i]] = newval;
     }
     return ok;
 }
@@ -188,7 +188,7 @@ bool write_list_to_cpustate(ARMCPU *cpu)
 
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]);
-        uint64_t v = cpu->cpreg_values[i];
+        uint64_t v = cpu->cpreg_values[cpu->cpreg_value_indexes[i]];
         const ARMCPRegInfo *ri;
 
         ri = get_arm_cp_reginfo(cpu->cp_regs, regidx);
@@ -222,8 +222,12 @@ static void add_cpreg_to_list(gpointer key, gpointer opaque)
 
     if (!(ri->type & (ARM_CP_NO_RAW|ARM_CP_ALIAS))) {
         cpu->cpreg_indexes[cpu->cpreg_array_len] = cpreg_to_kvm_id(regidx);
+        cpu->cpreg_value_indexes[cpu->cpreg_array_len] =
+            cpu->cpreg_value_array_len;
+
         /* The value array need not be initialized at this point */
         cpu->cpreg_array_len++;
+        cpu->cpreg_value_array_len++;
     }
 }
 
@@ -238,6 +242,7 @@ static void count_cpreg(gpointer key, gpointer opaque)
 
     if (!(ri->type & (ARM_CP_NO_RAW|ARM_CP_ALIAS))) {
         cpu->cpreg_array_len++;
+        cpu->cpreg_value_array_len++;
     }
 }
 
@@ -261,26 +266,32 @@ void init_cpreg_list(ARMCPU *cpu)
      * Note that we require cpreg_tuples[] to be sorted by key ID.
      */
     GList *keys;
-    int arraylen;
+    int arraylen, value_arraylen;
 
     keys = g_hash_table_get_keys(cpu->cp_regs);
     keys = g_list_sort(keys, cpreg_key_compare);
 
     cpu->cpreg_array_len = 0;
-
+    cpu->cpreg_value_array_len = 0;
     g_list_foreach(keys, count_cpreg, cpu);
 
     arraylen = cpu->cpreg_array_len;
+    value_arraylen = cpu->cpreg_value_array_len;
     cpu->cpreg_indexes = g_new(uint64_t, arraylen);
-    cpu->cpreg_values = g_new(uint64_t, arraylen);
+    cpu->cpreg_value_indexes = g_new(uint32_t, arraylen);
+    cpu->cpreg_values = g_new(uint64_t, value_arraylen);
     cpu->cpreg_vmstate_indexes = g_new(uint64_t, arraylen);
-    cpu->cpreg_vmstate_values = g_new(uint64_t, arraylen);
+    cpu->cpreg_vmstate_value_indexes = g_new(uint32_t, arraylen);
+    cpu->cpreg_vmstate_values = g_new(uint64_t, value_arraylen);
     cpu->cpreg_vmstate_array_len = cpu->cpreg_array_len;
-    cpu->cpreg_array_len = 0;
+    cpu->cpreg_vmstate_value_array_len = cpu->cpreg_value_array_len;
 
+    cpu->cpreg_array_len = 0;
+    cpu->cpreg_value_array_len = 0;
     g_list_foreach(keys, add_cpreg_to_list, cpu);
 
     assert(cpu->cpreg_array_len == arraylen);
+    assert(cpu->cpreg_value_array_len == value_arraylen);
 
     g_list_free(keys);
 }
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/5] target/arm/hvf: Indirect addressing for coprocessor register storage
  2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
  2022-04-11  6:58 ` [PATCH 1/5] target/arm/tcg: Indirect addressing for coprocessor register storage Gavin Shan
@ 2022-04-11  6:58 ` Gavin Shan
  2022-04-11  6:58 ` [PATCH 3/5] target/arm/kvm: " Gavin Shan
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  6:58 UTC (permalink / raw)
  To: qemu-arm
  Cc: peter.maydell, drjones, richard.henderson, qemu-devel,
	eric.auger, agraf, shan.gavin, pbonzini

Similar to what we did for TCG, this uses @cpreg_value_indexes[] to
track the storage space for the corresponding coprocessor register.
As all coprocessor register have fixed 8 bytes storage space, so
the indirect and direct addressing mechanisms can co-exist and
interchangeable, even in migration circumstance.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/hvf/hvf.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 8c34f86792..8cca26a59c 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -334,6 +334,7 @@ int hvf_get_registers(CPUState *cpu)
     ARMCPU *arm_cpu = ARM_CPU(cpu);
     CPUARMState *env = &arm_cpu->env;
     hv_return_t ret;
+    uint32_t value_index;
     uint64_t val;
     hv_simd_fp_uchar16_t fpval;
     int i;
@@ -373,7 +374,8 @@ int hvf_get_registers(CPUState *cpu)
         ret = hv_vcpu_get_sys_reg(cpu->hvf->fd, hvf_sreg_match[i].reg, &val);
         assert_hvf_ok(ret);
 
-        arm_cpu->cpreg_values[hvf_sreg_match[i].cp_idx] = val;
+        value_index = arm_cpu->cpreg_value_indexes[hvf_sreg_match[i].cp_idx];
+        arm_cpu->cpreg_values[value_index] = val;
     }
     assert(write_list_to_cpustate(arm_cpu));
 
@@ -387,6 +389,7 @@ int hvf_put_registers(CPUState *cpu)
     ARMCPU *arm_cpu = ARM_CPU(cpu);
     CPUARMState *env = &arm_cpu->env;
     hv_return_t ret;
+    uint32_t value_index;
     uint64_t val;
     hv_simd_fp_uchar16_t fpval;
     int i;
@@ -421,7 +424,8 @@ int hvf_put_registers(CPUState *cpu)
             continue;
         }
 
-        val = arm_cpu->cpreg_values[hvf_sreg_match[i].cp_idx];
+        value_index = arm_cpu->cpreg_value_indexes[hvf_sreg_match[i].cp_idx];
+        val = arm_cpu->cpreg_values[value_index];
         ret = hv_vcpu_set_sys_reg(cpu->hvf->fd, hvf_sreg_match[i].reg, val);
         assert_hvf_ok(ret);
     }
@@ -573,12 +577,18 @@ int hvf_arch_init_vcpu(CPUState *cpu)
                                      sregs_match_len);
     arm_cpu->cpreg_values = g_renew(uint64_t, arm_cpu->cpreg_values,
                                     sregs_match_len);
+    arm_cpu->cpreg_value_indexes =
+        g_renew(uint32_t, arm_cpu->cpreg_value_indexes,
+                sregs_match_len);
     arm_cpu->cpreg_vmstate_indexes = g_renew(uint64_t,
                                              arm_cpu->cpreg_vmstate_indexes,
                                              sregs_match_len);
     arm_cpu->cpreg_vmstate_values = g_renew(uint64_t,
                                             arm_cpu->cpreg_vmstate_values,
                                             sregs_match_len);
+    arm_cpu->cpreg_vmstate_value_indexes =
+        g_renew(uint32_t, arm_cpu->cpreg_vmstate_value_indexes,
+                sregs_match_len);
 
     memset(arm_cpu->cpreg_values, 0, sregs_match_len * sizeof(uint64_t));
 
@@ -591,13 +601,17 @@ int hvf_arch_init_vcpu(CPUState *cpu)
         if (ri) {
             assert(!(ri->type & ARM_CP_NO_RAW));
             hvf_sreg_match[i].cp_idx = sregs_cnt;
-            arm_cpu->cpreg_indexes[sregs_cnt++] = cpreg_to_kvm_id(key);
+            arm_cpu->cpreg_indexes[sregs_cnt] = cpreg_to_kvm_id(key);
+            arm_cpu->cpreg_value_indexes[sregs_cnt] = sregs_cnt;
+            sregs_cnt++;
         } else {
             hvf_sreg_match[i].cp_idx = -1;
         }
     }
     arm_cpu->cpreg_array_len = sregs_cnt;
+    arm_cpu->cpreg_value_array_len = sregs_cnt;
     arm_cpu->cpreg_vmstate_array_len = sregs_cnt;
+    arm_cpu->cpreg_vmstate_value_array_len = sregs_cnt;
 
     assert(write_cpustate_to_list(arm_cpu, false));
 
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/5] target/arm/kvm: Indirect addressing for coprocessor register storage
  2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
  2022-04-11  6:58 ` [PATCH 1/5] target/arm/tcg: Indirect addressing for coprocessor register storage Gavin Shan
  2022-04-11  6:58 ` [PATCH 2/5] target/arm/hvf: " Gavin Shan
@ 2022-04-11  6:58 ` Gavin Shan
  2022-04-11  6:58 ` [PATCH 4/5] target/arm: Migrate coprocessor register indirect addressing information Gavin Shan
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  6:58 UTC (permalink / raw)
  To: qemu-arm
  Cc: peter.maydell, drjones, richard.henderson, qemu-devel,
	eric.auger, agraf, shan.gavin, pbonzini

Similar to what we did for TCG, this uses @cpreg_value_indexes[] to
track the storage space for the corresponding coprocessor register.
As all coprocessor register have fixed 8 bytes storage space, so
the indirect and direct addressing mechanisms can co-exist and
interchangeable, even in migration circumstance.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/kvm.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index bbf1ce7ba3..5d1ce431b0 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -429,12 +429,14 @@ static int compare_u64(const void *a, const void *b)
 static uint64_t *kvm_arm_get_cpreg_ptr(ARMCPU *cpu, uint64_t regidx)
 {
     uint64_t *res;
+    uint32_t value_index;
 
     res = bsearch(&regidx, cpu->cpreg_indexes, cpu->cpreg_array_len,
                   sizeof(uint64_t), compare_u64);
     assert(res);
 
-    return &cpu->cpreg_values[res - cpu->cpreg_indexes];
+    value_index = cpu->cpreg_value_indexes[res - cpu->cpreg_indexes];
+    return &cpu->cpreg_values[value_index];
 }
 
 /* Initialize the ARMCPU cpreg list according to the kernel's
@@ -445,7 +447,7 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
 {
     struct kvm_reg_list rl;
     struct kvm_reg_list *rlp;
-    int i, ret, arraylen;
+    int i, ret, arraylen, value_arraylen;
     CPUState *cs = CPU(cpu);
 
     rl.n = 0;
@@ -464,7 +466,7 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
      */
     qsort(&rlp->reg, rlp->n, sizeof(rlp->reg[0]), compare_u64);
 
-    for (i = 0, arraylen = 0; i < rlp->n; i++) {
+    for (i = 0, arraylen = 0, value_arraylen = 0; i < rlp->n; i++) {
         if (!kvm_arm_reg_syncs_via_cpreg_list(rlp->reg[i])) {
             continue;
         }
@@ -479,26 +481,36 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
         }
 
         arraylen++;
+        value_arraylen++;
     }
 
     cpu->cpreg_indexes = g_renew(uint64_t, cpu->cpreg_indexes, arraylen);
-    cpu->cpreg_values = g_renew(uint64_t, cpu->cpreg_values, arraylen);
+    cpu->cpreg_values = g_renew(uint64_t, cpu->cpreg_values, value_arraylen);
+    cpu->cpreg_value_indexes = g_renew(uint32_t, cpu->cpreg_value_indexes,
+                                       arraylen);
     cpu->cpreg_vmstate_indexes = g_renew(uint64_t, cpu->cpreg_vmstate_indexes,
                                          arraylen);
     cpu->cpreg_vmstate_values = g_renew(uint64_t, cpu->cpreg_vmstate_values,
-                                        arraylen);
+                                        value_arraylen);
+    cpu->cpreg_vmstate_value_indexes =
+        g_renew(uint32_t, cpu->cpreg_vmstate_value_indexes, arraylen);
     cpu->cpreg_array_len = arraylen;
+    cpu->cpreg_value_array_len = value_arraylen;
     cpu->cpreg_vmstate_array_len = arraylen;
+    cpu->cpreg_vmstate_value_array_len = value_arraylen;
 
-    for (i = 0, arraylen = 0; i < rlp->n; i++) {
+    for (i = 0, arraylen = 0, value_arraylen = 0; i < rlp->n; i++) {
         uint64_t regidx = rlp->reg[i];
         if (!kvm_arm_reg_syncs_via_cpreg_list(regidx)) {
             continue;
         }
         cpu->cpreg_indexes[arraylen] = regidx;
+        cpu->cpreg_value_indexes[arraylen] = value_arraylen;
         arraylen++;
+        value_arraylen++;
     }
     assert(cpu->cpreg_array_len == arraylen);
+    assert(cpu->cpreg_value_array_len == value_arraylen);
 
     if (!write_kvmstate_to_list(cpu)) {
         /* Shouldn't happen unless kernel is inconsistent about
@@ -533,11 +545,12 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
             r.addr = (uintptr_t)&v32;
             ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
             if (!ret) {
-                cpu->cpreg_values[i] = v32;
+                cpu->cpreg_values[cpu->cpreg_value_indexes[i]] = v32;
             }
             break;
         case KVM_REG_SIZE_U64:
-            r.addr = (uintptr_t)(cpu->cpreg_values + i);
+            r.addr = (uintptr_t)(cpu->cpreg_values +
+                                 cpu->cpreg_value_indexes[i]);
             ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
             break;
         default:
@@ -569,11 +582,12 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
         r.id = regidx;
         switch (regidx & KVM_REG_SIZE_MASK) {
         case KVM_REG_SIZE_U32:
-            v32 = cpu->cpreg_values[i];
+            v32 = cpu->cpreg_values[cpu->cpreg_value_indexes[i]];
             r.addr = (uintptr_t)&v32;
             break;
         case KVM_REG_SIZE_U64:
-            r.addr = (uintptr_t)(cpu->cpreg_values + i);
+            r.addr = (uintptr_t)(cpu->cpreg_values +
+                                 cpu->cpreg_value_indexes[i]);
             break;
         default:
             abort();
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/5] target/arm: Migrate coprocessor register indirect addressing information
  2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
                   ` (2 preceding siblings ...)
  2022-04-11  6:58 ` [PATCH 3/5] target/arm/kvm: " Gavin Shan
@ 2022-04-11  6:58 ` Gavin Shan
  2022-04-11  6:58 ` [PATCH 5/5] target/arm/kvm: Support coprocessor register with variable size Gavin Shan
  2022-04-11  9:22 ` [PATCH 0/5] target/arm: Support variable sized coprocessor registers Peter Maydell
  5 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  6:58 UTC (permalink / raw)
  To: qemu-arm
  Cc: peter.maydell, drjones, richard.henderson, qemu-devel,
	eric.auger, agraf, shan.gavin, pbonzini

This migrates @cpreg_value_array_len and @cpreg_value_indexes, which
are used to indirectly addressing the storage space for the corresponding
coprocessor register.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/machine.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/target/arm/machine.c b/target/arm/machine.c
index 135d2420b5..ce6f2599d8 100644
--- a/target/arm/machine.c
+++ b/target/arm/machine.c
@@ -677,10 +677,13 @@ static int cpu_pre_save(void *opaque)
     }
 
     cpu->cpreg_vmstate_array_len = cpu->cpreg_array_len;
+    cpu->cpreg_vmstate_value_array_len = cpu->cpreg_value_array_len;
     memcpy(cpu->cpreg_vmstate_indexes, cpu->cpreg_indexes,
            cpu->cpreg_array_len * sizeof(uint64_t));
     memcpy(cpu->cpreg_vmstate_values, cpu->cpreg_values,
-           cpu->cpreg_array_len * sizeof(uint64_t));
+           cpu->cpreg_value_array_len * sizeof(uint64_t));
+    memcpy(cpu->cpreg_vmstate_value_indexes, cpu->cpreg_value_indexes,
+           cpu->cpreg_array_len * sizeof(uint32_t));
 
     return 0;
 }
@@ -719,7 +722,7 @@ static int cpu_post_load(void *opaque, int version_id)
 {
     ARMCPU *cpu = opaque;
     CPUARMState *env = &cpu->env;
-    int i, v;
+    int i, v, n;
 
     /*
      * Handle migration compatibility from old QEMU which didn't
@@ -757,8 +760,19 @@ static int cpu_post_load(void *opaque, int version_id)
             /* register in their list but not ours: fail migration */
             return -1;
         }
+
         /* matching register, copy the value over */
-        cpu->cpreg_values[i] = cpu->cpreg_vmstate_values[v];
+        if (v < cpu->cpreg_vmstate_array_len - 1) {
+            n = cpu->cpreg_vmstate_value_indexes[v + 1] -
+                cpu->cpreg_vmstate_value_indexes[v];
+        } else {
+            n = cpu->cpreg_vmstate_value_array_len -
+                cpu->cpreg_vmstate_value_indexes[v];
+        }
+
+        memcpy(&cpu->cpreg_values[cpu->cpreg_value_indexes[i]],
+               &cpu->cpreg_vmstate_values[cpu->cpreg_vmstate_value_indexes[v]],
+               n * sizeof(uint64_t));
         v++;
     }
 
@@ -814,8 +828,8 @@ static int cpu_post_load(void *opaque, int version_id)
 
 const VMStateDescription vmstate_arm_cpu = {
     .name = "cpu",
-    .version_id = 22,
-    .minimum_version_id = 22,
+    .version_id = 23,
+    .minimum_version_id = 23,
     .pre_save = cpu_pre_save,
     .post_save = cpu_post_save,
     .pre_load = cpu_pre_load,
@@ -844,12 +858,16 @@ const VMStateDescription vmstate_arm_cpu = {
          * incoming data possibly overflowing the array.
          */
         VMSTATE_INT32_POSITIVE_LE(cpreg_vmstate_array_len, ARMCPU),
+        VMSTATE_INT32_POSITIVE_LE(cpreg_vmstate_value_array_len, ARMCPU),
         VMSTATE_VARRAY_INT32(cpreg_vmstate_indexes, ARMCPU,
                              cpreg_vmstate_array_len,
                              0, vmstate_info_uint64, uint64_t),
         VMSTATE_VARRAY_INT32(cpreg_vmstate_values, ARMCPU,
-                             cpreg_vmstate_array_len,
+                             cpreg_vmstate_value_array_len,
                              0, vmstate_info_uint64, uint64_t),
+        VMSTATE_VARRAY_INT32(cpreg_vmstate_value_indexes, ARMCPU,
+                             cpreg_vmstate_array_len,
+                             0, vmstate_info_uint32, uint32_t),
         VMSTATE_UINT64(env.exclusive_addr, ARMCPU),
         VMSTATE_UINT64(env.exclusive_val, ARMCPU),
         VMSTATE_UINT64(env.exclusive_high, ARMCPU),
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/5] target/arm/kvm: Support coprocessor register with variable size
  2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
                   ` (3 preceding siblings ...)
  2022-04-11  6:58 ` [PATCH 4/5] target/arm: Migrate coprocessor register indirect addressing information Gavin Shan
@ 2022-04-11  6:58 ` Gavin Shan
  2022-04-11  9:22 ` [PATCH 0/5] target/arm: Support variable sized coprocessor registers Peter Maydell
  5 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  6:58 UTC (permalink / raw)
  To: qemu-arm
  Cc: peter.maydell, drjones, richard.henderson, qemu-devel,
	eric.auger, agraf, shan.gavin, pbonzini

With the indirect addressing mechanism, we can support large-sized
coprocessor registers. This calculates the length of @cpreg_values
and allocate the storage space accordingly. @cpreg_value_indexes
is also initialized according to the coprocessor register size.

For those registers whose sizes are less than 8 bytes, we always
reserve 8 bytes storage space for it, to gurantee the storage
space address is aligned to 8 bytes. On the other hand, the storage
space size for other registers are allocated based on their sizes.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/kvm.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 5d1ce431b0..71f602fd81 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -471,8 +471,18 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
             continue;
         }
         switch (rlp->reg[i] & KVM_REG_SIZE_MASK) {
+        case KVM_REG_SIZE_U8:
+        case KVM_REG_SIZE_U16:
         case KVM_REG_SIZE_U32:
+            value_arraylen++;
+            break;
         case KVM_REG_SIZE_U64:
+        case KVM_REG_SIZE_U128:
+        case KVM_REG_SIZE_U256:
+        case KVM_REG_SIZE_U512:
+        case KVM_REG_SIZE_U1024:
+        case KVM_REG_SIZE_U2048:
+            value_arraylen += KVM_REG_SIZE(rlp->reg[i]) / sizeof(uint64_t);
             break;
         default:
             fprintf(stderr, "Can't handle size of register in kernel list\n");
@@ -481,7 +491,6 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
         }
 
         arraylen++;
-        value_arraylen++;
     }
 
     cpu->cpreg_indexes = g_renew(uint64_t, cpu->cpreg_indexes, arraylen);
@@ -507,7 +516,13 @@ int kvm_arm_init_cpreg_list(ARMCPU *cpu)
         cpu->cpreg_indexes[arraylen] = regidx;
         cpu->cpreg_value_indexes[arraylen] = value_arraylen;
         arraylen++;
-        value_arraylen++;
+
+        /* We validate the size just now and no need to do it again */
+        if (KVM_REG_SIZE(rlp->reg[i]) < sizeof(uint64_t)) {
+            value_arraylen++;
+        } else {
+            value_arraylen += KVM_REG_SIZE(rlp->reg[i]) / sizeof(uint64_t);
+        }
     }
     assert(cpu->cpreg_array_len == arraylen);
     assert(cpu->cpreg_value_array_len == value_arraylen);
@@ -535,12 +550,28 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         struct kvm_one_reg r;
         uint64_t regidx = cpu->cpreg_indexes[i];
+        uint8_t v8;
+        uint16_t v16;
         uint32_t v32;
         int ret;
 
         r.id = regidx;
 
         switch (regidx & KVM_REG_SIZE_MASK) {
+        case KVM_REG_SIZE_U8:
+            r.addr = (uintptr_t)&v8;
+            ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
+            if (!ret) {
+                cpu->cpreg_values[cpu->cpreg_value_indexes[i]] = v8;
+            }
+            break;
+        case KVM_REG_SIZE_U16:
+            r.addr = (uintptr_t)&v16;
+            ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
+            if (!ret) {
+                cpu->cpreg_values[cpu->cpreg_value_indexes[i]] = v16;
+            }
+            break;
         case KVM_REG_SIZE_U32:
             r.addr = (uintptr_t)&v32;
             ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
@@ -549,6 +580,11 @@ bool write_kvmstate_to_list(ARMCPU *cpu)
             }
             break;
         case KVM_REG_SIZE_U64:
+        case KVM_REG_SIZE_U128:
+        case KVM_REG_SIZE_U256:
+        case KVM_REG_SIZE_U512:
+        case KVM_REG_SIZE_U1024:
+        case KVM_REG_SIZE_U2048:
             r.addr = (uintptr_t)(cpu->cpreg_values +
                                  cpu->cpreg_value_indexes[i]);
             ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &r);
@@ -572,6 +608,8 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
     for (i = 0; i < cpu->cpreg_array_len; i++) {
         struct kvm_one_reg r;
         uint64_t regidx = cpu->cpreg_indexes[i];
+        uint8_t v8;
+        uint16_t v16;
         uint32_t v32;
         int ret;
 
@@ -581,11 +619,24 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
 
         r.id = regidx;
         switch (regidx & KVM_REG_SIZE_MASK) {
+        case KVM_REG_SIZE_U8:
+            v8 = cpu->cpreg_values[cpu->cpreg_value_indexes[i]];
+            r.addr = (uintptr_t)&v8;
+            break;
+        case KVM_REG_SIZE_U16:
+            v16 = cpu->cpreg_values[cpu->cpreg_value_indexes[i]];
+            r.addr = (uintptr_t)&v16;
+            break;
         case KVM_REG_SIZE_U32:
             v32 = cpu->cpreg_values[cpu->cpreg_value_indexes[i]];
             r.addr = (uintptr_t)&v32;
             break;
         case KVM_REG_SIZE_U64:
+        case KVM_REG_SIZE_U128:
+        case KVM_REG_SIZE_U256:
+        case KVM_REG_SIZE_U512:
+        case KVM_REG_SIZE_U1024:
+        case KVM_REG_SIZE_U2048:
             r.addr = (uintptr_t)(cpu->cpreg_values +
                                  cpu->cpreg_value_indexes[i]);
             break;
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
                   ` (4 preceding siblings ...)
  2022-04-11  6:58 ` [PATCH 5/5] target/arm/kvm: Support coprocessor register with variable size Gavin Shan
@ 2022-04-11  9:22 ` Peter Maydell
  2022-04-11  9:49   ` Gavin Shan
  2022-04-11 12:02   ` Andrew Jones
  5 siblings, 2 replies; 14+ messages in thread
From: Peter Maydell @ 2022-04-11  9:22 UTC (permalink / raw)
  To: Gavin Shan
  Cc: drjones, agraf, richard.henderson, qemu-devel, eric.auger,
	qemu-arm, shan.gavin, pbonzini

On Mon, 11 Apr 2022 at 07:59, Gavin Shan <gshan@redhat.com> wrote:
>
> There are two arrays for each CPU, to store the indexes and values of the
> coprocessor registers. Currently, 8 bytes fixed storage space is reserved
> for each coprocessor register. However, larger coprocessor registers have
> been defined and exposed by KVM. Except SVE registers, no coprocessor
> register exceeds 8 bytes in size. It doesn't mean large coprocessor registers
> won't be exploited in future. For example, I'm looking into SDEI virtualization
> support, which isn't merged into Linux upstream yet. I have plan to add
> several coprocessor ("firmware pseudo") registers to assist the migration.

So, can you give an example of coprocessor registers which are
not 8 bytes in size? How are they accessed by the guest?
If we need to support them then we need to support them, but this
cover letter/series doesn't seem to me to provide enough detail
to make the case that they really are necessary.

Also, we support SVE today, and we don't have variable size
coprocessor registers. Is there a bug here that we would be
fixing ?

thanks
-- PMM


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11  9:22 ` [PATCH 0/5] target/arm: Support variable sized coprocessor registers Peter Maydell
@ 2022-04-11  9:49   ` Gavin Shan
  2022-04-11 10:05     ` Peter Maydell
  2022-04-11 12:02   ` Andrew Jones
  1 sibling, 1 reply; 14+ messages in thread
From: Gavin Shan @ 2022-04-11  9:49 UTC (permalink / raw)
  To: Peter Maydell
  Cc: drjones, agraf, Oliver Upton, richard.henderson, qemu-devel,
	eric.auger, qemu-arm, shan.gavin, pbonzini

Hi Peter,

On 4/11/22 5:22 PM, Peter Maydell wrote:
> On Mon, 11 Apr 2022 at 07:59, Gavin Shan <gshan@redhat.com> wrote:
>>
>> There are two arrays for each CPU, to store the indexes and values of the
>> coprocessor registers. Currently, 8 bytes fixed storage space is reserved
>> for each coprocessor register. However, larger coprocessor registers have
>> been defined and exposed by KVM. Except SVE registers, no coprocessor
>> register exceeds 8 bytes in size. It doesn't mean large coprocessor registers
>> won't be exploited in future. For example, I'm looking into SDEI virtualization
>> support, which isn't merged into Linux upstream yet. I have plan to add
>> several coprocessor ("firmware pseudo") registers to assist the migration.
> 
> So, can you give an example of coprocessor registers which are
> not 8 bytes in size? How are they accessed by the guest?
> If we need to support them then we need to support them, but this
> cover letter/series doesn't seem to me to provide enough detail
> to make the case that they really are necessary.
> 
> Also, we support SVE today, and we don't have variable size
> coprocessor registers. Is there a bug here that we would be
> fixing ?
> 

[Cc Oliver Upon]

Apart from SVE registers, I don't think we have any more large registers
whose sizes exceed 8 bytes for now, until SDEI virtualization needs more
large registers for migration.

I'm working the KVM series to support SDEI virtualization and last revision
is v6. One of the requirement is to migrate the SDEI events and states.
In v5, the migration is done by the dedicated ioctl commands and it was
suggested by Oliver to use {GET, SET}_ONE_REG. Note that the series isn't
merged yet. So I had the prototype to support SDEI's migration through
{GET, SET}_ONE_REG. Note that those newly added registers are inaccessible
from guest.

https://github.com/gwshan/linux/commit/c2e5de5e210de6b003d1e1330eeb0958cf7007f5
(branch: kvm/arm64_sdei)

https://lore.kernel.org/lkml/20220403153911.12332-13-gshan@redhat.com/T/   (last revision: v6)
https://lore.kernel.org/kvmarm/YjtYuk+Jx1dFPQQ9@google.com/                (v5)

There are large coprocessor register sizes, like U2048, exposed by KVM.
However, it seems we never support those large coprocessor registers.
I'm not sure if we have any challenges to support those large registers,
or we don't have the needs yet?

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11  9:49   ` Gavin Shan
@ 2022-04-11 10:05     ` Peter Maydell
  2022-04-12  1:54       ` Gavin Shan
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2022-04-11 10:05 UTC (permalink / raw)
  To: Gavin Shan
  Cc: drjones, agraf, Oliver Upton, richard.henderson, qemu-devel,
	eric.auger, qemu-arm, shan.gavin, pbonzini

On Mon, 11 Apr 2022 at 10:50, Gavin Shan <gshan@redhat.com> wrote:
> On 4/11/22 5:22 PM, Peter Maydell wrote:
> > So, can you give an example of coprocessor registers which are
> > not 8 bytes in size? How are they accessed by the guest?
> > If we need to support them then we need to support them, but this
> > cover letter/series doesn't seem to me to provide enough detail
> > to make the case that they really are necessary.
> >
> > Also, we support SVE today, and we don't have variable size
> > coprocessor registers. Is there a bug here that we would be
> > fixing ?
> >
>
> [Cc Oliver Upon]
>
> Apart from SVE registers, I don't think we have any more large registers
> whose sizes exceed 8 bytes for now, until SDEI virtualization needs more
> large registers for migration.
>
> I'm working the KVM series to support SDEI virtualization and last revision
> is v6. One of the requirement is to migrate the SDEI events and states.
> In v5, the migration is done by the dedicated ioctl commands and it was
> suggested by Oliver to use {GET, SET}_ONE_REG. Note that the series isn't
> merged yet. So I had the prototype to support SDEI's migration through
> {GET, SET}_ONE_REG. Note that those newly added registers are inaccessible
> from guest.
>
> https://github.com/gwshan/linux/commit/c2e5de5e210de6b003d1e1330eeb0958cf7007f5
> (branch: kvm/arm64_sdei)
>
> https://lore.kernel.org/lkml/20220403153911.12332-13-gshan@redhat.com/T/   (last revision: v6)
> https://lore.kernel.org/kvmarm/YjtYuk+Jx1dFPQQ9@google.com/                (v5)

Could you please describe what you're trying to do here rather
than asking me to wade through a big kernel patchset that's
adding support for a firmware interface I don't know?

> There are large coprocessor register sizes, like U2048, exposed by KVM.
> However, it seems we never support those large coprocessor registers.
> I'm not sure if we have any challenges to support those large registers,
> or we don't have the needs yet?

The general idea of the coprocessor register accessors for aarch64 KVM
is that we're giving the VMM access to the same registers that the guest
accesses via the msr/mrs instructions. Those instructions by definition
access 64 bit quantities. In a few places we've borrowed this mechanism
to define KVM-specific pseudo-registers, but that wasn't the primary
design intent. So maybe it makes sense to extend it to do what you're
trying to, or maybe that would be the tail wagging the dog. It's hard
to tell without more detail on what exactly you're trying to expose
to the VMM here.

(The ONE_REG API is used by more than just aarch64 and more than just
for coprocessor registers, which is why it supports lots of different
sizes.)

thanks
-- PMM


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11  9:22 ` [PATCH 0/5] target/arm: Support variable sized coprocessor registers Peter Maydell
  2022-04-11  9:49   ` Gavin Shan
@ 2022-04-11 12:02   ` Andrew Jones
  2022-04-11 12:10     ` Peter Maydell
  2022-04-12  2:08     ` Gavin Shan
  1 sibling, 2 replies; 14+ messages in thread
From: Andrew Jones @ 2022-04-11 12:02 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Gavin Shan, agraf, richard.henderson, qemu-devel, eric.auger,
	qemu-arm, shan.gavin, pbonzini

On Mon, Apr 11, 2022 at 10:22:59AM +0100, Peter Maydell wrote:
> On Mon, 11 Apr 2022 at 07:59, Gavin Shan <gshan@redhat.com> wrote:
> >
> > There are two arrays for each CPU, to store the indexes and values of the
> > coprocessor registers. Currently, 8 bytes fixed storage space is reserved
> > for each coprocessor register. However, larger coprocessor registers have
> > been defined and exposed by KVM. Except SVE registers, no coprocessor
> > register exceeds 8 bytes in size. It doesn't mean large coprocessor registers
> > won't be exploited in future. For example, I'm looking into SDEI virtualization
> > support, which isn't merged into Linux upstream yet. I have plan to add
> > several coprocessor ("firmware pseudo") registers to assist the migration.
> 
> So, can you give an example of coprocessor registers which are
> not 8 bytes in size? How are they accessed by the guest?
> If we need to support them then we need to support them, but this
> cover letter/series doesn't seem to me to provide enough detail
> to make the case that they really are necessary.
> 
> Also, we support SVE today, and we don't have variable size
> coprocessor registers. Is there a bug here that we would be
> fixing ?

SVE registers are KVM_REG_SIZE_U2048 and KVM_REG_SIZE_U256 sized
registers. They work fine (just like the VFP registers which are
KVM_REG_SIZE_U128 sized). They work because they don't get stored in the
cpreg list. SVE and CORE (which includes VFP) registers are filtered
out by kvm_arm_reg_syncs_via_cpreg_list(). Since they're filtered
out they need to be handled specifically by kvm_arch_get/put_registers()

I asked Gavin to check if following the SVE pattern made sense for his
use case prior to sending this series, but I don't see the rationale for
not following the SVE pattern in this cover letter. Gavin, can you please
explain why following the SVE pattern doesn't work? Or, are you trying to
avoid adding an additional special case?

Thanks,
drew



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11 12:02   ` Andrew Jones
@ 2022-04-11 12:10     ` Peter Maydell
  2022-04-13  2:55       ` Gavin Shan
  2022-04-12  2:08     ` Gavin Shan
  1 sibling, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2022-04-11 12:10 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Gavin Shan, agraf, richard.henderson, qemu-devel, eric.auger,
	qemu-arm, shan.gavin, pbonzini

On Mon, 11 Apr 2022 at 13:02, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Apr 11, 2022 at 10:22:59AM +0100, Peter Maydell wrote:
> > Also, we support SVE today, and we don't have variable size
> > coprocessor registers. Is there a bug here that we would be
> > fixing ?
>
> SVE registers are KVM_REG_SIZE_U2048 and KVM_REG_SIZE_U256 sized
> registers. They work fine (just like the VFP registers which are
> KVM_REG_SIZE_U128 sized). They work because they don't get stored in the
> cpreg list. SVE and CORE (which includes VFP) registers are filtered
> out by kvm_arm_reg_syncs_via_cpreg_list(). Since they're filtered
> out they need to be handled specifically by kvm_arch_get/put_registers()

Right, this is the distinction between ONE_REG registers and
coprocessor registers (which are a subset of ONE_REG registers).
We wouldn't want to handle SVE regs in the copro list anyway,
I think, because we want their state to end up in env->vfp.zregs[]
so the gdbstub can find it there. And we wouldn't have benefited
from the copro regs handling's "no need for new QEMU to handle
migrating state of a new register" because we needed a lot of
special case code for SVE and couldn't enable it by default
for other reasons.

If we do add non-64-bit cpregs on the kernel side then we need to
make those new registers opt-in, because currently deployed QEMU
will refuse to start if the kernel passes it a register in the
KVM_GET_REG_LIST that is larger than 64 bits and isn't
KVM_REG_ARM_CORE or KVM_REG_ARM64_SVE (assuming I'm not misreading
the QEMU code).

-- PMM


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11 10:05     ` Peter Maydell
@ 2022-04-12  1:54       ` Gavin Shan
  0 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-12  1:54 UTC (permalink / raw)
  To: Peter Maydell
  Cc: drjones, agraf, Oliver Upton, richard.henderson, qemu-devel,
	eric.auger, qemu-arm, shan.gavin, pbonzini

Hi Peter,

On 4/11/22 6:05 PM, Peter Maydell wrote:
> On Mon, 11 Apr 2022 at 10:50, Gavin Shan <gshan@redhat.com> wrote:
>> On 4/11/22 5:22 PM, Peter Maydell wrote:
>>> So, can you give an example of coprocessor registers which are
>>> not 8 bytes in size? How are they accessed by the guest?
>>> If we need to support them then we need to support them, but this
>>> cover letter/series doesn't seem to me to provide enough detail
>>> to make the case that they really are necessary.
>>>
>>> Also, we support SVE today, and we don't have variable size
>>> coprocessor registers. Is there a bug here that we would be
>>> fixing ?
>>>
>>
>> [Cc Oliver Upon]
>>
>> Apart from SVE registers, I don't think we have any more large registers
>> whose sizes exceed 8 bytes for now, until SDEI virtualization needs more
>> large registers for migration.
>>
>> I'm working the KVM series to support SDEI virtualization and last revision
>> is v6. One of the requirement is to migrate the SDEI events and states.
>> In v5, the migration is done by the dedicated ioctl commands and it was
>> suggested by Oliver to use {GET, SET}_ONE_REG. Note that the series isn't
>> merged yet. So I had the prototype to support SDEI's migration through
>> {GET, SET}_ONE_REG. Note that those newly added registers are inaccessible
>> from guest.
>>
>> https://github.com/gwshan/linux/commit/c2e5de5e210de6b003d1e1330eeb0958cf7007f5
>> (branch: kvm/arm64_sdei)
>>
>> https://lore.kernel.org/lkml/20220403153911.12332-13-gshan@redhat.com/T/   (last revision: v6)
>> https://lore.kernel.org/kvmarm/YjtYuk+Jx1dFPQQ9@google.com/                (v5)
> 
> Could you please describe what you're trying to do here rather
> than asking me to wade through a big kernel patchset that's
> adding support for a firmware interface I don't know?
> 

I'm working on two serieses to support SDEI virtualization and asynchronous
page fault (Async PF) on arm64. When currently running thread is experiencing
stage-2 page fault, Async PF picks another thread for execution. After the
stage-2 page fault is resolved, that thread resumes. Async PF is driven by
notifications sent from host to guest. One of the notifications is delivered
by SDEI event. The SDEI event is something like NMI to some extent. When
the SDEI event is raised by host, it will be delivered to guest and the
previously registered (associated) event handler is invoked in guest.

One of the concerns for SDEI virtualization is how those SDEI events
and states can be migrated. Previously, I had dedicated ioctl commands
to read/write the SDEI events and states. Later, Oliver suggested to
use {GET,SET}_ONE_REG to do migration. I think it's much better than
the dedicated ioctl commands in terms of maintenance cost. In this case,
several 'firmware pseudo registers', like what PSCI is doing, will be
added and some of their sizes will exceed 8 bytes. On the other hand,
the SDEI events and states can be naturally treated as CPU's properties.
It's another reason why pseudo-registers fits the need to migrate
those SDEI events and states.

More information about SDEI can be found from its spec:

https://developer.arm.com/documentation/den0054/latest

>> There are large coprocessor register sizes, like U2048, exposed by KVM.
>> However, it seems we never support those large coprocessor registers.
>> I'm not sure if we have any challenges to support those large registers,
>> or we don't have the needs yet?
> 
> The general idea of the coprocessor register accessors for aarch64 KVM
> is that we're giving the VMM access to the same registers that the guest
> accesses via the msr/mrs instructions. Those instructions by definition
> access 64 bit quantities. In a few places we've borrowed this mechanism
> to define KVM-specific pseudo-registers, but that wasn't the primary
> design intent. So maybe it makes sense to extend it to do what you're
> trying to, or maybe that would be the tail wagging the dog. It's hard
> to tell without more detail on what exactly you're trying to expose
> to the VMM here.
> 
> (The ONE_REG API is used by more than just aarch64 and more than just
> for coprocessor registers, which is why it supports lots of different
> sizes.)
> 

Yes, we're considering 'KVM specific pseudo-registers' for migrating
SDEI events and states. Those pseudo-registers shouldn't be accessed
from guest, meaning they're only accessed by VMM (QEMU). I think
pseudo-registers fits the need very well, to migrate SDEI events
and states.

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11 12:02   ` Andrew Jones
  2022-04-11 12:10     ` Peter Maydell
@ 2022-04-12  2:08     ` Gavin Shan
  1 sibling, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-12  2:08 UTC (permalink / raw)
  To: Andrew Jones, Peter Maydell
  Cc: agraf, richard.henderson, qemu-devel, eric.auger, qemu-arm,
	shan.gavin, pbonzini

Hi Drew,

On 4/11/22 8:02 PM, Andrew Jones wrote:
> On Mon, Apr 11, 2022 at 10:22:59AM +0100, Peter Maydell wrote:
>> On Mon, 11 Apr 2022 at 07:59, Gavin Shan <gshan@redhat.com> wrote:
>>>
>>> There are two arrays for each CPU, to store the indexes and values of the
>>> coprocessor registers. Currently, 8 bytes fixed storage space is reserved
>>> for each coprocessor register. However, larger coprocessor registers have
>>> been defined and exposed by KVM. Except SVE registers, no coprocessor
>>> register exceeds 8 bytes in size. It doesn't mean large coprocessor registers
>>> won't be exploited in future. For example, I'm looking into SDEI virtualization
>>> support, which isn't merged into Linux upstream yet. I have plan to add
>>> several coprocessor ("firmware pseudo") registers to assist the migration.
>>
>> So, can you give an example of coprocessor registers which are
>> not 8 bytes in size? How are they accessed by the guest?
>> If we need to support them then we need to support them, but this
>> cover letter/series doesn't seem to me to provide enough detail
>> to make the case that they really are necessary.
>>
>> Also, we support SVE today, and we don't have variable size
>> coprocessor registers. Is there a bug here that we would be
>> fixing ?
> 
> SVE registers are KVM_REG_SIZE_U2048 and KVM_REG_SIZE_U256 sized
> registers. They work fine (just like the VFP registers which are
> KVM_REG_SIZE_U128 sized). They work because they don't get stored in the
> cpreg list. SVE and CORE (which includes VFP) registers are filtered
> out by kvm_arm_reg_syncs_via_cpreg_list(). Since they're filtered
> out they need to be handled specifically by kvm_arch_get/put_registers()
> 
> I asked Gavin to check if following the SVE pattern made sense for his
> use case prior to sending this series, but I don't see the rationale for
> not following the SVE pattern in this cover letter. Gavin, can you please
> explain why following the SVE pattern doesn't work? Or, are you trying to
> avoid adding an additional special case?
> 

Yes, SVE registers are special case. They're not synchronized through
coprocessor register list as you mentioned. For SDEI, we mimic PSCI
because both of them are firmware interfaces.

PSCI's pseudo-registers are synchronized and migrated through the
coprocessor register list. Besides, treating SDEI as additional
special case should work, but more maintaining load will be introduced.
We need separate functions to get/set SDEI's pseudo registers, like
what we did for SVE.

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/5] target/arm: Support variable sized coprocessor registers
  2022-04-11 12:10     ` Peter Maydell
@ 2022-04-13  2:55       ` Gavin Shan
  0 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2022-04-13  2:55 UTC (permalink / raw)
  To: Peter Maydell, Andrew Jones
  Cc: agraf, Oliver Upton, richard.henderson, qemu-devel, eric.auger,
	qemu-arm, shan.gavin, Raghavendra Rao Ananta, pbonzini

Hi Peter,

On 4/11/22 8:10 PM, Peter Maydell wrote:
> On Mon, 11 Apr 2022 at 13:02, Andrew Jones <drjones@redhat.com> wrote:
>> On Mon, Apr 11, 2022 at 10:22:59AM +0100, Peter Maydell wrote:
>>> Also, we support SVE today, and we don't have variable size
>>> coprocessor registers. Is there a bug here that we would be
>>> fixing ?
>>
>> SVE registers are KVM_REG_SIZE_U2048 and KVM_REG_SIZE_U256 sized
>> registers. They work fine (just like the VFP registers which are
>> KVM_REG_SIZE_U128 sized). They work because they don't get stored in the
>> cpreg list. SVE and CORE (which includes VFP) registers are filtered
>> out by kvm_arm_reg_syncs_via_cpreg_list(). Since they're filtered
>> out they need to be handled specifically by kvm_arch_get/put_registers()
> 
> Right, this is the distinction between ONE_REG registers and
> coprocessor registers (which are a subset of ONE_REG registers).
> We wouldn't want to handle SVE regs in the copro list anyway,
> I think, because we want their state to end up in env->vfp.zregs[]
> so the gdbstub can find it there. And we wouldn't have benefited
> from the copro regs handling's "no need for new QEMU to handle
> migrating state of a new register" because we needed a lot of
> special case code for SVE and couldn't enable it by default
> for other reasons.
> 

Yep, those new introduced SDEI pseudo-registers, the intention
is to avoid the special case code. So the coprocessor register
list fits the need well. The only barrier to use the coprocessor
register list is the variable register sizes.

> If we do add non-64-bit cpregs on the kernel side then we need to
> make those new registers opt-in, because currently deployed QEMU
> will refuse to start if the kernel passes it a register in the
> KVM_GET_REG_LIST that is larger than 64 bits and isn't
> KVM_REG_ARM_CORE or KVM_REG_ARM64_SVE (assuming I'm not misreading
> the QEMU code).
> 

Yes, we need make those new registers opt-in absolutely. Otherwise,
the old qemu, which doesn't have variable sized registers supported,
will crash on host kernel, where large sized registers are exposed
unconditionally.

I spent some time to think of the mechanisms for opt-in. There are
two options as I can figure out: (1) Using KVM_CAP_ARM_SDEI to check
if the large sized registers exist. (2) Using newly introduced
pseudo-register ("KVM_REG_ARM_STD_BMAP") in Raghavendra's series [1].
The individual bit in this pseudo-register corresponds to one
service in "standard hypervisor" category, where SDEI falls into.

I prefer (2) because those services or firmware interfaces are
exposed in a collective way by KVM_REG_ARM_STD_BMAP, comparing
to the individual capabilities. However, they are same in essence.
Another benefit to use KVM_REG_ARM_STD_BMAP is hiding SDEI interface
and the large sized registers for old QEMU.

[1] https://lore.kernel.org/linux-arm-kernel/20220407011605.1966778-10-rananta@google.com/T/#m0bc1aa4048ca157e8e99c593b3f349b879032543

Thanks,
Gavin



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2022-04-13  2:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-11  6:58 [PATCH 0/5] target/arm: Support variable sized coprocessor registers Gavin Shan
2022-04-11  6:58 ` [PATCH 1/5] target/arm/tcg: Indirect addressing for coprocessor register storage Gavin Shan
2022-04-11  6:58 ` [PATCH 2/5] target/arm/hvf: " Gavin Shan
2022-04-11  6:58 ` [PATCH 3/5] target/arm/kvm: " Gavin Shan
2022-04-11  6:58 ` [PATCH 4/5] target/arm: Migrate coprocessor register indirect addressing information Gavin Shan
2022-04-11  6:58 ` [PATCH 5/5] target/arm/kvm: Support coprocessor register with variable size Gavin Shan
2022-04-11  9:22 ` [PATCH 0/5] target/arm: Support variable sized coprocessor registers Peter Maydell
2022-04-11  9:49   ` Gavin Shan
2022-04-11 10:05     ` Peter Maydell
2022-04-12  1:54       ` Gavin Shan
2022-04-11 12:02   ` Andrew Jones
2022-04-11 12:10     ` Peter Maydell
2022-04-13  2:55       ` Gavin Shan
2022-04-12  2:08     ` Gavin Shan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.